IGVM: Add support for launching OVMF firmware when using an IGVM file for guest configuration #181
Conversation
roy-hopkins
force-pushed
the
qemu_igvm
branch
from
bc32428 to
23eb8b9
Compare
| @@ -473,8 +470,7 @@ pub extern "C" fn svsm_main() { | |||
| if let Err(e) = copy_tables_to_fw(fw_meta) { | |||
There was a problem hiding this comment.
Your PR moves the location of the firmware to the IGVM file (which is good) but there is still a hardcoded assumption that the firmware wants CPUID/secrets information in a particular place (see map_and_parse_fw_meta). Shouldn't this be abstracted in the IGVM as well so that the SVSM is not making assumptions about OVMF-specific layout?
There was a problem hiding this comment.
Yes. I mentioned in the commit and the description of the PR that although the firmware is in the IGVM file the builder must currently align the firmware to 4GB. This PR allows me to get back to the point where we can boot firmware from SVSM when using an IGVM file. At this stage I think it is ok to assume that OVMF places it's configuration at the top of its GPA range as this is well defined.
The future plan is to allow for IGVM to specify a different location for the firmware, and at that point we would need to describe the layout via parameters to SVSM. But this would require changes to OVMF which I think will be the subject of future work.
Do you think this approach is acceptable?
There was a problem hiding this comment.
Sorry, I meant something slightly different. The current implementation of map_and_parse_fw_meta has hard-coded logic which assumes that the firmware metadata must be at 4 GB less 4 KB. That may be great for OVMF today but won't be great for any other firmware that might be packaged into an IGVM file. Since you're making IGVM flexible enough to specify the base/size of the firmware, it seems like you should also have the IGVM file include the location of the metadata information (if it exists at all) so that map_and_parse_fw_meta can do its work to match what the IGVM specifies. Note that none of this has anything to with changing OVMF; it's just about making the SVSM self-consistent in the way it looks for FW-specific data (i.e., dictate all references from the IGVM and not from hard-coded constants).
I can always add that in a future PR if this is too much of a distraction from completing your current PR. It shouldn't be much work regardless of which one of us does it.
There was a problem hiding this comment.
As far as I'm aware, the format of the metadata itself is fairly unique to OVMF, including the location of where to find it. But I see your point - it is possible for the IGVM file to locate it elsewhere even if OVMF won't boot in that case. The fw range check in SVSM currently ensures it aligns with 4GB so it won't allow the firmware to be located anywhere else, but that will change and it does make sense to add the new field to describe the GPA of the firmware metadata page. I'll add it to the parameters.
There was a problem hiding this comment.
This is better, but I foresee a version where we have firmware but where the firmware doesn't use metadata in this way. In that case, I would expect the IGVM parameters to specify a base/size of the firmware but would specify a metadata address of zero. However, your new version of the code appears to treat that as an error, because get_fw_metadata is called unconditionally and would return an error. Can this be changed so that firmware that does not express a metadata location can just skip the step of preparing the metadata instead of generating an error.
There was a problem hiding this comment.
Ok, done. Firmware metadata is now optional and firmware can be launched even if metadata is not provided.
The self parameter is not mutated by the function so this commit replaces `&mut self` with `&self`. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
…block The IgvmParamBlock currently contains a flag that determines whether the guest firmware (OVMF) should be launched or not by the SVSM kernel. However, in order to launch the firmware embedded in an IGVM file the SVSM needs to know the GPA at which the firmware is loaded and the number of pages in order to set the correct VMPL permissions on the pages. For the non-IGVM case, this comes from fw_cfg but this is not populated when using IGVM files. So this commit replaces the launch_fw flag with a start GPA and page range for the firmware region. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
When using an IGVM file the OVMF firmware will be populated into the guest using pages described by IGVM and not using firmware volumes and fw_cfg. This patch uses the current configuration (IGVM or fw_cfg) to determine the firmware range and validate the pages accordingly before launching the firmware. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
roy-hopkins
force-pushed
the
qemu_igvm
branch
from
23eb8b9 to
c9d9a9d
Compare
When using fw_cfg, the OVMF metadata is located at a hardcoded location of 1 page below 4GB. When using IGVM for configuration it is possible for the IGVM file to specify a different location for the firmware itself so it makes sense to also specify a parameter to describe the metdata page location. This commit adds a new IGVM parameter field and uses this to determine the OVMF metadata location. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
roy-hopkins
force-pushed
the
qemu_igvm
branch
from
c9d9a9d to
2941fa9
Compare
| // This needs to be &mut self to prevent iterator invalidation, where the caller | ||
| // could do fw_cfg.select() while iterating. Having a mutable reference prevents | ||
| // other references. | ||
| pub fn iter_flash_regions(&mut self) -> impl Iterator<Item = MemoryRegion<PhysAddr>> + '_ { | ||
| pub fn iter_flash_regions(&self) -> impl Iterator<Item = MemoryRegion<PhysAddr>> + '_ { |
There was a problem hiding this comment.
This change ignores the comment above the function. Not taking &mut self allows potential misbehavior. The comment should be updated or the change reverted.
This changes the IGVM parameter block to replace the flag that indicates whether to boot OVMF with two parameters that define the GPA range of the OVMF region.
The same constraints on OVMF location are currently still true - the OVMF pages must be aligned up to the 4GB boundary therefore the IGVM builder must ensure this alignment for OVMF to be booted correctly.