“Maybe you entirely missed my message that I posted in response to
version 2 of this specific patch (i.e. you may have fully missed the
message I link at the top). That could be the case because I mentioned
"OvmfXen.dsc" under the v2 blurb as well.”

Yup. That’s the one. Saw this request, but not the patch feedback. Will address in the next version.

You want the PCD dropped just for Xen?

I would posit that dropping it for all of OVMF would negate the ability to use the unittest test to confirm the functionality in CI, which is something I would like to light up in future revisions, so I would need to hear the argument against it.

- Bret

From: Laszlo Ersek<mailto:lersek@redhat.com>
Sent: Friday, May 22, 2020 2:41 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>
Cc: Jordan Justen<mailto:jordan.l.justen@intel.com>; Ard Biesheuvel<mailto:ard.biesheuvel@arm.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v3 05/14] OvmfPkg: Add VariablePolicy engine to OvmfPkg platform

Hello Michael / Bret,

I don't understand the (lack of) updates in this patch:

On 05/22/20 00:43, Michael Kubacki wrote:
> From: Bret Barkelew <brbarkel@microsoft.com>
>
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2522&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C10ad08aa2f8b41619cdc08d7fe98e2f3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637257804909549643&amp;sdata=f2vsskZHgV2wgxtcFqxigKPVpSyz6hBE1Y74qEsAWTs%3D&amp;reserved=0
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Bret Barkelew <brbarkel@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 8 ++++++++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 8 ++++++++
>  OvmfPkg/OvmfPkgX64.dsc     | 8 ++++++++
>  OvmfPkg/OvmfXen.dsc        | 7 +++++++

My request (1) under the corresponding v2 patch was to include the
OvmfXen.dsc platform in the modifications. That request has been addressed.

Please find said v2 feedback from me here:

https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2Fa0e0e3d4-6712-078a-4d95-29408109b0b0%40redhat.com&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C10ad08aa2f8b41619cdc08d7fe98e2f3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637257804909549643&amp;sdata=8ajjcCIXFpvLAylSu9SBamajUhApYVtsJ6hUZQsG%2BXQ%3D&amp;reserved=0

(Alternative link: <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F59271&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C10ad08aa2f8b41619cdc08d7fe98e2f3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637257804909549643&amp;sdata=R4S5YPpNwBcUSy03I8g3Jqs6dk%2Ba6lZTFaL6iajwLFM%3D&amp;reserved=0>.<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F59271&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C10ad08aa2f8b41619cdc08d7fe98e2f3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637257804909549643&amp;sdata=R4S5YPpNwBcUSy03I8g3Jqs6dk%2Ba6lZTFaL6iajwLFM%3D&amp;reserved=0%3e.>)

However:

>  4 files changed, 31 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index cbc5f0e583bc..2c64591f88a3 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -3,6 +3,7 @@
>  #
>  #  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) Microsoft Corporation.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -194,6 +195,8 @@ [LibraryClasses]
>    AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
>  !endif
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> +  VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
>
>
>    #
> @@ -327,6 +330,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
>
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -480,6 +484,9 @@ [PcdsFixedAtBuild]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
>  !endif
>
> +  # Optional: Omit if VariablePolicy should be always-on.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
> +

My request (2) in the same message was to drop this PCD setting. That
request has not been addressed.

So I'm now confused -- based on addressing (1), it seems like my v2
review has been processed. But then, why was my request (2) silently
ignored? Did you miss it somehow?

... Maybe you entirely missed my message that I posted in response to
version 2 of this specific patch (i.e. you may have fully missed the
message I link at the top). That could be the case because I mentioned
"OvmfXen.dsc" under the v2 blurb as well. So perhaps you only read my
feedback to the blurb.

In v4, please remove the "PcdAllowVariablePolicyEnforcementDisable"
setting. The reason why I'm requesting that is captured in my v2
feedback (see link near the top).

Thanks,
Laszlo

>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> @@ -921,6 +928,7 @@ [Components]
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      NULL|MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>    }
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 6d69cc6cb56f..99527e03b9d0 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -3,6 +3,7 @@
>  #
>  #  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) Microsoft Corporation.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -198,6 +199,8 @@ [LibraryClasses]
>    AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
>  !endif
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> +  VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
>
>
>    #
> @@ -331,6 +334,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
>
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -484,6 +488,9 @@ [PcdsFixedAtBuild]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
>  !endif
>
> +  # Optional: Omit if VariablePolicy should be always-on.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> @@ -934,6 +941,7 @@ [Components.X64]
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      NULL|MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>    }
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 5ad4f461ce52..4a6b18d7899d 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -3,6 +3,7 @@
>  #
>  #  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) Microsoft Corporation.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -198,6 +199,8 @@ [LibraryClasses]
>    AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
>  !endif
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> +  VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
>
>
>    #
> @@ -331,6 +334,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
>
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -484,6 +488,9 @@ [PcdsFixedAtBuild]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
>  !endif
>
> +  # Optional: Omit if VariablePolicy should be always-on.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> @@ -932,6 +939,7 @@ [Components]
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      NULL|MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>    }
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 47ee8db8b884..c2d476133b9d 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -3,6 +3,7 @@
>  #
>  #  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) Microsoft Corporation.<BR>
>  #  Copyright (c) 2019, Citrix Systems, Inc.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -182,6 +183,8 @@ [LibraryClasses]
>
>    AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> +  VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
>
>
>    #
> @@ -301,6 +304,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
>
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -394,6 +398,9 @@ [PcdsFixedAtBuild]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
>  !endif
>
> +  # Optional: Omit if VariablePolicy should be always-on.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>