Re: [edk2-devel] [PATCH v4 10/10] ArmPkg: Add Universal/Smbios, a generic SMBIOS library for ARM

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

Hi Rebecca,

Please see my reply inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Rebecca Cran <rebecca@nuviainc.com> 
Sent: 03 January 2021 11:52 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io
Cc: Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@intel.com>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v4 10/10] ArmPkg: Add Universal/Smbios, a generic SMBIOS library for ARM

On 12/29/20 8:10 AM, Sami Mujawar wrote:

> Apologies for the delay in reviewing this patch.
> Please find my response inline marked [SAMI].

Thanks. I have a few comments: I've replied inline. Where I haven't 
replied, I've made the changes you suggested.


> +    2,                        // ProcessorManufacture
> [SAMI] Would defining a macro PROC_MANUFACTURER_STR_ID or an enum be helpful? Similarly, for other strings used across SMBIOS tables in this patch.
> [/SAMI]

The string IDs are specific to the table, so I'm not sure we should 
define a constant to re-use?
[SAMI] I see your point. It does not make sense to define macros to get rid of magic numbers, especially when these macros would not be used elsewhere.
[/SAMI]


> +  Clidr.Data = ReadCLIDR ();
> [SAMI] The ReadCLIDR() and similar functions would run on the current PE. I think this code would not work with a big.LITTLE system or a system that utilises a DSU with different CPUs.
> Is the assumption here that all PEs in the system are same?
> [/SAMI]


Yes, that code currently assumes all CPUs are the same.
I'll add code to allow platforms to specify different cache information 
tables for CPUs that are different.


> +  FreePool (HandleArray);
> [SAMI] Please correct me if I am wrong, from the spec it appears that there can be n handles appended at the end of the table. However, the code above appears to only assign the first handle.
> I think GetLinkTypeHandle() should be called before allocating the memory for the SmbiosRecord. That way additional space for the n handles can be allocated. The handle list can then be appended to the end of the TYPE2 table.
> So, the table data should look something like: SMBIOS_TABLE_TYPE2 + (n * HANDLES) + StringData.
> Does this also mean that the TYPE2 table should be the last table to be populated? Should SmbiosMiscEntryPoint() be modified to schedule the population of TYPE2 table at the end?
> [/SAMI]

The code fetches the first of the _chassis_ handles, which it's presumed 
there will only be one.
[SAMI] A comment to reflect this may be helpful if this is specific to this implementation. I was confused as the code sets the 'Board Type' to motherboard and the spec says "List of handles of other structures (for example, Baseboard,
Processor, Port, System Slots, Memory Device) that are contained by this baseboard". So, was expecting that this list would include Processor, etc.
[/SAMI]


> +  //ContainedElements
> +  (VOID)CopyMem (SmbiosRecord + 1, &ContainedElements, ExtendLength);
> [SAMI] If I understand correctly, the Contained Element data is never really copied, right?
> [/SAMI]


I'm not sure why it wouldn't be copied. Could you elaborate?
[SAMI] If InputData->ContainedElementCount = 0 or 1, then 'CopyMem (SmbiosRecord, InputData, sizeof (SMBIOS_TABLE_TYPE3));' should be ok (assuming InputData->ContainedElements[0] is populated with the relevant data).
But if ContainedElementCount = 2 then the SmbiosRecord is populated with zeros, as ContainedElements is a local variable that is zero initialised. 
Also, if ContainedElementCount > 2 the copy operation would access invalid stack data which could be a problem.
[/SAMI]


-- 
Rebecca Cran