This patch looks great to me, I would like to request a few small updates: On 09/21/17 07:20, Jian J Wang wrote: > QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer
(1) please replace the word "install" with "link". The VBE Shim is technically installed into the "real-mode" C segment, only the int 0x10 vector lives in page 0. > detection is enabled, this driver will fail to load. NULL pointer detection > bypassing code is added to prevent such problem during boot. > > Please note that Windows 7 will try to access VBE SHIM during boot if it's > installed, and then cause boot failure. This can be fixed by setting BIT7 > of PcdNullPointerDetectionPropertyMask to disable NULL pointer detection > after EndOfDxe. As far as we know, there's no other OSs has such issue. This is not a request, just a comment: I verified the default value in the .dec, and I see it is 0. So there's no need to post an additional patch for the OVMF DSC files, in order to set BIT7. > > Cc: Star Zeng <[email protected]> > Cc: Eric Dong <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Cc: Jiewen Yao <[email protected]> > Cc: Michael Kinney <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Ayellet Wolman <[email protected]> > Suggested-by: Ayellet Wolman <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <[email protected]> > --- > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 2 ++ > OvmfPkg/QemuVideoDxe/VbeShim.c | 22 ++++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > index 577e07b0a8..8078232ded 100644 > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > @@ -67,6 +67,7 @@ > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiLib > + DxeServicesTableLib > > [Protocols] > gEfiDriverSupportedEfiVersionProtocolGuid # PROTOCOL ALWAYS_PRODUCED > @@ -77,3 +78,4 @@ > [Pcd] > gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c > index e45a08e887..c3fb6d8d3c 100644 > --- a/OvmfPkg/QemuVideoDxe/VbeShim.c > +++ b/OvmfPkg/QemuVideoDxe/VbeShim.c > @@ -21,10 +21,13 @@ > WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > **/ > > +#include <Pi/PiDxeCis.h> (2) Question: what exactly is this necessary for? (I would think that "DxeServicesTableLib.h" gave you everything you needed, but I could be wrong.) > #include <IndustryStandard/LegacyVgaBios.h> > #include <Library/DebugLib.h> > #include <Library/PciLib.h> > #include <Library/PrintLib.h> > +#include <Library/DxeServicesTableLib.h> > + > #include <OvmfPlatforms.h> > > #include "Qemu.h" > @@ -74,11 +77,21 @@ InstallVbeShim ( > UINT8 *Ptr; > UINTN Printed; > VBE_MODE_INFO *VbeModeInfo; > + EFI_STATUS Status; > > Segment0 = 0x00000; > SegmentC = 0xC0000; > SegmentF = 0xF0000; > > + // > + // Disable NULL pointer detection temporarily. Otherwise the installation > + // will fail due to the lack of memory access right. > + // > + if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) > { > + Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE (1), 0); > + ASSERT_EFI_ERROR (Status); > + } > + (3) Please hoist the Segment0Pages = 1; assignment from below, and use it in the SetMemorySpaceAttributes() call, for the "Length" argument. (4) Please use the variable "Segment0" as the "BaseAddress" argument. (5) The Attributes=0 argument surprises me, and the end of this patch seems to confirm that I'm right to be surprised :) Instead of setting 0, can you please - first get the original attributes with GetMemorySpaceDescriptor(), - then clear only the attributes here that prevent read/write access, - and at the end of the function, restore the original attributes? I think this can be done without dynamic memory allocation, you just need a local EFI_GCD_MEMORY_SPACE_DESCRIPTOR object. > // > // Attempt to cover the real mode IVT with an allocation. This is a UEFI > // driver, hence the arch protocols have been installed previously. Among > @@ -304,5 +317,14 @@ InstallVbeShim ( > Int0x10->Segment = (UINT16) ((UINT32)SegmentC >> 4); > Int0x10->Offset = (UINT16) ((UINTN) (VbeModeInfo + 1) - SegmentC); > > + // > + // Get NULL pointer detection back > + // > + if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) > { > + Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1), > + EFI_MEMORY_RP); > + ASSERT_EFI_ERROR (Status); > + } > + > DEBUG ((EFI_D_INFO, "%a: VBE shim installed\n", __FUNCTION__)); > } > (6) The InstallVbeShim() function actually contains *two* earlier exits than this. Please search the function for "return" statements. I suggest the following: - put the restoration of the original page attributes at the very end of the function, - put a label called "RestoreSegment0Attributes" between the DEBUG message and the page attributes restoration, - replace the "return" statements with "goto RestoreSegment0Attributes". Thanks! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
