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

[Julien Grall <julien.grall@xxxxxxxxx>]

(sorry for the formatting)

On Wed, 7 Mar 2018, 18:23 Andre Przywara, <andre.przywara@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

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