From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR05-DB8-obe.outbound.protection.outlook.com (EUR05-DB8-obe.outbound.protection.outlook.com [40.107.20.77]) by mx.groups.io with SMTP id smtpd.web11.11686.1639057364583157363 for ; Thu, 09 Dec 2021 05:42:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@armh.onmicrosoft.com header.s=selector2-armh-onmicrosoft-com header.b=Xxnesy3U; spf=pass (domain: arm.com, ip: 40.107.20.77, mailfrom: christopher.jones@arm.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=CsFiJTf/ZFYzd6ScV1hOC3Cv6pQt/rvbscjhrYhLjGk=; b=Xxnesy3UGjDF9zwJsUY+jf2EfxbagDhRscoZXZuxYQ56GOur56YFJJt9NalThWKsqM8B4YyH3VITKjfSp4bNSHh1lpDGv2M34dlTF13LkUMYlmjYPKYa1Qui7qUyyclyNpez5vG4dYNCV1Cat0vzODvbZK34RRB4cuGE5kU3aik= Received: from DB6P192CA0003.EURP192.PROD.OUTLOOK.COM (2603:10a6:4:b8::13) by AM8PR08MB5827.eurprd08.prod.outlook.com (2603:10a6:20b:1da::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4755.16; Thu, 9 Dec 2021 13:42:41 +0000 Received: from DB5EUR03FT056.eop-EUR03.prod.protection.outlook.com (2603:10a6:4:b8:cafe::de) by DB6P192CA0003.outlook.office365.com (2603:10a6:4:b8::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4755.17 via Frontend Transport; Thu, 9 Dec 2021 13:42:41 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by DB5EUR03FT056.mail.protection.outlook.com (10.152.21.124) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4778.12 via Frontend Transport; Thu, 9 Dec 2021 13:42:41 +0000 Received: ("Tessian outbound dbb52aec1fa6:v110"); Thu, 09 Dec 2021 13:42:41 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: 4e0d5f46af13cf5b X-CR-MTA-TID: 64aa7808 Received: from d6b693853c99.2 by 64aa7808-outbound-1.mta.getcheckrecipient.com id E1D99C97-7155-4C05-81A3-84B50CDCFFE6.1; Thu, 09 Dec 2021 13:42:35 +0000 Received: from EUR04-HE1-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id d6b693853c99.2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Thu, 09 Dec 2021 13:42:35 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=SbMrRFujs27lnE2mmFg1JOe0ty4+N5VjX8mPjk8HqerORM9/lt1Wdu/n9ipAGsJlP//c3SWPCmDHQSuvcU/8AKCFTGB76tgSZ1Bs8z5Q1oUYBu3XBMn2qCRtQK8XcciskRX1I1jOwK0fo8mk5L8MBT1C0qOZTSxeUNzjf0freXvZewdc0p3WJ/8axUbjzYZ8sN7sR0+jYSu5JcWI0THpfI3kiCr+1+PeH1sVUlknEyKOifV6yR+1/yFiA5XrcdVDt9sQliHO+k+V8T+wGHLKzgJ7H1DI2Jm7mnnywNjOBC+TjBJHWWDD+2+duyApS/k4P+RfDYeZouBBhaD0RutWKg== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=CsFiJTf/ZFYzd6ScV1hOC3Cv6pQt/rvbscjhrYhLjGk=; b=TPQveHV9uXqiiPCZA2k+fe4XMuDacbIbfsOYOIcEjmHdtT3hG09dmQpXvZndzabUcmoY9cSZqnSx/bIFJp/NqnhqRD/5rF2AO5d5z+zX0M7PsJ2CJEUzTcYbo4v8pazn6aafSm813AyUERwN4aC0c0y66TbHdDTqcZO2ffYfMo5MA7wJd6pPdm4EVe50BDWoBI215CboE/nq5hdIAjBa8hxcQy7+QH5VYQ8rgcpmrQ5zXYUWMOtCtfKREAvr5NogXqp6GZRmYIO5yOyNhMSvqLC20BBdOnIql19bngXOhBc8mGBuH3SjS6p9OMH/i9nSQtsU3z2Mfpt1KP/r9W3HOA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=CsFiJTf/ZFYzd6ScV1hOC3Cv6pQt/rvbscjhrYhLjGk=; b=Xxnesy3UGjDF9zwJsUY+jf2EfxbagDhRscoZXZuxYQ56GOur56YFJJt9NalThWKsqM8B4YyH3VITKjfSp4bNSHh1lpDGv2M34dlTF13LkUMYlmjYPKYa1Qui7qUyyclyNpez5vG4dYNCV1Cat0vzODvbZK34RRB4cuGE5kU3aik= Received: from VE1PR08MB5758.eurprd08.prod.outlook.com (2603:10a6:800:1a0::11) by VI1PR08MB3790.eurprd08.prod.outlook.com (2603:10a6:803:bc::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4755.20; Thu, 9 Dec 2021 13:42:32 +0000 Received: from VE1PR08MB5758.eurprd08.prod.outlook.com ([fe80::c81e:87f6:288c:db76]) by VE1PR08MB5758.eurprd08.prod.outlook.com ([fe80::c81e:87f6:288c:db76%9]) with mapi id 15.20.4755.022; Thu, 9 Dec 2021 13:42:32 +0000 From: "Chris Jones" To: Sami Mujawar , "devel@edk2.groups.io" CC: "michael.d.kinney@intel.com" , "gaoliming@byosoft.com.cn" , "zhiguang.liu@intel.com" , "ray.ni@intel.com" , "zhichao.gao@intel.com" , Alexei Fedorov , nd Subject: Re: [PATCH v3 7/7] DynamicTablesPkg: Add CacheId to PPTT generator Thread-Topic: [PATCH v3 7/7] DynamicTablesPkg: Add CacheId to PPTT generator Thread-Index: AQHX7P9m/mUW5D7MGkKkXiUMTKeuI6wqKqbu Date: Thu, 9 Dec 2021 13:42:32 +0000 Message-ID: References: <20211208160630.10923-1-christopher.jones@arm.com> <20211208160630.10923-8-christopher.jones@arm.com> <0d19cc10-6b30-d48d-d407-0f5a12228b35@arm.com> In-Reply-To: <0d19cc10-6b30-d48d-d407-0f5a12228b35@arm.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: suggested_attachment_session_id: 33204630-e948-5e0b-0f53-78c4007760f2 Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; X-MS-Office365-Filtering-Correlation-Id: 052c06fb-56f8-49e5-1062-08d9bb19c5c7 x-ms-traffictypediagnostic: VI1PR08MB3790:EE_|DB5EUR03FT056:EE_|AM8PR08MB5827:EE_ X-Microsoft-Antispam-PRVS: x-checkrecipientrouted: true nodisclaimer: true x-ms-oob-tlc-oobclassifiers: OLM:10000;OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: LTOEA4s3N2NmYfyf2XddgItNRq46VAebEUK4Aav2wov+ErvbOxC1UazX0B0VloltTimK3VDdtpv9DR6Ua45oxxNr1/g+W5Ch7DrVkoy32UlqR2BaL5vQLSwVtC+i+MK/DeeeZcqKjlNuRHBoouKKLj+dNsJTCeW9X6r18OEAOJ5uU860+gka0lRc5tlYVwsVEMdgJTGX9JW44kmwsAHCH4v1xO9f+xngBG1ECbj/IO31dbBxpC7LvAoQCNdRuF7Y2abNCNrqu4KTsijT0McgdPw26DIh77+/nz7zZ+LME0qms4/3+y0omqYabpyJGacCRpM8z2rXzoJZHWrrw+KD1XPES7Npt67bwTZXBoi3vMNA2abh6v5FTRbWe6E0Qqkb1WzrHVLpK9604KXrlegrssSSRp3lzmsrdHbHWhOkpOVh1al86+jkgbhFmpyrgFzlNo/1P6q7pgVANxECXQMSEzXwPgofYxl+vkScMnyZyN1K8S92mX4qgfYpbyYaof55PXbnvcgF6uLanr0XhenqMM4YhOFaPQUQ/l8hUs68ouFqB7T8s4gXA0wRztb7tJ0eKx6XruNl5bMCV0P/zzhsqon6VaACc9GpLb39xfDKFg10QIb7thoXyKS9tlgrQgp0eN07kRktfz7SI7kW1XIRDXjq7+vSeDiz2XEDB2tAmVYIDWq+e21EbIe0WwmZTfT4tx+4I0xF6S2VCG8lad4V1m1nJZgYLTqw5qIuyk5l2zk5QI+8gGuXAq+ZWh1kfcCVlUHi8HfLgVEulbAfyA4W/1CcDdmG7ATGy1RolbLSCzc= X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:VE1PR08MB5758.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(38100700002)(71200400001)(166002)(33656002)(186003)(66446008)(64756008)(19627405001)(110136005)(2906002)(54906003)(122000001)(83380400001)(316002)(38070700005)(86362001)(53546011)(26005)(9686003)(5660300002)(4326008)(66946007)(8676002)(52536014)(55016003)(508600001)(66476007)(6506007)(8936002)(66556008)(76116006)(7696005);DIR:OUT;SFP:1101; MIME-Version: 1.0 X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR08MB3790 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Return-Path: Christopher.Jones@arm.com X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: DB5EUR03FT056.eop-EUR03.prod.protection.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: fdd6b60a-ed58-4d37-78c9-08d9bb19c059 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 5MPDy9kaXJzxlnBy0Fx/mCy7G4QM2Zd7vcfs1wbuTu6RbVO2kBO61gBxIcI7HSaOUbfS7itidDCeXPKl2e7MJfq/vQx5UWKX70ib/0/IWkjQ9dOHi8qMxHC6szcU+9ZOW77+Lj4BcNNynorh7FZC9FjTaZt0iELy7w7sEI0PncWW7GJy+3ng1jzj6qRefZqOhQuTNis+MCJz+qDhmJgj4F49ywqIJ/whGDUAch2nRILme81Rlc+Ummx2pQDrOcn7OP67YGIJVwJF77BE+HBBq+GV8BNQ4zMFh5PrvGRwOEB/KS1e9f+6V0oJRj6J7st2zxc2K7xt3EYrIr9TwCVg1dlWocUHaA1BDcph/8cr6lyWuLHcwfS6lwZOxj30Yiy4GrpmspUk8MLEgpe8j4UpJr8h/uTevZavcxxaW9HQbaB5yIewxPc6gy/2WOaOjemkaX9pMkn1/orLgbPWdiasP9E1y1JK/tD3km/uoql7U49m118LyJyV3jJJjAta33wW36tlAEZTa475m9ULZJsbh7PR6nK0KXfawkd5hR2FEwr1QiyyQesg3rXLOCXfJhpNBgn/i1ObudArqobBQSr2WWD0f/1pjFolIlFwilEFezCbeHweGWkudS5oPdqslyy1dqUXtyu0CYM0UIGbtU2n2hSRS0n+wXodya4Xn2lWyRhpo9dJ3UZ0jT8+svM2ADH96DHBbL/PL6s1X+5i+1cJ0gbwuemDI4TiJkm7qgPYfYp+bezntCZBRxjboIwJhuE5t+XRqVfpQbmfIc4u8f4f1I/jUeqQcx/eA634fDdoqGhtPcfiLXHujfOZBo2UVEy9Z+9ZxcwYVozMwALcA1WD5w== X-Forefront-Antispam-Report: CIP:63.35.35.123;CTRY:IE;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:64aa7808-outbound-1.mta.getcheckrecipient.com;PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com;CAT:NONE;SFS:(4636009)(46966006)(36840700001)(40470700001)(86362001)(166002)(316002)(8936002)(40460700001)(54906003)(7696005)(33656002)(336012)(2906002)(47076005)(508600001)(70206006)(83380400001)(26005)(19627405001)(81166007)(9686003)(186003)(52536014)(53546011)(55016003)(4326008)(110136005)(356005)(6506007)(30864003)(8676002)(82310400004)(5660300002)(36860700001)(70586007);DIR:OUT;SFP:1101; X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2021 13:42:41.6213 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 052c06fb-56f8-49e5-1062-08d9bb19c5c7 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: DB5EUR03FT056.eop-EUR03.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM8PR08MB5827 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_VE1PR08MB5758FCDBBD3526044C597EFEEF709VE1PR08MB5758eurp_" --_000_VE1PR08MB5758FCDBBD3526044C597EFEEF709VE1PR08MB5758eurp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Sami, The optimisation looks good to me and I would be glad if you added it to th= e patch. Thanks, Chris ________________________________ From: Sami Mujawar Sent: Thursday, December 9, 2021 1:19 PM To: Christopher Jones ; devel@edk2.groups.io Cc: michael.d.kinney@intel.com ; gaoliming@byos= oft.com.cn ; zhiguang.liu@intel.com ; ray.ni@intel.com ; zhichao.gao@intel.com ; Alexei Fedorov ; nd Subject: Re: [PATCH v3 7/7] DynamicTablesPkg: Add CacheId to PPTT generator Hi Chris, Thank you for this patch. Please see my feedback below inline marked [SAMI]. Regards, Sami Mujawar On 08/12/2021 04:06 PM, Chris Jones wrote: > Bugzilla: 3697 (https://bugzilla.tianocore.org/show_bug.cgi?id=3D3697) > > Update the PPTT generator with the CacheId field as defined in table > 5.140 of the ACPI 6.4 specification. > > Also add validations to ensure that the cache id generated is unique. > > Signed-off-by: Chris Jones > --- > DynamicTablesPkg/Include/ArmNameSpaceObjects.h | 4 = +- > DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 102 = ++++++++++++++++++-- > 2 files changed, 96 insertions(+), 10 deletions(-) > > diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTabl= esPkg/Include/ArmNameSpaceObjects.h > index 3246e8884723ac85340bf880a3859800726be3c1..6ea03fca487b96577b8fd8105= bc3d22047ff5697 100644 > --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > @@ -741,10 +741,12 @@ typedef struct CmArmCacheInfo { > /// PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX. Therfore this field > /// is 32-bit wide. > UINT32 Associativity; > - /// Cache attributes (ACPI 6.3 - January 2019, PPTT, Table 5-156) > + /// Cache attributes (ACPI 6.4 - January 2021, PPTT, Table 5.140) > UINT8 Attributes; > /// Line size in bytes > UINT16 LineSize; > + /// Unique ID for the cache > + UINT32 CacheId; > } CM_ARM_CACHE_INFO; > > /** A structure that describes a reference to another Configuration Man= ager > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerat= or.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c > index 3d416ca78ec16a1929ede87abbe4f8f4464ef0cf..6b74572ea2dd8478f14d013e6= cb7394216e45d8d 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c > @@ -726,6 +726,35 @@ AddProcHierarchyNodes ( > return Status; > } > > +/** > + Test whether CacheId is unique among the CacheIdList. > + > + @param [in] CacheId Cache ID to check. > + @param [in] CacheIdList List of already existing cache IDs. > + @param [in] CacheIdListSize Size of CacheIdList. > + > + @retval TRUE CacheId does not exist in CacheIdList. > + @retval FALSE CacheId already exists in CacheIdList. > +**/ > +STATIC > +BOOLEAN > +IsCacheIdUnique ( > + IN CONST UINT32 CacheId, > + IN CONST UINT32 *CacheIdList, > + IN CONST UINT32 CacheIdListSize > + ) > +{ > + UINT32 Index; > + > + for (Index =3D 0; Index < CacheIdListSize; Index++) { > + if (CacheIdList[Index] =3D=3D CacheId) { > + return FALSE; > + } > + } > + > + return TRUE; > +} > + > /** > Update the Cache Type Structure (Type 1) information. > > @@ -738,10 +767,12 @@ AddProcHierarchyNodes ( > @param [in] Pptt Pointer to PPTT table structure. > @param [in] NodesStartOffset Offset from the start of PPTT table= to the > start of Cache Type Structures. > + @param [in] Revision Revision of the PPTT table being req= uested. > > @retval EFI_SUCCESS Structures updated successfully. > @retval EFI_INVALID_PARAMETER A parameter is invalid. > @retval EFI_NOT_FOUND A required object was not found. > + @retval EFI_OUT_OF_RESOURCES Out of resources. > **/ > STATIC > EFI_STATUS > @@ -749,7 +780,8 @@ AddCacheTypeStructures ( > IN CONST ACPI_PPTT_GENERATOR *CONST Ge= nerator, > IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST Cf= gMgrProtocol, > IN CONST EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_HEADER *P= ptt, > - IN CONST UINT32 Nod= esStartOffset > + IN CONST UINT32 Nod= esStartOffset, > + IN CONST UINT32 Rev= ision > ) > { > EFI_STATUS Status; > @@ -758,6 +790,9 @@ AddCacheTypeStructures ( > CM_ARM_CACHE_INFO *CacheInfoNode; > PPTT_NODE_INDEXER *CacheNodeIterator; > UINT32 NodeCount; > + BOOLEAN CacheIdUnique; > + UINT32 TotalNodeCount; [SAMI] I think we could do slight optimisation by doing the following below= : (a) change TotalNode Count to NodeIndex. > + UINT32 *FoundCacheIds; > > ASSERT ( > (Generator !=3D NULL) && > @@ -770,6 +805,13 @@ AddCacheTypeStructures ( > > CacheNodeIterator =3D Generator->CacheStructIndexedList; > NodeCount =3D Generator->CacheStructCount; > + TotalNodeCount =3D NodeCount; > + > + FoundCacheIds =3D AllocateZeroPool (TotalNodeCount * sizeof (*FoundCac= heIds)); [SAMI] (b) Change TotalNodeCount to NodeCount. > + if (FoundCacheIds =3D=3D NULL) { > + DEBUG ((DEBUG_ERROR, "ERROR: PPTT: Failed to allocate resources.\n")= ); > + return EFI_OUT_OF_RESOURCES; > + } > > while (NodeCount-- !=3D 0) { [SAMI] (c) Replace while loop to a for loop where NodeIndes =3D 0 to NodeCount. > CacheInfoNode =3D (CM_ARM_CACHE_INFO *)CacheNodeIterator->Object; > @@ -789,6 +831,7 @@ AddCacheTypeStructures ( > CacheStruct->Flags.CacheTypeValid =3D 1; > CacheStruct->Flags.WritePolicyValid =3D 1; > CacheStruct->Flags.LineSizeValid =3D 1; > + CacheStruct->Flags.CacheIdValid =3D 1; > CacheStruct->Flags.Reserved =3D 0; > > // Populate the reference to the next level of cache > @@ -811,7 +854,7 @@ AddCacheTypeStructures ( > CacheInfoNode->Token, > Status > )); > - return Status; > + goto cleanup; > } > > // Update Cache Structure with the offset for the next level of c= ache > @@ -835,7 +878,7 @@ AddCacheTypeStructures ( > CacheInfoNode->NumberOfSets, > Status > )); > - return Status; > + goto cleanup; > } > > if (CacheInfoNode->NumberOfSets > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX= ) { > @@ -862,7 +905,7 @@ AddCacheTypeStructures ( > CacheInfoNode->Associativity, > Status > )); > - return Status; > + goto cleanup; > } > > // Validate the Associativity field based on the architecture speci= fication > @@ -881,7 +924,7 @@ AddCacheTypeStructures ( > CacheInfoNode->Associativity, > Status > )); > - return Status; > + goto cleanup; > } > > if (CacheInfoNode->Associativity > PPTT_ARM_CACHE_ASSOCIATIVITY_MAX= ) { > @@ -923,7 +966,7 @@ AddCacheTypeStructures ( > CacheInfoNode->LineSize, > Status > )); > - return Status; > + goto cleanup; > } > > if ((CacheInfoNode->LineSize & (CacheInfoNode->LineSize - 1)) !=3D = 0) { > @@ -935,18 +978,58 @@ AddCacheTypeStructures ( > CacheInfoNode->LineSize, > Status > )); > - return Status; > + goto cleanup; > } > > CacheStruct->LineSize =3D CacheInfoNode->LineSize; > > + if (Revision >=3D 3) { > + // Validate and populate cache id > + if (CacheInfoNode->CacheId =3D=3D 0) { > + Status =3D EFI_INVALID_PARAMETER; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: PPTT: The cache id cannot be zero. Status =3D %r\n", > + Status > + )); > + goto cleanup; > + } > + > + CacheIdUnique =3D IsCacheIdUnique ( > + CacheInfoNode->CacheId, > + FoundCacheIds, > + TotalNodeCount [SAMI] (d) Replace TotalNodeCount with NodeIndex in call above. By doing (a) to (d) and (e) below we can reduce the number of iterations in IsCacheIdUnique (). If you agree I will make the adjustments before pushing this patch series. [/SAMI] > + ); > + if (!CacheIdUnique) { > + Status =3D EFI_INVALID_PARAMETER; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: PPTT: The cache id is not unique. " \ > + "CacheId =3D %d. Status =3D %r\n", > + CacheInfoNode->CacheId, > + Status > + )); > + goto cleanup; > + } > + > + // Store the cache id so we can check future cache ids for uniquen= ess > + FoundCacheIds[NodeCount] =3D CacheInfoNode->CacheId; [SAMI] (e) Replace NodeCount with NodeIndex above. > + > + CacheStruct->CacheId =3D CacheInfoNode->CacheId; > + } > + > // Next Cache Type Structure > CacheStruct =3D (EFI_ACPI_6_4_PPTT_STRUCTURE_CACHE *)((UINT8 *)Cach= eStruct + > CacheStruct->Le= ngth); > CacheNodeIterator++; > } // Cache Type Structure > > - return EFI_SUCCESS; > + Status =3D EFI_SUCCESS; > + > +cleanup: > + FreePool (FoundCacheIds); > + > + return Status; > } > > /** > @@ -1205,7 +1288,8 @@ BuildPpttTable ( > Generator, > CfgMgrProtocol, > Pptt, > - CacheStructOffset > + CacheStructOffset, > + AcpiTableInfo->AcpiTableRevision > ); > if (EFI_ERROR (Status)) { > DEBUG (( --_000_VE1PR08MB5758FCDBBD3526044C597EFEEF709VE1PR08MB5758eurp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
Hi Sami,
The optimisation looks good to me and I would be glad if you added it to th= e patch.


Thanks,
Chris
From: Sami Mujawar <Sami= .Mujawar@arm.com>
Sent: Thursday, December 9, 2021 1:19 PM
To: Christopher Jones <Christopher.Jones@arm.com>; devel@edk2.= groups.io <devel@edk2.groups.io>
Cc: michael.d.kinney@intel.com <michael.d.kinney@intel.com>; g= aoliming@byosoft.com.cn <gaoliming@byosoft.com.cn>; zhiguang.liu@inte= l.com <zhiguang.liu@intel.com>; ray.ni@intel.com <ray.ni@intel.com= >; zhichao.gao@intel.com <zhichao.gao@intel.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; nd <nd@arm.com>
Subject: Re: [PATCH v3 7/7] DynamicTablesPkg: Add CacheId to PPTT ge= nerator
 
Hi Chris,

Thank you for this patch.

Please see my feedback below inline marked [SAMI].

Regards,

Sami Mujawar


On 08/12/2021 04:06 PM, Chris Jones wrote:
> Bugzilla: 3697 (https://bugzilla.tianocore.org/show_bug.cgi?id=3D3697)
>
> Update the PPTT generator with the CacheId field as defined in table > 5.140 of the ACPI 6.4 specification.
>
> Also add validations to ensure that the cache id generated is unique.<= br> >
> Signed-off-by: Chris Jones <christopher.jones@arm.com>
> ---
>   DynamicTablesPkg/Include/ArmNameSpaceObjects.h  =             &nb= sp;    |   4 +-
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGener= ator.c | 102 ++++++++++++++++++--
>   2 files changed, 96 insertions(+), 10 deletions(-)
>
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicT= ablesPkg/Include/ArmNameSpaceObjects.h
> index 3246e8884723ac85340bf880a3859800726be3c1..6ea03fca487b96577b8fd8= 105bc3d22047ff5697 100644
> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> @@ -741,10 +741,12 @@ typedef struct CmArmCacheInfo {
>     /// PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX. Th= erfore this field
>     /// is 32-bit wide.
>     UINT32      &nbs= p;      Associativity;
> -  /// Cache attributes (ACPI 6.3 - January 2019, PPTT, Table 5-1= 56)
> +  /// Cache attributes (ACPI 6.4 - January 2021, PPTT, Table 5.1= 40)
>     UINT8       = ;       Attributes;
>     /// Line size in bytes
>     UINT16      &nbs= p;      LineSize;
> +  /// Unique ID for the cache
> +  UINT32         &n= bsp;   CacheId;
>   } CM_ARM_CACHE_INFO;
>  
>   /** A structure that describes a reference to another Conf= iguration Manager
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGene= rator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c<= br> > index 3d416ca78ec16a1929ede87abbe4f8f4464ef0cf..6b74572ea2dd8478f14d01= 3e6cb7394216e45d8d 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c=
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c=
> @@ -726,6 +726,35 @@ AddProcHierarchyNodes (
>     return Status;
>   }
>  
> +/**
> +  Test whether CacheId is unique among the CacheIdList.
> +
> +  @param [in]  CacheId      &= nbsp;   Cache ID to check.
> +  @param [in]  CacheIdList      Li= st of already existing cache IDs.
> +  @param [in]  CacheIdListSize  Size of CacheIdList. > +
> +  @retval TRUE        &n= bsp;         CacheId does not exist= in CacheIdList.
> +  @retval FALSE        &= nbsp;        CacheId already exists in C= acheIdList.
> +**/
> +STATIC
> +BOOLEAN
> +IsCacheIdUnique (
> +  IN CONST UINT32  CacheId,
> +  IN CONST UINT32  *CacheIdList,
> +  IN CONST UINT32  CacheIdListSize
> +  )
> +{
> +  UINT32  Index;
> +
> +  for (Index =3D 0; Index < CacheIdListSize; Index++) {
> +    if (CacheIdList[Index] =3D=3D CacheId) {
> +      return FALSE;
> +    }
> +  }
> +
> +  return TRUE;
> +}
> +
>   /**
>     Update the Cache Type Structure (Type 1) infor= mation.
>  
> @@ -738,10 +767,12 @@ AddProcHierarchyNodes (
>     @param [in]  Pptt    =              Po= inter to PPTT table structure.
>     @param [in]  NodesStartOffset  =    Offset from the start of PPTT table to the
>            = ;            &n= bsp;            = ;  start of Cache Type Structures.
> +  @param [in]  Revision      =        Revision of the PPTT table being reque= sted.
>  
>     @retval EFI_SUCCESS    &nb= sp;          Structures update= d successfully.
>     @retval EFI_INVALID_PARAMETER   = ;  A parameter is invalid.
>     @retval EFI_NOT_FOUND    &= nbsp;        A required object was not f= ound.
> +  @retval EFI_OUT_OF_RESOURCES      Out= of resources.
>   **/
>   STATIC
>   EFI_STATUS
> @@ -749,7 +780,8 @@ AddCacheTypeStructures (
>     IN  CONST ACPI_PPTT_GENERATOR  =             &nb= sp;    *CONST        = ;     Generator,
>     IN  CONST EDKII_CONFIGURATION_MANAGER_PRO= TOCOL  *CONST         &nb= sp;   CfgMgrProtocol,
>     IN  CONST EFI_ACPI_6_4_PROCESSOR_PROPERTI= ES_TOPOLOGY_TABLE_HEADER  *Pptt,
> -  IN  CONST UINT32       = ;            &n= bsp;            = ;            &n= bsp;      NodesStartOffset
> +  IN  CONST UINT32       = ;            &n= bsp;            = ;            &n= bsp;      NodesStartOffset,
> +  IN  CONST UINT32       = ;            &n= bsp;            = ;            &n= bsp;      Revision
>     )
>   {
>     EFI_STATUS      =             &nb= sp;      Status;
> @@ -758,6 +790,9 @@ AddCacheTypeStructures (
>     CM_ARM_CACHE_INFO     = ;             *= CacheInfoNode;
>     PPTT_NODE_INDEXER     = ;             *= CacheNodeIterator;
>     UINT32      &nbs= p;            &= nbsp;         NodeCount;
> +  BOOLEAN         &= nbsp;           &nbs= p;      CacheIdUnique;
> +  UINT32         &n= bsp;            = ;       TotalNodeCount;
[SAMI] I think we could do slight optimisation by doing the following below= :
            &nb= sp;  (a) change TotalNode Count to NodeIndex.
> +  UINT32         &n= bsp;            = ;       *FoundCacheIds;
>  
>     ASSERT (
>       (Generator !=3D NULL) && > @@ -770,6 +805,13 @@ AddCacheTypeStructures (
>  
>     CacheNodeIterator =3D Generator->CacheStruc= tIndexedList;
>     NodeCount      &= nbsp;  =3D Generator->CacheStructCount;
> +  TotalNodeCount    =3D NodeCount;
> +
> +  FoundCacheIds =3D AllocateZeroPool (TotalNodeCount * sizeof (*= FoundCacheIds));
[SAMI] (b) Change TotalNodeCount to NodeCount.
> +  if (FoundCacheIds =3D=3D NULL) {
> +    DEBUG ((DEBUG_ERROR, "ERROR: PPTT: Failed to = allocate resources.\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
>  
>     while (NodeCount-- !=3D 0) {
[SAMI] (c) Replace while loop to a for loop where NodeIndes =3D 0 to
NodeCount.
>       CacheInfoNode =3D (CM_ARM_CACHE_IN= FO *)CacheNodeIterator->Object;
> @@ -789,6 +831,7 @@ AddCacheTypeStructures (
>       CacheStruct->Flags.CacheTypeVal= id      =3D 1;
>       CacheStruct->Flags.WritePolicyV= alid    =3D 1;
>       CacheStruct->Flags.LineSizeVali= d       =3D 1;
> +    CacheStruct->Flags.CacheIdValid  &nbs= p;     =3D 1;
>       CacheStruct->Flags.Reserved&nbs= p;           =3D 0;
>  
>       // Populate the reference to the n= ext level of cache
> @@ -811,7 +854,7 @@ AddCacheTypeStructures (
>            = ; CacheInfoNode->Token,
>            = ; Status
>            = ; ));
> -        return Status;
> +        goto cleanup;
>         }
>  
>         // Update Cache Struct= ure with the offset for the next level of cache
> @@ -835,7 +878,7 @@ AddCacheTypeStructures (
>           CacheInfoN= ode->NumberOfSets,
>           Status
>           ));
> -      return Status;
> +      goto cleanup;
>       }
>  
>       if (CacheInfoNode->NumberOfSets= > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX) {
> @@ -862,7 +905,7 @@ AddCacheTypeStructures (
>           CacheInfoN= ode->Associativity,
>           Status
>           ));
> -      return Status;
> +      goto cleanup;
>       }
>  
>       // Validate the Associativity fiel= d based on the architecture specification
> @@ -881,7 +924,7 @@ AddCacheTypeStructures (
>           CacheInfoN= ode->Associativity,
>           Status
>           ));
> -      return Status;
> +      goto cleanup;
>       }
>  
>       if (CacheInfoNode->Associativit= y > PPTT_ARM_CACHE_ASSOCIATIVITY_MAX) {
> @@ -923,7 +966,7 @@ AddCacheTypeStructures (
>           CacheInfoN= ode->LineSize,
>           Status
>           ));
> -      return Status;
> +      goto cleanup;
>       }
>  
>       if ((CacheInfoNode->LineSize &a= mp; (CacheInfoNode->LineSize - 1)) !=3D 0) {
> @@ -935,18 +978,58 @@ AddCacheTypeStructures (
>           CacheInfoN= ode->LineSize,
>           Status
>           ));
> -      return Status;
> +      goto cleanup;
>       }
>  
>       CacheStruct->LineSize =3D Cache= InfoNode->LineSize;
>  
> +    if (Revision >=3D 3) {
> +      // Validate and populate cache id
> +      if (CacheInfoNode->CacheId =3D=3D 0= ) {
> +        Status =3D EFI_INVALID_PAR= AMETER;
> +        DEBUG ((
> +          DEBUG_ERROR, > +          "ERROR: P= PTT: The cache id cannot be zero. Status =3D %r\n",
> +          Status
> +          ));
> +        goto cleanup;
> +      }
> +
> +      CacheIdUnique =3D IsCacheIdUnique ( > +           &nb= sp;            Cache= InfoNode->CacheId,
> +           &nb= sp;            Found= CacheIds,
> +           &nb= sp;            Total= NodeCount
[SAMI] (d) Replace TotalNodeCount with NodeIndex in call above.
            &nb= sp;  By doing (a) to (d) and (e) below we can reduce the
number of iterations in IsCacheIdUnique ().
            &nb= sp;  If you agree I will make the adjustments before pushing
this patch series.
[/SAMI]
> +           &nb= sp;            ); > +      if (!CacheIdUnique) {
> +        Status =3D EFI_INVALID_PAR= AMETER;
> +        DEBUG ((
> +          DEBUG_ERROR, > +          "ERROR: P= PTT: The cache id is not unique. " \
> +          "CacheId = =3D %d. Status =3D %r\n",
> +          CacheInfoNode-= >CacheId,
> +          Status
> +          ));
> +        goto cleanup;
> +      }
> +
> +      // Store the cache id so we can check = future cache ids for uniqueness
> +      FoundCacheIds[NodeCount] =3D CacheInfo= Node->CacheId;
[SAMI] (e) Replace NodeCount with NodeIndex above.
> +
> +      CacheStruct->CacheId =3D CacheInfoN= ode->CacheId;
> +    }
> +
>       // Next Cache Type Structure
>       CacheStruct =3D (EFI_ACPI_6_4_PPTT= _STRUCTURE_CACHE *)((UINT8 *)CacheStruct +
>            = ;            &n= bsp;            = ;            &n= bsp;         CacheStruct->Length= );
>       CacheNodeIterator++;
>     } // Cache Type Structure
>  
> -  return EFI_SUCCESS;
> +  Status =3D EFI_SUCCESS;
> +
> +cleanup:
> +  FreePool (FoundCacheIds);
> +
> +  return Status;
>   }
>  
>   /**
> @@ -1205,7 +1288,8 @@ BuildPpttTable (
>            = ;      Generator,
>            = ;      CfgMgrProtocol,
>            = ;      Pptt,
> -           &nb= sp;   CacheStructOffset
> +           &nb= sp;   CacheStructOffset,
> +           &nb= sp;   AcpiTableInfo->AcpiTableRevision
>            = ;      );
>       if (EFI_ERROR (Status)) {
>         DEBUG ((

--_000_VE1PR08MB5758FCDBBD3526044C597EFEEF709VE1PR08MB5758eurp_--