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

["Daniel P. Berrangé" <berrange@redhat.com>]

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