Re: [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> Xen is currently directly storing the value of register GICD_IPRIORITYR

"of the GICD_... register"

> in the rank. This makes emulation of the register access very simple
> but makes the code to get the priority for a given IRQ more complex.
> 
> While the priority of an IRQ is retrieved everytime an IRQ is injected

"every time".

> to the guest, the access to register occurs less often.
> 
> So the data structure should be optimized for the most common case
> rather than the inverse.
> 
> This patch introduces the usage of an array to store the priority for
> every interrupt in the rank. This will make the code to get the priority
> very quick. The emulation code will now have to generate the
> GICD_PRIORITYR
> register for read access and split it to store in a convenient way.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
> 
> The real reason is I'd like to drop vgic_byte_* helpers in favors of more
> generic access helper. Although it would make the code to retrieve the
> priority more complex. So reworking the way to get the priority was the
> best solution.
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/vgic-v2.c     | 24 ++++++++++++++----------
>  xen/arch/arm/vgic-v3.c     | 27 ++++++++++++++++-----------
>  xen/arch/arm/vgic.c        | 46
> ++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vgic.h | 18 +++++++++++++++++-
>  4 files changed, 93 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 47f9da9..23d1982 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -139,8 +139,8 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
>          if ( rank == NULL) goto read_as_zero;
>  
>          vgic_lock_rank(v, rank, flags);
> -        *r = rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
> -                                            DABT_WORD)];
> +        /* Recreate the IPRIORITYR register */
> +        *r = vgic_generate_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR);
>          if ( dabt.size == DABT_BYTE )
>              *r = vgic_byte_read(*r, gicd_reg);
>          vgic_unlock_rank(v, rank, flags);
> @@ -400,18 +400,25 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>      }
>  
>      case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
> +    {
> +        uint32_t ipriorityr;
> +
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
>          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
>          if ( rank == NULL) goto write_ignore;
>          vgic_lock_rank(v, rank, flags);
>          if ( dabt.size == DABT_WORD )
> -            rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
> -                                           DABT_WORD)] = r;
> +            ipriorityr = r;
>          else
> -            vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
> -                        gicd_reg - GICD_IPRIORITYR, DABT_WORD)], r, gicd_reg);
> +        {
> +            ipriorityr = vgic_generate_ipriorityr(rank,
> +                                                  gicd_reg - GICD_IPRIORITYR);
> +            vgic_byte_write(&ipriorityr, r, gicd_reg);
> +        }
> +        vgic_store_ipriorityr(rank, gicd_reg - GICD_IPRIORITYR, ipriorityr);
>          vgic_unlock_rank(v, rank, flags);
>          return 1;
> +    }
>  
>      case GICD_ICFGR: /* SGIs */
>          goto write_ignore_32;
> @@ -516,14 +523,11 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
>  
>  static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
>  {
> -    int priority;
>      struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>  
>      ASSERT(spin_is_locked(&rank->lock));
> -    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
> -                                              irq, DABT_WORD)], irq & 0x3);
>  
> -    return priority;
> +    return rank->priority[irq & INTERRUPT_RANK_MASK];
>  }
>  
>  static int vgic_v2_vcpu_init(struct vcpu *v)
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index c013200..2787507 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -430,18 +430,26 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>          return 0;
>  
>      case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
> +    {
> +        uint32_t ipriorityr;
> +
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
>          rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
> -        if ( rank == NULL ) goto write_ignore;
> +        if ( rank == NULL) goto write_ignore;
> +
>          vgic_lock_rank(v, rank, flags);
>          if ( dabt.size == DABT_WORD )
> -            rank->ipriority[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
> -                                           DABT_WORD)] = r;
> +            ipriorityr = r;
>          else
> -            vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
> -                       reg - GICD_IPRIORITYR, DABT_WORD)], r, reg);
> +        {
> +            ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
> +            vgic_byte_write(&ipriorityr, r, reg);
> +        }
> +        vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);

Given the underlying change to the datastructure I think you should instead
supply a helper to set the priority of a given IRQ and use that for the
DABT_BYTE case.

For the DABT_WORD case your existing store helper would become 4 calls to
the byte helper.

That would be better than generating the full 32-bit value,  masking in the
updated byte and then deconstructing the word again.

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a6835a8..50ad360 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -61,6 +61,52 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
>      return vgic_get_rank(v, rank);
>  }
>  
> +#define NR_PRIORITY_PER_REG 4U
> +#define NR_BIT_PER_PRIORITY 8U
> +
> +/*
> + * Generate the associated IPRIORITYR register based on an offset in the rank.
> + * Note the offset will be round down to match a real HW register.

"rounded".

>  struct vgic_irq_rank {
>      spinlock_t lock; /* Covers access to all other members of this struct */
>      uint32_t ienable;
>      uint32_t icfg[2];
> -    uint32_t ipriority[8];
> +
> +    /*
> +     * It's more convenient to store one priority per interrupt than
> +     * the register IPRIORITYR itself
> +     */
> +    uint8_t priority[32];

Does:

    union {
        uint32_t ipriorityr[8];
        uint8_t priority[32];
    }

Get us the best of both worlds, or are we stymied by endianess?

Ian.