[PATCH v2 0/2] ArmPkg/ArmGicV3Dxe: fix writes to GICD_IPRIORITYR<n> when ARE enable

[PATCH v2 0/2] ArmPkg/ArmGicV3Dxe: fix writes to GICD_IPRIORITYR<n> when ARE enable

[Quan Nguyen]

According to ARM IHI 0069F, section 11.9.18 GICD_IPRIORITYR<n>,
Interrupt Priority Registers, n = 0 - 254, when affinity routing is
enabled for the Security state of an interrupt, GICR_IPRIORITYR<n> is
used instead of GICD_IPRIORITYR<n> where n = 0 to 7 (that is, for SGIs
and PPIs).

Current ArmGicV3Dxe tries to initialize all GICD_IPRIORITYR<n> to
a default state, so it should write to GICR_IPRIORITYR<n> registers
when Affinity Routing is Enabled.

v2:
- Update Ard's comment on stack variable and FeaturePcdGet [Ard]
- Introduce new helper function to handle register discrepancy [Quan]
- Set priority using new helper function [Quan]

Quan Nguyen (2):
  ArmPkg/ArmGicLib: Add ArmGicSetInterruptPriority() helper function
  ArmPkg/ArmGicV3Dxe: Use ArmGicSetInterruptPriority() to set priority

 ArmPkg/Drivers/ArmGic/ArmGicLib.c         | 44 +++++++++++++++++++++++
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 13 +++----
 ArmPkg/Include/Library/ArmGicLib.h        |  9 +++++
 3 files changed, 58 insertions(+), 8 deletions(-)

-- 
2.28.0

[PATCH v2 1/2] ArmPkg/ArmGicLib: Add ArmGicSetInterruptPriority() helper function

[Quan Nguyen]

According to ARM IHI 0069F, section 11.9.18 GICD_IPRIORITYR<n>,
Interrupt Priority Registers, n = 0 - 254, when affinity routing is
enabled for the Security state of an interrupt, GICR_IPRIORITYR<n>
is used instead of GICD_IPRIORITYR<n> where n = 0 to 7 (that is, for
SGIs and PPIs).

As setting interrupt priority for SGIs and PPIs are handled using
difference registers depends on the mode, this patch instroduces
ArmGicSetInterruptPriority() helper function to handle the discrepancy.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c  | 44 ++++++++++++++++++++++++++++++
 ArmPkg/Include/Library/ArmGicLib.h |  9 ++++++
 2 files changed, 53 insertions(+)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 001e6b143104..8ef32b33a154 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -199,6 +199,50 @@ ArmGicEndOfInterrupt (
   }
 }
 
+VOID
+EFIAPI
+ArmGicSetInterruptPriority (
+  IN UINTN                  GicDistributorBase,
+  IN UINTN                  GicRedistributorBase,
+  IN UINTN                  Source,
+  IN UINTN                  Priority
+  )
+{
+  UINT32                RegOffset;
+  UINTN                 RegShift;
+  ARM_GIC_ARCH_REVISION Revision;
+  UINTN                 GicCpuRedistributorBase;
+
+  // Calculate register offset and bit position
+  RegOffset = Source / 4;
+  RegShift = (Source % 4) * 8;
+
+  Revision = ArmGicGetSupportedArchRevision ();
+  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
+      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
+      SourceIsSpi (Source)) {
+    MmioAndThenOr32 (
+      GicDistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
+      ~(0xff << RegShift),
+      Priority << RegShift
+      );
+  } else {
+    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
+                                GicRedistributorBase,
+                                Revision
+                                );
+    if (GicCpuRedistributorBase == 0) {
+      return;
+    }
+
+    MmioAndThenOr32 (
+      GicCpuRedistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
+      ~(0xff << RegShift),
+      Priority << RegShift
+      );
+  }
+}
+
 VOID
 EFIAPI
 ArmGicEnableInterrupt (
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 55093189638b..7bcfc001115b 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -208,6 +208,15 @@ ArmGicSetPriorityMask (
   IN  INTN          PriorityMask
   );
 
+VOID
+EFIAPI
+ArmGicSetInterruptPriority (
+  IN UINTN                  GicDistributorBase,
+  IN UINTN                  GicRedistributorBase,
+  IN UINTN                  Source,
+  IN UINTN                  Priority
+  );
+
 VOID
 EFIAPI
 ArmGicEnableInterrupt (
-- 
2.28.0

[PATCH v2 2/2] ArmPkg/ArmGicV3Dxe: Use ArmGicSetInterruptPriority() to set priority

[Quan Nguyen]

When Affinity Routing enabled, the GICR_IPRIORITYR<n> is used to set
priority for SGIs and PPIs instead of GICD_IPRIORITYR<n>.
This patch calls ArmGicSetInterruptPriority() helper function when
setting priority to handle the difference.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Victor Gallardo <Victor@os.amperecomputing.com>
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index d7da1f198d9e..16bccbff413b 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -374,8 +374,6 @@ GicV3DxeInitialize (
 {
   EFI_STATUS              Status;
   UINTN                   Index;
-  UINT32                  RegOffset;
-  UINTN                   RegShift;
   UINT64                  CpuTarget;
   UINT64                  MpId;
 
@@ -397,12 +395,11 @@ GicV3DxeInitialize (
     GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
 
     // Set Priority
-    RegOffset = Index / 4;
-    RegShift = (Index % 4) * 8;
-    MmioAndThenOr32 (
-      mGicDistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
-      ~(0xff << RegShift),
-      ARM_GIC_DEFAULT_PRIORITY << RegShift
+    ArmGicSetInterruptPriority (
+      mGicDistributorBase,
+      mGicRedistributorsBase,
+      Index,
+      ARM_GIC_DEFAULT_PRIORITY
       );
   }
 
-- 
2.28.0

Re: [PATCH v2 0/2] ArmPkg/ArmGicV3Dxe: fix writes to GICD_IPRIORITYR<n> when ARE enable

[Ard Biesheuvel]

On 12/16/20 2:25 PM, Quan Nguyen wrote:
> According to ARM IHI 0069F, section 11.9.18 GICD_IPRIORITYR<n>,
> Interrupt Priority Registers, n = 0 - 254, when affinity routing is
> enabled for the Security state of an interrupt, GICR_IPRIORITYR<n> is
> used instead of GICD_IPRIORITYR<n> where n = 0 to 7 (that is, for SGIs
> and PPIs).
> 
> Current ArmGicV3Dxe tries to initialize all GICD_IPRIORITYR<n> to
> a default state, so it should write to GICR_IPRIORITYR<n> registers
> when Affinity Routing is Enabled.
> 
> v2:
> - Update Ard's comment on stack variable and FeaturePcdGet [Ard]
> - Introduce new helper function to handle register discrepancy [Quan]
> - Set priority using new helper function [Quan]
> 
> Quan Nguyen (2):
>   ArmPkg/ArmGicLib: Add ArmGicSetInterruptPriority() helper function
>   ArmPkg/ArmGicV3Dxe: Use ArmGicSetInterruptPriority() to set priority
> 

Reviewed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Merged as #1245

Thanks,

>  ArmPkg/Drivers/ArmGic/ArmGicLib.c         | 44 +++++++++++++++++++++++
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 13 +++----
>  ArmPkg/Include/Library/ArmGicLib.h        |  9 +++++
>  3 files changed, 58 insertions(+), 8 deletions(-)
>