Re: [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator


Sami Mujawar
 

Hi Abner,

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

´╗┐On 13/09/2022, 04:00, "Chang, Abner" <Abner.Chang@...> wrote:

[AMD Official Use Only - General]

One question in below with tag [Abner],

-----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami
> Mujawar via groups.io
> Sent: Monday, September 12, 2022 10:57 PM
> To: Girish Mahadevan <gmahadevan@...>; devel@edk2.groups.io;
> Alexei Fedorov <Alexei.Fedorov@...>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; Jeff
> Brasen <jbrasen@...>; Ashish Singhal <ashishsingha@...>;
> Akanksha Jain <Akanksha.Jain2@...>; Matteo Carlini
> <Matteo.Carlini@...>; Hemendra Dassanayake
> <Hemendra.Dassanayake@...>; Nick Ramirez <nramirez@...>;
> William Watson <wwatson@...>; Akanksha Jain
> <Akanksha.Jain2@...>; nd@...
> Subject: Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios
> Type17 Table generator
...

> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +FreeSmbiosType17TableEx (
> > + IN CONST SMBIOS_TABLE_GENERATOR *CONST This,
> > + IN CONST CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST
> SmbiosTableInfo,
> > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
> CfgMgrProtocol,
> > + IN OUT SMBIOS_STRUCTURE ***CONST Table,
> > + IN CONST UINTN TableCount
> > + )
> > +{
> > + 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 Configuration
> > + 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 != NULL);
> > + ASSERT (SmbiosTableInfo != NULL);
> > + ASSERT (CfgMgrProtocol != NULL);
> > + ASSERT (Table != NULL);
> > + ASSERT (TableCount != NULL);
> > + ASSERT (SmbiosTableInfo->TableGeneratorId == This->GeneratorID);
> > +
> > + DEBUG ((DEBUG_ERROR, "%a : Start \n", __FUNCTION__)); *Table =
> > + NULL; Status = GetEArmObjMemoryDeviceInfo (
> > + CfgMgrProtocol,
> > + CM_NULL_TOKEN,
> > + &MemoryDevicesInfo,
> > + &NumMemDevices
> > + );
[Abner]
SMBIOS type 17 record is generic to all platform architectures, however here we have the dependency with ARM namespace object.
[SAMI] It would certainly be very good to have a common codebase across architectures. We welcome contribution from community members towards this effort.
So my question is what should we do if non-ARM platforms would like to leverage this library?
[SAMI] I think we could define the SMBIOS specific objects in a separate namespace ID e.g. 1010b - SMBIOS Objects , see https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ConfigurationManagerObject.h#L30-L34
We can then define the SMBIOS objects as SMBIOS namespace objects. However, I would like to avoid duplicating any information between the ARM namespace objects and SMBIOS namespace objects (e.g. information about CPU, Cache, etc.).
I have some initial thoughts on how this could be done by introducing an object mapper. However, I would first like to understand if you intend to use the Dynamic SMBIOS support only or you will use the Dynamic ACPI support as well?
[/SAMI]
Thanks
Abner

> > + if (EFI_ERROR (Status)) {
> > + DEBUG ((
> > + DEBUG_ERROR,
> > + "Failed to get Memory Devices CM Object %r\n",
> > + Status
> > + ));
> > + return Status;
> > + }
...

Join devel@edk2.groups.io to automatically receive all group messages.