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(-)
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 ); } }
(+ 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
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,
M.
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
CpuTargetThat'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
-
Ard Biesheuvel -
Marc Zyngier -
Ming Huang