RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> From: Alex Williamson [mailto:alex.william...@redhat.com] > Sent: Tuesday, March 26, 2019 11:35 PM > To: Liu, Yi L > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > On Tue, 26 Mar 2019 12:37:37 + > "Liu, Yi L" wrote: > > > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > > Sent: Tuesday, March 26, 2019 2:17 AM > > > To: Liu, Yi L > > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > > > > > On Sat, 23 Mar 2019 11:06:44 + > > > "Liu, Yi L" wrote: > > > > Hi Alex, > > > > [...] > > > > > > > > > > I tried to get a common file which includes the definitions of the > > > > module > > > > options and the common interfaces and get it linked separately with each > > > > module. It works well when linked separately by config the > > > > CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel > > > > configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro > > > > for the mdev wrapped version driver. However, if building the vfio-pci > > > > and the mdev wrapped version into kernel image (config the > > > > CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols > > > > defined in the common file will be shared thus doesn't allow dissimilar > > > > user settings. > > > > > > > > Per my understanding, I think we expect to allow simultaneous usage of > > > > the two drivers. So I think the way above doesn't meet our expectation. > > > > > > I agree. They should be related in implementation only, from a user > > > perspective they should be entirely separate. > > > > > > > I considered a possible proposal as below. May listen to your opinion > > > > on it before heading to cook. Also, better idea is welcomed. :-) > > > > > > > > - get a common file includes interfaces which are common and have > > > > input parameters to differentiate the calling from vfio-pci and the > > > > wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c. > > > > > > > > - get another common file includes the definitions of the module > > > > options, > > > > and the functions which referred the options. Define all of them as > > > > static. > > > > may call it as common.c > > > > > > > > - get vfio_pci.c which includes the module_init/exit interfaces and > > > > driver > > > > registration operations of vfio-pci.ko. This file should include the > common.c > > > > above to have same module options with the mdev wrapped version. > > > > > > > > - get vfio_pci_mdev.c which includes the module_init/exit interfaces and > > > > driver registration operations of vfio-pci-mdev.ko. It should also > > > > include > > > > the common.c above to have same module options with vfio-pci.ko. > > > > > > > > - Makefile: > > > > vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o > vfio_pci_rdwr.o > > > vfio_pci_config.o > > > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o > > > > vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o > > > > > > > > vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o > > > vfio_pci_rdwr.o vfio_pci_config.o > > > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o > > > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o > > > > > > > > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o > > > > obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o > > > > > > Each module needs it's own module_init/exit and will register its own > > > struct pci_driver, which gives us separate control of the probe and > > > > Agreed. > > > > > remove callbacks. I think we want the drivers to have the same module > > > parameters initially, but we don't necessarily want to require it for > > > any future options, so we can duplicate the parameter declarations. > > > Then to support the shared code, I think we can easily push nointxmask, > > > disable_vga, and disable_idle_d3 into bools on the struct > > > vfio_pci_device, which would be allocated and set by each module's > > > probe function before calling the shared probe function. > > > > sounds good to me. > > > > > vfio_fi
Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
On Tue, 26 Mar 2019 12:37:37 +
"Liu, Yi L" wrote:
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Tuesday, March 26, 2019 2:17 AM
> > To: Liu, Yi L
> > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> >
> > On Sat, 23 Mar 2019 11:06:44 +
> > "Liu, Yi L" wrote:
> > > Hi Alex,
>
> [...]
>
> > >
> > > I tried to get a common file which includes the definitions of the module
> > > options and the common interfaces and get it linked separately with each
> > > module. It works well when linked separately by config the
> > > CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel
> > > configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro
> > > for the mdev wrapped version driver. However, if building the vfio-pci
> > > and the mdev wrapped version into kernel image (config the
> > > CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols
> > > defined in the common file will be shared thus doesn't allow dissimilar
> > > user settings.
> > >
> > > Per my understanding, I think we expect to allow simultaneous usage of
> > > the two drivers. So I think the way above doesn't meet our expectation.
> >
> > I agree. They should be related in implementation only, from a user
> > perspective they should be entirely separate.
> >
> > > I considered a possible proposal as below. May listen to your opinion
> > > on it before heading to cook. Also, better idea is welcomed. :-)
> > >
> > > - get a common file includes interfaces which are common and have
> > > input parameters to differentiate the calling from vfio-pci and the
> > > wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c.
> > >
> > > - get another common file includes the definitions of the module options,
> > > and the functions which referred the options. Define all of them as
> > > static.
> > > may call it as common.c
> > >
> > > - get vfio_pci.c which includes the module_init/exit interfaces and driver
> > > registration operations of vfio-pci.ko. This file should include the
> > > common.c
> > > above to have same module options with the mdev wrapped version.
> > >
> > > - get vfio_pci_mdev.c which includes the module_init/exit interfaces and
> > > driver registration operations of vfio-pci-mdev.ko. It should also
> > > include
> > > the common.c above to have same module options with vfio-pci.ko.
> > >
> > > - Makefile:
> > > vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o
> > > vfio_pci_rdwr.o
> > vfio_pci_config.o
> > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> > >
> > > vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o
> > vfio_pci_rdwr.o vfio_pci_config.o
> > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> > >
> > > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> > > obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o
> >
> > Each module needs it's own module_init/exit and will register its own
> > struct pci_driver, which gives us separate control of the probe and
>
> Agreed.
>
> > remove callbacks. I think we want the drivers to have the same module
> > parameters initially, but we don't necessarily want to require it for
> > any future options, so we can duplicate the parameter declarations.
> > Then to support the shared code, I think we can easily push nointxmask,
> > disable_vga, and disable_idle_d3 into bools on the struct
> > vfio_pci_device, which would be allocated and set by each module's
> > probe function before calling the shared probe function.
>
> sounds good to me.
>
> > vfio_fill_ids() could take a pointer to the array to keep them separate
> > between modules.
>
> Agreed.
>
> > I think that just leaves the config permission bits,
> > vfio_pci_{un}init_perm_bits(). Could we use a simple atomic reference
> > counter on those to potentially share them so they get initialized by
> > the first caller and freed by the last user, at least in the case of
> > both drivers being compiled statically into the kernel? Thanks,
>
> Sure, I can add it. The two modules will still share the cap_perms and
> ecap_perms config bits when built statically in kernel. However, I think
> such share is reasonable. I'll check if any other similar bits in other files.
>
> > Alex
>
> Thanks for the suggestions, Alex. Let me prepare another RFC.
Thank Yi, I appreciate your work on this. Also, I wonder if we might
want to reconsider placing this driver in samples, the Makefile might
be a little bit ugly with paths back to drivers/vfio/pci, but I don't
think we run into the same barriers as you did with previous
approaches. Placing it in samples would at least alleviate any
confusion that this isn't a vfio-pci replacement, but more of an mdev
wrapper proof of concept. Thanks,
Alex
RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, March 26, 2019 2:17 AM
> To: Liu, Yi L
> Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
>
> On Sat, 23 Mar 2019 11:06:44 +
> "Liu, Yi L" wrote:
> > Hi Alex,
[...]
> >
> > I tried to get a common file which includes the definitions of the module
> > options and the common interfaces and get it linked separately with each
> > module. It works well when linked separately by config the
> > CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel
> > configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro
> > for the mdev wrapped version driver. However, if building the vfio-pci
> > and the mdev wrapped version into kernel image (config the
> > CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols
> > defined in the common file will be shared thus doesn't allow dissimilar
> > user settings.
> >
> > Per my understanding, I think we expect to allow simultaneous usage of
> > the two drivers. So I think the way above doesn't meet our expectation.
>
> I agree. They should be related in implementation only, from a user
> perspective they should be entirely separate.
>
> > I considered a possible proposal as below. May listen to your opinion
> > on it before heading to cook. Also, better idea is welcomed. :-)
> >
> > - get a common file includes interfaces which are common and have
> > input parameters to differentiate the calling from vfio-pci and the
> > wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c.
> >
> > - get another common file includes the definitions of the module options,
> > and the functions which referred the options. Define all of them as
> > static.
> > may call it as common.c
> >
> > - get vfio_pci.c which includes the module_init/exit interfaces and driver
> > registration operations of vfio-pci.ko. This file should include the
> > common.c
> > above to have same module options with the mdev wrapped version.
> >
> > - get vfio_pci_mdev.c which includes the module_init/exit interfaces and
> > driver registration operations of vfio-pci-mdev.ko. It should also include
> > the common.c above to have same module options with vfio-pci.ko.
> >
> > - Makefile:
> > vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o
> vfio_pci_config.o
> > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> >
> > vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o
> vfio_pci_rdwr.o vfio_pci_config.o
> > vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> >
> > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> > obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o
>
> Each module needs it's own module_init/exit and will register its own
> struct pci_driver, which gives us separate control of the probe and
Agreed.
> remove callbacks. I think we want the drivers to have the same module
> parameters initially, but we don't necessarily want to require it for
> any future options, so we can duplicate the parameter declarations.
> Then to support the shared code, I think we can easily push nointxmask,
> disable_vga, and disable_idle_d3 into bools on the struct
> vfio_pci_device, which would be allocated and set by each module's
> probe function before calling the shared probe function.
sounds good to me.
> vfio_fill_ids() could take a pointer to the array to keep them separate
> between modules.
Agreed.
> I think that just leaves the config permission bits,
> vfio_pci_{un}init_perm_bits(). Could we use a simple atomic reference
> counter on those to potentially share them so they get initialized by
> the first caller and freed by the last user, at least in the case of
> both drivers being compiled statically into the kernel? Thanks,
Sure, I can add it. The two modules will still share the cap_perms and
ecap_perms config bits when built statically in kernel. However, I think
such share is reasonable. I'll check if any other similar bits in other files.
> Alex
Thanks for the suggestions, Alex. Let me prepare another RFC.
Regards,
Yi Liu
Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
On Sat, 23 Mar 2019 11:06:44 + "Liu, Yi L" wrote: > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > Sent: Thursday, March 21, 2019 3:22 AM > > To: Liu, Yi L > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > > > On Wed, 20 Mar 2019 11:49:37 + > > "Liu, Yi L" wrote: > > > > > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > > > Sent: Wednesday, March 20, 2019 2:14 AM > > > > To: Liu, Yi L > > > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > > > > > > > On Tue, 12 Mar 2019 16:18:22 +0800 > > > > "Liu, Yi L" wrote: > > > > > > > > > This patch exports the following symbols from vfio-pci driver > > > > > for vfio-pci alike driver. e.g. vfio-pci-mdev driver > > > > > > > > > > *) vfio_pci_set_vga_decode(); > > > > > *) vfio_pci_release(); > > > > > *) vfio_pci_open(); > > > > > *) vfio_pci_register_dev_region(); > > > > > *) vfio_pci_ioctl(); > > > > > *) vfio_pci_rw(); > > > > > *) vfio_pci_mmap(); > > > > > *) vfio_pci_request(); > > > > > *) vfio_pci_probe_misc(); > > > > > *) vfio_pci_remove_misc(); > > > > > *) vfio_err_handlers; > > > > > *) vfio_pci_reflck_attach(); > > > > > *) vfio_pci_reflck_put(); > > > > > > > > Exporting all these symbols scares me a bit. They're GPL and we don't > > > > guarantee a kernel ABI, but I don't really want to support arbitrary > > > > use cases either. What if we re-factored the shared bits into a common > > > > file and just linked them together? Thanks, > > > > > > Hi, Alex, > > > > > > Before refactor the code, I'd like to check with you on the module > > > parameters for the two modules. The existing vfio-pci driver has > > > some module parameters. e.g. ids, nointxmask, disable_idle_d3. > > > For future usage and maintain, I think it is better to let the two > > > drivers have same parameters. However, I'm not 100% on whether > > > we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with > > > different parameter value? e.g. load vfio-pci.ko with nointxmask=false > > > while load vfio-pci-mdev.ko with nointxmask=true. How about your > > > opinion on it? > > > > Hi Yi, > > > > I agree that it makes sense to retain the module options for the mdev > > wrapped version, but I expect we should also allow dissimilar user > > settings. If those lived in the common code that gets linked separately > > with each module, that should work fine, I think. We can worry about > > refactoring for future driver that might not want those options later. > > Hi Alex, > > I tried to get a common file which includes the definitions of the module > options and the common interfaces and get it linked separately with each > module. It works well when linked separately by config the > CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel > configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro > for the mdev wrapped version driver. However, if building the vfio-pci > and the mdev wrapped version into kernel image (config the > CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols > defined in the common file will be shared thus doesn't allow dissimilar > user settings. > > Per my understanding, I think we expect to allow simultaneous usage of > the two drivers. So I think the way above doesn't meet our expectation. I agree. They should be related in implementation only, from a user perspective they should be entirely separate. > I considered a possible proposal as below. May listen to your opinion > on it before heading to cook. Also, better idea is welcomed. :-) > > - get a common file includes interfaces which are common and have > input parameters to differentiate the calling from vfio-pci and the > wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c. > > - get another common file includes the definitions of the module options, > and the functions which referred the options. Define all of them as static. > may call it as common.c > > - get vfio_pci.c which includes the module_init/exit interfaces and driver > registration operations of vfio-pci.ko. This file should include the > common.c > above to have same module options with the mdev wrapped version. > > - get vfio_pci_mdev.c wh
RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> From: Alex Williamson [mailto:alex.william...@redhat.com] > Sent: Thursday, March 21, 2019 3:22 AM > To: Liu, Yi L > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > On Wed, 20 Mar 2019 11:49:37 + > "Liu, Yi L" wrote: > > > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > > Sent: Wednesday, March 20, 2019 2:14 AM > > > To: Liu, Yi L > > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > > > > > On Tue, 12 Mar 2019 16:18:22 +0800 > > > "Liu, Yi L" wrote: > > > > > > > This patch exports the following symbols from vfio-pci driver > > > > for vfio-pci alike driver. e.g. vfio-pci-mdev driver > > > > > > > > *) vfio_pci_set_vga_decode(); > > > > *) vfio_pci_release(); > > > > *) vfio_pci_open(); > > > > *) vfio_pci_register_dev_region(); > > > > *) vfio_pci_ioctl(); > > > > *) vfio_pci_rw(); > > > > *) vfio_pci_mmap(); > > > > *) vfio_pci_request(); > > > > *) vfio_pci_probe_misc(); > > > > *) vfio_pci_remove_misc(); > > > > *) vfio_err_handlers; > > > > *) vfio_pci_reflck_attach(); > > > > *) vfio_pci_reflck_put(); > > > > > > Exporting all these symbols scares me a bit. They're GPL and we don't > > > guarantee a kernel ABI, but I don't really want to support arbitrary > > > use cases either. What if we re-factored the shared bits into a common > > > file and just linked them together? Thanks, > > > > Hi, Alex, > > > > Before refactor the code, I'd like to check with you on the module > > parameters for the two modules. The existing vfio-pci driver has > > some module parameters. e.g. ids, nointxmask, disable_idle_d3. > > For future usage and maintain, I think it is better to let the two > > drivers have same parameters. However, I'm not 100% on whether > > we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with > > different parameter value? e.g. load vfio-pci.ko with nointxmask=false > > while load vfio-pci-mdev.ko with nointxmask=true. How about your > > opinion on it? > > Hi Yi, > > I agree that it makes sense to retain the module options for the mdev > wrapped version, but I expect we should also allow dissimilar user > settings. If those lived in the common code that gets linked separately > with each module, that should work fine, I think. We can worry about > refactoring for future driver that might not want those options later. Hi Alex, I tried to get a common file which includes the definitions of the module options and the common interfaces and get it linked separately with each module. It works well when linked separately by config the CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro for the mdev wrapped version driver. However, if building the vfio-pci and the mdev wrapped version into kernel image (config the CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols defined in the common file will be shared thus doesn't allow dissimilar user settings. Per my understanding, I think we expect to allow simultaneous usage of the two drivers. So I think the way above doesn't meet our expectation. I considered a possible proposal as below. May listen to your opinion on it before heading to cook. Also, better idea is welcomed. :-) - get a common file includes interfaces which are common and have input parameters to differentiate the calling from vfio-pci and the wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c. - get another common file includes the definitions of the module options, and the functions which referred the options. Define all of them as static. may call it as common.c - get vfio_pci.c which includes the module_init/exit interfaces and driver registration operations of vfio-pci.ko. This file should include the common.c above to have same module options with the mdev wrapped version. - get vfio_pci_mdev.c which includes the module_init/exit interfaces and driver registration operations of vfio-pci-mdev.ko. It should also include the common.c above to have same module options with vfio-pci.ko. - Makefile: vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o obj-$(CONFIG_VFIO_PCI) += vfio-pci.o obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o Thanks, Yi Liu
Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
On Wed, 20 Mar 2019 11:49:37 + "Liu, Yi L" wrote: > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > Sent: Wednesday, March 20, 2019 2:14 AM > > To: Liu, Yi L > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > > > On Tue, 12 Mar 2019 16:18:22 +0800 > > "Liu, Yi L" wrote: > > > > > This patch exports the following symbols from vfio-pci driver > > > for vfio-pci alike driver. e.g. vfio-pci-mdev driver > > > > > > *) vfio_pci_set_vga_decode(); > > > *) vfio_pci_release(); > > > *) vfio_pci_open(); > > > *) vfio_pci_register_dev_region(); > > > *) vfio_pci_ioctl(); > > > *) vfio_pci_rw(); > > > *) vfio_pci_mmap(); > > > *) vfio_pci_request(); > > > *) vfio_pci_probe_misc(); > > > *) vfio_pci_remove_misc(); > > > *) vfio_err_handlers; > > > *) vfio_pci_reflck_attach(); > > > *) vfio_pci_reflck_put(); > > > > Exporting all these symbols scares me a bit. They're GPL and we don't > > guarantee a kernel ABI, but I don't really want to support arbitrary > > use cases either. What if we re-factored the shared bits into a common > > file and just linked them together? Thanks, > > Hi, Alex, > > Before refactor the code, I'd like to check with you on the module > parameters for the two modules. The existing vfio-pci driver has > some module parameters. e.g. ids, nointxmask, disable_idle_d3. > For future usage and maintain, I think it is better to let the two > drivers have same parameters. However, I'm not 100% on whether > we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with > different parameter value? e.g. load vfio-pci.ko with nointxmask=false > while load vfio-pci-mdev.ko with nointxmask=true. How about your > opinion on it? Hi Yi, I agree that it makes sense to retain the module options for the mdev wrapped version, but I expect we should also allow dissimilar user settings. If those lived in the common code that gets linked separately with each module, that should work fine, I think. We can worry about refactoring for future driver that might not want those options later. Thanks, Alex
RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> From: Alex Williamson [mailto:alex.william...@redhat.com] > Sent: Wednesday, March 20, 2019 2:14 AM > To: Liu, Yi L > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci > > On Tue, 12 Mar 2019 16:18:22 +0800 > "Liu, Yi L" wrote: > > > This patch exports the following symbols from vfio-pci driver > > for vfio-pci alike driver. e.g. vfio-pci-mdev driver > > > > *) vfio_pci_set_vga_decode(); > > *) vfio_pci_release(); > > *) vfio_pci_open(); > > *) vfio_pci_register_dev_region(); > > *) vfio_pci_ioctl(); > > *) vfio_pci_rw(); > > *) vfio_pci_mmap(); > > *) vfio_pci_request(); > > *) vfio_pci_probe_misc(); > > *) vfio_pci_remove_misc(); > > *) vfio_err_handlers; > > *) vfio_pci_reflck_attach(); > > *) vfio_pci_reflck_put(); > > Exporting all these symbols scares me a bit. They're GPL and we don't > guarantee a kernel ABI, but I don't really want to support arbitrary > use cases either. What if we re-factored the shared bits into a common > file and just linked them together? Thanks, Hi, Alex, Before refactor the code, I'd like to check with you on the module parameters for the two modules. The existing vfio-pci driver has some module parameters. e.g. ids, nointxmask, disable_idle_d3. For future usage and maintain, I think it is better to let the two drivers have same parameters. However, I'm not 100% on whether we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with different parameter value? e.g. load vfio-pci.ko with nointxmask=false while load vfio-pci-mdev.ko with nointxmask=true. How about your opinion on it? > > Alex Thanks, Yi Liu
Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
On Tue, 12 Mar 2019 16:18:22 +0800
"Liu, Yi L" wrote:
> This patch exports the following symbols from vfio-pci driver
> for vfio-pci alike driver. e.g. vfio-pci-mdev driver
>
> *) vfio_pci_set_vga_decode();
> *) vfio_pci_release();
> *) vfio_pci_open();
> *) vfio_pci_register_dev_region();
> *) vfio_pci_ioctl();
> *) vfio_pci_rw();
> *) vfio_pci_mmap();
> *) vfio_pci_request();
> *) vfio_pci_probe_misc();
> *) vfio_pci_remove_misc();
> *) vfio_err_handlers;
> *) vfio_pci_reflck_attach();
> *) vfio_pci_reflck_put();
Exporting all these symbols scares me a bit. They're GPL and we don't
guarantee a kernel ABI, but I don't really want to support arbitrary
use cases either. What if we re-factored the shared bits into a common
file and just linked them together? Thanks,
Alex
> Cc: Kevin Tian
> Cc: Lu Baolu
> Signed-off-by: Liu, Yi L
> ---
> drivers/vfio/pci/vfio_pci.c | 101
> ++--
> drivers/vfio/pci/vfio_pci_private.h | 17 ++
> 2 files changed, 78 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index ff60bd1..e36b922 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -73,7 +73,7 @@ static inline bool vfio_vga_disabled(void)
> * has no way to get to it and routing can be disabled externally at the
> * bridge.
> */
> -static unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
> +unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
> {
> struct vfio_pci_device *vdev = opaque;
> struct pci_dev *tmp = NULL, *pdev = vdev->pdev;
> @@ -103,6 +103,7 @@ static unsigned int vfio_pci_set_vga_decode(void *opaque,
> bool single_vga)
>
> return decodes;
> }
> +EXPORT_SYMBOL_GPL(vfio_pci_set_vga_decode);
>
> static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
> {
> @@ -410,7 +411,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
> pci_set_power_state(pdev, PCI_D3hot);
> }
>
> -static void vfio_pci_release(void *device_data)
> +void vfio_pci_release(void *device_data)
> {
> struct vfio_pci_device *vdev = device_data;
>
> @@ -425,8 +426,9 @@ static void vfio_pci_release(void *device_data)
>
> module_put(THIS_MODULE);
> }
> +EXPORT_SYMBOL_GPL(vfio_pci_release);
>
> -static int vfio_pci_open(void *device_data)
> +int vfio_pci_open(void *device_data)
> {
> struct vfio_pci_device *vdev = device_data;
> int ret = 0;
> @@ -450,6 +452,7 @@ static int vfio_pci_open(void *device_data)
> module_put(THIS_MODULE);
> return ret;
> }
> +EXPORT_SYMBOL_GPL(vfio_pci_open);
>
> static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
> {
> @@ -634,9 +637,10 @@ int vfio_pci_register_dev_region(struct vfio_pci_device
> *vdev,
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(vfio_pci_register_dev_region);
>
> -static long vfio_pci_ioctl(void *device_data,
> -unsigned int cmd, unsigned long arg)
> +long vfio_pci_ioctl(void *device_data,
> +unsigned int cmd, unsigned long arg)
> {
> struct vfio_pci_device *vdev = device_data;
> unsigned long minsz;
> @@ -1079,8 +1083,9 @@ static long vfio_pci_ioctl(void *device_data,
>
> return -ENOTTY;
> }
> +EXPORT_SYMBOL_GPL(vfio_pci_ioctl);
>
> -static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
> +ssize_t vfio_pci_rw(void *device_data, char __user *buf,
> size_t count, loff_t *ppos, bool iswrite)
> {
> unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> @@ -,6 +1116,7 @@ static ssize_t vfio_pci_rw(void *device_data, char
> __user *buf,
>
> return -EINVAL;
> }
> +EXPORT_SYMBOL_GPL(vfio_pci_rw);
>
> static ssize_t vfio_pci_read(void *device_data, char __user *buf,
>size_t count, loff_t *ppos)
> @@ -1130,7 +1136,7 @@ static ssize_t vfio_pci_write(void *device_data, const
> char __user *buf,
> return vfio_pci_rw(device_data, (char __user *)buf, count, ppos, true);
> }
>
> -static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> +int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> {
> struct vfio_pci_device *vdev = device_data;
> struct pci_dev *pdev = vdev->pdev;
> @@ -1173,7 +1179,7 @@ static int vfio_pci_mmap(void *device_data, struct
> vm_area_struct *vma)
>*/
> if (!vdev->barmap[index]) {
> ret = pci_request_selected_regions(pdev,
> -1 << index, "vfio-pci");
> +1 << index, vdev->name);
> if (ret)
> return ret;
>
> @@ -1191,8 +1197,9 @@ static int vfio_pci_mmap(void *device_data, struct
> vm_area_struct *vma)
> return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> req_len,
