[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 4/6] xen/arm: vgic: Optimize the way to store the target vCPU in the rank
On Mon, 9 Nov 2015, Julien Grall wrote: > Xen is currently directly storing the value of GICD_ITARGETSR register > (for GICv2) and GICD_IROUTER (for GICv3) in the rank. This makes the > emulation of the registers access very simple but makes the code to get > the target vCPU for a given vIRQ more complex. > > While the target vCPU of an vIRQ is retrieved every time an vIRQ is > injected to the guest, the access to the 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 target vCPU for > every interrupt in the rank. This will make the code to get the target > very quick. The emulation code will now have to generate the GICD_ITARGETSR > and GICD_IROUTER register for read access and split it to store in a > convenient way. > > With the new way to store the target vCPU, the structure vgic_irq_rank > is shrunk down from 320 bytes to 92 bytes. This is saving about 228 > bytes of memory allocated separately per vCPU. nice > Note that with these changes, any read to those register will list only > the target vCPU used by Xen. As the spec is not clear whether this is a > valid choice or not, OSes which have a different interpretation of the > spec (i.e OSes which perform read-modify-write operations on these > registers) may not boot anymore on Xen. Although, I think this is fair > trade between memory usage in Xen (1KB less on a domain using 4 vCPUs > with no SPIs) and a strict interpretation of the spec (though all the > cases are not clearly defined). > > Furthermore, the implementation of the callback get_target_vcpu is now > exactly the same. Consolidate the implementation in the common vGIC code > and drop the callback. > > Finally take the opportunity to fix coding style and replace "irq" by > "virq" to make clear that we are dealing with virtual IRQ in section we > are modifying. > > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx> > > --- > The real reason is I'd like to drop vgic_byte_* helpers in favor of more > generic access helpers. Although it would make the code to retrieve the > priority more complex. So reworking the code to get the target vCPU > was the best solution. > > Changes in v5: > - NR_TARGET_PER_REG has been renamed to NR_TARGETS_PER_ITARGETSR > - NR_BIT_PER_TARGET has been renamed to NR_BITS_PER_TARGET > - Fix comments > - s/NR_BYTE_PER_IROUTER/NR_BYTES_PER_IROUTER/ > - Remove the duplicate declaration of virq in vgic_store_itargetsr > > Changes in v4: > - Move the behavior changes in 2 separate patches: > * patch #1: Handle correctly byte write in ITARGETSR > * patch #2: Don't ignore a write in ITARGETSR if one field is 0 > - Update commit message > > Changes in v3: > - Make clear in the commit message that we rely on a corner case > of the spec. > - Typoes > > Changes in v2: > - Remove get_target_vcpu callback > - s/irq/virq/ to make clear we are using virtual IRQ > - Update the commit message > - Rename vgic_generate_* to vgic_fetch > - Improve code comment > - Move the introduction of vgic_rank_init in a separate patch > - Make use of rank->idx > > Changes in v1: > - Patch added > --- > xen/arch/arm/vgic-v2.c | 86 +++++++++++++------------------- > xen/arch/arm/vgic-v3.c | 119 > ++++++++++++++++++++++++--------------------- > xen/arch/arm/vgic.c | 45 ++++++++++++----- > xen/include/asm-arm/vgic.h | 18 +++---- > 4 files changed, 137 insertions(+), 131 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index b5249ff..4f976d4 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -89,18 +89,72 @@ static struct vcpu *vgic_v3_irouter_to_vcpu(struct domain > *d, uint64_t irouter) > return d->vcpu[vcpu_id]; > } > > -static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq) > -{ > - struct vcpu *v_target; > - struct vgic_irq_rank *rank = vgic_rank_irq(v, irq); > +#define NR_BYTES_PER_IROUTER 8U Given the way this constant is used, it might be better to define it as a bit shift of 3, removing the two division operations below. > +/* > + * Fetch an IROUTER register based on the offset from IROUTER0. Only one > + * vCPU will be listed for a given vIRQ. > + * > + * Note the offset will be aligned to the appropriate boundary. > + */ > +static uint64_t vgic_fetch_irouter(struct vgic_irq_rank *rank, > + unsigned int offset) > +{ > ASSERT(spin_is_locked(&rank->lock)); > > - v_target = vgic_v3_irouter_to_vcpu(v->domain, rank->v3.irouter[irq % > 32]); > + /* There is exactly 1 vIRQ per IROUTER */ > + offset /= NR_BYTES_PER_IROUTER; > > - ASSERT(v_target != NULL); > + /* Get the index in the rank */ > + offset &= INTERRUPT_RANK_MASK; > > - return v_target; > + return vcpuid_to_vaffinity(rank->vcpu[offset]); > +} > + > +/* > + * Store an IROUTER register in a convenient way and migrate the vIRQ > + * if necessary. This function only deals with IROUTER32 and onwards. > + * > + * Note the offset will be aligned to the appropriate boundary. > + */ > +static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank, > + unsigned int offset, uint64_t irouter) > +{ > + struct vcpu *new_vcpu, *old_vcpu; > + unsigned int virq; > + > + /* There is 1 vIRQ per IROUTER */ > + virq = offset / NR_BYTES_PER_IROUTER; > + > + /* > + * The IROUTER0-31, used for SGIs/PPIs, are reserved and should > + * never call this function. > + */ > + ASSERT(virq >= 32); > + > + /* Get the index in the rank */ > + offset &= virq & INTERRUPT_RANK_MASK; > + > + new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter); > + old_vcpu = d->vcpu[rank->vcpu[offset]]; > + > + /* > + * From the spec (see 8.9.13 in IHI 0069A), any write with an > + * invalid vCPU will lead to the interrupt being ignored. > + * > + * But the current code to inject an IRQ is not able to cope with > + * invalid vCPU. So for now, just ignore the write. > + * > + * TODO: Respect the spec > + */ > + if ( !new_vcpu ) > + return; > + > + /* Only migrate the IRQ if the target vCPU has changed */ > + if ( new_vcpu != old_vcpu ) > + vgic_migrate_irq(old_vcpu, new_vcpu, virq); > + > + rank->vcpu[offset] = new_vcpu->vcpu_id; > } > > static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > @@ -726,8 +780,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, > mmio_info_t *info, > DABT_DOUBLE_WORD); > if ( rank == NULL ) goto read_as_zero; > vgic_lock_rank(v, rank, flags); > - *r = rank->v3.irouter[REG_RANK_INDEX(64, > - (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)]; > + *r = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER); > vgic_unlock_rank(v, rank, flags); > return 1; > case GICD_NSACR ... GICD_NSACRN: > @@ -812,8 +865,6 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, > mmio_info_t *info, > struct hsr_dabt dabt = info->dabt; > struct vgic_irq_rank *rank; > unsigned long flags; > - uint64_t new_irouter, old_irouter; > - struct vcpu *old_vcpu, *new_vcpu; > int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase); > > perfc_incr(vgicd_writes); > @@ -881,32 +932,8 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, > mmio_info_t *info, > rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, > DABT_DOUBLE_WORD); > if ( rank == NULL ) goto write_ignore; > - new_irouter = r; > vgic_lock_rank(v, rank, flags); > - > - old_irouter = rank->v3.irouter[REG_RANK_INDEX(64, > - (gicd_reg - GICD_IROUTER), > - DABT_DOUBLE_WORD)]; > - old_vcpu = vgic_v3_irouter_to_vcpu(v->domain, old_irouter); > - new_vcpu = vgic_v3_irouter_to_vcpu(v->domain, new_irouter); > - > - if ( !new_vcpu ) > - { > - printk(XENLOG_G_DEBUG > - "%pv: vGICD: wrong irouter at offset %#08x val > %#"PRIregister, > - v, gicd_reg, r); > - vgic_unlock_rank(v, rank, flags); > - /* > - * TODO: Don't inject a fault to the guest when the MPIDR is > - * not valid. From the spec, the interrupt should be > - * ignored. > - */ > - return 0; > - } > - rank->v3.irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER), > - DABT_DOUBLE_WORD)] = new_irouter; > - if ( old_vcpu != new_vcpu ) > - vgic_migrate_irq(old_vcpu, new_vcpu, (gicd_reg - > GICD_IROUTER)/8); > + vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER, r); > vgic_unlock_rank(v, rank, flags); > return 1; > case GICD_NSACR ... GICD_NSACRN: > @@ -1036,7 +1063,6 @@ static const struct mmio_handler_ops > vgic_distr_mmio_handler = { > static int vgic_v3_vcpu_init(struct vcpu *v) > { > int i; > - uint64_t affinity; > paddr_t rdist_base; > struct vgic_rdist_region *region; > unsigned int last_cpu; > @@ -1045,15 +1071,6 @@ static int vgic_v3_vcpu_init(struct vcpu *v) > struct domain *d = v->domain; > uint32_t rdist_stride = d->arch.vgic.rdist_stride; > > - /* For SGI and PPI the target is always this CPU */ > - affinity = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 32 | > - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 16 | > - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 8 | > - MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0)); > - > - for ( i = 0 ; i < 32 ; i++ ) > - v->arch.vgic.private_irqs->v3.irouter[i] = affinity; > - > /* > * Find the region where the re-distributor lives. For this purpose, > * we look one region ahead as we have only the first CPU in hand. > @@ -1098,7 +1115,7 @@ static int vgic_v3_vcpu_init(struct vcpu *v) > > static int vgic_v3_domain_init(struct domain *d) > { > - int i, idx; > + int i; > > /* > * Domain 0 gets the hardware address. > @@ -1150,13 +1167,6 @@ static int vgic_v3_domain_init(struct domain *d) > d->arch.vgic.rdist_regions[0].first_cpu = 0; > } > > - /* By default deliver to CPU0 */ > - for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ ) > - { > - for ( idx = 0; idx < 32; idx++ ) > - d->arch.vgic.shared_irqs[i].v3.irouter[idx] = 0; > - } > - > /* Register mmio handle for the Distributor */ > register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase, > SZ_64K, NULL); > @@ -1182,7 +1192,6 @@ static int vgic_v3_domain_init(struct domain *d) > static const struct vgic_ops v3_ops = { > .vcpu_init = vgic_v3_vcpu_init, > .domain_init = vgic_v3_domain_init, > - .get_target_vcpu = vgic_v3_get_target_vcpu, > .emulate_sysreg = vgic_v3_emulate_sysreg, > /* > * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |