From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (NAM10-BN7-obe.outbound.protection.outlook.com [40.107.92.73]) by mx.groups.io with SMTP id smtpd.web08.450.1663038013630640212 for ; Mon, 12 Sep 2022 20:00:14 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amd.com header.s=selector1 header.b=pULtGvNt; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.92.73, mailfrom: abner.chang@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FdmuUnGzQ9b/1b/70YwcUrV1y96YXWai1LWKc71mPbDnIbP9HvqRWH0HfDAmYoPta/1iWB4zqYh0uxT+26gxHdFDYpYcVathGe+HJtjHjSLUz9JqChBcDrogctChlCFfC5PH5A7Y7TGQ27dk5iwvUyEVEslrHhS2JALm5Szi/m65DNSvqCYRNwViVroq2XCOrkUaPR7TO59WvjGepsmFA8cbDkiT+8aR4M/mPjtkKW3yVuvVE5BKfxZciv1NqTE9eeAA1Gb6TlQ1iVZX5YZNNcCvJfuEc4yQSgR16mdoRnJmn4o6Ci1gKW0/WMmQY+YH9kx4ngVJjPAD0QleTK/TDw== 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=aLmJcay0CFxJznCKWQPkYZn5Dudnr2kL0joVOQiPU2M=; b=VGvDWVfLRN2wdeQlj+vmuBEgWDrmeEGf/iGokmFJdrTeC9FtWKOlMiCU9sPGlmIfJcEU+0vtX8MPVEZunP5CGh5PxvhPNyHFHGC0lkrtwU31h56hS6JuYnAeeZIPorPaVRRcvNq/O2NQnOzHEFoovbsx9KFBxP3eZv6gSpJ2wD3aF4Y2cAOYsKuMQypQHXmp0zdO8Yvw7TZ4ZAI5V6wC41DSC805LOATMYKobuNJwtdZJNbxEpxJXal5qkQG4Scs+na+vKJ6OeuKI97FwudlxzzTbQkoXfXleuW1nicwNRZvEkJvyutqmYZ4YVgGNq7hQgSKDTVuDEdMIerVO4b4pg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=aLmJcay0CFxJznCKWQPkYZn5Dudnr2kL0joVOQiPU2M=; b=pULtGvNtvsGD9HaVYn3SypIgU08Ll6iP+VXZnkLzkP0INrNZZ+6NSwZ6dXeYlK35+LrQ8kxrqwJlymeWgPTeD3KtEbUw0Q+/7RnEOWTcPyoJ76j8Jo1TNNvfx16VfAVHjrACHGXgTM+2qReyCxbEa55qxbEKLuPkWprk+6vl9n0= Received: from MN2PR12MB3966.namprd12.prod.outlook.com (2603:10b6:208:165::18) by BL1PR12MB5826.namprd12.prod.outlook.com (2603:10b6:208:395::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5612.22; Tue, 13 Sep 2022 03:00:10 +0000 Received: from MN2PR12MB3966.namprd12.prod.outlook.com ([fe80::9c44:17db:7f29:1fa8]) by MN2PR12MB3966.namprd12.prod.outlook.com ([fe80::9c44:17db:7f29:1fa8%7]) with mapi id 15.20.5612.022; Tue, 13 Sep 2022 03:00:10 +0000 From: "Chang, Abner" To: "devel@edk2.groups.io" , "sami.mujawar@arm.com" , Girish Mahadevan , Alexei Fedorov CC: Samer El-Haj-Mahmoud , Jeff Brasen , Ashish Singhal , Akanksha Jain , Matteo Carlini , Hemendra Dassanayake , Nick Ramirez , William Watson , "nd@arm.com" Subject: Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator Thread-Topic: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator Thread-Index: AQHYuXLO+LYYNEG6pk+V4kDyIbCZpa3b/QWAgADGzrA= Date: Tue, 13 Sep 2022 03:00:10 +0000 Message-ID: References: <90bcdad9b53f1ca184a857da720aac1ab89882f7.1661534045.git.gmahadevan@nvidia.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_Enabled=true; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_SetDate=2022-09-13T03:00:08Z; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_Method=Standard; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_Name=General; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_SiteId=3dd8961f-e488-4e60-8e11-a82d994e183d; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_ActionId=56746ffd-80cf-4ea5-b7ba-7da276f28373; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_ContentBits=1 authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN2PR12MB3966:EE_|BL1PR12MB5826:EE_ x-ms-office365-filtering-correlation-id: cf7156e7-aca1-45dd-5e52-08da9534126d x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: dUtx47zUkClL9n7VNgoJP5OTwxqWQjl8rTmBxe/cs3x02tOkOjoviow9cjyMKYXCtuwQCSzqeWADCEmmMKxNyTKdiYeP1FpGhXUGtzBDsCSi40FDatjjnjua8rlgZeE5aTYM6McuZZ7QYJPHj6SQ00caJ85hPmHmdN3FelrxevuhQSOYBSyaZKCJAQhUEEgDqKYQqw9kPzd3I6CmtU4jSzZ6hCJ5wlddrSRpyUvEpgi56SDj/+K1zk6gmNDa35ps4wMufGGZarEfsg+apJpzmimhPJFW9CJhkN3M1geR4UjR08F9ZUM7uaCBXfwcElBcC+HaMnxUPkDgeXWVJWonqrlWn8DuyUvYdFuLfvwINqEguEnNuTl88LaWdghZbyN8JPd51AMSJHLG4QKIe6ak1wHtKa/coIGhVKczWMmdlJ+BmAll7Wg/34goHuyhqlhrcFyK2IB6g+PV4erRD0j4nVlrYIPaLrcDiErfYxi9L3dyNIqwyE6YtPf11mnATvkSUySSas1E/TxkR+PL4y3rD42hPxb08X/EaV/8gQNKryz+CcPRu+qWqlgdvGRKkznxifLcaLDdeXk4OvlE/ul/LGkzvNBB7wC/O/5hW1zqWUc52Jot/KxCWpCb3rHJtRmdLqP3OFLzqsqU37yHyqnMg1oGIHm3Ai7EOsBMg168S6NlPxa6m8E+CGsfnN5aL6marXLetEWNZP6YP/xXLFD/DY7Svled/pg3ozpsWZtacsaCuDfw3zM/29q5V0LwJQ98Zwhh4SROUYlqXiXoUJe150obj4FtDSKUqRZDVeBJNEepbIMhCsRtYvaGePtbI5DYo4laQCvCPRuSzMUC/sPG01SCFOWllY/0rt/kqY6Latk= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MN2PR12MB3966.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230022)(4636009)(39860400002)(136003)(376002)(396003)(366004)(346002)(451199015)(41300700001)(55016003)(7696005)(316002)(54906003)(9686003)(7416002)(6506007)(966005)(478600001)(86362001)(26005)(110136005)(33656002)(66476007)(66946007)(53546011)(8936002)(52536014)(2906002)(83380400001)(186003)(4326008)(64756008)(76116006)(66556008)(45080400002)(38100700002)(66446008)(8676002)(30864003)(71200400001)(5660300002)(122000001)(19627235002)(38070700005)(21314003)(579004)(559001);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?CCEoaH58+qlgVAroOncCzPaUm4NAV5f2SYDzwMSseE5y0JncFptFoLDVnO?= =?iso-8859-1?Q?tj0xQZktC6VmjQrQTEFjHk2dUGfexl9bqZ1s3kvQcCFhmK8PqN2dspOr8D?= =?iso-8859-1?Q?4Ylml5442VJ/atYJhHjbRYfG3YRJUll39kuX17rB0U9LHNci5Jhv4+K1aO?= =?iso-8859-1?Q?RHh29HHnAKIfAt+9nhSDfhu/lZkGnZXwx+Zw8opEN8JMU2imm9LvwK++C+?= =?iso-8859-1?Q?gQBdcf0TasmzxRMzFykeV+F2qO6XkMzl3V+nbES1rr/2qL3XRfwV+qeWOM?= =?iso-8859-1?Q?8yJjNB24LfpAoOVRpG2yAlIKPXMiaTEF3GLbH1OtxUr44ZEG/cuH63NQJm?= =?iso-8859-1?Q?nhSJ+ToGoWwGaTYlgzmvky1wrVF/m34hgJiCNafWfwwWAGXWzT5ZWpzMeW?= =?iso-8859-1?Q?pDBFrR6Ztq3hTwZCjgTCNfugaI/hmxaUR0o+hWRtLEznfeuKe7404UVKz3?= =?iso-8859-1?Q?w5LsDzME8Li+UBcxBXDroBOifdpErnbaaVXNKQ0SIaNFAZswQMnyFrSx+a?= =?iso-8859-1?Q?wlLD6lK9yfJsI7CyIIHPIdJHCaThtAUnxJwLOjokcIbTbi/SzzvfqMkrzQ?= =?iso-8859-1?Q?B/P8guKwcxEBZp+AntpVnj1R1qNCV+U7GFuw/s/iEznKiOsRlgPqX2JOas?= =?iso-8859-1?Q?eLA9k+jl7P/XHFtdgqY6YYCvcSExQT8viHGkgVG5sz8BGEdDM6lzxXHX9x?= =?iso-8859-1?Q?/gZC9HfXx28qoEvQlGsjO51YLuo7aUpendtJetSGaM5OJ5NT/7kj0spC4e?= =?iso-8859-1?Q?4/jK3K7cedtP1QaoI7Vw9x4KF7jLv4INc8bKj9oDeIyzzP+Q/z+0xd3HcJ?= =?iso-8859-1?Q?tzOPab2JrH7aKNLk0JFq6gisHbOtk7AaRnukL9x751Raisr9j/Bv9V5n4y?= =?iso-8859-1?Q?gadUuCbXJZU2XvpkmiC/8dpZJzsSAaGZGujfV4yEJh7i0xuD/wltMp031M?= =?iso-8859-1?Q?IeljYQ4Oo1hUo/Wxd06O5HU+Gsk8o0CN4rtcfnNIfEuCz7x0KmIKLt5T1U?= =?iso-8859-1?Q?e0frdWOlURfB9Knu8S5Oiwu/fswOJgvc3CB0i9VFZLwZsiJQ89kEhWVCgs?= =?iso-8859-1?Q?3V3KcixoHb20zkulMqV3+nclviGIkWUDgeGl+4CCReNfWHmN8MOOmexbG3?= =?iso-8859-1?Q?1quXXkaXPaPQQ6KqZ+N7hb0s/7xzMt+tK0wJPQPFuPg9Lzkemibfra8YVE?= =?iso-8859-1?Q?VAO39+fo8Js2GtqpLvpExHwNce0b/l+7885x7hcSPQJlPQe2/siT+py7nY?= =?iso-8859-1?Q?FVrZB4fVwDzWzM8zMaF+1h4QHHVnpcO0TF7A+PQT0avNQLGL4YRcbDiAgH?= =?iso-8859-1?Q?U21KJzcKm+exPPJRO3/2KHDOsRaO9pKLTorx/MmrAxNMoUvUoIZE5UPgi7?= =?iso-8859-1?Q?ArnbkGKNGkyLThvbEUn3wjgpwHU2ivvFatDzrzxvhOKCovFe6fmqH9L+mZ?= =?iso-8859-1?Q?ckWNjacZq8bnTHRWDfPJsWntCS1RdxJUZzoMMp73hvHri+sEQ36u+3KWMc?= =?iso-8859-1?Q?45moz7mkdax9i9rezoRk40O31jfWCG6g+1784GP+JTej6V/63tcx7gz6UO?= =?iso-8859-1?Q?hwGNkxLTTnGZEcTcj0HoRFjucYGn21scsUH95gLne6iuKixeclUdaSwZwg?= =?iso-8859-1?Q?BEibamZaseo1o=3D?= MIME-Version: 1.0 X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN2PR12MB3966.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: cf7156e7-aca1-45dd-5e52-08da9534126d X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Sep 2022 03:00:10.5717 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: r7okqucNk5XIXQMm0UBwyRbwXQ5iw8Mo6ap0FESl4BIJhppnX9YOvZYf6vt4GzgCyy0cs3ilb4SZq0IH53iXlA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5826 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable [AMD Official Use Only - General] One question in below with tag [Abner], > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Sami > Mujawar via groups.io > Sent: Monday, September 12, 2022 10:57 PM > To: Girish Mahadevan ; devel@edk2.groups.io; > Alexei Fedorov > Cc: Samer El-Haj-Mahmoud ; Jeff > Brasen ; Ashish Singhal ; > Akanksha Jain ; Matteo Carlini > ; Hemendra Dassanayake > ; Nick Ramirez ; > William Watson ; Akanksha Jain > ; nd@arm.com > Subject: Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios > Type17 Table generator >=20 > [CAUTION: External Email] >=20 > Hi Girish, >=20 > Thank you for this patch and for the effort for bringing forward dynamic > SMBIOS generation. >=20 > Please find my feedback inline marked [SAMI]. >=20 > Regards, >=20 > Sami Mujawar >=20 > On 26/08/2022 06:37 pm, Girish Mahadevan wrote: > > Add a new CM object to describe memory devices and setup a new > > Generator Library for SMBIOS Type17 table. > > > > Signed-off-by: Girish Mahadevan > > --- > > .../Include/ArmNameSpaceObjects.h | 59 +++ > > .../SmbiosType17Lib/SmbiosType17Generator.c | 338 > ++++++++++++++++++ > > .../SmbiosType17Lib/SmbiosType17LibArm.inf | 32 ++ > > 3 files changed, 429 insertions(+) > > create mode 100644 > DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Ge > nerator.c > > create mode 100644 > > > DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Lib > Arm > > .inf > > > > diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > > b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > > index 102e0f96be..199a19e997 100644 > > --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > > +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > > @@ -63,6 +63,7 @@ typedef enum ArmObjectID { > > EArmObjPciInterruptMapInfo, ///< 39 - Pci Interrupt Map In= fo > > EArmObjRmr, ///< 40 - Reserved Memory Rang= e Node > > EArmObjMemoryRangeDescriptor, ///< 41 - Memory Range > Descriptor > > + EArmObjMemoryDeviceInfo, ///< 42 - Memory Device Informa= tion > > EArmObjMax > > } EARM_OBJECT_ID; > > > > @@ -1070,6 +1071,64 @@ typedef struct CmArmRmrDescriptor { > > UINT64 Length; > > } CM_ARM_MEMORY_RANGE_DESCRIPTOR; > > > > +/** A structure that describes the physical memory device. > > + > > + The physical memory devices on the system are described by this obje= ct. > > + > > + SMBIOS Specification v3.5.0 Type17 > > + > > + ID: EArmObjMemoryDeviceInfo, > > +*/ > > +typedef struct CmArmMemoryDeviceInfo { > [SAMI] I think we may need a Token pointing to the Type 16 object so that > the Physical Memory Array Handle can be setup, see my comment below > about the HandleManager. > > + /** Size of the device. > > + Size of the device in bytes. > > + */ > > + UINT64 Size; > > + > > + /** Device Set */ > > + UINT8 DeviceSet; > > + > > + /** Speed of the device > > + Speed of the device in MegaTransfers/second. > > + */ > > + UINT32 Speed; > > + > > + /** Serial Number of device */ > > + CHAR8 *SerialNum; > > + > > + /** AssetTag identifying the device */ > > + CHAR8 *AssetTag; > > + > > + /** Device Locator String for the device. > > + String that describes the slot or position of the device on the boa= rd. > > + */ > > + CHAR8 *DeviceLocator; > > + > > + /** Bank locator string for the device. > > + String that describes the bank where the device is located. > > + */ > > + CHAR8 *BankLocator; > > + > > + /** Firmware version of the memory device */ > > + CHAR8 *FirmwareVersion; > > + > > + /** Manufacturer Id. > > + 2 byte Manufacturer Id as per JEDEC Standard JEP106AV */ > > + UINT16 ModuleManufacturerId; > > + > > + /** Manufacturer Product Id > > + 2 byte Manufacturer Id as designated by Manufacturer. > > + */ > > + UINT16 ModuleProductId; > > + > > + /** Device Attributes */ > > + UINT8 Attributes; > > + > > + /** Device Configured Voltage in millivolts */ > > + UINT16 ConfiguredVoltage; > [SAMI] This field does not appear to be used in the generator. If the > intention is to use this in the future, then it may be better to add this= at a > later stage. > > +} CM_ARM_MEMORY_DEVICE_INFO; > > + > > #pragma pack() > > > > #endif // ARM_NAMESPACE_OBJECTS_H_ > > diff --git > > > a/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17G > ene > > rator.c > > > b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17 > Gene > > rator.c > > new file mode 100644 > > index 0000000000..5683ca570f > > --- /dev/null > > +++ > b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17 > > +++ Generator.c > > @@ -0,0 +1,338 @@ > > +/** @file > > + SMBIOS Type17 Table Generator. > > + > > + Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > + Copyright (c) 2020 - 2021, Arm Limited. All rights reserved.
> > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent **/ > > + > > +#include > > +#include > > +#include > > +#include > > +#include #include > > + > [SAMI] I could not find SmbiosType17FixupLib.h in this patch series. Can = you > check, please? > > + > > +// Module specific include files. > > +#include #include > > + #include > > + > > +#include > [SAMI] I think Protocol/Smbios.h may not be required in this file. Can yo= u > check, please? > > +#include > > + > > +/** This macro expands to a function that retrieves the Memory Device > > + information from the Configuration Manager. > > +*/ > > +GET_OBJECT_LIST ( > > + EObjNameSpaceArm, > > + EArmObjMemoryDeviceInfo, > > + CM_ARM_MEMORY_DEVICE_INFO > > + ) > > + > > +// Default Values for Memory Device > > +STATIC SMBIOS_TABLE_TYPE17 MemDeviceInfoTemplate =3D { > > + { // Hdr > > + EFI_SMBIOS_TYPE_MEMORY_DEVICE, // Type > > + sizeof (SMBIOS_TABLE_TYPE17), // Length > > + 0 // Handle > > + }, > > + 0, // MemoryArrayHandle >=20 > [SAMI] Do you have any thoughts on how the MemoryArrayHandle can be > setup? >=20 > The same applies for the following MemoryErrorInformationHandle field. >=20 > I think we need some sort of a HandleManager in DynamicTablesFramework > that can keep track of the mappings between SMBIOS Objects and Table > Handles. >=20 > e.g. Smbios - HandleManager >=20 > +-------------------------------+-------------------------------+ >=20 > |=A0=A0=A0 Object Token | Table Handle = | >=20 > +-------------------------------+-------------------------------+ >=20 > | Type16Obj_token | Type 16 Table handle | >=20 > +-------------------------------+-------------------------------+ >=20 > ... >=20 > - The Type17Object i.e. CM_ARM_MEMORY_DEVICE_INFO can then hold a > token for the Type16Object. >=20 > - If Type 17 table is to be installed, DynamicTablemanager shall search= the > SMBIOS table list to see if a Type16 table is requested to be installed. >=20 > - If a Type16 table is present in the list of SMBIOS table to install, th= e Type16 > table shall be installed first and an entry is made in the Smbios > HandleManager to create a mapping of Type16Obj_token <=3D=3D> Type16 > Table Handle. >=20 > - The Type17 table can now be built and if a the Type16Object token is > provided in CM_ARM_MEMORY_DEVICE_INFO, the Smbios HandleManager > shall be searched (using Type16Obj_token) to retrieve the Type16 Table > handle and populate the Type 17 Physical Memory Array Handle field. >=20 > I think we may have to experiment a bit before we arrive at the correct > design. However, please do let me know your thoughts on the above. >=20 > [SAMI] > > + 0, // MemoryErrorInformationHand= le > > + 0xFFFF, // TotalWidth > > + 0xFFFF, // DataWIdth >=20 > [SAMI] I need to find out how these fields should be populated, but the > Annex A, SMBIOS specification version 3.6.0 says the following: >=20 > 4.8.4 Total Width is not 0FFFFh (Unknown) if the memory device is install= ed. > (Size is not 0.) >=20 > 4.8.5 Data Width is not 0FFFFh (Unknown). >=20 > Can you explain how this field is used, please? >=20 > [/SAMI] >=20 > > + 0x7FFF, // Size (always use Extended = Size) >=20 > [SAMI] I think this field should be set based on the Size. >=20 > The spec says "If the size is 32 GB-1 MB or greater, the field value is 7= FFFh > and the actual size is stored in the Extended Size field." >=20 > I think it would be good to have a static function that encodes the size= in > wither the Size field or the Extended Size field. >=20 > e.g. UpdateSmbiosTable17Size (SMBIOS_TABLE_TYPE17 *SmbiosRecord, > UINTN > Size) { >=20 > if (Size > 32GB-1MB) { >=20 > SmbiosRecord->Size =3D EncodeSize (xxx); >=20 > } else { >=20 > SmbiosRecord->ExtendedSize =3D EncodeExtendedSize (xxx); >=20 > } >=20 > } >=20 > [/SAMI] >=20 > > + MemoryTypeUnknown, // FormFactor > > + 0xFF, // DeviceSet > > + 1, // Device Locator > > + 2, // Bank Locator > > + MemoryTypeSdram, // MemoryType > > + { // TypeDetail > > + 0 > > + }, > > + 0xFFFF, // Speed (Use Extended Speed) > [SAMI] Maybe we need a helper function (similar to > UpdateSmbiosTable17Size()) for this field as well. > > + 0, // Manufacturer > > + // (Unused Use ModuleManufact= uerId) > > + 3, // SerialNumber > > + 4, // AssetTag > > + 0, // PartNumber > > + // (Unused Use ModuleProductI= d) > > + 0, // Attributes > > + 0, // ExtendedSize > > + 2000, // ConfiguredMemoryClockSpeed > [SAMI] Should this be provided through CM_ARM_MEMORY_DEVICE_INFO? > > + 0, // MinimumVoltage > > + 0, // MaximumVoltage > > + 0, // ConfiguredVoltage > > + MemoryTechnologyDram, // MemoryTechnology > [SAMI] Should this be provided through CM_ARM_MEMORY_DEVICE_INFO? > > + { // MemoryOperatingModeCapabil= ity > > + .Uint16 =3D 0x000 > > + }, > [SAMI] I think the above initialisation may not be portable. > > + 5, // FirmwareVersion > > + 0, // ModuleManufacturerId > > + 0, // ModuleProductId > > + 0, // MemorySubsystemContollerMan= ufacturerId > > + 0, // MemorySybsystemControllerPr= oductId > > + 0, // NonVolatileSize > > + 0, // VolatileSize > > + 0, // CacheSize > > + 0, // LogicalSize > > + 0, // ExtendedSpeed > > + 0, // ExtendedConfiguredMemorySpe= ed > > +}; > > + > > +STATIC CHAR8 UnknownStr[] =3D "Unknown\0"; > [SAMI] Would it be possible to add the CONST qualifier, please? > > + > > +STATIC > > +EFI_STATUS > > +EFIAPI > > +FreeSmbiosType17TableEx ( > > + IN CONST SMBIOS_TABLE_GENERATOR *CONST Thi= s, > > + IN CONST CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST > SmbiosTableInfo, > > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST > CfgMgrProtocol, > > + IN OUT SMBIOS_STRUCTURE ***CONST Tab= le, > > + IN CONST UINTN Tab= leCount > > + ) > > +{ > > + return EFI_SUCCESS; > > +} > > + > > +/** Construct SMBIOS Type17 Table describing memory devices. > > + > > + If this function allocates any resources then they must be freed > > + in the FreeXXXXTableResources function. > > + > > + @param [in] This Pointer to the SMBIOS table generator. > > + @param [in] SmbiosTableInfo Pointer to the SMBIOS table information= . > > + @param [in] CfgMgrProtocol Pointer to the Configuration Manager > > + Protocol interface. > > + @param [out] Table Pointer to the SMBIOS table. > > + > > + @retval EFI_SUCCESS Table generated successfully. > > + @retval EFI_BAD_BUFFER_SIZE The size returned by the Configuratio= n > > + Manager is less than the Object size = for > > + the requested object. > > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > > + @retval EFI_NOT_FOUND Could not find information. > > + @retval EFI_OUT_OF_RESOURCES Could not allocate memory. > > + @retval EFI_UNSUPPORTED Unsupported configuration. > > +**/ > > +STATIC > > +EFI_STATUS > > +EFIAPI > > +BuildSmbiosType17TableEx ( > > + IN CONST SMBIOS_TABLE_GENERATOR *This, > > + IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST > SmbiosTableInfo, > > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST > CfgMgrProtocol, > > + OUT SMBIOS_STRUCTURE ***Table, > > + OUT UINTN *CONST TableCount > > + ) > > +{ > > + EFI_STATUS Status; > > + UINT32 NumMemDevices; > > + SMBIOS_STRUCTURE **TableList; > > + CM_ARM_MEMORY_DEVICE_INFO *MemoryDevicesInfo; > > + UINTN Index; > > + UINTN SerialNumLen; > > + CHAR8 *SerialNum; > > + UINTN AssetTagLen; > > + CHAR8 *AssetTag; > > + UINTN DeviceLocatorLen; > > + CHAR8 *DeviceLocator; > > + UINTN BankLocatorLen; > > + CHAR8 *BankLocator; > > + UINTN FirmwareVersionLen; > > + CHAR8 *FirmwareVersion; > > + CHAR8 *OptionalStrings; > > + SMBIOS_TABLE_TYPE17 *SmbiosRecord; > > + > > + ASSERT (This !=3D NULL); > > + ASSERT (SmbiosTableInfo !=3D NULL); > > + ASSERT (CfgMgrProtocol !=3D NULL); > > + ASSERT (Table !=3D NULL); > > + ASSERT (TableCount !=3D NULL); > > + ASSERT (SmbiosTableInfo->TableGeneratorId =3D=3D This->GeneratorID); > > + > > + DEBUG ((DEBUG_ERROR, "%a : Start \n", __FUNCTION__)); *Table =3D > > + NULL; Status =3D GetEArmObjMemoryDeviceInfo ( > > + CfgMgrProtocol, > > + CM_NULL_TOKEN, > > + &MemoryDevicesInfo, > > + &NumMemDevices > > + ); [Abner]=20 SMBIOS type 17 record is generic to all platform architectures, however her= e we have the dependency with ARM namespace object. So my question is what = should we do if non-ARM platforms would like to leverage this library?=20 Thanks Abner > > + if (EFI_ERROR (Status)) { > > + DEBUG (( > > + DEBUG_ERROR, > > + "Failed to get Memory Devices CM Object %r\n", > > + Status > > + )); > > + return Status; > > + } > > + > > + TableList =3D (SMBIOS_STRUCTURE **)AllocateZeroPool (sizeof > > + (SMBIOS_STRUCTURE *) * NumMemDevices); > [SAMI] The memory allocated here should be freed in > FreeSmbiosType17TableEx. > > + if (TableList =3D=3D NULL) { > > + DEBUG ((DEBUG_ERROR, "Failed to alloc memory for %u devices > table\n")); > > + Status =3D EFI_OUT_OF_RESOURCES; > > + goto exit; > > + } > > + > > + for (Index =3D 0; Index < NumMemDevices; Index++) { > > + if (MemoryDevicesInfo[Index].SerialNum !=3D NULL) { > > + SerialNumLen =3D AsciiStrLen (MemoryDevicesInfo[Index].SerialNum= ); > > + SerialNum =3D MemoryDevicesInfo[Index].SerialNum; > > + } else { > > + SerialNumLen =3D AsciiStrLen (UnknownStr); > > + SerialNum =3D UnknownStr; >=20 > [SAMI] If the serial number is not provided, then should the string field= be > set to 0? >=20 > See Section 6.1.3, SMBIOS Spec Version 3.6.0 which states >=20 > "If a string field references no string, a null (0) is placed in that str= ing field." >=20 > [/SAMI] >=20 > > + } > > + > > + if (MemoryDevicesInfo[Index].AssetTag !=3D NULL) { > > + AssetTagLen =3D AsciiStrLen (MemoryDevicesInfo[Index].AssetTag); > > + AssetTag =3D MemoryDevicesInfo[Index].AssetTag; > > + } else { > > + AssetTagLen =3D AsciiStrLen (UnknownStr); > > + AssetTag =3D UnknownStr; > > + } > > + > > + if (MemoryDevicesInfo[Index].DeviceLocator !=3D NULL) { > > + DeviceLocatorLen =3D AsciiStrLen > (MemoryDevicesInfo[Index].DeviceLocator); > > + DeviceLocator =3D MemoryDevicesInfo[Index].DeviceLocator; > > + } else { > > + DeviceLocatorLen =3D AsciiStrLen (UnknownStr); > > + DeviceLocator =3D UnknownStr; > > + } > > + > > + if (MemoryDevicesInfo[Index].BankLocator !=3D NULL) { > > + BankLocatorLen =3D AsciiStrLen > (MemoryDevicesInfo[Index].BankLocator); > > + BankLocator =3D MemoryDevicesInfo[Index].BankLocator; > > + } else { > > + BankLocatorLen =3D AsciiStrLen (UnknownStr); > > + BankLocator =3D UnknownStr; > > + } > > + > > + if (MemoryDevicesInfo[Index].FirmwareVersion !=3D NULL) { > > + FirmwareVersionLen =3D AsciiStrLen > (MemoryDevicesInfo[Index].FirmwareVersion); > > + FirmwareVersion =3D MemoryDevicesInfo[Index].FirmwareVersion; > > + } else { > > + FirmwareVersionLen =3D AsciiStrLen (UnknownStr); > > + FirmwareVersion =3D UnknownStr; > > + } > > + > > + SmbiosRecord =3D (SMBIOS_TABLE_TYPE17 *)AllocateZeroPool ( > > + sizeof (SMBIOS_TABLE_TYPE1= 7) + > > + SerialNumLen + 1 + > > + AssetTagLen + 1 + DeviceLo= catorLen + 1 + > > + BankLocatorLen + 1 + Firmw= areVersionLen + 1 + 1 > > + ); > [SAMI] The memory allocated here needs to be freed in > FreeSmbiosType17TableEx as SmbiosProtocol->Add () makes a copy of the > SmbiosTable, see > https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fgith > ub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FMdeModulePkg%2FU > niversal%2FSmbiosDxe%2FSmbiosDxe.c%23L476&data=3D05%7C01%7Cab > ner.chang%40amd.com%7C0f7ab9327b14444577e608da94cf2b73%7C3dd8961 > fe4884e608e11a82d994e183d%7C0%7C0%7C637985914789485474%7CUnkno > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1 > haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3DNSB9I00z4gS0N5 > 97Bs1IMWIJoPPgzdnHsVrgpcPOuHw%3D&reserved=3D0 > and > https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fgith > ub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FMdeModulePkg%2FU > niversal%2FSmbiosDxe%2FSmbiosDxe.c%23L516&data=3D05%7C01%7Cab > ner.chang%40amd.com%7C0f7ab9327b14444577e608da94cf2b73%7C3dd8961 > fe4884e608e11a82d994e183d%7C0%7C0%7C637985914789485474%7CUnkno > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1 > haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3DHgtuAt2lf%2F9L1 > jNAPpShJCrDKSb7V0t6kE8UuTUHS7c%3D&reserved=3D0. >=20 > > + if (SmbiosRecord =3D=3D NULL) { > > + Status =3D EFI_OUT_OF_RESOURCES; > > + goto exit; > > + } > > + > > + CopyMem (SmbiosRecord, &MemDeviceInfoTemplate, sizeof > (SMBIOS_TABLE_TYPE17)); > > + SmbiosRecord->ExtendedSize =3D MemoryDevicesInfo[Index].Si= ze; >=20 > [SAMI] CM_ARM_MEMORY_DEVICE_INFO.Size is documented as size in > bytes, while looking at the SMBIOS specification, section 7.18.5 for the > Extended Size Bits 30:0 represent the size of the memory device in > megabytes. >=20 > I think it will be useful to create UpdateSmbiosTable17Size() which does = the > appropriate encoding and updation of the SMBIOS table fieds. >=20 > [/SAMI] >=20 > > + SmbiosRecord->DeviceSet =3D > MemoryDevicesInfo[Index].DeviceSet; > > + SmbiosRecord->ModuleManufacturerID =3D > > + MemoryDevicesInfo[Index].ModuleManufacturerId; > > + SmbiosRecord->ModuleProductID =3D > > + MemoryDevicesInfo[Index].ModuleProductId; > > + SmbiosRecord->Attributes =3D > > + MemoryDevicesInfo[Index].Attributes; > > + SmbiosRecord->ExtendedSpeed =3D MemoryDevicesInfo[Index].Speed; > > + OptionalStrings =3D (CHAR8 *)(SmbiosRecord + 1); > > + AsciiSPrint (OptionalStrings, DeviceLocatorLen + 1, > > + DeviceLocator); > [SAMI] I think we can simplify the publishing of the SMBIOS strings using > SmbiosStringTableLib. Please see the patch at > https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk > 2.groups.io%2Fg%2Fdevel%2Fmessage%2F93651&data=3D05%7C01%7Cab > ner.chang%40amd.com%7C0f7ab9327b14444577e608da94cf2b73%7C3dd8961 > fe4884e608e11a82d994e183d%7C0%7C0%7C637985914789485474%7CUnkno > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1 > haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3DB8WFDH%2FYQy > vWiGZaXSM5GFKMKcLSeZYIsCYSJaGBiOM%3D&reserved=3D0 > > + OptionalStrings =3D OptionalStrings + DeviceLocatorLen + 1; > > + AsciiSPrint (OptionalStrings, BankLocatorLen + 1, BankLocator); > > + OptionalStrings =3D OptionalStrings + BankLocatorLen + 1; > > + AsciiSPrint (OptionalStrings, SerialNumLen + 1, SerialNum); > > + OptionalStrings =3D OptionalStrings + SerialNumLen + 1; > > + AsciiSPrint (OptionalStrings, AssetTagLen + 1, AssetTag); > > + OptionalStrings =3D OptionalStrings + AssetTagLen + 1; > > + AsciiSPrint (OptionalStrings, FirmwareVersionLen + 1, FirmwareVers= ion); > > + OptionalStrings =3D OptionalStrings + FirmwareVersionLen + 1; > > + TableList[Index] =3D (SMBIOS_STRUCTURE *)SmbiosRecord; } > > + > > + *Table =3D TableList; > > + *TableCount =3D NumMemDevices; > > + > > +exit: > > + DEBUG ((DEBUG_ERROR, "%a : Done \n", __FUNCTION__)); > > + return Status; > > +} > > + > > +/** The interface for the SMBIOS Type17 Table Generator. > > +*/ > > +STATIC > > +CONST > > +SMBIOS_TABLE_GENERATOR SmbiosType17Generator =3D { > > + // Generator ID > > + CREATE_STD_SMBIOS_TABLE_GEN_ID (EStdSmbiosTableIdType17), > > + // Generator Description > > + L"SMBIOS.TYPE17.GENERATOR", > > + // SMBIOS Table Type > > + EFI_SMBIOS_TYPE_MEMORY_DEVICE, > > + NULL, > > + NULL, > > + // Build table function Extended. > > + BuildSmbiosType17TableEx, > > + // Free function Extended. > > + FreeSmbiosType17TableEx > > +}; > > + > > +/** Register the Generator with the SMBIOS Table Factory. > > + > > + @param [in] ImageHandle The handle to the image. > > + @param [in] SystemTable Pointer to the System Table. > > + > > + @retval EFI_SUCCESS The Generator is registered. > > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > > + @retval EFI_ALREADY_STARTED The Generator for the Table ID > > + is already registered. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +SmbiosType17LibConstructor ( > > + IN EFI_HANDLE ImageHandle, > > + IN EFI_SYSTEM_TABLE *SystemTable > > + ) > > +{ > > + EFI_STATUS Status; > > + > > + Status =3D RegisterSmbiosTableGenerator (&SmbiosType17Generator); > > + DEBUG (( > > + DEBUG_INFO, > > + "SMBIOS Type 17: Register Generator. Status =3D %r\n", > > + Status > > + )); > > + ASSERT_EFI_ERROR (Status); > > + > > + return Status; > > +} > > + > > +/** Deregister the Generator from the SMBIOS Table Factory. > > + > > + @param [in] ImageHandle The handle to the image. > > + @param [in] SystemTable Pointer to the System Table. > > + > > + @retval EFI_SUCCESS The Generator is deregistered. > > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > > + @retval EFI_NOT_FOUND The Generator is not registered. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +SmbiosType17LibDestructor ( > > + IN EFI_HANDLE ImageHandle, > > + IN EFI_SYSTEM_TABLE *SystemTable > > + ) > > +{ > > + EFI_STATUS Status; > > + > > + Status =3D DeregisterSmbiosTableGenerator (&SmbiosType17Generator); > > + DEBUG (( > > + DEBUG_INFO, > > + "SMBIOS Type17: Deregister Generator. Status =3D %r\n", > > + Status > > + )); > > + ASSERT_EFI_ERROR (Status); > > + return Status; > > +} > > diff --git > > > a/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Li > bA > > rm.inf > > > b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Li > bA > > rm.inf > > new file mode 100644 > > index 0000000000..78a80b75f0 > > --- /dev/null > > +++ > b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17 > > +++ LibArm.inf > > @@ -0,0 +1,32 @@ > > +## @file > > +# SMBIOS Type17 Table Generator > > +# > > +# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > +# Copyright (c) 2019 - 2021, Arm Limited. All rights reserved.
# > > +# SPDX-License-Identifier: BSD-2-Clause-Patent ## > > + > > +[Defines] > > + INF_VERSION =3D 0x0001001B > > + BASE_NAME =3D SmbiosType17LibArm > > + FILE_GUID =3D 1f063bac-f8f1-4e08-8ffd-9aae52c75497 > > + VERSION_STRING =3D 1.0 > > + MODULE_TYPE =3D DXE_DRIVER > > + LIBRARY_CLASS =3D NULL|DXE_DRIVER > > + CONSTRUCTOR =3D SmbiosType17LibConstructor > > + DESTRUCTOR =3D SmbiosType17LibDestructor > > + > > +[Sources] > > + SmbiosType17Generator.c > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > + EmbeddedPkg/EmbeddedPkg.dec > > + ArmPlatformPkg/ArmPlatformPkg.dec > > + DynamicTablesPkg/DynamicTablesPkg.dec > > + > > +[LibraryClasses] > > + BaseLib > > + DebugLib >=20 >=20 >=20 >=20