Re: ASSERT in QemuVideoDxe driver during reset

["Gao, Liming" <liming.gao@intel.com>]

Laszlo:

> * Independently: if the CSM too conflicts with the null detection
> feature, then we should probably report an "unsupported configuration"
> error for the following case:
> 
>   -D NULL_DETECT_ENABLE -D CSM_ENABLE
> 
> But, I don't know if triggering build errors, with custom error
> messages, is possible in DSC files.

Now, there is no support to report the error message for the invalid combination. I think this is a good request to BaseTools. We can introduce !error "error message" syntax in DSC/FDF. If the invalid combination happen, !error will trig build error message. Could you help submit this feature request in BugZillar?

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Wednesday, September 6, 2017 9:19 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] ASSERT in QemuVideoDxe driver during reset
> 
> On 09/06/17 13:16, Yao, Jiewen wrote:
> > HI
> > I think the NULL detection feature should *never* break any existing platform.
> > No real function should be skipped for PcdNullDetection.
> >
> > If InstallVbeShim () is needed for CSM, we should always call InstallVbeShim() for CSM.
> 
> Wait a second please -- the VBE shim exists so that we can boot Windows7
> on OVMF *without* the CSM.
> 
> If you build OVMF with the SeaBIOS CSM, then Windows7 will simply boot
> in legacy mode, and work as usual.
> 
> But if you build OVMF without a CSM, then Windows7 will break mid-way
> into booting. The reason is that Windows7 insists on some legacy VBE
> services even if it is booted on a pure UEFI system. By faking a minimal
> set of legacy VBE services, Windows7 boots just fine in UEFI mode.
> 
> (Windows 7 doesn't actually execute the VBE services' real mode code on
> the processor; instead the code is interpreted in a software emulator.)
> 
> The dependency on legacy VBE services is arguably a bug of UEFI Windows
> 7. One way to work it around is to build OVMF with the SeaBIOS CSM, and
> just boot Windows7 in legacy mode.
> 
> However, in some environments, building OVMF with a CSM is not
> desirable. That's why we created the VBE shim. It is a terrible hack,
> but it fits the Windows7 bug just fine.
> 
> > I suggest below options:
> >
> > 1)       In OVMF, if CSM is enabled, disable PcdNullDetection.
> >
> > 2)       In OVMF, if any driver need access first 4K page, the code should explicit call SetAttribute(0, 4K, READ|WRITE), if
> PcdNullDetection is enabled.
> >
> > Either #1 or #2 is OK.
> > #1 is simple, and #2 can help detect more potential issue.
> 
> * Regarding #1:
> 
> As I wrote above, the VBE shim is mutually exclusive with CSM:
> 
>   //
>   // The allocation request may fail, eg. if LegacyBiosDxe has already run.
>   //
> 
> So, if the CSM was built into OVMF, then the VBE shim is already skipped
> at boot.
> 
> 
> * Regarding #2:
> 
> I agree that *temporarily* enabling R/W access to page zero, using the
> appropriate DXE service, could make the VBE Shim installation work.
> 
> However, making page 0 unaccessible right after, could very easily break
> Windows 7, if some component of Windows 7 accessed page 0, before
> setting up new page tables. After all, in the InstallVbeShim() function,
> we write stuff to page 0 *because* Windows 7 will read it from there.
> 
> 
> * If it is more convenient than a "--pcd=..." option for "build", we can
> also add another build-time macro to the OVMF DSC files, called
> NULL_DETECT_ENABLE. This would then turn on the PcdNullDetection bits,
> for "-D NULL_DETECT_ENABLE".
> 
> NULL_DETECT_ENABLE should default to FALSE.
> 
> 
> * Independently: if the CSM too conflicts with the null detection
> feature, then we should probably report an "unsupported configuration"
> error for the following case:
> 
>   -D NULL_DETECT_ENABLE -D CSM_ENABLE
> 
> But, I don't know if triggering build errors, with custom error
> messages, is possible in DSC files.
> 
> Thanks
> Laszlo
> 
> 
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, September 6, 2017 6:21 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: Re: ASSERT in QemuVideoDxe driver during reset
> >
> > On 09/06/17 10:15, Wang, Jian J wrote:
> >> Hi guys,
> >>
> >> I found an ASSERT issue in function InstallVbeShim() in QemuVideoDxe
> >> driver during reset. The assert statement is like below.
> >>
> >>     ASSERT (Int0x10->Segment == 0x0000);
> >>     ASSERT (Int0x10->Offset  == 0x0000);
> >>
> >> This happened after I have enabled NULL pointer access detection
> >> feature, in which page 0 (4K)  is disabled.
> >
> > The NULL pointer access detection conflicts with the VBE shim. For
> > installing the VBE shim, OVMF has to write to page 0, on purpose.
> >
> > Please see commit 90803342b1b6 ("OvmfPkg: QemuVideoDxe: Int10h stub for
> > Windows 7 & 2008 (stdvga, QXL)", 2014-05-20).
> >
> >> And because of page 0 disabled, I have to skip the memory clearing for
> >> page 0 in DXE core.
> >
> > By doing that, you invalidate the following comment in said OVMF commit:
> >
> > +    //
> > +    // We managed to allocate the page at zero. SVN r14218 guarantees that it
> > +    // is NUL-filled.
> > +    //
> > +    ASSERT (Int0x10->Segment == 0x0000);
> > +    ASSERT (Int0x10->Offset  == 0x0000);
> >
> > Because SVN r14218 was what added the clearing of page 0 to the DXE
> > core. (Expressed as a git commit: d436d5ca0936.)
> >
> >> Otherwise it will cause page fault exception there. It seems that QEMU
> >> may clear all its memory at startup. Skipping the action of clearing
> >> page 0 in core won't cause ASSERT issue in QemuVideoDxe, for the first
> >> time boot. But QemuVideoDxe will write int10 vector at memory 0x10 and
> >> QEMU will not clear all its memory during warm boot. ASSERT will be
> >> triggered after reset.
> >
> > QEMU does not clear guest RAM at reboot (or S3 resume).
> >
> >> It's easy to fix this issue but there're some subtle situations which
> >> I'm not quite certain. I'd like your opinions for them.
> >>
> >> Here're my thoughts on several solutions:
> >> a) Remove the ASSERT statement in InstallVbeShim().
> >
> > Not a good idea.
> >
> >> But I'm sure if it is safe to do so because I don't quite understand
> >> the purpose of the ASSERT.
> >
> > The ASSERT expresses our belief that, after we manage to *allocate* page
> > 0, we can freely write the real mode Int0x10 vector there, without
> > overwriting anything else.
> >
> >> b) Instead of skipping clearing page 0, enable it, do clearing and
> >> then disable it. The problem here is that CPU arch protocol is not
> >> ready at that time. I have to "manually" do page operation, which
> >> might be non-portable and a little bit odd in DXE core.
> >> c) Move code clearing page 0 from DXE core to another place wherever
> >> appropriate, like DxeIpl or cpu driver. But I think there's a good
> >> reason to put code there before.
> >
> > I'm not sure how the NULL pointer access detection is enabled. Is it
> > enabled by a PCD?
> >
> > Hm, yes, it seems, from your patch
> >
> >   [edk2] [PATCH 1/2] Implement NULL pointer detection for EDK-II Core
> >
> > that the PCD is called
> >
> >   gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection
> >
> > I can see in that patch that the DXE core is updated *not* to clear page
> > 0 if the PCD is set to TRUE. That makes sense.
> >
> > However, OVMF should also be updated to skip the VBE shim installation
> > if the PCD is set to TRUE. Namely, if the PCD is set to TRUE, then we
> > simply cannot put the legacy Int0x10 vector in place, so we shouldn't
> > even try.
> >
> > Please extend your original patch set with a patch for OvmfPkg. In the
> > file "OvmfPkg/QemuVideoDxe/Driver.c", locate the InstallVbeShim() call
> > site:
> >
> >> #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);
> >>   }
> >> #endif
> >
> > and please skip the call if the PCD is set to TRUE:
> >
> >> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64
> >>   if (!FeaturePcdGet (PcdNullPointerDetection) &&
> >>       (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
> >>        Private->Variant == QEMU_VIDEO_BOCHS)) {
> >>     InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase);
> >>   }
> >> #endif
> >
> > Now, the consequence of this would be a working OVMF build, but it would
> > also break booting UEFI Windows 7 on OVMF. Therefore, please append
> > *another* patch to your patch set, setting the Feature PCD to FALSE in
> > all three OVMF DSC files:
> >
> > - OvmfPkg/OvmfPkgIa32.dsc
> > - OvmfPkg/OvmfPkgIa32X64.dsc
> > - OvmfPkg/OvmfPkgX64.dsc
> >
> > We cannot break Windows 7 booting in upstream OVMF. If someone really
> > wants the null pointer detection in OVMF (and doesn't need Windows 7 for
> > sure), they can patch the DSC files, or else pass the
> >
> >   --pcd=gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection=TRUE
> >
> > option to the "build" utility.
> >
> > ... I'm now seeing in the original discussion that perhaps you will
> > introduce a bitmap instead (one bit per firmware phase). That's OK with
> > me; please reinterpret all of the above with the "DXE bit" then.
> >
> > When you post the next version of the patch set, please CC Jordan and
> > myself on the OvmfPkg patches.
> >
> > Thank you!
> > Laszlo
> >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel