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

["Rebecca Cran" <rebecca@nuviainc.com>]

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?


> +  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.


> +  //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?

-- 
Rebecca Cran