Re: [PATCH v2 03/11] x86/vlapic: introduce an EOI callback mechanism

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2020 12:41, Roger Pau Monne wrote:
> Add a new vlapic_set_irq_callback helper in order to inject a vector
> and set a callback to be executed when the guest performs the end of
> interrupt acknowledgment.

On v1 I did ask

"One thing I don't understand at all for now is how these
 callbacks are going to be re-instated after migration for
 not-yet-EOIed interrupts."

Afaics I didn't get an answer on this.

> ---
> RFC: should callbacks also be executed in vlapic_do_init (which is
> called by vlapic_reset). We would need to make sure ISR and IRR
> are cleared using some kind of test and clear atomic functionality to
> make this race free.

I guess this can't be decided at this point of the series, as it
may depend on what exactly the callbacks mean to do. It may even
be that whether a callback wants to do something depends on
whether it gets called "normally" or from vlapic_do_init().

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -144,7 +144,32 @@ bool vlapic_test_irq(const struct vlapic *vlapic, 
> uint8_t vec)
>      return vlapic_test_vector(vec, &vlapic->regs->data[APIC_IRR]);
>  }
>  
> -void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> +void vlapic_set_callback(struct vlapic *vlapic, unsigned int vec,
> +                         vlapic_eoi_callback_t *callback, void *data)
> +{
> +    unsigned long flags;
> +    unsigned int index = vec - 16;
> +
> +    if ( !callback || vec < 16 || vec >= X86_NR_VECTORS )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return;
> +    }
> +
> +    spin_lock_irqsave(&vlapic->callback_lock, flags);
> +    if ( vlapic->callbacks[index].callback &&
> +         vlapic->callbacks[index].callback != callback )
> +        printk(XENLOG_G_WARNING
> +               "%pv overriding vector %#x callback %ps (%p) with %ps (%p)\n",
> +               vlapic_vcpu(vlapic), vec, vlapic->callbacks[index].callback,
> +               vlapic->callbacks[index].callback, callback, callback);
> +    vlapic->callbacks[index].callback = callback;
> +    vlapic->callbacks[index].data = data;

Should "data" perhaps also be compared in the override check above?

> @@ -1629,9 +1672,23 @@ int vlapic_init(struct vcpu *v)
>      }
>      clear_page(vlapic->regs);
>  
> +    if ( !vlapic->callbacks )
> +    {
> +        vlapic->callbacks = xmalloc_array(typeof(*vlapic->callbacks),
> +                                          X86_NR_VECTORS - 16);
> +        if ( !vlapic->callbacks )
> +        {
> +            dprintk(XENLOG_ERR, "%pv: alloc vlapic callbacks error\n", v);
> +            return -ENOMEM;
> +        }
> +    }
> +    memset(vlapic->callbacks, 0, sizeof(*vlapic->callbacks) *
> +                                 (X86_NR_VECTORS - 16));

While it resembles code earlier in this function, it widens an
existing memory leak (I'll make a patch for that one) and also
makes it appear as if this function could be called more than
once for a vCPU (maybe I'll also make a patch for this).

Jan