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

["Zeng, Star" <star.zeng@intel.com>]

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 <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.

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
>>