[edk2-devel] [PATCH v4 10/10] ArmPkg: Add Universal/Smbios, a generic SMBIOS library for ARM
Rebecca Cran
rebecca at nuviainc.com
Sun Jan 3 23:52:04 UTC 2021
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69516): https://edk2.groups.io/g/devel/message/69516
Mute This Topic: https://groups.io/mt/78784077/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list