Hi Laszlo,

On 2019/1/14 19:40, Laszlo Ersek wrote:
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 <ler...@redhat.com>
Cc: Ard Biesheuvel <ard.biesheu...@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.z...@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.

Agree.


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

Good information.


(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)?

A new patch has been added for it in V2 patch series at https://lists.01.org/pipermail/edk2-devel/2019-January/035015.html.

Thanks,
Star


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


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to