[edk2-devel] [PATCH v1 0/3] Add support for handling SGI and pending interrupts

[edk2-devel] [PATCH v1 0/3] Add support for handling SGI and pending interrupts

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4466

This patch series introduce a few improvements related to interrupt
handling for ArmGic driver and library.

1. The current implementation of the `ArmGicSendSgiTo` is based on GIC v2
and does not work on GIC v3 and on.
2. The pending interrupt related primitives does not exist for end users.

The change added the SGI support compatible with GIC v3 and v4. A few
pending interrupt related utility functions were also added.

This change was tested on QEMU, FVP and proprietary hardware platforms.

Pathc v1 branch: https://github.com/kuqin12/edk2/tree/improve_gic_v1

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Kun Qin (3):
  ArmPkg: ArmGic: Added support to send SGI to NS G1 EL1
  ArmPkg: ArmGicLib: Added GIC v3 and v4 support to ArmGicSendSgiTo
  ArmPkg: ArmGic: Added functionalities to manipulate pending interrupts

 ArmPkg/Drivers/ArmGic/ArmGicLib.c              | 214 +++++++++++++++++++-
 ArmPkg/ArmPkg.ci.yaml                          |   3 +
 ArmPkg/Drivers/ArmGic/GicV3/AArch64/ArmGicV3.S |  11 +
 ArmPkg/Drivers/ArmGic/GicV3/Arm/ArmGicV3.S     |  10 +
 ArmPkg/Include/Library/ArmGicLib.h             |  76 +++++++
 5 files changed, 308 insertions(+), 6 deletions(-)

-- 
2.41.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107187): https://edk2.groups.io/g/devel/message/107187
Mute This Topic: https://groups.io/mt/100337221/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/3] ArmPkg: ArmGic: Added support to send SGI to NS G1 EL1

[Kun Qin]

From: Kun Qin <kuqin@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4466

This change extended the functionality of ArmGic to support sending
software generated interrupts to non-secure group 1 at EL1.

The change made here follows the ARM documentation `ICC_SGI1R_EL1,
Interrupt Controller Software Generated Interrupt Group 1 Register`.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Kun Qin <kuqin@microsoft.com>
---
 ArmPkg/ArmPkg.ci.yaml                          |  1 +
 ArmPkg/Drivers/ArmGic/GicV3/AArch64/ArmGicV3.S | 11 +++++++++++
 ArmPkg/Drivers/ArmGic/GicV3/Arm/ArmGicV3.S     | 10 ++++++++++
 ArmPkg/Include/Library/ArmGicLib.h             |  5 +++++
 4 files changed, 27 insertions(+)

diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index d31248161189..8a8f738437d5 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -158,6 +158,7 @@
           "ipriority",
           "irouter",
           "isenabler",
+          "ishst",
           "istatus",
           "itargets",
           "lable",
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/AArch64/ArmGicV3.S b/ArmPkg/Drivers/ArmGic/GicV3/AArch64/ArmGicV3.S
index 20f83aa85f3b..f2ab57174be3 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/AArch64/ArmGicV3.S
+++ b/ArmPkg/Drivers/ArmGic/GicV3/AArch64/ArmGicV3.S
@@ -23,6 +23,7 @@
 #define ICC_IAR1_EL1            S3_0_C12_C12_0
 #define ICC_PMR_EL1             S3_0_C4_C6_0
 #define ICC_BPR1_EL1            S3_0_C12_C12_3
+#define ICC_SGI1R_EL1           S3_0_C12_C11_5
 
 #endif
 
@@ -55,6 +56,16 @@ ASM_FUNC(ArmGicV3SetControlSystemRegisterEnable)
 4:  isb
         ret
 
+// VOID
+// ArmGicV3SendNsG1Sgi (
+//  IN UINT64          SgiVal
+//  );
+ASM_FUNC(ArmGicV3SendNsG1Sgi)
+        dsb     ishst
+        msr     ICC_SGI1R_EL1, x0
+        isb
+        ret
+
 //VOID
 //ArmGicV3EnableInterruptInterface (
 //  VOID
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/Arm/ArmGicV3.S b/ArmPkg/Drivers/ArmGic/GicV3/Arm/ArmGicV3.S
index 8c43a613dc57..79e57e4afb70 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/Arm/ArmGicV3.S
+++ b/ArmPkg/Drivers/ArmGic/GicV3/Arm/ArmGicV3.S
@@ -29,6 +29,16 @@ ASM_FUNC(ArmGicV3SetControlSystemRegisterEnable)
         isb
         bx      lr
 
+// VOID
+// ArmGicV3SendNsG1Sgi (
+//  IN UINT64          SgiVal
+//  );
+ASM_FUNC(ArmGicV3SendNsG1Sgi)
+        dsb     ishst
+        mcrr    p15, 0, r0, r1, c12 // ICC_SGI1R_EL1
+        isb
+        bx      lr
+
 //VOID
 //ArmGicV3EnableInterruptInterface (
 //  VOID
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 93ce8aeb1994..773b27954522 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -332,4 +332,9 @@ ArmGicV3SetPriorityMask (
   IN UINTN  Priority
   );
 
+VOID
+ArmGicV3SendNsG1Sgi (
+  IN UINT64  SgiVal
+  );
+
 #endif // ARMGIC_H_
-- 
2.41.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107188): https://edk2.groups.io/g/devel/message/107188
Mute This Topic: https://groups.io/mt/100337222/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/3] ArmPkg: ArmGicLib: Added GIC v3 and v4 support to ArmGicSendSgiTo

[Kun Qin]

From: Kun Qin <kuqin@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4466

This change extended the existing function `ArmGicSendSgiTo` of ArmGicLib
to format the incoming parameters to comply with GICv3 and GICv4 spec,
and signal software generated interrupts to non secure group 1 at EL1.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Kun Qin <kuqin@microsoft.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c  | 52 +++++++++++++++++---
 ArmPkg/Include/Library/ArmGicLib.h | 22 +++++++++
 2 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 7f4bb248fc72..830d822d2c05 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -146,12 +146,52 @@ ArmGicSendSgiTo (
   IN  UINT8  SgiId
   )
 {
-  MmioWrite32 (
-    GicDistributorBase + ARM_GIC_ICDSGIR,
-    ((TargetListFilter & 0x3) << 24) |
-    ((CPUTargetList & 0xFF) << 16)   |
-    (SgiId & 0xF)
-    );
+  ARM_GIC_ARCH_REVISION  Revision;
+  UINT32                 ApplicableTargets;
+  UINT32                 AFF3;
+  UINT32                 AFF2;
+  UINT32                 AFF1;
+  UINT32                 AFF0;
+  UINT32                 Irm;
+  UINT64                 SGIValue;
+
+  Revision = ArmGicGetSupportedArchRevision ();
+  if (Revision == ARM_GIC_ARCH_REVISION_2) {
+    MmioWrite32 (
+      GicDistributorBase + ARM_GIC_ICDSGIR,
+      ((TargetListFilter & 0x3) << 24) |
+      ((CPUTargetList & 0xFF) << 16)   |
+      (SgiId & 0xF)
+      );
+  } else {
+    // Below routine is adopted from gicv3_raise_secure_g0_sgi in TF-A
+
+    /* Extract affinity fields from target */
+    AFF0 = GET_MPIDR_AFF0 (CPUTargetList);
+    AFF1 = GET_MPIDR_AFF1 (CPUTargetList);
+    AFF2 = GET_MPIDR_AFF2 (CPUTargetList);
+    AFF3 = GET_MPIDR_AFF3 (CPUTargetList);
+
+    /*
+     * Make target list from affinity 0, and ensure GICv3 SGI can target
+     * this PE.
+     */
+    ApplicableTargets = (1 << AFF0);
+
+    /*
+     * Evaluate the filter to see if this is for the target or all others
+     */
+    Irm = (TargetListFilter == ARM_GIC_ICDSGIR_FILTER_EVERYONEELSE) ? SGIR_IRM_TO_OTHERS : SGIR_IRM_TO_AFF;
+
+    /* Raise SGI to PE specified by its affinity */
+    SGIValue = GICV3_SGIR_VALUE (AFF3, AFF2, AFF1, SgiId, Irm, ApplicableTargets);
+
+    /*
+     * Ensure that any shared variable updates depending on out of band
+     * interrupt trigger are observed before raising SGI.
+     */
+    ArmGicV3SendNsG1Sgi (SGIValue);
+  }
 }
 
 /*
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 773b27954522..28d58f187d4f 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -110,6 +110,28 @@
 // Bit Mask for
 #define ARM_GIC_ICCIAR_ACKINTID  0x3FF
 
+/* ICC SGI macros */
+#define SGIR_TGT_MASK     ((UINT64)0xffff)
+#define SGIR_AFF1_SHIFT   16
+#define SGIR_INTID_SHIFT  24
+#define SGIR_INTID_MASK   ((UINT64)0xf)
+#define SGIR_AFF2_SHIFT   32
+#define SGIR_IRM_SHIFT    40
+#define SGIR_IRM_MASK     ((UINT64)0x1)
+#define SGIR_AFF3_SHIFT   48
+#define SGIR_AFF_MASK     ((UINT64)0xff)
+
+#define SGIR_IRM_TO_AFF     0
+#define SGIR_IRM_TO_OTHERS  1
+
+#define GICV3_SGIR_VALUE(_aff3, _aff2, _aff1, _intid, _irm, _tgt) \
+  ((((UINT64) (_aff3) & SGIR_AFF_MASK) << SGIR_AFF3_SHIFT) | \
+   (((UINT64) (_irm) & SGIR_IRM_MASK) << SGIR_IRM_SHIFT) | \
+   (((UINT64) (_aff2) & SGIR_AFF_MASK) << SGIR_AFF2_SHIFT) | \
+   (((_intid) & SGIR_INTID_MASK) << SGIR_INTID_SHIFT) | \
+   (((_aff1) & SGIR_AFF_MASK) << SGIR_AFF1_SHIFT) | \
+   ((_tgt) & SGIR_TGT_MASK))
+
 UINT32
 EFIAPI
 ArmGicGetInterfaceIdentification (
-- 
2.41.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107189): https://edk2.groups.io/g/devel/message/107189
Mute This Topic: https://groups.io/mt/100337223/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 3/3] ArmPkg: ArmGic: Added functionalities to manipulate pending interrupts

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4466

This change provides additional functionalities to `ArmGicLib` to
manipulate pending interrupt related status. The added functions include:

- `ArmGicSetPendingInterrupt`
- `ArmGicClearPendingInterrupt`
- `ArmGicIsInterruptPending`

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Kun Qin <kuqin@microsoft.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c  | 162 ++++++++++++++++++++
 ArmPkg/ArmPkg.ci.yaml              |   2 +
 ArmPkg/Include/Library/ArmGicLib.h |  49 ++++++
 3 files changed, 213 insertions(+)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 830d822d2c05..3844dc05e2af 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -33,6 +33,12 @@
 #define IPRIORITY_ADDRESS(base, offset)  ((base) +\
           ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDIPR + 4 * (offset))
 
+#define ISPENDR_ADDRESS(base, offset)  ((base) +\
+          ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISPENDR + 4 * (offset))
+
+#define ICPENDR_ADDRESS(base, offset)  ((base) +\
+          ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ICPENDR + 4 * (offset))
+
 /**
  *
  * Return whether the Source interrupt index refers to a shared interrupt (SPI)
@@ -440,6 +446,162 @@ ArmGicIsInterruptEnabled (
   return ((Interrupts & (1 << RegShift)) != 0);
 }
 
+/**
+  Set an interrupt to pending state from GIC.
+
+  @param GicDistributorBase    Base address of platform GIC Distributor.
+  @param GicRedistributorBase  Base address of platform GIC Redistributor.
+  @param Source                Interrupt source ID.
+**/
+VOID
+EFIAPI
+ArmGicSetPendingInterrupt (
+  IN UINTN  GicDistributorBase,
+  IN UINTN  GicRedistributorBase,
+  IN UINTN  Source
+  )
+{
+  UINT32                 RegOffset;
+  UINTN                  RegShift;
+  ARM_GIC_ARCH_REVISION  Revision;
+  UINTN                  GicCpuRedistributorBase;
+
+  // Calculate enable register offset and bit position
+  RegOffset = (UINT32)(Source / 32);
+  RegShift  = Source % 32;
+
+  Revision = ArmGicGetSupportedArchRevision ();
+  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
+      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
+      SourceIsSpi (Source))
+  {
+    // Write set-pending register
+    MmioWrite32 (
+      GicDistributorBase + ARM_GIC_ICDSPR + (4 * RegOffset),
+      1 << RegShift
+      );
+  } else {
+    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
+                                GicRedistributorBase,
+                                Revision
+                                );
+    if (GicCpuRedistributorBase == 0) {
+      ASSERT_EFI_ERROR (EFI_NOT_FOUND);
+      return;
+    }
+
+    // Write set-enable register
+    MmioWrite32 (
+      ISPENDR_ADDRESS (GicCpuRedistributorBase, RegOffset),
+      1 << RegShift
+      );
+  }
+}
+
+/**
+  Clear a pending interrupt from GIC.
+
+  @param GicDistributorBase    Base address of platform GIC Distributor.
+  @param GicRedistributorBase  Base address of platform GIC Redistributor.
+  @param Source                Interrupt source ID.
+**/
+VOID
+EFIAPI
+ArmGicClearPendingInterrupt (
+  IN UINTN  GicDistributorBase,
+  IN UINTN  GicRedistributorBase,
+  IN UINTN  Source
+  )
+{
+  UINT32                 RegOffset;
+  UINTN                  RegShift;
+  ARM_GIC_ARCH_REVISION  Revision;
+  UINTN                  GicCpuRedistributorBase;
+
+  // Calculate enable register offset and bit position
+  RegOffset = (UINT32)(Source / 32);
+  RegShift  = Source % 32;
+
+  Revision = ArmGicGetSupportedArchRevision ();
+  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
+      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
+      SourceIsSpi (Source))
+  {
+    // Write clear-enable register
+    MmioWrite32 (
+      GicDistributorBase + ARM_GIC_ICDICPR + (4 * RegOffset),
+      1 << RegShift
+      );
+  } else {
+    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
+                                GicRedistributorBase,
+                                Revision
+                                );
+    if (GicCpuRedistributorBase == 0) {
+      return;
+    }
+
+    // Write clear-enable register
+    MmioWrite32 (
+      ICPENDR_ADDRESS (GicCpuRedistributorBase, RegOffset),
+      1 << RegShift
+      );
+  }
+}
+
+/**
+  Check if an interrupt is pending in GIC.
+
+  @param GicDistributorBase    Base address of platform GIC Distributor.
+  @param GicRedistributorBase  Base address of platform GIC Redistributor.
+  @param Source                Interrupt source ID.
+
+  @return BOOLEAN   TRUE if the interrupt is pending, FALSE otherwise.
+**/
+BOOLEAN
+EFIAPI
+ArmGicIsInterruptPending (
+  IN UINTN  GicDistributorBase,
+  IN UINTN  GicRedistributorBase,
+  IN UINTN  Source
+  )
+{
+  UINT32                 RegOffset;
+  UINTN                  RegShift;
+  ARM_GIC_ARCH_REVISION  Revision;
+  UINTN                  GicCpuRedistributorBase;
+  UINT32                 Interrupts;
+
+  // Calculate enable register offset and bit position
+  RegOffset = (UINT32)(Source / 32);
+  RegShift  = Source % 32;
+
+  Revision = ArmGicGetSupportedArchRevision ();
+  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
+      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
+      SourceIsSpi (Source))
+  {
+    Interrupts = MmioRead32 (
+                   GicDistributorBase + ARM_GIC_ICDSPR + (4 * RegOffset)
+                   );
+  } else {
+    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
+                                GicRedistributorBase,
+                                Revision
+                                );
+    if (GicCpuRedistributorBase == 0) {
+      return 0;
+    }
+
+    // Read set-enable register
+    Interrupts = MmioRead32 (
+                   ISPENDR_ADDRESS (GicCpuRedistributorBase, RegOffset)
+                   );
+  }
+
+  return ((Interrupts & (1 << RegShift)) != 0);
+}
+
 VOID
 EFIAPI
 ArmGicDisableDistributor (
diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index 8a8f738437d5..06e31498cf79 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -154,11 +154,13 @@
           "icdsgir",
           "icdspr",
           "icenabler",
+          "icpendr",
           "intid",
           "ipriority",
           "irouter",
           "isenabler",
           "ishst",
+          "ispendr",
           "istatus",
           "itargets",
           "lable",
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 28d58f187d4f..83d52756d61a 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -78,6 +78,8 @@
 // GIC SGI & PPI Redistributor frame
 #define ARM_GICR_ISENABLER  0x0100      // Interrupt Set-Enable Registers
 #define ARM_GICR_ICENABLER  0x0180      // Interrupt Clear-Enable Registers
+#define ARM_GICR_ISPENDR    0x0200      // Interrupt Set-Pending Registers
+#define ARM_GICR_ICPENDR    0x0280      // Interrupt Clear-Pending Registers
 
 // GIC Cpu interface
 #define ARM_GIC_ICCICR   0x00         // CPU Interface Control Register
@@ -167,6 +169,53 @@ ArmGicDisableInterruptInterface (
   IN  UINTN  GicInterruptInterfaceBase
   );
 
+/**
+  Set an interrupt to pending state from GIC.
+
+  @param GicDistributorBase    Base address of platform GIC Distributor.
+  @param GicRedistributorBase  Base address of platform GIC Redistributor.
+  @param Source                Interrupt source ID.
+**/
+VOID
+EFIAPI
+ArmGicSetPendingInterrupt (
+  IN UINTN  GicDistributorBase,
+  IN UINTN  GicRedistributorBase,
+  IN UINTN  Source
+  );
+
+/**
+  Clear a pending interrupt from GIC.
+
+  @param GicDistributorBase    Base address of platform GIC Distributor.
+  @param GicRedistributorBase  Base address of platform GIC Redistributor.
+  @param Source                Interrupt source ID.
+**/
+VOID
+EFIAPI
+ArmGicClearPendingInterrupt (
+  IN UINTN  GicDistributorBase,
+  IN UINTN  GicRedistributorBase,
+  IN UINTN  Source
+  );
+
+/**
+  Check if an interrupt is pending in GIC.
+
+  @param GicDistributorBase    Base address of platform GIC Distributor.
+  @param GicRedistributorBase  Base address of platform GIC Redistributor.
+  @param Source                Interrupt source ID.
+
+  @return BOOLEAN   TRUE if the interrupt is pending, FALSE otherwise.
+**/
+BOOLEAN
+EFIAPI
+ArmGicIsInterruptPending (
+  IN UINTN  GicDistributorBase,
+  IN UINTN  GicRedistributorBase,
+  IN UINTN  Source
+  );
+
 VOID
 EFIAPI
 ArmGicEnableDistributor (
-- 
2.41.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107190): https://edk2.groups.io/g/devel/message/107190
Mute This Topic: https://groups.io/mt/100337225/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/3] Add support for handling SGI and pending interrupts

[Ard Biesheuvel]

On Mon, 24 Jul 2023 at 22:15, Kun Qin <kuqin12@gmail.com> wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4466
>
> This patch series introduce a few improvements related to interrupt
> handling for ArmGic driver and library.
>
> 1. The current implementation of the `ArmGicSendSgiTo` is based on GIC v2
> and does not work on GIC v3 and on.
> 2. The pending interrupt related primitives does not exist for end users.
>
> The change added the SGI support compatible with GIC v3 and v4. A few
> pending interrupt related utility functions were also added.
>
> This change was tested on QEMU, FVP and proprietary hardware platforms.
>
> Pathc v1 branch: https://github.com/kuqin12/edk2/tree/improve_gic_v1
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
>
> Kun Qin (3):
>   ArmPkg: ArmGic: Added support to send SGI to NS G1 EL1
>   ArmPkg: ArmGicLib: Added GIC v3 and v4 support to ArmGicSendSgiTo
>   ArmPkg: ArmGic: Added functionalities to manipulate pending interrupts
>

Hello Kun,

Could you please explain how this will be used by platforms?

SGIs are only used by the MPcore SEC code, which is deprecated and
shouldn't be used. And manipulating pending interrupts is another
thing we should only support if there is a compelling use case.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107266): https://edk2.groups.io/g/devel/message/107266
Mute This Topic: https://groups.io/mt/100337221/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/3] Add support for handling SGI and pending interrupts

[Kun Qin]

Hi Ard,

Our current use case is around AP core suspension and wake-ups.

The program can suspend the secondary cores through PSCI interfaces 
(after powering
them on). BSP can then wake up the suspended cores through SGI on demand.

The pending interrupt manipulation is to support BSP suspension and 
wakeup. We
currently use watchdog timer to wake up suspended BSPs after a timeout.

Platforms can perform needed tasks during suspension, such as core power 
consumption
analysis, in UEFI environment.

Please let me if you have any suggestions.

Thanks,
Kun

On 7/26/2023 1:45 AM, Ard Biesheuvel wrote:
> On Mon, 24 Jul 2023 at 22:15, Kun Qin <kuqin12@gmail.com> wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4466
>>
>> This patch series introduce a few improvements related to interrupt
>> handling for ArmGic driver and library.
>>
>> 1. The current implementation of the `ArmGicSendSgiTo` is based on GIC v2
>> and does not work on GIC v3 and on.
>> 2. The pending interrupt related primitives does not exist for end users.
>>
>> The change added the SGI support compatible with GIC v3 and v4. A few
>> pending interrupt related utility functions were also added.
>>
>> This change was tested on QEMU, FVP and proprietary hardware platforms.
>>
>> Pathc v1 branch: https://github.com/kuqin12/edk2/tree/improve_gic_v1
>>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Kun Qin (3):
>>    ArmPkg: ArmGic: Added support to send SGI to NS G1 EL1
>>    ArmPkg: ArmGicLib: Added GIC v3 and v4 support to ArmGicSendSgiTo
>>    ArmPkg: ArmGic: Added functionalities to manipulate pending interrupts
>>
> Hello Kun,
>
> Could you please explain how this will be used by platforms?
>
> SGIs are only used by the MPcore SEC code, which is deprecated and
> shouldn't be used. And manipulating pending interrupts is another
> thing we should only support if there is a compelling use case.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107276): https://edk2.groups.io/g/devel/message/107276
Mute This Topic: https://groups.io/mt/100337221/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/3] Add support for handling SGI and pending interrupts

[Ard Biesheuvel]

On Wed, 26 Jul 2023 at 20:45, Kun Qin <kuqin12@gmail.com> wrote:
>
> Hi Ard,
>
> Our current use case is around AP core suspension and wake-ups.
>
> The program can suspend the secondary cores through PSCI interfaces
> (after powering
> them on). BSP can then wake up the suspended cores through SGI on demand.
>

The use of PSCI is already dubious in the context of UEFI - combining
it with the use of SGIs for communication between cores seems like a
slippery slope I don't think we should be going down.

> The pending interrupt manipulation is to support BSP suspension and
> wakeup. We
> currently use watchdog timer to wake up suspended BSPs after a timeout.
>

suspended APs right?

> Platforms can perform needed tasks during suspension, such as core power
> consumption
> analysis, in UEFI environment.
>
> Please let me if you have any suggestions.
>

Is there any basis in the UEFI or PI specifications for this functionality?


> On 7/26/2023 1:45 AM, Ard Biesheuvel wrote:
> > On Mon, 24 Jul 2023 at 22:15, Kun Qin <kuqin12@gmail.com> wrote:
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4466
> >>
> >> This patch series introduce a few improvements related to interrupt
> >> handling for ArmGic driver and library.
> >>
> >> 1. The current implementation of the `ArmGicSendSgiTo` is based on GIC v2
> >> and does not work on GIC v3 and on.
> >> 2. The pending interrupt related primitives does not exist for end users.
> >>
> >> The change added the SGI support compatible with GIC v3 and v4. A few
> >> pending interrupt related utility functions were also added.
> >>
> >> This change was tested on QEMU, FVP and proprietary hardware platforms.
> >>
> >> Pathc v1 branch: https://github.com/kuqin12/edk2/tree/improve_gic_v1
> >>
> >> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >>
> >> Kun Qin (3):
> >>    ArmPkg: ArmGic: Added support to send SGI to NS G1 EL1
> >>    ArmPkg: ArmGicLib: Added GIC v3 and v4 support to ArmGicSendSgiTo
> >>    ArmPkg: ArmGic: Added functionalities to manipulate pending interrupts
> >>
> > Hello Kun,
> >
> > Could you please explain how this will be used by platforms?
> >
> > SGIs are only used by the MPcore SEC code, which is deprecated and
> > shouldn't be used. And manipulating pending interrupts is another
> > thing we should only support if there is a compelling use case.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107552): https://edk2.groups.io/g/devel/message/107552
Mute This Topic: https://groups.io/mt/100337221/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/3] Add support for handling SGI and pending interrupts

[Leif Lindholm]

On 2023-08-03 16:33, Ard Biesheuvel wrote:
> On Wed, 26 Jul 2023 at 20:45, Kun Qin <kuqin12@gmail.com> wrote:
>>
>> Hi Ard,
>>
>> Our current use case is around AP core suspension and wake-ups.
>>
>> The program can suspend the secondary cores through PSCI interfaces
>> (after powering
>> them on). BSP can then wake up the suspended cores through SGI on demand.
>>
> 
> The use of PSCI is already dubious in the context of UEFI - combining
> it with the use of SGIs for communication between cores seems like a
> slippery slope I don't think we should be going down.

Well, UEFI has no concept of multiple cores, so I don't think there's 
anything fishy about using PSCI to bring up/down APs. But I agree adding 
interrupt support, not considered by either UEFI nor PI, feels off.

What is the fundamental use-case? (Pre-)silicon testing?

/
     Leif



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107554): https://edk2.groups.io/g/devel/message/107554
Mute This Topic: https://groups.io/mt/100337221/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/3] Add support for handling SGI and pending interrupts

[Kun Qin]

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

Hi Leif & Ard,

You are correct. The main use case is around silicon testing.

There is currently no UEFI or PI spec around such functionality though.

Manipulating pending interrupts might be a far fetch, but can we fix
"ArmGicSendSgiTo" function on GIC v3 and v4? It is declared as a public
interface provided through ArmGicLib, but the fact that it does not work
on GIC v3 and v4 without any notes seems confusing and misleading.

Please let me know if you have other suggestions.

Regards,
Kun

On 8/3/2023 10:02 AM, Leif Lindholm wrote:
> On 2023-08-03 16:33, Ard Biesheuvel wrote:
>> On Wed, 26 Jul 2023 at 20:45, Kun Qin <kuqin12@gmail.com> wrote:
>>>
>>> Hi Ard,
>>>
>>> Our current use case is around AP core suspension and wake-ups.
>>>
>>> The program can suspend the secondary cores through PSCI interfaces
>>> (after powering
>>> them on). BSP can then wake up the suspended cores through SGI on 
>>> demand.
>>>
>>
>> The use of PSCI is already dubious in the context of UEFI - combining
>> it with the use of SGIs for communication between cores seems like a
>> slippery slope I don't think we should be going down.
>
> Well, UEFI has no concept of multiple cores, so I don't think there's 
> anything fishy about using PSCI to bring up/down APs. But I agree 
> adding interrupt support, not considered by either UEFI nor PI, feels 
> off.
>
> What is the fundamental use-case? (Pre-)silicon testing?
>
> /
>     Leif
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107555): https://edk2.groups.io/g/devel/message/107555
Mute This Topic: https://groups.io/mt/100337221/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 3830 bytes --]