Patchwork Various x86emu fixes

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

Thanks for the great work.

MM
Use more care when implimenting the PCI BIOS functions.

The 'CHECK' function seemed to be both wrong code and the wrong
number, so I have just added the corrct code in parallel.

The READ_CONF and WRITE_CONF functions would both do the wrong thing
if the passed in BDF was not found.  We should return and error to the
caller, but not stop running the option ROM.

Signed-off-by: Mark Marshall <mark.marshall@csr.com>
When loading an option ROM use the class stored in the device to
decide whether the option ROM is a special VGA type.

An S3 card that I've got has the wrong class in the VGA BIOS.
(A Stealth 64 DRAM T PCI, from 1994 - BIOS V2.02)

Signed-off-by: Mark Marshall <mark.marshall@csr.com>

Index: src/devices/pci_rom.c
===================================================================
--- src/devices/pci_rom.c	(revision 4822)
+++ src/devices/pci_rom.c	(working copy)
@@ -116,7 +116,10 @@
 
 	rom_size = rom_header->size * 512;
 
-	if (PCI_CLASS_DISPLAY_VGA == rom_data->class_hi) {
+	// We check to see if the device thinks it is a VGA device not
+	// whether the ROM image is for a VGA device because some
+	// devices have a mismatch between the hardware and the ROM
+ 	if (PCI_CLASS_DISPLAY_VGA == (dev->class >> 8)) {
 #if CONFIG_CONSOLE_VGA == 1 && CONFIG_CONSOLE_VGA_MULTI == 0
 		extern device_t vga_pri;	// the primary vga device, defined in device.c
 		if (dev != vga_pri) return NULL; // only one VGA supported
Get the passed in Bus/Device/Function from the correct location on the
stack.

Signed-off-by: Mark Marshall <mark.marshall@csr.com>

Index: util/x86emu/x86_asm.S
===================================================================
--- util/x86emu/x86_asm.S	(revision 4855)
+++ util/x86emu/x86_asm.S	(working copy)
@@ -68,8 +68,8 @@
 
 	/* Get devfn into %ecx */
 	movl    %esp, %ebp
-	// FIXME: Should this function be called with regparm=0?
-	movl    8(%ebp), %ecx
+	/* This function is now called with regparm=0. */
+	movl    36(%ebp), %ecx
 
 	/* Activate the right segment descriptor real mode. */
 	ljmp	$0x28, $RELOCATED(1f)

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

ron

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
>

> 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

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

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

On Fri, Nov 6, 2009 at 10:18 AM, Peter Stuge <peter@stuge.se> 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 <stepan@coresystems.de>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

Myles Watson wrote:
> 
> 
> On Fri, Nov 6, 2009 at 10:18 AM, Peter Stuge <peter@stuge.se 
> <mailto:peter@stuge.se>> 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 <stepan@coresystems.de 
> <mailto:stepan@coresystems.de>> 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