From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3F297211B76B3 for ; Mon, 14 Jan 2019 03:40:13 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6DC395AF7B; Mon, 14 Jan 2019 11:40:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-239.rdu2.redhat.com [10.10.120.239]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1962B5D73F; Mon, 14 Jan 2019 11:40:10 +0000 (UTC) To: Star Zeng References: <1547393875-37188-1-git-send-email-star.zeng@intel.com> <1547393875-37188-9-git-send-email-star.zeng@intel.com> Cc: edk2-devel@lists.01.org, Julien Grall , Ard Biesheuvel From: Laszlo Ersek Message-ID: <45e10e1d-c410-6fb2-22e3-0bab184f7f15@redhat.com> Date: Mon, 14 Jan 2019 12:40:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1547393875-37188-9-git-send-email-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 14 Jan 2019 11:40:12 +0000 (UTC) Subject: Re: [PATCH 08/12] ArmVirtXen: Use merged variable driver for emulated NV mode X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Jan 2019 11:40:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ard Biesheuvel (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 > --- > 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.
> +# Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
> # > # 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 { > + > + gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable|TRUE > + > + 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.
> +# Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
> # > # 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 >