Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR<n> when ARE enable

["Quan Nguyen" <quan@os.amperecomputing.com>]

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;
>>>
>