-
Notifications
You must be signed in to change notification settings - Fork 47
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
IGVM: Add support for launching OVMF firmware when using an IGVM file for guest configuration #181
Changes from all commits
93edc9d
a7cfe9d
6617b5e
2941fa9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ | |
|
||
extern crate alloc; | ||
|
||
use alloc::vec::Vec; | ||
use svsm::fw_meta::{parse_fw_meta_data, print_fw_meta, validate_fw_memory, SevFWMetaData}; | ||
|
||
use core::arch::global_asm; | ||
|
@@ -39,7 +38,7 @@ use svsm::mm::alloc::{memory_info, print_memory_info, root_mem_init}; | |
use svsm::mm::memory::init_memory_map; | ||
use svsm::mm::pagetable::paging_init; | ||
use svsm::mm::virtualrange::virt_log_usage; | ||
use svsm::mm::{init_kernel_mapping_info, PerCPUPageMappingGuard, SIZE_1G}; | ||
use svsm::mm::{init_kernel_mapping_info, PerCPUPageMappingGuard}; | ||
use svsm::requests::{request_loop, update_mappings}; | ||
use svsm::serial::SerialPort; | ||
use svsm::sev::secrets_page::{copy_secrets_page, disable_vmpck0, SecretsPage}; | ||
|
@@ -219,10 +218,8 @@ fn launch_fw() -> Result<(), SvsmError> { | |
Ok(()) | ||
} | ||
|
||
fn validate_flash() -> Result<(), SvsmError> { | ||
let mut fw_cfg = FwCfg::new(&CONSOLE_IO); | ||
|
||
let flash_regions = fw_cfg.iter_flash_regions().collect::<Vec<_>>(); | ||
fn validate_fw(config: &SvsmConfig) -> Result<(), SvsmError> { | ||
let flash_regions = config.get_fw_regions()?; | ||
let kernel_region = LAUNCH_INFO.kernel_region(); | ||
let flash_range = { | ||
let one_gib = 1024 * 1024 * 1024usize; | ||
|
@@ -307,10 +304,9 @@ fn mapping_info_init(launch_info: &KernelLaunchInfo) { | |
); | ||
} | ||
|
||
fn map_and_parse_fw_meta() -> Result<SevFWMetaData, SvsmError> { | ||
// Map meta-data location, it starts at 32 bytes below 4GiB | ||
let pstart = PhysAddr::from((4 * SIZE_1G) - PAGE_SIZE); | ||
let guard = PerCPUPageMappingGuard::create_4k(pstart)?; | ||
fn map_and_parse_fw_meta(fw_metadata_phys: PhysAddr) -> Result<SevFWMetaData, SvsmError> { | ||
// Map the metadata location which is defined by the firmware config | ||
let guard = PerCPUPageMappingGuard::create_4k(fw_metadata_phys)?; | ||
let vstart = guard.virt_addr().as_ptr::<u8>(); | ||
// Safety: we just mapped a page, so the size must hold. The type | ||
// of the slice elements is `u8` so there are no alignment requirements. | ||
|
@@ -461,14 +457,10 @@ pub extern "C" fn svsm_main() { | |
|
||
start_secondary_cpus(&cpus); | ||
|
||
let fw_metadata = if config.should_launch_fw() { | ||
Some( | ||
map_and_parse_fw_meta() | ||
.unwrap_or_else(|e| panic!("Failed to parse FW SEV meta-data: {:#?}", e)), | ||
) | ||
} else { | ||
None | ||
}; | ||
let fw_metadata = config.get_fw_metadata().map(|fw_metadata_phys| { | ||
map_and_parse_fw_meta(fw_metadata_phys) | ||
.unwrap_or_else(|e| panic!("Failed to parse FW SEV meta-data: {:#?}", e)) | ||
}); | ||
|
||
if let Some(ref fw_meta) = fw_metadata { | ||
print_fw_meta(fw_meta); | ||
|
@@ -480,8 +472,10 @@ pub extern "C" fn svsm_main() { | |
if let Err(e) = copy_tables_to_fw(fw_meta) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I meant something slightly different. The current implementation of 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, done. Firmware metadata is now optional and firmware can be launched even if metadata is not provided. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
panic!("Failed to copy firmware tables: {:#?}", e); | ||
} | ||
} | ||
|
||
if let Err(e) = validate_flash() { | ||
if config.should_launch_fw() { | ||
if let Err(e) = validate_fw(&config) { | ||
panic!("Failed to validate flash memory: {:#?}", e); | ||
} | ||
} | ||
|
@@ -494,7 +488,7 @@ pub extern "C" fn svsm_main() { | |
|
||
virt_log_usage(); | ||
|
||
if fw_metadata.is_some() { | ||
if config.should_launch_fw() { | ||
if let Err(e) = launch_fw() { | ||
panic!("Failed to launch FW: {:#?}", e); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change ignores the comment above the function. Not taking
&mut self
allows potential misbehavior. The comment should be updated or the change reverted.