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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-