Re: [PATCH 37/57] ARM: new VGIC: Add ENABLE registers handlers

[Julien Grall <julien.grall@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 11926 bytes --]

(sorry for the formatting)

On Wed, 7 Mar 2018, 18:23 Andre Przywara, <andre.przywara@linaro.org> wrote:

> Hi,
>
> On 07/03/18 17:01, Julien Grall wrote:
> > Hi Andre,
> >
> > On 03/05/2018 04:03 PM, Andre Przywara wrote:
> >> As the enable register handlers are shared between the v2 and v3
> >> emulation, their implementation goes into vgic-mmio.c, to be easily
> >> referenced from the v3 emulation as well later.
> >> This introduces a vgic_sync_hardware_irq() function, which updates the
> >> physical side of a hardware mapped virtual IRQ.
> >> Because the existing locking order between vgic_irq->irq_lock and
> >> irq_desc->lock dictates so, we dropu the irq_lock and retake them in the
> >> proper order.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >> ---
> >> Changelog RFC ... v1:
> >> - extend and move vgic_sync_hardware_irq()
> >> - do proper locking sequence
> >> - skip already disabled/enabled IRQs
> >>
> >>   xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
> >>   xen/arch/arm/vgic/vgic-mmio.c    | 117
> >> +++++++++++++++++++++++++++++++++++++++
> >>   xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
> >>   xen/arch/arm/vgic/vgic.c         |  38 +++++++++++++
> >>   xen/arch/arm/vgic/vgic.h         |   3 +
> >>   5 files changed, 171 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> index 2e015ed0b1..3dd983f885 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> @@ -80,10 +80,10 @@ static const struct vgic_register_region
> >> vgic_v2_dist_registers[] = {
> >>           vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> >>           VGIC_ACCESS_32bit),
> >>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
> >> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> +        vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
> >>           VGIC_ACCESS_32bit),
> >>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
> >> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> +        vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> >>           VGIC_ACCESS_32bit),
> >>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
> >>           vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio.c
> >> b/xen/arch/arm/vgic/vgic-mmio.c
> >> index 284a92d288..f8f0252eff 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> >> @@ -39,6 +39,123 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t
> >> addr,
> >>       /* Ignore */
> >>   }
> >>   +/*
> >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the
> >> value
> >> + * of the enabled bit, so there is only one function for both here.
> >> + */
> >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
> >> +                                    paddr_t addr, unsigned int len)
> >> +{
> >> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> +    uint32_t value = 0;
> >> +    unsigned int i;
> >> +
> >> +    /* Loop over all IRQs affected by this read */
> >> +    for ( i = 0; i < len * 8; i++ )
> >> +    {
> >> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid
> >> + i);
> >> +
> >> +        if ( irq->enabled )
> >> +            value |= (1U << i);
> >> +
> >> +        vgic_put_irq(vcpu->domain, irq);
> >> +    }
> >> +
> >> +    return value;
> >> +}
> >> +
> >> +void vgic_mmio_write_senable(struct vcpu *vcpu,
> >> +                             paddr_t addr, unsigned int len,
> >> +                             unsigned long val)
> >> +{
> >> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> +    unsigned int i;
> >> +
> >> +    for_each_set_bit( i, &val, len * 8 )
> >> +    {
> >> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid
> >> + i);
> >> +        unsigned long flags;
> >> +        irq_desc_t *desc;
> >> +
> >> +        spin_lock_irqsave(&irq->irq_lock, flags);
> >> +
> >> +        if ( irq->enabled )            /* skip already enabled IRQs */
> >> +        {
> >> +            spin_unlock_irqrestore(&irq->irq_lock, flags);
> >> +            vgic_put_irq(vcpu->domain, irq);
> >> +            continue;
> >> +        }
> >> +
> >> +        irq->enabled = true;
> >> +        if ( irq->hw )
> >> +        {
> >> +            /*
> >> +             * The irq cannot be a PPI, we only support delivery
> >> +             * of SPIs to guests.
> >> +             */
> >> +            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
> >> +
> >> +            desc = irq_to_desc(irq->hwintid);
> >> +        }
> >> +        else
> >> +            desc = NULL;
> >
> > You could just initialize desc to NULL at the declaration time and drop
> > the else part.
>
> Can we rely on the initializer to be called on every loop iteration? I
> wasn't sure about this and what the standard has to say about this.
>

Every loop is a new scope. So everything declared within that scope is
initialized again. We do use that extensively in Xen.



> >> +
> >> +        vgic_queue_irq_unlock(vcpu->domain, irq, flags);
> >> +
> >> +        if ( desc )
> >> +            vgic_sync_hardware_irq(vcpu->domain, desc, irq);
> >
> > A comment explaining why desc is done outside the locking would be
> > useful. This would avoid to loose time using git blame.
> >
> >> +
> >> +        vgic_put_irq(vcpu->domain, irq);
> >> +    }
> >> +}
> >> +
> >> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
> >> +                 paddr_t addr, unsigned int len,
> >> +                 unsigned long val)
> >> +{
> >> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> +    unsigned int i;
> >> +
> >> +    for_each_set_bit( i, &val, len * 8 )
> >> +    {
> >> +        struct vgic_irq *irq;
> >> +        unsigned long flags;
> >> +        irq_desc_t *desc;
> >> +
> >> +        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> >> +        spin_lock_irqsave(&irq->irq_lock, flags);
> >> +
> >> +        if ( !irq->enabled )            /* skip already disabled IRQs
> */
> >> +        {
> >> +            spin_unlock_irqrestore(&irq->irq_lock, flags);
> >> +            vgic_put_irq(vcpu->domain, irq);
> >> +            continue;
> >> +        }
> >> +
> >> +        irq->enabled = false;
> >> +
> >> +        if ( irq->hw )
> >> +        {
> >> +            /*
> >> +             * The irq cannot be a PPI, we only support delivery
> >> +             * of SPIs to guests.
> >> +             */
> >> +            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
> >> +
> >> +            desc = irq_to_desc(irq->hwintid);
> >> +        }
> >> +        else
> >> +            desc = NULL;
> >> +
> >> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
> >> +
> >> +        if ( desc )
> >> +            vgic_sync_hardware_irq(vcpu->domain, desc, irq);
> >
> > Ditto.
> >
> >> +
> >> +        vgic_put_irq(vcpu->domain, irq);
> >> +    }
> >> +}
> >> +
> >>   static int match_region(const void *key, const void *elt)
> >>   {
> >>       const unsigned int offset = (unsigned long)key;
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio.h
> >> b/xen/arch/arm/vgic/vgic-mmio.h
> >> index 621b9a281c..2ddcbbf58d 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio.h
> >> +++ b/xen/arch/arm/vgic/vgic-mmio.h
> >> @@ -96,6 +96,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
> >>   void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
> >>                           unsigned int len, unsigned long val);
> >>   +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
> >> +                                    paddr_t addr, unsigned int len);
> >> +
> >> +void vgic_mmio_write_senable(struct vcpu *vcpu,
> >> +                             paddr_t addr, unsigned int len,
> >> +                             unsigned long val);
> >> +
> >> +void vgic_mmio_write_cenable(struct vcpu *vcpu,
> >> +                             paddr_t addr, unsigned int len,
> >> +                             unsigned long val);
> >> +
> >>   unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
> >>     #endif
> >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> >> index 465a95f415..5246d7c2e7 100644
> >> --- a/xen/arch/arm/vgic/vgic.c
> >> +++ b/xen/arch/arm/vgic/vgic.c
> >> @@ -698,6 +698,44 @@ void vgic_kick_vcpus(struct domain *d)
> >>       }
> >>   }
> >>   +static unsigned int translate_irq_type(bool is_level)
> >> +{
> >> +    return is_level ? IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING;
> >> +}
> >> +
> >> +void vgic_sync_hardware_irq(struct domain *d,
> >> +                            irq_desc_t *desc, struct vgic_irq *irq)
> >> +{
> >> +    unsigned long flags;
> >> +
> >> +    spin_lock_irqsave(&desc->lock, flags);
> >> +    spin_lock(&irq->irq_lock);
> >> +
> >> +    /* Is that association actually still valid? (we entered with no
> >> locks) */
> >
> > If the association is not valid, then you need to fetch the new desc.
> > Right?
>
> I am not so sure it's that easy. If the association changed, then the
> whole reason of this call might have become invalid. So I rather bail
> out here and do nothing. The check is just to prevent doing the wrong
> thing, not necessarily to always do the right thing.
> To be honest this whole "lock drop dance" is just to cope with the
> locking order, which I consider wrong, according to my gut feeling.
>

If you don't do the dance here, you would have to do in other place. I
still think taking the desc->lock first is the right thing to do as Xen
deal with physical first then it might be a virtual (so second) or handled
by a driver.



> This function here is called from several places, so it seems a bit
> fragile to assume a way how to fix a broken association here. I can go
> back and check every existing caller in this respect, but to be honest
> I'd rather change the locking order, so we don't need to worry about
> this. But I feel like we should do this as a fixup on top later.
>

See some thought in the next patch. We might be able to simplify the whole
logic by forbidding the interrupt to be removed.



> Cheers,
> Andre.
>
>
> >
> >> +    if ( desc->irq == irq->hwintid )
> >> +    {
> >> +        if ( irq->enabled )
> >> +        {
> >> +            /*
> >> +             * We might end up from various callers, so check that the
> >> +             * interrrupt is disabled before trying to change the
> >> config.
> >> +             */
> >> +            if ( irq_type_set_by_domain(d) &&
> >> +                 test_bit(_IRQ_DISABLED, &desc->status) )
> >> +                gic_set_irq_type(desc,
> translate_irq_type(irq->config));
> >> +
> >> +            if ( irq->target_vcpu )
> >> +                irq_set_affinity(desc,
> >> cpumask_of(irq->target_vcpu->processor));
> >> +            desc->handler->enable(desc);
> >> +        }
> >> +        else
> >> +            desc->handler->disable(desc);
> >> +    }
> >> +
> >> +    spin_unlock(&irq->irq_lock);
> >> +    spin_unlock_irqrestore(&desc->lock, flags);
> >> +}
> >> +
> >>   /*
> >>    * Local variables:
> >>    * mode: C
> >> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> >> index 588bd067b7..68e205d10a 100644
> >> --- a/xen/arch/arm/vgic/vgic.h
> >> +++ b/xen/arch/arm/vgic/vgic.h
> >> @@ -50,6 +50,9 @@ static inline void vgic_get_irq_kref(struct vgic_irq
> >> *irq)
> >>       atomic_inc(&irq->refcount);
> >>   }
> >>   +void vgic_sync_hardware_irq(struct domain *d,
> >> +                            irq_desc_t *desc, struct vgic_irq *irq);
> >> +
> >>   void vgic_v2_fold_lr_state(struct vcpu *vcpu);
> >>   void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq *irq,
> >> int lr);
> >>   void vgic_v2_set_underflow(struct vcpu *vcpu);
> >>
> >
> > Cheers,
> >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 17102 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel