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

["Ming Huang" <huangming@linux.alibaba.com>]

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
>>