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

[Philippe Mathieu-Daudé]

On 14/1/22 19:07, Michael S. Tsirkin wrote:
> 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.

It certainly would be simpler to have a ROM for the CODE part.
IIUC using CFI pflash allows the firmware to poll the underlying
device size.

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

While "pc*" is specific to the PC machines, "x86*" is shared between
PC and microvm. "pc.h" must not be included in "x86.c". The shared
method introduced in the previous patch becomes
x86_system_ovmf_initialize_sev(). The dual pflash mechanism is proper
to OVMF, so having this method in "x86.h" seems correct.

Phil.