Re: [RFC PATCH v3 03/38] xen: Add XEN_DISABLED mode and make it default

[Paul Durrant]

On 20/12/2022 22:59, David Woodhouse wrote:
> On Tue, 2022-12-20 at 14:39 +0000, Paul Durrant wrote:
> > On 16/12/2022 00:40, David Woodhouse wrote:
> > > From: David Woodhouse <dwmw@amazon.co.uk>
> > >
> > > Also check for XEN_ATTACH mode in xen_init()
> > >
> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > > ---
> > >     accel/xen/xen-all.c  | 4 ++++
> > >     include/hw/xen/xen.h | 5 +++--
> > >     2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> > > index 69aa7d018b..109d2e84bc 100644
> > > --- a/accel/xen/xen-all.c
> > > +++ b/accel/xen/xen-all.c
> > > @@ -158,6 +158,10 @@ static int xen_init(MachineState *ms)
> > >     {
> > >         MachineClass *mc = MACHINE_GET_CLASS(ms);
> > >     
> > > +    if (xen_mode != XEN_ATTACH) {
> > > +        xen_pv_printf(NULL, 0, "xen requires --xen-attach mode\n");
> > > +        return -1;
> > > +    }
> >
> > This is new requirement, isn't it? Libxl only passes --xen-attach
> > for PV domains so AFAICT this will break Xen HVM domains.
>
> So HVM domains are currently running with xen_mode == XEN_EMULATE and
> work because nobody ever actually looks at that variable except in
> xen_init_pv().
>
> What was the reason for ever needing it to be explicitly set on the
> command line; couldn't it have been implicit?

Judging by the comments on the enum, it dates back to xenner. I guess it 
was needed to disambiguate use of xenner from attaching to a real Xen 
instance.
The only cases I see it being used in libxl are for starting up a qdisk 
backend when spawning a PV domU, or retrospectively for dom0.

As you say, the only place it is checked is in xen_init_pv() and even 
then it has no effect. The comment in the code says "nothing to do, 
libxl handles everything".

> Seems like the right thing to do is to make it implicit in xen_init()
> above, and *set* it to XEN_ATTACH instead of requiring it to have been
> set on the command line?

I guess, now that your reviving Xen emulation in QEMU, the mode does 
become relevant again so making XEN_ATTACH implicit when QEMU is 
launched by libxl, in all cases, is the appropriate thing to do.

  Paul