Re: [PATCH 08/12] ArmVirtXen: Use merged variable driver for emulated NV mode

[Laszlo Ersek <lersek@redhat.com>]

On 01/13/19 16:37, Star Zeng wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1323
> Merge EmuVariable and Real variable driver.
> 
> The real variable driver has been updated to support emulated
> variable NV mode and the EmuVariableRuntimeDxe will be removed
> later, so use merged variable driver for emulated NV mode.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

(1) Please CC Julien Grall (I'm adding him now); he's listed as a
Reviewer for ArmVirtPkg in "Maintainers.txt", and we depend on him for
Xen testing.

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  ArmVirtPkg/ArmVirtXen.dsc | 11 +++++++++--
>  ArmVirtPkg/ArmVirtXen.fdf |  4 ++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
> index a29d8a4ae717..126782372a58 100644
> --- a/ArmVirtPkg/ArmVirtXen.dsc
> +++ b/ArmVirtPkg/ArmVirtXen.dsc
> @@ -1,7 +1,7 @@
>  #
>  #  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
>  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
> -#  Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -172,7 +172,14 @@ [Components.common]
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>  
> -  MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
> +  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
> +    <PcdsFixedAtBuild>
> +      gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable|TRUE
> +    <LibraryClasses>
> +      AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
> +      TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> +      VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  }

OK, I've stared at this long and hard, to see if we should try to unify
it with the ArmVirtQemu* platforms. Ultimately, I don't think so (see
commits 44d71c217ccbd and bf453d581ecff, which don't apply to Xen).
Thus, I agree that adding the "VariableRuntimeDxe.inf" reference
*itself* to "ArmVirtXen.dsc" is fine.

That said, I'd suggest some tweaks:

(1) I think we should add the "PcdEmuVariableNvModeEnable" override
simply to the [PcdsFixedAtBuild.common] section of "ArmVirtXen.dsc". I
think there's no reason to scope the PCD more narrowly than that; so for
clarity, we shouldn't.

(2) The resolutions for the AuthVariableLib, TpmMeasurementLib,
VarCheckLib classes seem superfluous; they are already in effect, from
"ArmVirt.dsc.inc".

(3) The following tweak should be covered in an additional patch:

- In commit 7a9f5b202c7a ("ArmVirtPkg: Link separated VarCheckUefiLib
NULL class library instance", 2015-08-25), you linked VarCheckUefiLib
into VariableRuntimeDxe in ArmVirtQemu.

- In commit 8de84d424221 ("ArmVirtPkg: implement ArmVirtQemuKernel",
2016-02-05), Ard created the ArmVirtQemuKernel platform with
VarCheckUefiLib used at once.

- So I think at this point we should link VarCheckUefiLib into
ArmVirtXen's VariableRuntimeDxe binary as well -- however, because it is
a functional change, and not strictly related to this driver
unification, it should be a separate patch.

If you decide that (3) is out of scope for this work, we can file a BZ
for it and address it separately -- after BZ#1323 is fixed. Otherwise,
if it's not a big burden, you could include the patch in this series
too. Can you please state which approach you prefer, for (3)?

Thank you!
Laszlo

>  
>    MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
>    MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
> diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
> index 50e670254d52..5655c0df2926 100644
> --- a/ArmVirtPkg/ArmVirtXen.fdf
> +++ b/ArmVirtPkg/ArmVirtXen.fdf
> @@ -1,7 +1,7 @@
>  #
>  #  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
>  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
> -#  Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -137,7 +137,7 @@ [FV.FvMain]
>    INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>    INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>  
> -  INF MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
> +  INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>  
>    INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
>    INF MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>