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

Marcin Wojtas posted 9 patches 6 years, 4 months ago
There is a newer version of this series
[edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Posted by Marcin Wojtas 6 years, 4 months ago
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
+
 #RTC
   gMarvellTokenSpaceGuid.PcdRtcBaseAddress|0x0|UINT64|0x40000052
 
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 <IndustryStandard/SmBios.h>
 
 //
+// 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[] = {
-  "Marvell                        \0",/* Manufacturer placeholder */
-  "Revision unknown               \0",/* Version placeholder */
-  "Serial Not Set                 \0",/* Serial placeholder */
+  mSysInfoManufacturer,
+  mSysInfoVersion,
+  mSysInfoSerial,
   NULL
 };
 
@@ -743,6 +759,45 @@ SmbiosMemoryInstall (
 }
 
 /**
+   Copy Type1, Type2, Type3 strings form PCD
+**/
+
+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);
+
+  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 (#48702): https://edk2.groups.io/g/devel/message/48702
Mute This Topic: https://groups.io/mt/34471905/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Posted by Leif Lindholm 6 years, 4 months ago
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.

>  #RTC
>    gMarvellTokenSpaceGuid.PcdRtcBaseAddress|0x0|UINT64|0x40000052
>  
> 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 <IndustryStandard/SmBios.h>
>  
>  //
> +// 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[] = {
> -  "Marvell                        \0",/* Manufacturer placeholder */
> -  "Revision unknown               \0",/* Version placeholder */
> -  "Serial Not Set                 \0",/* Serial placeholder */
> +  mSysInfoManufacturer,
> +  mSysInfoVersion,
> +  mSysInfoSerial,
>    NULL
>  };
>  
> @@ -743,6 +759,45 @@ SmbiosMemoryInstall (
>  }
>  
>  /**
> +   Copy Type1, Type2, Type3 strings form PCD
> +**/
> +
> +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?

/
    Leif

> +
> +  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 (#48759): https://edk2.groups.io/g/devel/message/48759
Mute This Topic: https://groups.io/mt/34471905/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Posted by Marcin Wojtas 6 years, 4 months ago
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
> >

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Posted by Leif Lindholm 6 years, 4 months ago
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@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?

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
> > >
> > >     @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 (#48765): https://edk2.groups.io/g/devel/message/48765
Mute This Topic: https://groups.io/mt/34471905/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Posted by Marcin Wojtas 6 years, 4 months ago
Hi Leif,

pt., 11 paź 2019 o 01:51 Leif Lindholm <leif.lindholm@linaro.org> 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@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?
>
> 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
> > > >

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

View/Reply Online (#48801): https://edk2.groups.io/g/devel/message/48801
Mute This Topic: https://groups.io/mt/34471905/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD
Posted by Marcin Wojtas 6 years, 4 months ago
Leif,

pt., 11 paź 2019 o 09:30 Marcin Wojtas <mw@semihalf.com> napisał(a):
>
> Hi Leif,
>
> pt., 11 paź 2019 o 01:51 Leif Lindholm <leif.lindholm@linaro.org> 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@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?
> >
> > 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))];
>

Compilation test was not enough, please ignore my previous email - I
have to find out something better :)

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 (#48804): https://edk2.groups.io/g/devel/message/48804
Mute This Topic: https://groups.io/mt/34471905/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-