Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Qemu adn UEFI Capsule Update support #26

Merged
merged 58 commits into from
Nov 20, 2024
Merged

Add Qemu adn UEFI Capsule Update support #26

merged 58 commits into from
Nov 20, 2024

Conversation

DaniilKl
Copy link
Contributor

This PR adds support for Qemu so to be able to test DTS and its features more easily. Additionally, this PR adds support for Dasharo firmware updates provided via UEFI capsules.

@DaniilKl DaniilKl self-assigned this Aug 19, 2024
@DaniilKl
Copy link
Contributor Author

DaniilKl commented Aug 21, 2024

@macpijan, thought it is not a part of this PR, I have a question about EC firmware installation process.

Currently in scripts/dasharo-deploy, in function install we have a code, which installs EC firmware:

(...)
  if [ "$HAVE_EC" == "true" ]; then
    echo "Checking for Open Source Embedded Controller firmware"
    $DASHARO_ECTOOL info >> $ERR_LOG_FILE 2>&1
    if [ $? -eq 0 ]; then
      echo "Device has already Open Source Embedded Controller firmware, do not flash EC..."
    else
      _ec_fw_version=$($FLASHROM -p "$PROGRAMMER_EC" ${FLASH_CHIP_SELECT} | grep "Mainboard EC Version" | tr -d ' ' | cut -d ':' -f 2)
      if [ "$_ec_fw_version" != "$COMPATIBLE_EC_FW_VERSION" ]; then
        print_warning "EC version: $_ec_fw_version is not supported, update required"
        install_ec
      fi
    fi
  fi
(...)

IIUC, we want EC firmware version to match COMPATIBLE_EC_FW_VERSION which is an EC firmware version needed by Dasharo firmware. If it so, why we skip checking version of the EC firmware in case Device has already Open Source Embedded Controller firmware, do not flash EC... ? If a device already has some "Open Source Embedded Controller firmware", it does not mean it has the right version.

@macpijan
Copy link
Contributor

If it so, why we skip checking version of the EC firmware in case Device has already Open Source Embedded Controller firmware, do not flash EC... ? If a device already has some "Open Source Embedded Controller firmware", it does not mean it has the right version.

If a device already has open-source EC, then it most likely already has Dasharo as well, and the install function would not be called at all, as this is for initial deployment mostly, IIRC?

@DaniilKl
Copy link
Contributor Author

DaniilKl commented Aug 22, 2024

as this is for initial deployment mostly, IIRC?

It is.

If a device already has open-source EC, then it most likely already has Dasharo as well, and the install function would not be called at all

Ok then, but nobody stops a user with Dasharo installed to choose install Dasharo to install another version (e.g. intentionally downgrade) that will lead him to not installing proper version of EC firmware, I cannot find any code which will restrict such case.

@macpijan
Copy link
Contributor

macpijan commented Aug 22, 2024

Ok then, but nobody stops a user with Dasharo installed to choose install Dasharo to install another version (e.g. intentionally downgrade) that will lead him to not installing proper version of EC firmware, I cannot find any code which will restrict such case.

If Dasharo is displayed, Install Dasharo option is not visible in the menu. Only option to update Dasahro should be visible at this point.

@DaniilKl
Copy link
Contributor Author

If Dasharo is displayed, Install Dasharo option is not visible in the menu. Only option to update Dasahro should be visible at this point.

My bad, have forgotten about it.

@DaniilKl DaniilKl force-pushed the add-qemu-support branch 14 times, most recently from 4c31e38 to 45c05be Compare September 30, 2024 11:08
@DaniilKl DaniilKl force-pushed the add-qemu-support branch 4 times, most recently from 6327fbb to 761536a Compare October 10, 2024 13:34
This warning serves as a checkpoint in tests in Dasharo/OSFV repo, it
was unconvenient to have different warnings all other dasharo-deploy
script, so this commit brings it to one format.

Signed-off-by: Daniil Klimuk <[email protected]>
Reviewed-by: Michał Iwanicki <[email protected]>
Signed-off-by: Daniil Klimuk <[email protected]>
Reviewed-by: Michał Iwanicki <[email protected]>
Signed-off-by: Daniil Klimuk <[email protected]>
Reviewed-by: Michał Iwanicki <[email protected]>
Signed-off-by: Daniil Klimuk <[email protected]>
Reviewed-by: Michał Iwanicki <[email protected]>
Signed-off-by: Daniil Klimuk <[email protected]>
flashrom does not support QEMU and fails every time. I have not found a
better way to handle it, but we should not place hardwere related
configs all around the code.

Signed-off-by: Daniil Klimuk <[email protected]>
@DaniilKl DaniilKl marked this pull request as ready for review November 20, 2024 11:25
@DaniilKl DaniilKl requested a review from m-iwanicki November 20, 2024 11:25
Signed-off-by: Daniil Klimuk <[email protected]>
…rver

login_to_dpp_server should only try to login, the decision to print
warning or not should be done outside this function basing on the output
of this function. Otherwise every time the function is being called the
warning could be printed too, which is not always wanted.

Signed-off-by: Daniil Klimuk <[email protected]>
@DaniilKl
Copy link
Contributor Author

DaniilKl commented Nov 20, 2024

Forgot to delete one WIP commit. Deleted now.

Copy link
Contributor

@m-iwanicki m-iwanicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Will need to be tested before release

@DaniilKl DaniilKl merged commit e757053 into main Nov 20, 2024
1 check passed
@DaniilKl DaniilKl deleted the add-qemu-support branch November 20, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants