Re: [RFC PATCH 09/21] pc_piix: allow xenfv machine with XEN_EMULATE

[David Woodhouse]

On Mon, 2022-12-05 at 23:06 +0100, Philippe Mathieu-Daudé wrote:
> On 5/12/22 18:31, David Woodhouse wrote:
> > From: Joao Martins <joao.m.martins@oracle.com>
> > 
> > This allows -machine xenfv to work with Xen emulated guests.
> > 
> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >   hw/i386/pc_piix.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 3dcac2f4b6..d1127adde0 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -404,8 +404,8 @@ static void pc_xen_hvm_init(MachineState *machine)
> >   {
> >       PCMachineState *pcms = PC_MACHINE(machine);
> >   
> > -    if (!xen_enabled()) {
> > -        error_report("xenfv machine requires the xen accelerator");
> > +    if (!xen_enabled() && (xen_mode != XEN_EMULATE)) {
> > +        error_report("xenfv machine requires the xen or kvm accelerator");
> >           exit(1);
> >       }
> 
> What about the XEN_EMULATE case? Shouldn't this be:
> 
>    if (!xen_enabled()) {
>       if (xen_mode == XEN_EMULATE) {
>           error_report("xenfv machine requires the xen accelerator");
>       } else {
>           error_report("xenfv machine requires the xen or kvm accelerator");
>       }
>       exit(1);
>    }
> 
> ?

Erm... that one I cherry-picked directly from the original and I
confess I haven't yet done much thinking about it.

There are two sane cases.

If xen_mode == XEN_ATTACH, then xen_enabled() should be true.

If xen_mode == XEN_EMULATED, then we'd better be using KVM with the Xen
support (which could *theoretically* be added to TCG if anyone really
wanted to).

So this check is working because it's allowing *either* xen_enabled()
*or* xen_mode==XEN_ATTACH to satisfy it. But it's too lax. I think it
should *require* KVM in the case of XEN_EMULATE.

That ought to be sufficient since it's going to set the xen-version
machine property, and that would cause KVM itself to bail out if the
required support isn't present in the kernel.

(I'm assuming the existing XEN_EMULATE mode is long dead along with
Xenner? Even on true Xen we run PV guests in a shim these days, and
that shim works without modification under KVM and will eventually be
one of my test cases as I get this all working under qemu)

smime.p7s
Description: S/MIME cryptographic signature