Re: [PATCH v2 05/11] x86/vioapic: switch to use the EOI callback mechanism

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

On Wed, Sep 30, 2020 at 01:09:21PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Roger 
> > Pau Monne
> > Sent: 30 September 2020 11:41
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>; Jan Beulich 
> > <jbeulich@xxxxxxxx>; Andrew Cooper
> > <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>
> > Subject: [PATCH v2 05/11] x86/vioapic: switch to use the EOI callback 
> > mechanism
> > 
> > Switch the emulated IO-APIC code to use the local APIC EOI callback
> > mechanism. This allows to remove the last hardcoded callback from
> > vlapic_handle_EOI. Removing the hardcoded vIO-APIC callback also
> > allows to getting rid of setting the EOI exit bitmap based on the
> > triggering mode, as now all users that require an EOI action use the
> > newly introduced callback mechanism.
> > 
> > Move and rename the vioapic_update_EOI now that it can be made static.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Changes since v1:
> >  - Remove the triggering check in the update_eoi_exit_bitmap call.
> >  - Register the vlapic callbacks when loading the vIO-APIC state.
> >  - Reduce scope of ent.
> > ---
> >  xen/arch/x86/hvm/vioapic.c | 131 ++++++++++++++++++++++++-------------
> >  xen/arch/x86/hvm/vlapic.c  |   5 +-
> >  2 files changed, 86 insertions(+), 50 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> > index 752fc410db..dce98b1479 100644
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -375,6 +375,50 @@ static const struct hvm_mmio_ops vioapic_mmio_ops = {
> >      .write = vioapic_write
> >  };
> > 
> > +static void eoi_callback(unsigned int vector, void *data)
> > +{
> > +    struct domain *d = current->domain;
> > +    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > +    unsigned int i;
> > +
> > +    ASSERT(has_vioapic(d));
> > +
> > +    spin_lock(&d->arch.hvm.irq_lock);
> > +
> > +    for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
> > +    {
> > +        struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> > +        unsigned int pin;
> > +
> > +        for ( pin = 0; pin < vioapic->nr_pins; pin++ )
> > +        {
> > +            union vioapic_redir_entry *ent = &vioapic->redirtbl[pin];
> > +
> > +            if ( ent->fields.vector != vector )
> > +                continue;
> > +
> > +            ent->fields.remote_irr = 0;
> > +
> > +            if ( is_iommu_enabled(d) )
> > +            {
> > +                spin_unlock(&d->arch.hvm.irq_lock);
> > +                hvm_dpci_eoi(vioapic->base_gsi + pin, ent);
> > +                spin_lock(&d->arch.hvm.irq_lock);
> 
> Is this safe? If so, why lock around the whole loop in the first place?

It's my understanding that locking around the whole loop is mostly
done for convenience reasons, as vioapic entries cannot go away after
initialization.

The lock here is taken to assure consistency of the contents of
vioapic_redir_entry entry, so that other concurrent read/writes don't
change the entry while being processed here - but the entry can never
disappear under our feet.

Jan expressed similar concerns on the previous version, but I'm afraid
I didn't look much at whether hvm_dpci_eoi could be called with the
lock taken, or whether we could move the call of the EOI hooks out of
the locked region. I was mostly leaving this part for later, since
the series is already fairly long and this didn't seem critical.

Thanks, Roger.