[PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported

[PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported

[adr.her.arc.95]

The maximum number of interrupts supported is determined as
32 * (GICD_TYPER.ITLinesNumber + 1).

When GICD_TYPER.ITLinesNumber = 0b11111, the maximum number of
interrupts supported is 1024. However, both GICv2 and GICv3 reserve
INTIDs 1020-1023 for special purposes.

This results in runtime crashes because:
  (1) ArmGicLib functions do not guard against special interrupts.
  (2) ArmGicGetMaxNumInterrupts number includes special interrupts.
  (2) ArmGicV*Dxe relies on ArmGicGetMaxNumInterrupts, and thus
      programs special interrupts through ArmGicLib.

ArmGicGetMaxNumInterrupts now does not include special interrupts, that
is, it reports 1020 instead of 1024 when GICD_TYPER.ITLinesNumber = 0b11111.
This avoids the overhead of guarding ArmGicLib functions while not
requiring to modify the drivers code.

Signed-off-by: Adrián Herrera <adr.her.arc.95@gmail.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 6b01c88206..dff1401e9c 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -120,7 +120,10 @@ ArmGicGetMaxNumInterrupts (
   IN  INTN          GicDistributorBase
   )
 {
-  return 32 * ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F) + 1);
+  UINT32 ITLinesNumber;
+
+  ITLinesNumber = MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F;
+  return MIN (32 * (ITLinesNumber+ 1), 1020);
 }
 
 VOID
-- 
2.30.0

Re: [edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported

[Sami Mujawar]

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

Hi Adrian,

I believe there is already a similar patch on the mailing list at https://edk2.groups.io/g/devel/message/72596. This patch is pending review and tested-by.
Can you check if this patch covers the problems you describe, please?

Regards,

Sami Mujawar

From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Adrián Herrera via groups.io <adr.her.arc.95=gmail.com@groups.io>
Date: Saturday, 24 April 2021 at 03:57
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Adrián Herrera <adr.her.arc.95@gmail.com>
Subject: [edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported
The maximum number of interrupts supported is determined as
32 * (GICD_TYPER.ITLinesNumber + 1).

When GICD_TYPER.ITLinesNumber = 0b11111, the maximum number of
interrupts supported is 1024. However, both GICv2 and GICv3 reserve
INTIDs 1020-1023 for special purposes.

This results in runtime crashes because:
  (1) ArmGicLib functions do not guard against special interrupts.
  (2) ArmGicGetMaxNumInterrupts number includes special interrupts.
  (2) ArmGicV*Dxe relies on ArmGicGetMaxNumInterrupts, and thus
      programs special interrupts through ArmGicLib.

ArmGicGetMaxNumInterrupts now does not include special interrupts, that
is, it reports 1020 instead of 1024 when GICD_TYPER.ITLinesNumber = 0b11111.
This avoids the overhead of guarding ArmGicLib functions while not
requiring to modify the drivers code.

Signed-off-by: Adrián Herrera <adr.her.arc.95@gmail.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 6b01c88206..dff1401e9c 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -120,7 +120,10 @@ ArmGicGetMaxNumInterrupts (
   IN  INTN          GicDistributorBase


   )


 {


-  return 32 * ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F) + 1);


+  UINT32 ITLinesNumber;


+


+  ITLinesNumber = MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F;


+  return MIN (32 * (ITLinesNumber+ 1), 1020);


 }





 VOID


--
2.30.0






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

Re: [edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported

[Adrián Herrera]

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

Hi Sami,

That patch provides the same solution. I'll be sure to check the mailing
list before sending the next time.
Any benefit on marking this as duplicate? If so, how could I do it?

Thank you,
Adrián.

On Mon, 26 Apr 2021 at 08:56, Sami Mujawar <Sami.Mujawar@arm.com> wrote:

> Hi Adrian,
>
>
>
> I believe there is already a similar patch on the mailing list at
> https://edk2.groups.io/g/devel/message/72596. This patch is pending
> review and tested-by.
>
> Can you check if this patch covers the problems you describe, please?
>
>
>
> Regards,
>
>
>
> Sami Mujawar
>
>
>
> *From: *devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Adrián
> Herrera via groups.io <adr.her.arc.95=gmail.com@groups.io>
> *Date: *Saturday, 24 April 2021 at 03:57
> *To: *devel@edk2.groups.io <devel@edk2.groups.io>
> *Cc: *Adrián Herrera <adr.her.arc.95@gmail.com>
> *Subject: *[edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts
> supported
>
> The maximum number of interrupts supported is determined as
> 32 * (GICD_TYPER.ITLinesNumber + 1).
>
> When GICD_TYPER.ITLinesNumber = 0b11111, the maximum number of
> interrupts supported is 1024. However, both GICv2 and GICv3 reserve
> INTIDs 1020-1023 for special purposes.
>
> This results in runtime crashes because:
>   (1) ArmGicLib functions do not guard against special interrupts.
>   (2) ArmGicGetMaxNumInterrupts number includes special interrupts.
>   (2) ArmGicV*Dxe relies on ArmGicGetMaxNumInterrupts, and thus
>       programs special interrupts through ArmGicLib.
>
> ArmGicGetMaxNumInterrupts now does not include special interrupts, that
> is, it reports 1020 instead of 1024 when GICD_TYPER.ITLinesNumber =
> 0b11111.
> This avoids the overhead of guarding ArmGicLib functions while not
> requiring to modify the drivers code.
>
> Signed-off-by: Adrián Herrera <adr.her.arc.95@gmail.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 6b01c88206..dff1401e9c 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -120,7 +120,10 @@ ArmGicGetMaxNumInterrupts (
>    IN  INTN          GicDistributorBase
>
>
>    )
>
>
>  {
>
>
> -  return 32 * ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F)
> + 1);
>
>
> +  UINT32 ITLinesNumber;
>
>
> +
>
>
> +  ITLinesNumber = MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) &
> 0x1F;
>
>
> +  return MIN (32 * (ITLinesNumber+ 1), 1020);
>
>
>  }
>
>
>
>
>
>  VOID
>
>
> --
> 2.30.0
>
>
>
> 
>
>

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

Re: [edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported

[Sami Mujawar]

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

Hi Adrian,

I think the mailing list discussions here should be sufficient.
However, it would be great if you can test that the earlier patch works and reply with a tested-by.

Regards,

Sami Mujawar

From: Adrián Herrera Arcila <adr.her.arc.95@gmail.com>
Date: Tuesday, 27 April 2021 at 10:48
To: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>, Joey Gouly <Joey.Gouly@arm.com>, Ard Biesheuvel <ardb+tianocore@kernel.org>, leif@nuviainc.com <leif@nuviainc.com>, nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported
Hi Sami,

That patch provides the same solution. I'll be sure to check the mailing list before sending the next time.
Any benefit on marking this as duplicate? If so, how could I do it?

Thank you,
Adrián.

On Mon, 26 Apr 2021 at 08:56, Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>> wrote:
Hi Adrian,

I believe there is already a similar patch on the mailing list at https://edk2.groups.io/g/devel/message/72596. This patch is pending review and tested-by.
Can you check if this patch covers the problems you describe, please?

Regards,

Sami Mujawar

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> on behalf of Adrián Herrera via groups.io<http://groups.io> <adr.her.arc.95=gmail.com@groups.io<mailto:gmail.com@groups.io>>
Date: Saturday, 24 April 2021 at 03:57
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Adrián Herrera <adr.her.arc.95@gmail.com<mailto:adr.her.arc.95@gmail.com>>
Subject: [edk2-devel] [PATCH] ArmPkg/ArmGicLib: fix maximum interrupts supported
The maximum number of interrupts supported is determined as
32 * (GICD_TYPER.ITLinesNumber + 1).

When GICD_TYPER.ITLinesNumber = 0b11111, the maximum number of
interrupts supported is 1024. However, both GICv2 and GICv3 reserve
INTIDs 1020-1023 for special purposes.

This results in runtime crashes because:
  (1) ArmGicLib functions do not guard against special interrupts.
  (2) ArmGicGetMaxNumInterrupts number includes special interrupts.
  (2) ArmGicV*Dxe relies on ArmGicGetMaxNumInterrupts, and thus
      programs special interrupts through ArmGicLib.

ArmGicGetMaxNumInterrupts now does not include special interrupts, that
is, it reports 1020 instead of 1024 when GICD_TYPER.ITLinesNumber = 0b11111.
This avoids the overhead of guarding ArmGicLib functions while not
requiring to modify the drivers code.

Signed-off-by: Adrián Herrera <adr.her.arc.95@gmail.com<mailto:adr.her.arc.95@gmail.com>>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 6b01c88206..dff1401e9c 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -120,7 +120,10 @@ ArmGicGetMaxNumInterrupts (
   IN  INTN          GicDistributorBase


   )


 {


-  return 32 * ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F) + 1);


+  UINT32 ITLinesNumber;


+


+  ITLinesNumber = MmioRead32 (GicDistributorBase + ARM_GIC_ICDICTR) & 0x1F;


+  return MIN (32 * (ITLinesNumber+ 1), 1020);


 }





 VOID


--
2.30.0





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