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

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Wednesday, September 14, 2022 11:35 PM
> To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io; Girish
> Mahadevan <gmahadevan@nvidia.com>; Alexei Fedorov
> <Alexei.Fedorov@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Jeff Brasen
> (jbrasen@nvidia.com) <jbrasen@nvidia.com>; Ashish Singhal
> <ashishsingha@nvidia.com>; Akanksha Jain <Akanksha.Jain2@arm.com>;
> Matteo Carlini <Matteo.Carlini@arm.com>; Hemendra Dassanayake
> <Hemendra.Dassanayake@arm.com>; Nick Ramirez <nramirez@nvidia.com>;
> William Watson <wwatson@nvidia.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17
> Table generator
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> Hi Abner,
> 
> Please see my response inline marked [SAMI].
> 
> Regards,
> 
> Sami Mujawar
> 
> On 13/09/2022, 04:00, "Chang, Abner" <Abner.Chang@amd.com> 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@nvidia.com>; devel@edk2.groups.io;
>     > Alexei Fedorov <Alexei.Fedorov@arm.com>
>     > Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Jeff
>     > Brasen <jbrasen@nvidia.com>; Ashish Singhal <ashishsingha@nvidia.com>;
>     > Akanksha Jain <Akanksha.Jain2@arm.com>; Matteo Carlini
>     > <Matteo.Carlini@arm.com>; Hemendra Dassanayake
>     > <Hemendra.Dassanayake@arm.com>; Nick Ramirez
> <nramirez@nvidia.com>;
>     > William Watson <wwatson@nvidia.com>; Akanksha Jain
>     > <Akanksha.Jain2@arm.com>; nd@arm.com
>     > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FDynamicTablesPkg%2FInclude
> %2FConfigurationManagerObject.h%23L30-
> L34&amp;data=05%7C01%7CAbner.Chang%40amd.com%7C425be8c5f1464596
> 4ada08da9666aa5f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6
> 37987664940879922%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
> LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&am
> p;sdata=trKkXJe7yDBkoKIlSk5kCI0tZ6nG573wXqORHfd5y2o%3D&amp;reserved=
> 0
> We can then define the SMBIOS objects as SMBIOS namespace objects.
[Abner]
This sounds good. We can define SMBIOS name space and the corresponding object in SmbiosNameSpaceObjects.h under \Include. So platform has to install EDKII_CONFIGURATION_MANAGER_PROTOCOL and returns the SMBIOS object, right?

> 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.
[Abner] 
I think you refer to have SMBIOS namespace object as the generic one, but introduce a wrapper on it for the processor arch specific information?

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?
[ Abner] 
I am not sure if we will also use Dynamic ACPI table at the moment but that would be possible. The current use case I can think of is to build up SMBIOS 42h using dynamic SMBIOS table generator.
This also brings another question, the current DynamicTableFactoryDxe pulls in ACPI/SMBIOS/DT table generator functions; what if the platform only uses SMBIOS functionality? Pull in ACPI/DT code into DynamicTableFactoryDxe seems redundant. I think we can consider to make AcpiTableFactory/DeviceTreeTableFactory/SmbiosTableFactor as libraries, use NULL instances (empty function for Get/Register/Deregister generator) as default for DynamicTableFactoryDxe. Platform can pull in the library that has the implementation into build on demand. Not for now but we can do it later if you think this makes sense.

BTW what is the 'E' prefix to "Arm" means? E.g. Get'E'ArmObjMemoryDeviceInfo

Thanks
Abner

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