Re: ASSERT in QemuVideoDxe driver during reset

["Yao, Jiewen" <jiewen.yao@intel.com>]

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.

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.

Thank you
Yao Jiewen

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