Re: [PATCH] x86/hvm: save/restore per-VCPU event channel upcall vector

[David Vrabel <dvrabel@xxxxxxxxxx>]

On 06/01/2022 16:48, Jan Beulich wrote:
> On 06.01.2022 16:54, David Vrabel wrote:
> > The Windows XENBUS driver sets the per-VCPU LAPIC vector for event
> > channel interrupts using the HVMOP_set_evtchn_upcall_vector hypercall
> > (rather than using a vector-type callback in the CALLBACK_IRQ HVM
> > parameter since the vectors might be different for different VCPUs).
> >
> > This state needs to be saved/restored or a restored guest may not be
> > able to get an event channel interrupts.
> >
> > Note that the Windows XENBUS driver workarounds this by reissuing the
> > hypercall when resuming after a migration, but this workaround would
> > not be possible in an guest transparent live migration or a live
> > update.
> Why would this be needed only for this one form of "via"? And how do
> you guarantee no observable change in behavior for existing guests?
> It would seem to me that this information is something to be saved /
> restored _only_ during transparent migration, as aware guests may
> deliberately defer re-registration.
I was under the impression that the HVM parameters (including 
CALLBACK_IRQ) were saved/restored so the intent here was to save/restore 
all event channel delivery mechanism state consistently but it seems I 
was confused by some internal work that's not upstream yet.
So, I agree, this patch on it's own doesn't make sense.

I've sent a patch reserving another save record ID instead.

> > +static int hvm_load_evtchn_upcall_vector(
> > +    struct domain *d, hvm_domain_context_t *h)
> > +{
> > +    unsigned int vcpuid;
> > +    struct vcpu *v;
> > +    struct hvm_evtchn_upcall_vector upcall;
> > +
> > +    vcpuid = hvm_load_instance(h);
> > +    if ( (v = domain_vcpu(d, vcpuid)) == NULL )
> > +        return -EINVAL;
> > +
> > +    if ( hvm_load_entry(EVTCHN_UPCALL_VECTOR, h, &upcall) != 0 )
> > +        return -EINVAL;
> > +
> > +    hvm_set_evtchn_upcall_vector(v, upcall.vector);
> > +
> > +    return 0;
> > +}
> Don't you need to also set callback_via_type accordingly?
The callback_via_type is ignored if a per-vcpu upcall vector is set.

You can use a mix of a CALLBACK_IRQ defined upcalls on some VCPUs, and a 
HVMOP_set_evtchn_upcall_vector defined one on others, so setting the 
per-domain type wouldn't work.
I'm not sure why you would do this, but the ABI (as implemented, if not 
intentionally) allows it...
David