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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4071,6 +4071,52 @@ static int hvmop_flush_tlb_all(void)
>      return paging_flush_tlb(NULL) ? 0 : -ERESTART;
>  }
>  
> +static void hvm_set_evtchn_upcall_vector(struct vcpu *v, uint8_t vector)
> +{
> +    printk(XENLOG_G_INFO "%pv: upcall vector %02x\n", v, vector);
> +
> +    v->arch.hvm.evtchn_upcall_vector = vector;
> +    hvm_assert_evtchn_irq(v);

While asserting is needed during guest initiated registration, is
this really correct also when registering "behind the back" of the
guest?

> +}
> +
> +static int hvm_save_evtchn_upcall_vector(
> +    struct vcpu *v, hvm_domain_context_t *h)
> +{
> +    struct hvm_evtchn_upcall_vector upcall;
> +
> +    /* Skip record if VCPU is down or the upcall vector is not used. */
> +    if ( test_bit(_VPF_down, &v->pause_flags) )
> +        return 0;
> +    if ( v->arch.hvm.evtchn_upcall_vector == 0 )
> +        return 0;

Aren't you assigning meaning here to vector 0 which hasn't been
assigned so far? Shouldn't you check callback_via_type instead?

> +    upcall.vector = v->arch.hvm.evtchn_upcall_vector;
> +
> +    return hvm_save_entry(EVTCHN_UPCALL_VECTOR, v->vcpu_id, h, &upcall);
> +}
> +
> +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?

Jan