Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

[edk2] [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

Posted by Sergey Temerkhanov 7 years, 7 months ago

These registers are reserved for PPIs and unimplemented on
some architectures

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 8af97a9..dc6b896 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -257,7 +257,7 @@ GicV3DxeInitialize (
     MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
   }
 
-  for (Index = 0; Index < mGicNumInterrupts; Index++) {
+  for (Index = 32; Index < mGicNumInterrupts; Index++) {
     GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
 
     // Set Priority
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

Posted by Ard Biesheuvel 7 years, 7 months ago

On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
> These registers are reserved for PPIs and unimplemented on
> some architectures
>


What do you mean by 'architectures'? Could you elaborate on which SoC
needs this?

> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> ---
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 8af97a9..dc6b896 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>    }
>
> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>
>      // Set Priority
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

Posted by Sergei Temerkhanov 7 years, 7 months ago

On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
>> These registers are reserved for PPIs and unimplemented on
>> some architectures
>>
>
>
> What do you mean by 'architectures'?

GIC core implementations.

> Could you elaborate on which SoC
> needs this?
At least Cavium ThunderX/OcteonTX need it.

The GICv3 spec says this on the subject:

8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254

"These registers are always used when affinity routing is not enabled.
When affinity routing is enabled for the Security state of an
interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
where n = 0 to 7."

>
>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> ---
>>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> index 8af97a9..dc6b896 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
>>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>>    }
>>
>> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
>> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
>>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>>
>>      // Set Priority
>> --
>> 2.7.4
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

Posted by Leif Lindholm 7 years, 7 months ago

On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
> >> These registers are reserved for PPIs and unimplemented on
> >> some architectures
> >>
> >
> >
> > What do you mean by 'architectures'?
> 
> GIC core implementations.
> 
> > Could you elaborate on which SoC
> > needs this?
> At least Cavium ThunderX/OcteonTX need it.
> 
> The GICv3 spec says this on the subject:
> 
> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
> 
> "These registers are always used when affinity routing is not enabled.
> When affinity routing is enabled for the Security state of an
> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
> where n = 0 to 7."

Since they are RAZ/WI, why is this change needed?

/
    Leif

> >
> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> >> ---
> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> index 8af97a9..dc6b896 100644
> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
> >>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
> >>    }
> >>
> >> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> >> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
> >>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
> >>
> >>      // Set Priority
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

Posted by Sergei Temerkhanov 7 years, 7 months ago

On Fri, May 19, 2017 at 1:26 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
>> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
>> >> These registers are reserved for PPIs and unimplemented on
>> >> some architectures
>> >>
>> >
>> >
>> > What do you mean by 'architectures'?
>>
>> GIC core implementations.
>>
>> > Could you elaborate on which SoC
>> > needs this?
>> At least Cavium ThunderX/OcteonTX need it.
>>
>> The GICv3 spec says this on the subject:
>>
>> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
>>
>> "These registers are always used when affinity routing is not enabled.
>> When affinity routing is enabled for the Security state of an
>> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
>> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
>> where n = 0 to 7."
>
> Since they are RAZ/WI, why is this change needed?

B/c for some GICv3 cores accessing these registers result in exceptions

Regards,
Sergey

>
> /
>     Leif
>
>> >
>> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> >> ---
>> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> >> index 8af97a9..dc6b896 100644
>> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> >> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
>> >>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>> >>    }
>> >>
>> >> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
>> >> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
>> >>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>> >>
>> >>      // Set Priority
>> >> --
>> >> 2.7.4
>> >>
>> >> _______________________________________________
>> >> edk2-devel mailing list
>> >> edk2-devel@lists.01.org
>> >> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

Posted by Leif Lindholm 7 years, 7 months ago

On Fri, May 19, 2017 at 05:31:36PM +0300, Sergei Temerkhanov wrote:
> On Fri, May 19, 2017 at 1:26 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
> >> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org> wrote:
> >> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
> >> >> These registers are reserved for PPIs and unimplemented on
> >> >> some architectures
> >> >>
> >> >
> >> >
> >> > What do you mean by 'architectures'?
> >>
> >> GIC core implementations.
> >>
> >> > Could you elaborate on which SoC
> >> > needs this?
> >> At least Cavium ThunderX/OcteonTX need it.
> >>
> >> The GICv3 spec says this on the subject:
> >>
> >> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
> >>
> >> "These registers are always used when affinity routing is not enabled.
> >> When affinity routing is enabled for the Security state of an
> >> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
> >> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
> >> where n = 0 to 7."
> >
> > Since they are RAZ/WI, why is this change needed?
> 
> B/c for some GICv3 cores accessing these registers result in exceptions

Right, so that's architecturally non-compliant.

At which point, as a workaround for a hardware erratum, I think this
should either be a configurable option (ArmPkg Pcd), or the code
should be modified to use GICR_IPRIORITYR as the text describes (if
applicable).

Regards,

Leif

> Regards,
> Sergey
> 
> >
> > /
> >     Leif
> >
> >> >
> >> >> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> >> >> ---
> >> >>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> >> index 8af97a9..dc6b896 100644
> >> >> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> >> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> >> >> @@ -257,7 +257,7 @@ GicV3DxeInitialize (
> >> >>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
> >> >>    }
> >> >>
> >> >> -  for (Index = 0; Index < mGicNumInterrupts; Index++) {
> >> >> +  for (Index = 32; Index < mGicNumInterrupts; Index++) {
> >> >>      GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
> >> >>
> >> >>      // Set Priority
> >> >> --
> >> >> 2.7.4
> >> >>
> >> >> _______________________________________________
> >> >> edk2-devel mailing list
> >> >> edk2-devel@lists.01.org
> >> >> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Arm: GICv3: Don't access GIC_ICDIPR for interrupts 0..31

Posted by Ard Biesheuvel 7 years, 7 months ago

On 22 May 2017 at 10:45, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, May 19, 2017 at 05:31:36PM +0300, Sergei Temerkhanov wrote:
>> On Fri, May 19, 2017 at 1:26 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Fri, May 19, 2017 at 05:37:02AM +0300, Sergei Temerkhanov wrote:
>> >> On Thu, May 18, 2017 at 7:08 PM, Ard Biesheuvel
>> >> <ard.biesheuvel@linaro.org> wrote:
>> >> > On 16 May 2017 at 03:56, Sergey Temerkhanov <s.temerkhanov@gmail.com> wrote:
>> >> >> These registers are reserved for PPIs and unimplemented on
>> >> >> some architectures
>> >> >>
>> >> >
>> >> >
>> >> > What do you mean by 'architectures'?
>> >>
>> >> GIC core implementations.
>> >>
>> >> > Could you elaborate on which SoC
>> >> > needs this?
>> >> At least Cavium ThunderX/OcteonTX need it.
>> >>
>> >> The GICv3 spec says this on the subject:
>> >>
>> >> 8.9.12 GICD_IPRIORITYR, Interrupt Priority Registers, n = 0 - 254
>> >>
>> >> "These registers are always used when affinity routing is not enabled.
>> >> When affinity routing is enabled for the Security state of an
>> >> interrupt: • GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where
>> >> n = 0 to 7 (that is, for SGIs and PPIs). • GICD_IPRIORITYR is RAZ/WI
>> >> where n = 0 to 7."
>> >
>> > Since they are RAZ/WI, why is this change needed?
>>
>> B/c for some GICv3 cores accessing these registers result in exceptions
>
> Right, so that's architecturally non-compliant.
>
> At which point, as a workaround for a hardware erratum, I think this
> should either be a configurable option (ArmPkg Pcd), or the code
> should be modified to use GICR_IPRIORITYR as the text describes (if
> applicable).
>

That is a good point. The intention of the code is to disable all
interrupt sources, so it is incorrect to begin with, given that PPIs
and SGIs need to be disabled at the respective redistributor of each
CPU. Fix that, and it should no longer raise exceptions on the broken
hardware.

-- 
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] MdeModulePkg: Fix undefined behavior on variadic parameters

Posted by Sergey Temerkhanov 7 years, 7 months ago

Fix undefined behavior by avoiding automatic type promotion

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 0a325de..03605b9 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -1611,7 +1611,7 @@ CHAR8 *
 EFIAPI
 VariableGetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages,
-  IN BOOLEAN      Iso639Language,
+  IN UINTN      Iso639Language,
   ...
   )
 {
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] MdePkg: Fix undefined behavior on variadic parameters

Posted by Sergey Temerkhanov 7 years, 7 months ago

Fix undefined behavior by avoiding parameter type promotion

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 MdePkg/Include/Library/UefiLib.h | 2 +-
 MdePkg/Library/UefiLib/UefiLib.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 0b14792..4e4697c 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -818,7 +818,7 @@ CHAR8 *
 EFIAPI
 GetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages, 
-  IN BOOLEAN      Iso639Language,
+  IN UINTN      Iso639Language,
   ...
   );
 
diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
index a7eee01..74528ec 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -1514,7 +1514,7 @@ CHAR8 *
 EFIAPI
 GetBestLanguage (
   IN CONST CHAR8  *SupportedLanguages, 
-  IN BOOLEAN      Iso639Language,
+  IN UINTN      Iso639Language,
   ...
   )
 {
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel