Re: [edk2-devel] [PATCH v5 17/28] OvmfPkg: Use GetMemoryProtectionsLib instead of Memory Protection PCDs

["Laszlo Ersek" <lersek@redhat.com>]

On 10/9/23 02:07, Taylor Beebe wrote:
> Replace references to the memory protection PCDs to instead
> check the platform protections via GetMemoryProtectionsLib.
>
> Because the protection profile is equivalent to the PCD settings,
> this updated does not cause a torn state.
>
> Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Abner Chang <abner.chang@amd.com>
> ---
>  OvmfPkg/Fdt/HighMemDxe/HighMemDxe.c   | 5 ++---
>  OvmfPkg/QemuVideoDxe/VbeShim.c        | 3 ++-
>  OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf | 4 +---
>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 +-
>  4 files changed, 6 insertions(+), 8 deletions(-)

Should be two patches. Now I need to quote out of order.

>
> diff --git a/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.c b/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.c
> index 779bf5c827f5..2bef34427341 100644
> --- a/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.c
> +++ b/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.c
> @@ -13,6 +13,7 @@
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/GetMemoryProtectionsLib.h>
>
>  #include <Protocol/Cpu.h>
>  #include <Protocol/FdtClient.h>
> @@ -148,9 +149,7 @@ InitializeHighMemDxe (
>          // on the page table mappings by going through the cpu arch protocol.
>          //
>          Attributes = EFI_MEMORY_WB;
> -        if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> -             (1U << (UINT32)EfiConventionalMemory)) != 0)
> -        {
> +        if (gMps.Dxe.ExecutionProtection.EnabledForType[EfiConventionalMemory]) {
>            Attributes |= EFI_MEMORY_XP;
>          }
>

> diff --git a/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf b/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf
> index c7dde9f455f2..40cbbe1c39af 100644
> --- a/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf
> +++ b/OvmfPkg/Fdt/HighMemDxe/HighMemDxe.inf
> @@ -33,13 +33,11 @@ [LibraryClasses]
>    PcdLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  GetMemoryProtectionsLib
>
>  [Protocols]
>    gEfiCpuArchProtocolGuid                 ## CONSUMES
>    gFdtClientProtocolGuid                  ## CONSUMES
>
> -[Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
> -
>  [Depex]
>    gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid

Thus, with this, HighMemDxe loses its only PcdGet call -- I think you
should remove the PcdLib.h #include directive from the C file, and the
PcdLib dependency from [LibraryClasses] in the INF file.

> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
> index 8f151b96f9a5..a60e409f50de 100644
> --- a/OvmfPkg/QemuVideoDxe/VbeShim.c
> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
> @@ -19,6 +19,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/PciLib.h>
>  #include <Library/PrintLib.h>
> +#include <Library/GetMemoryProtectionsLib.h>
>  #include <OvmfPlatforms.h>
>
>  #include "Qemu.h"
> @@ -69,7 +70,7 @@ InstallVbeShim (
>    UINTN                 Printed;
>    VBE_MODE_INFO         *VbeModeInfo;
>
> -  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) {
> +  if (gMps.Dxe.NullPointerDetection.Enabled && !gMps.Dxe.NullPointerDetection.DisableEndOfDxe) {
>      DEBUG ((
>        DEBUG_WARN,
>        "%a: page 0 protected, not installing VBE shim\n",

The conversion looks right, at the surface, but could you also test it?

(See commit 90f3922b018e, "OvmfPkg/QemuVideoDxe: Bypass NULL pointer
detection during VBE SHIM installing", 2017-10-11. You'll need a Windows
7 or Windows Server 2008 R2 guest for triggering the debug message.)

> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> index 43a6e07faa88..15693ce85674 100644
> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> @@ -55,6 +55,7 @@ [LibraryClasses]
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib
> +  GetMemoryProtectionsLib
>
>  [Protocols]
>    gEfiGraphicsOutputProtocolGuid                # PROTOCOL BY_START

Please keep [LibraryClasses] sections, and all other sections in INF
files, alphabetically sorted -- assuming the section is already sorted
pre-patch.

(Unfortunately, in this case, the section is already in disorder; I
failed to catch the original mistake when reviewing the patch that would
become commit 5b2291f9567a, "OvmfPkg: QemuVideoDxe uses
MdeModulePkg/FrameBufferLib", 2016-10-12.)


> @@ -64,6 +65,5 @@ [Protocols]
>  [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>    gUefiOvmfPkgTokenSpaceGuid.PcdVideoResolutionSource
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109444): https://edk2.groups.io/g/devel/message/109444
Mute This Topic: https://groups.io/mt/101843361/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-