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

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 28/09/15 11:50, Ian Campbell wrote:
>> 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.

We discussed it IRL, so I will summarize here for everyone IRL.

In a follow-up patch (#7), the code to emulate IPRIORITYR will call
vgic_reg32_write which is a generic helper to handle any access size.
The goal is to drop any access size handling in the code. After patch
#7, the code will look like:

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;

vgic_lock_rank(v, rank, flags);
ipriorityr = vgic_generate_ipriorityr(rank, reg - GICD_IPRIORITYR);
vgic_reg32_write(&ipriorityr, r, info);
vgic_store_ipriorityr(rank, reg - GICD_IPRIORITYR, ipriorityr);
vgic_unlock_rank(v, rank, flags);

Introducing code path depending on access size means possibly more buggy
code. Note that we already hit a lot of wrong access size in the
emulation. So I would prefer to keep the code as generic as possible
even if it means a small impact on emulating byte access.

> 
>> 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?

The support of big-endian in Xen should be generic. I.e we deal with the
endianess before calling the handlers, so the handler will always have
the value in the hypervisor endianess.

Note that I've got a support for big-endian but never had the chance to
upstream it.

I think, as long as Xen stays in little endian, we would have no issue
with your solution.

Though using big endian in Xen would be much work that this small
changes. I'll give a look to implement it.

Regards,

-- 
Julien Grall