[PATCH edk2 v1 1/1] ArmPkg/ArmGicLib: Fix GICR_IPRIORITYR address wrong issue

[PATCH edk2 v1 1/1] ArmPkg/ArmGicLib: Fix GICR_IPRIORITYR address wrong issue

[Ming Huang]

The address of GICR_IPRIORITYR is in SGI_base frame. ARM_GICR_CTLR_FRAME_SIZE
should add to GicCpuRedistributorBase for GICR_IPRIORITYR. Otherwise RAS
error(Uncorrected software error) will reported in ArmGicDxe.

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 8ef32b33a1..7a54972455 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -235,6 +235,9 @@ ArmGicSetInterruptPriority (
       return;
     }
 
+    // The address of GICR_IPRIORITYR is in SGI_base frame.
+    // ARM_GICR_CTLR_FRAME_SIZE should add to GicCpuRedistributorBase for GICR_IPRIORITYR.
+    GicCpuRedistributorBase += ARM_GICR_CTLR_FRAME_SIZE;
     MmioAndThenOr32 (
       GicCpuRedistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
       ~(0xff << RegShift),
-- 
2.17.1

Re: [PATCH edk2 v1 1/1] ArmPkg/ArmGicLib: Fix GICR_IPRIORITYR address wrong issue

[Leif Lindholm]

Hi Ming,

On Sat, Feb 20, 2021 at 15:08:39 +0800, Ming Huang wrote:
> The address of GICR_IPRIORITYR is in SGI_base frame. ARM_GICR_CTLR_FRAME_SIZE
> should add to GicCpuRedistributorBase for GICR_IPRIORITYR. Otherwise RAS
> error(Uncorrected software error) will reported in ArmGicDxe.
> 
> Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 8ef32b33a1..7a54972455 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -235,6 +235,9 @@ ArmGicSetInterruptPriority (
>        return;
>      }
>  
> +    // The address of GICR_IPRIORITYR is in SGI_base frame.
> +    // ARM_GICR_CTLR_FRAME_SIZE should add to GicCpuRedistributorBase for GICR_IPRIORITYR.
> +    GicCpuRedistributorBase += ARM_GICR_CTLR_FRAME_SIZE;

I agree with the error report, but not the fix.
Changing the value of a variable called GicCpuRedistributorBase to
something that is not the base address of the redistributor makes the
code confusing.

If you look at the subsequent function, ArmGicEnableInterrupt, it
resolvess the same situation using the ISENABLER_ADDRESS macro
defined at the top of the file:

#define ISENABLER_ADDRESS(base,offset) ((base) + \
          ARM_GICR_CTLR_FRAME_SIZE +  ARM_GICR_ISENABLER + (4 * offset))

I would suggest creating an IPRIORITY_ADDRESS macro by the same
pattern and using that.

Would that solution be OK with you?

Best Regards,

Leif

>      MmioAndThenOr32 (
>        GicCpuRedistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
>        ~(0xff << RegShift),
> -- 
> 2.17.1
> 

Re: [PATCH edk2 v1 1/1] ArmPkg/ArmGicLib: Fix GICR_IPRIORITYR address wrong issue

[Ming Huang]

On 2/22/21 8:52 PM, Leif Lindholm wrote:
> Hi Ming,
> 
> On Sat, Feb 20, 2021 at 15:08:39 +0800, Ming Huang wrote:
>> The address of GICR_IPRIORITYR is in SGI_base frame. ARM_GICR_CTLR_FRAME_SIZE
>> should add to GicCpuRedistributorBase for GICR_IPRIORITYR. Otherwise RAS
>> error(Uncorrected software error) will reported in ArmGicDxe.
>>
>> Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
>> ---
>>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
>> index 8ef32b33a1..7a54972455 100644
>> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
>> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
>> @@ -235,6 +235,9 @@ ArmGicSetInterruptPriority (
>>        return;
>>      }
>>  
>> +    // The address of GICR_IPRIORITYR is in SGI_base frame.
>> +    // ARM_GICR_CTLR_FRAME_SIZE should add to GicCpuRedistributorBase for GICR_IPRIORITYR.
>> +    GicCpuRedistributorBase += ARM_GICR_CTLR_FRAME_SIZE;
> 
> I agree with the error report, but not the fix.
> Changing the value of a variable called GicCpuRedistributorBase to
> something that is not the base address of the redistributor makes the
> code confusing.
> 
> If you look at the subsequent function, ArmGicEnableInterrupt, it
> resolvess the same situation using the ISENABLER_ADDRESS macro
> defined at the top of the file:
> 
> #define ISENABLER_ADDRESS(base,offset) ((base) + \
>           ARM_GICR_CTLR_FRAME_SIZE +  ARM_GICR_ISENABLER + (4 * offset))
> 
> I would suggest creating an IPRIORITY_ADDRESS macro by the same
> pattern and using that.
> 
> Would that solution be OK with you?

Good idea. Thanks for your solution.
Modify it in v2.

Thanks,
Ming

> 
> Best Regards,
> 
> Leif
> 
>>      MmioAndThenOr32 (
>>        GicCpuRedistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
>>        ~(0xff << RegShift),
>> -- 
>> 2.17.1
>>