From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0D8BA21DF8078 for ; Wed, 13 Sep 2017 22:47:23 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP; 13 Sep 2017 22:50:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,391,1500966000"; d="scan'208";a="1218609931" Received: from cwbutler-mobl.amr.corp.intel.com (HELO localhost) ([10.254.13.190]) by fmsmga002.fm.intel.com with ESMTP; 13 Sep 2017 22:50:20 -0700 MIME-Version: 1.0 To: "Wang, Jian J" , Laszlo Ersek Message-ID: <150536822038.12853.16381995043242387680@jljusten-skl> From: Jordan Justen In-Reply-To: Cc: "edk2-devel@lists.01.org" , "Dong, Eric" , "Kinney, Michael D" , "Wolman, Ayellet" , "Yao, Jiewen" , "Zeng, Star" References: <20170913092507.12504-1-jian.j.wang@intel.com> <20170913092507.12504-5-jian.j.wang@intel.com> <848bc70b-b3f7-62fb-ef2d-df89ff8805a6@redhat.com> User-Agent: alot/0.6 Date: Wed, 13 Sep 2017 22:50:20 -0700 Subject: Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Sep 2017 05:47:23 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 sit= uation. 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 i= dea, = > which brings the same readability. I didn't think of that before. Alt= hough = > 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 driv= ers > 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 > Cc: edk2-devel@lists.01.org; Justen, Jordan L = ; Dong, Eric ; Kinney, Michael D ; Wolman, Ayellet ; Yao, Jiewen ; Zeng, Star > 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 > > Cc: Eric Dong > > Cc: Star Zeng > > Cc: Laszlo Ersek > > Cc: Justen, Jordan L > > Cc: Kinney, Michael D > > Cc: Wolman, Ayellet > > Suggested-by: Wolman, Ayellet > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Wang, Jian J > = > (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" > Cc: "Kinney, Michael D" > Cc: "Wolman, Ayellet" > Suggested-by: "Wolman, Ayellet" > Signed-off-by: "Wang, Jian J" > = > 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 " 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* (=3D=3D also enabled) after End-of-Dx= e". > = > 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/Drive= r.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 =3D gBS->RaiseTPL (TPL_CALLBACK); > > > > @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart ( > > #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > > if (Private->Variant =3D=3D QEMU_VIDEO_BOCHS_MMIO || > > Private->Variant =3D=3D QEMU_VIDEO_BOCHS) { > > - InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBuf= ferBase); > > + // > > + // Prepare CPU arch protocol for NULL pointer detection > > + // > > + Status =3D gBS->LocateProtocol ( > > + &gEfiCpuArchProtocolGuid, > > + NULL, > > + (VOID **) &Cpu > > + ); > > + ASSERT_EFI_ERROR (Status); > > + > > + DISABLE_NULL_DETECTION(Cpu); > > + InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameB= ufferBase); > > + 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 > > #include > > #include > > +#include > > > > #include > > #include > > @@ -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 fa= ult exception > > +// if NULL pointer detection mechanism is enabled. Following macros ca= n be used to > > +// disable/enable NULL pointer detection before/after accessing those = memory. > > +// > > +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPrope= rtyMask) & (BIT0|BIT7)) =3D=3D 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/QemuVideoD= xe/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