[PATCH edk2/ArmPkg v1 0/1] Fix Gic interrupt routing modes bug

[PATCH edk2/ArmPkg v1 0/1] Fix Gic interrupt routing modes bug

[Ming Huang]

As GicV3 Spec, Interrupt Routing Modes should be 0 for
routing the SPIs to the primary CPU.

BTW:
Edk2 latest commit(2f6693c283b5) has build error:
make[2]: Entering directory
'/home/huangming/source/new/edk2/BaseTools/Source/C/BrotliCompress'
gcc  -c -I ./include -I .. -I ../Include/Common -I ../Include/ -I
../Include/IndustryStandard -I ../Common/ -I .. -I . -I ../Include/X64/  -MD
-fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-deprecated-declarations
-Wno-stringop-truncation -Wno-restrict -Wno-unused-result -nostdlib -g -O2
tools/brotli.c -o tools/brotli.o
make[2]: *** No rule to make target 'common/././types.h', needed by
'common/dictionary.o'.  Stop.
make[2]: Leaving directory
'/home/huangming/source/new/edk2/BaseTools/Source/C/BrotliCompress'
GNUmakefile:85: recipe for target 'BrotliCompress' failed
make[1]: *** [BrotliCompress] Error 2
make[1]: Leaving directory '/home/huangming/source/new/edk2/BaseTools/Source/C'
GNUmakefile:25: recipe for target 'Source/C' failed
make: *** [Source/C] Error 2


Ming Huang (1):
  ArmPkg: Fix Gic interrupt routing modes bug

 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.18.0

[PATCH edk2/ArmPkg v1 1/1] ArmPkg: Fix Gic interrupt routing modes bug

[Ming Huang]

As GicV3 Spec, Interrupt Routing Modes should be 0 for
routing the SPIs to the primary CPU.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ming Huang <ming.huang@linaro.org>
---
 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 01154848f443..1558db31713a 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -469,7 +469,7 @@ GicV3DxeInitialize (
     for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {
       MmioWrite32 (
         mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8),
-        CpuTarget | ARM_GICD_IROUTER_IRM
+        CpuTarget
         );
     }
   }
-- 
2.18.0

Re: [PATCH edk2/ArmPkg v1 1/1] ArmPkg: Fix Gic interrupt routing modes bug

[Ard Biesheuvel]

(+ Marc)


On 29 October 2018 at 05:57, Ming Huang <ming.huang@linaro.org> wrote:
> As GicV3 Spec, Interrupt Routing Modes should be 0 for
> routing the SPIs to the primary CPU.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> ---
>  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 01154848f443..1558db31713a 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -469,7 +469,7 @@ GicV3DxeInitialize (
>      for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {
>        MmioWrite32 (
>          mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8),
> -        CpuTarget | ARM_GICD_IROUTER_IRM
> +        CpuTarget
>          );
>      }
>    }
> --
> 2.18.0
>

Re: [PATCH edk2/ArmPkg v1 1/1] ArmPkg: Fix Gic interrupt routing modes bug

[Ard Biesheuvel]

On 7 November 2018 at 16:07, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 07/11/18 14:48, Ard Biesheuvel wrote:
>> (+ Marc)
>>
>>
>> On 29 October 2018 at 05:57, Ming Huang <ming.huang@linaro.org> wrote:
>>> As GicV3 Spec, Interrupt Routing Modes should be 0 for
>>> routing the SPIs to the primary CPU.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>>> ---
>>>  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 01154848f443..1558db31713a 100644
>>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>> @@ -469,7 +469,7 @@ GicV3DxeInitialize (
>>>      for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {
>>>        MmioWrite32 (
>>>          mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8),
>>> -        CpuTarget | ARM_GICD_IROUTER_IRM
>>> +        CpuTarget
>
> That's a very odd thing indeed. IRM being set means "broadcast to all
> CPUs, ignoring the affinity", and there are a number of problems with that:
>
> - There is no guarantee that it is actually implemented
> - Setting this bit *and* the affinity is like saying "yes" and "no" at
> the same time
>
> So as far as I can see, this patch is correct, assuming that CpuTarget
> is set to something sensible. What is a bit odd is the commit message,
> which doesn't really explain the problem. How about something like:
>
> <commit>
> Setting the GICD_IROUTERn.IMR and GICD_IROUTERn.{Aff3, Aff2, Aff1, Aff0}
> at the same time is nonsensical (see 8.9.13 in the GICv3 spec, which
> says of GICD_IROUTERn.IMR that "When this bit is set to 1,
> GICD_IROUTER<n>.{Aff3, Aff2, Aff1, Aff0} are UNKNOWN"). There is also no
> guarantee that IMR is implemented (see GICD_TYPER.No1N which indicates
> whether the implementation supports this or not).
>
> Let's thus not set this bit, as we want all SPIs to be delivered to the
> same CPU, and not be broadcast to all of them.
> </commit>
>
> Otherwise:
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>

Thanks Marc

Pushed as  0adc6eae9480..b66e38b50134, with the commit log updated as
per your suggestion