Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 7/18/22 4:56 AM, Andrew Cooper wrote:
> On 15/07/2022 14:10, Boris Ostrovsky wrote:
> > On 7/15/22 5:50 AM, Andrew Cooper wrote:
> > > On 15/07/2022 09:18, Jane Malalane wrote:
> > > > On 14/07/2022 00:27, Boris Ostrovsky wrote:
> > > > > >         xen_hvm_smp_init();
> > > > > >         WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
> > > > > > xen_cpu_dead_hvm));
> > > > > > diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
> > > > > > index 9d548b0c772f..be66e027ef28 100644
> > > > > > --- a/arch/x86/xen/suspend_hvm.c
> > > > > > +++ b/arch/x86/xen/suspend_hvm.c
> > > > > > @@ -5,6 +5,7 @@
> > > > > >     #include <xen/hvm.h>
> > > > > >     #include <xen/features.h>
> > > > > >     #include <xen/interface/features.h>
> > > > > > +#include <xen/events.h>
> > > > > >     #include "xen-ops.h"
> > > > > > @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
> > > > > >             xen_hvm_init_shared_info();
> > > > > >             xen_vcpu_restore();
> > > > > >         }
> > > > > > -    xen_setup_callback_vector();
> > > > > > +    if (xen_ack_upcall) {
> > > > > > +        unsigned int cpu;
> > > > > > +
> > > > > > +        for_each_online_cpu(cpu) {
> > > > > > +            xen_hvm_evtchn_upcall_vector_t op = {
> > > > > > +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
> > > > > > +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
> > > > > > +            };
> > > > > > +
> > > > > > +            BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
> > > > > > +                         &op));
> > > > > > +            /* Trick toolstack to think we are enlightened. */
> > > > > > +            if (!cpu)
> > > > > > +                BUG_ON(xen_set_callback_via(1));
> > > > > What are you trying to make the toolstack aware of? That we have *a*
> > > > > callback (either global or percpu)?
> > > > Yes, specifically for the check in libxl__domain_pvcontrol_available.
> > > And others.
> > >
> > > This is all a giant bodge, but basically a lot of tooling uses the
> > > non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
> > > Xen-aware drivers loaded or not.
> > >
> > > The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
> > > reason this doesn't explode everywhere is because the
> > > evtchn_upcall_vector registration takes priority over GSI delivery.
> > >
> > > This is decades of tech debt piled on top of tech debt.
> > Feels like it (setting the callback parameter) is something that the
> > hypervisor should do --- no need to expose guests to this.
> Sensible or not, it is the ABI.
>
> Linux still needs to work (nicely) with older Xen's in the world, and we
> can't just retrofit a change in the hypervisor which says "btw, this ABI
> we've just changed now has a side effect of modifying a field that you
> also logically own".

The hypercall has been around for a while so I understand ABI concerns there 
but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago. Why not tie 
presence of this bit to no longer having to explicitly set the callback field?


-boris