| Message ID | 20220331083549.749566-3-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386: firmware parsing and sev setup for -bios loaded firmware | expand |
On Thu, Mar 31, 2022 at 10:35:48AM +0200, Gerd Hoffmann wrote: > move sev firmware setup to separate function so it can be used from > other code paths. No functional change. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Tested-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > include/hw/i386/x86.h | 3 +++ > hw/i386/pc_sysfw.c | 36 ++++++++++++++++++++++-------------- > 2 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h > index 916cc325eeb1..4841a49f86c0 100644 > --- a/include/hw/i386/x86.h > +++ b/include/hw/i386/x86.h > @@ -140,4 +140,7 @@ void gsi_handler(void *opaque, int n, int level); > void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name); > DeviceState *ioapic_init_secondary(GSIState *gsi_state); > > +/* pc_sysfw.c */ > +void x86_firmware_configure(void *ptr, int size); > + > #endif > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index c8b17af95353..36b6121b77b9 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -148,7 +148,6 @@ static void pc_system_flash_map(PCMachineState *pcms, > MemoryRegion *flash_mem; > void *flash_ptr; > int flash_size; > - int ret; > > assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled); > > @@ -196,19 +195,7 @@ static void pc_system_flash_map(PCMachineState *pcms, > if (sev_enabled()) { > flash_ptr = memory_region_get_ram_ptr(flash_mem); > flash_size = memory_region_size(flash_mem); > - /* > - * OVMF places a GUIDed structures in the flash, so > - * search for them > - */ > - pc_system_parse_ovmf_flash(flash_ptr, flash_size); > - > - ret = sev_es_save_reset_vector(flash_ptr, flash_size); > - if (ret) { > - error_report("failed to locate and/or save reset vector"); > - exit(1); > - } > - > - sev_encrypt_flash(flash_ptr, flash_size, &error_fatal); > + x86_firmware_configure(flash_ptr, flash_size); > } > } > } > @@ -260,3 +247,24 @@ void pc_system_firmware_init(PCMachineState *pcms, > > pc_system_flash_cleanup_unused(pcms); > } > + > +void x86_firmware_configure(void *ptr, int size) > +{ > + int ret; > + > + /* > + * OVMF places a GUIDed structures in the flash, so > + * search for them > + */ > + pc_system_parse_ovmf_flash(ptr, size); Any reason you chose to put this outside the sev_enabled() check when you moved it, as that is a functional change ? It ought to be harmless in theory, unless someone figures out a way to break pc_system_parse_ovmf_flash code with unexpected input. > + > + if (sev_enabled()) { > + ret = sev_es_save_reset_vector(ptr, size); > + if (ret) { > + error_report("failed to locate and/or save reset vector"); > + exit(1); > + } > + > + sev_encrypt_flash(ptr, size, &error_fatal); > + } > +} > -- > 2.35.1 > > With regards, Daniel
Hi, > > +void x86_firmware_configure(void *ptr, int size) > > +{ > > + int ret; > > + > > + /* > > + * OVMF places a GUIDed structures in the flash, so > > + * search for them > > + */ > > + pc_system_parse_ovmf_flash(ptr, size); > > Any reason you chose to put this outside the sev_enabled() > check when you moved it, as that is a functional change ? Well, we probably get a 'if (tdx_enabled())' branch soon, and pc_system_parse_ovmf_flash() will be needed for both sev and tdx. > It ought to be harmless in theory, unless someone figures > out a way to break pc_system_parse_ovmf_flash code with > unexpected input. Yes, strictly speaking this is a functional change. Without sev the pc_system_parse_ovmf_flash() results will be ignored though, so there should be no change in qemu behavior ... take care, Gerd
On Thu, Mar 31, 2022 at 10:35:48AM +0200, Gerd Hoffmann wrote: > move sev firmware setup to separate function so it can be used from > other code paths. No functional change. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Tested-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > include/hw/i386/x86.h | 3 +++ > hw/i386/pc_sysfw.c | 36 ++++++++++++++++++++++-------------- > 2 files changed, 25 insertions(+), 14 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On 31/3/22 10:35, Gerd Hoffmann wrote: > move sev firmware setup to separate function so it can be used from > other code paths. No functional change. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Tested-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > include/hw/i386/x86.h | 3 +++ > hw/i386/pc_sysfw.c | 36 ++++++++++++++++++++++-------------- > 2 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h > index 916cc325eeb1..4841a49f86c0 100644 > --- a/include/hw/i386/x86.h > +++ b/include/hw/i386/x86.h > @@ -140,4 +140,7 @@ void gsi_handler(void *opaque, int n, int level); > void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name); > DeviceState *ioapic_init_secondary(GSIState *gsi_state); > > +/* pc_sysfw.c */ > +void x86_firmware_configure(void *ptr, int size); > + > #endif > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index c8b17af95353..36b6121b77b9 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -148,7 +148,6 @@ static void pc_system_flash_map(PCMachineState *pcms, > MemoryRegion *flash_mem; > void *flash_ptr; > int flash_size; > - int ret; > > assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled); > > @@ -196,19 +195,7 @@ static void pc_system_flash_map(PCMachineState *pcms, > if (sev_enabled()) { ^^^ > flash_ptr = memory_region_get_ram_ptr(flash_mem); > flash_size = memory_region_size(flash_mem); Can we remove the SEV check ... > - /* > - * OVMF places a GUIDed structures in the flash, so > - * search for them > - */ > - pc_system_parse_ovmf_flash(flash_ptr, flash_size); > - > - ret = sev_es_save_reset_vector(flash_ptr, flash_size); > - if (ret) { > - error_report("failed to locate and/or save reset vector"); > - exit(1); > - } > - > - sev_encrypt_flash(flash_ptr, flash_size, &error_fatal); > + x86_firmware_configure(flash_ptr, flash_size); ... making this code generic ...? > } > } > } > @@ -260,3 +247,24 @@ void pc_system_firmware_init(PCMachineState *pcms, > > pc_system_flash_cleanup_unused(pcms); > } > + > +void x86_firmware_configure(void *ptr, int size) > +{ > + int ret; > + > + /* > + * OVMF places a GUIDed structures in the flash, so > + * search for them > + */ > + pc_system_parse_ovmf_flash(ptr, size); > + > + if (sev_enabled()) { ... because we are still checking SEV here. > + ret = sev_es_save_reset_vector(ptr, size); > + if (ret) { > + error_report("failed to locate and/or save reset vector"); > + exit(1); > + } > + > + sev_encrypt_flash(ptr, size, &error_fatal); > + } > +}
> > if (sev_enabled()) { > > ^^^ > Can we remove the SEV check ... > > + pc_system_parse_ovmf_flash(ptr, size); > > + > > + if (sev_enabled()) { > > ... because we are still checking SEV here. Well, the two checks have slightly different purposes. The first check will probably become "if (sev || tdx)" soon, whereas the second will become "if (sev) { ... } if (tdx) { ... }". We could remove the first. pc_system_parse_ovmf_flash() would run unconditionally then. Not needed, but should not have any bad side effects. take care, Gerd
On 4/1/2022 1:08 PM, Gerd Hoffmann wrote: >>> if (sev_enabled()) { >> >> ^^^ > >> Can we remove the SEV check ... > >>> + pc_system_parse_ovmf_flash(ptr, size); >>> + >>> + if (sev_enabled()) { >> >> ... because we are still checking SEV here. > > Well, the two checks have slightly different purposes. The first check > will probably become "if (sev || tdx)" soon, Not soon for TDX since the hacky pflash interface to load TDVF is rejected. > whereas the second will > become "if (sev) { ... } if (tdx) { ... }". > > We could remove the first. pc_system_parse_ovmf_flash() would run > unconditionally then. Not needed, but should not have any bad side > effects. > > take care, > Gerd >
On 1/4/22 07:28, Xiaoyao Li wrote: > On 4/1/2022 1:08 PM, Gerd Hoffmann wrote: >>>> if (sev_enabled()) { >>> >>> ^^^ >> >>> Can we remove the SEV check ... >> >>>> + pc_system_parse_ovmf_flash(ptr, size); >>>> + >>>> + if (sev_enabled()) { >>> >>> ... because we are still checking SEV here. >> >> Well, the two checks have slightly different purposes. The first check >> will probably become "if (sev || tdx)" soon, > > Not soon for TDX since the hacky pflash interface to load TDVF is rejected. You can still convince us you need a pflash for TDX, and particularly "a pflash that doesn't behave like pflash". Also, see the comment in the next patch of this series: + * [...] there is no need to register + * the firmware as rom to properly re-initialize on reset. + * Just go for a straight file load instead. + */ >> whereas the second will >> become "if (sev) { ... } if (tdx) { ... }". >> >> We could remove the first. pc_system_parse_ovmf_flash() would run >> unconditionally then. Not needed, but should not have any bad side >> effects. OK, then: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 4/1/2022 6:36 PM, Philippe Mathieu-Daudé wrote: > On 1/4/22 07:28, Xiaoyao Li wrote: >> On 4/1/2022 1:08 PM, Gerd Hoffmann wrote: >>>>> if (sev_enabled()) { >>>> >>>> ^^^ >>> >>>> Can we remove the SEV check ... >>> >>>>> + pc_system_parse_ovmf_flash(ptr, size); >>>>> + >>>>> + if (sev_enabled()) { >>>> >>>> ... because we are still checking SEV here. >>> >>> Well, the two checks have slightly different purposes. The first check >>> will probably become "if (sev || tdx)" soon, >> >> Not soon for TDX since the hacky pflash interface to load TDVF is >> rejected. > > You can still convince us you need a pflash for TDX, and particularly > "a pflash that doesn't behave like pflash". I'm fine with "-bios" option to load TDVF. :) > Also, see the comment in > the next patch of this series: > > + * [...] there is no need to register > + * the firmware as rom to properly re-initialize on reset. > + * Just go for a straight file load instead. > + */ Yes, Gerd's this series make it easier for TDX to load TDVF via -bios. >>> whereas the second will >>> become "if (sev) { ... } if (tdx) { ... }". >>> >>> We could remove the first. pc_system_parse_ovmf_flash() would run >>> unconditionally then. Not needed, but should not have any bad side >>> effects. > > OK, then: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h index 916cc325eeb1..4841a49f86c0 100644 --- a/include/hw/i386/x86.h +++ b/include/hw/i386/x86.h @@ -140,4 +140,7 @@ void gsi_handler(void *opaque, int n, int level); void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name); DeviceState *ioapic_init_secondary(GSIState *gsi_state); +/* pc_sysfw.c */ +void x86_firmware_configure(void *ptr, int size); + #endif diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index c8b17af95353..36b6121b77b9 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -148,7 +148,6 @@ static void pc_system_flash_map(PCMachineState *pcms, MemoryRegion *flash_mem; void *flash_ptr; int flash_size; - int ret; assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled); @@ -196,19 +195,7 @@ static void pc_system_flash_map(PCMachineState *pcms, if (sev_enabled()) { flash_ptr = memory_region_get_ram_ptr(flash_mem); flash_size = memory_region_size(flash_mem); - /* - * OVMF places a GUIDed structures in the flash, so - * search for them - */ - pc_system_parse_ovmf_flash(flash_ptr, flash_size); - - ret = sev_es_save_reset_vector(flash_ptr, flash_size); - if (ret) { - error_report("failed to locate and/or save reset vector"); - exit(1); - } - - sev_encrypt_flash(flash_ptr, flash_size, &error_fatal); + x86_firmware_configure(flash_ptr, flash_size); } } } @@ -260,3 +247,24 @@ void pc_system_firmware_init(PCMachineState *pcms, pc_system_flash_cleanup_unused(pcms); } + +void x86_firmware_configure(void *ptr, int size) +{ + int ret; + + /* + * OVMF places a GUIDed structures in the flash, so + * search for them + */ + pc_system_parse_ovmf_flash(ptr, size); + + if (sev_enabled()) { + ret = sev_es_save_reset_vector(ptr, size); + if (ret) { + error_report("failed to locate and/or save reset vector"); + exit(1); + } + + sev_encrypt_flash(ptr, size, &error_fatal); + } +}