Add support for Hyper-V firmware execution #201
Conversation
Signed-off-by: Jon Lange <jlange@microsoft.com>
…ltiple of pages in size Signed-off-by: Jon Lange <jlange@microsoft.com>
Signed-off-by: Jon Lange <jlange@microsoft.com>
Signed-off-by: Jon Lange <jlange@microsoft.com>
Signed-off-by: Jon Lange <jlange@microsoft.com>
Signed-off-by: Jon Lange <jlange@microsoft.com>
Signed-off-by: Jon Lange <jlange@microsoft.com>
| // the final file. | ||
| if (allocation_size != data_size) | ||
| { | ||
| memset((uint8_t *)data_object->data, data_size, allocation_size - data_size); |
There was a problem hiding this comment.
This doesn't zero the padding. It is setting the start of the buffer to the first byte of data_size.
| memset((uint8_t *)data_object->data, data_size, allocation_size - data_size); | |
| memset(&((uint8_t *)data_object->data)[data_size], 0, allocation_size - data_size); |
There was a problem hiding this comment.
I fixed this locally before submitting but apparently submitted the wrong version. I wonder what other incorrect code made it in. In any case, I will definitely fix this.
| // Copyright (c) Microsoft Corporation | ||
| // | ||
| // Author: Jon Lange (jlange@microsoft.com) | ||
|
|
There was a problem hiding this comment.
Missing guard to prevent including more than once.
| _reserved: u32, | ||
| /// Indicates that the initial location of firmware is at the base of | ||
| /// memory and will not be loaded into the ROM range. | ||
| pub in_low_memory: u8, |
There was a problem hiding this comment.
I wonder if it makes more sense to invert this flag and name it something like in_system_region (see comment below regarding 'system')? This would then explain the change in behaviour when the firmware uses the ROM region. Or just rename it to outside_system_region?
There was a problem hiding this comment.
We could; my intention here was that any parameter that is zero continues the existing behavior, and new behavior is only invoked by non-zero parameters. That means that the existing "FW lives in ROM range" idea would be associated with a zero parameter. There's no good reason that I can't invert that if we think it enhances clarity.
| fn check_ovmf_regions( | ||
| flash_regions: &[MemoryRegion<PhysAddr>], | ||
| kernel_region: &MemoryRegion<PhysAddr>, | ||
| ) { |
There was a problem hiding this comment.
Further to our email/call discussion about the name of 'flash' in SVSM, I wonder if this function and parameters should be renamed. I'm not sure what would be a good name for the region from 3GB-4GB. ROM region? System region?
| fn check_ovmf_regions( | |
| flash_regions: &[MemoryRegion<PhysAddr>], | |
| kernel_region: &MemoryRegion<PhysAddr>, | |
| ) { | |
| fn check_system_region( | |
| fw_regions: &[MemoryRegion<PhysAddr>], | |
| kernel_region: &MemoryRegion<PhysAddr>, | |
| ) { |
| if !igvm_params.fw_in_low_memory() { | ||
| check_ovmf_regions(&flash_regions, kernel_region); | ||
| } |
There was a problem hiding this comment.
Renaming in_low_memory and check_ovmf_regions() as per my above comments would result in something like this, which I think conveys the intent more clearly.
| if !igvm_params.fw_in_low_memory() { | |
| check_ovmf_regions(&flash_regions, kernel_region); | |
| } | |
| if igvm_params.fw_in_system_region() { | |
| check_system_region(&fw_regions, kernel_region); | |
| } |
|
|
||
| let mut regions = Vec::new(); | ||
|
|
||
| if self.igvm_param_block.firmware.in_low_memory != 0 { |
There was a problem hiding this comment.
You could use fw_in_low_memory() here.
| else | ||
| { | ||
| fprintf(stderr, "--firmware only supported for hyperv targets\n"); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
This else condition can never be executed as it's already in an if (is_hyperv).
This change adds support for execution of the Hyper-V VM firmware. This includes placing the firmware in the low portion of the address space (not in flash space), configuring the initial execution context to launch directly in 64-bit mode, and enabling vTOM in the guest address space.