conditionalize PCI reassign code

PATCH In review Jan Beulich 5 participants 11 messages Oct 14, 2008 – Oct 17, 2008

Subsystems (2)

PCI Xen hypervisor interface

lore

Share & tools

lore

Jan Beulich

author

2008-10-14 13:39 UTC #0 lore raw

PATCH 1/1 conditionalize PCI reassign code

... by a config option, selected only from privileged Xen configurations.

Also eliminate the pointless new macro ROUND_UP_TO_PAGESIZE().

Finally, I'm not really understanding the need for two command line options (reassigndev= and reassign_resources) here - wouldn't the former suffice? Specifying one without the other doesn't seem to make much sense...

As usual, written and tested on 2.6.27 and made apply to the 2.6.18 tree without further testing.

Signed-off-by Jan Beulich <[email protected]>

drivers/pci/Kconfig +3 -0

Index: head-2008-10-13/drivers/pci/Kconfig
===================================================================
@@ -21,6 +21,9 @@ config PCI_MSI
 
 	   If you don't know what to do here, say N.
 
+config PCI_REASSIGN
+	bool
+
 config PCI_DEBUG
 	bool "PCI Debugging"
 	depends on PCI && DEBUG_KERNEL

drivers/pci/Makefile +2 -2

Index: head-2008-10-13/drivers/pci/Makefile
===================================================================
@@ -3,8 +3,8 @@
 #
 
 obj-y		+= access.o bus.o probe.o remove.o pci.o quirks.o \
-			pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
-			reassigndev.o
+			pci-driver.o search.o pci-sysfs.o rom.o setup-res.o
+obj-$(CONFIG_PCI_REASSIGN) += reassigndev.o
 obj-$(CONFIG_PROC_FS) += proc.o
 
 # Build PCI Express stuff if needed

drivers/pci/pci.h +5 -2

Index: head-2008-10-13/drivers/pci/pci.h
===================================================================
@@ -144,8 +144,11 @@ struct pci_slot_attribute {
 	return NULL;
 }
 
-#define ROUND_UP_TO_PAGESIZE(size) ((size + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1))
-
+#ifdef CONFIG_PCI_REASSIGN
 extern int reassign_resources;
 extern int is_reassigndev(struct pci_dev *dev);
 extern void pci_update_bridge(struct pci_dev *dev, int resno);
+#else
+#define reassign_resources 0
+#define is_reassigndev(dev) 0
+#endif

drivers/pci/setup-bus.c +2 -3

Index: head-2008-10-13/drivers/pci/setup-bus.c
===================================================================
@@ -354,9 +354,8 @@ static int pbus_size_mem(struct pci_bus 
 				continue;
 			r_size = r->end - r->start + 1;
 
-			if (reassign) {
-				r_size = ROUND_UP_TO_PAGESIZE(r_size);
-			}
+			if (reassign)
+				r_size = ALIGN(r_size, PAGE_SIZE);
 
 			/* For bridges size != alignment */
 			align = (i < PCI_BRIDGE_RESOURCES) ? r_size : r->start;

drivers/pci/setup-res.c +9 -5

Index: head-2008-10-13/drivers/pci/setup-res.c
===================================================================
@@ -126,7 +126,8 @@ pci_claim_resource(struct pci_dev *dev,
 }
 EXPORT_SYMBOL_GPL(pci_claim_resource);
 
-void 
+#ifdef CONFIG_PCI_REASSIGN
+void
 pci_update_bridge(struct pci_dev *dev, int resno)
 {
 	struct resource *res = &dev->resource[resno];

@@ -193,6 +194,7 @@ pci_update_bridge(struct pci_dev *dev, i
 		break;
 	}
 }
+#endif
 
 int pci_assign_resource(struct pci_dev *dev, int resno)
 {

@@ -215,7 +217,7 @@ int pci_assign_resource(struct pci_dev *
 		align = size;
 		if ((reassigndev) &&
 		    (res->flags & IORESOURCE_MEM)) {
-			align = ROUND_UP_TO_PAGESIZE(align);
+			align = ALIGN(align, PAGE_SIZE);
 		}
 	} else {
 		align = res->start;

@@ -242,9 +244,11 @@ int pci_assign_resource(struct pci_dev *
 			resno, (unsigned long long)size,
 			(unsigned long long)res->start, pci_name(dev));
 	} else if (resno < PCI_BRIDGE_RESOURCES) {
-		printk(KERN_DEBUG "PCI: Assign resource(%d) on %s "
-			"%016llx - %016llx\n", resno, pci_name(dev),
-			(u64)res->start, (u64)res->end);
+		if (reassign_resources && is_reassigndev(dev))
+			printk(KERN_DEBUG "PCI: Assign resource(%d) on %s "
+				"%016llx - %016llx\n", resno, pci_name(dev),
+				 (unsigned long long)res->start,
+				 (unsigned long long)res->end);
 		pci_update_resource(dev, res, resno);
 	}

drivers/xen/Kconfig +1 -0

Index: head-2008-10-13/drivers/xen/Kconfig
===================================================================
@@ -16,6 +16,7 @@ menu "XEN"
 
 config XEN_PRIVILEGED_GUEST
 	bool "Privileged Guest (domain 0)"
+	select PCI_REASSIGN if PCI
 	help
 	  Support for privileged operation (domain 0)

Yuji Shimada

2008-10-15 07:56 UTC #1 lore raw

Quoted 4 lines · On Tue, 14 Oct 2008 14:39:50 +0100 "Jan Beulich" <[email protected]> wrote:

Finally, I'm not really understanding the need for two command line options (reassigndev= and reassign_resources) here - wouldn't the former suffice? Specifying one without the other doesn't seem to make much sense...

I agree with you that reassign_resources option is not needed.

I will submit the patch to remove it, after submitting the patch to fix the issue which was found by Zhao, Yu.

Thanks,

--
Yuji Shimada

Yu Zhao

2008-10-15 15:14 UTC #2 lore raw

Hello Yuji,

It would be great if you could submit your patch to the linux-pci. Since the native kernel (KVM) may also interested in this feature.

Thanks,
Yu

Quoted 22 lines · Yuji Shimada

Quoted 4 lines · On Tue, 14 Oct 2008 14:39:50 +0100 "Jan Beulich" <[email protected]> wrote:
Finally, I'm not really understanding the need for two command line options (reassigndev= and reassign_resources) here - wouldn't the former suffice? Specifying one without the other doesn't seem to make much sense...
I agree with you that reassign_resources option is not needed.
I will submit the patch to remove it, after submitting the patch to fix the issue which was found by Zhao, Yu.
Thanks,
--
Yuji Shimada
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Zhao, Yu

2008-10-17 02:43 UTC #3 lore raw

In reply to Jan Beulich

There is build error (c/s 701) with this patch, please take a look.

Thanks.

Quoted 142 lines · Jan Beulich

... by a config option, selected only from privileged Xen configurations.
Also eliminate the pointless new macro ROUND_UP_TO_PAGESIZE().
Finally, I'm not really understanding the need for two command line options (reassigndev= and reassign_resources) here - wouldn't the former suffice? Specifying one without the other doesn't seem to make much sense...
As usual, written and tested on 2.6.27 and made apply to the 2.6.18 tree without further testing.
Signed-off-by Jan Beulich <[email protected]>
Index: head-2008-10-13/drivers/pci/Kconfig
===================================================================
drivers/pci/Kconfig 2008-10-13 15:14:53.000000000 +0200 renamed from drivers/pci/Kconfig 2008-10-13 15:11:33.000000000 +0200 +3 -0
@@ -21,6 +21,9 @@ config PCI_MSI
 
           If you don't know what to do here, say N.
 
+config PCI_REASSIGN
+       bool
+
 config PCI_DEBUG
        bool "PCI Debugging"
        depends on PCI && DEBUG_KERNEL
Index: head-2008-10-13/drivers/pci/Makefile
===================================================================
drivers/pci/Makefile 2008-10-13 15:14:53.000000000 +0200 renamed from drivers/pci/Makefile 2008-10-13 15:11:33.000000000 +0200 +2 -2
@@ -3,8 +3,8 @@
 #
 
 obj-y          += access.o bus.o probe.o remove.o pci.o quirks.o \
-                       pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
-                       reassigndev.o
+                       pci-driver.o search.o pci-sysfs.o rom.o setup-res.o
+obj-$(CONFIG_PCI_REASSIGN) += reassigndev.o
 obj-$(CONFIG_PROC_FS) += proc.o
 
 # Build PCI Express stuff if needed
Index: head-2008-10-13/drivers/pci/pci.h
===================================================================
drivers/pci/pci.h 2008-10-13 15:14:53.000000000 +0200 renamed from drivers/pci/pci.h 2008-10-13 15:11:33.000000000 +0200 +5 -2
@@ -144,8 +144,11 @@ struct pci_slot_attribute {
        return NULL;
 }
 
-#define ROUND_UP_TO_PAGESIZE(size) ((size + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1))
-
+#ifdef CONFIG_PCI_REASSIGN
 extern int reassign_resources;
 extern int is_reassigndev(struct pci_dev *dev);
 extern void pci_update_bridge(struct pci_dev *dev, int resno);
+#else
+#define reassign_resources 0
+#define is_reassigndev(dev) 0
+#endif
Index: head-2008-10-13/drivers/pci/setup-bus.c
===================================================================
drivers/pci/setup-bus.c 2008-10-13 15:14:53.000000000 +0200 renamed from drivers/pci/setup-bus.c 2008-10-13 15:11:33.000000000 +0200 +2 -3
@@ -354,9 +354,8 @@ static int pbus_size_mem(struct pci_bus
                                continue;
                        r_size = r->end - r->start + 1;
 
-                       if (reassign) {
-                               r_size = ROUND_UP_TO_PAGESIZE(r_size);
-                       }
+                       if (reassign)
+                               r_size = ALIGN(r_size, PAGE_SIZE);
 
                        /* For bridges size != alignment */
                        align = (i < PCI_BRIDGE_RESOURCES) ? r_size : r->start;
Index: head-2008-10-13/drivers/pci/setup-res.c
===================================================================
drivers/pci/setup-res.c 2008-10-13 15:14:53.000000000 +0200 renamed from drivers/pci/setup-res.c 2008-10-13 15:11:33.000000000 +0200 +9 -5
@@ -126,7 +126,8 @@ pci_claim_resource(struct pci_dev *dev,
 }
 EXPORT_SYMBOL_GPL(pci_claim_resource);
 
-void
+#ifdef CONFIG_PCI_REASSIGN
+void
 pci_update_bridge(struct pci_dev *dev, int resno)
 {
        struct resource *res = &dev->resource[resno];
@@ -193,6 +194,7 @@ pci_update_bridge(struct pci_dev *dev, i
                break;
        }
 }
+#endif
 
 int pci_assign_resource(struct pci_dev *dev, int resno)
 {
@@ -215,7 +217,7 @@ int pci_assign_resource(struct pci_dev *
                align = size;
                if ((reassigndev) &&
                    (res->flags & IORESOURCE_MEM)) {
-                       align = ROUND_UP_TO_PAGESIZE(align);
+                       align = ALIGN(align, PAGE_SIZE);
                }
        } else {
                align = res->start;
@@ -242,9 +244,11 @@ int pci_assign_resource(struct pci_dev *
                        resno, (unsigned long long)size,
                        (unsigned long long)res->start, pci_name(dev));
        } else if (resno < PCI_BRIDGE_RESOURCES) {
-               printk(KERN_DEBUG "PCI: Assign resource(%d) on %s "
-                       "%016llx - %016llx\n", resno, pci_name(dev),
-                       (u64)res->start, (u64)res->end);
+               if (reassign_resources && is_reassigndev(dev))
+                       printk(KERN_DEBUG "PCI: Assign resource(%d) on %s "
+                               "%016llx - %016llx\n", resno, pci_name(dev),
+                                (unsigned long long)res->start,
+                                (unsigned long long)res->end);
                pci_update_resource(dev, res, resno);
        }
 
Index: head-2008-10-13/drivers/xen/Kconfig
===================================================================
drivers/xen/Kconfig 2008-10-13 15:16:22.000000000 +0200 renamed from drivers/xen/Kconfig 2008-10-13 15:14:34.000000000 +0200 +1 -0
@@ -16,6 +16,7 @@ menu "XEN"
 
 config XEN_PRIVILEGED_GUEST
        bool "Privileged Guest (domain 0)"
+       select PCI_REASSIGN if PCI
        help
          Support for privileged operation (domain 0)
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jan Beulich

author

2008-10-17 07:17 UTC #4 lore raw

Quoted 2 lines

Quoted 1 line
Quoted 1 line
"Zhao, Yu" <[email protected]> 17.10.08 04:43 >>>
There is build error (c/s 701) with this patch, please take a look.

This is why I always include

Quoted 2 lines

Quoted 2 lines
As usual, written and tested on 2.6.27 and made apply to the 2.6.18 tree without further testing.

I'm making a best effort attempt at getting things in proper state for that tree, but time-wise can't afford full validation. But I always assumed that if this appears in the real (not the staging) tree, it at least survived building in Citrix' trees - is that not correct, Keir?

As you observed the build error - why don't you provide at least the error the compiler generated? That might make it possible to provide a fix quickly.

Jan

Keir Fraser

2008-10-17 07:22 UTC #5 lore raw

Quoted 7 lines · Jan Beulich

Quoted 2 lines
Quoted 2 lines
As usual, written and tested on 2.6.27 and made apply to the 2.6.18 tree without further testing.
I'm making a best effort attempt at getting things in proper state for that tree, but time-wise can't afford full validation. But I always assumed that if this appears in the real (not the staging) tree, it at least survived building in Citrix' trees - is that not correct, Keir?

Yes, but obviously only one configuration is tested. If this build failure is with CONFIG_PCI_REASSIGN we would not pick that up unless it's in the default buildconfig.

-- Keir

Jan Beulich

author

2008-10-17 07:27 UTC #6 lore raw

Quoted 5 lines

Quoted 1 line
Quoted 1 line
Keir Fraser <[email protected]> 17.10.08 09:22 >>>
Quoted 3 lines · Jan Beulich
Quoted 2 lines
Quoted 2 lines
As usual, written and tested on 2.6.27 and made apply to the 2.6.18 tree without further testing.

Quoted 8 lines

Quoted 4 lines
I'm making a best effort attempt at getting things in proper state for that tree, but time-wise can't afford full validation. But I always assumed that if this appears in the real (not the staging) tree, it at least survived building in Citrix' trees - is that not correct, Keir?
Yes, but obviously only one configuration is tested. If this build failure is with CONFIG_PCI_REASSIGN we would not pick that up unless it's in the default buildconfig.

CONFIG_PCI_REASSIGN is not a user-visible option, it gets selected from XEN_PRIVILEGED_GUEST. So as long as your builds include a Dom0 kernel build, they'd cover this case.

Jan

Zhao, Yu

2008-10-17 07:27 UTC #7 lore raw

In reply to Jan Beulich

Quoted 8 lines · Jan Beulich

Quoted 2 lines
Quoted 1 line
Quoted 1 line
"Zhao, Yu" <[email protected]> 17.10.08 04:43 >>>
There is build error (c/s 701) with this patch, please take a look.
This is why I always include
Quoted 2 lines
Quoted 2 lines
As usual, written and tested on 2.6.27 and made apply to the 2.6.18 tree without further testing.

Quoted 8 lines

I'm making a best effort attempt at getting things in proper state for that tree, but time-wise can't afford full validation. But I always assumed that if this appears in the real (not the staging) tree, it at least survived building in Citrix' trees - is that not correct, Keir?
As you observed the build error - why don't you provide at least the error the compiler generated? That might make it possible to provide a fix quickly.

+#ifdef CONFIG_PCI_REASSIGN
extern int reassign_resources;
extern int is_reassigndev(struct pci_dev *dev); extern void pci_update_bridge(struct pci_dev *dev, int resno);
+#else
+#define reassign_resources 0
+#define is_reassigndev(dev) 0
+#endif

When the CONFIG_PCI_REASSIGN is not set, e.g., a domU kernel, the following line fails to be compiled (drivers/pci/quirks.c):

int reassign_resources = 0;

Quoted 3 lines

Jan

Keir Fraser

2008-10-17 07:34 UTC #8 lore raw

Quoted 13 lines · Yu

Quoted 3 lines
As you observed the build error - why don't you provide at least the error the compiler generated? That might make it possible to provide a fix quickly.
drivers/pci/pci.h +5 -0
+#ifdef CONFIG_PCI_REASSIGN
  extern int reassign_resources;
  extern int is_reassigndev(struct pci_dev *dev);
  extern void pci_update_bridge(struct pci_dev *dev, int resno);
+#else
+#define reassign_resources 0
+#define is_reassigndev(dev) 0
+#endif

Quoted 4 lines

When the CONFIG_PCI_REASSIGN is not set, e.g., a domU kernel, the following line fails to be compiled (drivers/pci/quirks.c):
int reassign_resources = 0;

Ah, the automated tests only build the -xen config.

-- Keir

Zhao, Yu

2008-10-17 07:38 UTC #9 lore raw

Quoted 16 lines · Keir Fraser

Quoted 12 lines · Yu
Quoted 3 lines
As you observed the build error - why don't you provide at least the error the compiler generated? That might make it possible to provide a fix quickly.
drivers/pci/pci.h +5 -0
+#ifdef CONFIG_PCI_REASSIGN
  extern int reassign_resources;
  extern int is_reassigndev(struct pci_dev *dev);
  extern void pci_update_bridge(struct pci_dev *dev, int resno);
+#else
+#define reassign_resources 0
+#define is_reassigndev(dev) 0
+#endif

Quoted 10 lines

Quoted 4 lines
When the CONFIG_PCI_REASSIGN is not set, e.g., a domU kernel, the following line fails to be compiled (drivers/pci/quirks.c):
int reassign_resources = 0;
Ah, the automated tests only build the -xen config.
-- Keir

Actually I happened to get it by misusing a native kernel config ...

Jan Beulich

author

2008-10-17 08:46 UTC #10 lore raw

In reply to Zhao, Yu

Quoted 10 lines

Quoted 1 line

Quoted 1 line
"Zhao, Yu" <[email protected]> 17.10.08 09:27 >>>

(quoted diff) +5 -0

+#ifdef CONFIG_PCI_REASSIGN
 extern int reassign_resources;
 extern int is_reassigndev(struct pci_dev *dev);
 extern void pci_update_bridge(struct pci_dev *dev, int resno);
+#else
+#define reassign_resources 0
+#define is_reassigndev(dev) 0
+#endif

Quoted 4 lines

When the CONFIG_PCI_REASSIGN is not set, e.g., a domU kernel, the following line fails to be compiled (drivers/pci/quirks.c):
int reassign_resources = 0;

Ah, yes, I overlooked that, because when I merged these changes into our kernel the main change to quirks.c needed to be done differently (and then properly in reassigndev.c) anyway, and hence I at once moved the misplaced (and as we agreed elsewhere unnecessary) option into reassigndev.c. So the (untested) patch below should fix it until the patch to eliminate the option will arrive from the original submitter.

Signed-off-by Jan Beulich <[email protected]>

drivers/pci/quirks.c +0 ~~-13~~

@@ -33,19 +33,6 @@
 }
 __setup("pci-mem-align", set_pci_mem_align);
 
-
-int reassign_resources = 0;
-
-static int __init set_reassign_resources(char *str)
-{
-	/* resources reassign on */
-	reassign_resources = 1;
-	printk(KERN_DEBUG "PCI: resource reassign ON.\n");
-
-	return 1;
-}
-__setup("reassign_resources", set_reassign_resources);
-
 /* This quirk function enables us to force all memory resources which are 
  * assigned to PCI devices, to be page-aligned.
  */

drivers/pci/reassigndev.c +12 -0

@@ -34,6 +34,18 @@ static int pbus_size_mem(struct pci_bus 
 }
 __setup("reassigndev=", reassigndev_setup);
 
+int reassign_resources = 0;
+
+static int __init set_reassign_resources(char *str)
+{
+	/* resources reassign on */
+	reassign_resources = 1;
+	printk(KERN_DEBUG "PCI: resource reassign ON.\n");
+
+	return 1;
+}
+__setup("reassign_resources", set_reassign_resources);
+
 int is_reassigndev(struct pci_dev *dev)
 {
 	char dev_str[TOKEN_MAX+1];