| Message ID | 1422555950-31821-7-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 02/02/15 15:40, Ian Campbell wrote: > On Thu, 2015-01-29 at 18:25 +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. > > Can this defaulting not be pulled further to the initialisation of > gicv3.rdist_stride? With the upcoming GICv4, the stride may be different for each distributor (see the check on GICR_TYPER.VLPIS in gicv3_populate_rdist). So I'd like to avoid the check of rdist_stride. >> This is only happening for DOM0, > > Please say instead "Because domU uses a static stride configuration this > only happens for dom0..." or similar (i.e. include the reason why domU > is excluded) I will do. >> so we can move this code in >> gicv_v3_init. Take the opportunity to move the stride setting a bit ealier > > "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 v2: >> - Patch added >> --- >> xen/arch/arm/gic-v3.c | 11 ++++++++++- >> xen/arch/arm/vgic-v3.c | 6 +----- >> 2 files changed, 11 insertions(+), 6 deletions(-) >> >> 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..db6b514 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) > > Why not the write case too? By mistake it has been dropped in a following patch ("Emulate correctly the re-distributor"). I will move the changes here. 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..db6b514 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);
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. This is only happening for DOM0, so we can move this code in gicv_v3_init. Take the opportunity to move the stride setting a bit ealier 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 v2: - Patch added --- xen/arch/arm/gic-v3.c | 11 ++++++++++- xen/arch/arm/vgic-v3.c | 6 +----- 2 files changed, 11 insertions(+), 6 deletions(-)