* [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. @ 2017-09-13 8:07 Wang, Jian J 0 siblings, 0 replies; 21+ messages in thread From: Wang, Jian J @ 2017-09-13 8:07 UTC (permalink / raw) To: edk2-devel QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer detection is enabled, page 0 must be enabled temporarily before installing and disabled again afterwards. For Windows 7 boot, BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Justen, Jordan L <jordan.l.justen@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com> Cc: Wolman, Ayellet <ayellet.wolman@intel.com> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> --- OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 --- a/OvmfPkg/QemuVideoDxe/Driver.c +++ b/OvmfPkg/QemuVideoDxe/Driver.c @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( PCI_TYPE00 Pci; QEMU_VIDEO_CARD *Card; EFI_PCI_IO_PROTOCOL *ChildPciIo; + EFI_CPU_ARCH_PROTOCOL *Cpu; OldTpl = gBS->RaiseTPL (TPL_CALLBACK); @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || Private->Variant == QEMU_VIDEO_BOCHS) { - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); + // + // Prepare CPU arch protocol for NULL pointer detection + // + Status = gBS->LocateProtocol ( + &gEfiCpuArchProtocolGuid, + NULL, + (VOID **) &Cpu + ); + ASSERT_EFI_ERROR (Status); + + DISABLE_NULL_DETECTION(Cpu); + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); + ENABLE_NULL_DETECTION(Cpu); } #endif diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 --- a/OvmfPkg/QemuVideoDxe/Qemu.h +++ b/OvmfPkg/QemuVideoDxe/Qemu.h @@ -25,6 +25,7 @@ #include <Protocol/PciIo.h> #include <Protocol/DriverSupportedEfiVersion.h> #include <Protocol/DevicePath.h> +#include <Protocol/Cpu.h> #include <Library/DebugLib.h> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ typedef struct { #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff +// +// VBE code will access memory between 0-4095 which will cause page fault exception +// if NULL pointer detection mechanism is enabled. Following macros can be used to +// disable/enable NULL pointer detection before/after accessing those memory. +// +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) +#define DISABLE_NULL_DETECTION(Cpu) \ + if (NULL_DETECTION_ENABLED) { \ + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ + } +#define ENABLE_NULL_DETECTION(Cpu) \ + if (NULL_DETECTION_ENABLED) { \ + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ + } + // // QEMU Video Private Data Structure // diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf index 7c7d429bca..5d166eb99c 100644 --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf @@ -72,7 +72,9 @@ gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START gEfiDevicePathProtocolGuid # PROTOCOL BY_START gEfiPciIoProtocolGuid # PROTOCOL TO_START + gEfiCpuArchProtocolGuid [Pcd] gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask -- 2.14.1.windows.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <Implement NULL pointer detection feature>]
* [PATCH 0/4] Implement NULL pointer detection feature for special pool [not found] <Implement NULL pointer detection feature> @ 2017-09-13 9:25 ` Wang, Jian J 2017-09-13 9:25 ` [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled Wang, Jian J 0 siblings, 1 reply; 21+ messages in thread From: Wang, Jian J @ 2017-09-13 9:25 UTC (permalink / raw) To: edk2-devel The mechanism behind is to trigger a page fault exception at address 0. This can be made by disabling page 0 (0-4095) during page table setup. So this feature can only be available on platform with paging enabled. Once this feature is enabled, any code, like CSM, which has to access memory in page 0 needs to enable this page temporarily in advance and disable it afterwards. PcdNullPointerDetectionPropertyMask is used to control and elaborate the use cases. For example, BIT7 of this PCD must be set for Windows 7 boot on Qemu if BIT0 set; or boot will fail. Wang, Jian J (4): Implement NULL pointer detection in EDK-II Core. Implement NULL pointer detection for SMM mode code. Update CSM code to temporarily bypass NULL pointer detection if enabled. Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c | 10 +++- .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h | 18 +++++++ .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf | 2 + .../Csm/LegacyBiosDxe/LegacyBda.c | 4 ++ .../Csm/LegacyBiosDxe/LegacyBios.c | 55 ++++++++++++++++++---- .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf | 2 + .../Csm/LegacyBiosDxe/LegacyBiosInterface.h | 23 +++++++++ .../Csm/LegacyBiosDxe/LegacyBootSupport.c | 33 ++++++++++--- .../Csm/LegacyBiosDxe/LegacyPci.c | 17 ++++++- IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c | 41 ++++++++++------ MdeModulePkg/Core/Dxe/DxeMain.inf | 3 +- MdeModulePkg/Core/Dxe/Mem/Page.c | 21 +++++---- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 47 ++++++++++++++++++ MdeModulePkg/Core/DxeIplPeim/DxeIpl.h | 15 ++++++ MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 3 +- MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 53 +++++++++++++++++++++ MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 8 +++- MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 2 + MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++++++--- MdeModulePkg/MdeModulePkg.dec | 12 +++++ OvmfPkg/QemuVideoDxe/Driver.c | 15 +++++- OvmfPkg/QemuVideoDxe/Qemu.h | 16 +++++++ OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 + UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++++ UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 25 +++++++++- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 2 + UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++---- UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 11 +++++ 28 files changed, 429 insertions(+), 62 deletions(-) -- 2.14.1.windows.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-13 9:25 ` [PATCH 0/4] Implement NULL pointer detection feature for special pool Wang, Jian J @ 2017-09-13 9:25 ` Wang, Jian J 2017-09-13 16:33 ` Johnson, Brian (EXL - Eagan) 2017-09-13 23:34 ` Laszlo Ersek 0 siblings, 2 replies; 21+ messages in thread From: Wang, Jian J @ 2017-09-13 9:25 UTC (permalink / raw) To: edk2-devel Cc: Jiewen Yao, Eric Dong, Star Zeng, Laszlo Ersek, Justen, Jordan L, Kinney, Michael D, Wolman, Ayellet QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer detection is enabled, page 0 must be enabled temporarily before installing and disabled again afterwards. For Windows 7 boot, BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Justen, Jordan L <jordan.l.justen@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com> Cc: Wolman, Ayellet <ayellet.wolman@intel.com> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> --- OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 --- a/OvmfPkg/QemuVideoDxe/Driver.c +++ b/OvmfPkg/QemuVideoDxe/Driver.c @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( PCI_TYPE00 Pci; QEMU_VIDEO_CARD *Card; EFI_PCI_IO_PROTOCOL *ChildPciIo; + EFI_CPU_ARCH_PROTOCOL *Cpu; OldTpl = gBS->RaiseTPL (TPL_CALLBACK); @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || Private->Variant == QEMU_VIDEO_BOCHS) { - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); + // + // Prepare CPU arch protocol for NULL pointer detection + // + Status = gBS->LocateProtocol ( + &gEfiCpuArchProtocolGuid, + NULL, + (VOID **) &Cpu + ); + ASSERT_EFI_ERROR (Status); + + DISABLE_NULL_DETECTION(Cpu); + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); + ENABLE_NULL_DETECTION(Cpu); } #endif diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 --- a/OvmfPkg/QemuVideoDxe/Qemu.h +++ b/OvmfPkg/QemuVideoDxe/Qemu.h @@ -25,6 +25,7 @@ #include <Protocol/PciIo.h> #include <Protocol/DriverSupportedEfiVersion.h> #include <Protocol/DevicePath.h> +#include <Protocol/Cpu.h> #include <Library/DebugLib.h> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ typedef struct { #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff +// +// VBE code will access memory between 0-4095 which will cause page fault exception +// if NULL pointer detection mechanism is enabled. Following macros can be used to +// disable/enable NULL pointer detection before/after accessing those memory. +// +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) +#define DISABLE_NULL_DETECTION(Cpu) \ + if (NULL_DETECTION_ENABLED) { \ + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ + } +#define ENABLE_NULL_DETECTION(Cpu) \ + if (NULL_DETECTION_ENABLED) { \ + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ + } + // // QEMU Video Private Data Structure // diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf index 7c7d429bca..5d166eb99c 100644 --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf @@ -72,7 +72,9 @@ gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START gEfiDevicePathProtocolGuid # PROTOCOL BY_START gEfiPciIoProtocolGuid # PROTOCOL TO_START + gEfiCpuArchProtocolGuid [Pcd] gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask -- 2.14.1.windows.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-13 9:25 ` [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled Wang, Jian J @ 2017-09-13 16:33 ` Johnson, Brian (EXL - Eagan) 2017-09-13 23:34 ` Laszlo Ersek 1 sibling, 0 replies; 21+ messages in thread From: Johnson, Brian (EXL - Eagan) @ 2017-09-13 16:33 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org Cc: Justen@ml01.01.org, Eric Dong, Kinney@ml01.01.org, Jordan L, Wolman@ml01.01.org, Jiewen Yao, Ayellet, Michael D, Laszlo Ersek, Star Zeng Acked-by: Brian J. Johnson <brian.johnson@hpe.com> -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J Sent: Wednesday, September 13, 2017 4:25 AM To: edk2-devel@lists.01.org Cc: Justen@ml01.01.org; Eric Dong <eric.dong@intel.com>; Kinney@ml01.01.org; Jordan L <jordan.l.justen@intel.com>; Wolman@ml01.01.org; Jiewen Yao <jiewen.yao@intel.com>; Ayellet <ayellet.wolman@intel.com>; Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Star Zeng <star.zeng@intel.com> Subject: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer detection is enabled, page 0 must be enabled temporarily before installing and disabled again afterwards. For Windows 7 boot, BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Justen, Jordan L <jordan.l.justen@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com> Cc: Wolman, Ayellet <ayellet.wolman@intel.com> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> --- OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 --- a/OvmfPkg/QemuVideoDxe/Driver.c +++ b/OvmfPkg/QemuVideoDxe/Driver.c @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( PCI_TYPE00 Pci; QEMU_VIDEO_CARD *Card; EFI_PCI_IO_PROTOCOL *ChildPciIo; + EFI_CPU_ARCH_PROTOCOL *Cpu; OldTpl = gBS->RaiseTPL (TPL_CALLBACK); @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || Private->Variant == QEMU_VIDEO_BOCHS) { - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); + // + // Prepare CPU arch protocol for NULL pointer detection + // + Status = gBS->LocateProtocol ( + &gEfiCpuArchProtocolGuid, + NULL, + (VOID **) &Cpu + ); + ASSERT_EFI_ERROR (Status); + + DISABLE_NULL_DETECTION(Cpu); + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); + ENABLE_NULL_DETECTION(Cpu); } #endif diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 --- a/OvmfPkg/QemuVideoDxe/Qemu.h +++ b/OvmfPkg/QemuVideoDxe/Qemu.h @@ -25,6 +25,7 @@ #include <Protocol/PciIo.h> #include <Protocol/DriverSupportedEfiVersion.h> #include <Protocol/DevicePath.h> +#include <Protocol/Cpu.h> #include <Library/DebugLib.h> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ typedef struct { #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff +// +// VBE code will access memory between 0-4095 which will cause page fault exception +// if NULL pointer detection mechanism is enabled. Following macros can be used to +// disable/enable NULL pointer detection before/after accessing those memory. +// +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) +#define DISABLE_NULL_DETECTION(Cpu) \ + if (NULL_DETECTION_ENABLED) { \ + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ + } +#define ENABLE_NULL_DETECTION(Cpu) \ + if (NULL_DETECTION_ENABLED) { \ + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ + } + // // QEMU Video Private Data Structure // diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf index 7c7d429bca..5d166eb99c 100644 --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf @@ -72,7 +72,9 @@ gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START gEfiDevicePathProtocolGuid # PROTOCOL BY_START gEfiPciIoProtocolGuid # PROTOCOL TO_START + gEfiCpuArchProtocolGuid [Pcd] gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask -- 2.14.1.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-13 9:25 ` [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled Wang, Jian J 2017-09-13 16:33 ` Johnson, Brian (EXL - Eagan) @ 2017-09-13 23:34 ` Laszlo Ersek 2017-09-14 1:17 ` Wang, Jian J 1 sibling, 1 reply; 21+ messages in thread From: Laszlo Ersek @ 2017-09-13 23:34 UTC (permalink / raw) To: Jian J Wang Cc: edk2-devel, Jordan Justen (Intel address), Eric Dong, Michael Kinney, Ayellet Wolman, Jiewen Yao, Star Zeng Hi, some of the points I'm going to make have already been pointed out by Jordan: (1) When posting a patch series, please collect the Cc: tags from all of the patches, and add them *all* to the cover letter. This way everyone will get a personal copy of the general description. (2) The subject line is too long. One possible simplification: OvmfPkg/QemuVideoDxe: bypass NULL pointer detection On 09/13/17 11:25, Wang, Jian J wrote: > QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer > detection is enabled, page 0 must be enabled temporarily before > installing and disabled again afterwards. For Windows 7 boot, BIT7 of > PcdNullPointerDetectionPropertyMask must still be set to avoid hang. (3) Subject line and commit message both should not exceed 74 characters line length. (Not sure how many chars PatchCheck.py actually enforces, I always stick with 74, following the Linux kernel tradition.) I rewrapped the commit message here for readability. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Justen, Jordan L <jordan.l.justen@intel.com> > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Wolman, Ayellet <ayellet.wolman@intel.com> > Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> (4) I think this is also something that Jordan had pointed out a long time ago (apologies if I mis-remember): The strings after the tags should form correct email addresses, and if there are various email meta-characters in them, like "." (dot) and "," (comma), then they should be quoted, like this: Cc: "Justen, Jordan L" <jordan.l.justen@intel.com> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com> Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com> Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com> Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com> If you look at the actual addresses on the emails that have been sent out, you can see they are all malformed. For example, I have: "Jordan L <jordan.l.justen@intel.com>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). At least for my reply, I have fixed up the email addresses. (5) The commit message mentions BIT7 of the new PCD. First, thanks for checking Windows 7 boot (and I'm happy that I got suspicious of the feature with regard to Windows 7). I think BIT7 is a good feature. However, please include the short description of that feature in the commit message -- it is one sentence; "Disable NULL pointer detection just after EndOfDxe." (I think BIT7 is a really smart feature, and I like *how* it is used in "NULL_DETECTION_ENABLED" below. The check means, "if the protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; more on that below.) > --- > OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- > OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ > 3 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 0dce80e59b..ee0eed7214 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( > PCI_TYPE00 Pci; > QEMU_VIDEO_CARD *Card; > EFI_PCI_IO_PROTOCOL *ChildPciIo; > + EFI_CPU_ARCH_PROTOCOL *Cpu; (6) I believe I mentioned this in the earlier discussion, in some form, but now I'll say it again: A UEFI driver has no business poking at the CPU Arch protocol. The PI spec (1.6) states, 12.3 CPU Architectural Protocol EFI_CPU_ARCH_PROTOCOL Summary Abstracts the processor services that are required to implement some of the DXE services. This protocol must be produced by a boot service or runtime DXE driver and may only be consumed by the DXE Foundation and DXE drivers that produce architectural protocols. The DXE core is obviously free to use the CPU Arch protocol, but a UEFI driver is forbidden from it, *even if* we say that, in this UEFI driver, we are going to use DXE services. (Which come from the PI spec, and not the UEFI spec.) Therefore, here we have to use gDS->SetMemorySpaceAttributes(). The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch protocol, by the PI spec. It is not easy to see, because the PI spec has a formatting error in this area. If you look under GetMemorySpaceDescriptor(), there is an error code EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU architectural protocol is not available yet. Obviously this error code belongs to SetMemorySpaceAttributes(), not GetMemorySpaceDescriptor(). Anyway, gDS should be used, architectural protocols shouldn't be. > > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( > #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > Private->Variant == QEMU_VIDEO_BOCHS) { > - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > + // > + // Prepare CPU arch protocol for NULL pointer detection > + // > + Status = gBS->LocateProtocol ( > + &gEfiCpuArchProtocolGuid, > + NULL, > + (VOID **) &Cpu > + ); > + ASSERT_EFI_ERROR (Status); > + > + DISABLE_NULL_DETECTION(Cpu); > + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > + ENABLE_NULL_DETECTION(Cpu); (7) The NULL detection disabling and enabling should bracket the affected code as tightly as possible. So please move this into InstallVbeShim() accordingly. > } > #endif > > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index 7fbb25b3ef..bb3bc6eb0f 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -25,6 +25,7 @@ > #include <Protocol/PciIo.h> > #include <Protocol/DriverSupportedEfiVersion.h> > #include <Protocol/DevicePath.h> > +#include <Protocol/Cpu.h> > > #include <Library/DebugLib.h> > #include <Library/UefiDriverEntryPoint.h> > @@ -82,6 +83,21 @@ typedef struct { > > #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff > > +// > +// VBE code will access memory between 0-4095 which will cause page fault exception > +// if NULL pointer detection mechanism is enabled. Following macros can be used to > +// disable/enable NULL pointer detection before/after accessing those memory. > +// > +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) > +#define DISABLE_NULL_DETECTION(Cpu) \ > + if (NULL_DETECTION_ENABLED) { \ > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ > + } > +#define ENABLE_NULL_DETECTION(Cpu) \ > + if (NULL_DETECTION_ENABLED) { \ > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ > + } > + (8) I believe Jordan too commented on these macros elsewhere (under patch 1/4). In my opinion, this functionality should be extracted into a library class, with a library instance that is suitable for at least UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) You could add a separate library instance for SMM drivers, if that were necessary. (9) Style comment: please put one space character between the function designator and the opening parenthesis. > // > // QEMU Video Private Data Structure > // > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > index 7c7d429bca..5d166eb99c 100644 > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > @@ -72,7 +72,9 @@ > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > gEfiPciIoProtocolGuid # PROTOCOL TO_START > + gEfiCpuArchProtocolGuid > > [Pcd] > gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask (10) Instead of these, the library class that I described under (8) should be added here. Any further dependencies like PCDs, protocols etc should be inherited by the driver through the library instance that the platform DSC file resolves the library class to. Bonus: should you realize that the feature is impossible to implement without accessing the CPU Arch protocol directly, you could hide the protocol GUID dependency in the library instance INF file, and I'd be none the wiser. ... Well, I could at least pretend that. :) Thanks, Laszlo ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-13 23:34 ` Laszlo Ersek @ 2017-09-14 1:17 ` Wang, Jian J 2017-09-14 3:17 ` Wang, Jian J ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Wang, Jian J @ 2017-09-14 1:17 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Justen, Jordan L, Dong, Eric, Kinney, Michael D, Wolman, Ayellet, Yao, Jiewen, Zeng, Star Thanks for the comments and good advices. Sorry the format issues. This is my first patch for this project. Too many details for me to get familiar with. (1) Sure. (2) I'll change that. (3) I'll use the tool to ensure the patch format. (4) I'll remove the ',' in name (5) I'll add more description about it. (6) You're right. I should use SetMemorySpaceAttributes() of DXE service instead. The only reason I didn't do it is that I found GetMemorySpaceDescriptor() doesn't return the same information which SetMemorySpaceAttributes() just changed. So I feel using CPU arch protocol is a bit safer. Anyway, I'll change it. (7) I did put those macros in the install function before. To reduce the number of changed files, I made current changes. You're right it's not worthy. (8) Using macro can help the readability, which is more important to me. I know function can do the same. But it looks a bit heavy in this situation. I have to admit replacing the macros with a library is a very good idea, which brings the same readability. I didn't think of that before. Although Library is still a little bit heavy to me but it's in a different way, I think it worth a trying. (9) Putting a space before open parenthesis is forced style? If so, I'll add it. (10) You're right. Using library can reduce the disturbs to affected drivers by this feature to the minimum. -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, September 14, 2017 7:35 AM To: Wang, Jian J <jian.j.wang@intel.com> Cc: edk2-devel@lists.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. Hi, some of the points I'm going to make have already been pointed out by Jordan: (1) When posting a patch series, please collect the Cc: tags from all of the patches, and add them *all* to the cover letter. This way everyone will get a personal copy of the general description. (2) The subject line is too long. One possible simplification: OvmfPkg/QemuVideoDxe: bypass NULL pointer detection On 09/13/17 11:25, Wang, Jian J wrote: > QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer > detection is enabled, page 0 must be enabled temporarily before > installing and disabled again afterwards. For Windows 7 boot, BIT7 of > PcdNullPointerDetectionPropertyMask must still be set to avoid hang. (3) Subject line and commit message both should not exceed 74 characters line length. (Not sure how many chars PatchCheck.py actually enforces, I always stick with 74, following the Linux kernel tradition.) I rewrapped the commit message here for readability. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Justen, Jordan L <jordan.l.justen@intel.com> > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Wolman, Ayellet <ayellet.wolman@intel.com> > Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> (4) I think this is also something that Jordan had pointed out a long time ago (apologies if I mis-remember): The strings after the tags should form correct email addresses, and if there are various email meta-characters in them, like "." (dot) and "," (comma), then they should be quoted, like this: Cc: "Justen, Jordan L" <jordan.l.justen@intel.com> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com> Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com> Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com> Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com> If you look at the actual addresses on the emails that have been sent out, you can see they are all malformed. For example, I have: "Jordan L <jordan.l.justen@intel.com>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). At least for my reply, I have fixed up the email addresses. (5) The commit message mentions BIT7 of the new PCD. First, thanks for checking Windows 7 boot (and I'm happy that I got suspicious of the feature with regard to Windows 7). I think BIT7 is a good feature. However, please include the short description of that feature in the commit message -- it is one sentence; "Disable NULL pointer detection just after EndOfDxe." (I think BIT7 is a really smart feature, and I like *how* it is used in "NULL_DETECTION_ENABLED" below. The check means, "if the protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; more on that below.) > --- > OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- > OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ > 3 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 0dce80e59b..ee0eed7214 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( > PCI_TYPE00 Pci; > QEMU_VIDEO_CARD *Card; > EFI_PCI_IO_PROTOCOL *ChildPciIo; > + EFI_CPU_ARCH_PROTOCOL *Cpu; (6) I believe I mentioned this in the earlier discussion, in some form, but now I'll say it again: A UEFI driver has no business poking at the CPU Arch protocol. The PI spec (1.6) states, 12.3 CPU Architectural Protocol EFI_CPU_ARCH_PROTOCOL Summary Abstracts the processor services that are required to implement some of the DXE services. This protocol must be produced by a boot service or runtime DXE driver and may only be consumed by the DXE Foundation and DXE drivers that produce architectural protocols. The DXE core is obviously free to use the CPU Arch protocol, but a UEFI driver is forbidden from it, *even if* we say that, in this UEFI driver, we are going to use DXE services. (Which come from the PI spec, and not the UEFI spec.) Therefore, here we have to use gDS->SetMemorySpaceAttributes(). The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch protocol, by the PI spec. It is not easy to see, because the PI spec has a formatting error in this area. If you look under GetMemorySpaceDescriptor(), there is an error code EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU architectural protocol is not available yet. Obviously this error code belongs to SetMemorySpaceAttributes(), not GetMemorySpaceDescriptor(). Anyway, gDS should be used, architectural protocols shouldn't be. > > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( > #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > Private->Variant == QEMU_VIDEO_BOCHS) { > - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > + // > + // Prepare CPU arch protocol for NULL pointer detection > + // > + Status = gBS->LocateProtocol ( > + &gEfiCpuArchProtocolGuid, > + NULL, > + (VOID **) &Cpu > + ); > + ASSERT_EFI_ERROR (Status); > + > + DISABLE_NULL_DETECTION(Cpu); > + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > + ENABLE_NULL_DETECTION(Cpu); (7) The NULL detection disabling and enabling should bracket the affected code as tightly as possible. So please move this into InstallVbeShim() accordingly. > } > #endif > > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index 7fbb25b3ef..bb3bc6eb0f 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -25,6 +25,7 @@ > #include <Protocol/PciIo.h> > #include <Protocol/DriverSupportedEfiVersion.h> > #include <Protocol/DevicePath.h> > +#include <Protocol/Cpu.h> > > #include <Library/DebugLib.h> > #include <Library/UefiDriverEntryPoint.h> > @@ -82,6 +83,21 @@ typedef struct { > > #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff > > +// > +// VBE code will access memory between 0-4095 which will cause page fault exception > +// if NULL pointer detection mechanism is enabled. Following macros can be used to > +// disable/enable NULL pointer detection before/after accessing those memory. > +// > +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) > +#define DISABLE_NULL_DETECTION(Cpu) \ > + if (NULL_DETECTION_ENABLED) { \ > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ > + } > +#define ENABLE_NULL_DETECTION(Cpu) \ > + if (NULL_DETECTION_ENABLED) { \ > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ > + } > + (8) I believe Jordan too commented on these macros elsewhere (under patch 1/4). In my opinion, this functionality should be extracted into a library class, with a library instance that is suitable for at least UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) You could add a separate library instance for SMM drivers, if that were necessary. (9) Style comment: please put one space character between the function designator and the opening parenthesis. > // > // QEMU Video Private Data Structure > // > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > index 7c7d429bca..5d166eb99c 100644 > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > @@ -72,7 +72,9 @@ > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > gEfiPciIoProtocolGuid # PROTOCOL TO_START > + gEfiCpuArchProtocolGuid > > [Pcd] > gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask (10) Instead of these, the library class that I described under (8) should be added here. Any further dependencies like PCDs, protocols etc should be inherited by the driver through the library instance that the platform DSC file resolves the library class to. Bonus: should you realize that the feature is impossible to implement without accessing the CPU Arch protocol directly, you could hide the protocol GUID dependency in the library instance INF file, and I'd be none the wiser. ... Well, I could at least pretend that. :) Thanks, Laszlo ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 1:17 ` Wang, Jian J @ 2017-09-14 3:17 ` Wang, Jian J 2017-09-14 8:30 ` Laszlo Ersek 2017-09-14 5:50 ` Jordan Justen 2017-09-14 8:26 ` Laszlo Ersek 2 siblings, 1 reply; 21+ messages in thread From: Wang, Jian J @ 2017-09-14 3:17 UTC (permalink / raw) To: Laszlo Ersek Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Yao, Jiewen, Wolman, Ayellet, Kinney, Michael D, Zeng, Star For the use of arch protocol, there's one thing I mentioned is not accurate. I actually tried gDS->SetMemorySpaceAttributes() but it cannot change page attributes. That's why I have to turn to cpu arch protocol. -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J Sent: Thursday, September 14, 2017 9:17 AM To: Laszlo Ersek <lersek@redhat.com> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. Thanks for the comments and good advices. Sorry the format issues. This is my first patch for this project. Too many details for me to get familiar with. (1) Sure. (2) I'll change that. (3) I'll use the tool to ensure the patch format. (4) I'll remove the ',' in name (5) I'll add more description about it. (6) You're right. I should use SetMemorySpaceAttributes() of DXE service instead. The only reason I didn't do it is that I found GetMemorySpaceDescriptor() doesn't return the same information which SetMemorySpaceAttributes() just changed. So I feel using CPU arch protocol is a bit safer. Anyway, I'll change it. (7) I did put those macros in the install function before. To reduce the number of changed files, I made current changes. You're right it's not worthy. (8) Using macro can help the readability, which is more important to me. I know function can do the same. But it looks a bit heavy in this situation. I have to admit replacing the macros with a library is a very good idea, which brings the same readability. I didn't think of that before. Although Library is still a little bit heavy to me but it's in a different way, I think it worth a trying. (9) Putting a space before open parenthesis is forced style? If so, I'll add it. (10) You're right. Using library can reduce the disturbs to affected drivers by this feature to the minimum. -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, September 14, 2017 7:35 AM To: Wang, Jian J <jian.j.wang@intel.com> Cc: edk2-devel@lists.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. Hi, some of the points I'm going to make have already been pointed out by Jordan: (1) When posting a patch series, please collect the Cc: tags from all of the patches, and add them *all* to the cover letter. This way everyone will get a personal copy of the general description. (2) The subject line is too long. One possible simplification: OvmfPkg/QemuVideoDxe: bypass NULL pointer detection On 09/13/17 11:25, Wang, Jian J wrote: > QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer > detection is enabled, page 0 must be enabled temporarily before > installing and disabled again afterwards. For Windows 7 boot, BIT7 of > PcdNullPointerDetectionPropertyMask must still be set to avoid hang. (3) Subject line and commit message both should not exceed 74 characters line length. (Not sure how many chars PatchCheck.py actually enforces, I always stick with 74, following the Linux kernel tradition.) I rewrapped the commit message here for readability. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Justen, Jordan L <jordan.l.justen@intel.com> > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Wolman, Ayellet <ayellet.wolman@intel.com> > Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> (4) I think this is also something that Jordan had pointed out a long time ago (apologies if I mis-remember): The strings after the tags should form correct email addresses, and if there are various email meta-characters in them, like "." (dot) and "," (comma), then they should be quoted, like this: Cc: "Justen, Jordan L" <jordan.l.justen@intel.com> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com> Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com> Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com> Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com> If you look at the actual addresses on the emails that have been sent out, you can see they are all malformed. For example, I have: "Jordan L <jordan.l.justen@intel.com>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). At least for my reply, I have fixed up the email addresses. (5) The commit message mentions BIT7 of the new PCD. First, thanks for checking Windows 7 boot (and I'm happy that I got suspicious of the feature with regard to Windows 7). I think BIT7 is a good feature. However, please include the short description of that feature in the commit message -- it is one sentence; "Disable NULL pointer detection just after EndOfDxe." (I think BIT7 is a really smart feature, and I like *how* it is used in "NULL_DETECTION_ENABLED" below. The check means, "if the protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; more on that below.) > --- > OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- > OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ > 3 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 0dce80e59b..ee0eed7214 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( > PCI_TYPE00 Pci; > QEMU_VIDEO_CARD *Card; > EFI_PCI_IO_PROTOCOL *ChildPciIo; > + EFI_CPU_ARCH_PROTOCOL *Cpu; (6) I believe I mentioned this in the earlier discussion, in some form, but now I'll say it again: A UEFI driver has no business poking at the CPU Arch protocol. The PI spec (1.6) states, 12.3 CPU Architectural Protocol EFI_CPU_ARCH_PROTOCOL Summary Abstracts the processor services that are required to implement some of the DXE services. This protocol must be produced by a boot service or runtime DXE driver and may only be consumed by the DXE Foundation and DXE drivers that produce architectural protocols. The DXE core is obviously free to use the CPU Arch protocol, but a UEFI driver is forbidden from it, *even if* we say that, in this UEFI driver, we are going to use DXE services. (Which come from the PI spec, and not the UEFI spec.) Therefore, here we have to use gDS->SetMemorySpaceAttributes(). The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch protocol, by the PI spec. It is not easy to see, because the PI spec has a formatting error in this area. If you look under GetMemorySpaceDescriptor(), there is an error code EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU architectural protocol is not available yet. Obviously this error code belongs to SetMemorySpaceAttributes(), not GetMemorySpaceDescriptor(). Anyway, gDS should be used, architectural protocols shouldn't be. > > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( > #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > Private->Variant == QEMU_VIDEO_BOCHS) { > - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > + // > + // Prepare CPU arch protocol for NULL pointer detection > + // > + Status = gBS->LocateProtocol ( > + &gEfiCpuArchProtocolGuid, > + NULL, > + (VOID **) &Cpu > + ); > + ASSERT_EFI_ERROR (Status); > + > + DISABLE_NULL_DETECTION(Cpu); > + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > + ENABLE_NULL_DETECTION(Cpu); (7) The NULL detection disabling and enabling should bracket the affected code as tightly as possible. So please move this into InstallVbeShim() accordingly. > } > #endif > > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index 7fbb25b3ef..bb3bc6eb0f 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -25,6 +25,7 @@ > #include <Protocol/PciIo.h> > #include <Protocol/DriverSupportedEfiVersion.h> > #include <Protocol/DevicePath.h> > +#include <Protocol/Cpu.h> > > #include <Library/DebugLib.h> > #include <Library/UefiDriverEntryPoint.h> > @@ -82,6 +83,21 @@ typedef struct { > > #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff > > +// > +// VBE code will access memory between 0-4095 which will cause page fault exception > +// if NULL pointer detection mechanism is enabled. Following macros can be used to > +// disable/enable NULL pointer detection before/after accessing those memory. > +// > +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) > +#define DISABLE_NULL_DETECTION(Cpu) \ > + if (NULL_DETECTION_ENABLED) { \ > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ > + } > +#define ENABLE_NULL_DETECTION(Cpu) \ > + if (NULL_DETECTION_ENABLED) { \ > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ > + } > + (8) I believe Jordan too commented on these macros elsewhere (under patch 1/4). In my opinion, this functionality should be extracted into a library class, with a library instance that is suitable for at least UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) You could add a separate library instance for SMM drivers, if that were necessary. (9) Style comment: please put one space character between the function designator and the opening parenthesis. > // > // QEMU Video Private Data Structure > // > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > index 7c7d429bca..5d166eb99c 100644 > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > @@ -72,7 +72,9 @@ > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > gEfiPciIoProtocolGuid # PROTOCOL TO_START > + gEfiCpuArchProtocolGuid > > [Pcd] > gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask (10) Instead of these, the library class that I described under (8) should be added here. Any further dependencies like PCDs, protocols etc should be inherited by the driver through the library instance that the platform DSC file resolves the library class to. Bonus: should you realize that the feature is impossible to implement without accessing the CPU Arch protocol directly, you could hide the protocol GUID dependency in the library instance INF file, and I'd be none the wiser. ... Well, I could at least pretend that. :) Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 3:17 ` Wang, Jian J @ 2017-09-14 8:30 ` Laszlo Ersek 2017-09-14 8:38 ` Yao, Jiewen 0 siblings, 1 reply; 21+ messages in thread From: Laszlo Ersek @ 2017-09-14 8:30 UTC (permalink / raw) To: Wang, Jian J Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Yao, Jiewen, Wolman, Ayellet, Kinney, Michael D, Zeng, Star On 09/14/17 05:17, Wang, Jian J wrote: > For the use of arch protocol, there's one thing I mentioned is not > accurate. I actually tried gDS->SetMemorySpaceAttributes() but it > cannot change page attributes. That's why I have to turn to cpu arch > protocol. Thank you for the explanation. In that case, we cannot avoid violating the PI spec. I agree we can break the spec if it happens for security purposes, but in that case, I believe that hiding the implementation behind a library class is mandatory. Viewed from the spec side, accessing the CPU Arch protocol is a hack, and so it should not be spread to the source code of several driver modules. Thank you, Laszlo > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J > Sent: Thursday, September 14, 2017 9:17 AM > To: Laszlo Ersek <lersek@redhat.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum. > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 7:35 AM > To: Wang, Jian J <jian.j.wang@intel.com> > Cc: edk2-devel@lists.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Hi, > > some of the points I'm going to make have already been pointed out by > Jordan: > > (1) When posting a patch series, please collect the Cc: tags from all of > the patches, and add them *all* to the cover letter. This way everyone > will get a personal copy of the general description. > > > (2) The subject line is too long. One possible simplification: > > OvmfPkg/QemuVideoDxe: bypass NULL pointer detection > > > On 09/13/17 11:25, Wang, Jian J wrote: >> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer >> detection is enabled, page 0 must be enabled temporarily before >> installing and disabled again afterwards. For Windows 7 boot, BIT7 of >> PcdNullPointerDetectionPropertyMask must still be set to avoid hang. > > (3) Subject line and commit message both should not exceed 74 characters > line length. (Not sure how many chars PatchCheck.py actually enforces, I > always stick with 74, following the Linux kernel tradition.) > > I rewrapped the commit message here for readability. > > >> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Eric Dong <eric.dong@intel.com> >> Cc: Star Zeng <star.zeng@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Justen, Jordan L <jordan.l.justen@intel.com> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com> >> Cc: Wolman, Ayellet <ayellet.wolman@intel.com> >> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> > > (4) I think this is also something that Jordan had pointed out a long > time ago (apologies if I mis-remember): > > The strings after the tags should form correct email addresses, and if > there are various email meta-characters in them, like "." (dot) and "," > (comma), then they should be quoted, like this: > > Cc: "Justen, Jordan L" <jordan.l.justen@intel.com> > Cc: "Kinney, Michael D" <michael.d.kinney@intel.com> > Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com> > Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com> > Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com> > > If you look at the actual addresses on the emails that have been sent > out, you can see they are all malformed. For example, I have: > > "Jordan L <jordan.l.justen@intel.com>" for Jordan -- the part before the > comma was taken to be a separate email address (a malformed one). > > At least for my reply, I have fixed up the email addresses. > > > (5) The commit message mentions BIT7 of the new PCD. > > First, thanks for checking Windows 7 boot (and I'm happy that I got > suspicious of the feature with regard to Windows 7). I think BIT7 is a > good feature. > > However, please include the short description of that feature in the > commit message -- it is one sentence; "Disable NULL pointer detection > just after EndOfDxe." > > (I think BIT7 is a really smart feature, and I like *how* it is used in > "NULL_DETECTION_ENABLED" below. The check means, "if the protection is > enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". > > This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; > more on that below.) > > >> --- >> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >> 3 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c >> index 0dce80e59b..ee0eed7214 100644 >> --- a/OvmfPkg/QemuVideoDxe/Driver.c >> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >> PCI_TYPE00 Pci; >> QEMU_VIDEO_CARD *Card; >> EFI_PCI_IO_PROTOCOL *ChildPciIo; >> + EFI_CPU_ARCH_PROTOCOL *Cpu; > > (6) I believe I mentioned this in the earlier discussion, in some form, > but now I'll say it again: > > A UEFI driver has no business poking at the CPU Arch protocol. The PI > spec (1.6) states, > > 12.3 CPU Architectural Protocol > EFI_CPU_ARCH_PROTOCOL > > Summary > > Abstracts the processor services that are required to implement some > of the DXE services. This protocol must be produced by a boot service > or runtime DXE driver and may only be consumed by the DXE Foundation > and DXE drivers that produce architectural protocols. > > The DXE core is obviously free to use the CPU Arch protocol, but a UEFI > driver is forbidden from it, *even if* we say that, in this UEFI driver, > we are going to use DXE services. (Which come from the PI spec, and not > the UEFI spec.) > > Therefore, here we have to use gDS->SetMemorySpaceAttributes(). > > The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch > protocol, by the PI spec. It is not easy to see, because the PI spec has > a formatting error in this area. If you look under > GetMemorySpaceDescriptor(), there is an error code > > EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU > architectural protocol is not available yet. > > Obviously this error code belongs to SetMemorySpaceAttributes(), not > GetMemorySpaceDescriptor(). > > Anyway, gDS should be used, architectural protocols shouldn't be. > > >> >> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >> >> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( >> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 >> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >> Private->Variant == QEMU_VIDEO_BOCHS) { >> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + // >> + // Prepare CPU arch protocol for NULL pointer detection >> + // >> + Status = gBS->LocateProtocol ( >> + &gEfiCpuArchProtocolGuid, >> + NULL, >> + (VOID **) &Cpu >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> + DISABLE_NULL_DETECTION(Cpu); >> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + ENABLE_NULL_DETECTION(Cpu); > > (7) The NULL detection disabling and enabling should bracket the > affected code as tightly as possible. > > So please move this into InstallVbeShim() accordingly. > > >> } >> #endif >> >> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h >> index 7fbb25b3ef..bb3bc6eb0f 100644 >> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >> @@ -25,6 +25,7 @@ >> #include <Protocol/PciIo.h> >> #include <Protocol/DriverSupportedEfiVersion.h> >> #include <Protocol/DevicePath.h> >> +#include <Protocol/Cpu.h> >> >> #include <Library/DebugLib.h> >> #include <Library/UefiDriverEntryPoint.h> >> @@ -82,6 +83,21 @@ typedef struct { >> >> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >> >> +// >> +// VBE code will access memory between 0-4095 which will cause page fault exception >> +// if NULL pointer detection mechanism is enabled. Following macros can be used to >> +// disable/enable NULL pointer detection before/after accessing those memory. >> +// >> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >> +#define DISABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >> + } >> +#define ENABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >> + } >> + > > (8) I believe Jordan too commented on these macros elsewhere (under > patch 1/4). > > In my opinion, this functionality should be extracted into a library > class, with a library instance that is suitable for at least UEFI_DRIVER > modules. (Maybe even for DXE_DRIVER modules.) > > You could add a separate library instance for SMM drivers, if that were > necessary. > > > (9) Style comment: please put one space character between the function > designator and the opening parenthesis. > > >> // >> // QEMU Video Private Data Structure >> // >> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> index 7c7d429bca..5d166eb99c 100644 >> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> @@ -72,7 +72,9 @@ >> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >> gEfiPciIoProtocolGuid # PROTOCOL TO_START >> + gEfiCpuArchProtocolGuid >> >> [Pcd] >> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > > (10) Instead of these, the library class that I described under (8) > should be added here. > > Any further dependencies like PCDs, protocols etc should be inherited by > the driver through the library instance that the platform DSC file > resolves the library class to. > > Bonus: should you realize that the feature is impossible to implement > without accessing the CPU Arch protocol directly, you could hide the > protocol GUID dependency in the library instance INF file, and I'd be > none the wiser. > > ... Well, I could at least pretend that. :) > > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 8:30 ` Laszlo Ersek @ 2017-09-14 8:38 ` Yao, Jiewen 2017-09-14 8:46 ` Wang, Jian J 2017-09-14 8:52 ` Laszlo Ersek 0 siblings, 2 replies; 21+ messages in thread From: Yao, Jiewen @ 2017-09-14 8:38 UTC (permalink / raw) To: Laszlo Ersek, Wang, Jian J Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Wolman, Ayellet, Kinney, Michael D, Zeng, Star HI I wonder if it is spec limitation or implementation limitation. If it is implementation limitation, we can enhance the DxeCore to allow it. Thank you Yao Jiewen From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, September 14, 2017 4:30 PM To: Wang, Jian J <jian.j.wang@intel.com> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 05:17, Wang, Jian J wrote: > For the use of arch protocol, there's one thing I mentioned is not > accurate. I actually tried gDS->SetMemorySpaceAttributes() but it > cannot change page attributes. That's why I have to turn to cpu arch > protocol. Thank you for the explanation. In that case, we cannot avoid violating the PI spec. I agree we can break the spec if it happens for security purposes, but in that case, I believe that hiding the implementation behind a library class is mandatory. Viewed from the spec side, accessing the CPU Arch protocol is a hack, and so it should not be spread to the source code of several driver modules. Thank you, Laszlo > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J > Sent: Thursday, September 14, 2017 9:17 AM > To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> > Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum. > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 7:35 AM > To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Hi, > > some of the points I'm going to make have already been pointed out by > Jordan: > > (1) When posting a patch series, please collect the Cc: tags from all of > the patches, and add them *all* to the cover letter. This way everyone > will get a personal copy of the general description. > > > (2) The subject line is too long. One possible simplification: > > OvmfPkg/QemuVideoDxe: bypass NULL pointer detection > > > On 09/13/17 11:25, Wang, Jian J wrote: >> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer >> detection is enabled, page 0 must be enabled temporarily before >> installing and disabled again afterwards. For Windows 7 boot, BIT7 of >> PcdNullPointerDetectionPropertyMask must still be set to avoid hang. > > (3) Subject line and commit message both should not exceed 74 characters > line length. (Not sure how many chars PatchCheck.py actually enforces, I > always stick with 74, following the Linux kernel tradition.) > > I rewrapped the commit message here for readability. > > >> >> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >> Cc: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > (4) I think this is also something that Jordan had pointed out a long > time ago (apologies if I mis-remember): > > The strings after the tags should form correct email addresses, and if > there are various email meta-characters in them, like "." (dot) and "," > (comma), then they should be quoted, like this: > > Cc: "Justen, Jordan L" <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> > Cc: "Kinney, Michael D" <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> > Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> > Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> > Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > If you look at the actual addresses on the emails that have been sent > out, you can see they are all malformed. For example, I have: > > "Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the > comma was taken to be a separate email address (a malformed one). > > At least for my reply, I have fixed up the email addresses. > > > (5) The commit message mentions BIT7 of the new PCD. > > First, thanks for checking Windows 7 boot (and I'm happy that I got > suspicious of the feature with regard to Windows 7). I think BIT7 is a > good feature. > > However, please include the short description of that feature in the > commit message -- it is one sentence; "Disable NULL pointer detection > just after EndOfDxe." > > (I think BIT7 is a really smart feature, and I like *how* it is used in > "NULL_DETECTION_ENABLED" below. The check means, "if the protection is > enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". > > This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; > more on that below.) > > >> --- >> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >> 3 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c >> index 0dce80e59b..ee0eed7214 100644 >> --- a/OvmfPkg/QemuVideoDxe/Driver.c >> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >> PCI_TYPE00 Pci; >> QEMU_VIDEO_CARD *Card; >> EFI_PCI_IO_PROTOCOL *ChildPciIo; >> + EFI_CPU_ARCH_PROTOCOL *Cpu; > > (6) I believe I mentioned this in the earlier discussion, in some form, > but now I'll say it again: > > A UEFI driver has no business poking at the CPU Arch protocol. The PI > spec (1.6) states, > > 12.3 CPU Architectural Protocol > EFI_CPU_ARCH_PROTOCOL > > Summary > > Abstracts the processor services that are required to implement some > of the DXE services. This protocol must be produced by a boot service > or runtime DXE driver and may only be consumed by the DXE Foundation > and DXE drivers that produce architectural protocols. > > The DXE core is obviously free to use the CPU Arch protocol, but a UEFI > driver is forbidden from it, *even if* we say that, in this UEFI driver, > we are going to use DXE services. (Which come from the PI spec, and not > the UEFI spec.) > > Therefore, here we have to use gDS->SetMemorySpaceAttributes(). > > The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch > protocol, by the PI spec. It is not easy to see, because the PI spec has > a formatting error in this area. If you look under > GetMemorySpaceDescriptor(), there is an error code > > EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU > architectural protocol is not available yet. > > Obviously this error code belongs to SetMemorySpaceAttributes(), not > GetMemorySpaceDescriptor(). > > Anyway, gDS should be used, architectural protocols shouldn't be. > > >> >> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >> >> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( >> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 >> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >> Private->Variant == QEMU_VIDEO_BOCHS) { >> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + // >> + // Prepare CPU arch protocol for NULL pointer detection >> + // >> + Status = gBS->LocateProtocol ( >> + &gEfiCpuArchProtocolGuid, >> + NULL, >> + (VOID **) &Cpu >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> + DISABLE_NULL_DETECTION(Cpu); >> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + ENABLE_NULL_DETECTION(Cpu); > > (7) The NULL detection disabling and enabling should bracket the > affected code as tightly as possible. > > So please move this into InstallVbeShim() accordingly. > > >> } >> #endif >> >> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h >> index 7fbb25b3ef..bb3bc6eb0f 100644 >> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >> @@ -25,6 +25,7 @@ >> #include <Protocol/PciIo.h> >> #include <Protocol/DriverSupportedEfiVersion.h> >> #include <Protocol/DevicePath.h> >> +#include <Protocol/Cpu.h> >> >> #include <Library/DebugLib.h> >> #include <Library/UefiDriverEntryPoint.h> >> @@ -82,6 +83,21 @@ typedef struct { >> >> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >> >> +// >> +// VBE code will access memory between 0-4095 which will cause page fault exception >> +// if NULL pointer detection mechanism is enabled. Following macros can be used to >> +// disable/enable NULL pointer detection before/after accessing those memory. >> +// >> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >> +#define DISABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >> + } >> +#define ENABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >> + } >> + > > (8) I believe Jordan too commented on these macros elsewhere (under > patch 1/4). > > In my opinion, this functionality should be extracted into a library > class, with a library instance that is suitable for at least UEFI_DRIVER > modules. (Maybe even for DXE_DRIVER modules.) > > You could add a separate library instance for SMM drivers, if that were > necessary. > > > (9) Style comment: please put one space character between the function > designator and the opening parenthesis. > > >> // >> // QEMU Video Private Data Structure >> // >> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> index 7c7d429bca..5d166eb99c 100644 >> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> @@ -72,7 +72,9 @@ >> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >> gEfiPciIoProtocolGuid # PROTOCOL TO_START >> + gEfiCpuArchProtocolGuid >> >> [Pcd] >> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > > (10) Instead of these, the library class that I described under (8) > should be added here. > > Any further dependencies like PCDs, protocols etc should be inherited by > the driver through the library instance that the platform DSC file > resolves the library class to. > > Bonus: should you realize that the feature is impossible to implement > without accessing the CPU Arch protocol directly, you could hide the > protocol GUID dependency in the library instance INF file, and I'd be > none the wiser. > > ... Well, I could at least pretend that. :) > > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 8:38 ` Yao, Jiewen @ 2017-09-14 8:46 ` Wang, Jian J 2017-09-14 8:48 ` Yao, Jiewen 2017-09-14 8:54 ` Laszlo Ersek 2017-09-14 8:52 ` Laszlo Ersek 1 sibling, 2 replies; 21+ messages in thread From: Wang, Jian J @ 2017-09-14 8:46 UTC (permalink / raw) To: Yao, Jiewen, Laszlo Ersek Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Wolman, Ayellet, Kinney, Michael D, Zeng, Star It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). From: Yao, Jiewen Sent: Thursday, September 14, 2017 4:38 PM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. HI I wonder if it is spec limitation or implementation limitation. If it is implementation limitation, we can enhance the DxeCore to allow it. Thank you Yao Jiewen From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, September 14, 2017 4:30 PM To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 05:17, Wang, Jian J wrote: > For the use of arch protocol, there's one thing I mentioned is not > accurate. I actually tried gDS->SetMemorySpaceAttributes() but it > cannot change page attributes. That's why I have to turn to cpu arch > protocol. Thank you for the explanation. In that case, we cannot avoid violating the PI spec. I agree we can break the spec if it happens for security purposes, but in that case, I believe that hiding the implementation behind a library class is mandatory. Viewed from the spec side, accessing the CPU Arch protocol is a hack, and so it should not be spread to the source code of several driver modules. Thank you, Laszlo > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J > Sent: Thursday, September 14, 2017 9:17 AM > To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> > Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum. > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 7:35 AM > To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Hi, > > some of the points I'm going to make have already been pointed out by > Jordan: > > (1) When posting a patch series, please collect the Cc: tags from all of > the patches, and add them *all* to the cover letter. This way everyone > will get a personal copy of the general description. > > > (2) The subject line is too long. One possible simplification: > > OvmfPkg/QemuVideoDxe: bypass NULL pointer detection > > > On 09/13/17 11:25, Wang, Jian J wrote: >> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer >> detection is enabled, page 0 must be enabled temporarily before >> installing and disabled again afterwards. For Windows 7 boot, BIT7 of >> PcdNullPointerDetectionPropertyMask must still be set to avoid hang. > > (3) Subject line and commit message both should not exceed 74 characters > line length. (Not sure how many chars PatchCheck.py actually enforces, I > always stick with 74, following the Linux kernel tradition.) > > I rewrapped the commit message here for readability. > > >> >> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >> Cc: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > (4) I think this is also something that Jordan had pointed out a long > time ago (apologies if I mis-remember): > > The strings after the tags should form correct email addresses, and if > there are various email meta-characters in them, like "." (dot) and "," > (comma), then they should be quoted, like this: > > Cc: "Justen, Jordan L" <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> > Cc: "Kinney, Michael D" <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> > Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> > Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> > Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > If you look at the actual addresses on the emails that have been sent > out, you can see they are all malformed. For example, I have: > > "Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the > comma was taken to be a separate email address (a malformed one). > > At least for my reply, I have fixed up the email addresses. > > > (5) The commit message mentions BIT7 of the new PCD. > > First, thanks for checking Windows 7 boot (and I'm happy that I got > suspicious of the feature with regard to Windows 7). I think BIT7 is a > good feature. > > However, please include the short description of that feature in the > commit message -- it is one sentence; "Disable NULL pointer detection > just after EndOfDxe." > > (I think BIT7 is a really smart feature, and I like *how* it is used in > "NULL_DETECTION_ENABLED" below. The check means, "if the protection is > enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". > > This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; > more on that below.) > > >> --- >> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >> 3 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c >> index 0dce80e59b..ee0eed7214 100644 >> --- a/OvmfPkg/QemuVideoDxe/Driver.c >> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >> PCI_TYPE00 Pci; >> QEMU_VIDEO_CARD *Card; >> EFI_PCI_IO_PROTOCOL *ChildPciIo; >> + EFI_CPU_ARCH_PROTOCOL *Cpu; > > (6) I believe I mentioned this in the earlier discussion, in some form, > but now I'll say it again: > > A UEFI driver has no business poking at the CPU Arch protocol. The PI > spec (1.6) states, > > 12.3 CPU Architectural Protocol > EFI_CPU_ARCH_PROTOCOL > > Summary > > Abstracts the processor services that are required to implement some > of the DXE services. This protocol must be produced by a boot service > or runtime DXE driver and may only be consumed by the DXE Foundation > and DXE drivers that produce architectural protocols. > > The DXE core is obviously free to use the CPU Arch protocol, but a UEFI > driver is forbidden from it, *even if* we say that, in this UEFI driver, > we are going to use DXE services. (Which come from the PI spec, and not > the UEFI spec.) > > Therefore, here we have to use gDS->SetMemorySpaceAttributes(). > > The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch > protocol, by the PI spec. It is not easy to see, because the PI spec has > a formatting error in this area. If you look under > GetMemorySpaceDescriptor(), there is an error code > > EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU > architectural protocol is not available yet. > > Obviously this error code belongs to SetMemorySpaceAttributes(), not > GetMemorySpaceDescriptor(). > > Anyway, gDS should be used, architectural protocols shouldn't be. > > >> >> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >> >> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( >> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 >> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >> Private->Variant == QEMU_VIDEO_BOCHS) { >> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + // >> + // Prepare CPU arch protocol for NULL pointer detection >> + // >> + Status = gBS->LocateProtocol ( >> + &gEfiCpuArchProtocolGuid, >> + NULL, >> + (VOID **) &Cpu >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> + DISABLE_NULL_DETECTION(Cpu); >> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + ENABLE_NULL_DETECTION(Cpu); > > (7) The NULL detection disabling and enabling should bracket the > affected code as tightly as possible. > > So please move this into InstallVbeShim() accordingly. > > >> } >> #endif >> >> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h >> index 7fbb25b3ef..bb3bc6eb0f 100644 >> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >> @@ -25,6 +25,7 @@ >> #include <Protocol/PciIo.h> >> #include <Protocol/DriverSupportedEfiVersion.h> >> #include <Protocol/DevicePath.h> >> +#include <Protocol/Cpu.h> >> >> #include <Library/DebugLib.h> >> #include <Library/UefiDriverEntryPoint.h> >> @@ -82,6 +83,21 @@ typedef struct { >> >> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >> >> +// >> +// VBE code will access memory between 0-4095 which will cause page fault exception >> +// if NULL pointer detection mechanism is enabled. Following macros can be used to >> +// disable/enable NULL pointer detection before/after accessing those memory. >> +// >> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >> +#define DISABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >> + } >> +#define ENABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >> + } >> + > > (8) I believe Jordan too commented on these macros elsewhere (under > patch 1/4). > > In my opinion, this functionality should be extracted into a library > class, with a library instance that is suitable for at least UEFI_DRIVER > modules. (Maybe even for DXE_DRIVER modules.) > > You could add a separate library instance for SMM drivers, if that were > necessary. > > > (9) Style comment: please put one space character between the function > designator and the opening parenthesis. > > >> // >> // QEMU Video Private Data Structure >> // >> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> index 7c7d429bca..5d166eb99c 100644 >> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> @@ -72,7 +72,9 @@ >> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >> gEfiPciIoProtocolGuid # PROTOCOL TO_START >> + gEfiCpuArchProtocolGuid >> >> [Pcd] >> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > > (10) Instead of these, the library class that I described under (8) > should be added here. > > Any further dependencies like PCDs, protocols etc should be inherited by > the driver through the library instance that the platform DSC file > resolves the library class to. > > Bonus: should you realize that the feature is impossible to implement > without accessing the CPU Arch protocol directly, you could hide the > protocol GUID dependency in the library instance INF file, and I'd be > none the wiser. > > ... Well, I could at least pretend that. :) > > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 8:46 ` Wang, Jian J @ 2017-09-14 8:48 ` Yao, Jiewen 2017-09-14 8:54 ` Laszlo Ersek 1 sibling, 0 replies; 21+ messages in thread From: Yao, Jiewen @ 2017-09-14 8:48 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Wolman, Ayellet, Kinney, Michael D, Zeng, Star, Yao, Jiewen If so, can we enhance the GCD implementation? I think this is compatible, because we allow more usage. And most important we still follow spec. Thank you Yao Jiewen From: Wang, Jian J Sent: Thursday, September 14, 2017 4:46 PM To: Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). From: Yao, Jiewen Sent: Thursday, September 14, 2017 4:38 PM To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. HI I wonder if it is spec limitation or implementation limitation. If it is implementation limitation, we can enhance the DxeCore to allow it. Thank you Yao Jiewen From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, September 14, 2017 4:30 PM To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 05:17, Wang, Jian J wrote: > For the use of arch protocol, there's one thing I mentioned is not > accurate. I actually tried gDS->SetMemorySpaceAttributes() but it > cannot change page attributes. That's why I have to turn to cpu arch > protocol. Thank you for the explanation. In that case, we cannot avoid violating the PI spec. I agree we can break the spec if it happens for security purposes, but in that case, I believe that hiding the implementation behind a library class is mandatory. Viewed from the spec side, accessing the CPU Arch protocol is a hack, and so it should not be spread to the source code of several driver modules. Thank you, Laszlo > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J > Sent: Thursday, September 14, 2017 9:17 AM > To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> > Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum. > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 7:35 AM > To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Hi, > > some of the points I'm going to make have already been pointed out by > Jordan: > > (1) When posting a patch series, please collect the Cc: tags from all of > the patches, and add them *all* to the cover letter. This way everyone > will get a personal copy of the general description. > > > (2) The subject line is too long. One possible simplification: > > OvmfPkg/QemuVideoDxe: bypass NULL pointer detection > > > On 09/13/17 11:25, Wang, Jian J wrote: >> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer >> detection is enabled, page 0 must be enabled temporarily before >> installing and disabled again afterwards. For Windows 7 boot, BIT7 of >> PcdNullPointerDetectionPropertyMask must still be set to avoid hang. > > (3) Subject line and commit message both should not exceed 74 characters > line length. (Not sure how many chars PatchCheck.py actually enforces, I > always stick with 74, following the Linux kernel tradition.) > > I rewrapped the commit message here for readability. > > >> >> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >> Cc: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > (4) I think this is also something that Jordan had pointed out a long > time ago (apologies if I mis-remember): > > The strings after the tags should form correct email addresses, and if > there are various email meta-characters in them, like "." (dot) and "," > (comma), then they should be quoted, like this: > > Cc: "Justen, Jordan L" <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> > Cc: "Kinney, Michael D" <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> > Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> > Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> > Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > If you look at the actual addresses on the emails that have been sent > out, you can see they are all malformed. For example, I have: > > "Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the > comma was taken to be a separate email address (a malformed one). > > At least for my reply, I have fixed up the email addresses. > > > (5) The commit message mentions BIT7 of the new PCD. > > First, thanks for checking Windows 7 boot (and I'm happy that I got > suspicious of the feature with regard to Windows 7). I think BIT7 is a > good feature. > > However, please include the short description of that feature in the > commit message -- it is one sentence; "Disable NULL pointer detection > just after EndOfDxe." > > (I think BIT7 is a really smart feature, and I like *how* it is used in > "NULL_DETECTION_ENABLED" below. The check means, "if the protection is > enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". > > This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; > more on that below.) > > >> --- >> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >> 3 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c >> index 0dce80e59b..ee0eed7214 100644 >> --- a/OvmfPkg/QemuVideoDxe/Driver.c >> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >> PCI_TYPE00 Pci; >> QEMU_VIDEO_CARD *Card; >> EFI_PCI_IO_PROTOCOL *ChildPciIo; >> + EFI_CPU_ARCH_PROTOCOL *Cpu; > > (6) I believe I mentioned this in the earlier discussion, in some form, > but now I'll say it again: > > A UEFI driver has no business poking at the CPU Arch protocol. The PI > spec (1.6) states, > > 12.3 CPU Architectural Protocol > EFI_CPU_ARCH_PROTOCOL > > Summary > > Abstracts the processor services that are required to implement some > of the DXE services. This protocol must be produced by a boot service > or runtime DXE driver and may only be consumed by the DXE Foundation > and DXE drivers that produce architectural protocols. > > The DXE core is obviously free to use the CPU Arch protocol, but a UEFI > driver is forbidden from it, *even if* we say that, in this UEFI driver, > we are going to use DXE services. (Which come from the PI spec, and not > the UEFI spec.) > > Therefore, here we have to use gDS->SetMemorySpaceAttributes(). > > The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch > protocol, by the PI spec. It is not easy to see, because the PI spec has > a formatting error in this area. If you look under > GetMemorySpaceDescriptor(), there is an error code > > EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU > architectural protocol is not available yet. > > Obviously this error code belongs to SetMemorySpaceAttributes(), not > GetMemorySpaceDescriptor(). > > Anyway, gDS should be used, architectural protocols shouldn't be. > > >> >> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >> >> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( >> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 >> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >> Private->Variant == QEMU_VIDEO_BOCHS) { >> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + // >> + // Prepare CPU arch protocol for NULL pointer detection >> + // >> + Status = gBS->LocateProtocol ( >> + &gEfiCpuArchProtocolGuid, >> + NULL, >> + (VOID **) &Cpu >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> + DISABLE_NULL_DETECTION(Cpu); >> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >> + ENABLE_NULL_DETECTION(Cpu); > > (7) The NULL detection disabling and enabling should bracket the > affected code as tightly as possible. > > So please move this into InstallVbeShim() accordingly. > > >> } >> #endif >> >> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h >> index 7fbb25b3ef..bb3bc6eb0f 100644 >> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >> @@ -25,6 +25,7 @@ >> #include <Protocol/PciIo.h> >> #include <Protocol/DriverSupportedEfiVersion.h> >> #include <Protocol/DevicePath.h> >> +#include <Protocol/Cpu.h> >> >> #include <Library/DebugLib.h> >> #include <Library/UefiDriverEntryPoint.h> >> @@ -82,6 +83,21 @@ typedef struct { >> >> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >> >> +// >> +// VBE code will access memory between 0-4095 which will cause page fault exception >> +// if NULL pointer detection mechanism is enabled. Following macros can be used to >> +// disable/enable NULL pointer detection before/after accessing those memory. >> +// >> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >> +#define DISABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >> + } >> +#define ENABLE_NULL_DETECTION(Cpu) \ >> + if (NULL_DETECTION_ENABLED) { \ >> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >> + } >> + > > (8) I believe Jordan too commented on these macros elsewhere (under > patch 1/4). > > In my opinion, this functionality should be extracted into a library > class, with a library instance that is suitable for at least UEFI_DRIVER > modules. (Maybe even for DXE_DRIVER modules.) > > You could add a separate library instance for SMM drivers, if that were > necessary. > > > (9) Style comment: please put one space character between the function > designator and the opening parenthesis. > > >> // >> // QEMU Video Private Data Structure >> // >> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> index 7c7d429bca..5d166eb99c 100644 >> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> @@ -72,7 +72,9 @@ >> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >> gEfiPciIoProtocolGuid # PROTOCOL TO_START >> + gEfiCpuArchProtocolGuid >> >> [Pcd] >> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > > (10) Instead of these, the library class that I described under (8) > should be added here. > > Any further dependencies like PCDs, protocols etc should be inherited by > the driver through the library instance that the platform DSC file > resolves the library class to. > > Bonus: should you realize that the feature is impossible to implement > without accessing the CPU Arch protocol directly, you could hide the > protocol GUID dependency in the library instance INF file, and I'd be > none the wiser. > > ... Well, I could at least pretend that. :) > > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 8:46 ` Wang, Jian J 2017-09-14 8:48 ` Yao, Jiewen @ 2017-09-14 8:54 ` Laszlo Ersek 2017-09-14 9:39 ` Zeng, Star 1 sibling, 1 reply; 21+ messages in thread From: Laszlo Ersek @ 2017-09-14 8:54 UTC (permalink / raw) To: Wang, Jian J, Yao, Jiewen Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Wolman, Ayellet, Kinney, Michael D, Zeng, Star On 09/14/17 10:46, Wang, Jian J wrote: > It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes. Does "git blame" provide a reason, from the message of the commit that added the filtering? Thanks, Laszlo > From: Yao, Jiewen > Sent: Thursday, September 14, 2017 4:38 PM > To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > HI > I wonder if it is spec limitation or implementation limitation. > > If it is implementation limitation, we can enhance the DxeCore to allow it. > > Thank you > Yao Jiewen > > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:30 PM > To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 05:17, Wang, Jian J wrote: >> For the use of arch protocol, there's one thing I mentioned is not >> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >> cannot change page attributes. That's why I have to turn to cpu arch >> protocol. > Thank you for the explanation. > > In that case, we cannot avoid violating the PI spec. I agree we can > break the spec if it happens for security purposes, but in that case, I > believe that hiding the implementation behind a library class is > mandatory. Viewed from the spec side, accessing the CPU Arch protocol is > a hack, and so it should not be spread to the source code of several > driver modules. > > Thank you, > Laszlo > > >> >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J >> Sent: Thursday, September 14, 2017 9:17 AM >> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> Thanks for the comments and good advices. Sorry the format issues. >> This is my first patch for this project. Too many details for me to get >> familiar with. >> >> (1) Sure. >> (2) I'll change that. >> (3) I'll use the tool to ensure the patch format. >> (4) I'll remove the ',' in name >> (5) I'll add more description about it. >> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >> instead. The only reason I didn't do it is that I found >> GetMemorySpaceDescriptor() doesn't return the same information >> which SetMemorySpaceAttributes() just changed. So I feel using CPU >> arch protocol is a bit safer. Anyway, I'll change it. >> (7) I did put those macros in the install function before. To reduce the >> number of changed files, I made current changes. You're right it's >> not worthy. >> (8) Using macro can help the readability, which is more important to me. >> I know function can do the same. But it looks a bit heavy in this situation. >> I have to admit replacing the macros with a library is a very good idea, >> which brings the same readability. I didn't think of that before. Although >> Library is still a little bit heavy to me but it's in a different way, I think it >> worth a trying. >> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >> (10) You're right. Using library can reduce the disturbs to affected drivers >> by this feature to the minimum. >> >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 7:35 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> Hi, >> >> some of the points I'm going to make have already been pointed out by >> Jordan: >> >> (1) When posting a patch series, please collect the Cc: tags from all of >> the patches, and add them *all* to the cover letter. This way everyone >> will get a personal copy of the general description. >> >> >> (2) The subject line is too long. One possible simplification: >> >> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >> >> >> On 09/13/17 11:25, Wang, Jian J wrote: >>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer >>> detection is enabled, page 0 must be enabled temporarily before >>> installing and disabled again afterwards. For Windows 7 boot, BIT7 of >>> PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >> >> (3) Subject line and commit message both should not exceed 74 characters >> line length. (Not sure how many chars PatchCheck.py actually enforces, I >> always stick with 74, following the Linux kernel tradition.) >> >> I rewrapped the commit message here for readability. >> >> >>> >>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> >> (4) I think this is also something that Jordan had pointed out a long >> time ago (apologies if I mis-remember): >> >> The strings after the tags should form correct email addresses, and if >> there are various email meta-characters in them, like "." (dot) and "," >> (comma), then they should be quoted, like this: >> >> Cc: "Justen, Jordan L" <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >> Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> >> If you look at the actual addresses on the emails that have been sent >> out, you can see they are all malformed. For example, I have: >> >> "Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the >> comma was taken to be a separate email address (a malformed one). >> >> At least for my reply, I have fixed up the email addresses. >> >> >> (5) The commit message mentions BIT7 of the new PCD. >> >> First, thanks for checking Windows 7 boot (and I'm happy that I got >> suspicious of the feature with regard to Windows 7). I think BIT7 is a >> good feature. >> >> However, please include the short description of that feature in the >> commit message -- it is one sentence; "Disable NULL pointer detection >> just after EndOfDxe." >> >> (I think BIT7 is a really smart feature, and I like *how* it is used in >> "NULL_DETECTION_ENABLED" below. The check means, "if the protection is >> enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >> >> This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; >> more on that below.) >> >> >>> --- >>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>> 3 files changed, 32 insertions(+), 1 deletion(-) >>> >>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c >>> index 0dce80e59b..ee0eed7214 100644 >>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>> PCI_TYPE00 Pci; >>> QEMU_VIDEO_CARD *Card; >>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >> >> (6) I believe I mentioned this in the earlier discussion, in some form, >> but now I'll say it again: >> >> A UEFI driver has no business poking at the CPU Arch protocol. The PI >> spec (1.6) states, >> >> 12.3 CPU Architectural Protocol >> EFI_CPU_ARCH_PROTOCOL >> >> Summary >> >> Abstracts the processor services that are required to implement some >> of the DXE services. This protocol must be produced by a boot service >> or runtime DXE driver and may only be consumed by the DXE Foundation >> and DXE drivers that produce architectural protocols. >> >> The DXE core is obviously free to use the CPU Arch protocol, but a UEFI >> driver is forbidden from it, *even if* we say that, in this UEFI driver, >> we are going to use DXE services. (Which come from the PI spec, and not >> the UEFI spec.) >> >> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >> >> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >> protocol, by the PI spec. It is not easy to see, because the PI spec has >> a formatting error in this area. If you look under >> GetMemorySpaceDescriptor(), there is an error code >> >> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >> architectural protocol is not available yet. >> >> Obviously this error code belongs to SetMemorySpaceAttributes(), not >> GetMemorySpaceDescriptor(). >> >> Anyway, gDS should be used, architectural protocols shouldn't be. >> >> >>> >>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>> >>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( >>> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 >>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>> Private->Variant == QEMU_VIDEO_BOCHS) { >>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>> + // >>> + // Prepare CPU arch protocol for NULL pointer detection >>> + // >>> + Status = gBS->LocateProtocol ( >>> + &gEfiCpuArchProtocolGuid, >>> + NULL, >>> + (VOID **) &Cpu >>> + ); >>> + ASSERT_EFI_ERROR (Status); >>> + >>> + DISABLE_NULL_DETECTION(Cpu); >>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>> + ENABLE_NULL_DETECTION(Cpu); >> >> (7) The NULL detection disabling and enabling should bracket the >> affected code as tightly as possible. >> >> So please move this into InstallVbeShim() accordingly. >> >> >>> } >>> #endif >>> >>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h >>> index 7fbb25b3ef..bb3bc6eb0f 100644 >>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>> @@ -25,6 +25,7 @@ >>> #include <Protocol/PciIo.h> >>> #include <Protocol/DriverSupportedEfiVersion.h> >>> #include <Protocol/DevicePath.h> >>> +#include <Protocol/Cpu.h> >>> >>> #include <Library/DebugLib.h> >>> #include <Library/UefiDriverEntryPoint.h> >>> @@ -82,6 +83,21 @@ typedef struct { >>> >>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>> >>> +// >>> +// VBE code will access memory between 0-4095 which will cause page fault exception >>> +// if NULL pointer detection mechanism is enabled. Following macros can be used to >>> +// disable/enable NULL pointer detection before/after accessing those memory. >>> +// >>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>> + if (NULL_DETECTION_ENABLED) { \ >>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>> + } >>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>> + if (NULL_DETECTION_ENABLED) { \ >>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>> + } >>> + >> >> (8) I believe Jordan too commented on these macros elsewhere (under >> patch 1/4). >> >> In my opinion, this functionality should be extracted into a library >> class, with a library instance that is suitable for at least UEFI_DRIVER >> modules. (Maybe even for DXE_DRIVER modules.) >> >> You could add a separate library instance for SMM drivers, if that were >> necessary. >> >> >> (9) Style comment: please put one space character between the function >> designator and the opening parenthesis. >> >> >>> // >>> // QEMU Video Private Data Structure >>> // >>> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> index 7c7d429bca..5d166eb99c 100644 >>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> @@ -72,7 +72,9 @@ >>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>> + gEfiCpuArchProtocolGuid >>> >>> [Pcd] >>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask >> >> (10) Instead of these, the library class that I described under (8) >> should be added here. >> >> Any further dependencies like PCDs, protocols etc should be inherited by >> the driver through the library instance that the platform DSC file >> resolves the library class to. >> >> Bonus: should you realize that the feature is impossible to implement >> without accessing the CPU Arch protocol directly, you could hide the >> protocol GUID dependency in the library instance INF file, and I'd be >> none the wiser. >> >> ... Well, I could at least pretend that. :) >> >> Thanks, >> Laszlo >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> https://lists.01.org/mailman/listinfo/edk2-devel >> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 8:54 ` Laszlo Ersek @ 2017-09-14 9:39 ` Zeng, Star 2017-09-14 9:55 ` Laszlo Ersek 0 siblings, 1 reply; 21+ messages in thread From: Zeng, Star @ 2017-09-14 9:39 UTC (permalink / raw) To: Laszlo Ersek, Wang, Jian J, Yao, Jiewen Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Wolman, Ayellet, Kinney, Michael D, Zeng, Star There is a realistic problem I just found and want to share. GCD database is built based on resource hob reported from PEI, and GCD checks the input Attributes against Capabilities when setting Attributes, but we (I just searched the whole codebase) can see there is no code reporting resource hob with EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE, EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set. Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, September 14, 2017 4:54 PM To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 10:46, Wang, Jian J wrote: > It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes. Does "git blame" provide a reason, from the message of the commit that added the filtering? Thanks, Laszlo > From: Yao, Jiewen > Sent: Thursday, September 14, 2017 4:38 PM > To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J > <jian.j.wang@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet > <ayellet.wolman@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > HI > I wonder if it is spec limitation or implementation limitation. > > If it is implementation limitation, we can enhance the DxeCore to allow it. > > Thank you > Yao Jiewen > > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:30 PM > To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; > Justen, Jordan L > <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen > <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet > <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, > Michael D > <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, > Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 05:17, Wang, Jian J wrote: >> For the use of arch protocol, there's one thing I mentioned is not >> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >> cannot change page attributes. That's why I have to turn to cpu arch >> protocol. > Thank you for the explanation. > > In that case, we cannot avoid violating the PI spec. I agree we can > break the spec if it happens for security purposes, but in that case, > I believe that hiding the implementation behind a library class is > mandatory. Viewed from the spec side, accessing the CPU Arch protocol > is a hack, and so it should not be spread to the source code of > several driver modules. > > Thank you, > Laszlo > > >> >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >> Of Wang, Jian J >> Sent: Thursday, September 14, 2017 9:17 AM >> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >> Justen, Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >> Michael D >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> Thanks for the comments and good advices. Sorry the format issues. >> This is my first patch for this project. Too many details for me to >> get familiar with. >> >> (1) Sure. >> (2) I'll change that. >> (3) I'll use the tool to ensure the patch format. >> (4) I'll remove the ',' in name >> (5) I'll add more description about it. >> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >> instead. The only reason I didn't do it is that I found >> GetMemorySpaceDescriptor() doesn't return the same information >> which SetMemorySpaceAttributes() just changed. So I feel using CPU >> arch protocol is a bit safer. Anyway, I'll change it. >> (7) I did put those macros in the install function before. To reduce the >> number of changed files, I made current changes. You're right it's >> not worthy. >> (8) Using macro can help the readability, which is more important to me. >> I know function can do the same. But it looks a bit heavy in this situation. >> I have to admit replacing the macros with a library is a very good idea, >> which brings the same readability. I didn't think of that before. Although >> Library is still a little bit heavy to me but it's in a different way, I think it >> worth a trying. >> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >> (10) You're right. Using library can reduce the disturbs to affected drivers >> by this feature to the minimum. >> >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 7:35 AM >> To: Wang, Jian J >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, >> Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, >> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, >> Michael D >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >> Wolman, Ayellet >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, >> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, >> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> Hi, >> >> some of the points I'm going to make have already been pointed out by >> Jordan: >> >> (1) When posting a patch series, please collect the Cc: tags from all >> of the patches, and add them *all* to the cover letter. This way >> everyone will get a personal copy of the general description. >> >> >> (2) The subject line is too long. One possible simplification: >> >> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >> >> >> On 09/13/17 11:25, Wang, Jian J wrote: >>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL >>> pointer detection is enabled, page 0 must be enabled temporarily >>> before installing and disabled again afterwards. For Windows 7 boot, >>> BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >> >> (3) Subject line and commit message both should not exceed 74 >> characters line length. (Not sure how many chars PatchCheck.py >> actually enforces, I always stick with 74, following the Linux kernel >> tradition.) >> >> I rewrapped the commit message here for readability. >> >> >>> >>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Justen, Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: Kinney, Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Wang, Jian J >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> >> (4) I think this is also something that Jordan had pointed out a long >> time ago (apologies if I mis-remember): >> >> The strings after the tags should form correct email addresses, and >> if there are various email meta-characters in them, like "." (dot) and "," >> (comma), then they should be quoted, like this: >> >> Cc: "Justen, Jordan L" >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >> Cc: "Kinney, Michael D" >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >> Cc: "Wolman, Ayellet" >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Suggested-by: "Wolman, Ayellet" >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Signed-off-by: "Wang, Jian J" >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> >> If you look at the actual addresses on the emails that have been sent >> out, you can see they are all malformed. For example, I have: >> >> "Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). >> >> At least for my reply, I have fixed up the email addresses. >> >> >> (5) The commit message mentions BIT7 of the new PCD. >> >> First, thanks for checking Windows 7 boot (and I'm happy that I got >> suspicious of the feature with regard to Windows 7). I think BIT7 is >> a good feature. >> >> However, please include the short description of that feature in the >> commit message -- it is one sentence; "Disable NULL pointer detection >> just after EndOfDxe." >> >> (I think BIT7 is a really smart feature, and I like *how* it is used >> in "NULL_DETECTION_ENABLED" below. The check means, "if the >> protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >> >> This doesn't mean that I like the NULL_DETECTION_ENABLED macro >> itself; more on that below.) >> >> >>> --- >>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>> 3 files changed, 32 insertions(+), 1 deletion(-) >>> >>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c >>> b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 >>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>> PCI_TYPE00 Pci; >>> QEMU_VIDEO_CARD *Card; >>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >> >> (6) I believe I mentioned this in the earlier discussion, in some >> form, but now I'll say it again: >> >> A UEFI driver has no business poking at the CPU Arch protocol. The PI >> spec (1.6) states, >> >> 12.3 CPU Architectural Protocol >> EFI_CPU_ARCH_PROTOCOL >> >> Summary >> >> Abstracts the processor services that are required to implement some >> of the DXE services. This protocol must be produced by a boot service >> or runtime DXE driver and may only be consumed by the DXE Foundation >> and DXE drivers that produce architectural protocols. >> >> The DXE core is obviously free to use the CPU Arch protocol, but a >> UEFI driver is forbidden from it, *even if* we say that, in this UEFI >> driver, we are going to use DXE services. (Which come from the PI >> spec, and not the UEFI spec.) >> >> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >> >> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >> protocol, by the PI spec. It is not easy to see, because the PI spec >> has a formatting error in this area. If you look under >> GetMemorySpaceDescriptor(), there is an error code >> >> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >> architectural protocol is not available yet. >> >> Obviously this error code belongs to SetMemorySpaceAttributes(), not >> GetMemorySpaceDescriptor(). >> >> Anyway, gDS should be used, architectural protocols shouldn't be. >> >> >>> >>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>> >>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined >>> MDE_CPU_IA32 || defined MDE_CPU_X64 >>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>> Private->Variant == QEMU_VIDEO_BOCHS) { >>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>> + // >>> + // Prepare CPU arch protocol for NULL pointer detection >>> + // >>> + Status = gBS->LocateProtocol ( >>> + &gEfiCpuArchProtocolGuid, >>> + NULL, >>> + (VOID **) &Cpu >>> + ); >>> + ASSERT_EFI_ERROR (Status); >>> + >>> + DISABLE_NULL_DETECTION(Cpu); >>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>> + ENABLE_NULL_DETECTION(Cpu); >> >> (7) The NULL detection disabling and enabling should bracket the >> affected code as tightly as possible. >> >> So please move this into InstallVbeShim() accordingly. >> >> >>> } >>> #endif >>> >>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h >>> b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 >>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>> @@ -25,6 +25,7 @@ >>> #include <Protocol/PciIo.h> >>> #include <Protocol/DriverSupportedEfiVersion.h> >>> #include <Protocol/DevicePath.h> >>> +#include <Protocol/Cpu.h> >>> >>> #include <Library/DebugLib.h> >>> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ >>> typedef struct { >>> >>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>> >>> +// >>> +// VBE code will access memory between 0-4095 which will cause page >>> +fault exception // if NULL pointer detection mechanism is enabled. >>> +Following macros can be used to // disable/enable NULL pointer detection before/after accessing those memory. >>> +// >>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>> + if (NULL_DETECTION_ENABLED) { \ >>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>> + } >>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>> + if (NULL_DETECTION_ENABLED) { \ >>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>> + } >>> + >> >> (8) I believe Jordan too commented on these macros elsewhere (under >> patch 1/4). >> >> In my opinion, this functionality should be extracted into a library >> class, with a library instance that is suitable for at least >> UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) >> >> You could add a separate library instance for SMM drivers, if that >> were necessary. >> >> >> (9) Style comment: please put one space character between the >> function designator and the opening parenthesis. >> >> >>> // >>> // QEMU Video Private Data Structure // diff --git >>> a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> index 7c7d429bca..5d166eb99c 100644 >>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> @@ -72,7 +72,9 @@ >>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>> + gEfiCpuArchProtocolGuid >>> >>> [Pcd] >>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>> + >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask >> >> (10) Instead of these, the library class that I described under (8) >> should be added here. >> >> Any further dependencies like PCDs, protocols etc should be inherited >> by the driver through the library instance that the platform DSC file >> resolves the library class to. >> >> Bonus: should you realize that the feature is impossible to implement >> without accessing the CPU Arch protocol directly, you could hide the >> protocol GUID dependency in the library instance INF file, and I'd be >> none the wiser. >> >> ... Well, I could at least pretend that. :) >> >> Thanks, >> Laszlo >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> https://lists.01.org/mailman/listinfo/edk2-devel >> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 9:39 ` Zeng, Star @ 2017-09-14 9:55 ` Laszlo Ersek 2017-09-14 10:16 ` Zeng, Star 0 siblings, 1 reply; 21+ messages in thread From: Laszlo Ersek @ 2017-09-14 9:55 UTC (permalink / raw) To: Zeng, Star, Wang, Jian J, Yao, Jiewen Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Wolman, Ayellet, Kinney, Michael D On 09/14/17 11:39, Zeng, Star wrote: > There is a realistic problem I just found and want to share. > > GCD database is built based on resource hob reported from PEI, and > GCD checks the input Attributes against Capabilities when setting > Attributes, but we (I just searched the whole codebase) can see there > is no code reporting resource hob with > EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE, > EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or > EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set. ... Are you saying that, if we modify AddMemoryBaseSizeHob() in OvmfPkg/PlatformPei, to report EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE (*), then gDS->SetMemorySpaceAttributes() will start to work? ((*) "MdePkg/Include/Pi/PiHob.h": "NOTE: Since PI spec 1.4, please use EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE as Memory capability attribute: The memory supports being protected from processor writes") If that's the case, then I hope Jian can include such an OvmfPkg patch in his series. :) Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:54 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 10:46, Wang, Jian J wrote: >> It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). > > That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes. > > Does "git blame" provide a reason, from the message of the commit that added the filtering? > > Thanks, > Laszlo > >> From: Yao, Jiewen >> Sent: Thursday, September 14, 2017 4:38 PM >> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J >> <jian.j.wang@intel.com> >> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet >> <ayellet.wolman@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> HI >> I wonder if it is spec limitation or implementation limitation. >> >> If it is implementation limitation, we can enhance the DxeCore to allow it. >> >> Thank you >> Yao Jiewen >> >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 4:30 PM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >> Justen, Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >> Michael D >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, >> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> On 09/14/17 05:17, Wang, Jian J wrote: >>> For the use of arch protocol, there's one thing I mentioned is not >>> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >>> cannot change page attributes. That's why I have to turn to cpu arch >>> protocol. >> Thank you for the explanation. >> >> In that case, we cannot avoid violating the PI spec. I agree we can >> break the spec if it happens for security purposes, but in that case, >> I believe that hiding the implementation behind a library class is >> mandatory. Viewed from the spec side, accessing the CPU Arch protocol >> is a hack, and so it should not be spread to the source code of >> several driver modules. >> >> Thank you, >> Laszlo >> >> >>> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>> Of Wang, Jian J >>> Sent: Thursday, September 14, 2017 9:17 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>> Justen, Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Thanks for the comments and good advices. Sorry the format issues. >>> This is my first patch for this project. Too many details for me to >>> get familiar with. >>> >>> (1) Sure. >>> (2) I'll change that. >>> (3) I'll use the tool to ensure the patch format. >>> (4) I'll remove the ',' in name >>> (5) I'll add more description about it. >>> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >>> instead. The only reason I didn't do it is that I found >>> GetMemorySpaceDescriptor() doesn't return the same information >>> which SetMemorySpaceAttributes() just changed. So I feel using CPU >>> arch protocol is a bit safer. Anyway, I'll change it. >>> (7) I did put those macros in the install function before. To reduce the >>> number of changed files, I made current changes. You're right it's >>> not worthy. >>> (8) Using macro can help the readability, which is more important to me. >>> I know function can do the same. But it looks a bit heavy in this situation. >>> I have to admit replacing the macros with a library is a very good idea, >>> which brings the same readability. I didn't think of that before. Although >>> Library is still a little bit heavy to me but it's in a different way, I think it >>> worth a trying. >>> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >>> (10) You're right. Using library can reduce the disturbs to affected drivers >>> by this feature to the minimum. >>> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Thursday, September 14, 2017 7:35 AM >>> To: Wang, Jian J >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, >>> Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, >>> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, >>> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, >>> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Hi, >>> >>> some of the points I'm going to make have already been pointed out by >>> Jordan: >>> >>> (1) When posting a patch series, please collect the Cc: tags from all >>> of the patches, and add them *all* to the cover letter. This way >>> everyone will get a personal copy of the general description. >>> >>> >>> (2) The subject line is too long. One possible simplification: >>> >>> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >>> >>> >>> On 09/13/17 11:25, Wang, Jian J wrote: >>>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL >>>> pointer detection is enabled, page 0 must be enabled temporarily >>>> before installing and disabled again afterwards. For Windows 7 boot, >>>> BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >>> >>> (3) Subject line and commit message both should not exceed 74 >>> characters line length. (Not sure how many chars PatchCheck.py >>> actually enforces, I always stick with 74, following the Linux kernel >>> tradition.) >>> >>> I rewrapped the commit message here for readability. >>> >>> >>>> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>> Cc: Justen, Jordan L >>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>>> Cc: Kinney, Michael D >>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>>> Cc: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Suggested-by: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Wang, Jian J >>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> (4) I think this is also something that Jordan had pointed out a long >>> time ago (apologies if I mis-remember): >>> >>> The strings after the tags should form correct email addresses, and >>> if there are various email meta-characters in them, like "." (dot) and "," >>> (comma), then they should be quoted, like this: >>> >>> Cc: "Justen, Jordan L" >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: "Kinney, Michael D" >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Signed-off-by: "Wang, Jian J" >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> If you look at the actual addresses on the emails that have been sent >>> out, you can see they are all malformed. For example, I have: >>> >>> "Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). >>> >>> At least for my reply, I have fixed up the email addresses. >>> >>> >>> (5) The commit message mentions BIT7 of the new PCD. >>> >>> First, thanks for checking Windows 7 boot (and I'm happy that I got >>> suspicious of the feature with regard to Windows 7). I think BIT7 is >>> a good feature. >>> >>> However, please include the short description of that feature in the >>> commit message -- it is one sentence; "Disable NULL pointer detection >>> just after EndOfDxe." >>> >>> (I think BIT7 is a really smart feature, and I like *how* it is used >>> in "NULL_DETECTION_ENABLED" below. The check means, "if the >>> protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >>> >>> This doesn't mean that I like the NULL_DETECTION_ENABLED macro >>> itself; more on that below.) >>> >>> >>>> --- >>>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>>> 3 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c >>>> b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>>> PCI_TYPE00 Pci; >>>> QEMU_VIDEO_CARD *Card; >>>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >>> >>> (6) I believe I mentioned this in the earlier discussion, in some >>> form, but now I'll say it again: >>> >>> A UEFI driver has no business poking at the CPU Arch protocol. The PI >>> spec (1.6) states, >>> >>> 12.3 CPU Architectural Protocol >>> EFI_CPU_ARCH_PROTOCOL >>> >>> Summary >>> >>> Abstracts the processor services that are required to implement some >>> of the DXE services. This protocol must be produced by a boot service >>> or runtime DXE driver and may only be consumed by the DXE Foundation >>> and DXE drivers that produce architectural protocols. >>> >>> The DXE core is obviously free to use the CPU Arch protocol, but a >>> UEFI driver is forbidden from it, *even if* we say that, in this UEFI >>> driver, we are going to use DXE services. (Which come from the PI >>> spec, and not the UEFI spec.) >>> >>> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >>> >>> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >>> protocol, by the PI spec. It is not easy to see, because the PI spec >>> has a formatting error in this area. If you look under >>> GetMemorySpaceDescriptor(), there is an error code >>> >>> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >>> architectural protocol is not available yet. >>> >>> Obviously this error code belongs to SetMemorySpaceAttributes(), not >>> GetMemorySpaceDescriptor(). >>> >>> Anyway, gDS should be used, architectural protocols shouldn't be. >>> >>> >>>> >>>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>>> >>>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined >>>> MDE_CPU_IA32 || defined MDE_CPU_X64 >>>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>>> Private->Variant == QEMU_VIDEO_BOCHS) { >>>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + // >>>> + // Prepare CPU arch protocol for NULL pointer detection >>>> + // >>>> + Status = gBS->LocateProtocol ( >>>> + &gEfiCpuArchProtocolGuid, >>>> + NULL, >>>> + (VOID **) &Cpu >>>> + ); >>>> + ASSERT_EFI_ERROR (Status); >>>> + >>>> + DISABLE_NULL_DETECTION(Cpu); >>>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + ENABLE_NULL_DETECTION(Cpu); >>> >>> (7) The NULL detection disabling and enabling should bracket the >>> affected code as tightly as possible. >>> >>> So please move this into InstallVbeShim() accordingly. >>> >>> >>>> } >>>> #endif >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>>> @@ -25,6 +25,7 @@ >>>> #include <Protocol/PciIo.h> >>>> #include <Protocol/DriverSupportedEfiVersion.h> >>>> #include <Protocol/DevicePath.h> >>>> +#include <Protocol/Cpu.h> >>>> >>>> #include <Library/DebugLib.h> >>>> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ >>>> typedef struct { >>>> >>>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>>> >>>> +// >>>> +// VBE code will access memory between 0-4095 which will cause page >>>> +fault exception // if NULL pointer detection mechanism is enabled. >>>> +Following macros can be used to // disable/enable NULL pointer detection before/after accessing those memory. >>>> +// >>>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>>> + } >>>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>>> + } >>>> + >>> >>> (8) I believe Jordan too commented on these macros elsewhere (under >>> patch 1/4). >>> >>> In my opinion, this functionality should be extracted into a library >>> class, with a library instance that is suitable for at least >>> UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) >>> >>> You could add a separate library instance for SMM drivers, if that >>> were necessary. >>> >>> >>> (9) Style comment: please put one space character between the >>> function designator and the opening parenthesis. >>> >>> >>>> // >>>> // QEMU Video Private Data Structure // diff --git >>>> a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> index 7c7d429bca..5d166eb99c 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> @@ -72,7 +72,9 @@ >>>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>>> + gEfiCpuArchProtocolGuid >>>> >>>> [Pcd] >>>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>>> + >>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask >>> >>> (10) Instead of these, the library class that I described under (8) >>> should be added here. >>> >>> Any further dependencies like PCDs, protocols etc should be inherited >>> by the driver through the library instance that the platform DSC file >>> resolves the library class to. >>> >>> Bonus: should you realize that the feature is impossible to implement >>> without accessing the CPU Arch protocol directly, you could hide the >>> protocol GUID dependency in the library instance INF file, and I'd be >>> none the wiser. >>> >>> ... Well, I could at least pretend that. :) >>> >>> Thanks, >>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 9:55 ` Laszlo Ersek @ 2017-09-14 10:16 ` Zeng, Star 2017-09-15 0:15 ` Wang, Jian J 0 siblings, 1 reply; 21+ messages in thread From: Zeng, Star @ 2017-09-14 10:16 UTC (permalink / raw) To: Laszlo Ersek, Wang, Jian J, Yao, Jiewen Cc: Kinney, Michael D, Justen, Jordan L, edk2-devel@lists.01.org, Dong, Eric, Wolman, Ayellet, Zeng, Star No, I want to say even the GCD is updated to not filter page attributes, it will still not work as all the memory controller code for now are not reporting the Capability. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, September 14, 2017 5:55 PM To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 11:39, Zeng, Star wrote: > There is a realistic problem I just found and want to share. > > GCD database is built based on resource hob reported from PEI, and GCD > checks the input Attributes against Capabilities when setting > Attributes, but we (I just searched the whole codebase) can see there > is no code reporting resource hob with > EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE, > EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or > EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set. ... Are you saying that, if we modify AddMemoryBaseSizeHob() in OvmfPkg/PlatformPei, to report EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE (*), then gDS->SetMemorySpaceAttributes() will start to work? ((*) "MdePkg/Include/Pi/PiHob.h": "NOTE: Since PI spec 1.4, please use EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE as Memory capability attribute: The memory supports being protected from processor writes") If that's the case, then I hope Jian can include such an OvmfPkg patch in his series. :) Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:54 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet > <ayellet.wolman@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 10:46, Wang, Jian J wrote: >> It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). > > That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes. > > Does "git blame" provide a reason, from the message of the commit that added the filtering? > > Thanks, > Laszlo > >> From: Yao, Jiewen >> Sent: Thursday, September 14, 2017 4:38 PM >> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J >> <jian.j.wang@intel.com> >> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet >> <ayellet.wolman@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> HI >> I wonder if it is spec limitation or implementation limitation. >> >> If it is implementation limitation, we can enhance the DxeCore to allow it. >> >> Thank you >> Yao Jiewen >> >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 4:30 PM >> To: Wang, Jian J >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >> Justen, Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >> Michael D >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> On 09/14/17 05:17, Wang, Jian J wrote: >>> For the use of arch protocol, there's one thing I mentioned is not >>> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >>> cannot change page attributes. That's why I have to turn to cpu arch >>> protocol. >> Thank you for the explanation. >> >> In that case, we cannot avoid violating the PI spec. I agree we can >> break the spec if it happens for security purposes, but in that case, >> I believe that hiding the implementation behind a library class is >> mandatory. Viewed from the spec side, accessing the CPU Arch protocol >> is a hack, and so it should not be spread to the source code of >> several driver modules. >> >> Thank you, >> Laszlo >> >> >>> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>> Of Wang, Jian J >>> Sent: Thursday, September 14, 2017 9:17 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>> Justen, Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Thanks for the comments and good advices. Sorry the format issues. >>> This is my first patch for this project. Too many details for me to >>> get familiar with. >>> >>> (1) Sure. >>> (2) I'll change that. >>> (3) I'll use the tool to ensure the patch format. >>> (4) I'll remove the ',' in name >>> (5) I'll add more description about it. >>> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >>> instead. The only reason I didn't do it is that I found >>> GetMemorySpaceDescriptor() doesn't return the same information >>> which SetMemorySpaceAttributes() just changed. So I feel using CPU >>> arch protocol is a bit safer. Anyway, I'll change it. >>> (7) I did put those macros in the install function before. To reduce the >>> number of changed files, I made current changes. You're right it's >>> not worthy. >>> (8) Using macro can help the readability, which is more important to me. >>> I know function can do the same. But it looks a bit heavy in this situation. >>> I have to admit replacing the macros with a library is a very good idea, >>> which brings the same readability. I didn't think of that before. Although >>> Library is still a little bit heavy to me but it's in a different way, I think it >>> worth a trying. >>> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >>> (10) You're right. Using library can reduce the disturbs to affected drivers >>> by this feature to the minimum. >>> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Thursday, September 14, 2017 7:35 AM >>> To: Wang, Jian J >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, >>> Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, >>> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, >>> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, >>> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Hi, >>> >>> some of the points I'm going to make have already been pointed out >>> by >>> Jordan: >>> >>> (1) When posting a patch series, please collect the Cc: tags from >>> all of the patches, and add them *all* to the cover letter. This way >>> everyone will get a personal copy of the general description. >>> >>> >>> (2) The subject line is too long. One possible simplification: >>> >>> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >>> >>> >>> On 09/13/17 11:25, Wang, Jian J wrote: >>>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL >>>> pointer detection is enabled, page 0 must be enabled temporarily >>>> before installing and disabled again afterwards. For Windows 7 >>>> boot, >>>> BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >>> >>> (3) Subject line and commit message both should not exceed 74 >>> characters line length. (Not sure how many chars PatchCheck.py >>> actually enforces, I always stick with 74, following the Linux >>> kernel >>> tradition.) >>> >>> I rewrapped the commit message here for readability. >>> >>> >>>> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>> Cc: Justen, Jordan L >>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>>> Cc: Kinney, Michael D >>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>>> Cc: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Suggested-by: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Wang, Jian J >>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> (4) I think this is also something that Jordan had pointed out a >>> long time ago (apologies if I mis-remember): >>> >>> The strings after the tags should form correct email addresses, and >>> if there are various email meta-characters in them, like "." (dot) and "," >>> (comma), then they should be quoted, like this: >>> >>> Cc: "Justen, Jordan L" >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: "Kinney, Michael D" >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Signed-off-by: "Wang, Jian J" >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> If you look at the actual addresses on the emails that have been >>> sent out, you can see they are all malformed. For example, I have: >>> >>> "Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). >>> >>> At least for my reply, I have fixed up the email addresses. >>> >>> >>> (5) The commit message mentions BIT7 of the new PCD. >>> >>> First, thanks for checking Windows 7 boot (and I'm happy that I got >>> suspicious of the feature with regard to Windows 7). I think BIT7 is >>> a good feature. >>> >>> However, please include the short description of that feature in the >>> commit message -- it is one sentence; "Disable NULL pointer >>> detection just after EndOfDxe." >>> >>> (I think BIT7 is a really smart feature, and I like *how* it is used >>> in "NULL_DETECTION_ENABLED" below. The check means, "if the >>> protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >>> >>> This doesn't mean that I like the NULL_DETECTION_ENABLED macro >>> itself; more on that below.) >>> >>> >>>> --- >>>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>>> 3 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c >>>> b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>>> PCI_TYPE00 Pci; >>>> QEMU_VIDEO_CARD *Card; >>>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >>> >>> (6) I believe I mentioned this in the earlier discussion, in some >>> form, but now I'll say it again: >>> >>> A UEFI driver has no business poking at the CPU Arch protocol. The >>> PI spec (1.6) states, >>> >>> 12.3 CPU Architectural Protocol >>> EFI_CPU_ARCH_PROTOCOL >>> >>> Summary >>> >>> Abstracts the processor services that are required to implement some >>> of the DXE services. This protocol must be produced by a boot service >>> or runtime DXE driver and may only be consumed by the DXE Foundation >>> and DXE drivers that produce architectural protocols. >>> >>> The DXE core is obviously free to use the CPU Arch protocol, but a >>> UEFI driver is forbidden from it, *even if* we say that, in this >>> UEFI driver, we are going to use DXE services. (Which come from the >>> PI spec, and not the UEFI spec.) >>> >>> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >>> >>> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >>> protocol, by the PI spec. It is not easy to see, because the PI spec >>> has a formatting error in this area. If you look under >>> GetMemorySpaceDescriptor(), there is an error code >>> >>> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >>> architectural protocol is not available yet. >>> >>> Obviously this error code belongs to SetMemorySpaceAttributes(), not >>> GetMemorySpaceDescriptor(). >>> >>> Anyway, gDS should be used, architectural protocols shouldn't be. >>> >>> >>>> >>>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>>> >>>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined >>>> MDE_CPU_IA32 || defined MDE_CPU_X64 >>>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>>> Private->Variant == QEMU_VIDEO_BOCHS) { >>>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + // >>>> + // Prepare CPU arch protocol for NULL pointer detection >>>> + // >>>> + Status = gBS->LocateProtocol ( >>>> + &gEfiCpuArchProtocolGuid, >>>> + NULL, >>>> + (VOID **) &Cpu >>>> + ); >>>> + ASSERT_EFI_ERROR (Status); >>>> + >>>> + DISABLE_NULL_DETECTION(Cpu); >>>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + ENABLE_NULL_DETECTION(Cpu); >>> >>> (7) The NULL detection disabling and enabling should bracket the >>> affected code as tightly as possible. >>> >>> So please move this into InstallVbeShim() accordingly. >>> >>> >>>> } >>>> #endif >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>>> @@ -25,6 +25,7 @@ >>>> #include <Protocol/PciIo.h> >>>> #include <Protocol/DriverSupportedEfiVersion.h> >>>> #include <Protocol/DevicePath.h> >>>> +#include <Protocol/Cpu.h> >>>> >>>> #include <Library/DebugLib.h> >>>> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ >>>> typedef struct { >>>> >>>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>>> >>>> +// >>>> +// VBE code will access memory between 0-4095 which will cause >>>> +page fault exception // if NULL pointer detection mechanism is enabled. >>>> +Following macros can be used to // disable/enable NULL pointer detection before/after accessing those memory. >>>> +// >>>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>>> + } >>>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>>> + } >>>> + >>> >>> (8) I believe Jordan too commented on these macros elsewhere (under >>> patch 1/4). >>> >>> In my opinion, this functionality should be extracted into a library >>> class, with a library instance that is suitable for at least >>> UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) >>> >>> You could add a separate library instance for SMM drivers, if that >>> were necessary. >>> >>> >>> (9) Style comment: please put one space character between the >>> function designator and the opening parenthesis. >>> >>> >>>> // >>>> // QEMU Video Private Data Structure // diff --git >>>> a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> index 7c7d429bca..5d166eb99c 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> @@ -72,7 +72,9 @@ >>>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>>> + gEfiCpuArchProtocolGuid >>>> >>>> [Pcd] >>>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>>> + >>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMas >>>> + k >>> >>> (10) Instead of these, the library class that I described under (8) >>> should be added here. >>> >>> Any further dependencies like PCDs, protocols etc should be >>> inherited by the driver through the library instance that the >>> platform DSC file resolves the library class to. >>> >>> Bonus: should you realize that the feature is impossible to >>> implement without accessing the CPU Arch protocol directly, you >>> could hide the protocol GUID dependency in the library instance INF >>> file, and I'd be none the wiser. >>> >>> ... Well, I could at least pretend that. :) >>> >>> Thanks, >>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 10:16 ` Zeng, Star @ 2017-09-15 0:15 ` Wang, Jian J 2017-09-15 6:05 ` Wang, Jian J 0 siblings, 1 reply; 21+ messages in thread From: Wang, Jian J @ 2017-09-15 0:15 UTC (permalink / raw) To: Zeng, Star, Laszlo Ersek, Yao, Jiewen Cc: Kinney, Michael D, Justen, Jordan L, edk2-devel@lists.01.org, Dong, Eric, Wolman, Ayellet gDS-> SetMemorySpaceCapabilities() can change the Capability at any time. But I'm not sure about any side-effect of it. -----Original Message----- From: Zeng, Star Sent: Thursday, September 14, 2017 6:17 PM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. No, I want to say even the GCD is updated to not filter page attributes, it will still not work as all the memory controller code for now are not reporting the Capability. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, September 14, 2017 5:55 PM To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 11:39, Zeng, Star wrote: > There is a realistic problem I just found and want to share. > > GCD database is built based on resource hob reported from PEI, and GCD > checks the input Attributes against Capabilities when setting > Attributes, but we (I just searched the whole codebase) can see there > is no code reporting resource hob with > EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE, > EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or > EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set. ... Are you saying that, if we modify AddMemoryBaseSizeHob() in OvmfPkg/PlatformPei, to report EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE (*), then gDS->SetMemorySpaceAttributes() will start to work? ((*) "MdePkg/Include/Pi/PiHob.h": "NOTE: Since PI spec 1.4, please use EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE as Memory capability attribute: The memory supports being protected from processor writes") If that's the case, then I hope Jian can include such an OvmfPkg patch in his series. :) Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:54 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet > <ayellet.wolman@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 10:46, Wang, Jian J wrote: >> It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). > > That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes. > > Does "git blame" provide a reason, from the message of the commit that added the filtering? > > Thanks, > Laszlo > >> From: Yao, Jiewen >> Sent: Thursday, September 14, 2017 4:38 PM >> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J >> <jian.j.wang@intel.com> >> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet >> <ayellet.wolman@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> HI >> I wonder if it is spec limitation or implementation limitation. >> >> If it is implementation limitation, we can enhance the DxeCore to allow it. >> >> Thank you >> Yao Jiewen >> >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 4:30 PM >> To: Wang, Jian J >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >> Justen, Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >> Michael D >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> On 09/14/17 05:17, Wang, Jian J wrote: >>> For the use of arch protocol, there's one thing I mentioned is not >>> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >>> cannot change page attributes. That's why I have to turn to cpu arch >>> protocol. >> Thank you for the explanation. >> >> In that case, we cannot avoid violating the PI spec. I agree we can >> break the spec if it happens for security purposes, but in that case, >> I believe that hiding the implementation behind a library class is >> mandatory. Viewed from the spec side, accessing the CPU Arch protocol >> is a hack, and so it should not be spread to the source code of >> several driver modules. >> >> Thank you, >> Laszlo >> >> >>> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>> Of Wang, Jian J >>> Sent: Thursday, September 14, 2017 9:17 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>> Justen, Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Thanks for the comments and good advices. Sorry the format issues. >>> This is my first patch for this project. Too many details for me to >>> get familiar with. >>> >>> (1) Sure. >>> (2) I'll change that. >>> (3) I'll use the tool to ensure the patch format. >>> (4) I'll remove the ',' in name >>> (5) I'll add more description about it. >>> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >>> instead. The only reason I didn't do it is that I found >>> GetMemorySpaceDescriptor() doesn't return the same information >>> which SetMemorySpaceAttributes() just changed. So I feel using CPU >>> arch protocol is a bit safer. Anyway, I'll change it. >>> (7) I did put those macros in the install function before. To reduce the >>> number of changed files, I made current changes. You're right it's >>> not worthy. >>> (8) Using macro can help the readability, which is more important to me. >>> I know function can do the same. But it looks a bit heavy in this situation. >>> I have to admit replacing the macros with a library is a very good idea, >>> which brings the same readability. I didn't think of that before. Although >>> Library is still a little bit heavy to me but it's in a different way, I think it >>> worth a trying. >>> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >>> (10) You're right. Using library can reduce the disturbs to affected drivers >>> by this feature to the minimum. >>> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Thursday, September 14, 2017 7:35 AM >>> To: Wang, Jian J >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, >>> Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, >>> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, >>> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, >>> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Hi, >>> >>> some of the points I'm going to make have already been pointed out >>> by >>> Jordan: >>> >>> (1) When posting a patch series, please collect the Cc: tags from >>> all of the patches, and add them *all* to the cover letter. This way >>> everyone will get a personal copy of the general description. >>> >>> >>> (2) The subject line is too long. One possible simplification: >>> >>> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >>> >>> >>> On 09/13/17 11:25, Wang, Jian J wrote: >>>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL >>>> pointer detection is enabled, page 0 must be enabled temporarily >>>> before installing and disabled again afterwards. For Windows 7 >>>> boot, >>>> BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >>> >>> (3) Subject line and commit message both should not exceed 74 >>> characters line length. (Not sure how many chars PatchCheck.py >>> actually enforces, I always stick with 74, following the Linux >>> kernel >>> tradition.) >>> >>> I rewrapped the commit message here for readability. >>> >>> >>>> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>> Cc: Justen, Jordan L >>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>>> Cc: Kinney, Michael D >>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>>> Cc: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Suggested-by: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Wang, Jian J >>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> (4) I think this is also something that Jordan had pointed out a >>> long time ago (apologies if I mis-remember): >>> >>> The strings after the tags should form correct email addresses, and >>> if there are various email meta-characters in them, like "." (dot) and "," >>> (comma), then they should be quoted, like this: >>> >>> Cc: "Justen, Jordan L" >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: "Kinney, Michael D" >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Signed-off-by: "Wang, Jian J" >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> If you look at the actual addresses on the emails that have been >>> sent out, you can see they are all malformed. For example, I have: >>> >>> "Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). >>> >>> At least for my reply, I have fixed up the email addresses. >>> >>> >>> (5) The commit message mentions BIT7 of the new PCD. >>> >>> First, thanks for checking Windows 7 boot (and I'm happy that I got >>> suspicious of the feature with regard to Windows 7). I think BIT7 is >>> a good feature. >>> >>> However, please include the short description of that feature in the >>> commit message -- it is one sentence; "Disable NULL pointer >>> detection just after EndOfDxe." >>> >>> (I think BIT7 is a really smart feature, and I like *how* it is used >>> in "NULL_DETECTION_ENABLED" below. The check means, "if the >>> protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >>> >>> This doesn't mean that I like the NULL_DETECTION_ENABLED macro >>> itself; more on that below.) >>> >>> >>>> --- >>>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>>> 3 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c >>>> b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>>> PCI_TYPE00 Pci; >>>> QEMU_VIDEO_CARD *Card; >>>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >>> >>> (6) I believe I mentioned this in the earlier discussion, in some >>> form, but now I'll say it again: >>> >>> A UEFI driver has no business poking at the CPU Arch protocol. The >>> PI spec (1.6) states, >>> >>> 12.3 CPU Architectural Protocol >>> EFI_CPU_ARCH_PROTOCOL >>> >>> Summary >>> >>> Abstracts the processor services that are required to implement some >>> of the DXE services. This protocol must be produced by a boot service >>> or runtime DXE driver and may only be consumed by the DXE Foundation >>> and DXE drivers that produce architectural protocols. >>> >>> The DXE core is obviously free to use the CPU Arch protocol, but a >>> UEFI driver is forbidden from it, *even if* we say that, in this >>> UEFI driver, we are going to use DXE services. (Which come from the >>> PI spec, and not the UEFI spec.) >>> >>> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >>> >>> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >>> protocol, by the PI spec. It is not easy to see, because the PI spec >>> has a formatting error in this area. If you look under >>> GetMemorySpaceDescriptor(), there is an error code >>> >>> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >>> architectural protocol is not available yet. >>> >>> Obviously this error code belongs to SetMemorySpaceAttributes(), not >>> GetMemorySpaceDescriptor(). >>> >>> Anyway, gDS should be used, architectural protocols shouldn't be. >>> >>> >>>> >>>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>>> >>>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined >>>> MDE_CPU_IA32 || defined MDE_CPU_X64 >>>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>>> Private->Variant == QEMU_VIDEO_BOCHS) { >>>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + // >>>> + // Prepare CPU arch protocol for NULL pointer detection >>>> + // >>>> + Status = gBS->LocateProtocol ( >>>> + &gEfiCpuArchProtocolGuid, >>>> + NULL, >>>> + (VOID **) &Cpu >>>> + ); >>>> + ASSERT_EFI_ERROR (Status); >>>> + >>>> + DISABLE_NULL_DETECTION(Cpu); >>>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + ENABLE_NULL_DETECTION(Cpu); >>> >>> (7) The NULL detection disabling and enabling should bracket the >>> affected code as tightly as possible. >>> >>> So please move this into InstallVbeShim() accordingly. >>> >>> >>>> } >>>> #endif >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>>> @@ -25,6 +25,7 @@ >>>> #include <Protocol/PciIo.h> >>>> #include <Protocol/DriverSupportedEfiVersion.h> >>>> #include <Protocol/DevicePath.h> >>>> +#include <Protocol/Cpu.h> >>>> >>>> #include <Library/DebugLib.h> >>>> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ >>>> typedef struct { >>>> >>>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>>> >>>> +// >>>> +// VBE code will access memory between 0-4095 which will cause >>>> +page fault exception // if NULL pointer detection mechanism is enabled. >>>> +Following macros can be used to // disable/enable NULL pointer detection before/after accessing those memory. >>>> +// >>>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>>> + } >>>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>>> + } >>>> + >>> >>> (8) I believe Jordan too commented on these macros elsewhere (under >>> patch 1/4). >>> >>> In my opinion, this functionality should be extracted into a library >>> class, with a library instance that is suitable for at least >>> UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) >>> >>> You could add a separate library instance for SMM drivers, if that >>> were necessary. >>> >>> >>> (9) Style comment: please put one space character between the >>> function designator and the opening parenthesis. >>> >>> >>>> // >>>> // QEMU Video Private Data Structure // diff --git >>>> a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> index 7c7d429bca..5d166eb99c 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> @@ -72,7 +72,9 @@ >>>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>>> + gEfiCpuArchProtocolGuid >>>> >>>> [Pcd] >>>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>>> + >>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMas >>>> + k >>> >>> (10) Instead of these, the library class that I described under (8) >>> should be added here. >>> >>> Any further dependencies like PCDs, protocols etc should be >>> inherited by the driver through the library instance that the >>> platform DSC file resolves the library class to. >>> >>> Bonus: should you realize that the feature is impossible to >>> implement without accessing the CPU Arch protocol directly, you >>> could hide the protocol GUID dependency in the library instance INF >>> file, and I'd be none the wiser. >>> >>> ... Well, I could at least pretend that. :) >>> >>> Thanks, >>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-15 0:15 ` Wang, Jian J @ 2017-09-15 6:05 ` Wang, Jian J 2017-09-15 6:28 ` Zeng, Star 0 siblings, 1 reply; 21+ messages in thread From: Wang, Jian J @ 2017-09-15 6:05 UTC (permalink / raw) To: Wang, Jian J, Zeng, Star, Laszlo Ersek, Yao, Jiewen Cc: Kinney, Michael D, Justen, Jordan L, edk2-devel@lists.01.org, Dong, Eric, Wolman, Ayellet For record, we'll make following changes to address the page attributes issue: (1) Update gDS->SetMemorySpaceAddress() to remove the filter set on page attributes (2) Update CPU driver to sync all memory attributes including page related. No platform code changes needed. Above changes will be submitted in a separate patch. As a result (after it's done), the related changes for this patch will be (3) Replace macros with functions, in which gDS->SetMemorySpaceAttributes() will be used instead of CPU arch method. -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J Sent: Friday, September 15, 2017 8:15 AM To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. gDS-> SetMemorySpaceCapabilities() can change the Capability at any time. But I'm not sure about any side-effect of it. -----Original Message----- From: Zeng, Star Sent: Thursday, September 14, 2017 6:17 PM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. No, I want to say even the GCD is updated to not filter page attributes, it will still not work as all the memory controller code for now are not reporting the Capability. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, September 14, 2017 5:55 PM To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 11:39, Zeng, Star wrote: > There is a realistic problem I just found and want to share. > > GCD database is built based on resource hob reported from PEI, and GCD > checks the input Attributes against Capabilities when setting > Attributes, but we (I just searched the whole codebase) can see there > is no code reporting resource hob with > EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE, > EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or > EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set. ... Are you saying that, if we modify AddMemoryBaseSizeHob() in OvmfPkg/PlatformPei, to report EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE (*), then gDS->SetMemorySpaceAttributes() will start to work? ((*) "MdePkg/Include/Pi/PiHob.h": "NOTE: Since PI spec 1.4, please use EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE as Memory capability attribute: The memory supports being protected from processor writes") If that's the case, then I hope Jian can include such an OvmfPkg patch in his series. :) Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:54 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet > <ayellet.wolman@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 10:46, Wang, Jian J wrote: >> It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). > > That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes. > > Does "git blame" provide a reason, from the message of the commit that added the filtering? > > Thanks, > Laszlo > >> From: Yao, Jiewen >> Sent: Thursday, September 14, 2017 4:38 PM >> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J >> <jian.j.wang@intel.com> >> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet >> <ayellet.wolman@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> HI >> I wonder if it is spec limitation or implementation limitation. >> >> If it is implementation limitation, we can enhance the DxeCore to allow it. >> >> Thank you >> Yao Jiewen >> >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 4:30 PM >> To: Wang, Jian J >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >> Justen, Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >> Michael D >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> On 09/14/17 05:17, Wang, Jian J wrote: >>> For the use of arch protocol, there's one thing I mentioned is not >>> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >>> cannot change page attributes. That's why I have to turn to cpu arch >>> protocol. >> Thank you for the explanation. >> >> In that case, we cannot avoid violating the PI spec. I agree we can >> break the spec if it happens for security purposes, but in that case, >> I believe that hiding the implementation behind a library class is >> mandatory. Viewed from the spec side, accessing the CPU Arch protocol >> is a hack, and so it should not be spread to the source code of >> several driver modules. >> >> Thank you, >> Laszlo >> >> >>> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>> Of Wang, Jian J >>> Sent: Thursday, September 14, 2017 9:17 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>> Justen, Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Thanks for the comments and good advices. Sorry the format issues. >>> This is my first patch for this project. Too many details for me to >>> get familiar with. >>> >>> (1) Sure. >>> (2) I'll change that. >>> (3) I'll use the tool to ensure the patch format. >>> (4) I'll remove the ',' in name >>> (5) I'll add more description about it. >>> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >>> instead. The only reason I didn't do it is that I found >>> GetMemorySpaceDescriptor() doesn't return the same information >>> which SetMemorySpaceAttributes() just changed. So I feel using CPU >>> arch protocol is a bit safer. Anyway, I'll change it. >>> (7) I did put those macros in the install function before. To reduce the >>> number of changed files, I made current changes. You're right it's >>> not worthy. >>> (8) Using macro can help the readability, which is more important to me. >>> I know function can do the same. But it looks a bit heavy in this situation. >>> I have to admit replacing the macros with a library is a very good idea, >>> which brings the same readability. I didn't think of that before. Although >>> Library is still a little bit heavy to me but it's in a different way, I think it >>> worth a trying. >>> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >>> (10) You're right. Using library can reduce the disturbs to affected drivers >>> by this feature to the minimum. >>> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Thursday, September 14, 2017 7:35 AM >>> To: Wang, Jian J >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, >>> Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, >>> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, >>> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, >>> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Hi, >>> >>> some of the points I'm going to make have already been pointed out >>> by >>> Jordan: >>> >>> (1) When posting a patch series, please collect the Cc: tags from >>> all of the patches, and add them *all* to the cover letter. This way >>> everyone will get a personal copy of the general description. >>> >>> >>> (2) The subject line is too long. One possible simplification: >>> >>> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >>> >>> >>> On 09/13/17 11:25, Wang, Jian J wrote: >>>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL >>>> pointer detection is enabled, page 0 must be enabled temporarily >>>> before installing and disabled again afterwards. For Windows 7 >>>> boot, >>>> BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >>> >>> (3) Subject line and commit message both should not exceed 74 >>> characters line length. (Not sure how many chars PatchCheck.py >>> actually enforces, I always stick with 74, following the Linux >>> kernel >>> tradition.) >>> >>> I rewrapped the commit message here for readability. >>> >>> >>>> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>> Cc: Justen, Jordan L >>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>>> Cc: Kinney, Michael D >>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>>> Cc: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Suggested-by: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Wang, Jian J >>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> (4) I think this is also something that Jordan had pointed out a >>> long time ago (apologies if I mis-remember): >>> >>> The strings after the tags should form correct email addresses, and >>> if there are various email meta-characters in them, like "." (dot) and "," >>> (comma), then they should be quoted, like this: >>> >>> Cc: "Justen, Jordan L" >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: "Kinney, Michael D" >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Signed-off-by: "Wang, Jian J" >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> If you look at the actual addresses on the emails that have been >>> sent out, you can see they are all malformed. For example, I have: >>> >>> "Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). >>> >>> At least for my reply, I have fixed up the email addresses. >>> >>> >>> (5) The commit message mentions BIT7 of the new PCD. >>> >>> First, thanks for checking Windows 7 boot (and I'm happy that I got >>> suspicious of the feature with regard to Windows 7). I think BIT7 is >>> a good feature. >>> >>> However, please include the short description of that feature in the >>> commit message -- it is one sentence; "Disable NULL pointer >>> detection just after EndOfDxe." >>> >>> (I think BIT7 is a really smart feature, and I like *how* it is used >>> in "NULL_DETECTION_ENABLED" below. The check means, "if the >>> protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >>> >>> This doesn't mean that I like the NULL_DETECTION_ENABLED macro >>> itself; more on that below.) >>> >>> >>>> --- >>>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>>> 3 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c >>>> b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>>> PCI_TYPE00 Pci; >>>> QEMU_VIDEO_CARD *Card; >>>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >>> >>> (6) I believe I mentioned this in the earlier discussion, in some >>> form, but now I'll say it again: >>> >>> A UEFI driver has no business poking at the CPU Arch protocol. The >>> PI spec (1.6) states, >>> >>> 12.3 CPU Architectural Protocol >>> EFI_CPU_ARCH_PROTOCOL >>> >>> Summary >>> >>> Abstracts the processor services that are required to implement some >>> of the DXE services. This protocol must be produced by a boot service >>> or runtime DXE driver and may only be consumed by the DXE Foundation >>> and DXE drivers that produce architectural protocols. >>> >>> The DXE core is obviously free to use the CPU Arch protocol, but a >>> UEFI driver is forbidden from it, *even if* we say that, in this >>> UEFI driver, we are going to use DXE services. (Which come from the >>> PI spec, and not the UEFI spec.) >>> >>> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >>> >>> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >>> protocol, by the PI spec. It is not easy to see, because the PI spec >>> has a formatting error in this area. If you look under >>> GetMemorySpaceDescriptor(), there is an error code >>> >>> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >>> architectural protocol is not available yet. >>> >>> Obviously this error code belongs to SetMemorySpaceAttributes(), not >>> GetMemorySpaceDescriptor(). >>> >>> Anyway, gDS should be used, architectural protocols shouldn't be. >>> >>> >>>> >>>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>>> >>>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined >>>> MDE_CPU_IA32 || defined MDE_CPU_X64 >>>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>>> Private->Variant == QEMU_VIDEO_BOCHS) { >>>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + // >>>> + // Prepare CPU arch protocol for NULL pointer detection >>>> + // >>>> + Status = gBS->LocateProtocol ( >>>> + &gEfiCpuArchProtocolGuid, >>>> + NULL, >>>> + (VOID **) &Cpu >>>> + ); >>>> + ASSERT_EFI_ERROR (Status); >>>> + >>>> + DISABLE_NULL_DETECTION(Cpu); >>>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + ENABLE_NULL_DETECTION(Cpu); >>> >>> (7) The NULL detection disabling and enabling should bracket the >>> affected code as tightly as possible. >>> >>> So please move this into InstallVbeShim() accordingly. >>> >>> >>>> } >>>> #endif >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>>> @@ -25,6 +25,7 @@ >>>> #include <Protocol/PciIo.h> >>>> #include <Protocol/DriverSupportedEfiVersion.h> >>>> #include <Protocol/DevicePath.h> >>>> +#include <Protocol/Cpu.h> >>>> >>>> #include <Library/DebugLib.h> >>>> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ >>>> typedef struct { >>>> >>>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>>> >>>> +// >>>> +// VBE code will access memory between 0-4095 which will cause >>>> +page fault exception // if NULL pointer detection mechanism is enabled. >>>> +Following macros can be used to // disable/enable NULL pointer detection before/after accessing those memory. >>>> +// >>>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>>> + } >>>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>>> + } >>>> + >>> >>> (8) I believe Jordan too commented on these macros elsewhere (under >>> patch 1/4). >>> >>> In my opinion, this functionality should be extracted into a library >>> class, with a library instance that is suitable for at least >>> UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) >>> >>> You could add a separate library instance for SMM drivers, if that >>> were necessary. >>> >>> >>> (9) Style comment: please put one space character between the >>> function designator and the opening parenthesis. >>> >>> >>>> // >>>> // QEMU Video Private Data Structure // diff --git >>>> a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> index 7c7d429bca..5d166eb99c 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> @@ -72,7 +72,9 @@ >>>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>>> + gEfiCpuArchProtocolGuid >>>> >>>> [Pcd] >>>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>>> + >>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMas >>>> + k >>> >>> (10) Instead of these, the library class that I described under (8) >>> should be added here. >>> >>> Any further dependencies like PCDs, protocols etc should be >>> inherited by the driver through the library instance that the >>> platform DSC file resolves the library class to. >>> >>> Bonus: should you realize that the feature is impossible to >>> implement without accessing the CPU Arch protocol directly, you >>> could hide the protocol GUID dependency in the library instance INF >>> file, and I'd be none the wiser. >>> >>> ... Well, I could at least pretend that. :) >>> >>> Thanks, >>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-15 6:05 ` Wang, Jian J @ 2017-09-15 6:28 ` Zeng, Star 0 siblings, 0 replies; 21+ messages in thread From: Zeng, Star @ 2017-09-15 6:28 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek, Yao, Jiewen Cc: Kinney, Michael D, Justen, Jordan L, edk2-devel@lists.01.org, Dong, Eric, Wolman, Ayellet, Zeng, Star With this approach, please make sure to do more test, for example, check memory map change and OS boot, etc. Thanks, Star -----Original Message----- From: Wang, Jian J Sent: Friday, September 15, 2017 2:06 PM To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. For record, we'll make following changes to address the page attributes issue: (1) Update gDS->SetMemorySpaceAddress() to remove the filter set on page attributes (2) Update CPU driver to sync all memory attributes including page related. No platform code changes needed. Above changes will be submitted in a separate patch. As a result (after it's done), the related changes for this patch will be (3) Replace macros with functions, in which gDS->SetMemorySpaceAttributes() will be used instead of CPU arch method. -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J Sent: Friday, September 15, 2017 8:15 AM To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. gDS-> SetMemorySpaceCapabilities() can change the Capability at any time. But I'm not sure about any side-effect of it. -----Original Message----- From: Zeng, Star Sent: Thursday, September 14, 2017 6:17 PM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. No, I want to say even the GCD is updated to not filter page attributes, it will still not work as all the memory controller code for now are not reporting the Capability. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, September 14, 2017 5:55 PM To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 09/14/17 11:39, Zeng, Star wrote: > There is a realistic problem I just found and want to share. > > GCD database is built based on resource hob reported from PEI, and GCD > checks the input Attributes against Capabilities when setting > Attributes, but we (I just searched the whole codebase) can see there > is no code reporting resource hob with > EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE, > EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or > EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set. ... Are you saying that, if we modify AddMemoryBaseSizeHob() in OvmfPkg/PlatformPei, to report EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE (*), then gDS->SetMemorySpaceAttributes() will start to work? ((*) "MdePkg/Include/Pi/PiHob.h": "NOTE: Since PI spec 1.4, please use EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE as Memory capability attribute: The memory supports being protected from processor writes") If that's the case, then I hope Jian can include such an OvmfPkg patch in his series. :) Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:54 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet > <ayellet.wolman@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 10:46, Wang, Jian J wrote: >> It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c). > > That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes. > > Does "git blame" provide a reason, from the message of the commit that added the filtering? > > Thanks, > Laszlo > >> From: Yao, Jiewen >> Sent: Thursday, September 14, 2017 4:38 PM >> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J >> <jian.j.wang@intel.com> >> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet >> <ayellet.wolman@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> HI >> I wonder if it is spec limitation or implementation limitation. >> >> If it is implementation limitation, we can enhance the DxeCore to allow it. >> >> Thank you >> Yao Jiewen >> >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 4:30 PM >> To: Wang, Jian J >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >> Justen, Jordan L >> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >> Michael D >> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> On 09/14/17 05:17, Wang, Jian J wrote: >>> For the use of arch protocol, there's one thing I mentioned is not >>> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >>> cannot change page attributes. That's why I have to turn to cpu arch >>> protocol. >> Thank you for the explanation. >> >> In that case, we cannot avoid violating the PI spec. I agree we can >> break the spec if it happens for security purposes, but in that case, >> I believe that hiding the implementation behind a library class is >> mandatory. Viewed from the spec side, accessing the CPU Arch protocol >> is a hack, and so it should not be spread to the source code of >> several driver modules. >> >> Thank you, >> Laszlo >> >> >>> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>> Of Wang, Jian J >>> Sent: Thursday, September 14, 2017 9:17 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>> Justen, Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen >>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Thanks for the comments and good advices. Sorry the format issues. >>> This is my first patch for this project. Too many details for me to >>> get familiar with. >>> >>> (1) Sure. >>> (2) I'll change that. >>> (3) I'll use the tool to ensure the patch format. >>> (4) I'll remove the ',' in name >>> (5) I'll add more description about it. >>> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >>> instead. The only reason I didn't do it is that I found >>> GetMemorySpaceDescriptor() doesn't return the same information >>> which SetMemorySpaceAttributes() just changed. So I feel using CPU >>> arch protocol is a bit safer. Anyway, I'll change it. >>> (7) I did put those macros in the install function before. To reduce the >>> number of changed files, I made current changes. You're right it's >>> not worthy. >>> (8) Using macro can help the readability, which is more important to me. >>> I know function can do the same. But it looks a bit heavy in this situation. >>> I have to admit replacing the macros with a library is a very good idea, >>> which brings the same readability. I didn't think of that before. Although >>> Library is still a little bit heavy to me but it's in a different way, I think it >>> worth a trying. >>> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >>> (10) You're right. Using library can reduce the disturbs to affected drivers >>> by this feature to the minimum. >>> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Thursday, September 14, 2017 7:35 AM >>> To: Wang, Jian J >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, >>> Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, >>> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, >>> Michael D >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; >>> Wolman, Ayellet >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, >>> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, >>> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >>> >>> Hi, >>> >>> some of the points I'm going to make have already been pointed out >>> by >>> Jordan: >>> >>> (1) When posting a patch series, please collect the Cc: tags from >>> all of the patches, and add them *all* to the cover letter. This way >>> everyone will get a personal copy of the general description. >>> >>> >>> (2) The subject line is too long. One possible simplification: >>> >>> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >>> >>> >>> On 09/13/17 11:25, Wang, Jian J wrote: >>>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL >>>> pointer detection is enabled, page 0 must be enabled temporarily >>>> before installing and disabled again afterwards. For Windows 7 >>>> boot, >>>> BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >>> >>> (3) Subject line and commit message both should not exceed 74 >>> characters line length. (Not sure how many chars PatchCheck.py >>> actually enforces, I always stick with 74, following the Linux >>> kernel >>> tradition.) >>> >>> I rewrapped the commit message here for readability. >>> >>> >>>> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>> Cc: Justen, Jordan L >>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>>> Cc: Kinney, Michael D >>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>>> Cc: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Suggested-by: Wolman, Ayellet >>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Wang, Jian J >>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> (4) I think this is also something that Jordan had pointed out a >>> long time ago (apologies if I mis-remember): >>> >>> The strings after the tags should form correct email addresses, and >>> if there are various email meta-characters in them, like "." (dot) and "," >>> (comma), then they should be quoted, like this: >>> >>> Cc: "Justen, Jordan L" >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: "Kinney, Michael D" >>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: "Wolman, Ayellet" >>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Signed-off-by: "Wang, Jian J" >>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>> >>> If you look at the actual addresses on the emails that have been >>> sent out, you can see they are all malformed. For example, I have: >>> >>> "Jordan L >>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one). >>> >>> At least for my reply, I have fixed up the email addresses. >>> >>> >>> (5) The commit message mentions BIT7 of the new PCD. >>> >>> First, thanks for checking Windows 7 boot (and I'm happy that I got >>> suspicious of the feature with regard to Windows 7). I think BIT7 is >>> a good feature. >>> >>> However, please include the short description of that feature in the >>> commit message -- it is one sentence; "Disable NULL pointer >>> detection just after EndOfDxe." >>> >>> (I think BIT7 is a really smart feature, and I like *how* it is used >>> in "NULL_DETECTION_ENABLED" below. The check means, "if the >>> protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >>> >>> This doesn't mean that I like the NULL_DETECTION_ENABLED macro >>> itself; more on that below.) >>> >>> >>>> --- >>>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>>> 3 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c >>>> b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>>> PCI_TYPE00 Pci; >>>> QEMU_VIDEO_CARD *Card; >>>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >>> >>> (6) I believe I mentioned this in the earlier discussion, in some >>> form, but now I'll say it again: >>> >>> A UEFI driver has no business poking at the CPU Arch protocol. The >>> PI spec (1.6) states, >>> >>> 12.3 CPU Architectural Protocol >>> EFI_CPU_ARCH_PROTOCOL >>> >>> Summary >>> >>> Abstracts the processor services that are required to implement some >>> of the DXE services. This protocol must be produced by a boot service >>> or runtime DXE driver and may only be consumed by the DXE Foundation >>> and DXE drivers that produce architectural protocols. >>> >>> The DXE core is obviously free to use the CPU Arch protocol, but a >>> UEFI driver is forbidden from it, *even if* we say that, in this >>> UEFI driver, we are going to use DXE services. (Which come from the >>> PI spec, and not the UEFI spec.) >>> >>> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >>> >>> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >>> protocol, by the PI spec. It is not easy to see, because the PI spec >>> has a formatting error in this area. If you look under >>> GetMemorySpaceDescriptor(), there is an error code >>> >>> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >>> architectural protocol is not available yet. >>> >>> Obviously this error code belongs to SetMemorySpaceAttributes(), not >>> GetMemorySpaceDescriptor(). >>> >>> Anyway, gDS should be used, architectural protocols shouldn't be. >>> >>> >>>> >>>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>>> >>>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( #if defined >>>> MDE_CPU_IA32 || defined MDE_CPU_X64 >>>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>>> Private->Variant == QEMU_VIDEO_BOCHS) { >>>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + // >>>> + // Prepare CPU arch protocol for NULL pointer detection >>>> + // >>>> + Status = gBS->LocateProtocol ( >>>> + &gEfiCpuArchProtocolGuid, >>>> + NULL, >>>> + (VOID **) &Cpu >>>> + ); >>>> + ASSERT_EFI_ERROR (Status); >>>> + >>>> + DISABLE_NULL_DETECTION(Cpu); >>>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>>> + ENABLE_NULL_DETECTION(Cpu); >>> >>> (7) The NULL detection disabling and enabling should bracket the >>> affected code as tightly as possible. >>> >>> So please move this into InstallVbeShim() accordingly. >>> >>> >>>> } >>>> #endif >>>> >>>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>>> @@ -25,6 +25,7 @@ >>>> #include <Protocol/PciIo.h> >>>> #include <Protocol/DriverSupportedEfiVersion.h> >>>> #include <Protocol/DevicePath.h> >>>> +#include <Protocol/Cpu.h> >>>> >>>> #include <Library/DebugLib.h> >>>> #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ >>>> typedef struct { >>>> >>>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>>> >>>> +// >>>> +// VBE code will access memory between 0-4095 which will cause >>>> +page fault exception // if NULL pointer detection mechanism is enabled. >>>> +Following macros can be used to // disable/enable NULL pointer detection before/after accessing those memory. >>>> +// >>>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>>> + } >>>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>>> + if (NULL_DETECTION_ENABLED) { \ >>>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>>> + } >>>> + >>> >>> (8) I believe Jordan too commented on these macros elsewhere (under >>> patch 1/4). >>> >>> In my opinion, this functionality should be extracted into a library >>> class, with a library instance that is suitable for at least >>> UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.) >>> >>> You could add a separate library instance for SMM drivers, if that >>> were necessary. >>> >>> >>> (9) Style comment: please put one space character between the >>> function designator and the opening parenthesis. >>> >>> >>>> // >>>> // QEMU Video Private Data Structure // diff --git >>>> a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> index 7c7d429bca..5d166eb99c 100644 >>>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>>> @@ -72,7 +72,9 @@ >>>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>>> + gEfiCpuArchProtocolGuid >>>> >>>> [Pcd] >>>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>>> + >>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMas >>>> + k >>> >>> (10) Instead of these, the library class that I described under (8) >>> should be added here. >>> >>> Any further dependencies like PCDs, protocols etc should be >>> inherited by the driver through the library instance that the >>> platform DSC file resolves the library class to. >>> >>> Bonus: should you realize that the feature is impossible to >>> implement without accessing the CPU Arch protocol directly, you >>> could hide the protocol GUID dependency in the library instance INF >>> file, and I'd be none the wiser. >>> >>> ... Well, I could at least pretend that. :) >>> >>> Thanks, >>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 8:38 ` Yao, Jiewen 2017-09-14 8:46 ` Wang, Jian J @ 2017-09-14 8:52 ` Laszlo Ersek 1 sibling, 0 replies; 21+ messages in thread From: Laszlo Ersek @ 2017-09-14 8:52 UTC (permalink / raw) To: Yao, Jiewen, Wang, Jian J Cc: Dong, Eric, Justen, Jordan L, edk2-devel@lists.01.org, Wolman, Ayellet, Kinney, Michael D, Zeng, Star On 09/14/17 10:38, Yao, Jiewen wrote: > HI > I wonder if it is spec limitation or implementation limitation. > > If it is implementation limitation, we can enhance the DxeCore to allow it. My opinion is that the spec limits the use of the CPU Arch protocol to said core modules because those modules need to "own" the CPU Arch protocol. If random code messes with the CPU Arch protocol, then the system's actual state can diverge from what the DXE core (for example) thinks about the system, and that could be bad. If SetMemorySpaceAttributes() doesn't work due to an implementation limitation, as you say, I agree the implementation should be fixed. However, if the SetMemorySpaceAttributes() spec itself makes the service unsuitable for this, even preventing future PI releases from extending the service backwards-compatibly -- and I can't tell if that's the case --, then the "proper way" to enhance the spec would be to introduce a new DXE service. I believe. Thanks Laszlo > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 4:30 PM > To: Wang, Jian J <jian.j.wang@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > On 09/14/17 05:17, Wang, Jian J wrote: >> For the use of arch protocol, there's one thing I mentioned is not >> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it >> cannot change page attributes. That's why I have to turn to cpu arch >> protocol. > Thank you for the explanation. > > In that case, we cannot avoid violating the PI spec. I agree we can > break the spec if it happens for security purposes, but in that case, I > believe that hiding the implementation behind a library class is > mandatory. Viewed from the spec side, accessing the CPU Arch protocol is > a hack, and so it should not be spread to the source code of several > driver modules. > > Thank you, > Laszlo > > >> >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J >> Sent: Thursday, September 14, 2017 9:17 AM >> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> Thanks for the comments and good advices. Sorry the format issues. >> This is my first patch for this project. Too many details for me to get >> familiar with. >> >> (1) Sure. >> (2) I'll change that. >> (3) I'll use the tool to ensure the patch format. >> (4) I'll remove the ',' in name >> (5) I'll add more description about it. >> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service >> instead. The only reason I didn't do it is that I found >> GetMemorySpaceDescriptor() doesn't return the same information >> which SetMemorySpaceAttributes() just changed. So I feel using CPU >> arch protocol is a bit safer. Anyway, I'll change it. >> (7) I did put those macros in the install function before. To reduce the >> number of changed files, I made current changes. You're right it's >> not worthy. >> (8) Using macro can help the readability, which is more important to me. >> I know function can do the same. But it looks a bit heavy in this situation. >> I have to admit replacing the macros with a library is a very good idea, >> which brings the same readability. I didn't think of that before. Although >> Library is still a little bit heavy to me but it's in a different way, I think it >> worth a trying. >> (9) Putting a space before open parenthesis is forced style? If so, I'll add it. >> (10) You're right. Using library can reduce the disturbs to affected drivers >> by this feature to the minimum. >> >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, September 14, 2017 7:35 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>> >> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. >> >> Hi, >> >> some of the points I'm going to make have already been pointed out by >> Jordan: >> >> (1) When posting a patch series, please collect the Cc: tags from all of >> the patches, and add them *all* to the cover letter. This way everyone >> will get a personal copy of the general description. >> >> >> (2) The subject line is too long. One possible simplification: >> >> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection >> >> >> On 09/13/17 11:25, Wang, Jian J wrote: >>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer >>> detection is enabled, page 0 must be enabled temporarily before >>> installing and disabled again afterwards. For Windows 7 boot, BIT7 of >>> PcdNullPointerDetectionPropertyMask must still be set to avoid hang. >> >> (3) Subject line and commit message both should not exceed 74 characters >> line length. (Not sure how many chars PatchCheck.py actually enforces, I >> always stick with 74, following the Linux kernel tradition.) >> >> I rewrapped the commit message here for readability. >> >> >>> >>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> >>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>> Cc: Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >>> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >>> Cc: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> >> (4) I think this is also something that Jordan had pointed out a long >> time ago (apologies if I mis-remember): >> >> The strings after the tags should form correct email addresses, and if >> there are various email meta-characters in them, like "." (dot) and "," >> (comma), then they should be quoted, like this: >> >> Cc: "Justen, Jordan L" <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> >> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >> Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>> >> Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >> >> If you look at the actual addresses on the emails that have been sent >> out, you can see they are all malformed. For example, I have: >> >> "Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the >> comma was taken to be a separate email address (a malformed one). >> >> At least for my reply, I have fixed up the email addresses. >> >> >> (5) The commit message mentions BIT7 of the new PCD. >> >> First, thanks for checking Windows 7 boot (and I'm happy that I got >> suspicious of the feature with regard to Windows 7). I think BIT7 is a >> good feature. >> >> However, please include the short description of that feature in the >> commit message -- it is one sentence; "Disable NULL pointer detection >> just after EndOfDxe." >> >> (I think BIT7 is a really smart feature, and I like *how* it is used in >> "NULL_DETECTION_ENABLED" below. The check means, "if the protection is >> enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". >> >> This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; >> more on that below.) >> >> >>> --- >>> OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- >>> OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ >>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ >>> 3 files changed, 32 insertions(+), 1 deletion(-) >>> >>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c >>> index 0dce80e59b..ee0eed7214 100644 >>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( >>> PCI_TYPE00 Pci; >>> QEMU_VIDEO_CARD *Card; >>> EFI_PCI_IO_PROTOCOL *ChildPciIo; >>> + EFI_CPU_ARCH_PROTOCOL *Cpu; >> >> (6) I believe I mentioned this in the earlier discussion, in some form, >> but now I'll say it again: >> >> A UEFI driver has no business poking at the CPU Arch protocol. The PI >> spec (1.6) states, >> >> 12.3 CPU Architectural Protocol >> EFI_CPU_ARCH_PROTOCOL >> >> Summary >> >> Abstracts the processor services that are required to implement some >> of the DXE services. This protocol must be produced by a boot service >> or runtime DXE driver and may only be consumed by the DXE Foundation >> and DXE drivers that produce architectural protocols. >> >> The DXE core is obviously free to use the CPU Arch protocol, but a UEFI >> driver is forbidden from it, *even if* we say that, in this UEFI driver, >> we are going to use DXE services. (Which come from the PI spec, and not >> the UEFI spec.) >> >> Therefore, here we have to use gDS->SetMemorySpaceAttributes(). >> >> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch >> protocol, by the PI spec. It is not easy to see, because the PI spec has >> a formatting error in this area. If you look under >> GetMemorySpaceDescriptor(), there is an error code >> >> EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU >> architectural protocol is not available yet. >> >> Obviously this error code belongs to SetMemorySpaceAttributes(), not >> GetMemorySpaceDescriptor(). >> >> Anyway, gDS should be used, architectural protocols shouldn't be. >> >> >>> >>> OldTpl = gBS->RaiseTPL (TPL_CALLBACK); >>> >>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( >>> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 >>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || >>> Private->Variant == QEMU_VIDEO_BOCHS) { >>> - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>> + // >>> + // Prepare CPU arch protocol for NULL pointer detection >>> + // >>> + Status = gBS->LocateProtocol ( >>> + &gEfiCpuArchProtocolGuid, >>> + NULL, >>> + (VOID **) &Cpu >>> + ); >>> + ASSERT_EFI_ERROR (Status); >>> + >>> + DISABLE_NULL_DETECTION(Cpu); >>> + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); >>> + ENABLE_NULL_DETECTION(Cpu); >> >> (7) The NULL detection disabling and enabling should bracket the >> affected code as tightly as possible. >> >> So please move this into InstallVbeShim() accordingly. >> >> >>> } >>> #endif >>> >>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h >>> index 7fbb25b3ef..bb3bc6eb0f 100644 >>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h >>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h >>> @@ -25,6 +25,7 @@ >>> #include <Protocol/PciIo.h> >>> #include <Protocol/DriverSupportedEfiVersion.h> >>> #include <Protocol/DevicePath.h> >>> +#include <Protocol/Cpu.h> >>> >>> #include <Library/DebugLib.h> >>> #include <Library/UefiDriverEntryPoint.h> >>> @@ -82,6 +83,21 @@ typedef struct { >>> >>> #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff >>> >>> +// >>> +// VBE code will access memory between 0-4095 which will cause page fault exception >>> +// if NULL pointer detection mechanism is enabled. Following macros can be used to >>> +// disable/enable NULL pointer detection before/after accessing those memory. >>> +// >>> +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) >>> +#define DISABLE_NULL_DETECTION(Cpu) \ >>> + if (NULL_DETECTION_ENABLED) { \ >>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ >>> + } >>> +#define ENABLE_NULL_DETECTION(Cpu) \ >>> + if (NULL_DETECTION_ENABLED) { \ >>> + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ >>> + } >>> + >> >> (8) I believe Jordan too commented on these macros elsewhere (under >> patch 1/4). >> >> In my opinion, this functionality should be extracted into a library >> class, with a library instance that is suitable for at least UEFI_DRIVER >> modules. (Maybe even for DXE_DRIVER modules.) >> >> You could add a separate library instance for SMM drivers, if that were >> necessary. >> >> >> (9) Style comment: please put one space character between the function >> designator and the opening parenthesis. >> >> >>> // >>> // QEMU Video Private Data Structure >>> // >>> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> index 7c7d429bca..5d166eb99c 100644 >>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >>> @@ -72,7 +72,9 @@ >>> gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START >>> gEfiDevicePathProtocolGuid # PROTOCOL BY_START >>> gEfiPciIoProtocolGuid # PROTOCOL TO_START >>> + gEfiCpuArchProtocolGuid >>> >>> [Pcd] >>> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask >> >> (10) Instead of these, the library class that I described under (8) >> should be added here. >> >> Any further dependencies like PCDs, protocols etc should be inherited by >> the driver through the library instance that the platform DSC file >> resolves the library class to. >> >> Bonus: should you realize that the feature is impossible to implement >> without accessing the CPU Arch protocol directly, you could hide the >> protocol GUID dependency in the library instance INF file, and I'd be >> none the wiser. >> >> ... Well, I could at least pretend that. :) >> >> Thanks, >> Laszlo >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> https://lists.01.org/mailman/listinfo/edk2-devel >> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 1:17 ` Wang, Jian J 2017-09-14 3:17 ` Wang, Jian J @ 2017-09-14 5:50 ` Jordan Justen 2017-09-14 6:52 ` Wang, Jian J 2017-09-14 8:26 ` Laszlo Ersek 2 siblings, 1 reply; 21+ messages in thread From: Jordan Justen @ 2017-09-14 5:50 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek Cc: edk2-devel@lists.01.org, Dong, Eric, Kinney, Michael D, Wolman, Ayellet, Yao, Jiewen, Zeng, Star On 2017-09-13 18:17:26, Wang, Jian J wrote: > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. A macro can sometimes help readibility if it is doing a very common task. I see the macros are only being used in 2 places. (Once each.) In this case I would prefer you to just write the code all out rather than using macros. I don't think it will make the code that much bigger in this case, and it'll be easier to know what the code is actually doing. -Jordan > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum. > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 7:35 AM > To: Wang, Jian J <jian.j.wang@intel.com> > Cc: edk2-devel@lists.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Hi, > > some of the points I'm going to make have already been pointed out by > Jordan: > > (1) When posting a patch series, please collect the Cc: tags from all of > the patches, and add them *all* to the cover letter. This way everyone > will get a personal copy of the general description. > > > (2) The subject line is too long. One possible simplification: > > OvmfPkg/QemuVideoDxe: bypass NULL pointer detection > > > On 09/13/17 11:25, Wang, Jian J wrote: > > QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer > > detection is enabled, page 0 must be enabled temporarily before > > installing and disabled again afterwards. For Windows 7 boot, BIT7 of > > PcdNullPointerDetectionPropertyMask must still be set to avoid hang. > > (3) Subject line and commit message both should not exceed 74 characters > line length. (Not sure how many chars PatchCheck.py actually enforces, I > always stick with 74, following the Linux kernel tradition.) > > I rewrapped the commit message here for readability. > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Justen, Jordan L <jordan.l.justen@intel.com> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > > Cc: Wolman, Ayellet <ayellet.wolman@intel.com> > > Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> > > (4) I think this is also something that Jordan had pointed out a long > time ago (apologies if I mis-remember): > > The strings after the tags should form correct email addresses, and if > there are various email meta-characters in them, like "." (dot) and "," > (comma), then they should be quoted, like this: > > Cc: "Justen, Jordan L" <jordan.l.justen@intel.com> > Cc: "Kinney, Michael D" <michael.d.kinney@intel.com> > Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com> > Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com> > Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com> > > If you look at the actual addresses on the emails that have been sent > out, you can see they are all malformed. For example, I have: > > "Jordan L <jordan.l.justen@intel.com>" for Jordan -- the part before the > comma was taken to be a separate email address (a malformed one). > > At least for my reply, I have fixed up the email addresses. > > > (5) The commit message mentions BIT7 of the new PCD. > > First, thanks for checking Windows 7 boot (and I'm happy that I got > suspicious of the feature with regard to Windows 7). I think BIT7 is a > good feature. > > However, please include the short description of that feature in the > commit message -- it is one sentence; "Disable NULL pointer detection > just after EndOfDxe." > > (I think BIT7 is a really smart feature, and I like *how* it is used in > "NULL_DETECTION_ENABLED" below. The check means, "if the protection is > enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". > > This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; > more on that below.) > > > > --- > > OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- > > OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ > > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ > > 3 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > > index 0dce80e59b..ee0eed7214 100644 > > --- a/OvmfPkg/QemuVideoDxe/Driver.c > > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > > @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( > > PCI_TYPE00 Pci; > > QEMU_VIDEO_CARD *Card; > > EFI_PCI_IO_PROTOCOL *ChildPciIo; > > + EFI_CPU_ARCH_PROTOCOL *Cpu; > > (6) I believe I mentioned this in the earlier discussion, in some form, > but now I'll say it again: > > A UEFI driver has no business poking at the CPU Arch protocol. The PI > spec (1.6) states, > > 12.3 CPU Architectural Protocol > EFI_CPU_ARCH_PROTOCOL > > Summary > > Abstracts the processor services that are required to implement some > of the DXE services. This protocol must be produced by a boot service > or runtime DXE driver and may only be consumed by the DXE Foundation > and DXE drivers that produce architectural protocols. > > The DXE core is obviously free to use the CPU Arch protocol, but a UEFI > driver is forbidden from it, *even if* we say that, in this UEFI driver, > we are going to use DXE services. (Which come from the PI spec, and not > the UEFI spec.) > > Therefore, here we have to use gDS->SetMemorySpaceAttributes(). > > The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch > protocol, by the PI spec. It is not easy to see, because the PI spec has > a formatting error in this area. If you look under > GetMemorySpaceDescriptor(), there is an error code > > EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU > architectural protocol is not available yet. > > Obviously this error code belongs to SetMemorySpaceAttributes(), not > GetMemorySpaceDescriptor(). > > Anyway, gDS should be used, architectural protocols shouldn't be. > > > > > > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > > > @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( > > #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > > Private->Variant == QEMU_VIDEO_BOCHS) { > > - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > > + // > > + // Prepare CPU arch protocol for NULL pointer detection > > + // > > + Status = gBS->LocateProtocol ( > > + &gEfiCpuArchProtocolGuid, > > + NULL, > > + (VOID **) &Cpu > > + ); > > + ASSERT_EFI_ERROR (Status); > > + > > + DISABLE_NULL_DETECTION(Cpu); > > + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > > + ENABLE_NULL_DETECTION(Cpu); > > (7) The NULL detection disabling and enabling should bracket the > affected code as tightly as possible. > > So please move this into InstallVbeShim() accordingly. > > > > } > > #endif > > > > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > > index 7fbb25b3ef..bb3bc6eb0f 100644 > > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > > @@ -25,6 +25,7 @@ > > #include <Protocol/PciIo.h> > > #include <Protocol/DriverSupportedEfiVersion.h> > > #include <Protocol/DevicePath.h> > > +#include <Protocol/Cpu.h> > > > > #include <Library/DebugLib.h> > > #include <Library/UefiDriverEntryPoint.h> > > @@ -82,6 +83,21 @@ typedef struct { > > > > #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff > > > > +// > > +// VBE code will access memory between 0-4095 which will cause page fault exception > > +// if NULL pointer detection mechanism is enabled. Following macros can be used to > > +// disable/enable NULL pointer detection before/after accessing those memory. > > +// > > +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) > > +#define DISABLE_NULL_DETECTION(Cpu) \ > > + if (NULL_DETECTION_ENABLED) { \ > > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ > > + } > > +#define ENABLE_NULL_DETECTION(Cpu) \ > > + if (NULL_DETECTION_ENABLED) { \ > > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ > > + } > > + > > (8) I believe Jordan too commented on these macros elsewhere (under > patch 1/4). > > In my opinion, this functionality should be extracted into a library > class, with a library instance that is suitable for at least UEFI_DRIVER > modules. (Maybe even for DXE_DRIVER modules.) > > You could add a separate library instance for SMM drivers, if that were > necessary. > > > (9) Style comment: please put one space character between the function > designator and the opening parenthesis. > > > > // > > // QEMU Video Private Data Structure > > // > > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > index 7c7d429bca..5d166eb99c 100644 > > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > @@ -72,7 +72,9 @@ > > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > > gEfiPciIoProtocolGuid # PROTOCOL TO_START > > + gEfiCpuArchProtocolGuid > > > > [Pcd] > > gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion > > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > > (10) Instead of these, the library class that I described under (8) > should be added here. > > Any further dependencies like PCDs, protocols etc should be inherited by > the driver through the library instance that the platform DSC file > resolves the library class to. > > Bonus: should you realize that the feature is impossible to implement > without accessing the CPU Arch protocol directly, you could hide the > protocol GUID dependency in the library instance INF file, and I'd be > none the wiser. > > ... Well, I could at least pretend that. :) > > Thanks, > Laszlo ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 5:50 ` Jordan Justen @ 2017-09-14 6:52 ` Wang, Jian J 0 siblings, 0 replies; 21+ messages in thread From: Wang, Jian J @ 2017-09-14 6:52 UTC (permalink / raw) To: Justen, Jordan L, Laszlo Ersek Cc: edk2-devel@lists.01.org, Dong, Eric, Kinney, Michael D, Wolman, Ayellet, Yao, Jiewen, Zeng, Star Sure. I'll change them to functions. -----Original Message----- From: Justen, Jordan L Sent: Thursday, September 14, 2017 1:50 PM To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com> Cc: edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. On 2017-09-13 18:17:26, Wang, Jian J wrote: > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. A macro can sometimes help readibility if it is doing a very common task. I see the macros are only being used in 2 places. (Once each.) In this case I would prefer you to just write the code all out rather than using macros. I don't think it will make the code that much bigger in this case, and it'll be easier to know what the code is actually doing. -Jordan > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum. > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, September 14, 2017 7:35 AM > To: Wang, Jian J <jian.j.wang@intel.com> > Cc: edk2-devel@lists.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. > > Hi, > > some of the points I'm going to make have already been pointed out by > Jordan: > > (1) When posting a patch series, please collect the Cc: tags from all of > the patches, and add them *all* to the cover letter. This way everyone > will get a personal copy of the general description. > > > (2) The subject line is too long. One possible simplification: > > OvmfPkg/QemuVideoDxe: bypass NULL pointer detection > > > On 09/13/17 11:25, Wang, Jian J wrote: > > QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer > > detection is enabled, page 0 must be enabled temporarily before > > installing and disabled again afterwards. For Windows 7 boot, BIT7 of > > PcdNullPointerDetectionPropertyMask must still be set to avoid hang. > > (3) Subject line and commit message both should not exceed 74 characters > line length. (Not sure how many chars PatchCheck.py actually enforces, I > always stick with 74, following the Linux kernel tradition.) > > I rewrapped the commit message here for readability. > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Justen, Jordan L <jordan.l.justen@intel.com> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > > Cc: Wolman, Ayellet <ayellet.wolman@intel.com> > > Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Wang, Jian J <jian.j.wang@intel.com> > > (4) I think this is also something that Jordan had pointed out a long > time ago (apologies if I mis-remember): > > The strings after the tags should form correct email addresses, and if > there are various email meta-characters in them, like "." (dot) and "," > (comma), then they should be quoted, like this: > > Cc: "Justen, Jordan L" <jordan.l.justen@intel.com> > Cc: "Kinney, Michael D" <michael.d.kinney@intel.com> > Cc: "Wolman, Ayellet" <ayellet.wolman@intel.com> > Suggested-by: "Wolman, Ayellet" <ayellet.wolman@intel.com> > Signed-off-by: "Wang, Jian J" <jian.j.wang@intel.com> > > If you look at the actual addresses on the emails that have been sent > out, you can see they are all malformed. For example, I have: > > "Jordan L <jordan.l.justen@intel.com>" for Jordan -- the part before the > comma was taken to be a separate email address (a malformed one). > > At least for my reply, I have fixed up the email addresses. > > > (5) The commit message mentions BIT7 of the new PCD. > > First, thanks for checking Windows 7 boot (and I'm happy that I got > suspicious of the feature with regard to Windows 7). I think BIT7 is a > good feature. > > However, please include the short description of that feature in the > commit message -- it is one sentence; "Disable NULL pointer detection > just after EndOfDxe." > > (I think BIT7 is a really smart feature, and I like *how* it is used in > "NULL_DETECTION_ENABLED" below. The check means, "if the protection is > enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe". > > This doesn't mean that I like the NULL_DETECTION_ENABLED macro itself; > more on that below.) > > > > --- > > OvmfPkg/QemuVideoDxe/Driver.c | 15 ++++++++++++++- > > OvmfPkg/QemuVideoDxe/Qemu.h | 16 ++++++++++++++++ > > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ > > 3 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > > index 0dce80e59b..ee0eed7214 100644 > > --- a/OvmfPkg/QemuVideoDxe/Driver.c > > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > > @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart ( > > PCI_TYPE00 Pci; > > QEMU_VIDEO_CARD *Card; > > EFI_PCI_IO_PROTOCOL *ChildPciIo; > > + EFI_CPU_ARCH_PROTOCOL *Cpu; > > (6) I believe I mentioned this in the earlier discussion, in some form, > but now I'll say it again: > > A UEFI driver has no business poking at the CPU Arch protocol. The PI > spec (1.6) states, > > 12.3 CPU Architectural Protocol > EFI_CPU_ARCH_PROTOCOL > > Summary > > Abstracts the processor services that are required to implement some > of the DXE services. This protocol must be produced by a boot service > or runtime DXE driver and may only be consumed by the DXE Foundation > and DXE drivers that produce architectural protocols. > > The DXE core is obviously free to use the CPU Arch protocol, but a UEFI > driver is forbidden from it, *even if* we say that, in this UEFI driver, > we are going to use DXE services. (Which come from the PI spec, and not > the UEFI spec.) > > Therefore, here we have to use gDS->SetMemorySpaceAttributes(). > > The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch > protocol, by the PI spec. It is not easy to see, because the PI spec has > a formatting error in this area. If you look under > GetMemorySpaceDescriptor(), there is an error code > > EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU > architectural protocol is not available yet. > > Obviously this error code belongs to SetMemorySpaceAttributes(), not > GetMemorySpaceDescriptor(). > > Anyway, gDS should be used, architectural protocols shouldn't be. > > > > > > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > > > @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( > > #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > > Private->Variant == QEMU_VIDEO_BOCHS) { > > - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > > + // > > + // Prepare CPU arch protocol for NULL pointer detection > > + // > > + Status = gBS->LocateProtocol ( > > + &gEfiCpuArchProtocolGuid, > > + NULL, > > + (VOID **) &Cpu > > + ); > > + ASSERT_EFI_ERROR (Status); > > + > > + DISABLE_NULL_DETECTION(Cpu); > > + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase); > > + ENABLE_NULL_DETECTION(Cpu); > > (7) The NULL detection disabling and enabling should bracket the > affected code as tightly as possible. > > So please move this into InstallVbeShim() accordingly. > > > > } > > #endif > > > > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > > index 7fbb25b3ef..bb3bc6eb0f 100644 > > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > > @@ -25,6 +25,7 @@ > > #include <Protocol/PciIo.h> > > #include <Protocol/DriverSupportedEfiVersion.h> > > #include <Protocol/DevicePath.h> > > +#include <Protocol/Cpu.h> > > > > #include <Library/DebugLib.h> > > #include <Library/UefiDriverEntryPoint.h> > > @@ -82,6 +83,21 @@ typedef struct { > > > > #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER 0xffff > > > > +// > > +// VBE code will access memory between 0-4095 which will cause page fault exception > > +// if NULL pointer detection mechanism is enabled. Following macros can be used to > > +// disable/enable NULL pointer detection before/after accessing those memory. > > +// > > +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) > > +#define DISABLE_NULL_DETECTION(Cpu) \ > > + if (NULL_DETECTION_ENABLED) { \ > > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0); \ > > + } > > +#define ENABLE_NULL_DETECTION(Cpu) \ > > + if (NULL_DETECTION_ENABLED) { \ > > + (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); \ > > + } > > + > > (8) I believe Jordan too commented on these macros elsewhere (under > patch 1/4). > > In my opinion, this functionality should be extracted into a library > class, with a library instance that is suitable for at least UEFI_DRIVER > modules. (Maybe even for DXE_DRIVER modules.) > > You could add a separate library instance for SMM drivers, if that were > necessary. > > > (9) Style comment: please put one space character between the function > designator and the opening parenthesis. > > > > // > > // QEMU Video Private Data Structure > > // > > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > index 7c7d429bca..5d166eb99c 100644 > > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > @@ -72,7 +72,9 @@ > > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > > gEfiPciIoProtocolGuid # PROTOCOL TO_START > > + gEfiCpuArchProtocolGuid > > > > [Pcd] > > gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion > > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > > (10) Instead of these, the library class that I described under (8) > should be added here. > > Any further dependencies like PCDs, protocols etc should be inherited by > the driver through the library instance that the platform DSC file > resolves the library class to. > > Bonus: should you realize that the feature is impossible to implement > without accessing the CPU Arch protocol directly, you could hide the > protocol GUID dependency in the library instance INF file, and I'd be > none the wiser. > > ... Well, I could at least pretend that. :) > > Thanks, > Laszlo ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. 2017-09-14 1:17 ` Wang, Jian J 2017-09-14 3:17 ` Wang, Jian J 2017-09-14 5:50 ` Jordan Justen @ 2017-09-14 8:26 ` Laszlo Ersek 2 siblings, 0 replies; 21+ messages in thread From: Laszlo Ersek @ 2017-09-14 8:26 UTC (permalink / raw) To: Wang, Jian J Cc: edk2-devel@lists.01.org, Justen, Jordan L, Dong, Eric, Kinney, Michael D, Wolman, Ayellet, Yao, Jiewen, Zeng, Star On 09/14/17 03:17, Wang, Jian J wrote: > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. Yes, it is in the CCS: https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/52_spacing.html#52-spacing > 5.2.2.6 Always put space before an open parenthesis > > The only exception is macro definitions. > > if (... > while (... > EfiLibAllocateCopyPool (... Thanks, Laszlo > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-09-15 6:26 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-13 8:07 [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled Wang, Jian J
[not found] <Implement NULL pointer detection feature>
2017-09-13 9:25 ` [PATCH 0/4] Implement NULL pointer detection feature for special pool Wang, Jian J
2017-09-13 9:25 ` [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled Wang, Jian J
2017-09-13 16:33 ` Johnson, Brian (EXL - Eagan)
2017-09-13 23:34 ` Laszlo Ersek
2017-09-14 1:17 ` Wang, Jian J
2017-09-14 3:17 ` Wang, Jian J
2017-09-14 8:30 ` Laszlo Ersek
2017-09-14 8:38 ` Yao, Jiewen
2017-09-14 8:46 ` Wang, Jian J
2017-09-14 8:48 ` Yao, Jiewen
2017-09-14 8:54 ` Laszlo Ersek
2017-09-14 9:39 ` Zeng, Star
2017-09-14 9:55 ` Laszlo Ersek
2017-09-14 10:16 ` Zeng, Star
2017-09-15 0:15 ` Wang, Jian J
2017-09-15 6:05 ` Wang, Jian J
2017-09-15 6:28 ` Zeng, Star
2017-09-14 8:52 ` Laszlo Ersek
2017-09-14 5:50 ` Jordan Justen
2017-09-14 6:52 ` Wang, Jian J
2017-09-14 8:26 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox