On Wed, 23 Aug 2023 15:29:47 +0000
"Zeng, Xin" <xin.zeng@xxxxxxxxx> 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
