[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 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@xxxxxxxxxx> > --- > > 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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |