Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Leif,
pt., 11 paź 2019 o 09:30 Marcin Wojtas napisał(a):
>
> Hi Leif,
>
> pt., 11 paź 2019 o 01:51 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
> > > napisał(a):
> > > >
> > > > On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
> > > > > From: Patryk Duda
> > > > >
> > > > > 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
> > > > > ---
> > > > > 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*|0x334
> > > > >gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x335
> > > > >
> > > > > +#Platform description
> > > > > + gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell
> > > > >\0"|VOID*|0x5100
> > > > > + gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development
> > > > > Board \0"|VOID*|0x5101
> > > > > + gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set
> > > > > \0"|VOID*|0x5103
> > > > > + gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown
> > > > > \0"|VOID*|0x5102
> > > > > +
> > > >
> > > > 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 (PcdPr
Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Hi Leif,
pt., 11 paź 2019 o 01:51 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
> > napisał(a):
> > >
> > > On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
> > > > From: Patryk Duda
> > > >
> > > > 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
> > > > ---
> > > > 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*|0x334
> > > >gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x335
> > > >
> > > > +#Platform description
> > > > + gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell
> > > > \0"|VOID*|0x5100
> > > > + gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development
> > > > Board \0"|VOID*|0x5101
> > > > + gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set
> > > >\0"|VOID*|0x5103
> > > > + gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown
> > > > \0"|VOID*|0x5102
> > > > +
> > >
> > > 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_ER
Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
On Fri, Oct 11, 2019 at 01:33:49AM +0200, Marcin Wojtas wrote:
> Hi Leif,
>
> pt., 11 paź 2019 o 01:04 Leif Lindholm napisał(a):
> >
> > On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
> > > From: Patryk Duda
> > >
> > > 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
> > > ---
> > > 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*|0x334
> > >gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x335
> > >
> > > +#Platform description
> > > + gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell
> > >\0"|VOID*|0x5100
> > > + gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development
> > > Board \0"|VOID*|0x5101
> > > + gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set
> > > \0"|VOID*|0x5103
> > > + gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown
> > > \0"|VOID*|0x5102
> > > +
> >
> > 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.
Best Regards,
Leif
> 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
> > >
> > >
Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Hi Leif,
pt., 11 paź 2019 o 01:04 Leif Lindholm napisał(a):
>
> On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
> > From: Patryk Duda
> >
> > 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
> > ---
> > 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*|0x334
> >gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x335
> >
> > +#Platform description
> > + gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell
> > \0"|VOID*|0x5100
> > + gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development Board
> > \0"|VOID*|0x5101
> > + gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set
> > \0"|VOID*|0x5103
> > + gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown
> > \0"|VOID*|0x5102
> > +
>
> 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote:
> From: Patryk Duda
>
> 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
> ---
> 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*|0x334
>gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x335
>
> +#Platform description
> + gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell
>\0"|VOID*|0x5100
> + gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development Board
>\0"|VOID*|0x5101
> + gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set
> \0"|VOID*|0x5103
> + gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown
> \0"|VOID*|0x5102
> +
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.
> #RTC
>gMarvellTokenSpaceGuid.PcdRtcBaseAddress|0x0|UINT64|0x4052
>
> diff --git a/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> b/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> index 8b4586c..7722146 100644
> --- a/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> +++ b/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> @@ -36,6 +36,10 @@
>
> [FixedPcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision
> + gMarvellTokenSpaceGuid.PcdProductManufacturer
> + gMarvellTokenSpaceGuid.PcdProductPlatformName
> + gMarvellTokenSpaceGuid.PcdProductSerial
> + gMarvellTokenSpaceGuid.PcdProductVersion
>
> [Protocols]
>gEfiSmbiosProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> diff --git a/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> b/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> index 08f4fa7..c5b1d77 100644
> --- a/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> +++ b/Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> @@ -21,6 +21,22 @@
> #include
>
> //
> +// SMBIOS specification indicates that there is no limit for string size.
> +// However, some strings are printed in UEFI and OS. Printing very big string
> +// can lead to unexpected behaviour. Second reason of string size definition
> +// is that static buffers can be used instead of dynamic ones.
> +//
> +// Nevertheless, this value can be increased if necessary
> +//
> +
> +#define MV_SMBIOS_STRING_MAX_SIZE 32
> +
> +STATIC CHAR8 mSysInfoManufacturer[MV_SMBIOS_STRING_MAX_SIZE];
> +STATIC CHAR8 mSysInfoProductName[MV_SMBIOS_STRING_MAX_SIZE];
> +STATIC CHAR8 mSysInfoVersion[MV_SMBIOS_STRING_MAX_SIZE];
> +STATIC CHAR8 mSysInfoSerial[MV_SMBIOS_STRING_MAX_SIZE];
> +
> +//
> // SMBIOS tables often reference each other using
> // fixed constants, define a list of these constants
> // for our hardcoded tables
> @@ -101,10 +117,10 @@ STATIC SMBIOS_TABLE_TYPE1 mArmadaDefaultType1 = {
> };
>
> STATIC CHAR8 CONST *mArmadaDefaultType1Strings[] = {
> - "Marvell\0",/* Manufacturer */
> - "Armada 7k/8k Family Board \0",/* Product Name placeholder*/
> - "Revision unknown \0",/* Version placeholder */
> - " \0",/* 32 character buffer */
> + mSysInfoManufacturer,
> + mSysInfoProductName,
> + mSysInfoVersion,
> + mSysInfoSerial,
>NULL
> };
>
> @@ -129,10 +145,10 @@ STATIC SMBIOS_TABLE_TYPE2 mArmadaDefaultType2 = {
> };
>
> STATIC CHAR8 CONST *mArmadaDefaultType2Strings[] = {
> - "Marvell\0",/* Manufacturer */
> - "Armada 7k/8k Family Board \0",/* Product Name placeholder*/
> - "Revision unknown \0",/* Version placeholder */
> - "Serial Not Set \0",/* Serial */
> + mSysInfoManufacturer,
> + mSysInfoProductName,
> + mSysInfoVersion,
> + mSysInfoSerial,
>"Base of Chassis\0",/* Board location */
>NULL
> };
> @@ -160,9 +176,9 @@ STATIC SMBIOS_TABLE_TYPE3 mArmadaDefaultType3 = {
> };
>
> STATIC CHAR8 CONST *mArmadaDefaultType3Strings[]
