Re: [Xen-devel] [PATCH 04/57] ARM: GICv3: simplify GICv3 redistributor stride handling

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 05/03/18 16:03, Andre Przywara wrote:
> Instead of hard coding the architected redistributor stride into the
> code, lets use a clear #define to the two values for GICv3 and GICv4 and
> clarify the algorithm to determine the needed stride value.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
> Changelog RFC ... v1:
> - no changes
>
>   xen/arch/arm/gic-v3.c             | 18 ++++++++++--------
>   xen/include/asm-arm/gic_v3_defs.h |  5 +++++
>   2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index b1f8a86409..be1787b39a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -690,6 +690,15 @@ static int __init gicv3_populate_rdist(void)
>           do {
>               typer = readq_relaxed(ptr + GICR_TYPER);
>   
> +            /* Set the architectural redist size if not overridden by DT. */
> +            if ( !gicv3.rdist_stride )
> +            {
> +                if ( typer & GICR_TYPER_VLPIS )
Is there anything in the spec promising you that *all* the 
redistributors will support vLPIs?
But I am not fully convinced of having this patch in Xen. I feel it 
makes the code more confusing to read. The current code shows that the 
next re-distributor start is based on the current TYPER values. So I 
think it would make sense to just rework the current code:
if ( gicv3.rdist_stride )
{
   ptr += ...
}
else
{
  if ( typer & GICR_TYPER_VLPIS )
    ptr += GICV4_GICR_SIZE;
  else
    ptr += GICV3_GICR_SIZE;
}

> +                    gicv3.rdist_stride = GICV4_GICR_SIZE;
> +                else
> +                    gicv3.rdist_stride = GICV3_GICR_SIZE;
> +            } > +
>               if ( (typer >> 32) == aff )
>               {
>                   this_cpu(rbase) = ptr;
> @@ -732,14 +741,7 @@ static int __init gicv3_populate_rdist(void)
>               if ( gicv3.rdist_regions[i].single_rdist )
>                   break;
>   
> -            if ( gicv3.rdist_stride )
> -                ptr += gicv3.rdist_stride;
> -            else
> -            {
> -                ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
> -                if ( typer & GICR_TYPER_VLPIS )
> -                    ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
> -            }
> +            ptr += gicv3.rdist_stride;
>   
>           } while ( !(typer & GICR_TYPER_LAST) );
>       }
> diff --git a/xen/include/asm-arm/gic_v3_defs.h 
> b/xen/include/asm-arm/gic_v3_defs.h
> index 65c9dc47cf..412e41afed 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -18,6 +18,8 @@
>   #ifndef __ASM_ARM_GIC_V3_DEFS_H__
>   #define __ASM_ARM_GIC_V3_DEFS_H__
>   
> +#include <xen/sizes.h>
> +
>   /*
>    * Additional registers defined in GIC v3.
>    * Common GICD registers are defined in gic.h
> @@ -68,6 +70,9 @@
>   #define GICV3_GICD_IIDR_VAL          0x34c
>   #define GICV3_GICR_IIDR_VAL          GICV3_GICD_IIDR_VAL
>   
> +#define GICV3_GICR_SIZE              (2 * SZ_64K)
> +#define GICV4_GICR_SIZE              (4 * SZ_64K)
Do you mind adding a comment either linking to the spec (Section 8.10 
ARM IHI 0069D) or explain the 2/4?
> +
>   #define GICR_CTLR                    (0x0000)
>   #define GICR_IIDR                    (0x0004)
>   #define GICR_TYPER                   (0x0008)
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel