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

["Marcin Wojtas" <mw@semihalf.com>]

Hi Leif,

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