Add hypervisor field to IgvmParamBlock #609
Conversation
The is_qemu field in IgvmParamBlock as used right now has two meanings. One is that we can assume access to qemu fw services such as TestDev as checked by is_qemu_test_env(). The other meaning is that we're running on top of qemu as checked in stage2 for determining suppress_svsm_interrupts. We'd like to suppress_svsm_interrupts for Vanadium and may want to toggle enabling/disabling options separately per hypervisor, so let's add a more generic hypervisor field to IgvmParamBlock to check in general. Then we can leave the is_qemu field to only signify accessibility to qemu_fw_services. A hypervisor enum already exists in igvmbuilder::cmd_options::Hypervisor. Unfortunately this is not directly usable in bootlib::igvm_params::IgvmParamBlock for two reasons. One is that bootlib is a dependency of igvmbuilder, this could be moved if not for reason two which is that the clap parser derivations used by CmdOptions don't support no_std which bootlib is built for. Thus it seems simplest to create a second enum type and convert between the two. Having disambiguated the is_qemu field, we then set is_qemu to true for Vanadium to ensure that all the tests making use of TestDev will also run for Vanadium. Since interrupts aren't yet support in Vanadium, we also set suppress_svsm_interrupts to accordingly. Signed-off-by: Jacob Xu <jacobhxu@google.com>
Now that there's a hypervisor field in IgvmParamBlock, let's rename is_qemu to reduce ambiguity. Signed-off-by: Jacob Xu <jacobhxu@google.com>
|
has_qemu_fw_services is still too generic for us. The "qemu fw sevices" come from both the pc-testdev device (installed in I'm also a bit confused about having both the hypervisor enum and a boolean (qemu fw services) that solely depends on the enum. IMO it'd be better to have a bunch of booleans in the IgvmParamBlock that specify behavior rather than the hypervisor enum. So in that case we'd have "has_qemu_testdev", "has_test_iorequests", and "suppress_svsm_interrupts". Igvmbuilder would specify which of these to set based on the hypervisor. |
|
For suppress_svsm_interrupts, it's currently conditioned on both SNP and the hypervisor. Since IGVM is arch-agnostic for snp/tdx, it seems the determination of this may be necessary to do at runtime instead of igvm build time.
Perhaps it's better to remove the qemu_fw_services/is_qemu field entirely from IgvmParamsBlock then and just make do with the hypervisor field? Then we can make those checks based on hypervisor. |
Name it
That would be better than having both, but IMO individual bools are more "morally correct" than a hypervisor enum. The idea should be that the IGVM could boot on any hypervisor even if practically there are different requirements from each hypervisor. |
Ah I see, that makes sense; I was initially confused since we're building different IGVM files for each hypervisor; but this makes sense if we're moving towards that direction. I'll write up a separate PR and then close this one. |
| } | ||
| } | ||
|
|
||
| pub fn hypervisor(&self) -> bootlib::igvm_params::Hypervisor { |
There was a problem hiding this comment.
Since the only definition of Hypervisor that is usable by the kernel environment is the one in bootlib::igvm_params, can we just use bootlib::igvm_params::Hypervisor and avoid the need for explicit namespace decoration here and in other uses?
| self.igvm_param_block.has_qemu_fw_services != 0 | ||
| } | ||
|
|
||
| pub fn hypervisor(&self) -> bootlib::igvm_params::Hypervisor { |
There was a problem hiding this comment.
Above comment about use bootlib::igvm_params::Hypervisor applies here as well.
| // This is required when running SNP under KVM/QEMU. | ||
| let suppress_svsm_interrupts = match platform_type { | ||
| SvsmPlatformType::Snp => config.is_qemu(), | ||
| SvsmPlatformType::Snp => { | ||
| config.hypervisor() == Hypervisor::Vanadium || config.hypervisor() == Hypervisor::Qemu |
There was a problem hiding this comment.
Can we define a method like SvsmConfig::can_use_svsm_interrupts so that all of the per-hypervisor logic can exist in the config object instead of having decisions leak out to other places like this one?
This PR intends to suppress interrupts on Vanadium, and also enable the use of TestDev on Vanadium. To do this, it also disambiguates the IgvmParamBlock.is_qemu field as described in #608