[PATCH 0/3] i386: firmware parsing and sev setup for -bios loaded firmware

[PATCH 0/3] i386: firmware parsing and sev setup for -bios loaded firmware

[Gerd Hoffmann]

Gerd Hoffmann (3):
  i386: move bios load error message
  i386: factor out x86_firmware_configure()
  i386: firmware parsing and sev setup for -bios loaded firmware

 include/hw/i386/x86.h |  3 +++
 hw/i386/pc_sysfw.c    | 36 ++++++++++++++++++++++--------------
 hw/i386/x86.c         | 32 ++++++++++++++++++++++++--------
 3 files changed, 49 insertions(+), 22 deletions(-)

-- 
2.35.1

[PATCH 1/3] i386: move bios load error message

[Gerd Hoffmann]

Switch to usual goto-end-of-function error handling style.
No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 hw/i386/x86.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 4cf107baea34..b2e801a8720e 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1121,9 +1121,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
     }
     ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
     if (ret != 0) {
-    bios_error:
-        fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
-        exit(1);
+        goto bios_error;
     }
     g_free(filename);
 
@@ -1144,6 +1142,11 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
     memory_region_add_subregion(rom_memory,
                                 (uint32_t)(-bios_size),
                                 bios);
+    return;
+
+bios_error:
+    fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
+    exit(1);
 }
 
 bool x86_machine_is_smm_enabled(const X86MachineState *x86ms)
-- 
2.35.1

[PATCH 2/3] i386: factor out x86_firmware_configure()

[Gerd Hoffmann]

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.1

[PATCH 3/3] i386: firmware parsing and sev setup for -bios loaded firmware

[Gerd Hoffmann]

Don't register firmware as rom, not needed (see comment).
Add x86_firmware_configure() call for proper sev initialization.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 hw/i386/x86.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b2e801a8720e..f98483c7fe83 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1116,12 +1116,25 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
     }
     bios = g_malloc(sizeof(*bios));
     memory_region_init_ram(bios, NULL, "pc.bios", bios_size, &error_fatal);
-    if (!isapc_ram_fw) {
-        memory_region_set_readonly(bios, true);
-    }
-    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
-    if (ret != 0) {
-        goto bios_error;
+    if (sev_enabled()) {
+        /*
+         * The concept of a "reset" simply doesn't exist for
+         * confidential computing guests, we have to destroy and
+         * re-launch them instead.  So there is no need to register
+         * the firmware as rom to properly re-initialize on reset.
+         * Just go for a straight file load instead.
+         */
+        void *ptr = memory_region_get_ram_ptr(bios);
+        load_image_size(filename, ptr, bios_size);
+        x86_firmware_configure(ptr, bios_size);
+    } else {
+        if (!isapc_ram_fw) {
+            memory_region_set_readonly(bios, true);
+        }
+        ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
+        if (ret != 0) {
+            goto bios_error;
+        }
     }
     g_free(filename);
 
-- 
2.35.1

Re: [PATCH 1/3] i386: move bios load error message

[Daniel P. Berrangé]

On Thu, Mar 31, 2022 at 10:35:47AM +0200, Gerd Hoffmann wrote:
> Switch to usual goto-end-of-function error handling style.
> No functional change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Tested-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  hw/i386/x86.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 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 :|

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Daniel P. Berrangé]

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 :|

Re: [PATCH 3/3] i386: firmware parsing and sev setup for -bios loaded firmware

[Daniel P. Berrangé]

On Thu, Mar 31, 2022 at 10:35:49AM +0200, Gerd Hoffmann wrote:
> Don't register firmware as rom, not needed (see comment).
> Add x86_firmware_configure() call for proper sev initialization.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Tested-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  hw/i386/x86.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

I validated that I could validate the measurement of a SEV
guest with -bios, and see the firmware start at least.

Tested-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b2e801a8720e..f98483c7fe83 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1116,12 +1116,25 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
>      }
>      bios = g_malloc(sizeof(*bios));
>      memory_region_init_ram(bios, NULL, "pc.bios", bios_size, &error_fatal);
> -    if (!isapc_ram_fw) {
> -        memory_region_set_readonly(bios, true);
> -    }
> -    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
> -    if (ret != 0) {
> -        goto bios_error;
> +    if (sev_enabled()) {
> +        /*
> +         * The concept of a "reset" simply doesn't exist for
> +         * confidential computing guests, we have to destroy and
> +         * re-launch them instead.  So there is no need to register
> +         * the firmware as rom to properly re-initialize on reset.
> +         * Just go for a straight file load instead.
> +         */
> +        void *ptr = memory_region_get_ram_ptr(bios);
> +        load_image_size(filename, ptr, bios_size);
> +        x86_firmware_configure(ptr, bios_size);
> +    } else {
> +        if (!isapc_ram_fw) {
> +            memory_region_set_readonly(bios, true);
> +        }
> +        ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
> +        if (ret != 0) {
> +            goto bios_error;
> +        }
>      }
>      g_free(filename);
>  
> -- 
> 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 :|

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Gerd Hoffmann]

  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

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Daniel P. Berrangé]

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 :|

Re: [PATCH 3/3] i386: firmware parsing and sev setup for -bios loaded firmware

[Gerd Hoffmann]

On Thu, Mar 31, 2022 at 02:10:04PM +0100, Daniel P. Berrangé wrote:
> On Thu, Mar 31, 2022 at 10:35:49AM +0200, Gerd Hoffmann wrote:
> > Don't register firmware as rom, not needed (see comment).
> > Add x86_firmware_configure() call for proper sev initialization.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Tested-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> >  hw/i386/x86.c | 25 +++++++++++++++++++------
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> I validated that I could validate the measurement of a SEV
> guest with -bios, and see the firmware start at least.
> 
> Tested-by: Daniel P. Berrangé <berrange@redhat.com>

Nice.

With that in place we should be pretty close to get sev going with
microvm.  Probably needs revert of edk2 commit 06fa1f1931e9
("OvmfPkg/Microvm: no sev"), with some luck direct kernel boot works
without that though.

take care,
  Gerd

Re: [PATCH 0/3] i386: firmware parsing and sev setup for -bios loaded firmware

[Michael S. Tsirkin]

On Thu, Mar 31, 2022 at 10:35:46AM +0200, Gerd Hoffmann wrote:
> 


Looks good.
Acked-by: Michael S. Tsirkin <mst@redhat.com>


Who's merging this? Yourself?

> Gerd Hoffmann (3):
>   i386: move bios load error message
>   i386: factor out x86_firmware_configure()
>   i386: firmware parsing and sev setup for -bios loaded firmware
> 
>  include/hw/i386/x86.h |  3 +++
>  hw/i386/pc_sysfw.c    | 36 ++++++++++++++++++++++--------------
>  hw/i386/x86.c         | 32 ++++++++++++++++++++++++--------
>  3 files changed, 49 insertions(+), 22 deletions(-)
> 
> -- 
> 2.35.1
> 

Re: [PATCH 1/3] i386: move bios load error message

[Philippe Mathieu-Daudé]

On 31/3/22 10:35, Gerd Hoffmann wrote:
> Switch to usual goto-end-of-function error handling style.
> No functional change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Tested-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   hw/i386/x86.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 3/3] i386: firmware parsing and sev setup for -bios loaded firmware

[Philippe Mathieu-Daudé]

On 31/3/22 10:35, Gerd Hoffmann wrote:
> Don't register firmware as rom, not needed (see comment).
> Add x86_firmware_configure() call for proper sev initialization.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Tested-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   hw/i386/x86.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Philippe Mathieu-Daudé]

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);
> +    }
> +}

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Gerd Hoffmann]

> >               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

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Xiaoyao Li]

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
> 

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 2/3] i386: factor out x86_firmware_configure()

[Xiaoyao Li]

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>
> 
> 

Re: [PATCH 0/3] i386: firmware parsing and sev setup for -bios loaded firmware

[Gerd Hoffmann]

On Thu, Mar 31, 2022 at 04:24:28PM -0400, Michael S. Tsirkin wrote:
> On Thu, Mar 31, 2022 at 10:35:46AM +0200, Gerd Hoffmann wrote:
> > 
> 
> 
> Looks good.
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> Who's merging this? Yourself?

Just posted v2 with all the reviews, tests and acks added.

Will probably send a pull request in a week or so.  But I don't have
anything else pending right now, so I wouldn't mind if someone else
picks this up ...

take care,
  Gerd