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

["Sami Mujawar" <sami.mujawar@arm.com>]

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://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;
    > > +  }
...