| Message ID | 1424098255-22490-7-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 19/02/15 15:58, Ian Campbell wrote: > On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote: >> The stride may not be set if the hardware GIC is using the default >> layout. It happens on the Foundation model. >> >> On GICv3, the default stride is 2 * 64K. Therefore it's possible to avoid >> checking at every redistributor MMIO access if the stride is not set. >> >> Because domU uses a static stride configuration this only happens for >> dom0, so we can move this code in gicv_v3_init. Take the opportunity to move >> the stride setting a bit earlier because the loop to set regions will require >> the stride. >> >> Also, use 2 * 64K rather than 128K and explain the reason. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > >> I wasn't not sure where to move this code. I find very confusion the >> splitting between vgic and gicv. Maybe we should introduce a >> hwdom_gicv_init and giccc_map callbacks. Then move most of the >> initialization in the vgic one. > > What is giccc? I'm not familiar enough with this code to rule on what > should go where. Perhaps Stefano has an opinion. I think the name of giccc what bad here. What I wanted to mean is splitting the current gicv_setup code in 2 parts: - Mapping GICV to the guest - Set up the hwdom information This will bring a better partition to the code and bring GICv2 on GICv3 support more quickly. I will talk with Stefano about this possibility. Regards,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 47452ca..7b33ff7 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -897,12 +897,21 @@ static int gicv_v3_init(struct domain *d) { d->arch.vgic.dbase = gicv3.dbase; d->arch.vgic.dbase_size = gicv3.dbase_size; + + d->arch.vgic.rdist_stride = gicv3.rdist_stride; + /* + * If the stride is not set, the default stride for GICv3 is 2 * 64K: + * - first 64k page for Control and Physical LPIs + * - second 64k page for Control and Generation of SGIs + */ + if ( !d->arch.vgic.rdist_stride ) + d->arch.vgic.rdist_stride = 2 * SZ_64K; + for ( i = 0; i < gicv3.rdist_count; i++ ) { d->arch.vgic.rbase[i] = gicv3.rdist_regions[i].base; d->arch.vgic.rbase_size[i] = gicv3.rdist_regions[i].size; } - d->arch.vgic.rdist_stride = gicv3.rdist_stride; d->arch.vgic.rdist_count = gicv3.rdist_count; } else diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 2c14717..c5a743a 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -625,11 +625,7 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info) perfc_incr(vgicr_reads); - if ( v->domain->arch.vgic.rdist_stride != 0 ) - offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); - else - /* If stride is not set. Default 128K */ - offset = info->gpa & (SZ_128K - 1); + offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); if ( offset < SZ_64K ) return __vgic_v3_rdistr_rd_mmio_read(v, info, offset); @@ -649,11 +645,7 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info) perfc_incr(vgicr_writes); - if ( v->domain->arch.vgic.rdist_stride != 0 ) - offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); - else - /* If stride is not set. Default 128K */ - offset = info->gpa & (SZ_128K - 1); + offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); if ( offset < SZ_64K ) return __vgic_v3_rdistr_rd_mmio_write(v, info, offset);
The stride may not be set if the hardware GIC is using the default layout. It happens on the Foundation model. On GICv3, the default stride is 2 * 64K. Therefore it's possible to avoid checking at every redistributor MMIO access if the stride is not set. Because domU uses a static stride configuration this only happens for dom0, so we can move this code in gicv_v3_init. Take the opportunity to move the stride setting a bit earlier because the loop to set regions will require the stride. Also, use 2 * 64K rather than 128K and explain the reason. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- I wasn't not sure where to move this code. I find very confusion the splitting between vgic and gicv. Maybe we should introduce a hwdom_gicv_init and giccc_map callbacks. Then move most of the initialization in the vgic one. Changes in v3: - Fix typoes and update commit message - Forgot to remove the check on the stride the rdist write Changes in v2: - Patch added --- xen/arch/arm/gic-v3.c | 11 ++++++++++- xen/arch/arm/vgic-v3.c | 12 ++---------- 2 files changed, 12 insertions(+), 11 deletions(-)