[RFC,v2,1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon ACC devices

Commit Message

Shameerali Kolothum Thodi July 2, 2021, 9:58 a.m. UTC

Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
This will be extended in follow-up patches to add support for
vfio live migration feature.

Signed-off-by: Shameer Kolothum <[e-mail]>
---
 drivers/vfio/pci/Kconfig             |   9 +++
 drivers/vfio/pci/Makefile            |   2 +
 drivers/vfio/pci/hisi_acc_vfio_pci.c | 100 +++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c

Comments

Alex Williamson July 2, 2021, 8:29 p.m. UTC | #1

On Fri, 2 Jul 2021 10:58:46 +0100
Shameer Kolothum <[e-mail]> wrote:

> Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
> This will be extended in follow-up patches to add support for
> vfio live migration feature.
> 
> Signed-off-by: Shameer Kolothum <[e-mail]>
> ---
>  drivers/vfio/pci/Kconfig             |   9 +++
>  drivers/vfio/pci/Makefile            |   2 +
>  drivers/vfio/pci/hisi_acc_vfio_pci.c | 100 +++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>  create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c
> 
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index 9cdef46dd299..709807c28153 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -57,3 +57,12 @@ config MLX5_VFIO_PCI
>  	  framework.
>  
>  	  If you don't know what to do here, say N.
> +
> +config HISI_ACC_VFIO_PCI
> +	tristate "VFIO support for HiSilicon ACC devices"
> +	depends on ARM64 && VFIO_PCI_CORE
> +	help
> +	  This provides generic PCI support for HiSilicon devices using the VFIO
> +	  framework.
> +
> +	  If you don't know what to do here, say N.
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index a0df9c2a4bd9..d1de3e81921f 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -3,6 +3,7 @@
>  obj-$(CONFIG_VFIO_PCI_CORE) += vfio-pci-core.o
>  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
>  obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5-vfio-pci.o
> +obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisi-acc-vfio-pci.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
> @@ -11,3 +12,4 @@ vfio-pci-y := vfio_pci.o
>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>  
>  mlx5-vfio-pci-y := mlx5_vfio_pci.o
> +hisi-acc-vfio-pci-y := hisi_acc_vfio_pci.o
> diff --git a/drivers/vfio/pci/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisi_acc_vfio_pci.c
> new file mode 100644
> index 000000000000..a9e173098ab5
> --- /dev/null
> +++ b/drivers/vfio/pci/hisi_acc_vfio_pci.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, HiSilicon Ltd.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/eventfd.h>
> +#include <linux/file.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/vfio.h>
> +#include <linux/vfio_pci_core.h>
> +
> +static int hisi_acc_vfio_pci_open(struct vfio_device *core_vdev)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> +	int ret;
> +
> +	lockdep_assert_held(&core_vdev->reflck->lock);
> +
> +	ret = vfio_pci_core_enable(vdev);
> +	if (ret)
> +		return ret;
> +
> +	vfio_pci_core_finish_enable(vdev);
> +
> +	return 0;
> +}
> +
> +static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> +	.name		= "hisi-acc-vfio-pci",
> +	.open		= hisi_acc_vfio_pci_open,
> +	.release	= vfio_pci_core_release,
> +	.ioctl		= vfio_pci_core_ioctl,
> +	.read		= vfio_pci_core_read,
> +	.write		= vfio_pci_core_write,
> +	.mmap		= vfio_pci_core_mmap,
> +	.request	= vfio_pci_core_request,
> +	.match		= vfio_pci_core_match,
> +	.reflck_attach	= vfio_pci_core_reflck_attach,
> +};
> +
> +static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct vfio_pci_core_device *vdev;
> +	int ret;
> +
> +	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> +	if (!vdev)
> +		return -ENOMEM;
> +
> +	ret = vfio_pci_core_register_device(vdev, pdev, &hisi_acc_vfio_pci_ops);
> +	if (ret)
> +		goto out_free;
> +
> +	dev_set_drvdata(&pdev->dev, vdev);
> +
> +	return 0;
> +
> +out_free:
> +	kfree(vdev);
> +	return ret;
> +}
> +
> +static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev)
> +{
> +	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
> +
> +	vfio_pci_core_unregister_device(vdev);
> +	kfree(vdev);
> +}
> +
> +static const struct pci_device_id hisi_acc_vfio_pci_table[] = {
> +	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI, 0xa256) }, /* SEC VF */
> +	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI, 0xa259) }, /* HPRE VF */
> +	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI, 0xa251) }, /* ZIP VF */
> +	{ 0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, hisi_acc_vfio_pci_table);
> +
> +static struct pci_driver hisi_acc_vfio_pci_driver = {
> +	.name			= "hisi-acc-vfio-pci",
> +	.id_table		= hisi_acc_vfio_pci_table,
> +	.probe			= hisi_acc_vfio_pci_probe,
> +	.remove			= hisi_acc_vfio_pci_remove,
> +#ifdef CONFIG_PCI_IOV
> +	.sriov_configure	= vfio_pci_core_sriov_configure,
> +#endif

The device table suggests only VFs are supported by this driver, so it
really shouldn't need sriov_configure support, right?  Thanks,

Alex

> +	.err_handler		= &vfio_pci_core_err_handlers,
> +};
> +
> +module_pci_driver(hisi_acc_vfio_pci_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Liu Longfang <[e-mail]>");
> +MODULE_AUTHOR("Shameer Kolothum <[e-mail]>");
> +MODULE_DESCRIPTION("HiSilicon VFIO PCI - Generic VFIO PCI driver for HiSilicon ACC device family");

Leon Romanovsky July 4, 2021, 7:03 a.m. UTC | #2

On Fri, Jul 02, 2021 at 10:58:46AM +0100, Shameer Kolothum wrote:
> Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
> This will be extended in follow-up patches to add support for
> vfio live migration feature.
> 
> Signed-off-by: Shameer Kolothum <[e-mail]>
> ---
>  drivers/vfio/pci/Kconfig             |   9 +++
>  drivers/vfio/pci/Makefile            |   2 +
>  drivers/vfio/pci/hisi_acc_vfio_pci.c | 100 +++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>  create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c

<...>

> +static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> +	.name		= "hisi-acc-vfio-pci",
> +	.open		= hisi_acc_vfio_pci_open,
> +	.release	= vfio_pci_core_release,
> +	.ioctl		= vfio_pci_core_ioctl,
> +	.read		= vfio_pci_core_read,
> +	.write		= vfio_pci_core_write,
> +	.mmap		= vfio_pci_core_mmap,
> +	.request	= vfio_pci_core_request,
> +	.match		= vfio_pci_core_match,
> +	.reflck_attach	= vfio_pci_core_reflck_attach,

I don't remember what was proposed in vfio-pci-core conversion patches,
but would expect that default behaviour is to fallback to vfio_pci_core_* API
if ".release/.ioctl/e.t.c" are not redefined.

Thanks

Shameerali Kolothum Thodi July 5, 2021, 7:20 a.m. UTC | #3

> -----Original Message-----
> From: Alex Williamson [mailto:[e-mail]]
> Sent: 02 July 2021 21:29
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Linuxarm <[e-mail]>; liulongfang <[e-mail]>;
> Zengtao (B) <[e-mail]>; yuzenghui
> <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Wangzhou (B) <[e-mail]>
> Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon
> ACC devices
> 
> On Fri, 2 Jul 2021 10:58:46 +0100
> Shameer Kolothum <[e-mail]> wrote:
> 
> > Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
> > This will be extended in follow-up patches to add support for
> > vfio live migration feature.
> >
> > Signed-off-by: Shameer Kolothum
> <[e-mail]>
> > ---
> >  drivers/vfio/pci/Kconfig             |   9 +++
> >  drivers/vfio/pci/Makefile            |   2 +
> >  drivers/vfio/pci/hisi_acc_vfio_pci.c | 100 +++++++++++++++++++++++++++
> >  3 files changed, 111 insertions(+)
> >  create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c
> >
> > diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> > index 9cdef46dd299..709807c28153 100644
> > --- a/drivers/vfio/pci/Kconfig
> > +++ b/drivers/vfio/pci/Kconfig
> > @@ -57,3 +57,12 @@ config MLX5_VFIO_PCI
> >  	  framework.
> >
> >  	  If you don't know what to do here, say N.
> > +
> > +config HISI_ACC_VFIO_PCI
> > +	tristate "VFIO support for HiSilicon ACC devices"
> > +	depends on ARM64 && VFIO_PCI_CORE
> > +	help
> > +	  This provides generic PCI support for HiSilicon devices using the VFIO
> > +	  framework.
> > +
> > +	  If you don't know what to do here, say N.
> > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > index a0df9c2a4bd9..d1de3e81921f 100644
> > --- a/drivers/vfio/pci/Makefile
> > +++ b/drivers/vfio/pci/Makefile
> > @@ -3,6 +3,7 @@
> >  obj-$(CONFIG_VFIO_PCI_CORE) += vfio-pci-core.o
> >  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> >  obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5-vfio-pci.o
> > +obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisi-acc-vfio-pci.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
> > @@ -11,3 +12,4 @@ vfio-pci-y := vfio_pci.o
> >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >
> >  mlx5-vfio-pci-y := mlx5_vfio_pci.o
> > +hisi-acc-vfio-pci-y := hisi_acc_vfio_pci.o
> > diff --git a/drivers/vfio/pci/hisi_acc_vfio_pci.c
> b/drivers/vfio/pci/hisi_acc_vfio_pci.c
> > new file mode 100644
> > index 000000000000..a9e173098ab5
> > --- /dev/null
> > +++ b/drivers/vfio/pci/hisi_acc_vfio_pci.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2021, HiSilicon Ltd.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/eventfd.h>
> > +#include <linux/file.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/vfio.h>
> > +#include <linux/vfio_pci_core.h>
> > +
> > +static int hisi_acc_vfio_pci_open(struct vfio_device *core_vdev)
> > +{
> > +	struct vfio_pci_core_device *vdev =
> > +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> > +	int ret;
> > +
> > +	lockdep_assert_held(&core_vdev->reflck->lock);
> > +
> > +	ret = vfio_pci_core_enable(vdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	vfio_pci_core_finish_enable(vdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> > +	.name		= "hisi-acc-vfio-pci",
> > +	.open		= hisi_acc_vfio_pci_open,
> > +	.release	= vfio_pci_core_release,
> > +	.ioctl		= vfio_pci_core_ioctl,
> > +	.read		= vfio_pci_core_read,
> > +	.write		= vfio_pci_core_write,
> > +	.mmap		= vfio_pci_core_mmap,
> > +	.request	= vfio_pci_core_request,
> > +	.match		= vfio_pci_core_match,
> > +	.reflck_attach	= vfio_pci_core_reflck_attach,
> > +};
> > +
> > +static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> > +{
> > +	struct vfio_pci_core_device *vdev;
> > +	int ret;
> > +
> > +	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> > +	if (!vdev)
> > +		return -ENOMEM;
> > +
> > +	ret = vfio_pci_core_register_device(vdev, pdev, &hisi_acc_vfio_pci_ops);
> > +	if (ret)
> > +		goto out_free;
> > +
> > +	dev_set_drvdata(&pdev->dev, vdev);
> > +
> > +	return 0;
> > +
> > +out_free:
> > +	kfree(vdev);
> > +	return ret;
> > +}
> > +
> > +static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev)
> > +{
> > +	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
> > +
> > +	vfio_pci_core_unregister_device(vdev);
> > +	kfree(vdev);
> > +}
> > +
> > +static const struct pci_device_id hisi_acc_vfio_pci_table[] = {
> > +	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI,
> 0xa256) }, /* SEC VF */
> > +	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI,
> 0xa259) }, /* HPRE VF */
> > +	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI,
> 0xa251) }, /* ZIP VF */
> > +	{ 0, }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, hisi_acc_vfio_pci_table);
> > +
> > +static struct pci_driver hisi_acc_vfio_pci_driver = {
> > +	.name			= "hisi-acc-vfio-pci",
> > +	.id_table		= hisi_acc_vfio_pci_table,
> > +	.probe			= hisi_acc_vfio_pci_probe,
> > +	.remove			= hisi_acc_vfio_pci_remove,
> > +#ifdef CONFIG_PCI_IOV
> > +	.sriov_configure	= vfio_pci_core_sriov_configure,
> > +#endif
> 
> The device table suggests only VFs are supported by this driver, so it
> really shouldn't need sriov_configure support, right?  Thanks,

Right. Only VFs are supported. I will remove this.

Thanks,
Shameer

> 
> Alex
> 
> > +	.err_handler		= &vfio_pci_core_err_handlers,
> > +};
> > +
> > +module_pci_driver(hisi_acc_vfio_pci_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Liu Longfang <[e-mail]>");
> > +MODULE_AUTHOR("Shameer Kolothum
> <[e-mail]>");
> > +MODULE_DESCRIPTION("HiSilicon VFIO PCI - Generic VFIO PCI driver for
> HiSilicon ACC device family");

Shameerali Kolothum Thodi July 5, 2021, 8:47 a.m. UTC | #4

> -----Original Message-----
> From: Leon Romanovsky [mailto:[e-mail]]
> Sent: 04 July 2021 08:04
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Linuxarm <[e-mail]>; liulongfang
> <[e-mail]>; Zengtao (B) <[e-mail]>;
> yuzenghui <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Wangzhou (B) <[e-mail]>
> Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon
> ACC devices
> 
> On Fri, Jul 02, 2021 at 10:58:46AM +0100, Shameer Kolothum wrote:
> > Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
> > This will be extended in follow-up patches to add support for
> > vfio live migration feature.
> >
> > Signed-off-by: Shameer Kolothum
> <[e-mail]>
> > ---
> >  drivers/vfio/pci/Kconfig             |   9 +++
> >  drivers/vfio/pci/Makefile            |   2 +
> >  drivers/vfio/pci/hisi_acc_vfio_pci.c | 100 +++++++++++++++++++++++++++
> >  3 files changed, 111 insertions(+)
> >  create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c
> 
> <...>
> 
> > +static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> > +	.name		= "hisi-acc-vfio-pci",
> > +	.open		= hisi_acc_vfio_pci_open,
> > +	.release	= vfio_pci_core_release,
> > +	.ioctl		= vfio_pci_core_ioctl,
> > +	.read		= vfio_pci_core_read,
> > +	.write		= vfio_pci_core_write,
> > +	.mmap		= vfio_pci_core_mmap,
> > +	.request	= vfio_pci_core_request,
> > +	.match		= vfio_pci_core_match,
> > +	.reflck_attach	= vfio_pci_core_reflck_attach,
> 
> I don't remember what was proposed in vfio-pci-core conversion patches,
> but would expect that default behaviour is to fallback to vfio_pci_core_* API
> if ".release/.ioctl/e.t.c" are not redefined.

Yes, that would be nice, but don't think it does that in latest(v4).

Hi Max,
Could we please consider fall back to the core defaults, may be check and assign defaults
in vfio_pci_core_register_device() ?

Thanks,
Shameer

Max Gurtovoy July 5, 2021, 9:41 a.m. UTC | #5

On 7/5/2021 11:47 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Leon Romanovsky [mailto:[e-mail]]
>> Sent: 04 July 2021 08:04
>> To: Shameerali Kolothum Thodi <[e-mail]>
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; Linuxarm <[e-mail]>; liulongfang
>> <[e-mail]>; Zengtao (B) <[e-mail]>;
>> yuzenghui <[e-mail]>; Jonathan Cameron
>> <[e-mail]>; Wangzhou (B) <[e-mail]>
>> Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon
>> ACC devices
>>
>> On Fri, Jul 02, 2021 at 10:58:46AM +0100, Shameer Kolothum wrote:
>>> Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
>>> This will be extended in follow-up patches to add support for
>>> vfio live migration feature.
>>>
>>> Signed-off-by: Shameer Kolothum
>> <[e-mail]>
>>> ---
>>>   drivers/vfio/pci/Kconfig             |   9 +++
>>>   drivers/vfio/pci/Makefile            |   2 +
>>>   drivers/vfio/pci/hisi_acc_vfio_pci.c | 100 +++++++++++++++++++++++++++
>>>   3 files changed, 111 insertions(+)
>>>   create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c
>> <...>
>>
>>> +static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
>>> +	.name		= "hisi-acc-vfio-pci",
>>> +	.open		= hisi_acc_vfio_pci_open,
>>> +	.release	= vfio_pci_core_release,
>>> +	.ioctl		= vfio_pci_core_ioctl,
>>> +	.read		= vfio_pci_core_read,
>>> +	.write		= vfio_pci_core_write,
>>> +	.mmap		= vfio_pci_core_mmap,
>>> +	.request	= vfio_pci_core_request,
>>> +	.match		= vfio_pci_core_match,
>>> +	.reflck_attach	= vfio_pci_core_reflck_attach,
>> I don't remember what was proposed in vfio-pci-core conversion patches,
>> but would expect that default behaviour is to fallback to vfio_pci_core_* API
>> if ".release/.ioctl/e.t.c" are not redefined.
> Yes, that would be nice, but don't think it does that in latest(v4).
>
> Hi Max,
> Could we please consider fall back to the core defaults, may be check and assign defaults
> in vfio_pci_core_register_device() ?

I don't see why we should do this.

vfio_pci_core.ko is just a library driver. It shouldn't decide for the 
vendor driver ops.

If a vendor driver would like to use its helper functions - great.

If it wants to override it - great.

If it wants to leave some op as NULL - it can do it also.


>
> Thanks,
> Shameer

Shameerali Kolothum Thodi July 5, 2021, 10:18 a.m. UTC | #6

> -----Original Message-----
> From: Max Gurtovoy [mailto:[e-mail]]
> Sent: 05 July 2021 10:42
> To: Shameerali Kolothum Thodi <[e-mail]>;
> Leon Romanovsky <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Linuxarm <[e-mail]>; liulongfang <[e-mail]>;
> Zengtao (B) <[e-mail]>; yuzenghui
> <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Wangzhou (B) <[e-mail]>
> Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon
> ACC devices
> 
> 
> On 7/5/2021 11:47 AM, Shameerali Kolothum Thodi wrote:
> >
> >> -----Original Message-----
> >> From: Leon Romanovsky [mailto:[e-mail]]
> >> Sent: 04 July 2021 08:04
> >> To: Shameerali Kolothum Thodi <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> [e-mail];
> >> [e-mail]; Linuxarm <[e-mail]>; liulongfang
> >> <[e-mail]>; Zengtao (B) <[e-mail]>;
> >> yuzenghui <[e-mail]>; Jonathan Cameron
> >> <[e-mail]>; Wangzhou (B)
> <[e-mail]>
> >> Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for
> HiSilicon
> >> ACC devices
> >>
> >> On Fri, Jul 02, 2021 at 10:58:46AM +0100, Shameer Kolothum wrote:
> >>> Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
> >>> This will be extended in follow-up patches to add support for
> >>> vfio live migration feature.
> >>>
> >>> Signed-off-by: Shameer Kolothum
> >> <[e-mail]>
> >>> ---
> >>>   drivers/vfio/pci/Kconfig             |   9 +++
> >>>   drivers/vfio/pci/Makefile            |   2 +
> >>>   drivers/vfio/pci/hisi_acc_vfio_pci.c | 100
> +++++++++++++++++++++++++++
> >>>   3 files changed, 111 insertions(+)
> >>>   create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c
> >> <...>
> >>
> >>> +static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> >>> +	.name		= "hisi-acc-vfio-pci",
> >>> +	.open		= hisi_acc_vfio_pci_open,
> >>> +	.release	= vfio_pci_core_release,
> >>> +	.ioctl		= vfio_pci_core_ioctl,
> >>> +	.read		= vfio_pci_core_read,
> >>> +	.write		= vfio_pci_core_write,
> >>> +	.mmap		= vfio_pci_core_mmap,
> >>> +	.request	= vfio_pci_core_request,
> >>> +	.match		= vfio_pci_core_match,
> >>> +	.reflck_attach	= vfio_pci_core_reflck_attach,
> >> I don't remember what was proposed in vfio-pci-core conversion patches,
> >> but would expect that default behaviour is to fallback to vfio_pci_core_*
> API
> >> if ".release/.ioctl/e.t.c" are not redefined.
> > Yes, that would be nice, but don't think it does that in latest(v4).
> >
> > Hi Max,
> > Could we please consider fall back to the core defaults, may be check and
> assign defaults
> > in vfio_pci_core_register_device() ?
> 
> I don't see why we should do this.
> 
> vfio_pci_core.ko is just a library driver. It shouldn't decide for the
> vendor driver ops.
> 
> If a vendor driver would like to use its helper functions - great.
> 
> If it wants to override it - great.
> 
> If it wants to leave some op as NULL - it can do it also.

Based on the documentation of the vfio_device_ops callbacks,
It looks like we already have a precedence in the case of reflck_attach
callback where it uses the vfio core default one, if it is not implemented.

Also I would imagine that in majority use cases the vendor drivers will be
defaulting to core functions. 

I think, in any case, it would be good to update the Documentation based on
which way we end up doing this.

Thanks,
Shameer 

 
> 
> 
> >
> > Thanks,
> > Shameer

Leon Romanovsky July 5, 2021, 6:27 p.m. UTC | #7

On Mon, Jul 05, 2021 at 10:18:59AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Max Gurtovoy [mailto:[e-mail]]
> > Sent: 05 July 2021 10:42
> > To: Shameerali Kolothum Thodi <[e-mail]>;
> > Leon Romanovsky <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Linuxarm <[e-mail]>; liulongfang <[e-mail]>;
> > Zengtao (B) <[e-mail]>; yuzenghui
> > <[e-mail]>; Jonathan Cameron
> > <[e-mail]>; Wangzhou (B) <[e-mail]>
> > Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for HiSilicon
> > ACC devices
> > 
> > 
> > On 7/5/2021 11:47 AM, Shameerali Kolothum Thodi wrote:
> > >
> > >> -----Original Message-----
> > >> From: Leon Romanovsky [mailto:[e-mail]]
> > >> Sent: 04 July 2021 08:04
> > >> To: Shameerali Kolothum Thodi <[e-mail]>
> > >> Cc: [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > [e-mail];
> > >> [e-mail]; Linuxarm <[e-mail]>; liulongfang
> > >> <[e-mail]>; Zengtao (B) <[e-mail]>;
> > >> yuzenghui <[e-mail]>; Jonathan Cameron
> > >> <[e-mail]>; Wangzhou (B)
> > <[e-mail]>
> > >> Subject: Re: [RFC v2 1/4] hisi-acc-vfio-pci: add new vfio_pci driver for
> > HiSilicon
> > >> ACC devices
> > >>
> > >> On Fri, Jul 02, 2021 at 10:58:46AM +0100, Shameer Kolothum wrote:
> > >>> Add a vendor-specific vfio_pci driver for HiSilicon ACC devices.
> > >>> This will be extended in follow-up patches to add support for
> > >>> vfio live migration feature.
> > >>>
> > >>> Signed-off-by: Shameer Kolothum
> > >> <[e-mail]>
> > >>> ---
> > >>>   drivers/vfio/pci/Kconfig             |   9 +++
> > >>>   drivers/vfio/pci/Makefile            |   2 +
> > >>>   drivers/vfio/pci/hisi_acc_vfio_pci.c | 100
> > +++++++++++++++++++++++++++
> > >>>   3 files changed, 111 insertions(+)
> > >>>   create mode 100644 drivers/vfio/pci/hisi_acc_vfio_pci.c
> > >> <...>
> > >>
> > >>> +static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> > >>> +	.name		= "hisi-acc-vfio-pci",
> > >>> +	.open		= hisi_acc_vfio_pci_open,
> > >>> +	.release	= vfio_pci_core_release,
> > >>> +	.ioctl		= vfio_pci_core_ioctl,
> > >>> +	.read		= vfio_pci_core_read,
> > >>> +	.write		= vfio_pci_core_write,
> > >>> +	.mmap		= vfio_pci_core_mmap,
> > >>> +	.request	= vfio_pci_core_request,
> > >>> +	.match		= vfio_pci_core_match,
> > >>> +	.reflck_attach	= vfio_pci_core_reflck_attach,
> > >> I don't remember what was proposed in vfio-pci-core conversion patches,
> > >> but would expect that default behaviour is to fallback to vfio_pci_core_*
> > API
> > >> if ".release/.ioctl/e.t.c" are not redefined.
> > > Yes, that would be nice, but don't think it does that in latest(v4).
> > >
> > > Hi Max,
> > > Could we please consider fall back to the core defaults, may be check and
> > assign defaults
> > > in vfio_pci_core_register_device() ?
> > 
> > I don't see why we should do this.
> > 
> > vfio_pci_core.ko is just a library driver. It shouldn't decide for the
> > vendor driver ops.
> > 
> > If a vendor driver would like to use its helper functions - great.
> > 
> > If it wants to override it - great.
> > 
> > If it wants to leave some op as NULL - it can do it also.
> 
> Based on the documentation of the vfio_device_ops callbacks,
> It looks like we already have a precedence in the case of reflck_attach
> callback where it uses the vfio core default one, if it is not implemented.

The reflck_attach pattern is pretty common pattern in the kernel to provide fallback.

> 
> Also I would imagine that in majority use cases the vendor drivers will be
> defaulting to core functions. 

Right, this is whole idea of having core functionality in one place, if
vendor wants/needs, he will overwrite.

> 
> I think, in any case, it would be good to update the Documentation based on
> which way we end up doing this.

The request to update Documentation can be seen as an example of
choosing not-good API decisions. Expectation to see all drivers to
use same callbacks with same vfio-core function calls sounds strange
to me.

Thanks

> 
> Thanks,
> Shameer 
> 
>  
> > 
> > 
> > >
> > > Thanks,
> > > Shameer

Jason Gunthorpe July 5, 2021, 6:32 p.m. UTC | #8

On Mon, Jul 05, 2021 at 09:27:37PM +0300, Leon Romanovsky wrote:

> > I think, in any case, it would be good to update the Documentation based on
> > which way we end up doing this.
> 
> The request to update Documentation can be seen as an example of
> choosing not-good API decisions. Expectation to see all drivers to
> use same callbacks with same vfio-core function calls sounds strange
> to me.

It is not vfio-core, it is vfio-pci-core. It is similar to how some of
the fops stuff works, eg the generic_file whatever functions everyone
puts in.

It would be improved a bit by making the ops struct mutable and
populating it at runtime like we do in RDMA. Then the PCI ops and
driver ops could be merged together without the repetition.

Probably something that is more interesting if there are more drivers
as it is a fair amount of typing to make.

Jason

Leon Romanovsky July 6, 2021, 3:59 a.m. UTC | #9

On Mon, Jul 05, 2021 at 03:32:47PM -0300, Jason Gunthorpe wrote:
> On Mon, Jul 05, 2021 at 09:27:37PM +0300, Leon Romanovsky wrote:
> 
> > > I think, in any case, it would be good to update the Documentation based on
> > > which way we end up doing this.
> > 
> > The request to update Documentation can be seen as an example of
> > choosing not-good API decisions. Expectation to see all drivers to
> > use same callbacks with same vfio-core function calls sounds strange
> > to me.
> 
> It is not vfio-core, it is vfio-pci-core. It is similar to how some of
> the fops stuff works, eg the generic_file whatever functions everyone
> puts in.

It doesn't really matter if it is vfio-core or vfio-pci-core. This looks
horrible and it is going to be repeated for every driver:

+       .release        = vfio_pci_core_release,
+       .ioctl          = vfio_pci_core_ioctl,
+       .read           = vfio_pci_core_read,
+       .write          = vfio_pci_core_write,
+       .mmap           = vfio_pci_core_mmap,
+       .request        = vfio_pci_core_request,
+       .match          = vfio_pci_core_match,
+       .reflck_attach  = vfio_pci_core_reflck_attach,
+};

At some point of time you will add new .XXX callback and will
find yourself changing all drivers to have something like
".XXX = vfio_pci_core_XXX,"

Thanks

Christoph Hellwig July 6, 2021, 4:39 a.m. UTC | #10

On Mon, Jul 05, 2021 at 03:32:47PM -0300, Jason Gunthorpe wrote:
> It would be improved a bit by making the ops struct mutable and
> populating it at runtime like we do in RDMA. Then the PCI ops and
> driver ops could be merged together without the repetition.

No, that would be everything but an improvement.

Jason Gunthorpe July 6, 2021, 11:51 a.m. UTC | #11

On Tue, Jul 06, 2021 at 05:39:25AM +0100, Christoph Hellwig wrote:
> On Mon, Jul 05, 2021 at 03:32:47PM -0300, Jason Gunthorpe wrote:
> > It would be improved a bit by making the ops struct mutable and
> > populating it at runtime like we do in RDMA. Then the PCI ops and
> > driver ops could be merged together without the repetition.
> 
> No, that would be everything but an improvement.

Do you have an alternative? It has worked reasonably with the similar
RDMA problems.

Jason

Patch

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index 9cdef46dd299..709807c28153 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -57,3 +57,12 @@  config MLX5_VFIO_PCI
 	  framework.
 
 	  If you don't know what to do here, say N.
+
+config HISI_ACC_VFIO_PCI
+	tristate "VFIO support for HiSilicon ACC devices"
+	depends on ARM64 && VFIO_PCI_CORE
+	help
+	  This provides generic PCI support for HiSilicon devices using the VFIO
+	  framework.
+
+	  If you don't know what to do here, say N.
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index a0df9c2a4bd9..d1de3e81921f 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -3,6 +3,7 @@ 
 obj-$(CONFIG_VFIO_PCI_CORE) += vfio-pci-core.o
 obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
 obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5-vfio-pci.o
+obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisi-acc-vfio-pci.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
@@ -11,3 +12,4 @@  vfio-pci-y := vfio_pci.o
 vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
 
 mlx5-vfio-pci-y := mlx5_vfio_pci.o
+hisi-acc-vfio-pci-y := hisi_acc_vfio_pci.o
diff --git a/drivers/vfio/pci/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisi_acc_vfio_pci.c
new file mode 100644
index 000000000000..a9e173098ab5
--- /dev/null
+++ b/drivers/vfio/pci/hisi_acc_vfio_pci.c
@@ -0,0 +1,100 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, HiSilicon Ltd.
+ */
+
+#include <linux/device.h>
+#include <linux/eventfd.h>
+#include <linux/file.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/vfio.h>
+#include <linux/vfio_pci_core.h>
+
+static int hisi_acc_vfio_pci_open(struct vfio_device *core_vdev)
+{
+	struct vfio_pci_core_device *vdev =
+		container_of(core_vdev, struct vfio_pci_core_device, vdev);
+	int ret;
+
+	lockdep_assert_held(&core_vdev->reflck->lock);
+
+	ret = vfio_pci_core_enable(vdev);
+	if (ret)
+		return ret;
+
+	vfio_pci_core_finish_enable(vdev);
+
+	return 0;
+}
+
+static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
+	.name		= "hisi-acc-vfio-pci",
+	.open		= hisi_acc_vfio_pci_open,
+	.release	= vfio_pci_core_release,
+	.ioctl		= vfio_pci_core_ioctl,
+	.read		= vfio_pci_core_read,
+	.write		= vfio_pci_core_write,
+	.mmap		= vfio_pci_core_mmap,
+	.request	= vfio_pci_core_request,
+	.match		= vfio_pci_core_match,
+	.reflck_attach	= vfio_pci_core_reflck_attach,
+};
+
+static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct vfio_pci_core_device *vdev;
+	int ret;
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+
+	ret = vfio_pci_core_register_device(vdev, pdev, &hisi_acc_vfio_pci_ops);
+	if (ret)
+		goto out_free;
+
+	dev_set_drvdata(&pdev->dev, vdev);
+
+	return 0;
+
+out_free:
+	kfree(vdev);
+	return ret;
+}
+
+static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev)
+{
+	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
+
+	vfio_pci_core_unregister_device(vdev);
+	kfree(vdev);
+}
+
+static const struct pci_device_id hisi_acc_vfio_pci_table[] = {
+	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI, 0xa256) }, /* SEC VF */
+	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI, 0xa259) }, /* HPRE VF */
+	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_HUAWEI, 0xa251) }, /* ZIP VF */
+	{ 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, hisi_acc_vfio_pci_table);
+
+static struct pci_driver hisi_acc_vfio_pci_driver = {
+	.name			= "hisi-acc-vfio-pci",
+	.id_table		= hisi_acc_vfio_pci_table,
+	.probe			= hisi_acc_vfio_pci_probe,
+	.remove			= hisi_acc_vfio_pci_remove,
+#ifdef CONFIG_PCI_IOV
+	.sriov_configure	= vfio_pci_core_sriov_configure,
+#endif
+	.err_handler		= &vfio_pci_core_err_handlers,
+};
+
+module_pci_driver(hisi_acc_vfio_pci_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Liu Longfang <[e-mail]>");
+MODULE_AUTHOR("Shameer Kolothum <[e-mail]>");
+MODULE_DESCRIPTION("HiSilicon VFIO PCI - Generic VFIO PCI driver for HiSilicon ACC device family");