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

[Marcin Wojtas]

Hi Leif,

pt., 11 paź 2019 o 01:04 Leif Lindholm <leif.lindholm at linaro.org> napisał(a):
>
> On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
> > From: Patryk Duda <pdk at semihalf.com>
> >
> > 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 at semihalf.com>
> > ---
> >  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?

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48763): https://edk2.groups.io/g/devel/message/48763
Mute This Topic: https://groups.io/mt/34471905/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-