Re: [edk2-devel] [PATCH 1/1] OvmfPkg/SmbiosPlatformDxe: use PcdFirmwareVersionString

["Gerd Hoffmann" <kraxel@redhat.com>]

On Mon, Oct 10, 2022 at 10:27:39AM -0600, Rebecca Cran wrote:
> On 10/10/2022 8:42 AM, Gerd Hoffmann wrote:
> 
> > Instead of using hard-coded string "0.0.0" for BiosVersion (which is
> > quite useless) read PcdFirmwareVersionString and append that to the
> > type0 entry string table.
> > 
> > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> > ---
> > 
> >   #define TYPE0_STRINGS \
> >     "EFI Development Kit II / OVMF\0"     /* Vendor */ \
> > -  "0.0.0\0"                             /* BiosVersion */ \
> > -  "02/06/2015\0"                        /* BiosReleaseDate */
> > +  "02/06/2015"                          /* BiosReleaseDate */
> 
> I know this is unrelated to this patch, but should we update the release date at some point?

I saw we also have PcdFirmwareVendor + PcdFirmwareReleaseDateString.
So I guess it makes sense to just generate the whole string table
dynamically.

Next question is how to set them.  I think it makes sense to have some
sensible defaults, but still allow to override them.  MdeModulePkg
defines them to empty strings (except vendor).  Should we set them to
the most recent stable tag instead, i.e. something like this?

-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L""|VOID*|0x00010052
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"edk2-stable202208"|VOID*|0x00010052

-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareReleaseDateString|L""|VOID*|0x00010053
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareReleaseDateString|L"26/08/2022"|VOID*|0x00010053

When doing that:  Can this be overridden on the command line?  Trying to
do so using 'build --pcd PcdFirmwareVersionString=Test' didn't work for
me, the string wasn't translated to unicode ...

I've noticed ArmVirtPkg/ArmVirtXen.dsc has this line ...

  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"$(FIRMWARE_VER)"

... which allows to override using 'build -D FIRMWARE_VER=Test'.
Unicode encoding works that way, but it would also override the
MdeModulePkg default (if we add one).

> > +    DEBUG ((DEBUG_INFO, "FirmwareVersionString: \"%s\" (%d chars)\n", Str16, Chars));
> > +
> > +    Type0 = AllocateZeroPool (sizeof (mOvmfDefaultType0) + Chars + 2);
> 
> Should we check for an allocation failure here?

Yes, fixed.

take care,
  Gerd