Re: [libvirt] [RFC PATCH 1/9] vfio/pci: introduce mediate ops to intercept vfio-pci ops

> > > Currently, yes, i40e has build dependency on vfio-pci. > > > It's like this, if i40e decides to support SRIOV and compiles in vf > > > related code who depends on vfio-pci, it will also have build dependency > > > on vfio-pci. isn't it natural? > > > > No, this is not natural. There are certainly i40e VF use cases that > > have no interest in vfio and having dependencies between the two > > modules is unacceptable. I think you probably want to modularize the > > i40e vfio support code and then perhaps register a table in vfio-pci > > that the vfio-pci code can perform a module request when using a > > compatible device. Just and idea, there might be better options. I > > will not accept a solution that requires unloading the i40e driver in > > order to unload the vfio-pci driver. It's inconvenient with just one > > NIC driver, imagine how poorly that scales. > > > what about this way: > mediate driver registers a module notifier and every time when > vfio_pci is loaded, register to vfio_pci its mediate ops? > (Just like in below sample code) > This way vfio-pci is free to unload and this registering only gives > vfio-pci a name of what module to request. > After that, > in vfio_pci_open(), vfio-pci requests the mediate driver. (or puts > the mediate driver when mediate driver does not support mediating the > device) > in vfio_pci_release(), vfio-pci puts the mediate driver. > > static void register_mediate_ops(void) > { > int (*func)(struct vfio_pci_mediate_ops *ops) = NULL; > > func = symbol_get(vfio_pci_register_mediate_ops); > > if (func) { > func(&igd_dt_ops); > symbol_put(vfio_pci_register_mediate_ops); > } > } > > static int igd_module_notify(struct notifier_block *self, > unsigned long val, void *data) > { > struct module *mod = data; > int ret = 0; > > switch (val) { > case MODULE_STATE_LIVE: > if (!strcmp(mod->name, "vfio_pci")) > register_mediate_ops(); > break; > case MODULE_STATE_GOING: > break; > default: > break; > } > return ret; > } > > static struct notifier_block igd_module_nb = { > .notifier_call = igd_module_notify, > .priority = 0, > }; > > > > static int __init igd_dt_init(void) > { > ... > register_mediate_ops(); > register_module_notifier(&igd_module_nb); > ... > return 0; > } No, this is bad. Please look at MODULE_ALIAS() and request_module() as used in the vfio-platform for loading reset driver modules. I think the correct approach is that vfio-pci should perform a request_module() based on the device being probed. Having the mediation provider listening for vfio-pci and registering itself regardless of whether we intend to use it assumes that we will want to use it and assumes that the mediation provider module is already loaded. We should be able to support demand loading of modules that may serve no other purpose than providing this mediation. Thanks,