* [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
@ 2020-10-28 1:21 quan
2020-11-26 13:09 ` Quan Nguyen
2020-11-27 7:17 ` Ard Biesheuvel
0 siblings, 2 replies; 6+ messages in thread
From: quan @ 2020-10-28 1:21 UTC (permalink / raw)
To: Leif Lindholm, Ard Biesheuvel, devel
Cc: Open Source Review, Victor Gallardo, Quan Nguyen
According to ARM doc IHI 0069F, section 11.9.18, "Accessing the
GICD_IPRIORITYR<n>:", "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<n> is used instead of GICD_IPRIORITYR<n> where n = 0
to 7 (that is, for SGIs and PPIs)."
The current ArmGicV3 code tries to initialize all the IPRIORITYR
registers to a default state via the Distributor (GICD), so skip
writes to the first eight IPRIORITYR registers when Affinity Routing
is Enabled.
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Victor Gallardo <Victor@os.amperecomputing.com>
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index d7da1f198d9e..bc543502481b 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -378,6 +378,7 @@ GicV3DxeInitialize (
UINTN RegShift;
UINT64 CpuTarget;
UINT64 MpId;
+ BOOLEAN AffinityRoutingEnabled = FALSE;
// Make sure the Interrupt Controller Protocol is not already installed in
// the system.
@@ -391,11 +392,21 @@ GicV3DxeInitialize (
// Routing enabled. So ensure that the ARE bit is set.
if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
+ // If Affinity Routing is Enabled, the first 32 interrupts (SGI and PPI)
+ // can be programmed only through Redistributor interface (GICR).
+ // Initializing the GICD_IPRIORITYR registers for these interrupts can be
+ // skipped as the Redistributor will be powered up and initialized
+ // at the appropriate time (e.g. in EL3 by trusted firmware).
+ AffinityRoutingEnabled = TRUE;
}
for (Index = 0; Index < mGicNumInterrupts; Index++) {
GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
+ if (AffinityRoutingEnabled && Index < 32) {
+ continue;
+ }
+
// Set Priority
RegOffset = Index / 4;
RegShift = (Index % 4) * 8;
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
2020-10-28 1:21 [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable quan
@ 2020-11-26 13:09 ` Quan Nguyen
2020-11-27 7:17 ` Ard Biesheuvel
1 sibling, 0 replies; 6+ messages in thread
From: Quan Nguyen @ 2020-11-26 13:09 UTC (permalink / raw)
To: devel@edk2.groups.io
Cc: Open Source Review, Victor Gallardo OS, Leif Lindholm,
Ard Biesheuvel
Dear all,
Any comments about this patch ?
Without this patch there are errors recorded in GICT_ERR<n>STATUS as below:
(https://developer.arm.com/documentation/100336/0002/programmers-model/gict-register-summary/error-record-primary-status-register--gict-err-n-status?lang=en)
+ GICT_ERR<n>STATUS = 0x000000006C20030F
That means:
V = 1: Status register is valid
UE = 1: Un-correctable Error
OF = 1: overflow
MV =1 Misc0 and Misc1 are valid
IERR = 0x3 & SERR=0xF: SYN_GICR_ARE = Attempt to access GICR or GICD registers in mode that cannot work.
+ GICT_ERR<n>MISC0 = 0x0000000000000000
ie: Mis0[8:0] = 0: core ID 0
+ GICT_ERR<n>MISC1 = 0x0000000000000000
Thanks,
Quan
On 10/28/20, 08:22, "Quan Nguyen OS" <quan@os.amperecomputing.com> wrote:
According to ARM doc IHI 0069F, section 11.9.18, "Accessing the
GICD_IPRIORITYR<n>:", "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<n> is used instead of GICD_IPRIORITYR<n> where n = 0
to 7 (that is, for SGIs and PPIs)."
The current ArmGicV3 code tries to initialize all the IPRIORITYR
registers to a default state via the Distributor (GICD), so skip
writes to the first eight IPRIORITYR registers when Affinity Routing
is Enabled.
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Victor Gallardo <Victor@os.amperecomputing.com>
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index d7da1f198d9e..bc543502481b 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -378,6 +378,7 @@ GicV3DxeInitialize (
UINTN RegShift;
UINT64 CpuTarget;
UINT64 MpId;
+ BOOLEAN AffinityRoutingEnabled = FALSE;
// Make sure the Interrupt Controller Protocol is not already installed in
// the system.
@@ -391,11 +392,21 @@ GicV3DxeInitialize (
// Routing enabled. So ensure that the ARE bit is set.
if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
+ // If Affinity Routing is Enabled, the first 32 interrupts (SGI and PPI)
+ // can be programmed only through Redistributor interface (GICR).
+ // Initializing the GICD_IPRIORITYR registers for these interrupts can be
+ // skipped as the Redistributor will be powered up and initialized
+ // at the appropriate time (e.g. in EL3 by trusted firmware).
+ AffinityRoutingEnabled = TRUE;
}
for (Index = 0; Index < mGicNumInterrupts; Index++) {
GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
+ if (AffinityRoutingEnabled && Index < 32) {
+ continue;
+ }
+
// Set Priority
RegOffset = Index / 4;
RegShift = (Index % 4) * 8;
--
2.28.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
2020-10-28 1:21 [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable quan
2020-11-26 13:09 ` Quan Nguyen
@ 2020-11-27 7:17 ` Ard Biesheuvel
2020-11-28 1:43 ` Quan Nguyen
1 sibling, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2020-11-27 7:17 UTC (permalink / raw)
To: Quan Nguyen, Leif Lindholm, devel; +Cc: Open Source Review, Victor Gallardo
On 10/28/20 2:21 AM, Quan Nguyen wrote:
> According to ARM doc IHI 0069F, section 11.9.18, "Accessing the
> GICD_IPRIORITYR<n>:", "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<n> is used instead of GICD_IPRIORITYR<n> where n = 0
> to 7 (that is, for SGIs and PPIs)."
>
> The current ArmGicV3 code tries to initialize all the IPRIORITYR
> registers to a default state via the Distributor (GICD), so skip
> writes to the first eight IPRIORITYR registers when Affinity Routing
> is Enabled.
>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Victor Gallardo <Victor@os.amperecomputing.com>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
Isn't this already being taken into account in ArmGicDisableInterrupt()?
> ---
> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index d7da1f198d9e..bc543502481b 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -378,6 +378,7 @@ GicV3DxeInitialize (
> UINTN RegShift;
> UINT64 CpuTarget;
> UINT64 MpId;
> + BOOLEAN AffinityRoutingEnabled = FALSE;
>
> // Make sure the Interrupt Controller Protocol is not already installed in
> // the system.
> @@ -391,11 +392,21 @@ GicV3DxeInitialize (
> // Routing enabled. So ensure that the ARE bit is set.
> if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
> MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
> + // If Affinity Routing is Enabled, the first 32 interrupts (SGI and PPI)
> + // can be programmed only through Redistributor interface (GICR).
> + // Initializing the GICD_IPRIORITYR registers for these interrupts can be
> + // skipped as the Redistributor will be powered up and initialized
> + // at the appropriate time (e.g. in EL3 by trusted firmware).
> + AffinityRoutingEnabled = TRUE;
> }
>
> for (Index = 0; Index < mGicNumInterrupts; Index++) {
> GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>
> + if (AffinityRoutingEnabled && Index < 32) {
> + continue;
> + }
> +
> // Set Priority
> RegOffset = Index / 4;
> RegShift = (Index % 4) * 8;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
2020-11-27 7:17 ` Ard Biesheuvel
@ 2020-11-28 1:43 ` Quan Nguyen
2020-11-30 9:09 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Quan Nguyen @ 2020-11-28 1:43 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm, devel@edk2.groups.io
Cc: Open Source Review, Victor Gallardo OS
No, Ard,
ArmGicDisableInterrupt() does not access these registers (IPRIORITYR)
-Quan
From: Ard Biesheuvel <ard.biesheuvel@arm.com>
Date: Friday, November 27, 2020 at 14:17
To: Quan Nguyen OS <quan@os.amperecomputing.com>, Leif Lindholm <leif@nuviainc.com>, devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Open Source Review <OpenSourceReview@amperecomputing.com>, Victor Gallardo OS <Victor@os.amperecomputing.com>
Subject: Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
On 10/28/20 2:21 AM, Quan Nguyen wrote:
> According to ARM doc IHI 0069F, section 11.9.18, "Accessing the
> GICD_IPRIORITYR<n>:", "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<n> is used instead of GICD_IPRIORITYR<n> where n = 0
> to 7 (that is, for SGIs and PPIs)."
>
> The current ArmGicV3 code tries to initialize all the IPRIORITYR
> registers to a default state via the Distributor (GICD), so skip
> writes to the first eight IPRIORITYR registers when Affinity Routing
> is Enabled.
>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Victor Gallardo <Victor@os.amperecomputing.com>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
Isn't this already being taken into account in ArmGicDisableInterrupt()?
> ---
> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index d7da1f198d9e..bc543502481b 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -378,6 +378,7 @@ GicV3DxeInitialize (
> UINTN RegShift;
> UINT64 CpuTarget;
> UINT64 MpId;
> + BOOLEAN AffinityRoutingEnabled = FALSE;
>
> // Make sure the Interrupt Controller Protocol is not already installed in
> // the system.
> @@ -391,11 +392,21 @@ GicV3DxeInitialize (
> // Routing enabled. So ensure that the ARE bit is set.
> if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
> MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
> + // If Affinity Routing is Enabled, the first 32 interrupts (SGI and PPI)
> + // can be programmed only through Redistributor interface (GICR).
> + // Initializing the GICD_IPRIORITYR registers for these interrupts can be
> + // skipped as the Redistributor will be powered up and initialized
> + // at the appropriate time (e.g. in EL3 by trusted firmware).
> + AffinityRoutingEnabled = TRUE;
> }
>
> for (Index = 0; Index < mGicNumInterrupts; Index++) {
> GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>
> + if (AffinityRoutingEnabled && Index < 32) {
> + continue;
> + }
> +
> // Set Priority
> RegOffset = Index / 4;
> RegShift = (Index % 4) * 8;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
2020-11-28 1:43 ` Quan Nguyen
@ 2020-11-30 9:09 ` Ard Biesheuvel
2020-12-01 3:46 ` Quan Nguyen
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2020-11-30 9:09 UTC (permalink / raw)
To: Quan Nguyen OS, Leif Lindholm, devel@edk2.groups.io
Cc: Open Source Review, Victor Gallardo OS
On 11/28/20 2:43 AM, Quan Nguyen OS wrote:
> No, Ard,
> ArmGicDisableInterrupt() does not access these registers (IPRIORITYR)
>
OK, understood.
>
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Date: Friday, November 27, 2020 at 14:17
> To: Quan Nguyen OS <quan@os.amperecomputing.com>, Leif Lindholm <leif@nuviainc.com>, devel@edk2.groups.io <devel@edk2.groups.io>
> Cc: Open Source Review <OpenSourceReview@amperecomputing.com>, Victor Gallardo OS <Victor@os.amperecomputing.com>
> Subject: Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
> On 10/28/20 2:21 AM, Quan Nguyen wrote:
>> According to ARM doc IHI 0069F, section 11.9.18, "Accessing the
>> GICD_IPRIORITYR<n>:", "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<n> is used instead of GICD_IPRIORITYR<n> where n = 0
>> to 7 (that is, for SGIs and PPIs)."
>>
>> The current ArmGicV3 code tries to initialize all the IPRIORITYR
>> registers to a default state via the Distributor (GICD), so skip
>> writes to the first eight IPRIORITYR registers when Affinity Routing
>> is Enabled.
>>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Victor Gallardo <Victor@os.amperecomputing.com>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>
> Isn't this already being taken into account in ArmGicDisableInterrupt()?
>
>> ---
>> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> index d7da1f198d9e..bc543502481b 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> @@ -378,6 +378,7 @@ GicV3DxeInitialize (
>> UINTN RegShift;
>> UINT64 CpuTarget;
>> UINT64 MpId;
>> + BOOLEAN AffinityRoutingEnabled = FALSE;
>>
>> // Make sure the Interrupt Controller Protocol is not already installed in
>> // the system.
>> @@ -391,11 +392,21 @@ GicV3DxeInitialize (
>> // Routing enabled. So ensure that the ARE bit is set.
>> if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
>> MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>> + // If Affinity Routing is Enabled, the first 32 interrupts (SGI and PPI)
>> + // can be programmed only through Redistributor interface (GICR).
>> + // Initializing the GICD_IPRIORITYR registers for these interrupts can be
This should be GICR_IPRIORITYR, not GICD_IPRIORITYR, right?
So why does this only apply to SGIs and PPIs? Is it possible that we
don't need to program these priorities in the first place?
Note that a lot of this code dates back from the time when this *was*
the secure world firmware, and perhaps, we can get rid of some of this.
>> + // skipped as the Redistributor will be powered up and initialized
>> + // at the appropriate time (e.g. in EL3 by trusted firmware).
>> + AffinityRoutingEnabled = TRUE;
>> }
>>
>> for (Index = 0; Index < mGicNumInterrupts; Index++) {
>> GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol, Index);
>>
>> + if (AffinityRoutingEnabled && Index < 32) {
>> + continue;
>> + }
>> +
/If/ we need to keep this, I think we can move the priority setting into
GicV3DisableInterruptSource(), instead of modifying the loop like this.
Also, better to use FeaturePcdGet (PcdArmGicV3WithV2Legacy) directly -
no point in using a stack variable here.
>> // Set Priority
>> RegOffset = Index / 4;
>> RegShift = (Index % 4) * 8;
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable
2020-11-30 9:09 ` Ard Biesheuvel
@ 2020-12-01 3:46 ` Quan Nguyen
0 siblings, 0 replies; 6+ messages in thread
From: Quan Nguyen @ 2020-12-01 3:46 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm, devel@edk2.groups.io
Cc: Victor Gallardo OS, Open Source Submission
On 11/30/20 16:09, Ard Biesheuvel wrote:
> On 11/28/20 2:43 AM, Quan Nguyen OS wrote:
>> No, Ard,
>> ArmGicDisableInterrupt() does not access these registers (IPRIORITYR)
>>
>
> OK, understood.
>
>>
>> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Date: Friday, November 27, 2020 at 14:17
>> To: Quan Nguyen OS <quan@os.amperecomputing.com>, Leif Lindholm
>> <leif@nuviainc.com>, devel@edk2.groups.io <devel@edk2.groups.io>
>> Cc: Open Source Review <OpenSourceReview@amperecomputing.com>, Victor
>> Gallardo OS <Victor@os.amperecomputing.com>
>> Subject: Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to
>> GICD_IPRIORITYR<n> when ARE enable
>> On 10/28/20 2:21 AM, Quan Nguyen wrote:
>>> According to ARM doc IHI 0069F, section 11.9.18, "Accessing the
>>> GICD_IPRIORITYR<n>:", "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<n> is used instead of GICD_IPRIORITYR<n> where
>>> n = 0
>>> to 7 (that is, for SGIs and PPIs)."
>>>
>>> The current ArmGicV3 code tries to initialize all the IPRIORITYR
>>> registers to a default state via the Distributor (GICD), so skip
>>> writes to the first eight IPRIORITYR registers when Affinity Routing
>>> is Enabled.
>>>
>>> Cc: Leif Lindholm <leif@nuviainc.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Signed-off-by: Victor Gallardo <Victor@os.amperecomputing.com>
>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>
>> Isn't this already being taken into account in ArmGicDisableInterrupt()?
>>
>>> ---
>>> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++++++++
>>> 1 file changed, 11 insertions(+)
>>>
>>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>> b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>> index d7da1f198d9e..bc543502481b 100644
>>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>> @@ -378,6 +378,7 @@ GicV3DxeInitialize (
>>> UINTN RegShift;
>>> UINT64 CpuTarget;
>>> UINT64 MpId;
>>> + BOOLEAN AffinityRoutingEnabled = FALSE;
>>> // Make sure the Interrupt Controller Protocol is not already
>>> installed in
>>> // the system.
>>> @@ -391,11 +392,21 @@ GicV3DxeInitialize (
>>> // Routing enabled. So ensure that the ARE bit is set.
>>> if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
>>> MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR,
>>> ARM_GIC_ICDDCR_ARE);
>>> + // If Affinity Routing is Enabled, the first 32 interrupts (SGI
>>> and PPI)
>>> + // can be programmed only through Redistributor interface (GICR).
>>> + // Initializing the GICD_IPRIORITYR registers for these
>>> interrupts can be
>
> This should be GICR_IPRIORITYR, not GICD_IPRIORITYR, right?
>
Yes, that's right!
> So why does this only apply to SGIs and PPIs? Is it possible that we
> don't need to program these priorities in the first place?
>
> Note that a lot of this code dates back from the time when this *was*
> the secure world firmware, and perhaps, we can get rid of some of this.
>
>>> + // skipped as the Redistributor will be powered up and initialized
>>> + // at the appropriate time (e.g. in EL3 by trusted firmware).
>>> + AffinityRoutingEnabled = TRUE;
>>> }
>>> for (Index = 0; Index < mGicNumInterrupts; Index++) {
>>> GicV3DisableInterruptSource (&gHardwareInterruptV3Protocol,
>>> Index);
>>> + if (AffinityRoutingEnabled && Index < 32) {
>>> + continue;
>>> + }
>>> +
>
> /If/ we need to keep this, I think we can move the priority setting into
> GicV3DisableInterruptSource(), instead of modifying the loop like this.
> Also, better to use FeaturePcdGet (PcdArmGicV3WithV2Legacy) directly -
> no point in using a stack variable here.
>
Agree, will not use stack variable in v2.
But, somehow, it does not make sense to me to reset to default interrupt
priority every time interrupt disable, ie: GicV3DisableInterruptSource()
is called.
>>> // Set Priority
>>> RegOffset = Index / 4;
>>> RegShift = (Index % 4) * 8;
>>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-01 3:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-28 1:21 [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable quan
2020-11-26 13:09 ` Quan Nguyen
2020-11-27 7:17 ` Ard Biesheuvel
2020-11-28 1:43 ` Quan Nguyen
2020-11-30 9:09 ` Ard Biesheuvel
2020-12-01 3:46 ` Quan Nguyen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox