[edk2] ArmPkg/ArmGicV3Dxe: configure all interrupts as non-secure Group-1

Message ID 1466605935-19217-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit c7fefb690661f2e38afcb8200bd318ecf38ab961
Headers show

Commit Message

Ard Biesheuvel June 22, 2016, 2:32 p.m.
Reassign all interrupts to non-secure Group-1 if the GIC has its DS
(Disable Security) bit set. In this case, it is safe to assume that we
own the GIC, and that no other firmware has performed any configuration
yet, which means it is up to us to reconfigure the interrupts so they
can be taken by the non-secure firmware.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---

This is the EDK2 counterpart of Peter's patch against the Linux kernel

7c9b973061b0 ("irqchip/gic-v3: Configure all interrupts as non-secure Group-1")

which tweaks the GICv3 driver so that it works with the GICv3 emulation in
QEMU, which only emulates a single GIC security state when running without
the security extensions.

 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 16 ++++++++++++++++
 ArmPkg/Include/Library/ArmGicLib.h        |  5 +++--
 2 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek June 23, 2016, 1:25 p.m. | #1
On 06/22/16 16:32, Ard Biesheuvel wrote:
> Reassign all interrupts to non-secure Group-1 if the GIC has its DS

> (Disable Security) bit set. In this case, it is safe to assume that we

> own the GIC, and that no other firmware has performed any configuration

> yet, which means it is up to us to reconfigure the interrupts so they

> can be taken by the non-secure firmware.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

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

> ---

> 

> This is the EDK2 counterpart of Peter's patch against the Linux kernel

> 

> 7c9b973061b0 ("irqchip/gic-v3: Configure all interrupts as non-secure Group-1")

> 

> which tweaks the GICv3 driver so that it works with the GICv3 emulation in

> QEMU, which only emulates a single GIC security state when running without

> the security extensions.

> 

>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 16 ++++++++++++++++

>  ArmPkg/Include/Library/ArmGicLib.h        |  5 +++--

>  2 files changed, 19 insertions(+), 2 deletions(-)

> 

> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

> index 50fa56262eaf..106c669fcb87 100644

> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

> @@ -297,6 +297,22 @@ GicV3DxeInitialize (

>      MpId = ArmReadMpidr ();

>      CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);

>  

> +    if ((MmioRead32 (mGicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_DS) != 0) {

> +      //

> +      // If the Disable Security (DS) control bit is set, we are dealing with a

> +      // GIC that has only one security state. In this case, let's assume we are

> +      // executing in non-secure state (which is appropriate for DXE modules)

> +      // and that no other firmware has performed any configuration on the GIC.

> +      // This means we need to reconfigure all interrupts to non-secure Group 1

> +      // first.

> +      //

> +      MmioWrite32 (mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR, 0xffffffff);

> +

> +      for (Index = 32; Index < mGicNumInterrupts; Index += 32) {

> +        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 0xffffffff);

> +      }

> +    }

> +

>      // Route the SPIs to the primary CPU. SPIs start at the INTID 32

>      for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {

>        MmioWrite32 (mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8), CpuTarget | ARM_GICD_IROUTER_IRM);

> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h

> index 10c4a9d72eb2..4364f3ffef46 100644

> --- a/ArmPkg/Include/Library/ArmGicLib.h

> +++ b/ArmPkg/Include/Library/ArmGicLib.h

> @@ -47,8 +47,9 @@

>  // GICv3 specific registers

>  #define ARM_GICD_IROUTER        0x6100 // Interrupt Routing Registers

>  

> -// the Affinity Routing Enable (ARE) bit in GICD_CTLR

> -#define ARM_GIC_ICDDCR_ARE      (1 << 4)

> +// GICD_CTLR bits

> +#define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)

> +#define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)

>  

>  //

>  // GIC Redistributor

> 


Drew tested this patch; the result is positive:

http://thread.gmane.org/gmane.comp.emulators.qemu/418780/focus=421731

Based on that, I'm synthetizing:

Tested-by: Drew Jones <drjones@redhat.com>


Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 23, 2016, 1:26 p.m. | #2
On 23 June 2016 at 15:25, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/22/16 16:32, Ard Biesheuvel wrote:

>> Reassign all interrupts to non-secure Group-1 if the GIC has its DS

>> (Disable Security) bit set. In this case, it is safe to assume that we

>> own the GIC, and that no other firmware has performed any configuration

>> yet, which means it is up to us to reconfigure the interrupts so they

>> can be taken by the non-secure firmware.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

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

>> ---

>>

>> This is the EDK2 counterpart of Peter's patch against the Linux kernel

>>

>> 7c9b973061b0 ("irqchip/gic-v3: Configure all interrupts as non-secure Group-1")

>>

>> which tweaks the GICv3 driver so that it works with the GICv3 emulation in

>> QEMU, which only emulates a single GIC security state when running without

>> the security extensions.

>>

>>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 16 ++++++++++++++++

>>  ArmPkg/Include/Library/ArmGicLib.h        |  5 +++--

>>  2 files changed, 19 insertions(+), 2 deletions(-)

>>

>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

>> index 50fa56262eaf..106c669fcb87 100644

>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

>> @@ -297,6 +297,22 @@ GicV3DxeInitialize (

>>      MpId = ArmReadMpidr ();

>>      CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);

>>

>> +    if ((MmioRead32 (mGicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_DS) != 0) {

>> +      //

>> +      // If the Disable Security (DS) control bit is set, we are dealing with a

>> +      // GIC that has only one security state. In this case, let's assume we are

>> +      // executing in non-secure state (which is appropriate for DXE modules)

>> +      // and that no other firmware has performed any configuration on the GIC.

>> +      // This means we need to reconfigure all interrupts to non-secure Group 1

>> +      // first.

>> +      //

>> +      MmioWrite32 (mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR, 0xffffffff);

>> +

>> +      for (Index = 32; Index < mGicNumInterrupts; Index += 32) {

>> +        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 0xffffffff);

>> +      }

>> +    }

>> +

>>      // Route the SPIs to the primary CPU. SPIs start at the INTID 32

>>      for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {

>>        MmioWrite32 (mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8), CpuTarget | ARM_GICD_IROUTER_IRM);

>> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h

>> index 10c4a9d72eb2..4364f3ffef46 100644

>> --- a/ArmPkg/Include/Library/ArmGicLib.h

>> +++ b/ArmPkg/Include/Library/ArmGicLib.h

>> @@ -47,8 +47,9 @@

>>  // GICv3 specific registers

>>  #define ARM_GICD_IROUTER        0x6100 // Interrupt Routing Registers

>>

>> -// the Affinity Routing Enable (ARE) bit in GICD_CTLR

>> -#define ARM_GIC_ICDDCR_ARE      (1 << 4)

>> +// GICD_CTLR bits

>> +#define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)

>> +#define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)

>>

>>  //

>>  // GIC Redistributor

>>

>

> Drew tested this patch; the result is positive:

>

> http://thread.gmane.org/gmane.comp.emulators.qemu/418780/focus=421731

>

> Based on that, I'm synthetizing:

>

> Tested-by: Drew Jones <drjones@redhat.com>

>


Thanks!

@Leif: any concerns?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm June 23, 2016, 1:51 p.m. | #3
On Thu, Jun 23, 2016 at 03:26:53PM +0200, Ard Biesheuvel wrote:
> On 23 June 2016 at 15:25, Laszlo Ersek <lersek@redhat.com> wrote:

> > On 06/22/16 16:32, Ard Biesheuvel wrote:

> >> Reassign all interrupts to non-secure Group-1 if the GIC has its DS

> >> (Disable Security) bit set. In this case, it is safe to assume that we

> >> own the GIC, and that no other firmware has performed any configuration

> >> yet, which means it is up to us to reconfigure the interrupts so they

> >> can be taken by the non-secure firmware.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

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

> >> ---

> >>

> >> This is the EDK2 counterpart of Peter's patch against the Linux kernel

> >>

> >> 7c9b973061b0 ("irqchip/gic-v3: Configure all interrupts as non-secure Group-1")

> >>

> >> which tweaks the GICv3 driver so that it works with the GICv3 emulation in

> >> QEMU, which only emulates a single GIC security state when running without

> >> the security extensions.

> >>

> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 16 ++++++++++++++++

> >>  ArmPkg/Include/Library/ArmGicLib.h        |  5 +++--

> >>  2 files changed, 19 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

> >> index 50fa56262eaf..106c669fcb87 100644

> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

> >> @@ -297,6 +297,22 @@ GicV3DxeInitialize (

> >>      MpId = ArmReadMpidr ();

> >>      CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);

> >>

> >> +    if ((MmioRead32 (mGicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_DS) != 0) {

> >> +      //

> >> +      // If the Disable Security (DS) control bit is set, we are dealing with a

> >> +      // GIC that has only one security state. In this case, let's assume we are

> >> +      // executing in non-secure state (which is appropriate for DXE modules)

> >> +      // and that no other firmware has performed any configuration on the GIC.

> >> +      // This means we need to reconfigure all interrupts to non-secure Group 1

> >> +      // first.

> >> +      //

> >> +      MmioWrite32 (mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR, 0xffffffff);

> >> +

> >> +      for (Index = 32; Index < mGicNumInterrupts; Index += 32) {

> >> +        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 0xffffffff);

> >> +      }

> >> +    }

> >> +

> >>      // Route the SPIs to the primary CPU. SPIs start at the INTID 32

> >>      for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {

> >>        MmioWrite32 (mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8), CpuTarget | ARM_GICD_IROUTER_IRM);

> >> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h

> >> index 10c4a9d72eb2..4364f3ffef46 100644

> >> --- a/ArmPkg/Include/Library/ArmGicLib.h

> >> +++ b/ArmPkg/Include/Library/ArmGicLib.h

> >> @@ -47,8 +47,9 @@

> >>  // GICv3 specific registers

> >>  #define ARM_GICD_IROUTER        0x6100 // Interrupt Routing Registers

> >>

> >> -// the Affinity Routing Enable (ARE) bit in GICD_CTLR

> >> -#define ARM_GIC_ICDDCR_ARE      (1 << 4)

> >> +// GICD_CTLR bits

> >> +#define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)

> >> +#define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)

> >>

> >>  //

> >>  // GIC Redistributor

> >>

> >

> > Drew tested this patch; the result is positive:

> >

> > http://thread.gmane.org/gmane.comp.emulators.qemu/418780/focus=421731

> >

> > Based on that, I'm synthetizing:

> >

> > Tested-by: Drew Jones <drjones@redhat.com>

> >

> 

> Thanks!

> 

> @Leif: any concerns?


Nope.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 23, 2016, 2:21 p.m. | #4
On 23 June 2016 at 15:51, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Jun 23, 2016 at 03:26:53PM +0200, Ard Biesheuvel wrote:

>> On 23 June 2016 at 15:25, Laszlo Ersek <lersek@redhat.com> wrote:

>> > On 06/22/16 16:32, Ard Biesheuvel wrote:

>> >> Reassign all interrupts to non-secure Group-1 if the GIC has its DS

>> >> (Disable Security) bit set. In this case, it is safe to assume that we

>> >> own the GIC, and that no other firmware has performed any configuration

>> >> yet, which means it is up to us to reconfigure the interrupts so they

>> >> can be taken by the non-secure firmware.

>> >>

>> >> Contributed-under: TianoCore Contribution Agreement 1.0

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

>> >> ---

>> >>

>> >> This is the EDK2 counterpart of Peter's patch against the Linux kernel

>> >>

>> >> 7c9b973061b0 ("irqchip/gic-v3: Configure all interrupts as non-secure Group-1")

>> >>

>> >> which tweaks the GICv3 driver so that it works with the GICv3 emulation in

>> >> QEMU, which only emulates a single GIC security state when running without

>> >> the security extensions.

>> >>

>> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 16 ++++++++++++++++

>> >>  ArmPkg/Include/Library/ArmGicLib.h        |  5 +++--

>> >>  2 files changed, 19 insertions(+), 2 deletions(-)

>> >>

>> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

>> >> index 50fa56262eaf..106c669fcb87 100644

>> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

>> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c

>> >> @@ -297,6 +297,22 @@ GicV3DxeInitialize (

>> >>      MpId = ArmReadMpidr ();

>> >>      CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);

>> >>

>> >> +    if ((MmioRead32 (mGicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_DS) != 0) {

>> >> +      //

>> >> +      // If the Disable Security (DS) control bit is set, we are dealing with a

>> >> +      // GIC that has only one security state. In this case, let's assume we are

>> >> +      // executing in non-secure state (which is appropriate for DXE modules)

>> >> +      // and that no other firmware has performed any configuration on the GIC.

>> >> +      // This means we need to reconfigure all interrupts to non-secure Group 1

>> >> +      // first.

>> >> +      //

>> >> +      MmioWrite32 (mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR, 0xffffffff);

>> >> +

>> >> +      for (Index = 32; Index < mGicNumInterrupts; Index += 32) {

>> >> +        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 0xffffffff);

>> >> +      }

>> >> +    }

>> >> +

>> >>      // Route the SPIs to the primary CPU. SPIs start at the INTID 32

>> >>      for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {

>> >>        MmioWrite32 (mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8), CpuTarget | ARM_GICD_IROUTER_IRM);

>> >> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h

>> >> index 10c4a9d72eb2..4364f3ffef46 100644

>> >> --- a/ArmPkg/Include/Library/ArmGicLib.h

>> >> +++ b/ArmPkg/Include/Library/ArmGicLib.h

>> >> @@ -47,8 +47,9 @@

>> >>  // GICv3 specific registers

>> >>  #define ARM_GICD_IROUTER        0x6100 // Interrupt Routing Registers

>> >>

>> >> -// the Affinity Routing Enable (ARE) bit in GICD_CTLR

>> >> -#define ARM_GIC_ICDDCR_ARE      (1 << 4)

>> >> +// GICD_CTLR bits

>> >> +#define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)

>> >> +#define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)

>> >>

>> >>  //

>> >>  // GIC Redistributor

>> >>

>> >

>> > Drew tested this patch; the result is positive:

>> >

>> > http://thread.gmane.org/gmane.comp.emulators.qemu/418780/focus=421731

>> >

>> > Based on that, I'm synthetizing:

>> >

>> > Tested-by: Drew Jones <drjones@redhat.com>

>> >

>>

>> Thanks!

>>

>> @Leif: any concerns?

>

> Nope.

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


Thanks all. I committed this as c7fefb690661 (but I forgot to add
Drew's T-b, sorry about that)

-- 
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 50fa56262eaf..106c669fcb87 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -297,6 +297,22 @@  GicV3DxeInitialize (
     MpId = ArmReadMpidr ();
     CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
 
+    if ((MmioRead32 (mGicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_DS) != 0) {
+      //
+      // If the Disable Security (DS) control bit is set, we are dealing with a
+      // GIC that has only one security state. In this case, let's assume we are
+      // executing in non-secure state (which is appropriate for DXE modules)
+      // and that no other firmware has performed any configuration on the GIC.
+      // This means we need to reconfigure all interrupts to non-secure Group 1
+      // first.
+      //
+      MmioWrite32 (mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR, 0xffffffff);
+
+      for (Index = 32; Index < mGicNumInterrupts; Index += 32) {
+        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 0xffffffff);
+      }
+    }
+
     // Route the SPIs to the primary CPU. SPIs start at the INTID 32
     for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {
       MmioWrite32 (mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8), CpuTarget | ARM_GICD_IROUTER_IRM);
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 10c4a9d72eb2..4364f3ffef46 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -47,8 +47,9 @@ 
 // GICv3 specific registers
 #define ARM_GICD_IROUTER        0x6100 // Interrupt Routing Registers
 
-// the Affinity Routing Enable (ARE) bit in GICD_CTLR
-#define ARM_GIC_ICDDCR_ARE      (1 << 4)
+// GICD_CTLR bits
+#define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)
+#define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)
 
 //
 // GIC Redistributor