Re: [PATCH 2/2] hw/i386: support loading OVMF using -bios too

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 04:55:11PM +0000, Daniel P. Berrangé wrote:
> Traditionally the OVMF firmware has been loaded using the pflash
> mechanism. This is because it is usually provided as a pair of
> files, one read-only containing the code and one writable to
> provided persistence of non-volatile firmware variables.
> 
> The AMD SEV build of EDK2, however, is provided as a single
> file that contains only the code. This is intended to be used
> read-only and explicitly does not provide any ability for
> persistance of non-volatile firmware variables. While it is
> possible to configure this with the pflash mechanism, by only
> providing one of the 2 pflash blobs, conceptually it is a
> little strange to use pflash if there won't be any persistent
> data.
> 
> A stateless OVMF build can be loaded with -bios, however, QEMU
> does not currently initialize SEV in that scenario. This patch
> introduces the call needed for SEV initialization of the
> firmware.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/i386/x86.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb..c79d84936f 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -45,6 +45,7 @@
>  #include "target/i386/cpu.h"
>  #include "hw/i386/topology.h"
>  #include "hw/i386/fw_cfg.h"
> +#include "hw/i386/pc.h"
>  #include "hw/intc/i8259.h"
>  #include "hw/rtc/mc146818rtc.h"
>  #include "target/i386/sev.h"

This builds fine because there's a stub in pc_sysfw_ovmf-stubs.c

The unfortunate thing about this however is that it's too easy to pull
in a PC dependency, and people building with CONFIG_PC will not notice
until it breaks for others.

Is it time we split pc.h further and had pc_sysfw_ovmf.h ?

> @@ -1157,6 +1158,10 @@ void x86_bios_rom_init(MachineState *ms, const char 
> *default_firmware,
>      memory_region_add_subregion(rom_memory,
>                                  (uint32_t)(-bios_size),
>                                  bios);
> +
> +    pc_system_ovmf_initialize_sev(
> +        rom_ptr((uint32_t)-bios_size, bios_size),
> +        bios_size);

Just curious about the formatting here:

    pc_system_ovmf_initialize_sev(rom_ptr((uint32_t)-bios_size, bios_size),
                                  bios_size);

would be prettier ...

>  }
>  
>  bool x86_machine_is_smm_enabled(const X86MachineState *x86ms)
> -- 
> 2.33.1