| Message ID | 613c3dc97a29fa9b8baa8acf45cefe4fac24ea87.1712141833.git.roy.hopkins@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/10] meson: Add optional dependency on IGVM library | expand |
> On 3 Apr 2024, at 16:41, Roy Hopkins <roy.hopkins@suse.com> wrote: > > When using an IGVM file the configuration of the system firmware is > defined by IGVM directives contained in the file. In this case the user > should not configure any pflash devices. > > This commit skips initialization of the ROM mode when pflash0 is not set > then checks to ensure no pflash devices have been configured when using > IGVM, exiting with an error message if this is not the case. > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> > --- > hw/i386/pc_sysfw.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index 3efabbbab2..2412f26225 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -226,8 +226,13 @@ void pc_system_firmware_init(PCMachineState *pcms, > } > > if (!pflash_blk[0]) { > - /* Machine property pflash0 not set, use ROM mode */ > - x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false); > + /* > + * Machine property pflash0 not set, use ROM mode unless using IGVM, > + * in which case the firmware must be provided by the IGVM file. What if the igvm file does not contain a firmware? Can we have a check for that case somewhere and assert if firmware is absent? > + */ > + if (!cgs_is_igvm(MACHINE(pcms)->cgs)) { > + x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false); > + } > } else { > if (kvm_enabled() && !kvm_readonly_mem_enabled()) { > /* > @@ -243,6 +248,20 @@ void pc_system_firmware_init(PCMachineState *pcms, > } > > pc_system_flash_cleanup_unused(pcms); > + > + /* > + * The user should not have specified any pflash devices when using IGVM > + * to configure the guest. > + */ > + if (cgs_is_igvm(MACHINE(pcms)->cgs)) { > + for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { > + if (pcms->flash[i]) { > + error_report("pflash devices cannot be configured when " > + "using IGVM"); > + exit(1); > + } > + } > + } I have not tested this change but looking at pc_system_flash_create() creates flash[0] and flash[1] for all cases (well except for isapc machines). So for igvm case, would this not cause an exit all the time? > } > > void x86_firmware_configure(void *ptr, int size) > -- > 2.43.0 >
On Thu, 2024-04-04 at 18:06 +0530, Ani Sinha wrote: > > > > On 3 Apr 2024, at 16:41, Roy Hopkins <roy.hopkins@suse.com> wrote: > > > > When using an IGVM file the configuration of the system firmware is > > defined by IGVM directives contained in the file. In this case the user > > should not configure any pflash devices. > > > > This commit skips initialization of the ROM mode when pflash0 is not set > > then checks to ensure no pflash devices have been configured when using > > IGVM, exiting with an error message if this is not the case. > > > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> > > --- > > hw/i386/pc_sysfw.c | 23 +++++++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > > index 3efabbbab2..2412f26225 100644 > > --- a/hw/i386/pc_sysfw.c > > +++ b/hw/i386/pc_sysfw.c > > @@ -226,8 +226,13 @@ void pc_system_firmware_init(PCMachineState *pcms, > > } > > > > if (!pflash_blk[0]) { > > - /* Machine property pflash0 not set, use ROM mode */ > > - x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false); > > + /* > > + * Machine property pflash0 not set, use ROM mode unless using > > IGVM, > > + * in which case the firmware must be provided by the IGVM file. > > What if the igvm file does not contain a firmware? Can we have a check for > that case somewhere and assert if firmware is absent? > I don't think we can easily check for presence of firmware. In fact, using IGVM means that you can effectively launch a guest without traditional firmware. A good example of this is if you use IGVM to deploy an SVSM module that is used to help with guest migration. In this case, the SVSM code would be placed in memory (by the IGVM directives) and the initial CPU state configured to launch the SVSM kernel directly. > > + */ > > + if (!cgs_is_igvm(MACHINE(pcms)->cgs)) { > > + x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, > > false); > > + } > > } else { > > if (kvm_enabled() && !kvm_readonly_mem_enabled()) { > > /* > > @@ -243,6 +248,20 @@ void pc_system_firmware_init(PCMachineState *pcms, > > } > > > > pc_system_flash_cleanup_unused(pcms); > > + > > + /* > > + * The user should not have specified any pflash devices when using > > IGVM > > + * to configure the guest. > > + */ > > + if (cgs_is_igvm(MACHINE(pcms)->cgs)) { > > + for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { > > + if (pcms->flash[i]) { > > + error_report("pflash devices cannot be configured when " > > + "using IGVM"); > > + exit(1); > > + } > > + } > > + } > > I have not tested this change but looking at pc_system_flash_create() creates > flash[0] and flash[1] for all cases (well except for isapc machines). So for > igvm case, would this not cause an exit all the time? > Although the flash devices are created, if they are not configured then they are removed by the call to pc_system_flash_cleanup_unused() above, meaning that this check succeeds in the IGVM case. > > } > > > > void x86_firmware_configure(void *ptr, int size) > > -- > > 2.43.0 > > > Regards, Roy
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index 3efabbbab2..2412f26225 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -226,8 +226,13 @@ void pc_system_firmware_init(PCMachineState *pcms, } if (!pflash_blk[0]) { - /* Machine property pflash0 not set, use ROM mode */ - x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false); + /* + * Machine property pflash0 not set, use ROM mode unless using IGVM, + * in which case the firmware must be provided by the IGVM file. + */ + if (!cgs_is_igvm(MACHINE(pcms)->cgs)) { + x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false); + } } else { if (kvm_enabled() && !kvm_readonly_mem_enabled()) { /* @@ -243,6 +248,20 @@ void pc_system_firmware_init(PCMachineState *pcms, } pc_system_flash_cleanup_unused(pcms); + + /* + * The user should not have specified any pflash devices when using IGVM + * to configure the guest. + */ + if (cgs_is_igvm(MACHINE(pcms)->cgs)) { + for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { + if (pcms->flash[i]) { + error_report("pflash devices cannot be configured when " + "using IGVM"); + exit(1); + } + } + } } void x86_firmware_configure(void *ptr, int size)
When using an IGVM file the configuration of the system firmware is defined by IGVM directives contained in the file. In this case the user should not configure any pflash devices. This commit skips initialization of the ROM mode when pflash0 is not set then checks to ensure no pflash devices have been configured when using IGVM, exiting with an error message if this is not the case. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> --- hw/i386/pc_sysfw.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)