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]> Cc: Dong, Eric <[email protected]>; Justen, Jordan L <[email protected]>; [email protected]; Yao, Jiewen <[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. 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
