diff mbox series

[12/12] vfio/pci: Introduce vfio_pci_core.ko

Message ID 20210721161609.68223-13-yishaih@nvidia.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Introduce vfio_pci_core subsystem | expand

Commit Message

Yishai Hadas July 21, 2021, 4:16 p.m. UTC
From: Max Gurtovoy <mgurtovoy@nvidia.com>

Now that vfio_pci has been split into two source modules, one focusing
on the "struct pci_driver" (vfio_pci.c) and a toolbox library of code
(vfio_pci_core.c), complete the split and move them into two different
kernel modules.

As before vfio_pci.ko continues to present the same interface under
sysfs and this change will have no functional impact.

Splitting into another module and adding exports allows creating new HW
specific VFIO PCI drivers that can implement device specific
functionality, such as VFIO migration interfaces or specialized device
requirements.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
 drivers/vfio/pci/Kconfig                      | 30 ++++++++------
 drivers/vfio/pci/Makefile                     |  8 ++--
 drivers/vfio/pci/vfio_pci.c                   | 14 ++-----
 drivers/vfio/pci/vfio_pci_config.c            |  2 +-
 drivers/vfio/pci/vfio_pci_core.c              | 41 ++++++++++++++++---
 drivers/vfio/pci/vfio_pci_igd.c               |  2 +-
 drivers/vfio/pci/vfio_pci_intrs.c             |  2 +-
 drivers/vfio/pci/vfio_pci_rdwr.c              |  2 +-
 drivers/vfio/pci/vfio_pci_zdev.c              |  2 +-
 .../pci => include/linux}/vfio_pci_core.h     |  2 -
 10 files changed, 66 insertions(+), 39 deletions(-)
 rename {drivers/vfio/pci => include/linux}/vfio_pci_core.h (99%)

Comments

Leon Romanovsky July 21, 2021, 5:39 p.m. UTC | #1
On Wed, Jul 21, 2021 at 07:16:09PM +0300, Yishai Hadas wrote:
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
> 
> Now that vfio_pci has been split into two source modules, one focusing
> on the "struct pci_driver" (vfio_pci.c) and a toolbox library of code
> (vfio_pci_core.c), complete the split and move them into two different
> kernel modules.
> 
> As before vfio_pci.ko continues to present the same interface under
> sysfs and this change will have no functional impact.
> 
> Splitting into another module and adding exports allows creating new HW
> specific VFIO PCI drivers that can implement device specific
> functionality, such as VFIO migration interfaces or specialized device
> requirements.
> 
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/Kconfig                      | 30 ++++++++------
>  drivers/vfio/pci/Makefile                     |  8 ++--
>  drivers/vfio/pci/vfio_pci.c                   | 14 ++-----
>  drivers/vfio/pci/vfio_pci_config.c            |  2 +-
>  drivers/vfio/pci/vfio_pci_core.c              | 41 ++++++++++++++++---
>  drivers/vfio/pci/vfio_pci_igd.c               |  2 +-
>  drivers/vfio/pci/vfio_pci_intrs.c             |  2 +-
>  drivers/vfio/pci/vfio_pci_rdwr.c              |  2 +-
>  drivers/vfio/pci/vfio_pci_zdev.c              |  2 +-
>  .../pci => include/linux}/vfio_pci_core.h     |  2 -
>  10 files changed, 66 insertions(+), 39 deletions(-)
>  rename {drivers/vfio/pci => include/linux}/vfio_pci_core.h (99%)

<...>

> -#include "vfio_pci_core.h"
> +#include <linux/vfio_pci_core.h>
> +
> +#define DRIVER_VERSION  "0.2"

<...>

> +MODULE_VERSION(DRIVER_VERSION);

Please don't add driver versions to the upstream kernel, they useless.

Thanks
Yishai Hadas July 22, 2021, 9:06 a.m. UTC | #2
On 7/21/2021 8:39 PM, Leon Romanovsky wrote:
> On Wed, Jul 21, 2021 at 07:16:09PM +0300, Yishai Hadas wrote:
>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>>
>> Now that vfio_pci has been split into two source modules, one focusing
>> on the "struct pci_driver" (vfio_pci.c) and a toolbox library of code
>> (vfio_pci_core.c), complete the split and move them into two different
>> kernel modules.
>>
>> As before vfio_pci.ko continues to present the same interface under
>> sysfs and this change will have no functional impact.
>>
>> Splitting into another module and adding exports allows creating new HW
>> specific VFIO PCI drivers that can implement device specific
>> functionality, such as VFIO migration interfaces or specialized device
>> requirements.
>>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>> ---
>>   drivers/vfio/pci/Kconfig                      | 30 ++++++++------
>>   drivers/vfio/pci/Makefile                     |  8 ++--
>>   drivers/vfio/pci/vfio_pci.c                   | 14 ++-----
>>   drivers/vfio/pci/vfio_pci_config.c            |  2 +-
>>   drivers/vfio/pci/vfio_pci_core.c              | 41 ++++++++++++++++---
>>   drivers/vfio/pci/vfio_pci_igd.c               |  2 +-
>>   drivers/vfio/pci/vfio_pci_intrs.c             |  2 +-
>>   drivers/vfio/pci/vfio_pci_rdwr.c              |  2 +-
>>   drivers/vfio/pci/vfio_pci_zdev.c              |  2 +-
>>   .../pci => include/linux}/vfio_pci_core.h     |  2 -
>>   10 files changed, 66 insertions(+), 39 deletions(-)
>>   rename {drivers/vfio/pci => include/linux}/vfio_pci_core.h (99%)
> <...>
>
>> -#include "vfio_pci_core.h"
>> +#include <linux/vfio_pci_core.h>
>> +
>> +#define DRIVER_VERSION  "0.2"
> <...>
>
>> +MODULE_VERSION(DRIVER_VERSION);
> Please don't add driver versions to the upstream kernel, they useless.
>
> Thanks

This just preserves the code for driver/module version that was in 
vfio_pci.ko before the split.

However,  this can be removed in V2 if we may need to have.

Yishai
Max Gurtovoy July 22, 2021, 9:22 a.m. UTC | #3
On 7/22/2021 12:06 PM, Yishai Hadas wrote:
> On 7/21/2021 8:39 PM, Leon Romanovsky wrote:
>> On Wed, Jul 21, 2021 at 07:16:09PM +0300, Yishai Hadas wrote:
>>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>
>>> Now that vfio_pci has been split into two source modules, one focusing
>>> on the "struct pci_driver" (vfio_pci.c) and a toolbox library of code
>>> (vfio_pci_core.c), complete the split and move them into two different
>>> kernel modules.
>>>
>>> As before vfio_pci.ko continues to present the same interface under
>>> sysfs and this change will have no functional impact.
>>>
>>> Splitting into another module and adding exports allows creating new HW
>>> specific VFIO PCI drivers that can implement device specific
>>> functionality, such as VFIO migration interfaces or specialized device
>>> requirements.
>>>
>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>>> ---
>>>   drivers/vfio/pci/Kconfig                      | 30 ++++++++------
>>>   drivers/vfio/pci/Makefile                     |  8 ++--
>>>   drivers/vfio/pci/vfio_pci.c                   | 14 ++-----
>>>   drivers/vfio/pci/vfio_pci_config.c            |  2 +-
>>>   drivers/vfio/pci/vfio_pci_core.c              | 41 
>>> ++++++++++++++++---
>>>   drivers/vfio/pci/vfio_pci_igd.c               |  2 +-
>>>   drivers/vfio/pci/vfio_pci_intrs.c             |  2 +-
>>>   drivers/vfio/pci/vfio_pci_rdwr.c              |  2 +-
>>>   drivers/vfio/pci/vfio_pci_zdev.c              |  2 +-
>>>   .../pci => include/linux}/vfio_pci_core.h     |  2 -
>>>   10 files changed, 66 insertions(+), 39 deletions(-)
>>>   rename {drivers/vfio/pci => include/linux}/vfio_pci_core.h (99%)
>> <...>
>>
>>> -#include "vfio_pci_core.h"
>>> +#include <linux/vfio_pci_core.h>
>>> +
>>> +#define DRIVER_VERSION  "0.2"
>> <...>
>>
>>> +MODULE_VERSION(DRIVER_VERSION);
>> Please don't add driver versions to the upstream kernel, they useless.
>>
>> Thanks
>
> This just preserves the code for driver/module version that was in 
> vfio_pci.ko before the split.
>
> However,  this can be removed in V2 if we may need to have.

Right, we already agreed to preserve vfio_pci versioning scheme and 
we'll not add it to new mlx5_vfio_pci or future drivers.


>
> Yishai
>
Leon Romanovsky July 23, 2021, 2:13 p.m. UTC | #4
On Thu, Jul 22, 2021 at 12:22:05PM +0300, Max Gurtovoy wrote:
> 
> On 7/22/2021 12:06 PM, Yishai Hadas wrote:
> > On 7/21/2021 8:39 PM, Leon Romanovsky wrote:
> > > On Wed, Jul 21, 2021 at 07:16:09PM +0300, Yishai Hadas wrote:
> > > > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > 
> > > > Now that vfio_pci has been split into two source modules, one focusing
> > > > on the "struct pci_driver" (vfio_pci.c) and a toolbox library of code
> > > > (vfio_pci_core.c), complete the split and move them into two different
> > > > kernel modules.
> > > > 
> > > > As before vfio_pci.ko continues to present the same interface under
> > > > sysfs and this change will have no functional impact.
> > > > 
> > > > Splitting into another module and adding exports allows creating new HW
> > > > specific VFIO PCI drivers that can implement device specific
> > > > functionality, such as VFIO migration interfaces or specialized device
> > > > requirements.
> > > > 
> > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> > > > ---
> > > >   drivers/vfio/pci/Kconfig                      | 30 ++++++++------
> > > >   drivers/vfio/pci/Makefile                     |  8 ++--
> > > >   drivers/vfio/pci/vfio_pci.c                   | 14 ++-----
> > > >   drivers/vfio/pci/vfio_pci_config.c            |  2 +-
> > > >   drivers/vfio/pci/vfio_pci_core.c              | 41
> > > > ++++++++++++++++---
> > > >   drivers/vfio/pci/vfio_pci_igd.c               |  2 +-
> > > >   drivers/vfio/pci/vfio_pci_intrs.c             |  2 +-
> > > >   drivers/vfio/pci/vfio_pci_rdwr.c              |  2 +-
> > > >   drivers/vfio/pci/vfio_pci_zdev.c              |  2 +-
> > > >   .../pci => include/linux}/vfio_pci_core.h     |  2 -
> > > >   10 files changed, 66 insertions(+), 39 deletions(-)
> > > >   rename {drivers/vfio/pci => include/linux}/vfio_pci_core.h (99%)
> > > <...>
> > > 
> > > > -#include "vfio_pci_core.h"
> > > > +#include <linux/vfio_pci_core.h>
> > > > +
> > > > +#define DRIVER_VERSION  "0.2"
> > > <...>
> > > 
> > > > +MODULE_VERSION(DRIVER_VERSION);
> > > Please don't add driver versions to the upstream kernel, they useless.
> > > 
> > > Thanks
> > 
> > This just preserves the code for driver/module version that was in
> > vfio_pci.ko before the split.
> > 
> > However,  this can be removed in V2 if we may need to have.
> 
> Right, we already agreed to preserve vfio_pci versioning scheme and we'll
> not add it to new mlx5_vfio_pci or future drivers.

There is nothing to preserve, instead of keeping this useless code, just
delete it.

https://lore.kernel.org/ksummit-discuss/CA+55aFx9A=5cc0QZ7CySC4F2K7eYaEfzkdYEc9JaNgCcV25=rg@mail.gmail.com/

Thanks

> 
> 
> > 
> > Yishai
> >
Max Gurtovoy July 25, 2021, 10:45 a.m. UTC | #5
On 7/23/2021 5:13 PM, Leon Romanovsky wrote:
> On Thu, Jul 22, 2021 at 12:22:05PM +0300, Max Gurtovoy wrote:
>> On 7/22/2021 12:06 PM, Yishai Hadas wrote:
>>> On 7/21/2021 8:39 PM, Leon Romanovsky wrote:
>>>> On Wed, Jul 21, 2021 at 07:16:09PM +0300, Yishai Hadas wrote:
>>>>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>>
>>>>> Now that vfio_pci has been split into two source modules, one focusing
>>>>> on the "struct pci_driver" (vfio_pci.c) and a toolbox library of code
>>>>> (vfio_pci_core.c), complete the split and move them into two different
>>>>> kernel modules.
>>>>>
>>>>> As before vfio_pci.ko continues to present the same interface under
>>>>> sysfs and this change will have no functional impact.
>>>>>
>>>>> Splitting into another module and adding exports allows creating new HW
>>>>> specific VFIO PCI drivers that can implement device specific
>>>>> functionality, such as VFIO migration interfaces or specialized device
>>>>> requirements.
>>>>>
>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>>>>> ---
>>>>>    drivers/vfio/pci/Kconfig                      | 30 ++++++++------
>>>>>    drivers/vfio/pci/Makefile                     |  8 ++--
>>>>>    drivers/vfio/pci/vfio_pci.c                   | 14 ++-----
>>>>>    drivers/vfio/pci/vfio_pci_config.c            |  2 +-
>>>>>    drivers/vfio/pci/vfio_pci_core.c              | 41
>>>>> ++++++++++++++++---
>>>>>    drivers/vfio/pci/vfio_pci_igd.c               |  2 +-
>>>>>    drivers/vfio/pci/vfio_pci_intrs.c             |  2 +-
>>>>>    drivers/vfio/pci/vfio_pci_rdwr.c              |  2 +-
>>>>>    drivers/vfio/pci/vfio_pci_zdev.c              |  2 +-
>>>>>    .../pci => include/linux}/vfio_pci_core.h     |  2 -
>>>>>    10 files changed, 66 insertions(+), 39 deletions(-)
>>>>>    rename {drivers/vfio/pci => include/linux}/vfio_pci_core.h (99%)
>>>> <...>
>>>>
>>>>> -#include "vfio_pci_core.h"
>>>>> +#include <linux/vfio_pci_core.h>
>>>>> +
>>>>> +#define DRIVER_VERSION  "0.2"
>>>> <...>
>>>>
>>>>> +MODULE_VERSION(DRIVER_VERSION);
>>>> Please don't add driver versions to the upstream kernel, they useless.
>>>>
>>>> Thanks
>>> This just preserves the code for driver/module version that was in
>>> vfio_pci.ko before the split.
>>>
>>> However,  this can be removed in V2 if we may need to have.
>> Right, we already agreed to preserve vfio_pci versioning scheme and we'll
>> not add it to new mlx5_vfio_pci or future drivers.
> There is nothing to preserve, instead of keeping this useless code, just
> delete it.

Ok I guess we can do it since the is new module vfio_pci_core.ko.

We'll remove it in V2.

>
> https://lore.kernel.org/ksummit-discuss/CA+55aFx9A=5cc0QZ7CySC4F2K7eYaEfzkdYEc9JaNgCcV25=rg@mail.gmail.com/
>
> Thanks
>
>>
>>> Yishai
>>>
Alex Williamson July 27, 2021, 9:54 p.m. UTC | #6
On Wed, 21 Jul 2021 19:16:09 +0300
Yishai Hadas <yishaih@nvidia.com> wrote:

> From: Max Gurtovoy <mgurtovoy@nvidia.com>
> 
> Now that vfio_pci has been split into two source modules, one focusing
> on the "struct pci_driver" (vfio_pci.c) and a toolbox library of code
> (vfio_pci_core.c), complete the split and move them into two different
> kernel modules.
> 
> As before vfio_pci.ko continues to present the same interface under
> sysfs and this change will have no functional impact.
> 
> Splitting into another module and adding exports allows creating new HW
> specific VFIO PCI drivers that can implement device specific
> functionality, such as VFIO migration interfaces or specialized device
> requirements.
> 
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/Kconfig                      | 30 ++++++++------
>  drivers/vfio/pci/Makefile                     |  8 ++--
>  drivers/vfio/pci/vfio_pci.c                   | 14 ++-----
>  drivers/vfio/pci/vfio_pci_config.c            |  2 +-
>  drivers/vfio/pci/vfio_pci_core.c              | 41 ++++++++++++++++---
>  drivers/vfio/pci/vfio_pci_igd.c               |  2 +-
>  drivers/vfio/pci/vfio_pci_intrs.c             |  2 +-
>  drivers/vfio/pci/vfio_pci_rdwr.c              |  2 +-
>  drivers/vfio/pci/vfio_pci_zdev.c              |  2 +-
>  .../pci => include/linux}/vfio_pci_core.h     |  2 -
>  10 files changed, 66 insertions(+), 39 deletions(-)
>  rename {drivers/vfio/pci => include/linux}/vfio_pci_core.h (99%)
> 
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index afdab7d71e98..18898ae49919 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -1,19 +1,31 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -config VFIO_PCI
> +config VFIO_PCI_CORE
>  	tristate "VFIO support for PCI devices"
>  	depends on PCI
>  	depends on MMU
>  	select VFIO_VIRQFD
>  	select IRQ_BYPASS_MANAGER
>  	help
> -	  Support for the PCI VFIO bus driver.  This is required to make
> -	  use of PCI drivers using the VFIO framework.
> +	  Support for using PCI devices with VFIO.
> +
> +if VFIO_PCI_CORE
> +config VFIO_PCI_MMAP
> +	def_bool y if !S390
> +
> +config VFIO_PCI_INTX
> +	def_bool y if !S390
> +
> +config VFIO_PCI
> +	tristate "Generic VFIO support for any PCI device"
> +	help
> +	  Support for the generic PCI VFIO bus driver which can connect any
> +	  PCI device to the VFIO framework.
>  
>  	  If you don't know what to do here, say N.
>  

I'm still not happy with how this is likely to break users and even
downstreams when upgrading to a Kconfig with this change.  A previously
selected VFIO_PCI comes out disabled unless the user is keen enough to
enable VFIO_PCI_CORE.  I think I'd prefer to sacrifice the purity of
the menus to pull VFIO_PCI out of the if block and have it select
VFIO_PCI_CORE.  Thanks,

Alex
Jason Gunthorpe July 27, 2021, 11:09 p.m. UTC | #7
On Tue, Jul 27, 2021 at 03:54:40PM -0600, Alex Williamson wrote:

> I'm still not happy with how this is likely to break users and even
> downstreams when upgrading to a Kconfig with this change.

I've never heard of Kconfig as stable ABI. Christoph/Arnd, have you
heard of any cases where we want to keep it stable?

As far as I know we should change kconfig to keep it working properly,
eg by having correct menu structure and sane kconfig names.

In any event, upgrades work in a reasonable way. Starting from this
.config fragment:

CONFIG_VFIO_IOMMU_TYPE1=y
CONFIG_VFIO_VIRQFD=y
CONFIG_VFIO=y
CONFIG_VFIO_NOIOMMU=y
CONFIG_VFIO_PCI=y
CONFIG_VFIO_PCI_VGA=y
CONFIG_VFIO_PCI_MMAP=y
CONFIG_VFIO_PCI_INTX=y
CONFIG_VFIO_PCI_IGD=y
CONFIG_VFIO_PLATFORM=y
CONFIG_VFIO_AMBA=y
CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET=y
CONFIG_VFIO_PLATFORM_AMDXGBE_RESET=y
CONFIG_VFIO_PLATFORM_BCMFLEXRM_RESET=y
CONFIG_VFIO_MDEV=y
CONFIG_VFIO_FSL_MC=y
CONFIG_IRQ_BYPASS_MANAGER=y

Which might reasonably be from an old kernel. 'make oldconfig' prompts:

VFIO Non-Privileged userspace driver framework (VFIO) [Y/n/m/?] y
  VFIO No-IOMMU support (VFIO_NOIOMMU) [Y/n/?] y
  VFIO support for PCI devices (VFIO_PCI_CORE) [N/m/y/?] (NEW) 

Which is completely fine, IMHO.

The menu structure ends up looking like this, which is pretty good:

  --- VFIO Non-Privileged userspace driver framework
  [*]   VFIO No-IOMMU support
  <*>   VFIO support for PCI devices
  <*>     Generic VFIO support for any PCI device
  [*]       Generic VFIO PCI support for VGA devices
  [*]       Generic VFIO PCI extensions for Intel graphics (GVT-d)
  <*>     VFIO support for MLX5 PCI devices (NEW)
  <*>   VFIO support for platform devices
  <*>     VFIO support for AMBA devices
  <*>     VFIO support for calxeda xgmac reset
  <*>     VFIO support for AMD XGBE reset
  <*>     VFIO support for Broadcom FlexRM reset
  <*>   Mediated device driver framework
  <*>   VFIO support for QorIQ DPAA2 fsl-mc bus devices

Jason
Leon Romanovsky July 28, 2021, 4:56 a.m. UTC | #8
On Tue, Jul 27, 2021 at 08:09:41PM -0300, Jason Gunthorpe wrote:
> On Tue, Jul 27, 2021 at 03:54:40PM -0600, Alex Williamson wrote:
> 
> > I'm still not happy with how this is likely to break users and even
> > downstreams when upgrading to a Kconfig with this change.
> 
> I've never heard of Kconfig as stable ABI. Christoph/Arnd, have you
> heard of any cases where we want to keep it stable?

Of course not, otherwise we won't be able to do ANY cleanup in the kernel.

> 
> As far as I know we should change kconfig to keep it working properly,
> eg by having correct menu structure and sane kconfig names.

Everyone who upgrades through source code and needs rebuild kernel
should be proficient enough to enable/disable kernel config.

Thanks
Christoph Hellwig July 28, 2021, 5:43 a.m. UTC | #9
On Tue, Jul 27, 2021 at 08:09:41PM -0300, Jason Gunthorpe wrote:
> On Tue, Jul 27, 2021 at 03:54:40PM -0600, Alex Williamson wrote:
> 
> > I'm still not happy with how this is likely to break users and even
> > downstreams when upgrading to a Kconfig with this change.
> 
> I've never heard of Kconfig as stable ABI. Christoph/Arnd, have you
> heard of any cases where we want to keep it stable?

It isn't an ABI, but we really do try to avoid breaking if we can and
I rember Linus shouting at people if they did that for common options.

However lately for example the completely silly s/THUNDERBOLT/USB4/
change did slip through and did break my test setup with a vfio passed
through external nvme drive :(

> Which might reasonably be from an old kernel. 'make oldconfig' prompts:
> 
> VFIO Non-Privileged userspace driver framework (VFIO) [Y/n/m/?] y
>   VFIO No-IOMMU support (VFIO_NOIOMMU) [Y/n/?] y
>   VFIO support for PCI devices (VFIO_PCI_CORE) [N/m/y/?] (NEW) 
> 
> Which is completely fine, IMHO.

Why do we need to have VFIO_PCI_CORE as a user visible option?
I'd just select it.
Arnd Bergmann July 28, 2021, 7:04 a.m. UTC | #10
On Wed, Jul 28, 2021 at 7:43 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Jul 27, 2021 at 08:09:41PM -0300, Jason Gunthorpe wrote:
> > On Tue, Jul 27, 2021 at 03:54:40PM -0600, Alex Williamson wrote:
> >
> > > I'm still not happy with how this is likely to break users and even
> > > downstreams when upgrading to a Kconfig with this change.
> >
> > I've never heard of Kconfig as stable ABI. Christoph/Arnd, have you
> > heard of any cases where we want to keep it stable?
>
> It isn't an ABI, but we really do try to avoid breaking if we can and
> I rember Linus shouting at people if they did that for common options.

This is handled in very different ways depending on the maintainers,
some people go to great lengths to avoid breaking 'make oldconfig'
or 'make defconfig', others don't seem to mind at all.

CONFIG_USB_EHCI_TEGRA is an example of an option that was
left in place to help users of old config files, another one is
CONFIG_EXT3_FS. In both cases the idea is that the original
code was changed, but the old option left in place to point to
the replacement.

I think doing this is generally a good idea, but I would not consider
this a stable ABI in the sense that we can never break it. Most users
should have migrated to the new option after a few kernel releases,
and then I would remove the old one.

If a user upgrades across multiple kernel releases at once, usually
all hope of reusing an old .config is lost anyway.

> However lately for example the completely silly s/THUNDERBOLT/USB4/
> change did slip through and did break my test setup with a vfio passed
> through external nvme drive :(

Another recent example is CONFIG_FB no longer being selected by
the DRM subsystem, which broke a lot of defconfigs.

        Arnd
Leon Romanovsky July 28, 2021, 7:17 a.m. UTC | #11
On Wed, Jul 28, 2021 at 09:04:34AM +0200, Arnd Bergmann wrote:
> On Wed, Jul 28, 2021 at 7:43 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Jul 27, 2021 at 08:09:41PM -0300, Jason Gunthorpe wrote:
> > > On Tue, Jul 27, 2021 at 03:54:40PM -0600, Alex Williamson wrote:
> > >
> > > > I'm still not happy with how this is likely to break users and even
> > > > downstreams when upgrading to a Kconfig with this change.
> > >
> > > I've never heard of Kconfig as stable ABI. Christoph/Arnd, have you
> > > heard of any cases where we want to keep it stable?
> >
> > It isn't an ABI, but we really do try to avoid breaking if we can and
> > I rember Linus shouting at people if they did that for common options.
> 
> This is handled in very different ways depending on the maintainers,
> some people go to great lengths to avoid breaking 'make oldconfig'
> or 'make defconfig', others don't seem to mind at all.
> 
> CONFIG_USB_EHCI_TEGRA is an example of an option that was
> left in place to help users of old config files, another one is
> CONFIG_EXT3_FS. In both cases the idea is that the original
> code was changed, but the old option left in place to point to
> the replacement.

And here starts the problem, when people treat their obscure config
options as first class citizen. The exposure of CONFIG_EXT3_FS is
in magnitudes larger than CONFIG_USB_EHCI_TEGRA.

This is why I think that is generally bad idea to leave old config
options, most of the time such options will rotten for years till
someone actually will notice and delete them.

Thanks
Jason Gunthorpe July 28, 2021, 12:03 p.m. UTC | #12
On Wed, Jul 28, 2021 at 07:43:06AM +0200, Christoph Hellwig wrote:

> > Which might reasonably be from an old kernel. 'make oldconfig' prompts:
> > 
> > VFIO Non-Privileged userspace driver framework (VFIO) [Y/n/m/?] y
> >   VFIO No-IOMMU support (VFIO_NOIOMMU) [Y/n/?] y
> >   VFIO support for PCI devices (VFIO_PCI_CORE) [N/m/y/?] (NEW) 
> > 
> > Which is completely fine, IMHO.
> 
> Why do we need to have VFIO_PCI_CORE as a user visible option?
> I'd just select it.

I'm not great with kconfig, but AFAIK:

- It controls building a module so it needs to be a tristate

- tristates need to be exposed in the menu structure

- As it builds a module it also has depends on other things

- Select should not be used to target tristates

- Select should not be used to target options in the menu tree

- Select should not be used to target options that have depends

Which leaves us with this arrangement unless we delete the
vfio_pci_core.ko module - which seems like a bad direction just for
kconfig backwards compatibility.

Jason
Arnd Bergmann July 28, 2021, 12:12 p.m. UTC | #13
On Wed, Jul 28, 2021 at 2:03 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, Jul 28, 2021 at 07:43:06AM +0200, Christoph Hellwig wrote:
>
> > > Which might reasonably be from an old kernel. 'make oldconfig' prompts:
> > >
> > > VFIO Non-Privileged userspace driver framework (VFIO) [Y/n/m/?] y
> > >   VFIO No-IOMMU support (VFIO_NOIOMMU) [Y/n/?] y
> > >   VFIO support for PCI devices (VFIO_PCI_CORE) [N/m/y/?] (NEW)
> > >
> > > Which is completely fine, IMHO.
> >
> > Why do we need to have VFIO_PCI_CORE as a user visible option?
> > I'd just select it.
>
> I'm not great with kconfig, but AFAIK:
>
> - It controls building a module so it needs to be a tristate
>
> - tristates need to be exposed in the menu structure
>
> - As it builds a module it also has depends on other things
>
> - Select should not be used to target tristates
>
> - Select should not be used to target options in the menu tree
>
> - Select should not be used to target options that have depends
>
> Which leaves us with this arrangement unless we delete the
> vfio_pci_core.ko module - which seems like a bad direction just for
> kconfig backwards compatibility.

I have not looked at the requirements for this particular patch, but
generally speaking there is no problem with using 'select' on
a tristate symbol.

The other points are correct though: you can not 'select' a symbol
that has dependencies, unless the symbol selecting it already
depends on those same options, and you should not 'select' user
visible options or other subsystems.

One common mistake is to have a reverse dependency, where
A uses 'select B' or 'depends on B', but then exports an ELF
symbol that is consumed by B, as opposed to the other way round.
I don't think that is a problem here though.

            Arnd
Christoph Hellwig July 28, 2021, 12:29 p.m. UTC | #14
On Wed, Jul 28, 2021 at 09:03:26AM -0300, Jason Gunthorpe wrote:
> I'm not great with kconfig, but AFAIK:
> 
> - It controls building a module so it needs to be a tristate
> 
> - tristates need to be exposed in the menu structure

select can be used on tristates perfectly fine.

> - As it builds a module it also has depends on other things

So the dependencies are:

 - VFIO - duh, yeah, anything vfio related needs to select that.
   But this is a perfectly fine transitive select
 - PCI - yeah.  But we can expect everything that selects VFIO_PCI_CORE
   to select PCI.  Or a transitive select would be fine again
 - EVENTFD this is another classic transitive one that should just be selected
   instead of a user asking why it is not set
 - MMU: I suspect all of VFIO and thus the menuconfig really should
   depend on that

So not really an issue here.  VFIO_PCI_CORE really is underlying
infrastructure a user should not care about.
Jason Gunthorpe July 28, 2021, 12:47 p.m. UTC | #15
On Wed, Jul 28, 2021 at 02:29:56PM +0200, Christoph Hellwig wrote:

> So not really an issue here.  VFIO_PCI_CORE really is underlying
> infrastructure a user should not care about.

So then we can write it like below? Unfortunately it deletes the nice
menu structure that groups all the PCI drivers together like platform
(and mdev in future). Not sure this loss is worth the backwards compat

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index 318116d03c21a4..2611d7d91ddbd5 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -1,14 +1,12 @@
 # SPDX-License-Identifier: GPL-2.0-only
+if PCI || MMU
 config VFIO_PCI_CORE
-	tristate "VFIO support for PCI devices"
-	depends on PCI
-	depends on MMU
+	tristate
 	select VFIO_VIRQFD
 	select IRQ_BYPASS_MANAGER
 	help
 	  Support for using PCI devices with VFIO.
 
-if VFIO_PCI_CORE
 config VFIO_PCI_MMAP
 	def_bool y if !S390
 
@@ -17,6 +15,7 @@ config VFIO_PCI_INTX
 
 config VFIO_PCI
 	tristate "Generic VFIO support for any PCI device"
+	select VFIO_PCI_CORE
 	help
 	  Support for the generic PCI VFIO bus driver which can connect any
 	  PCI device to the VFIO framework.
@@ -50,6 +49,7 @@ endif
 config MLX5_VFIO_PCI
 	tristate "VFIO support for MLX5 PCI devices"
 	depends on MLX5_CORE
+	select VFIO_PCI_CORE
 	help
 	  This provides a PCI support for MLX5 devices using the VFIO
 	  framework. The device specific driver supports suspend/resume
Christoph Hellwig July 28, 2021, 12:55 p.m. UTC | #16
On Wed, Jul 28, 2021 at 09:47:55AM -0300, Jason Gunthorpe wrote:
> So then we can write it like below?

> +if PCI || MMU

The || here should be &&.  But otherwise, yes.

> Unfortunately it deletes the nice
> menu structure that groups all the PCI drivers together like platform
> (and mdev in future). Not sure this loss is worth the backwards compat

All the ther visible options should depend on VFIO_PCI not VFIO_PCI_CORE.
So we can still keep the same menu struture.
Arnd Bergmann July 28, 2021, 1:08 p.m. UTC | #17
On Wed, Jul 28, 2021 at 2:47 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, Jul 28, 2021 at 02:29:56PM +0200, Christoph Hellwig wrote:
>
> > So not really an issue here.  VFIO_PCI_CORE really is underlying
> > infrastructure a user should not care about.
>
> So then we can write it like below? Unfortunately it deletes the nice
> menu structure that groups all the PCI drivers together like platform
> (and mdev in future). Not sure this loss is worth the backwards compat

I think you can get back some structure by adding a 'menu "VFIO PCI drivers"'
and 'endmenu' around it.

> @@ -17,6 +15,7 @@ config VFIO_PCI_INTX
>
>  config VFIO_PCI
>         tristate "Generic VFIO support for any PCI device"
> +       select VFIO_PCI_CORE
>         help
>           Support for the generic PCI VFIO bus driver which can connect any
>           PCI device to the VFIO framework.
> @@ -50,6 +49,7 @@ endif
>  config MLX5_VFIO_PCI
>         tristate "VFIO support for MLX5 PCI devices"
>         depends on MLX5_CORE
> +       select VFIO_PCI_CORE
>         help

These two now have to get a 'depends on MMU' if they don't already inherit
that from elsewhere.

       Arnd
Jason Gunthorpe July 28, 2021, 1:31 p.m. UTC | #18
On Wed, Jul 28, 2021 at 02:55:05PM +0200, Christoph Hellwig wrote:
> On Wed, Jul 28, 2021 at 09:47:55AM -0300, Jason Gunthorpe wrote:
> > So then we can write it like below?
>
> > +if PCI || MMU
>
> The || here should be &&.  But otherwise, yes.

Woops, thanks

> > Unfortunately it deletes the nice
> > menu structure that groups all the PCI drivers together like platform
> > (and mdev in future). Not sure this loss is worth the backwards compat
>
> All the ther visible options should depend on VFIO_PCI not VFIO_PCI_CORE.
> So we can still keep the same menu struture.

It is the upcoming VFIO PCI drivers I'm looking at, eg the
MLX5_VFIO_PCI

It goes from this:

  --- VFIO Non-Privileged userspace driver framework
  [*]   VFIO No-IOMMU support
  <*>   VFIO support for PCI devices
  <*>     Generic VFIO support for any PCI device
  [*]       Generic VFIO PCI support for VGA devices
  [*]       Generic VFIO PCI extensions for Intel graphics (GVT-d)
  <*>     VFIO support for MLX5 PCI devices (NEW)
  <*>   VFIO support for platform devices
  <*>     VFIO support for AMBA devices
  <*>     VFIO support for calxeda xgmac reset
  <*>     VFIO support for AMD XGBE reset
  <*>     VFIO support for Broadcom FlexRM reset
  <*>   Mediated device driver framework
  <*>   VFIO support for QorIQ DPAA2 fsl-mc bus devices

To this:

  --- VFIO Non-Privileged userspace driver framework
  [*]   VFIO No-IOMMU support
  <*>   Generic VFIO support for any PCI device
  [*]     Generic VFIO PCI support for VGA devices
  [*]     Generic VFIO PCI extensions for Intel graphics (GVT-d)
  < >   VFIO support for MLX5 PCI devices (NEW)
  <*>   VFIO support for platform devices
  <*>     VFIO support for AMBA devices
  <*>     VFIO support for calxeda xgmac reset
  <*>     VFIO support for AMD XGBE reset
  <*>     VFIO support for Broadcom FlexRM reset
  <*>   Mediated device driver framework
  <*>   VFIO support for QorIQ DPAA2 fsl-mc bus devices

Look at how "VFIO support for MLX5 PCI devices" has changed its
position.

Arnd's suggstion to add a menu seems OK, it gives another screen in
kconfig but it does group them.

My preference is the first version because it is simplest. Adding menu
seems like something to do if we get > 5 more drivers. Just hiding it
preserves back compat but hurts the UI.

Honestly, I don't care very much, if Alex values kconfig backcompat
higher than the UI then lets use the diff in my last email.

Jason
Jason Gunthorpe July 28, 2021, 5:26 p.m. UTC | #19
On Wed, Jul 28, 2021 at 03:08:08PM +0200, Arnd Bergmann wrote:

> > @@ -17,6 +15,7 @@ config VFIO_PCI_INTX
> >
> >  config VFIO_PCI
> >         tristate "Generic VFIO support for any PCI device"
> > +       select VFIO_PCI_CORE
> >         help
> >           Support for the generic PCI VFIO bus driver which can connect any
> >           PCI device to the VFIO framework.
> > @@ -50,6 +49,7 @@ endif
> >  config MLX5_VFIO_PCI
> >         tristate "VFIO support for MLX5 PCI devices"
> >         depends on MLX5_CORE
> > +       select VFIO_PCI_CORE
> >         help
>
> These two now have to get a 'depends on MMU' if they don't already inherit
> that from elsewhere.

Just so I understand this remark properly, I added this at the top of
the file:

if PCI && MMU

And when I check CONFIG_MLX5_VFIO_PCI I see:

 Defined at drivers/vfio/pci/Kconfig:51
   Prompt: VFIO support for MLX5 PCI devices
   Depends on: VFIO [=y] && PCI [=y] && MMU [=y] && MLX5_CORE [=y]

So this is doing what you mean, right?

I've attached the whole thing below just for clarity

Thanks,
Jason

# SPDX-License-Identifier: GPL-2.0-only
if PCI && MMU
config VFIO_PCI_CORE
	tristate
	select VFIO_VIRQFD
	select IRQ_BYPASS_MANAGER
	help
	  Support for using PCI devices with VFIO.

config VFIO_PCI_MMAP
	def_bool y if !S390

config VFIO_PCI_INTX
	def_bool y if !S390

menu "VFIO PCI Drivers"

config VFIO_PCI
	tristate "Generic VFIO support for any PCI device"
	select VFIO_PCI_CORE
	help
	  Support for the generic PCI VFIO bus driver which can connect any
	  PCI device to the VFIO framework.

	  If you don't know what to do here, say N.

if VFIO_PCI
config VFIO_PCI_VGA
	bool "Generic VFIO PCI support for VGA devices"
	depends on X86 && VGA_ARB
	help
	  Support for VGA extension to VFIO PCI.  This exposes an additional
	  region on VGA devices for accessing legacy VGA addresses used by
	  BIOS and generic video drivers.

	  If you don't know what to do here, say N.

config VFIO_PCI_IGD
	bool "Generic VFIO PCI extensions for Intel graphics (GVT-d)"
	depends on X86
	default y
	help
	  Support for Intel IGD specific extensions to enable direct
	  assignment to virtual machines.  This includes exposing an IGD
	  specific firmware table and read-only copies of the host bridge
	  and LPC bridge config space.

	  To enable Intel IGD assignment through vfio-pci, say Y.
endif

config MLX5_VFIO_PCI
	tristate "VFIO support for MLX5 PCI devices"
	depends on MLX5_CORE
	select VFIO_PCI_CORE
	help
	  This provides a PCI support for MLX5 devices using the VFIO
	  framework. The device specific driver supports suspend/resume
	  of the MLX5 device.

	  If you don't know what to do here, say N.
endmenu
endif
diff mbox series

Patch

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index afdab7d71e98..18898ae49919 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -1,19 +1,31 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
-config VFIO_PCI
+config VFIO_PCI_CORE
 	tristate "VFIO support for PCI devices"
 	depends on PCI
 	depends on MMU
 	select VFIO_VIRQFD
 	select IRQ_BYPASS_MANAGER
 	help
-	  Support for the PCI VFIO bus driver.  This is required to make
-	  use of PCI drivers using the VFIO framework.
+	  Support for using PCI devices with VFIO.
+
+if VFIO_PCI_CORE
+config VFIO_PCI_MMAP
+	def_bool y if !S390
+
+config VFIO_PCI_INTX
+	def_bool y if !S390
+
+config VFIO_PCI
+	tristate "Generic VFIO support for any PCI device"
+	help
+	  Support for the generic PCI VFIO bus driver which can connect any
+	  PCI device to the VFIO framework.
 
 	  If you don't know what to do here, say N.
 
 if VFIO_PCI
 config VFIO_PCI_VGA
-	bool "VFIO PCI support for VGA devices"
+	bool "Generic VFIO PCI support for VGA devices"
 	depends on X86 && VGA_ARB
 	help
 	  Support for VGA extension to VFIO PCI.  This exposes an additional
@@ -22,14 +34,8 @@  config VFIO_PCI_VGA
 
 	  If you don't know what to do here, say N.
 
-config VFIO_PCI_MMAP
-	def_bool y if !S390
-
-config VFIO_PCI_INTX
-	def_bool y if !S390
-
 config VFIO_PCI_IGD
-	bool "VFIO PCI extensions for Intel graphics (GVT-d)"
+	bool "Generic VFIO PCI extensions for Intel graphics (GVT-d)"
 	depends on X86
 	default y
 	help
@@ -39,5 +45,5 @@  config VFIO_PCI_IGD
 	  and LPC bridge config space.
 
 	  To enable Intel IGD assignment through vfio-pci, say Y.
-
+endif
 endif
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index 8aa517b4b671..349d68d242b4 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -1,7 +1,9 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 
-vfio-pci-y := vfio_pci.o vfio_pci_core.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_S390) += vfio_pci_zdev.o
+vfio-pci-core-y := vfio_pci_core.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
+vfio-pci-core-$(CONFIG_S390) += vfio_pci_zdev.o
+obj-$(CONFIG_VFIO_PCI_CORE) += vfio-pci-core.o
 
+vfio-pci-y := vfio_pci.o
+vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
 obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 7a43edbe8618..41b4742aef20 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -25,7 +25,7 @@ 
 #include <linux/types.h>
 #include <linux/uaccess.h>
 
-#include "vfio_pci_core.h"
+#include <linux/vfio_pci_core.h>
 
 #define DRIVER_VERSION  "0.2"
 #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
@@ -154,6 +154,7 @@  static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ret = vfio_pci_core_register_device(vdev);
 	if (ret)
 		goto out_free;
+	dev_set_drvdata(&pdev->dev, vdev);
 	return 0;
 
 out_free:
@@ -249,14 +250,10 @@  static int __init vfio_pci_init(void)
 
 	vfio_pci_core_set_params(nointxmask, is_disable_vga, disable_idle_d3);
 
-	ret = vfio_pci_core_init();
-	if (ret)
-		return ret;
-
 	/* Register and scan for devices */
 	ret = pci_register_driver(&vfio_pci_driver);
 	if (ret)
-		goto out;
+		return ret;
 
 	vfio_pci_fill_ids();
 
@@ -264,17 +261,12 @@  static int __init vfio_pci_init(void)
 		pr_warn("device denylist disabled.\n");
 
 	return 0;
-
-out:
-	vfio_pci_core_cleanup();
-	return ret;
 }
 module_init(vfio_pci_init);
 
 static void __exit vfio_pci_cleanup(void)
 {
 	pci_unregister_driver(&vfio_pci_driver);
-	vfio_pci_core_cleanup();
 }
 module_exit(vfio_pci_cleanup);
 
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 1f034f768a27..6e58b4bf7a60 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -26,7 +26,7 @@ 
 #include <linux/vfio.h>
 #include <linux/slab.h>
 
-#include "vfio_pci_core.h"
+#include <linux/vfio_pci_core.h>
 
 /* Fake capability ID for standard config space */
 #define PCI_CAP_ID_BASIC	0
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index e65b154f17c3..3a5b6d889a69 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -8,6 +8,8 @@ 
  * Author: Tom Lyon, pugs@cisco.com
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/device.h>
 #include <linux/eventfd.h>
 #include <linux/file.h>
@@ -25,7 +27,11 @@ 
 #include <linux/nospec.h>
 #include <linux/sched/mm.h>
 
-#include "vfio_pci_core.h"
+#include <linux/vfio_pci_core.h>
+
+#define DRIVER_VERSION  "0.2"
+#define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
+#define DRIVER_DESC "core driver for VFIO based PCI devices"
 
 static bool nointxmask;
 static bool disable_vga;
@@ -306,6 +312,7 @@  int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_enable);
 
 void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 {
@@ -405,6 +412,7 @@  void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 	if (!disable_idle_d3)
 		vfio_pci_set_power_state(vdev, PCI_D3hot);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_disable);
 
 static struct vfio_pci_core_device *get_pf_vdev(struct vfio_pci_core_device *vdev)
 {
@@ -461,6 +469,7 @@  void vfio_pci_core_close_device(struct vfio_device *core_vdev)
 	}
 	mutex_unlock(&vdev->igate);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_close_device);
 
 void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
 {
@@ -468,6 +477,7 @@  void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
 	vfio_spapr_pci_eeh_open(vdev->pdev);
 	vfio_pci_vf_token_user_add(vdev, 1);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_finish_enable);
 
 static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_type)
 {
@@ -626,6 +636,7 @@  int vfio_pci_register_dev_region(struct vfio_pci_core_device *vdev,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_register_dev_region);
 
 long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 		unsigned long arg)
@@ -1170,6 +1181,7 @@  long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 
 	return -ENOTTY;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl);
 
 static ssize_t vfio_pci_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 			   size_t count, loff_t *ppos, bool iswrite)
@@ -1213,6 +1225,7 @@  ssize_t vfio_pci_core_read(struct vfio_device *core_vdev, char __user *buf,
 
 	return vfio_pci_rw(vdev, buf, count, ppos, false);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_read);
 
 ssize_t vfio_pci_core_write(struct vfio_device *core_vdev, const char __user *buf,
 		size_t count, loff_t *ppos)
@@ -1225,6 +1238,7 @@  ssize_t vfio_pci_core_write(struct vfio_device *core_vdev, const char __user *bu
 
 	return vfio_pci_rw(vdev, (char __user *)buf, count, ppos, true);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_write);
 
 /* Return 1 on zap and vma_lock acquired, 0 on contention (only with @try) */
 static int vfio_pci_zap_and_vma_lock(struct vfio_pci_core_device *vdev, bool try)
@@ -1503,6 +1517,7 @@  int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_mmap);
 
 void vfio_pci_core_request(struct vfio_device *core_vdev, unsigned int count)
 {
@@ -1525,6 +1540,7 @@  void vfio_pci_core_request(struct vfio_device *core_vdev, unsigned int count)
 
 	mutex_unlock(&vdev->igate);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_request);
 
 static int vfio_pci_validate_vf_token(struct vfio_pci_core_device *vdev,
 				      bool vf_token, uuid_t *uuid)
@@ -1669,6 +1685,7 @@  int vfio_pci_core_match(struct vfio_device *core_vdev, char *buf)
 
 	return 1; /* Match */
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_match);
 
 static int vfio_pci_bus_notifier(struct notifier_block *nb,
 				 unsigned long action, void *data)
@@ -1776,6 +1793,7 @@  void vfio_pci_core_init_device(struct vfio_pci_core_device *vdev,
 	INIT_LIST_HEAD(&vdev->vma_list);
 	init_rwsem(&vdev->memory_lock);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_init_device);
 
 void vfio_pci_core_uninit_device(struct vfio_pci_core_device *vdev)
 {
@@ -1786,6 +1804,7 @@  void vfio_pci_core_uninit_device(struct vfio_pci_core_device *vdev)
 	kfree(vdev->region);
 	kfree(vdev->pm_save);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_uninit_device);
 
 int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
 {
@@ -1847,7 +1866,6 @@  int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
 	ret = vfio_register_group_dev(&vdev->vdev);
 	if (ret)
 		goto out_power;
-	dev_set_drvdata(&pdev->dev, vdev);
 	return 0;
 
 out_power:
@@ -1859,6 +1877,7 @@  int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
 	vfio_iommu_group_put(group, &pdev->dev);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_register_device);
 
 void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev)
 {
@@ -1876,6 +1895,7 @@  void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev)
 	if (!disable_idle_d3)
 		vfio_pci_set_power_state(vdev, PCI_D0);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_unregister_device);
 
 static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 						  pci_channel_state_t state)
@@ -1921,10 +1941,12 @@  int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 
 	return ret < 0 ? ret : nr_virtfn;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_sriov_configure);
 
 const struct pci_error_handlers vfio_pci_core_err_handlers = {
 	.error_detected = vfio_pci_aer_err_detected,
 };
+EXPORT_SYMBOL_GPL(vfio_pci_core_err_handlers);
 
 static int vfio_pci_check_all_devices_bound(struct pci_dev *pdev, void *data)
 {
@@ -2094,16 +2116,23 @@  void vfio_pci_core_set_params(bool is_nointxmask, bool is_disable_vga,
 	disable_vga = is_disable_vga;
 	disable_idle_d3 = is_disable_idle_d3;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_core_set_params);
 
-/* This will become the __exit function of vfio_pci_core.ko */
-void vfio_pci_core_cleanup(void)
+static void vfio_pci_core_cleanup(void)
 {
 	vfio_pci_uninit_perm_bits();
 }
 
-/* This will become the __init function of vfio_pci_core.ko */
-int __init vfio_pci_core_init(void)
+static int __init vfio_pci_core_init(void)
 {
 	/* Allocate shared config space permission data used by all devices */
 	return vfio_pci_init_perm_bits();
 }
+
+module_init(vfio_pci_core_init);
+module_exit(vfio_pci_core_cleanup);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
index a324ca7e6b5a..7ca4109bba48 100644
--- a/drivers/vfio/pci/vfio_pci_igd.c
+++ b/drivers/vfio/pci/vfio_pci_igd.c
@@ -15,7 +15,7 @@ 
 #include <linux/uaccess.h>
 #include <linux/vfio.h>
 
-#include "vfio_pci_core.h"
+#include <linux/vfio_pci_core.h>
 
 #define OPREGION_SIGNATURE	"IntelGraphicsMem"
 #define OPREGION_SIZE		(8 * 1024)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 945ddbdf4d11..6069a11fb51a 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -20,7 +20,7 @@ 
 #include <linux/wait.h>
 #include <linux/slab.h>
 
-#include "vfio_pci_core.h"
+#include <linux/vfio_pci_core.h>
 
 /*
  * INTx
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 8fff4689dd44..57d3b2cbbd8e 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -17,7 +17,7 @@ 
 #include <linux/vfio.h>
 #include <linux/vgaarb.h>
 
-#include "vfio_pci_core.h"
+#include <linux/vfio_pci_core.h>
 
 #ifdef __LITTLE_ENDIAN
 #define vfio_ioread64	ioread64
diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
index 2ffbdc11f089..fe4def9ffffb 100644
--- a/drivers/vfio/pci/vfio_pci_zdev.c
+++ b/drivers/vfio/pci/vfio_pci_zdev.c
@@ -19,7 +19,7 @@ 
 #include <asm/pci_clp.h>
 #include <asm/pci_io.h>
 
-#include "vfio_pci_core.h"
+#include <linux/vfio_pci_core.h>
 
 /*
  * Add the Base PCI Function information to the device info region.
diff --git a/drivers/vfio/pci/vfio_pci_core.h b/include/linux/vfio_pci_core.h
similarity index 99%
rename from drivers/vfio/pci/vfio_pci_core.h
rename to include/linux/vfio_pci_core.h
index 7a2da1e14de3..ef9a44b6cf5d 100644
--- a/drivers/vfio/pci/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -207,8 +207,6 @@  static inline int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
 #endif
 
 /* Will be exported for vfio pci drivers usage */
-void vfio_pci_core_cleanup(void);
-int vfio_pci_core_init(void);
 void vfio_pci_core_set_params(bool nointxmask, bool is_disable_vga,
 			      bool is_disable_idle_d3);
 void vfio_pci_core_close_device(struct vfio_device *core_vdev);