Re: [PATCH v3 1/1] ArmPkg: Add support for GICv4

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 9 October 2018 at 17:35, Sami Mujawar <sami.mujawar@arm.com> wrote:
> Updated Redistributor base calculation to allow for the fact that
> GICv4 has 2 additional 64KB frames (for VLPI and a reserved frame).
> The code now tests the VLPIS bit in the GIC Redistributor Type
> Register (GICR_TYPER) and calculates the Redistributor granularity
> accordingly.
>
> The code changes are:
>   GICR_TYPER register fields, etc, added to the header.
>   Loop updated to pay attention to GICR_TYPER.Last.
>   Derive frame "stride" size from GICR_TYPER.VLPIS.
>
> Note: The assumption is that the redistributors are adjacent for
> all CPUs. However this may not be the case for NUMA systems.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as bbce001515bb..301402fa4797

Thanks Sami.

> ---
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/329_gicv4_granularity_v3
>
> Notes:
>     v3:
>      - Remove unused PcdCoreCount from .inf and add comment [ARD]
>        regarding assumption of adjacent redistributors
>      - Updated code based on review comments.               [SAMI]
>
>     v2:
>      - Code review comments to change variable name, expand
>        name for register, local variable name, addition of
>        macro for returning affinity & code comment update.  [LEIF]
>      - Updated code based on review comments.               [SAMI]
>
>     v1:
>      - Added support for initializing GICv4                 [SAMI]
>
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c   | 40 +++++++++++++-------
>  ArmPkg/Drivers/ArmGic/ArmGicLib.inf |  5 +--
>  ArmPkg/Include/Library/ArmGicLib.h  | 22 +++++++++--
>  3 files changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 0087399fb1dba0e697f7a6ccd6f7432a59311ac6..0be5b14b4f6eba74903739d03912e73fdeb0e4e7 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2018, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -19,6 +19,16 @@
>  #include <Library/IoLib.h>
>  #include <Library/PcdLib.h>
>
> +// In GICv3, there are 2 x 64KB frames:
> +// Redistributor control frame + SGI Control & Generation frame
> +#define GIC_V3_REDISTRIBUTOR_GRANULARITY  (ARM_GICR_CTLR_FRAME_SIZE           \
> +                                           + ARM_GICR_SGI_PPI_FRAME_SIZE)
> +
> +// In GICv4, there are 2 additional 64KB frames:
> +// VLPI frame + Reserved page frame
> +#define GIC_V4_REDISTRIBUTOR_GRANULARITY  (GIC_V3_REDISTRIBUTOR_GRANULARITY   \
> +                                           + ARM_GICR_SGI_VLPI_FRAME_SIZE     \
> +                                           + ARM_GICR_SGI_RESERVED_FRAME_SIZE)
>
>  #define ISENABLER_ADDRESS(base,offset) ((base) + \
>            ARM_GICR_CTLR_FRAME_SIZE +  ARM_GICR_ISENABLER + (4 * offset))
> @@ -54,12 +64,11 @@ GicGetCpuRedistributorBase (
>    IN ARM_GIC_ARCH_REVISION Revision
>    )
>  {
> -  UINTN Index;
>    UINTN MpId;
>    UINTN CpuAffinity;
>    UINTN Affinity;
> -  UINTN GicRedistributorGranularity;
>    UINTN GicCpuRedistributorBase;
> +  UINT64 TypeRegister;
>
>    MpId = ArmReadMpidr ();
>    // Define CPU affinity as:
> @@ -68,27 +77,30 @@ GicGetCpuRedistributorBase (
>    CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2)) |
>                  ((MpId & ARM_CORE_AFF3) >> 8);
>
> -  if (Revision == ARM_GIC_ARCH_REVISION_3) {
> -    // 2 x 64KB frame:
> -    //   Redistributor control frame + SGI Control & Generation frame
> -    GicRedistributorGranularity = ARM_GICR_CTLR_FRAME_SIZE
> -                                  + ARM_GICR_SGI_PPI_FRAME_SIZE;
> -  } else {
> +  if (Revision < ARM_GIC_ARCH_REVISION_3) {
>      ASSERT_EFI_ERROR (EFI_UNSUPPORTED);
>      return 0;
>    }
>
>    GicCpuRedistributorBase = GicRedistributorBase;
>
> -  for (Index = 0; Index < PcdGet32 (PcdCoreCount); Index++) {
> -    Affinity = MmioRead64 (GicCpuRedistributorBase + ARM_GICR_TYPER) >> 32;
> +  do {
> +    TypeRegister = MmioRead64 (GicCpuRedistributorBase + ARM_GICR_TYPER);
> +    Affinity = ARM_GICR_TYPER_GET_AFFINITY (TypeRegister);
>      if (Affinity == CpuAffinity) {
>        return GicCpuRedistributorBase;
>      }
>
> -    // Move to the next GIC Redistributor frame
> -    GicCpuRedistributorBase += GicRedistributorGranularity;
> -  }
> +    // Move to the next GIC Redistributor frame.
> +    // The GIC specification does not forbid a mixture of redistributors
> +    // with or without support for virtual LPIs, so we test Virtual LPIs
> +    // Support (VLPIS) bit for each frame to decide the granularity.
> +    // Note: The assumption here is that the redistributors are adjacent
> +    // for all CPUs. However this may not be the case for NUMA systems.
> +    GicCpuRedistributorBase += (((ARM_GICR_TYPER_VLPIS & TypeRegister) != 0)
> +                                ? GIC_V4_REDISTRIBUTOR_GRANULARITY
> +                                : GIC_V3_REDISTRIBUTOR_GRANULARITY);
> +  } while ((TypeRegister & ARM_GICR_TYPER_LAST) == 0);
>
>    // The Redistributor has not been found for the current CPU
>    ASSERT_EFI_ERROR (EFI_NOT_FOUND);
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.inf b/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
> index 047adac85ff40ee4f6bd63d9fc7c17283b44c7e0..93375b7be3eee76cf3c4222cb880a56e4db4c6f3 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
> @@ -1,5 +1,5 @@
>  #/* @file
> -#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2011-2018, ARM Limited. All rights reserved.
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -44,8 +44,5 @@ [Packages]
>    ArmPlatformPkg/ArmPlatformPkg.dec
>    MdePkg/MdePkg.dec
>
> -[Pcd]
> -  gArmPlatformTokenSpaceGuid.PcdCoreCount
> -
>  [FeaturePcd]
>    gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy
> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
> index 4b21ea9e4e76cb83c0c3421c1d9d88b456192687..5775905ca91baabca13de47b8b9f7ac507becd55 100644
> --- a/ArmPkg/Include/Library/ArmGicLib.h
> +++ b/ArmPkg/Include/Library/ArmGicLib.h
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2018, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -60,12 +60,28 @@
>
>
>  // GIC Redistributor
> -#define ARM_GICR_CTLR_FRAME_SIZE    SIZE_64KB
> -#define ARM_GICR_SGI_PPI_FRAME_SIZE SIZE_64KB
> +#define ARM_GICR_CTLR_FRAME_SIZE         SIZE_64KB
> +#define ARM_GICR_SGI_PPI_FRAME_SIZE      SIZE_64KB
> +#define ARM_GICR_SGI_VLPI_FRAME_SIZE     SIZE_64KB
> +#define ARM_GICR_SGI_RESERVED_FRAME_SIZE SIZE_64KB
>
>  // GIC Redistributor Control frame
>  #define ARM_GICR_TYPER          0x0008  // Redistributor Type Register
>
> +// GIC Redistributor TYPER bit assignments
> +#define ARM_GICR_TYPER_PLPIS        (1 << 0)              // Physical LPIs
> +#define ARM_GICR_TYPER_VLPIS        (1 << 1)              // Virtual LPIs
> +#define ARM_GICR_TYPER_DIRECTLPI    (1 << 3)              // Direct LPIs
> +#define ARM_GICR_TYPER_LAST         (1 << 4)              // Last Redistributor in series
> +#define ARM_GICR_TYPER_DPGS         (1 << 5)              // Disable Processor Group
> +                                                          // Selection Support
> +#define ARM_GICR_TYPER_PROCNO       (0xFFFF << 8)         // Processor Number
> +#define ARM_GICR_TYPER_COMMONLPIAFF (0x3 << 24)           // Common LPI Affinity
> +#define ARM_GICR_TYPER_AFFINITY     (0xFFFFFFFFULL << 32) // Redistributor Affinity
> +
> +#define ARM_GICR_TYPER_GET_AFFINITY(TypeReg)  (((TypeReg) & \
> +                                                ARM_GICR_TYPER_AFFINITY) >> 32)
> +
>  // GIC SGI & PPI Redistributor frame
>  #define ARM_GICR_ISENABLER      0x0100  // Interrupt Set-Enable Registers
>  #define ARM_GICR_ICENABLER      0x0180  // Interrupt Clear-Enable Registers
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>