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

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

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?


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

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

[Laszlo Ersek]

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

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Wed, 2019-06-19 at 23:57 +0200, Laszlo Ersek wrote:
> 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?


Makes sense. I've pushed it to
http://git.infradead.org/users/dwmw2/edk2.git/shortlog/refs/heads/csm

Thanks.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

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

[Laszlo Ersek]

On 06/20/19 10:59, David Woodhouse wrote:
> On Wed, 2019-06-19 at 23:57 +0200, Laszlo Ersek wrote:
>> 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?
> 
> 
> Makes sense. I've pushed it to
> http://git.infradead.org/users/dwmw2/edk2.git/shortlog/refs/heads/csm

Thanks -- it looks good to me (a09db38a866a).

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Still, can you please post the patch to the list, for review?

We've never merged pull requests before, but I remember that you prefer
those (because you dislike an edk2 subsys maintainer rebasing your
branch from your original fork-off point). So I'm not asking for the
patch email because I insist on using "git-am". I'm asking for it
because we'd like to review every patch on the list, for now.

Once you get the proper feedback tags (R-b and so on) on the list, you
can do the rebase / rewording yourself, and then you could submit a pull
request (*not* github pull request, but from "git-request-pull", in
email). We've never exercised that in the past, so our workflow is
totally immature on that. But I understand you insist on pull reqs, and
I think we should accommodate that (again, *not* github pull reqs). The
following QEMU wiki article could be the starting point for such a
pullreq workflow in edk2:

  https://wiki.qemu.org/Contribute/SubmitAPullRequest

That said, in case you were OK with git-am in this case, that would
likely expedite me applying your patch.

Thanks!
Laszlo

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On Thu, 2019-06-20 at 16:41 +0200, Laszlo Ersek wrote:
> On 06/20/19 10:59, David Woodhouse wrote:
> > On Wed, 2019-06-19 at 23:57 +0200, Laszlo Ersek wrote:
> > > 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?
> > 
> > 
> > Makes sense. I've pushed it to
> > http://git.infradead.org/users/dwmw2/edk2.git/shortlog/refs/heads/csm
> 
> Thanks -- it looks good to me (a09db38a866a).
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Still, can you please post the patch to the list, for review?

Sure! That wasn't meant to be an implicit pull request; just an interim
response while I get my act together and put together the rest of that
series including some other CSM fixes. The first two patches there have
already been posted in email, FWIW.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]

On Thu, 2019-06-20 at 16:41 +0200, Laszlo Ersek wrote:
> We've never merged pull requests before, but I remember that you prefer
> those (because you dislike an edk2 subsys maintainer rebasing your
> branch from your original fork-off point). So I'm not asking for the
> patch email because I insist on using "git-am". I'm asking for it
> because we'd like to review every patch on the list, for now.

Going back to this... I generally prefer *everyone* to use the git
workflow as $DEITY intended it, not just for my own submissions.

When a developer writes and — perhaps more to the point — tests a
patch, they have done so against a specific version of the project.

With the proper git workflow, their contribution is merged with the
correct parent information. The version control system is doing its job
correctly and recording what actually happened; not an approximation of
it.

With git-am being used to apply those contributions on a later version
of the tree, there is a *theoretical* possibility that some other
seemingly innocuous change elsewhere in the tree just happens to break
the newly-submitted code.

The thing is: over time, at scale, the chances of that theoretical,
highly unlikely, incompatibility happening approach 1.

Anyone who has experience maintaining out-of-tree patches for any
project over time will have seen this. Your patchset suddenly doesn't
work, although it applied cleanly to the source tree, and you have to
go hunting for whatever strange locking or other environment issue, or
API semantics, broke it since you last updated.

If — no, WHEN — this kind of thing happens, you can end up with code
being merged which, according to the recorded history, never actually
worked at all. There is no way to use tools like 'git bisect' to find
the working point, and what went wrong. It simply *never* worked,
because the true history has been thrown away and not stored in the
version control system. The version control system had *one* job, and
you haven't allowed it to do that.

And yes, it's unlikely and infrequent. But it *does* happen, and that's
why we should use the tools properly as a matter of habit.

For my own patches of course I do a 'git pull --rebase' occasionally to
ensure that it all still works when my patches are applied on top of
the git master branch. But even with the best intentions, that isn't
really good enough. Because all the exhaustive manual testing of
various corner cases (adding multiple boot targets, checking that
selecting each of them does the right thing, turning 64-bit BARs on and
off and doing builds with various different configs) is not happening
when I do that 'git pull --rebase' smoke test. So even with me doing
some basic retesting, it's possible that something ends up getting
committed that, for some esoteric corner case, apparently never worked
even though it *did* actually work when I wrote the code. And when we
realise that in a year's time and look back, the version control system
won't help us because we didn't use it properly.




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]