Jan Beulich
2006-Mar-08 11:06 UTC
[Xen-devel] [PATCH] linux: allow pciback to be built as a module
Also, a little bit of cleanup. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ryan
2006-Mar-08 16:05 UTC
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@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Mar-08 16:30 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
On 8 Mar 2006, at 16:05, Ryan wrote:> 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).Installing pciback early is definitely the best way to go, but it would be nice to support later loading as a module despite the limitations that imposes. I was minded to take Jan''s patch, my main concern being that using the pci_* functions instead of pcibios_* functions may have adverse side effects (Jan uses them only because they are the ones exported to modules). One way around that possible problem would be to define pciback_* functions that call pcibios_* functions as in the current driver when compiled into the kernel image, or call the pci_* functions when compiled as a module. How does that sound? Another nice thing to support in pciback would be late binding to a PCI device, if it hasn''t already been bound to by some other driver. This would avoid needing to specify hiding devices as a boot parameter if the administrator knows that no driver will try to bind to that device. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2006-Mar-08 16:34 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
>>> Ryan <hap9@epoch.ncsc.mil> 08.03.06 17:05:42 >>> >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).I understood that.>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).That''s why I wanted it to allow being a module; if whoever configures the kernel wants to avoid this two-step process, he can simply say ''Y'' to the config question.>> - 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.I don''t think so, that''s one of the significant differences between calling pcibios_disable_device() and pci_disable_device() - the latter takes care of that adjustment.>> - 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.The main reason for these changes is that only pci_* are exported, not pcibios_*.>> + __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).Correct. If indeed the module is meant to never be unloaded (which I would hope it isn''t), then this should be done the way you say; I did it the other way because I wanted to retain a reminder that this needs work.>> -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).Same as above.>> -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... :)Yes, probably that''d be cleaner... Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2006-Mar-08 16:38 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
>One way around that possible problem would be to define pciback_* >functions that call pcibios_* functions as in the current driver when >compiled into the kernel image, or call the pci_* functions when >compiled as a module. How does that sound?I thought about that option, too, but it seemed a hack to me. I''d like to go that route only when it is known that not using the pcibios_* functions has bad side effects beyond slightly worse performance. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ryan
2006-Mar-08 19:01 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
On Wed, 2006-03-08 at 17:34 +0100, Jan Beulich wrote:> >>> Ryan <hap9@epoch.ncsc.mil> 08.03.06 17:05:42 >>> > >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). > > I understood that. > > >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). > > That''s why I wanted it to allow being a module; if whoever configures the kernel > wants to avoid this two-step process, he can simply say ''Y'' to the config question. >That''s fine. I just wanted to make sure that the trade-offs between the approaches were understood. I might recommend, though, that you add a blurb to the help section in Kconfig so that people doing a custom-compile will understand the trade-offs between built-in and module that we''ve discussed here.> > >> - 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. > > I don''t think so, that''s one of the significant differences between calling > pcibios_disable_device() and pci_disable_device() - the latter takes care of > that adjustment. >Yes, you are correct. I missed that. :)> >> - 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. > > The main reason for these changes is that only pci_* are exported, not pcibios_*. >I think the only place where there could potentially be an issue is changing pcibios_enable_device to pci_enable_device. pci_enable_device calls pci_fixup_device which *may* have some side effects that a non-linux driver domain may not expect or want. Maybe you could call pci_enable_device_bars (which is exported) just like it''s called in pci_enable_device to skip the pci_fixup_device method. Ryan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ryan
2006-Mar-08 19:07 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
On Wed, 2006-03-08 at 16:30 +0000, Keir Fraser wrote:> On 8 Mar 2006, at 16:05, Ryan wrote: > > > 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). > > Installing pciback early is definitely the best way to go, but it would > be nice to support later loading as a module despite the limitations > that imposes. I was minded to take Jan''s patch, my main concern being > that using the pci_* functions instead of pcibios_* functions may have > adverse side effects (Jan uses them only because they are the ones > exported to modules).> One way around that possible problem would be to define pciback_* > functions that call pcibios_* functions as in the current driver when > compiled into the kernel image, or call the pci_* functions when > compiled as a module. How does that sound? >I don''t think there''s any problems with using the pci_* functions instead, but I haven''t tried it. Except for pci_enable_device (which could be replaced by pci_enable_device_bars to avoid potential side-effects in pci_fixup_device), I think in the worst case scenario, you read a pci configuration space register twice (once in the frontend and again in the backend) instead of once.> > Another nice thing to support in pciback would be late binding to a PCI > device, if it hasn''t already been bound to by some other driver. This > would avoid needing to specify hiding devices as a boot parameter if > the administrator knows that no driver will try to bind to that device. >Do you mean if no other driver grabs a device, have that device attach to the pcistub driver? I have a type of "late binding" support coded up and working, but it isn''t quite ready for submission yet. You can add/remove PCI slots to/from the pcistub driver at run-time. It doesn''t automatically pick-up devices that don''t have a driver, but it is useful for working with the bind/unbind driver attributes in sysfs in Linux. This would also be useful in scenarios with cardbus where you *may* not know the slot numbers in advance. Or if the administrator simply decides to change his mind and take, for example, a network card out of service in dom0 and put it in a domU or vice versa. Ryan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Mar-08 20:01 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
On 8 Mar 2006, at 19:07, Ryan wrote:> Do you mean if no other driver grabs a device, have that device attach > to the pcistub driver?I mean have some way of asking the pcistub driver to late-bind, but not have pcistub bind greedily and automatically.> I have a type of "late binding" support coded up and working, but it > isn''t quite ready for submission yet. You can add/remove PCI slots > to/from the pcistub driver at run-time. It doesn''t automatically > pick-up > devices that don''t have a driver, but it is useful for working with the > bind/unbind driver attributes in sysfs in Linux. This would also be > useful in scenarios with cardbus where you *may* not know the slot > numbers in advance. Or if the administrator simply decides to change > his > mind and take, for example, a network card out of service in dom0 and > put it in a domU or vice versa.It sounds like you have basically what I''m talking about already working. I''m not sure it makes sense to have pciback greedily bind to PCI devices. If you don;t have to bind really early (to avoid other driver probes during boot) then you can probably leave the bind until driver-domain creation. It''d be great to get a patch for that in the tree, and hook it into the tools (so that driver-domain creation fails only if the device is already bound to another driver --- currently we fail if the device is not already bound to pciback). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2006-Mar-09 09:26 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
>> >> - 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. >> >> The main reason for these changes is that only pci_* are exported, not pcibios_*. >> > >I think the only place where there could potentially be an issue is >changing pcibios_enable_device to pci_enable_device. pci_enable_device >calls pci_fixup_device which *may* have some side effects that a >non-linux driver domain may not expect or want. Maybe you could call >pci_enable_device_bars (which is exported) just like it''s called in >pci_enable_device to skip the pci_fixup_device method.I''m not certain about that - pci_fixup_device is called in a number of other places (with different stage arguments), most notably in pci_init for *all* devices, and hence not calling it here might result in inconsistencies. I would want to either suppress all of the calls for a device handled outside dom0, or none. Since suppressing all (or reverting possible previous adjustments) is going to be at least difficult, I think letting the call happen here is better. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ryan
2006-Mar-09 13:58 UTC
Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module
On Thu, 2006-03-09 at 10:26 +0100, Jan Beulich wrote:> >> >> - 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. > >> > >> The main reason for these changes is that only pci_* are exported, not pcibios_*. > >> > > > >I think the only place where there could potentially be an issue is > >changing pcibios_enable_device to pci_enable_device. pci_enable_device > >calls pci_fixup_device which *may* have some side effects that a > >non-linux driver domain may not expect or want. Maybe you could call > >pci_enable_device_bars (which is exported) just like it''s called in > >pci_enable_device to skip the pci_fixup_device method. > > I''m not certain about that - pci_fixup_device is called in a number of other places > (with different stage arguments), most notably in pci_init for *all* devices, and > hence not calling it here might result in inconsistencies. I would want to either > suppress all of the calls for a device handled outside dom0, or none. Since > suppressing all (or reverting possible previous adjustments) is going to be at > least difficult, I think letting the call happen here is better. >Well it appears that all of the fixups done on the enable hook (there''s only 3 of them in 2.6.16-rc5!) are pretty innocent so I guess there really isn''t a concern about this now. By concern about the enable was changing random registers while the domU thinks it is the only one in charge of the card. All of the other calls to pci_fixup_device happen at points in the kernel before the pci backend can prepare a device for export so changes won''t happen while a domU is controlling a device (the pci_fixup_device call from pci_init happens on device_initcall, drivers/pci gets linked in before drivers/xen so it should get called first). the pci_fixup_device in pci_enable_device is the one point where a fixup hook could change something significant without the domU aware. It appears that there''s nothing to worry about with the current code in the kernel, but someone could always add a new fixup hook in the future. I think the correct way to do this is use pci_enable_device_bars and skip the pci_fixup_device, but I don''t see any problem with just using pci_enable_device right now. Ryan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2006-Mar-09 14:41 UTC
[Xen-devel] [PATCH, updated] linux: allow pciback to be built as a module
Attached an update patch; most of the changes requested by Ryan have already been carried out, presumably by Keir. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel