Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

["Wang, Jian J" <jian.j.wang@intel.com>]

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