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

flashy: Support getting ImageFormats via buildname and add grandcanyon image formats #151

Closed
wants to merge 1 commit into from

Conversation

lhl2617
Copy link
Contributor

@lhl2617 lhl2617 commented Apr 19, 2021

Summary

Corresponding diff in fw-util: bc32863

Previously flashy was not able to acquire platform-specific image formats that are used to validate images/devices. This diff introduces a feature in which the ImageFormats to validate against are obtained via the buildname from /etc/issue.

This feature is required as grandcanyon is retrofitted to vboot as in bc32863

Test plan

Unit tests go test ./...
Please help run validation tests as per bc32863

/opt/flashy -checkimage -imagepath <IMAGE>

@@ -297,6 +297,41 @@ var GetOpenBMCVersionFromIssueFile = func() (string, error) {
return version, nil
}

// Deal with images that have changed names, but are otherwise compatible.
Copy link
Contributor Author

@lhl2617 lhl2617 Apr 19, 2021

Choose a reason for hiding this comment

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

This was moved from the validate package (to prevent an import cycle)

@facebook-github-bot
Copy link
Contributor

@kawmarco has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@lhl2617
Copy link
Contributor Author

lhl2617 commented Apr 26, 2021

Ah--I also meant to start another discussion here--instead of doing it this way which hides platform-specific checks embedded inside the image validation logic I was wondering if making a step tools/flashy/checks_and_remediations/common/41_validate_image_partitions.go for every platform would be a more sensible choice?

The problem is that the check-image utility will not be privy to that (and hence will wrongly validate the image and allow non-vboot images to pass validation). Every single platform would also require its XX_validate_image_partitions.go step, which if we forgot to add would mean forgoing the very important step of validating images.

@lhl2617 lhl2617 marked this pull request as draft April 26, 2021 14:39
@lhl2617
Copy link
Contributor Author

lhl2617 commented Apr 26, 2021

Ah--I also meant to start another discussion here--instead of doing it this way which hides platform-specific checks embedded inside the image validation logic I was wondering if making a step tools/flashy/checks_and_remediations/common/41_validate_image_partitions.go for every platform would be a more sensible choice?

The problem is that the check-image utility will not be privy to that (and hence will wrongly validate the image and allow non-vboot images to pass validation). Every single platform would also require its XX_validate_image_partitions.go step, which if we forgot to add would mean forgoing the very important step of validating images.

@kawmarco should we make image validation a platform-specific step with a step file for every single platform? That way we can specify the formats that are valid for each platform, which is a plus.

The problem would arise when one forgets to create this step for a new platform--but hopefully cross-checking this with flashy should not be a problem: I will write a test to make sure that all buildnames in https://github.com/facebook/openbmc/blob/helium/tools/platforms/platform_build_names has a corresponding image validation step and also flash step if necessary.

@kawmarco
Copy link
Contributor

Hey @lhl2617, thanks for bringing up the discussion. I think it should be good to move it to a platform-specific step for now as AFAIK it's the only platform that needs this check. In the future, we can try figuring out if we need a better abstraction for this but, given this is only needed for systems transitioning to vboot, these likely won't be needed for most of the platform's lifetime (i.e. eventually production and factory systems will be all vboot enabled, and the non-vboot version should fail normal vboot checks).

The problem is that the check-image utility will not be privy to that (and hence will wrongly validate the image and allow non-vboot images to pass validation

Sounds like a reasonable compromise to me 👍

@lhl2617
Copy link
Contributor Author

lhl2617 commented Apr 27, 2021

There will be some tricky conflicts to fix with #150, so I'll implement this after that gets merged into helium.

@lhl2617
Copy link
Contributor Author

lhl2617 commented Apr 28, 2021

Superseded by #156

@lhl2617 lhl2617 closed this Apr 28, 2021
@lhl2617 lhl2617 deleted the platform-validate branch May 10, 2021 15:38
facebook-github-bot pushed a commit that referenced this pull request Apr 5, 2023
Summary:
Override patch file #126 with #151 because the new patch (copied from
linux-aspeed-6.0) can be safely applied to all the openbmc platforms.

Test Plan:
1) "bitbake cmm-image" succeeded.
2) rebooted cmm image (running linux 6.1) for a few times: no issue
   found.

Reviewed By: williamspatrick

Differential Revision: D44718621

fbshipit-source-id: eb57a465d9fa8b9cfab86abc35564470b9f01552
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants