Re: [edk2] [PATCH] OvmfPkg: QemuVideoDxe: Int10h stub for Windows 2008 R2 SP1 (stdvga, QXL)

["Laszlo Ersek" <lersek@redhat.com>]

Hi David,

On 06/17/19 12:52, David Woodhouse wrote:
> On Mon, 2014-05-12 at 20:21 +0400, Mike Maslenkin wrote:
>>>
>>>>> +  Segment0      = 0;
>>>>> +  Segment0Pages = 1;
>>>>> +  Status = gBS->AllocatePages (AllocateAddress, EfiReservedMemoryType,
>>>>> +                  Segment0Pages, &Segment0);
>>>>> +  if (EFI_ERROR (Status)) {
>>>>> +    goto RestorePam1;
>>>>> +  }
>>>>
>>>> If CSM is enabled, we will fail to allocate, right?
>>
>> Allocation at LegacyBiosInstall() function will fail, but no one cares
>> about it and MemoryAddress remains uninitialized. This is because uefi
>> video driver is being initialized earlier.
> 
> Right... at the time the above code runs, the CSM has already been
> initialised and installed a stub 'iret' handler for INT 10h, which in
> my case happens to be at F000:F065.
> 
> Because the CSM chose to put that in the F-segment not the E-segment,
> that happens not to trigger the check for an existing handler:
> 
>     //
>     // Check if a video BIOS handler has been installed previously -- we
>     // shouldn't override a real video BIOS with our shim, nor our own shim if
>     // it's already present.
>     //
>     Handler = (Int0x10->Segment << 4) + Int0x10->Offset;
>     if (Handler >= SegmentC && Handler < SegmentF) {
>       DEBUG ((EFI_D_INFO, "%a: Video BIOS handler found at %04x:%04x\n",
>         __FUNCTION__, Int0x10->Segment, Int0x10->Offset));
>       return;
>     }
> 
> So InstallVbeShim() goes ahead and copies the shim to the C-segment and
> points the INT10 vector to it (at C000:0200 it seems).
> 
> Later, LegacyBiosInstallRom() shadows the video OpROM, stomping on the
> shim. The very *next* thing it does before actually invoking the newly-
> shadowed OpROM is make an INT 10h call to put the display into a plain
> text mode. Which blows up since there's nothing useful at C000:0200 any
> more.
> 
> 
> There are a few ways we could fix this...
> 
> If I just move that PrepareToScanRom hook invocation (that sets the
> text mode) to happen before the CopyMem() of the shadowing, that makes
> things work again. But mostly by luck.
> 
> If I change the check in InstallVbeShim() to be '<= SegmentF' then the
> VBE shim won't install itself even over the CSM's iret stub. This is
> basically equivalent to making the VBE Shim refuse to install if
> CSM_ENABLE is set. And might be the right thing to do, since the VBE
> Shim isn't enough to actually make legacy code work.
> 
> It might also work if you were to allocate the space for the VBE shim
> so that we don't later try to shadow the real ROM to the same location.
> 
> Or maybe we should be letting the legacy BIOS video driver take
> precedence if the CSM has a video BIOS, and not letting the native
> drivers bind at all?
> 

In 2013, you submitted the following patch:

  OvmfPkg: Don't build in QemuVideoDxe when we have CSM

The thread starts here:

  https://www.mail-archive.com/edk2-devel@lists.sourceforge.net/msg01871.html

After an update:

  http://mid.mail-archive.com/1360493281.7383.26.camel@shinybook.infradead.org

I had given my R-b:

  http://mid.mail-archive.com/511816AD.9000603@redhat.com

But, the patch was never merged.

The commit hash referenced in those messages still works (pointing into your personal repo):

  http://git.infradead.org/users/dwmw2/edk2.git/commitdiff/22253c949e5

Can you resubmit that patch please?

Thanks,
Laszlo