Re: [RFC PATCH 10/21] i386/xen: handle guest hypercalls

[Philippe Mathieu-Daudé]

+Thomas

On 6/12/22 02:10, David Woodhouse wrote:
> On Mon, 2022-12-05 at 23:11 +0100, Philippe Mathieu-Daudé wrote:
> > On 5/12/22 18:31, David Woodhouse wrote:
> > > From: Joao Martins <joao.m.martins@oracle.com>
> > >
> > > This means handling the new exit reason for Xen but still
> > > crashing on purpose. As we implement each of the hypercalls
> > > we will then return the right return code.
> > >
> > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > > [dwmw2: Add CPL to hypercall tracing, disallow hypercalls from CPL > 0]
> > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > > ---
> > >    target/i386/kvm/kvm.c    |  5 +++++
> > >    target/i386/trace-events |  3 +++
> > >    target/i386/xen.c        | 45 ++++++++++++++++++++++++++++++++++++++++
> > >    target/i386/xen.h        |  1 +
> > >    4 files changed, 54 insertions(+)
> > >
> > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > > index 4b21d03250..6396d11f1e 100644
> > > --- a/target/i386/kvm/kvm.c
> > > +++ b/target/i386/kvm/kvm.c
> > > @@ -5468,6 +5468,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
> > > *run)
> > >            assert(run->msr.reason == KVM_MSR_EXIT_REASON_FILTER);
> > >            ret = kvm_handle_wrmsr(cpu, run);
> > >            break;
> > > +#ifdef CONFIG_XEN
> >
> > CONFIG_XEN is set when the _host_ has Xen development files available.
> >
> > IIUC here you want to check if Xen HVM guest support is enabled.
> >
> > You might want to use a different CONFIG_XEN_xxx key, which itself
> > depends on CONFIG_XEN.
>
> Yeah, I'd be interested in opinions on that one.
>
> Strictly, the only one that *needs* to be a configure option is
> CONFIG_XEN for the Xen libraries, which is support for actually running
> on Xen.
>
> Any time KVM is present, we *could* pull in the rest of the xenfv
> machine support unconditionally, since that's no longer dependent on
> true Xen.
>
> But because there's a non-trivial amount of code in the event channel
> and grant table stuff, *perhaps* we want to make it optional? I don't
> really want to call that CONFIG_KVM_XEN since as noted, it's
> theoretically possible to do it with TCG or other accelerators too. So
> we could call it CONFIG_XEN_EMULATION.

I concur CONFIG_KVM_XEN is confusing; CONFIG_XEN_EMULATION / 
CONFIG_XEN_EMU sounds better.

Is it useful to have the CONFIG_XEN_EMU code under target/i386/ built
without having the xenfv machine built in?

I rather have hw/ and target/ features disentangled, so I'd use
CONFIG_XEN_EMU under target/ and CONFIG_XENFV_MACHINE under hw/,
eventually having CONFIG_XEN_EMU depending on CONFIG_XENFV_MACHINE
and -- for now -- CONFIG_KVM.

> I don't think we'd make that depend on CONFIG_XEN though, since none of
> the actual Xen libraries would be needed once everything's implemented
> and cleaned up.

Agreed.

> So things like the xenfv machine code would then depend on
> (CONFIG_XEN || CONFIG_XEN_EMULATION)... or we could make a new
> automatic config symbol CONFIG_XEN_MACHINE which has the same effect?

So per what you just cleared, not CONFIG_XEN but CONFIG_KVM.