[v1] ArmPkg: Arm GIC Library and Driver improvements

[edk2-devel] [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt

Posted by Sami Mujawar 1 year, 5 months ago

The IAR register of the Gic CPU interface is 32 bit, while
the value returned by ArmGicV2AcknowledgeInterrupt() is
UINTN. Therefore, typecast the return value to UINTN before
returning.

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

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
index f403bec367b5254c248e620e56471904e520f9f2..80115b243afabd5e4faad88089af738b19ce4cd1 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
@@ -16,7 +16,7 @@ ArmGicV2AcknowledgeInterrupt (
   )
 {
   // Read the Interrupt Acknowledge Register
-  return MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
+  return (UINTN)MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
 }
 
 VOID
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'



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

Re: [edk2-devel] [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt

Posted by Pedro Falcato 1 year, 5 months ago

On Tue, May 23, 2023 at 2:04 PM Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> The IAR register of the Gic CPU interface is 32 bit, while
> the value returned by ArmGicV2AcknowledgeInterrupt() is
> UINTN. Therefore, typecast the return value to UINTN before
> returning.

Since IAR is 32-bit, doesn't it make sense to return UINT32 here? Is
this for compatibility with GICv3 or...?

>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> index f403bec367b5254c248e620e56471904e520f9f2..80115b243afabd5e4faad88089af738b19ce4cd1 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> @@ -16,7 +16,7 @@ ArmGicV2AcknowledgeInterrupt (
>    )
>  {
>    // Read the Interrupt Acknowledge Register
> -  return MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
> +  return (UINTN)MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
>  }
You don't need to cast from UINT32 to UINTN IMO. UINTN is, as far as I
know, always going to be at least 32-bits, so casting makes little
sense here, in my view, as UINTN >= UINT32.

-- 
Pedro


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

Re: [edk2-devel] [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt

Posted by Sami Mujawar 1 year, 5 months ago

Hi Pedro,

Thank you for the feedback.

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 23/05/2023 02:32 pm, Pedro Falcato wrote:
> On Tue, May 23, 2023 at 2:04 PM Sami Mujawar<sami.mujawar@arm.com>  wrote:
>> The IAR register of the Gic CPU interface is 32 bit, while
>> the value returned by ArmGicV2AcknowledgeInterrupt() is
>> UINTN. Therefore, typecast the return value to UINTN before
>> returning.
> Since IAR is 32-bit, doesn't it make sense to return UINT32 here? Is
> this for compatibility with GICv3 or...?

[SAMI] IAR in GICv3 is 32-bit as well. I think this is done to keep 
compatibility with HARDWARE_INTERRUPT_SOURCE which is UINTN.

>> Signed-off-by: Sami Mujawar<sami.mujawar@arm.com>
>> ---
>>   ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> index f403bec367b5254c248e620e56471904e520f9f2..80115b243afabd5e4faad88089af738b19ce4cd1 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> @@ -16,7 +16,7 @@ ArmGicV2AcknowledgeInterrupt (
>>     )
>>   {
>>     // Read the Interrupt Acknowledge Register
>> -  return MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
>> +  return (UINTN)MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
>>   }
> You don't need to cast from UINT32 to UINTN IMO. UINTN is, as far as I
> know, always going to be at least 32-bits, so casting makes little
> sense here, in my view, as UINTN >= UINT32.
[SAMI] Ack. I will drop this patch.
>


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