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);
+
+ 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.1On 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>
>
>
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 -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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
© 2016 - 2026 Red Hat, Inc.