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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

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