Re: [Xen-devel] [RFC PATCH v2 07/22] ARM: vGIC: introduce priority setter/getter

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 11/08/17 15:10, Julien Grall wrote:
> Hi Andre,
> 
> On 21/07/17 20:59, Andre Przywara wrote:
>> Since the GICs MMIO access always covers a number of IRQs at once,
>> introduce wrapper functions which loop over those IRQs, take their
>> locks and read or update the priority values.
>> This will be used in a later patch.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  xen/arch/arm/vgic.c        | 37 +++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/vgic.h |  5 +++++
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 434b7e2..b2c9632 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -243,6 +243,43 @@ static int vgic_get_virq_priority(struct vcpu *v,
>> unsigned int virq)
>>      return ACCESS_ONCE(rank->priority[virq & INTERRUPT_RANK_MASK]);
>>  }
>>
>> +#define MAX_IRQS_PER_IPRIORITYR 4
> 
> The name gives the impression that you may have IPRIORITYR with only 1
> IRQ. But this is not true. The registers is always 4. However, you are
> able to access using byte or word.
> 
>> +uint32_t vgic_fetch_irq_priority(struct vcpu *v, unsigned int nrirqs,
> 
> I am well aware that the vgic code is mixing between virq and irq.
> Moving forward, we should use virq to avoid confusion.
> 
>> +                                 unsigned int first_irq)
> 
> Please stay consistent, with the naming. Either nr_irqs/first_irq or
> nrirqs/firstirq. But not a mix.
> 
> Also, it makes more sense to describe first the start then number.
> 
>> +{
>> +    struct pending_irq *pirqs[MAX_IRQS_PER_IPRIORITYR];
>> +    unsigned long flags;
>> +    uint32_t ret = 0, i;
>> +
>> +    local_irq_save(flags);
>> +    vgic_lock_irqs(v, nrirqs, first_irq, pirqs);
> 
> I am not convinced on the usefulness of taking all the locks in one go.
> At one point in the time, you only need to lock a given pending_irq.

I don't think so. The MMIO access a guest does is expected to be atomic,
so it expects to read the priorities of the four interrupts as they were
*at one point in time*.
This issue is more obvious for the enabled bit, for instance, but also
here a (32-bit) read and a write of some IPRIORITYR might race against
each other. This was covered by the rank lock before, but now we have to
bite the bullet and lock all involved IRQs.

Cheers,
Andre.

>> +
>> +    for ( i = 0; i < nrirqs; i++ )
>> +        ret |= pirqs[i]->priority << (i * 8);
> 
> Please avoid open-coding number.
> 
>> +
>> +    vgic_unlock_irqs(pirqs, nrirqs);
>> +    local_irq_restore(flags);
>> +
>> +    return ret;
>> +}
>> +
>> +void vgic_store_irq_priority(struct vcpu *v, unsigned int nrirqs,
>> +                             unsigned int first_irq, uint32_t value)
>> +{
>> +    struct pending_irq *pirqs[MAX_IRQS_PER_IPRIORITYR];
>> +    unsigned long flags;
>> +    unsigned int i;
>> +
>> +    local_irq_save(flags);
>> +    vgic_lock_irqs(v, nrirqs, first_irq, pirqs);
>> +
>> +    for ( i = 0; i < nrirqs; i++, value >>= 8 )
> 
> Same here.
> 
>> +        pirqs[i]->priority = value & 0xff;
>> +
>> +    vgic_unlock_irqs(pirqs, nrirqs);
>> +    local_irq_restore(flags);
>> +}
>> +
>>  bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned
>> int irq)
>>  {
>>      unsigned long flags;
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index ecf4969..f3791c8 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -198,6 +198,11 @@ void vgic_lock_irqs(struct vcpu *v, unsigned int
>> nrirqs, unsigned int first_irq,
>>                      struct pending_irq **pirqs);
>>  void vgic_unlock_irqs(struct pending_irq **pirqs, unsigned int nrirqs);
>>
>> +uint32_t vgic_fetch_irq_priority(struct vcpu *v, unsigned int nrirqs,
>> +                                 unsigned int first_irq);
>> +void vgic_store_irq_priority(struct vcpu *v, unsigned int nrirqs,
>> +                             unsigned int first_irq, uint32_t reg);
>> +
>>  enum gic_sgi_mode;
>>
>>  /*
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel