[v1] ArmPkg: Arm GIC Library and Driver improvements

[edk2-devel] [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt

Posted by Sami Mujawar 1 year, 5 months ago

The EIOR register of the Gic CPU interface is a 32 bit register.
However, the HARDWARE_INTERRUPT_SOURCE used to represent the
interrupt source (Interrupt ID) is typedefed as UINTN, see
EmbeddedPkg\Include\Protocol\HardwareInterrupt.h

Therfore, typecast the interrupt ID (Source) value to UINT32
before setting the EOIR register. Also, add an assert to check
that the value does not exceed 32 bits.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
index 80115b243afabd5e4faad88089af738b19ce4cd1..e98cd9705616e7a8dfc7aaba7c80b176f8f6d0c9 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
@@ -7,6 +7,7 @@
 **/
 
 #include <Library/ArmGicLib.h>
+#include <Library/DebugLib.h>
 #include <Library/IoLib.h>
 
 UINTN
@@ -26,5 +27,6 @@ ArmGicV2EndOfInterrupt (
   IN UINTN   Source
   )
 {
-  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, Source);
+  ASSERT (Source < MAX_UINT32);
+  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, (UINT32)Source);
 }
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105175): https://edk2.groups.io/g/devel/message/105175
Mute This Topic: https://groups.io/mt/99086450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt

Posted by Ard Biesheuvel 1 year, 5 months ago

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> The EIOR register of the Gic CPU interface is a 32 bit register.
> However, the HARDWARE_INTERRUPT_SOURCE used to represent the
> interrupt source (Interrupt ID) is typedefed as UINTN, see
> EmbeddedPkg\Include\Protocol\HardwareInterrupt.h
>
> Therfore, typecast the interrupt ID (Source) value to UINT32
> before setting the EOIR register. Also, add an assert to check
> that the value does not exceed 32 bits.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> index 80115b243afabd5e4faad88089af738b19ce4cd1..e98cd9705616e7a8dfc7aaba7c80b176f8f6d0c9 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> @@ -7,6 +7,7 @@
>  **/
>
>  #include <Library/ArmGicLib.h>
> +#include <Library/DebugLib.h>
>  #include <Library/IoLib.h>
>
>  UINTN
> @@ -26,5 +27,6 @@ ArmGicV2EndOfInterrupt (
>    IN UINTN   Source
>    )
>  {
> -  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, Source);
> +  ASSERT (Source < MAX_UINT32);

Should this be <= ?

> +  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, (UINT32)Source);
>  }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105187): https://edk2.groups.io/g/devel/message/105187
Mute This Topic: https://groups.io/mt/99086450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt

Posted by Sami Mujawar 1 year, 5 months ago

Hi Ard,

Thank you for the feedback.

I will fix this in the next series.

Regards,

Sami Mujawar

On 23/05/2023 02:26 pm, Ard Biesheuvel wrote:
> On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>> The EIOR register of the Gic CPU interface is a 32 bit register.
>> However, the HARDWARE_INTERRUPT_SOURCE used to represent the
>> interrupt source (Interrupt ID) is typedefed as UINTN, see
>> EmbeddedPkg\Include\Protocol\HardwareInterrupt.h
>>
>> Therfore, typecast the interrupt ID (Source) value to UINT32
>> before setting the EOIR register. Also, add an assert to check
>> that the value does not exceed 32 bits.
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>   ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> index 80115b243afabd5e4faad88089af738b19ce4cd1..e98cd9705616e7a8dfc7aaba7c80b176f8f6d0c9 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> @@ -7,6 +7,7 @@
>>   **/
>>
>>   #include <Library/ArmGicLib.h>
>> +#include <Library/DebugLib.h>
>>   #include <Library/IoLib.h>
>>
>>   UINTN
>> @@ -26,5 +27,6 @@ ArmGicV2EndOfInterrupt (
>>     IN UINTN   Source
>>     )
>>   {
>> -  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, Source);
>> +  ASSERT (Source < MAX_UINT32);
> Should this be <= ?
>
>> +  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, (UINT32)Source);
>>   }
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>>
>>
>> 
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105198): https://edk2.groups.io/g/devel/message/105198
Mute This Topic: https://groups.io/mt/99086450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-