Re: [PATCH 2/2] ArmVirtPkg: allow setting Firmware Version from build command line

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Gerd,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 12/10/2022 11:39 am, Gerd Hoffmann wrote:
> On Wed, Oct 12, 2022 at 07:35:23AM +0000, Oliver Steffen wrote:
>> Initialize
>> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
>> with with the value of the variable "FIRMWARE_VER"
>> in all flavors of ArmVirtPkg.
>>
>> This behavior is already implemented in ArmVirtXen.dsc.
>> It allows specifying the firmware version string on the
>> build command line with -D FIRMARE_VER=...
> I think we need to decide which approach we want support for
> setting PcdFirmwareVersionString (and write it down in armvirt/ovmf
> readme).
>
> The options we have are:
>
>    (1) -D "FIRMARE_VER=${version}" (needs this patch), or
>    (2) --pcd "PcdFirmwareVersionString=L'${version}\\0'"
>
> Advantage of (1) is that the build command line is a bit simpler.
>
> Disadvantage of (1) is that it overrides PcdFirmwareVersionString
> even in case FIRMARE_VER is not set on the command line.  Which doesn't
> make much of a difference today because the default value defined in
> MdeModulePkg is just the empty string.  In case we set the default
> to something more useful (https://edk2.groups.io/g/devel/message/94985)
> overriding it is not so nice though ...

[SAMI] Thank you for pointing me to the MdeModulePkg patch.

I think we could do the following in ArmVirtPkg:

-----

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index c39e2506a3ea..49e96c9fb91c 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -289,6 +289,10 @@ [PcdsFeatureFlag.AARCH64]
    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE

  [PcdsFixedAtBuild.common]
+!ifdef $(FIRMWARE_VER)
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"$(FIRMWARE_VER)"
+!endif
+
    gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
    gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
    gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0

----

With this if -D "FIRMARE_VER=${version}" is not provided, the default 
string definition from MdeModulePkg would be used.

Also, since ArmVirt.dsc.inc is included all the platforms in ArmVirtPkg, 
this change is required only in one place (ArmVirtXen.dsc would need 
updating though).

[/SAMI]

>
> take care,
>    Gerd
>