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

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

[Ming Huang]

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))
+
 /**
  *
  * 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
       );
-- 
2.17.1

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

[Ard Biesheuvel]

On Tue, 23 Feb 2021 at 12:07, Ming Huang <huangming@linux.alibaba.com> 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.

NOTE: missing sign-off

Patch seems fine to me

Tested-by: Ard Biesheuvel <ardb@kernel.org> # QEMU/kvm guest on ThunderX2



> ---
>  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))
> +
>  /**
>   *
>   * 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
>        );
> --
> 2.17.1
>

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

[Leif Lindholm]

On Tue, Feb 23, 2021 at 13:42:49 +0100, Ard Biesheuvel wrote:
> On Tue, 23 Feb 2021 at 12:07, Ming Huang <huangming@linux.alibaba.com> 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.
> 
> NOTE: missing sign-off
> 
> Patch seems fine to me
> 
> Tested-by: Ard Biesheuvel <ardb@kernel.org> # QEMU/kvm guest on ThunderX2

Also looks good in QEMU.
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Ming, can you confirm that the contribution conforms to the developer
certificate of origin by replyint with your Signed-off-by: ?

> 
> 
> 
> > ---
> >  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))
> > +
> >  /**
> >   *
> >   * 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
> >        );
> > --
> > 2.17.1
> >

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

[Heyi Guo]

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

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

[Leif Lindholm]

On Tue, Feb 23, 2021 at 20:58:14 +0800, Heyi Guo wrote:
> 
> 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.

You are correct.

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

If Ming could spin that at the same time as adding Signed-off-by, that
would be ideal.

Thanks!

/
    Leif

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

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

[Ming Huang]

On 2/23/21 8:58 PM, Leif Lindholm wrote:
> On Tue, Feb 23, 2021 at 13:42:49 +0100, Ard Biesheuvel wrote:
>> On Tue, 23 Feb 2021 at 12:07, Ming Huang <huangming@linux.alibaba.com> 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.
>>
>> NOTE: missing sign-off
>>
>> Patch seems fine to me
>>
>> Tested-by: Ard Biesheuvel <ardb@kernel.org> # QEMU/kvm guest on ThunderX2
> 
> Also looks good in QEMU.
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> 
> Ming, can you confirm that the contribution conforms to the developer
> certificate of origin by replyint with your Signed-off-by: ?

Sorry, I miss the Signed-off-by.
please help to add my Signed-off-by:
Signed-off-by: Ming Huang <huangming@linux.alibaba.com>

Thanks,
Ming

> 
>>
>>
>>
>>> ---
>>>  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))
>>> +
>>>  /**
>>>   *
>>>   * 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
>>>        );
>>> --
>>> 2.17.1
>>>

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

[Leif Lindholm]

Hi Ming,

Thanks for this.
However, just after I sent my reply, Heyi suggested another
improvement to this patch. Can you please implement that, and submit a
v2?

Best Regards,

Leif

On Wed, Feb 24, 2021 at 16:26:33 +0800, Ming Huang wrote:
> 
> 
> On 2/23/21 8:58 PM, Leif Lindholm wrote:
> > On Tue, Feb 23, 2021 at 13:42:49 +0100, Ard Biesheuvel wrote:
> >> On Tue, 23 Feb 2021 at 12:07, Ming Huang <huangming@linux.alibaba.com> 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.
> >>
> >> NOTE: missing sign-off
> >>
> >> Patch seems fine to me
> >>
> >> Tested-by: Ard Biesheuvel <ardb@kernel.org> # QEMU/kvm guest on ThunderX2
> > 
> > Also looks good in QEMU.
> > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> > 
> > Ming, can you confirm that the contribution conforms to the developer
> > certificate of origin by replyint with your Signed-off-by: ?
> 
> Sorry, I miss the Signed-off-by.
> please help to add my Signed-off-by:
> Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
> 
> Thanks,
> Ming
> 
> > 
> >>
> >>
> >>
> >>> ---
> >>>  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))
> >>> +
> >>>  /**
> >>>   *
> >>>   * 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
> >>>        );
> >>> --
> >>> 2.17.1
> >>>
> 
> 
> 
> 
> 

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

[Ming Huang]

On 2/24/21 7:49 PM, Leif Lindholm wrote:
> Hi Ming,
> 
> Thanks for this.
> However, just after I sent my reply, Heyi suggested another
> improvement to this patch. Can you please implement that, and submit a
> v2?

OK, I will implement that in v2.
As one issue in my email filter, I miss Heyi's email, sorry for that.

Thanks,
Ming

> 
> Best Regards,
> 
> Leif
> 
> On Wed, Feb 24, 2021 at 16:26:33 +0800, Ming Huang wrote:
>>
>>
>> On 2/23/21 8:58 PM, Leif Lindholm wrote:
>>> On Tue, Feb 23, 2021 at 13:42:49 +0100, Ard Biesheuvel wrote:
>>>> On Tue, 23 Feb 2021 at 12:07, Ming Huang <huangming@linux.alibaba.com> 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.
>>>>
>>>> NOTE: missing sign-off
>>>>
>>>> Patch seems fine to me
>>>>
>>>> Tested-by: Ard Biesheuvel <ardb@kernel.org> # QEMU/kvm guest on ThunderX2
>>>
>>> Also looks good in QEMU.
>>> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>>>
>>> Ming, can you confirm that the contribution conforms to the developer
>>> certificate of origin by replyint with your Signed-off-by: ?
>>
>> Sorry, I miss the Signed-off-by.
>> please help to add my Signed-off-by:
>> Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
>>
>> Thanks,
>> Ming
>>
>>>
>>>>
>>>>
>>>>
>>>>> ---
>>>>>  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))
>>>>> +
>>>>>  /**
>>>>>   *
>>>>>   * 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
>>>>>        );
>>>>> --
>>>>> 2.17.1
>>>>>
>>
>>
>> 
>>
>>