Re: [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD

Marcin Wojtas
 

Hi Leif,

pt., 11 paź 2019 o 01:51 Leif Lindholm <leif.lindholm@...> napisał(a):

On Fri, Oct 11, 2019 at 01:33:49AM +0200, Marcin Wojtas wrote:
Hi Leif,

pt., 11 paź 2019 o 01:04 Leif Lindholm <leif.lindholm@...> napisał(a):

On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
From: Patryk Duda <pdk@...>

This patch implements convenient way of changing strings included
in SMBIOS Table1, Table2, Table3.

Strings can be altered by defining following PCDs:
gMarvellTokenSpaceGuid.PcdProductManufacturer
gMarvellTokenSpaceGuid.PcdProductPlatformName
gMarvellTokenSpaceGuid.PcdProductVersion
gMarvellTokenSpaceGuid.PcdProductSerial

This patch adds also limit for length of string which can be increased
if necessary in future.

Signed-off-by: Patryk Duda <pdk@...>
---
Silicon/Marvell/Marvell.dec | 6 ++
Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 4 +
Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 79 +++++++++++++++++---
3 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
index d337d3e..a84b056 100644
--- a/Silicon/Marvell/Marvell.dec
+++ b/Silicon/Marvell/Marvell.dec
@@ -169,6 +169,12 @@
gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x0 }|VOID*|0x3000034
gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x3000035

+#Platform description
+ gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell \0"|VOID*|0x50000100
+ gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development Board \0"|VOID*|0x50000101
+ gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set \0"|VOID*|0x50000103
+ gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown \0"|VOID*|0x50000102
+
I'm not too pleased about this random number of spaces. I'm cool with
the strings, but they should be treated as such, not magical data
structures.
In v4 the trailing spaces will be removed from the defaults (as well as "\0").

+STATIC
+VOID
+MvSmbiosCopyStrings (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductManufacturer),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductPlatformName),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductVersion),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
+ ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductSerial),
+ MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE);
Especially given the current design, these ASSERTs seem a bit
... unhelpful. In fact, this whole MAX_SIZE thing seems ... restricted
by the implementation, not by external constraints. What is the
benefit? Not having to do a bunch of pointer conversions at
SetVirtualAddressMap?
The default static tables require constant initializers and the
placeholders should have some predefined size in current approach. So
max of 32 characters was picked and with the asserts, ensuring the
user will not exceed it. Do you think they should be removed?
Oh, you're saying this is basically "TO BE FILLED BY OEM"?
*yurgh*.

I still say it's horrible, but not more horrible than most existing
platforms. Nevertheless, the arbitrary size should be something
defined by the code generating the tables - it shouldn't depend on
what's in the .dec (or more usefully, the .dsc).

I also feel that if it is effectively "TO BE FILLED BY OEM", it would
be better if it said that than something that looks like it may be
proper names.

I would also prefer a compilation failure over an ASSERT if the string
ended up not fitting.
I think I found a solution to remove any fixed size constraint and asserts:
STATIC CHAR8 mSysInfoManufacturer[sizeof ((CHAR8 *)PcdGetPtr
(PcdProductManufacturer))];
STATIC CHAR8 mSysInfoProductName[sizeof ((CHAR8 *)PcdGetPtr
(PcdProductPlatformName))];
STATIC CHAR8 mSysInfoVersion[sizeof ((CHAR8 *)PcdGetPtr (PcdProductVersion))];
STATIC CHAR8 mSysInfoSerial[sizeof ((CHAR8 *)PcdGetPtr (PcdProductSerial))];

Please let know, if it's acceptable for you.

Best regards,
Marcin




+
+ Status = AsciiStrCpyS (mSysInfoManufacturer,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductManufacturer));
+ ASSERT_EFI_ERROR (Status);
+ Status = AsciiStrCpyS (mSysInfoProductName,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductPlatformName));
+ ASSERT_EFI_ERROR (Status);
+ Status = AsciiStrCpyS (mSysInfoVersion,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductVersion));
+ ASSERT_EFI_ERROR (Status);
+ Status = AsciiStrCpyS (mSysInfoSerial,
+ MV_SMBIOS_STRING_MAX_SIZE,
+ (CHAR8 *)PcdGetPtr (PcdProductSerial));
+ ASSERT_EFI_ERROR (Status);
+}
+
+/**
Install all structures from the DefaultTables structure

@param Smbios SMBIOS protocol
@@ -760,6 +815,8 @@ SmbiosInstallAllStructures (
FirmwareMajorRevisionNumber = (PcdGet32 (PcdFirmwareRevision) >> 16) & 0xFF;
FirmwareMinorRevisionNumber = PcdGet32 (PcdFirmwareRevision) & 0xFF;

+ MvSmbiosCopyStrings();
+
//
// Update Firmware Revision, CPU and DRAM frequencies.
//
--
2.7.4

Join devel@edk2.groups.io to automatically receive all group messages.