Re: [RFC 5/5] vfio/qat: Add vfio_pci driver for Intel QAT VF devices

From: Alex Williamson <alex.williamson@redhat.com>
To: "Zeng, Xin" <xin.zeng@intel.com>
Cc: "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Cabiddu, Giovanni" <giovanni.cabiddu@intel.com>,
	"andriy.shevchenko@linux.intel.com" 
	<andriy.shevchenko@linux.intel.com>,
	"Cao, Yahui" <yahui.cao@intel.com>,
	"jgg@nvidia.com" <jgg@nvidia.com>,
	"yishaih@nvidia.com" <yishaih@nvidia.com>,
	"shameerali.kolothum.thodi@huawei.com" 
	<shameerali.kolothum.thodi@huawei.com>,
	"Tian, Kevin" <kevin.tian@intel.com>
Subject: Re: [RFC 5/5] vfio/qat: Add vfio_pci driver for Intel QAT VF devices
Date: Thu, 24 Aug 2023 09:25:27 -0600	[thread overview]
Message-ID: <20230824092527.143eebbc.alex.williamson@redhat.com> (raw)
In-Reply-To: <DM4PR11MB550203FB22F8D6F0EAF8F717881CA@DM4PR11MB5502.namprd11.prod.outlook.com>

On Wed, 23 Aug 2023 15:29:47 +0000
"Zeng, Xin" <xin.zeng@intel.com> wrote:

> Thanks for the comments, Alex.
> On Thursday, July 27, 2023 3:37 AM, Alex Williamson wrote:
> > >  drivers/vfio/pci/Kconfig                 |   2 +
> > >  drivers/vfio/pci/Makefile                |   1 +
> > >  drivers/vfio/pci/qat/Kconfig             |  13 +
> > >  drivers/vfio/pci/qat/Makefile            |   4 +
> > >  drivers/vfio/pci/qat/qat_vfio_pci_main.c | 518  
> > +++++++++++++++++++++++
> > 
> > Rename to main.c.  
> 
> Will do in next version.
> 
> >   
> > >  5 files changed, 538 insertions(+)
> > >  create mode 100644 drivers/vfio/pci/qat/Kconfig
> > >  create mode 100644 drivers/vfio/pci/qat/Makefile
> > >  create mode 100644 drivers/vfio/pci/qat/qat_vfio_pci_main.c
> > >
> > > diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> > > index f9d0c908e738..47c9773cf0c7 100644
> > > --- a/drivers/vfio/pci/Kconfig
> > > +++ b/drivers/vfio/pci/Kconfig
> > > @@ -59,4 +59,6 @@ source "drivers/vfio/pci/mlx5/Kconfig"
> > >
> > >  source "drivers/vfio/pci/hisilicon/Kconfig"
> > >
> > > +source "drivers/vfio/pci/qat/Kconfig"
> > > +
> > >  endif
> > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > index 24c524224da5..dcc6366df8fa 100644
> > > --- a/drivers/vfio/pci/Makefile
> > > +++ b/drivers/vfio/pci/Makefile
> > > @@ -11,3 +11,4 @@ obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> > >  obj-$(CONFIG_MLX5_VFIO_PCI)           += mlx5/
> > >
> > >  obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisilicon/
> > > +obj-$(CONFIG_QAT_VFIO_PCI) += qat/
> > > diff --git a/drivers/vfio/pci/qat/Kconfig b/drivers/vfio/pci/qat/Kconfig
> > > new file mode 100644
> > > index 000000000000..38e5b4a0ca9c
> > > --- /dev/null
> > > +++ b/drivers/vfio/pci/qat/Kconfig
> > > @@ -0,0 +1,13 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +config QAT_VFIO_PCI
> > > +	tristate "VFIO support for QAT VF PCI devices"
> > > +	depends on X86  
> > 
> > What specific X86 dependency exists here?  CRYPTO_DEV_QAT and the
> > various versions of the QAT driver don't seem to have an explicit arch
> > dependency, therefore this shouldn't either.  
> 
> You are right. Will remove it.
> 
> >   
> > > +	depends on VFIO_PCI_CORE  
> > 
> > select VFIO_PCI_CORE, this was updated for all vfio-pci variant drivers
> > for v6.5.  
> 
> Will update it.
> 
> >   
> > > +
> > > diff --git a/drivers/vfio/pci/qat/qat_vfio_pci_main.c  
> > b/drivers/vfio/pci/qat/qat_vfio_pci_main.c  
> > > new file mode 100644
> > > index 000000000000..af971fd05fd2
> > > --- /dev/null
> > > +++ b/drivers/vfio/pci/qat/qat_vfio_pci_main.c
> > > @@ -0,0 +1,518 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/* Copyright(c) 2023 Intel Corporation */
> > > +#include <linux/anon_inodes.h>
> > > +#include <linux/container_of.h>
> > > +#include <linux/device.h>
> > > +#include <linux/file.h>
> > > +#include <linux/init.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/sizes.h>
> > > +#include <linux/types.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/vfio_pci_core.h>
> > > +#include <linux/qat/qat_vf_mig.h>
> > > +
> > > +struct qat_vf_mig_data {
> > > +	u8 state[SZ_4K];
> > > +};
> > > +
> > > +struct qat_vf_migration_file {
> > > +	struct file *filp;
> > > +	struct mutex lock; /* protect migration region context */
> > > +	bool disabled;
> > > +
> > > +	size_t total_length;
> > > +	struct qat_vf_mig_data mig_data;
> > > +};
> > > +
> > > +static void qat_vf_vfio_pci_remove(struct pci_dev *pdev)
> > > +{
> > > +	struct qat_vf_core_device *qat_vdev = qat_vf_drvdata(pdev);
> > > +
> > > +	vfio_pci_core_unregister_device(&qat_vdev->core_device);
> > > +	vfio_put_device(&qat_vdev->core_device.vdev);
> > > +}
> > > +
> > > +static const struct pci_device_id qat_vf_vfio_pci_table[] = {
> > > +	/* Intel QAT GEN4 4xxx VF device */
> > > +	{ PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_INTEL,  
> > 0x4941) },
> > 
> > Should this driver depend on CRYPTO_DEV_QAT_4XXX if that's the only
> > supported PF driver?  
> 
> This module has not any dependency to QAT_4XXX module at build time, but it indeed has implicit
> dependency on QAT_4XXX runtime to enable SRIOV and complete the QAT 4xxx VF migration,
> do you think we still need to put this dependency explicitly in Kconfig?

What benefit does it serve a user to be able to build this module if
the runtime dependency isn't present in the kernel?  We have
COMPILE_TEST to support build time regression testing.  Thanks,

Alex