|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
The reason that I did not code this as a module in the first place was
because the pcistub driver needs to be loaded before other device
drivers so that it gets first pick at seizing devices from the kernel.
That's why I use fs_initcall in the pcistub driver instead of
device_initcall or module_init, it gets called earlier in the kernel
boot process. As a module, you can't do that. If other PCI device
drivers (whether modules or not) are loaded before the pciback module,
they'll have a higher priority (because they're earlier in the linked
list of drivers) for grabbing devices. This functionality (using
fs_initcall) is somewhat of a "hack" (because I don't know of a cleaner
way to ensure that the pcistub driver gets probed for ALL pci devices).
Recent kernels have the "bind" and "unbind" attributes on drivers in
sysfs that would help you avoid this problem, but that seems like a lot
to ask of the user. They'd have to first unbind their device from the
driver that grabbed it. Then they'd have to load the pciback module (or,
if it was already loaded, they could use the "bind" attribute"). Either
way, it turns "hiding" a device from a one-step process (just specify
all devices on the kernel command-line) to a two-step (unbind each
device from driver, bind to pcistub driver).
See other comments in-line below.
On Wed, 2006-03-08 at 12:06 +0100, Jan Beulich wrote:
> Index: head-2006-03-06/drivers/xen/Kconfig
> ===================================================================
> --- head-2006-03-06.orig/drivers/xen/Kconfig 2006-03-06
> 13:18:54.000000000 +0100
> +++ head-2006-03-06/drivers/xen/Kconfig 2006-03-07 12:00:13.000000000
> +0100
> @@ -30,9 +30,9 @@ config XEN_UNPRIVILEGED_GUEST
> default !XEN_PRIVILEGED_GUEST
>
> config XEN_PCIDEV_BACKEND
> - bool "PCI device backend driver"
> - select PCI
> - default y if XEN_PRIVILEGED_GUEST
> + tristate "PCI device backend driver"
> + depends PCI
> + default XEN_PRIVILEGED_GUEST
> help
> The PCI device backend driver allows the kernel to export
> arbitrary
> PCI devices to other guests.
> Index: head-2006-03-06/drivers/xen/pciback/Makefile
> ===================================================================
> --- head-2006-03-06.orig/drivers/xen/pciback/Makefile 2006-03-06
> 11:15:49.000000000 +0100
> +++ head-2006-03-06/drivers/xen/pciback/Makefile 2006-03-07
> 11:21:26.000000000 +0100
> @@ -1,9 +1,9 @@
> -obj-y += pciback.o
> +obj-$(CONFIG_XEN_PCIDEV_BACKEND) += pciback.o
>
> pciback-y := pci_stub.o pciback_ops.o xenbus.o
> pciback-y += conf_space.o conf_space_header.o
> -pciback-${CONFIG_XEN_PCIDEV_BACKEND_VPCI} += vpci.o
> -pciback-${CONFIG_XEN_PCIDEV_BACKEND_PASS} += passthrough.o
> +pciback-$(CONFIG_XEN_PCIDEV_BACKEND_VPCI) += vpci.o
> +pciback-$(CONFIG_XEN_PCIDEV_BACKEND_PASS) += passthrough.o
>
> ifeq ($(CONFIG_XEN_PCIDEV_BE_DEBUG),y)
> EXTRA_CFLAGS += -DDEBUG
> Index: head-2006-03-06/drivers/xen/pciback/conf_space_header.c
> ===================================================================
> ---
> head-2006-03-06.orig/drivers/xen/pciback/conf_space_header.c
> 2006-03-06 11:15:49.000000000 +0100
> +++
> head-2006-03-06/drivers/xen/pciback/conf_space_header.c 2006-03-07
> 11:57:00.000000000 +0100
> @@ -25,12 +25,12 @@ static int command_write(struct pci_dev
> printk(KERN_DEBUG "pciback: %s: enable\n",
> pci_name(dev));
> dev->is_enabled = 1;
> - pcibios_enable_device(dev, (1 << PCI_NUM_RESOURCES) -
> 1);
> + pci_enable_device(dev);
> } else if (dev->is_enabled && !is_enable_cmd(value)) {
> if (unlikely(verbose_request))
> printk(KERN_DEBUG "pciback: %s: disable\n",
> pci_name(dev));
> - pciback_disable_device(dev);
> + pci_disable_device(dev);
> }
If you change the pciback_disable_device to pci_disable_device, you need
to add a dev->is_enabled = 0 here otherwise the is_enabled flag gets out
of sync with reality.
>
> if (!dev->is_busmaster && is_master_cmd(value)) {
> @@ -38,7 +38,7 @@ static int command_write(struct pci_dev
> printk(KERN_DEBUG "pciback: %s: set bus master
> \n",
> pci_name(dev));
> dev->is_busmaster = 1;
> - pcibios_set_master(dev);
> + pci_set_master(dev);
There's probably not any problem here, but most of pci_set_master is
already done in the frontend (when the frontend device driver calls
pci_set_master), I was trying to avoid duplicating the effects of
pci_set_master here by using pcibios_set_master instead.
> }
>
> if (value & PCI_COMMAND_INVALIDATE) {
> Index: head-2006-03-06/drivers/xen/pciback/pci_stub.c
> ===================================================================
> --- head-2006-03-06.orig/drivers/xen/pciback/pci_stub.c 2006-03-07
> 11:34:12.000000000 +0100
> +++ head-2006-03-06/drivers/xen/pciback/pci_stub.c 2006-03-07
> 12:12:39.000000000 +0100
> @@ -208,7 +208,9 @@ static int __init pcistub_init_devices_l
> return 0;
> }
>
> +#ifndef MODULE
> device_initcall(pcistub_init_devices_late);
> +#endif
>
> static int __devinit pcistub_seize(struct pci_dev *dev)
> {
> @@ -367,6 +369,8 @@ static int __init pcistub_init(void)
> return -EINVAL;
> }
>
> +#ifndef MODULE
> +
> /*
> * fs_initcall happens before device_initcall
> * so pciback *should* get called first (b/c we
> @@ -375,3 +379,33 @@ static int __init pcistub_init(void)
> * driver to register)
> */
> fs_initcall(pcistub_init);
> +
> +#else
> +
> +static __init int pciback_init(void)
> +{
> + int err;
> +
> + err = pcistub_init();
> + if (err < 0)
> + return err;
> + if (list_empty(&pci_stub_device_ids))
> + return -ENODEV;
> + pcistub_init_devices_late();
> + pciback_xenbus_register();
> +
> + __unsafe(THIS_MODULE);
> +
> + return 0;
> +}
> +
> +static void pciback_cleanup(void)
> +{
> + BUG();
> +}
> +
> +module_init(pciback_init);
> +module_exit(pciback_cleanup);
I believe that if you don't specify a module_exit routine at all, you
can't unload the module (see kernel/module.c::sys_delete_module). This
may be better than letting the user accidentally try unloading the
module and get a nasty BUG() message (and then you don't need the
"__unsafe" macro with the scary log message). I don't know the semantics
of what would happen here: if the module unloading proceeds despite the
BUG(), you'd be left with some dangling pointers (because there would be
many places within the PCI code referencing memory within this driver
since no clean-up occurs).
> +
> +MODULE_LICENSE("Dual BSD/GPL");
> +#endif
> Index: head-2006-03-06/drivers/xen/pciback/pciback.h
> ===================================================================
> --- head-2006-03-06.orig/drivers/xen/pciback/pciback.h 2006-03-06
> 11:15:49.000000000 +0100
> +++ head-2006-03-06/drivers/xen/pciback/pciback.h 2006-03-07
> 11:56:12.000000000 +0100
> @@ -43,7 +43,6 @@ struct pci_dev *pcistub_get_pci_dev(stru
> void pcistub_put_pci_dev(struct pci_dev *dev);
>
> /* Ensure a device is turned off or reset */
> -void pciback_disable_device(struct pci_dev *dev);
> void pciback_reset_device(struct pci_dev *pdev);
>
> /* Access a virtual configuration space for a PCI device */
> @@ -69,5 +68,9 @@ void pciback_release_devices(struct pcib
> /* Handles events from front-end */
> irqreturn_t pciback_handle_event(int irq, void *dev_id, struct
> pt_regs *regs);
>
> +#ifdef MODULE
> +int pciback_xenbus_register(void);
> +#endif
> +
> extern int verbose_request;
> #endif
> Index: head-2006-03-06/drivers/xen/pciback/pciback_ops.c
> ===================================================================
> ---
> head-2006-03-06.orig/drivers/xen/pciback/pciback_ops.c 2006-03-06
> 11:15:49.000000000 +0100
> +++ head-2006-03-06/drivers/xen/pciback/pciback_ops.c 2006-03-07
> 11:56:43.000000000 +0100
> @@ -10,17 +10,6 @@
> int verbose_request = 0;
> module_param(verbose_request, int, 0644);
>
> -/* For those architectures without a pcibios_disable_device */
> -void __attribute__ ((weak)) pcibios_disable_device(struct pci_dev
> *dev) { }
> -
> -void pciback_disable_device(struct pci_dev *dev)
> -{
> - if (dev->is_enabled) {
> - dev->is_enabled = 0;
> - pcibios_disable_device(dev);
> - }
> -}
> -
This should be fine. pciback_disable_device used to do something a bit
different in a previous iteration of code, but wherever
pciback_disable_device was called before, you must put a
"dev->is_enabled = 0" in its place.
The other reason I had for using pcibios_disable_device instead of
pci_disable_device (which is the method that replaced the calls to
pciback_disable_device in this patch) is because pci_disable_device is
already run in the frontend and we're just intercepting the end of it. I
don't think it's an issue, but there may be some code duplication (like
reading the PCI_COMMAND register twice to see if the bus master bit is
set).
> /* Ensure a device is "turned off" and ready to be exported.
> * This also sets up the device's private data to keep track of what
> should
> * be in the base address registers (BARs) so that we can keep the
> @@ -32,7 +21,7 @@ void pciback_reset_device(struct pci_dev
>
> /* Disable devices (but not bridges) */
> if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
> - pciback_disable_device(dev);
> + pci_disable_device(dev);
>
> pci_write_config_word(dev, PCI_COMMAND, 0);
>
> Index: head-2006-03-06/drivers/xen/pciback/xenbus.c
> ===================================================================
> --- head-2006-03-06.orig/drivers/xen/pciback/xenbus.c 2006-03-06
> 11:15:49.000000000 +0100
> +++ head-2006-03-06/drivers/xen/pciback/xenbus.c 2006-03-07
> 11:38:59.000000000 +0100
> @@ -430,10 +430,15 @@ static struct xenbus_driver xenbus_pciba
> .otherend_changed = pciback_frontend_changed,
> };
>
> -static __init int pciback_xenbus_register(void)
> +#ifndef MODULE
> +static
> +#endif
> +__init int pciback_xenbus_register(void)
> {
> return xenbus_register_backend(&xenbus_pciback_driver);
> }
>
> +#ifndef MODULE
> /* Must only initialize our xenbus driver after the pcistub driver */
> device_initcall(pciback_xenbus_register);
> +#endif
>
As a coding style preference of mine, if you want to make this code
module-friendly, I would just make this function non-static and always
call it from pci_stub.c rather than have the #ifdef MODULE stuff here.
All those preprocessor statements are intimidating... :)
Ryan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |