public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Sami Mujawar" <sami.mujawar@arm.com>
To: "Chang, Abner" <Abner.Chang@amd.com>,
	"devel@edk2.groups.io" <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
Date: Wed, 14 Sep 2022 15:34:38 +0000	[thread overview]
Message-ID: <5DCF0CDE-BD08-4549-94D2-E1D72448C09B@arm.com> (raw)
In-Reply-To: <MN2PR12MB39669883C8CD5A6782DF0E39EA479@MN2PR12MB3966.namprd12.prod.outlook.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;
    > > +  }
...


  reply	other threads:[~2022-09-14 15:34 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-26 17:37 [PATCH 1/2] DynamicTablesPkg: Add SMBIOS table generation gmahadevan
2022-08-26 17:37 ` [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator Girish Mahadevan
2022-09-12 14:57   ` Sami Mujawar
2022-09-13  3:00     ` [edk2-devel] " Chang, Abner
2022-09-14 15:34       ` Sami Mujawar [this message]
2022-09-15 15:19         ` Chang, Abner
2022-10-04 22:43     ` Girish Mahadevan
2022-10-18 15:36       ` [edk2-devel] " Sami Mujawar
2023-01-28  0:09         ` Girish Mahadevan
2023-03-08  8:18           ` Sami Mujawar
2022-09-12 14:55 ` [PATCH 1/2] DynamicTablesPkg: Add SMBIOS table generation Sami Mujawar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5DCF0CDE-BD08-4549-94D2-E1D72448C09B@arm.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox