[PATCH] Various x86emu fixes

[Mark Marshall]

Below are three small patches, they all seemed useful while I have been 
working with VGA option ROMs.

Thanks for the great work.

MM

[ron minnich]

These look really good to me anyway, maybe someone can give them a test?

ron

[Mark Marshall]

Ping.

Mark Marshall wrote:
> Below are three small patches, they all seemed useful while I have been 
> working with VGA option ROMs.
>
> Thanks for the great work.
>
> MM

[Myles Watson]

> Ping.

The two x86emu fixes look fine to me, but I haven't tested them.

I don't think we should depend on the value from the card.  The point of
that check is to make sure we don't run the wrong types of ROMs.  One way to
work around it would be to correct the VGA BIOS from your card and put it
into CBFS.

Thanks,
Myles

[Stefan Reinauer]

Mark Marshall wrote:
> Ping.
>
> Mark Marshall wrote:
> > Below are three small patches, they all seemed useful while I have
> > been working with VGA option ROMs.
> >
> > Thanks for the great work.
> >
> > MM

Awesome!

r4909 - r4911

[Peter Stuge]

Myles Watson wrote:
> I don't think we should depend on the value from the card.  The
> point of that check is to make sure we don't run the wrong types of
> ROMs. One way to work around it would be to correct the VGA BIOS
> from your card and put it into CBFS.

Stefan Reinauer wrote:
> > Ping.
>
> Awesome!

Stefan, what about Myles' comment above? I think it's a good point.

//Peter

[Myles Watson]

On Fri, Nov 6, 2009 at 10:18 AM, Peter Stuge  wrote:
> Myles Watson wrote:
> > I don't think we should depend on the value from the card.  The
> > point of that check is to make sure we don't run the wrong types of
> > ROMs. One way to work around it would be to correct the VGA BIOS
> > from your card and put it into CBFS.
>
> Stefan, what about Myles' comment above? I think it's a good point.

We started discussing it off list.  This should catch the list up:

On Thu, Nov 5, 2009 at 11:03 AM, Stefan Reinauer wrote:
> Myles Watson wrote:
> > Did you see my concern with checking the device instead of the ROM?
> >
> > Thanks,
> > Myles
>
> oops sorry.. I missed that one..

No problem.
> Hm.. not sure.. that particular check does not seem to be a consistency
> check but rather a check to determine from which location to run the VGA
> OPROM from... Generally, VGA oproms should run from 0xc0000.

Yes.  His card is broken.  I just wanted to make sure the change doesn't
break other things to enable his 10-year-old card.
> Maybe we should add another check whether class of the card and class of
> the device are the same?

If we did, that would break his card again.

I guess someone will complain if/when it matters.  I'm not sure what the
"right" thing to do is.

Thanks,
Myles

[Mark Marshall]

Myles Watson wrote:
> On Fri, Nov 6, 2009 at 10:18 AM, Peter Stuge <[e-mail] 
> > wrote:
> Myles Watson wrote:
>  > I don't think we should depend on the value from the card.  The
>  > point of that check is to make sure we don't run the wrong types of
>  > ROMs. One way to work around it would be to correct the VGA BIOS
>  > from your card and put it into CBFS.
>
> Stefan, what about Myles' comment above? I think it's a good point.
>
>
> We started discussing it off list.  This should catch the list up:
>
> On Thu, Nov 5, 2009 at 11:03 AM, Stefan Reinauer <[e-mail] 
> > wrote:
> Myles Watson wrote:
>  > Did you see my concern with checking the device instead of the ROM?
>  >
>  > Thanks,
>  > Myles
> oops sorry.. I missed that one..
>
> No problem.
> Hm.. not sure.. that particular check does not seem to be a consistency
> check but rather a check to determine from which location to run the VGA
> OPROM from... Generally, VGA oproms should run from 0xc0000.
>
> Yes.  His card is broken.  I just wanted to make sure the change doesn't 
> break other things to enable his 10-year-old card.
> Maybe we should add another check whether class of the card and class of
> the device are the same?
>
> If we did, that would break his card again.
>
> I guess someone will complain if/when it matters.  I'm not sure what the 
> "right" thing to do is.

I did think about this before I submitted my original patch, but didn't 
write my thoughts down anywhere, so here goes:

In pci_rom.c::pci_rom_brobe there is a check to see if the VID/PID of 
the device, as read from the PCI config space, matches the ROM image. 
This check was in the original code, and should be there.  I like it - 
it protects against user error.

There is also a check to see if the "class of device" as read from the 
PCI config space matches that contained in the ROM image.  This seems 
like a reasonable check to make, and to print a message if it fails, but 
I think the code should push on, even if this fails.

The change I made was a little later on, once we have decided to load 
the ROM image.  We have to do something different if the device we are 
loading the ROM for is a VGA (it has to be at 0xC000, and should be done 
early).  My change was to make this check based on the PCI config space 
- not the ROM image.  My thinking was along the lines that if someone 
made a PCI card that didn't declare itself to be a VGA in the PCI config 
space then most software wouldn't treat it as a VGA device.  (in other 
words the PCI config space is more likely to be correct than the ROM).

This also more closely matches my understanding of how this code might 
be called.  There would be one call early on where the first VGA device 
has it's ROM loaded, and then there would be a second pass where the non 
VGA devices have there ROMs loaded.

The final argument for having the code like I had it was that my PCI VGA 
card was quite popular, back in the day[*], and I assume that it must 
have worked with almost every motherboard out there, so legacy BIOSs 
must have been equally lax.

MM