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 Mon, 2015-09-28 at 18:10 +0100, Julien Grall wrote:
> 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.

Agreed, given this context that makes sense.

[...]
> >     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.

I wasn't concerned about Xen being big-endian, but rather about whether
ipriorityr[0] is the same byte as priority[0]&0xff or if it was actually
priority[0]>>24 in little endian mode. I wasn't feeling very well on Monday
so I didn't try to think about it too hard.

Ian.