From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web08.57510.1606727368976373718 for ; Mon, 30 Nov 2020 01:09:29 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DDD3C1FB; Mon, 30 Nov 2020 01:09:27 -0800 (PST) Received: from [192.168.1.81] (unknown [10.37.8.38]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E4C7B3F23F; Mon, 30 Nov 2020 01:09:25 -0800 (PST) Subject: Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR when ARE enable To: Quan Nguyen OS , Leif Lindholm , "devel@edk2.groups.io" Cc: Open Source Review , Victor Gallardo OS References: <20201028012112.12125-1-quan@os.amperecomputing.com> From: "Ard Biesheuvel" Message-ID: <12e96332-995e-3267-e79e-afa15fdf4d50@arm.com> Date: Mon, 30 Nov 2020 10:09:23 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 11/28/20 2:43 AM, Quan Nguyen OS wrote: > No, Ard, > ArmGicDisableInterrupt() does not access these registers (IPRIORITYR) >=20 OK, understood. >=20 > From: Ard Biesheuvel > Date: Friday, November 27, 2020 at 14:17 > To: Quan Nguyen OS , Leif Lindholm , devel@edk2.groups.io > Cc: Open Source Review , Victor G= allardo OS > Subject: Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIOR= ITYR 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:", "These registers are always used when affinity >> routing is not enabled. When affinity routing is enabled for the >> Security state of an interrupt: >> =C2=A0=C2=A0 * GICR_IPRIORITYR is used instead of GICD_IPRIORITYR<= n> where n =3D 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 >> Cc: Ard Biesheuvel >> Signed-off-by: Victor Gallardo >> Signed-off-by: Quan Nguyen >=20 > Isn't this already being taken into account in ArmGicDisableInterrupt()= ? >=20 >> --- >> =C2=A0=C2=A0 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++++++= ++ >> =C2=A0=C2=A0 1 file changed, 11 insertions(+) >> >> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Driver= s/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 ( >> =C2=A0=C2=A0=C2=A0=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Reg= Shift; >> =C2=A0=C2=A0=C2=A0=C2=A0 UINT64=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CpuTarget= ; >> =C2=A0=C2=A0=C2=A0=C2=A0 UINT64=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MpId; >> +=C2=A0 BOOLEAN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 AffinityRoutingEnabled =3D FALSE; >> =20 >> =C2=A0=C2=A0=C2=A0=C2=A0 // Make sure the Interrupt Controller Protoc= ol is not already installed in >> =C2=A0=C2=A0=C2=A0=C2=A0 // the system. >> @@ -391,11 +392,21 @@ GicV3DxeInitialize ( >> =C2=A0=C2=A0=C2=A0=C2=A0 // Routing enabled. So ensure that the ARE b= it is set. >> =C2=A0=C2=A0=C2=A0=C2=A0 if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)= ) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MmioOr32 (mGicDistributorBase + = ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE); >> +=C2=A0=C2=A0=C2=A0 // If Affinity Routing is Enabled, the first 32 in= terrupts (SGI and PPI) >> +=C2=A0=C2=A0=C2=A0 // can be programmed only through Redistributor in= terface (GICR). >> +=C2=A0=C2=A0=C2=A0 // 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=20 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*=20 the secure world firmware, and perhaps, we can get rid of some of this. >> +=C2=A0=C2=A0=C2=A0 // skipped as the Redistributor will be powered up= and initialized >> +=C2=A0=C2=A0=C2=A0 // at the appropriate time (e.g. in EL3 by trusted= firmware). >> +=C2=A0=C2=A0=C2=A0 AffinityRoutingEnabled =3D TRUE; >> =C2=A0=C2=A0=C2=A0=C2=A0 } >> =20 >> =C2=A0=C2=A0=C2=A0=C2=A0 for (Index =3D 0; Index < mGicNumInterrupts;= Index++) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GicV3DisableInterruptSource (&gH= ardwareInterruptV3Protocol, Index); >> =20 >> +=C2=A0=C2=A0=C2=A0 if (AffinityRoutingEnabled && Index < 32) { >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; >> +=C2=A0=C2=A0=C2=A0 } >> + /If/ we need to keep this, I think we can move the priority setting into=20 GicV3DisableInterruptSource(), instead of modifying the loop like this. Also, better to use FeaturePcdGet (PcdArmGicV3WithV2Legacy) directly -=20 no point in using a stack variable here. >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Set Priority >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegOffset =3D Index / 4; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegShift =3D (Index % 4) * 8; >>