Handling PCI/ROM space

Handling PCI/ROM space

[Jeremy Fitzhardinge]

I'm seeing oopses in probe_roms() and pci_find_bios(), apparently 
because those pages are not mapped under Xen.  I'm not sure why I'm 
seeing this now and not before, but I suspect its because I enabled 
CONFIG_DEBUG_PAGEALLOC.  Anyway, I've got these patches to deal with 
these cases:

--- a/arch/i386/kernel/setup.c
+++ b/arch/i386/kernel/setup.c
@@ -276,7 +276,14 @@ static struct resource standard_io_resou
 #define STANDARD_IO_RESOURCES \
 	(sizeof standard_io_resources / sizeof standard_io_resources[0])
 
-#define romsignature(x) (*(unsigned short *)(x) == 0xaa55)
+static inline int romsignature(const unsigned char *x)
+{
+     unsigned short sig;
+     int ret = 0;
+     if (__get_user(sig, (const unsigned short *)x) == 0)
+	  ret = (sig == 0xaa55);
+     return ret;
+}
 
 static int __init romchecksum(unsigned char *rom, unsigned long length)
 {
--- a/arch/i386/pci/pcbios.c
+++ b/arch/i386/pci/pcbios.c
@@ -5,6 +5,7 @@
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/module.h>
+#include <asm/uaccess.h>
 #include "pci.h"
 #include "pci-functions.h"
 
@@ -301,7 +302,7 @@ static struct pci_raw_ops pci_bios_acces
 
 static struct pci_raw_ops * __devinit pci_find_bios(void)
 {
-	union bios32 *check;
+	union bios32 *check, sig;
 	unsigned char sum;
 	int i, length;
 
@@ -314,6 +315,10 @@ static struct pci_raw_ops * __devinit pc
 	for (check = (union bios32 *) __va(0xe0000);
 	     check <= (union bios32 *) __va(0xffff0);
 	     ++check) {
+		long sig;
+		if (__get_user(sig, &check->fields.signature))
+			continue;
+
 		if (check->fields.signature != BIOS32_SIGNATURE)
 			continue;
 		length = check->fields.length * 16;


Does this seem reasonable, or should there be some other fix?

    J

Re: Handling PCI/ROM space

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> I'm seeing oopses in probe_roms() and pci_find_bios(), apparently 
> because those pages are not mapped under Xen.  I'm not sure why I'm 
> seeing this now and not before, but I suspect its because I enabled 
> CONFIG_DEBUG_PAGEALLOC.  Anyway, I've got these patches to deal with 
> these cases:

Probably a config file difference - GOBIOS vs GODIRECT.

These patches look fine, although is there a more general solution?  
Like bypassing the whole PCI ROM probing entirely?  We don't really want 
to use the PCI ROM here either, although we have a slightly worse 
problem - the pages are mapped and do have a PCI ROM.  Perhaps 
paravirt-ops should be able to flip a global switch to disable this 
(perhaps already there?).

Zach

Re: Handling PCI/ROM space

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Probably a config file difference - GOBIOS vs GODIRECT.
>
> These patches look fine, although is there a more general solution?  
> Like bypassing the whole PCI ROM probing entirely?  We don't really 
> want to use the PCI ROM here either, although we have a slightly worse 
> problem - the pages are mapped and do have a PCI ROM.  Perhaps 
> paravirt-ops should be able to flip a global switch to disable this 
> (perhaps already there?).

There's a pci_probe variable, which has a bitmask of which PCI access 
methods to use; if you mask out PCI_PROBE_BIOS then it won't bother.  I 
considered doing this, but it wasn't clear to me where this actually get 
set up (it's a bit diffuse), and whether to add a new pv_op hook 
intercept this.

I also thought that we don't necessarily want to unconditionally disable 
PCI access for paravirt guests since even an unprivileged guest may have 
a raw device exported to it, and we don't want to add too many special 
cases when dom0 is migrated to operate in the pv_op world.  So it seemed 
more correct to actually allow the probe but deal with unmapped pci ROM 
space.

    J