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....@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Star Zeng <star.z...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Justen, Jordan L <jordan.l.jus...@intel.com>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Wolman, Ayellet <ayellet.wol...@intel.com>
> Suggested-by: Wolman, Ayellet <ayellet.wol...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J <jian.j.w...@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.jus...@intel.com>
Cc: "Kinney, Michael D" <michael.d.kin...@intel.com>
Cc: "Wolman, Ayellet" <ayellet.wol...@intel.com>
Suggested-by: "Wolman, Ayellet" <ayellet.wol...@intel.com>
Signed-off-by: "Wang, Jian J" <jian.j.w...@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.jus...@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
