* [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