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.

I totally agree, but check this out:
xen/include/asm-arm/irq.h:#define nr_irqs NR_IRQS

So wherever you write nr_irqs in *any* part of ARM IRQ code you end up
with a compile error ...
Not easy to fix, though, hence I moved to the name without the
underscore, even though I don't really like it.

Cheers,
Andre.

> 
> 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.
> 
>> +
>> +    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