From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (NAM11-CO1-obe.outbound.protection.outlook.com [52.100.173.246]) by mx.groups.io with SMTP id smtpd.web11.5187.1606794410580548245 for ; Mon, 30 Nov 2020 19:46:50 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@os.amperecomputing.com header.s=selector2 header.b=cBRYAnBU; spf=pass (domain: os.amperecomputing.com, ip: 52.100.173.246, mailfrom: quan@os.amperecomputing.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cqLz+jR7t9LMettAXJajlV3gpRMfh3FCh8EotSBgvwieroOqgho6apVyvZwIgNuQrBTYdoABW40/BP886U1qG0Kj7RMUJ246nLFdGW8X+1LaboMX9UlxID55ajvP88icTQgMWG0+4Ts4L73UwHuIU4b1RJFl8+UtLGahehbteH0ZH4yfEbLJdP2J6Q+gJIRhCyscidVDv7/BE9v5bONQx0SEfP3qCxQ/tVU/o9lGVccyfUIK7PK83K1iw+cCk/S/HOZhpZ7SEXNr5tmi+75uxlLQiu39mKaU21yNA3svTzeznToy3p64cgjMMaVewhb9XPLukv0G2clyBH+n1CVOKA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=9jx1QnJvNDEoAhEAbZC8Ofr8fRCrZxRtqigCFqBkg/M=; b=Lk00OccWwCF69HmruMFHibNIxNqjacdVOHdrTXKJgIA/Vq11F4rEKBGBz6BUHqsE+hfWcrCkvARzm+D3p/2f4z+Ib+aDJR0nq23PEEZSe94SLUS6uqzBYvMvu9CNFGXELMXEKe3MNGbK/jPoLbK3L6KRnLY++ZyqumFtaYTgxQqXXVlA+icS1zs/B+G4Gr+Z04riTXsI3AivP+BVqadE60LmVJPnD+gknXu3ACWja7/Y5ucO45JdQJ1pkriGKw69+PokEsaOEzKX+xOUUgfHXOQXRVTWyoJUn/A5sGJHCHPEna8sfeqwEPfi5HDbkJUAJ1y90+OiFOB9Bsm2On2BIA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=os.amperecomputing.com; dmarc=pass action=none header.from=os.amperecomputing.com; dkim=pass header.d=os.amperecomputing.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=os.amperecomputing.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=9jx1QnJvNDEoAhEAbZC8Ofr8fRCrZxRtqigCFqBkg/M=; b=cBRYAnBUgJN+0jUAADRyx34ZZWDSbEZsUX1rie4V2pw3+q5Gg7gAv6G59D7Sm5ZwrnX6tbblYnQZvBWEv0/HR0dMeQuCBOL9NNIA2EI+GyJKzRpo8cXmqM+14tWDMvt3oTA59yn9EFH4TI5gwx6o/cT03zYgXSzM0Eud98svnks= Authentication-Results: amperecomputing.com; dkim=none (message not signed) header.d=none;amperecomputing.com; dmarc=none action=none header.from=os.amperecomputing.com; Received: from MW2PR0102MB3482.prod.exchangelabs.com (2603:10b6:302:c::32) by MWHPR0101MB2992.prod.exchangelabs.com (2603:10b6:301:2f::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3611.31; Tue, 1 Dec 2020 03:46:45 +0000 Received: from MW2PR0102MB3482.prod.exchangelabs.com ([fe80::9978:c933:a050:a0e8]) by MW2PR0102MB3482.prod.exchangelabs.com ([fe80::9978:c933:a050:a0e8%7]) with mapi id 15.20.3611.025; Tue, 1 Dec 2020 03:46:45 +0000 Subject: Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to GICD_IPRIORITYR when ARE enable To: Ard Biesheuvel , Leif Lindholm , "devel@edk2.groups.io" CC: Victor Gallardo OS , Open Source Submission References: <20201028012112.12125-1-quan@os.amperecomputing.com> <12e96332-995e-3267-e79e-afa15fdf4d50@arm.com> From: "Quan Nguyen" Message-ID: <546cb7a1-c8df-0248-a2eb-2e014c95d53c@os.amperecomputing.com> Date: Tue, 1 Dec 2020 10:46:26 +0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 In-Reply-To: <12e96332-995e-3267-e79e-afa15fdf4d50@arm.com> X-Originating-IP: [118.69.219.201] X-ClientProxiedBy: HK0PR01CA0068.apcprd01.prod.exchangelabs.com (2603:1096:203:a6::32) To MW2PR0102MB3482.prod.exchangelabs.com (2603:10b6:302:c::32) Return-Path: quan@os.amperecomputing.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.0.100] (118.69.219.201) by HK0PR01CA0068.apcprd01.prod.exchangelabs.com (2603:1096:203:a6::32) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3611.20 via Frontend Transport; Tue, 1 Dec 2020 03:46:42 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: bc75bea9-e2e7-41ce-af89-08d895abb8a8 X-MS-TrafficTypeDiagnostic: MWHPR0101MB2992: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:5236; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 7cMtMbvAtxM0K2wRw4cJiHTEhSXe6jSNajGXHbMq9XESZwCXaEgH83A+hEv5T8gJP/xw1D6g7NLZ5TCsFCMcC3rRtMZdaaNAKkvYOv7H+n0mQKfBg1e1qR9uWoUXwLZPcCuLrkCY+v7hvHLMEoIxl2lOKFFggRoS2Xzi+KylDvk3K3wDe6OzqFlj/9S85w0W3vMAp6QkEyWD6W7qe4wbWaJ1JkyNgbWbYUkpSksQHzGlWHnmX2kzEaXkjpv2bOopShWrH7/F+J35Kb+vIFXItPDmdyQGz5My6R3kUMzJuj0Clpt9DD7YyDbL28Q3q8Xz90tO3Ai8cpmUgQzZLdmMiNo7wYrwwmK0T8wqB6njjuDVzppMBRQ3vJQw02ZPkm/NuObWZ73nQy4Yb+2SKRQKVah34c+CS2rW8robdDa28k5XglIYXjJ+mB4/E2RgF5DhzWCGNGPeSTGpiRTJJ0SCnCvWFwv0fjTtR/vbTe84U1PxXvqYks1h7lrr6zC2ZrN67BcSmexZOKuYDmuQjAfzOw1007OWVjVDgoY6YCk5KR+52c8VzCYDBjp1osCaccknzi7yUR35SzXBAjrJO8CcKOa9v8cz1xRVb4wQfUz62wwCNWJ/CiVCRDMF6Sxt83IM/Cr9UrEr8bqyI7ZJk5r78fIqvkyfxY94Rw7aWgstUtpUKBb3D48e6Kb4iu95al+DL8fwap5gMCievPHPFGYHC7RrhlHOAsbB3NcPgO5ZqRM= X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:5;SRV:;IPV:NLI;SFV:SPM;H:MW2PR0102MB3482.prod.exchangelabs.com;PTR:;CAT:OSPM;SFS:(4636009)(376002)(39850400004)(346002)(366004)(136003)(396003)(83380400001)(52116002)(26005)(31696002)(19627235002)(110136005)(2906002)(107886003)(66946007)(8936002)(16526019)(53546011)(66476007)(66556008)(186003)(5660300002)(6486002)(86362001)(956004)(478600001)(2616005)(8676002)(6666004)(16576012)(54906003)(4326008)(316002)(31686004)(43740500002)(11215385002);DIR:OUT;SFP:1501; X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?wots6WseVCdyvv2Ezl3xZrp/XH/TwKt/k35f+m5iMOiZj8fueijH3Cv95CO2?= =?us-ascii?Q?rN65a78oXAzLKcHXCLGo35LQFh27hhO3aBVtbnUMqk+LCMT1XbfrArpwmgvV?= =?us-ascii?Q?BUNuw5S8Qq+h1Ll+hrMvY+S/OBnEGKuzx1iRT35JnOtPQwlWhjHqbvJarUGm?= =?us-ascii?Q?2oLn4DodcLllLEfKkaInTs7eGqdZz7Ds1MTIdjadPgOMRk6TRjj3KKHhWoYj?= =?us-ascii?Q?+faF+exgu1kNkFqKopeEblE40VQ+B20oE20EaTx/7s004NvfVjsHxVQrInmJ?= =?us-ascii?Q?sbZNODUdHI28UWH79bFSZMgUNe/HV5xrVqRlHwR7qd/xPdp0VAI47szJAgCp?= =?us-ascii?Q?200ng4p21fcy9gmPptabi/3DBluwuQgiGw+IhtA1/SNxKE7+7RkVF+A/0BNi?= =?us-ascii?Q?TcV4Bao6c2vxhnLQ1hSCDVoHZzosEYx8qtUBDg0fd53VQrwEbI0TZGN9FF5W?= =?us-ascii?Q?jBJ2RrbsfxKsx8wc5wx6qaldRMnbPvgy6Cl4z5Rrktr0qd7fPxuIgVE9yQQY?= =?us-ascii?Q?hhjwFTaYs+LuvpjjwAxbzLah/R+o1zKXQ3XzO5xQoRqAKvyrQKTlyTNgLL6P?= =?us-ascii?Q?94s3qqS7yY5/n/2xc75AvLt7K4zziKTsuawtLLLzrgkX5JSuooYMdioYDc/I?= =?us-ascii?Q?aqsPlgcmZuHlWzUj19MnWwOUKZMR+a7BZxQp8J/KZ4DCgrA7Ol5idI4rZDXR?= =?us-ascii?Q?9AItiX4oAqvlKvW9dkEj2iudDdXG7WM8yajdhP+Zi0p5QOYvbeltraXbJN8T?= =?us-ascii?Q?f08TdCh8JqFSS71cdZ/+X+2m5InFAj136bV/JIE8iSL+WxwtxBt/p6o71lpZ?= =?us-ascii?Q?IF7B1VccvppEcFjg0hwU6PFNGUeDGNBgeL/ODBKxClBarXyDLwy8mHiU/bXO?= =?us-ascii?Q?3KH+zvwPGQhW3BWwXv1sAFs+IRdfOgPbCxOiJiwitKTKGcqk+cnLb426pK4c?= =?us-ascii?Q?TkRxeUZOycu1XIYl9ndCiCv6pQEF1BylHvp55cldCMjvAjBrh+DEyGu94eJg?= =?us-ascii?Q?Igy+?= X-OriginatorOrg: os.amperecomputing.com X-MS-Exchange-CrossTenant-Network-Message-Id: bc75bea9-e2e7-41ce-af89-08d895abb8a8 X-MS-Exchange-CrossTenant-AuthSource: MW2PR0102MB3482.prod.exchangelabs.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2020 03:46:44.6122 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7 X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: Hs/nXX/itK9rqU1wRNKYgzXCkwCOh1TS1PNNYv7ODsewH69pBTnUrIML1o+MuuwiaM+JoIbp9sNbLUp0EAJHxWkIaT5WB1pGhF4n4mlSeoY= X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR0101MB2992 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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) >> >=20 > OK, understood. >=20 >> >> From: Ard Biesheuvel >> Date: Friday, November 27, 2020 at 14:17 >> To: Quan Nguyen OS , Leif Lindholm=20 >> , devel@edk2.groups.io >> Cc: Open Source Review , Victor=20 >> Gallardo OS >> Subject: Re: [PATCH v1 1/1] ArmPkg/ArmGicDxe: fix writes to=20 >> GICD_IPRIORITYR 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=C2=A0 * GICR_IPRIORITYR is used instead of GICD_IPRIORI= TYR where=20 >>> 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 >> >> Isn't this already being taken into account in ArmGicDisableInterrupt()? >> >>> --- >>> =C2=A0=C2=A0=C2=A0 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 11 +++++= ++++++ >>> =C2=A0=C2=A0=C2=A0 1 file changed, 11 insertions(+) >>> >>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c=20 >>> 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 ( >>> =C2=A0=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= RegShift; >>> =C2=A0=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 CpuTa= rget; >>> =C2=A0=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= ; >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Make sure the Interrupt Controller Pr= otocol is not already=20 >>> installed in >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // the system. >>> @@ -391,11 +392,21 @@ GicV3DxeInitialize ( >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Routing enabled. So ensure that the A= RE bit is set. >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!FeaturePcdGet (PcdArmGicV3WithV2Leg= acy)) { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MmioOr32 (mGicDistributorBas= e + ARM_GIC_ICDDCR,=20 >>> ARM_GIC_ICDDCR_ARE); >>> +=C2=A0=C2=A0=C2=A0 // If Affinity Routing is Enabled, the first 32 int= errupts (SGI=20 >>> and PPI) >>> +=C2=A0=C2=A0=C2=A0 // can be programmed only through Redistributor int= erface (GICR). >>> +=C2=A0=C2=A0=C2=A0 // Initializing the GICD_IPRIORITYR registers for t= hese=20 >>> interrupts can be >=20 > This should be GICR_IPRIORITYR, not GICD_IPRIORITYR, right? >=20 Yes, that's 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? >=20 > 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. >=20 >>> +=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=C2=A0 } >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for (Index =3D 0; Index < mGicNumInterru= pts; Index++) { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GicV3DisableInterruptSource = (&gHardwareInterruptV3Protocol,=20 >>> Index); >>> +=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 } >>> + >=20 > /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. >=20 Agree, will not use stack variable in v2. But, somehow, it does not make sense to me to reset to default interrupt=20 priority every time interrupt disable, ie: GicV3DisableInterruptSource()=20 is called. >>> =C2=A0=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=C2=A0 RegOffset =3D Index / 4; >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegShift =3D (Index % 4) * 8= ; >>> >=20