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 <[email protected]>; Wang, Jian J <[email protected]> > Cc: Dong, Eric <[email protected]>; Justen, Jordan L > <[email protected]>; [email protected]; Wolman, Ayellet > <[email protected]>; Kinney, Michael D <[email protected]>; > Zeng, Star <[email protected]> > 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:[email protected]] > Sent: Thursday, September 14, 2017 4:30 PM > To: Wang, Jian J <[email protected]<mailto:[email protected]>> > Cc: Dong, Eric <[email protected]<mailto:[email protected]>>; Justen, > Jordan L <[email protected]<mailto:[email protected]>>; > [email protected]<mailto:[email protected]>; Yao, Jiewen > <[email protected]<mailto:[email protected]>>; Wolman, Ayellet > <[email protected]<mailto:[email protected]>>; Kinney, Michael > D <[email protected]<mailto:[email protected]>>; Zeng, Star > <[email protected]<mailto:[email protected]>> > 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:[email protected]] On Behalf Of Wang, >> Jian J >> Sent: Thursday, September 14, 2017 9:17 AM >> To: Laszlo Ersek <[email protected]<mailto:[email protected]>> >> Cc: Dong, Eric <[email protected]<mailto:[email protected]>>; Justen, >> Jordan L <[email protected]<mailto:[email protected]>>; >> [email protected]<mailto:[email protected]>; Yao, Jiewen >> <[email protected]<mailto:[email protected]>>; Wolman, Ayellet >> <[email protected]<mailto:[email protected]>>; Kinney, Michael >> D <[email protected]<mailto:[email protected]>>; Zeng, >> Star <[email protected]<mailto:[email protected]>> >> 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:[email protected]] >> Sent: Thursday, September 14, 2017 7:35 AM >> To: Wang, Jian J <[email protected]<mailto:[email protected]>> >> Cc: [email protected]<mailto:[email protected]>; Justen, Jordan >> L <[email protected]<mailto:[email protected]>>; Dong, Eric >> <[email protected]<mailto:[email protected]>>; Kinney, Michael D >> <[email protected]<mailto:[email protected]>>; Wolman, >> Ayellet <[email protected]<mailto:[email protected]>>; Yao, >> Jiewen <[email protected]<mailto:[email protected]>>; Zeng, Star >> <[email protected]<mailto:[email protected]>> >> 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 <[email protected]<mailto:[email protected]>> >>> Cc: Eric Dong <[email protected]<mailto:[email protected]>> >>> Cc: Star Zeng <[email protected]<mailto:[email protected]>> >>> Cc: Laszlo Ersek <[email protected]<mailto:[email protected]>> >>> Cc: Justen, Jordan L >>> <[email protected]<mailto:[email protected]>> >>> Cc: Kinney, Michael D >>> <[email protected]<mailto:[email protected]>> >>> Cc: Wolman, Ayellet >>> <[email protected]<mailto:[email protected]>> >>> Suggested-by: Wolman, Ayellet >>> <[email protected]<mailto:[email protected]>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Wang, Jian J >>> <[email protected]<mailto:[email protected]>> >> >> (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" >> <[email protected]<mailto:[email protected]>> >> Cc: "Kinney, Michael D" >> <[email protected]<mailto:[email protected]>> >> Cc: "Wolman, Ayellet" >> <[email protected]<mailto:[email protected]>> >> Suggested-by: "Wolman, Ayellet" >> <[email protected]<mailto:[email protected]>> >> Signed-off-by: "Wang, Jian J" >> <[email protected]<mailto:[email protected]>> >> >> 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 <[email protected]<mailto:[email protected]>>" 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 >> [email protected]<mailto:[email protected]> >> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
