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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 02, 2020 at 11:39:50AM +0200, Jan Beulich wrote:
> 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.

Oh sorry, I remember your comment and I've changed further patches to
address this.

The setter of the callback will be in charge for setting the callback
again on resume. That's why vlapic_set_callback is not a static
function, and is added to the header.

Patch 5/11 [0] contains an example of hw the vIO-APIC resets the callbacks
on load. 

> 
> > ---
> > 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().

Well, lets try to get some progress on the other questions and we will
eventually get here I think.

> > --- 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?

Could do, there might indeed be cases where the callback is the same
but data has changed and should be interesting to log.

Thanks, Roger.

[0] 
https://lore.kernel.org/xen-devel/20200930104108.35969-6-roger.pau@xxxxxxxxxx/