Re: [PATCH v2 06/15] xen/arm: vgic-v3: Set stride during domain initialization

[Ian Campbell <Ian.Campbell@citrix.com>]

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?

> 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)

>  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?

>  
>      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);