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

[Ard Biesheuvel]

On 23 June 2016 at 15:51, Leif Lindholm  wrote:
> On Thu, Jun 23, 2016 at 03:26:53PM +0200, Ard Biesheuvel wrote:
>> On 23 June 2016 at 15:25, Laszlo Ersek  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 
>> >> ---
>> >>
>> >> 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, 0x);
>> >> +
>> >> +  for (Index = 32; Index < mGicNumInterrupts; Index += 32) {
>> >> +MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 
>> >> 0x);
>> >> +  }
>> >> +}
>> >> +
>> >>  // 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_IROUTER0x6100 // 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 
>> >
>>
>> Thanks!
>>
>> @Leif: any concerns?
>
> Nope.
> Reviewed-by: Leif Lindholm 

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

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

[Leif Lindholm]

On Thu, Jun 23, 2016 at 03:26:53PM +0200, Ard Biesheuvel wrote:
> On 23 June 2016 at 15:25, Laszlo Ersek  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 
> >> ---
> >>
> >> 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, 0x);
> >> +
> >> +  for (Index = 32; Index < mGicNumInterrupts; Index += 32) {
> >> +MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 
> >> 0x);
> >> +  }
> >> +}
> >> +
> >>  // 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_IROUTER0x6100 // 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 
> >
> 
> Thanks!
> 
> @Leif: any concerns?

Nope.
Reviewed-by: Leif Lindholm 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

On 23 June 2016 at 15:25, Laszlo Ersek  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 
>> ---
>>
>> 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, 0x);
>> +
>> +  for (Index = 32; Index < mGicNumInterrupts; Index += 32) {
>> +MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 
>> 0x);
>> +  }
>> +}
>> +
>>  // 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_IROUTER0x6100 // 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 
>

Thanks!

@Leif: any concerns?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

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 
> ---
> 
> 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, 0x);
> +
> +  for (Index = 32; Index < mGicNumInterrupts; Index += 32) {
> +MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 
> 0x);
> +  }
> +}
> +
>  // 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_IROUTER0x6100 // 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 

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel