Re: [edk2-devel] [PATCH edk2 v2 1/1] ArmPkg/ArmGic: Fix GICR_IPRIORITYR address wrong issue

["Heyi Guo" <guoheyi@linux.alibaba.com>]

On 2021/2/23 下午7:06, Ming Huang wrote:
> The register address of GICR_IPRIORITYR is in SGI_base frame. Add
> IPRIORITY_ADDRESS macro for getting GICR_IPRIORITYR address. Otherwise
> GIC RAS error(Uncorrected software error) may report in ArmGicDxe.
> ---
>   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 8ef32b33a1..b4d3965acb 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -30,6 +30,9 @@
>   #define ICENABLER_ADDRESS(base,offset) ((base) + \
>             ARM_GICR_CTLR_FRAME_SIZE +  ARM_GICR_ICENABLER + (4 * offset))
>   
> +#define IPRIORITY_ADDRESS(base,offset) ((base) + \
> +          ARM_GICR_CTLR_FRAME_SIZE +  ARM_GIC_ICDIPR + (4 * offset))

For macro parameter, I think we need to put "offset" in parentheses, and 
it is not necessary to put 4 * offset inside parentheses.

How about adding one more patch to fix the existing macros 
ICENABLER_ADDRESS and ISENABLER_ADDRESS first and then this patch?

Thanks,

Heyi


> +
>   /**
>    *
>    * Return whether the Source interrupt index refers to a shared interrupt (SPI)
> @@ -236,7 +239,7 @@ ArmGicSetInterruptPriority (
>       }
>   
>       MmioAndThenOr32 (
> -      GicCpuRedistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
> +      IPRIORITY_ADDRESS (GicCpuRedistributorBase, RegOffset),
>         ~(0xff << RegShift),
>         Priority << RegShift
>         );