Merge AndroidEfiLoaderCompositeDiskConfig with AndroidCompositeDiskConfig#2254
Merge AndroidEfiLoaderCompositeDiskConfig with AndroidCompositeDiskConfig#2254Databean wants to merge 3 commits intogoogle:mainfrom
Conversation
cjreynol
left a comment
cjreynol
left a comment
There was a problem hiding this comment.
I saw you came up with a test case, but consider checking with Dmitrii or Ram if there are any other cases to check? I am just ignorant to how disruptive a breakage here might be and am defaulting towards greater caution.
| Result<std::string> Path() const override; | ||
|
|
||
| private: | ||
| static constexpr std::string_view kName = "android_esp"; |
There was a problem hiding this comment.
Just curious, not a request for change:
Any reason to make this a class member rather than a static variable declared in the source file?
There was a problem hiding this comment.
No good reason, copied the class definition from another place that did need to expose it, while this one didn't. I moved it to private but it might as well be only in the .cc file.
There was a problem hiding this comment.
Moved to the source file.
|
FYI @dimorinny |
|
Looks like this "happens to work" in an unintuitive way. There's a comment in the code from before this CL saying
As written here, the partitions are put in a |
cjreynol
left a comment
cjreynol
left a comment
There was a problem hiding this comment.
Based on the latest comment about this not necessarily working except in certain cases.
Bug: b/491903055
Bug: b/491903055
The ImageFile mechanism is already dynamic in which partitions are added, and the only contribution of AndroidEfiLoaderCompositeDisk was to add another partition. Bug: b/491903055
Databean
force-pushed
the
merge_android_efi
branch
from
34fd5ed to
18da0b1
Compare
|
The "insert at the beginning" logic is now more explicit. |
|
|
||
| // Cuttlefish uboot EFI bootflow by default looks at the first partition | ||
| // for EFI application. Thus we put "android_esp" at the beginning. | ||
| std::vector<ImagePartition>::iterator insert_location = |
There was a problem hiding this comment.
nit: The requirement to strictly keep the ESP partition first is a bit annoying to maintain (especially once you merge it into the main android_composite_disk_config).
I played around with the U-Boot environment, and I think we can get rid of this requirement by relying on the partition name instead. For example, we could extend the WriteEFIEnvironment logic by adding a command to obtain a partition index by its name, so it would look something like this:
part number virtio 0 android_esp devplist && load virtio 0:${devplist} ${loadaddr} efi/boot/bootaa64.efi && bootefi ${loadaddr} ${fdtcontroladdr};
According to my local experiments, it should work:
=> part number virtio 0 android_esp PART_NUM
=> printenv PART_NUM
PART_NUM=0x1
Both are part of assembly_cvd, so I don't think we will have any compatibility problems with that approach.
This would be slightly outside the scope of the initial proposal, so if you prefer, I can create a separate bug and address it. Please let me know.
3 participants
We now have the
ImageFilemechanism where partitions can be supplied dynamically. Since the main contribution ofAndroidEfiLoaderCompositeDiskConfig, this can go through theImageFilelogic instead.Tested with a build fetched using
--android_efi_loader_build=aosp_uefi-gbl-mainline/gbl_efi_dist_and_test.Bug: b/491903055