[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 5/8] xen/arm: vgic: Optimize the way to store GICD_IPRIORITYR in the rank
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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |