Message-ID: <20230524134831.28dc97e2.alex.williamson@redhat.com>
Date:   Wed, 24 May 2023 13:48:31 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Nipun Gupta <nipun.gupta@....com>
Cc:     <jgg@...pe.ca>, <linux-kernel@...r.kernel.org>,
        <kvm@...r.kernel.org>, <masahiroy@...nel.org>, <nathan@...nel.org>,
        <ndesaulniers@...gle.com>, <nicolas@...sle.eu>, <git@....com>,
        <harpreet.anand@....com>, <pieter.jansen-van-vuuren@....com>,
        <nikhil.agarwal@....com>, <michal.simek@....com>
Subject: Re: [PATCH v6] vfio/cdx: add support for CDX bus

On Wed, 24 May 2023 10:45:29 -0600
Alex Williamson <alex.williamson@...hat.com> wrote:

> On Wed, 17 May 2023 15:27:18 +0530
> Nipun Gupta <nipun.gupta@....com> wrote:
> 
> > vfio-cdx driver enables IOCTLs for user space to query
> > MMIO regions for CDX devices and mmap them. This change
> > also adds support for reset of CDX devices. With VFIO
> > enabled on CDX devices, user-space applications can also
> > exercise DMA securely via IOMMU on these devices.
> > 
> > This change adds the VFIO CDX driver and enables the following
> > ioctls for CDX devices:
> >  - VFIO_DEVICE_GET_INFO:
> >  - VFIO_DEVICE_GET_REGION_INFO
> >  - VFIO_DEVICE_RESET
> > 
> > Signed-off-by: Nipun Gupta <nipun.gupta@....com>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@....com>
> > Tested-by: Nikhil Agarwal <nikhil.agarwal@....com>
> > ---
> > 
> > Changes v5->v6:
> > - removed forward declaration of vfio_cdx_driver
> > - removed un-necessary CDX_DRIVER_OVERRIDE_DEVICE_VFIO and
> >   vfio_cdx_regions_cleanup.
> > - removed unrequired dev_warn/dev_err
> > - used module_driver instead of module_init/exit
> > 
> > Changes v4->v5:
> > - renamed vfio_cdx.c to main.c and vfio_cdx_private.h
> >   to private.h
> > - have separate functions for get_info and get_region_info
> > 
> > Changes v3->v4:
> > - fix vfio info flags
> > 
> > Changes v2->v3:
> > - removed redundant init and release functions
> > - removed redundant dev and cdx_dev from vfio_cdx_device
> > - added support for iommufd
> > - added VFIO_DEVICE_FLAGS_CDX
> > - removed unrequried WARN_ON
> > - removed unused ioaddr
> > 
> > Changes v1->v2:
> > - Updated file2alias to support vfio_cdx
> > - removed some un-necessary checks in mmap
> > - removed vfio reset wrapper API
> > - converted complex macros to static APIs
> > - used pgprot_device and io_remap_pfn_range
> > 
> >  MAINTAINERS                       |   7 +
> >  drivers/vfio/Kconfig              |   1 +
> >  drivers/vfio/Makefile             |   1 +
> >  drivers/vfio/cdx/Kconfig          |  17 +++
> >  drivers/vfio/cdx/Makefile         |   8 +
> >  drivers/vfio/cdx/main.c           | 234 ++++++++++++++++++++++++++++++
> >  drivers/vfio/cdx/private.h        |  28 ++++
> >  include/linux/cdx/cdx_bus.h       |   1 -
> >  include/linux/mod_devicetable.h   |   6 +
> >  include/uapi/linux/vfio.h         |   1 +
> >  scripts/mod/devicetable-offsets.c |   1 +
> >  scripts/mod/file2alias.c          |  17 ++-
> >  12 files changed, 320 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/vfio/cdx/Kconfig
> >  create mode 100644 drivers/vfio/cdx/Makefile
> >  create mode 100644 drivers/vfio/cdx/main.c
> >  create mode 100644 drivers/vfio/cdx/private.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a72b8fcea261..d6d1ddb854d7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -22096,6 +22096,13 @@ F:	Documentation/filesystems/vfat.rst
> >  F:	fs/fat/
> >  F:	tools/testing/selftests/filesystems/fat/
> >  
> > +VFIO CDX DRIVER
> > +M:	Nipun Gupta <nipun.gupta@....com>
> > +M:	Nikhil Agarwal <nikhil.agarwal@....com>
> > +L:	kvm@...r.kernel.org
> > +S:	Maintained
> > +F:	drivers/vfio/cdx/*
> > +
> >  VFIO DRIVER
> >  M:	Alex Williamson <alex.williamson@...hat.com>
> >  L:	kvm@...r.kernel.org
> > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> > index 89e06c981e43..aba36f5be4ec 100644
> > --- a/drivers/vfio/Kconfig
> > +++ b/drivers/vfio/Kconfig
> > @@ -57,6 +57,7 @@ source "drivers/vfio/pci/Kconfig"
> >  source "drivers/vfio/platform/Kconfig"
> >  source "drivers/vfio/mdev/Kconfig"
> >  source "drivers/vfio/fsl-mc/Kconfig"
> > +source "drivers/vfio/cdx/Kconfig"
> >  endif
> >  
> >  source "virt/lib/Kconfig"
> > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> > index 70e7dcb302ef..1a27b2516612 100644
> > --- a/drivers/vfio/Makefile
> > +++ b/drivers/vfio/Makefile
> > @@ -14,3 +14,4 @@ obj-$(CONFIG_VFIO_PCI) += pci/
> >  obj-$(CONFIG_VFIO_PLATFORM) += platform/
> >  obj-$(CONFIG_VFIO_MDEV) += mdev/
> >  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> > +obj-$(CONFIG_VFIO_CDX) += cdx/
> > diff --git a/drivers/vfio/cdx/Kconfig b/drivers/vfio/cdx/Kconfig
> > new file mode 100644
> > index 000000000000..e6de0a0caa32
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/Kconfig
> > @@ -0,0 +1,17 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# VFIO CDX configuration
> > +#
> > +# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > +#
> > +
> > +config VFIO_CDX
> > +	tristate "VFIO support for CDX bus devices"
> > +	depends on CDX_BUS
> > +	select EVENTFD
> > +	help
> > +	  Driver to enable VFIO support for the devices on CDX bus.
> > +	  This is required to make use of CDX devices present in
> > +	  the system using the VFIO framework.
> > +
> > +	  If you don't know what to do here, say N.
> > diff --git a/drivers/vfio/cdx/Makefile b/drivers/vfio/cdx/Makefile
> > new file mode 100644
> > index 000000000000..cd4a2e6fe609
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/Makefile
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > +#
> > +
> > +obj-$(CONFIG_VFIO_CDX) += vfio-cdx.o
> > +
> > +vfio-cdx-objs := main.o
> > diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
> > new file mode 100644
> > index 000000000000..f03f491e0435
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/main.c
> > @@ -0,0 +1,234 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > + */
> > +
> > +#include <linux/vfio.h>
> > +#include <linux/cdx/cdx_bus.h>
> > +
> > +#include "private.h"
> > +
> > +static int vfio_cdx_open_device(struct vfio_device *core_vdev)
> > +{
> > +	struct vfio_cdx_device *vdev =
> > +		container_of(core_vdev, struct vfio_cdx_device, vdev);
> > +	struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> > +	int count = cdx_dev->res_count;
> > +	int i;
> > +
> > +	vdev->regions = kcalloc(count, sizeof(struct vfio_cdx_region),
> > +				GFP_KERNEL);  
> 
> Nit, GFP_KERNEL_ACCOUNT since we're allocating long term storage on
> behalf of a user operation.
> 
> > +	if (!vdev->regions)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		struct resource *res = &cdx_dev->res[i];
> > +
> > +		vdev->regions[i].addr = res->start;
> > +		vdev->regions[i].size = resource_size(res);
> > +		vdev->regions[i].type = res->flags;
> > +		/*
> > +		 * Only regions addressed with PAGE granularity may be
> > +		 * MMAP'ed securely.
> > +		 */
> > +		if (!(vdev->regions[i].addr & ~PAGE_MASK) &&
> > +		    !(vdev->regions[i].size & ~PAGE_MASK))
> > +			vdev->regions[i].flags |=
> > +					VFIO_REGION_INFO_FLAG_MMAP;
> > +		vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_READ;
> > +		if (!(cdx_dev->res[i].flags & IORESOURCE_READONLY))
> > +			vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void vfio_cdx_close_device(struct vfio_device *core_vdev)
> > +{
> > +	struct vfio_cdx_device *vdev =
> > +		container_of(core_vdev, struct vfio_cdx_device, vdev);
> > +
> > +	kfree(vdev->regions);
> > +	cdx_dev_reset(core_vdev->dev);
> > +}
> > +
> > +static int vfio_cdx_ioctl_get_info(struct vfio_cdx_device *vdev,
> > +				   struct vfio_device_info __user *arg)
> > +{
> > +	unsigned long minsz = offsetofend(struct vfio_device_info, num_irqs);
> > +	struct cdx_device *cdx_dev = to_cdx_device(vdev->vdev.dev);
> > +	struct vfio_device_info info;
> > +
> > +	if (copy_from_user(&info, arg, minsz))
> > +		return -EFAULT;
> > +
> > +	if (info.argsz < minsz)
> > +		return -EINVAL;
> > +
> > +	info.flags = VFIO_DEVICE_FLAGS_CDX;
> > +	info.flags |= VFIO_DEVICE_FLAGS_RESET;
> > +
> > +	info.num_regions = cdx_dev->res_count;
> > +	info.num_irqs = 0;
> > +
> > +	return copy_to_user(arg, &info, minsz) ? -EFAULT : 0;
> > +}
> > +
> > +static int vfio_cdx_ioctl_get_region_info(struct vfio_cdx_device *vdev,
> > +					  struct vfio_region_info __user *arg)
> > +{
> > +	unsigned long minsz = offsetofend(struct vfio_region_info, offset);
> > +	struct cdx_device *cdx_dev = to_cdx_device(vdev->vdev.dev);
> > +	struct vfio_region_info info;
> > +
> > +	if (copy_from_user(&info, arg, minsz))
> > +		return -EFAULT;
> > +
> > +	if (info.argsz < minsz)
> > +		return -EINVAL;
> > +
> > +	if (info.index >= cdx_dev->res_count)
> > +		return -EINVAL;
> > +
> > +	/* map offset to the physical address */
> > +	info.offset = vfio_cdx_index_to_offset(info.index);
> > +	info.size = vdev->regions[info.index].size;
> > +	info.flags = vdev->regions[info.index].flags;
> > +
> > +	return copy_to_user(arg, &info, minsz) ? -EFAULT : 0;
> > +}
> > +
> > +static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
> > +			   unsigned int cmd, unsigned long arg)
> > +{
> > +	struct vfio_cdx_device *vdev =
> > +		container_of(core_vdev, struct vfio_cdx_device, vdev);
> > +	void __user *uarg = (void __user *)arg;
> > +
> > +	switch (cmd) {
> > +	case VFIO_DEVICE_GET_INFO:
> > +		return vfio_cdx_ioctl_get_info(vdev, uarg);
> > +	case VFIO_DEVICE_GET_REGION_INFO:
> > +		return vfio_cdx_ioctl_get_region_info(vdev, uarg);
> > +	case VFIO_DEVICE_RESET:
> > +		return cdx_dev_reset(core_vdev->dev);
> > +	default:
> > +		return -ENOTTY;
> > +	}
> > +}
> > +
> > +static int vfio_cdx_mmap_mmio(struct vfio_cdx_region region,
> > +			      struct vm_area_struct *vma)
> > +{
> > +	u64 size = vma->vm_end - vma->vm_start;
> > +	u64 pgoff, base;
> > +
> > +	pgoff = vma->vm_pgoff &
> > +		((1U << (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> > +	base = pgoff << PAGE_SHIFT;
> > +
> > +	if (region.size < PAGE_SIZE || base + size > region.size)  
> 
> Nit, we've already enforced the first condition, a sub-page region
> won't have the mmap flag set and we already verified this region does
> have that flag set.
> 
> > +		return -EINVAL;
> > +
> > +	vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
> > +	vma->vm_page_prot = pgprot_device(vma->vm_page_prot);
> > +
> > +	return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > +				  size, vma->vm_page_prot);
> > +}
> > +
> > +static int vfio_cdx_mmap(struct vfio_device *core_vdev,
> > +			 struct vm_area_struct *vma)
> > +{
> > +	struct vfio_cdx_device *vdev =
> > +		container_of(core_vdev, struct vfio_cdx_device, vdev);
> > +	struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> > +	unsigned int index;
> > +
> > +	index = vma->vm_pgoff >> (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT);
> > +
> > +	if (index >= cdx_dev->res_count)
> > +		return -EINVAL;
> > +
> > +	if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_MMAP))
> > +		return -EINVAL;
> > +
> > +	if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_READ) &&
> > +	    (vma->vm_flags & VM_READ))
> > +		return -EINVAL;
> > +
> > +	if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE) &&
> > +	    (vma->vm_flags & VM_WRITE))
> > +		return -EINVAL;  
> 
> It might be useful to distinguish these two cases with -EPERM.
> Otherwise this looks ok to me.  Thanks,
> 
> Alex
> 
> > +
> > +	return vfio_cdx_mmap_mmio(vdev->regions[index], vma);
> > +}
> > +
> > +static const struct vfio_device_ops vfio_cdx_ops = {
> > +	.name		= "vfio-cdx",
> > +	.open_device	= vfio_cdx_open_device,
> > +	.close_device	= vfio_cdx_close_device,
> > +	.ioctl		= vfio_cdx_ioctl,
> > +	.mmap		= vfio_cdx_mmap,
> > +	.bind_iommufd	= vfio_iommufd_physical_bind,
> > +	.unbind_iommufd	= vfio_iommufd_physical_unbind,
> > +	.attach_ioas	= vfio_iommufd_physical_attach_ioas,
> > +};
> > +
> > +static int vfio_cdx_probe(struct cdx_device *cdx_dev)
> > +{
> > +	struct vfio_cdx_device *vdev = NULL;
> > +	struct device *dev = &cdx_dev->dev;
> > +	int ret;
> > +
> > +	vdev = vfio_alloc_device(vfio_cdx_device, vdev, dev,
> > +				 &vfio_cdx_ops);
> > +	if (IS_ERR(vdev))
> > +		return PTR_ERR(vdev);
> > +
> > +	ret = vfio_register_group_dev(&vdev->vdev);
> > +	if (ret)
> > +		goto out_uninit;
> > +
> > +	dev_set_drvdata(dev, vdev);
> > +	return 0;
> > +
> > +out_uninit:
> > +	vfio_put_device(&vdev->vdev);
> > +	return ret;
> > +}
> > +
> > +static int vfio_cdx_remove(struct cdx_device *cdx_dev)
> > +{
> > +	struct device *dev = &cdx_dev->dev;
> > +	struct vfio_cdx_device *vdev = dev_get_drvdata(dev);
> > +
> > +	vfio_unregister_group_dev(&vdev->vdev);
> > +	vfio_put_device(&vdev->vdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct cdx_device_id vfio_cdx_table[] = {
> > +	{ CDX_DEVICE_DRIVER_OVERRIDE(CDX_ANY_ID, CDX_ANY_ID,
> > +				     CDX_ID_F_VFIO_DRIVER_OVERRIDE) }, /* match all by default */
> > +	{}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(cdx, vfio_cdx_table);
> > +
> > +static struct cdx_driver vfio_cdx_driver = {
> > +	.probe		= vfio_cdx_probe,
> > +	.remove		= vfio_cdx_remove,
> > +	.match_id_table	= vfio_cdx_table,
> > +	.driver	= {
> > +		.name	= "vfio-cdx",
> > +		.owner	= THIS_MODULE,
> > +	},
> > +	.driver_managed_dma = true,

Hmm, looks like cdx bus is broken here, there's no actual
implementation of a dma_configure callback and no setup of the IOMMU
default domain for theoretical cdx drivers that might want to use the
DMA API.  Without that, this driver_manged_dma flag doesn't provide any
guarantees to a vfio driver that we have exclusive ownership of the
group.  Please fix, this flag needs to actually have some meaning on
cdx.  Thanks,

Alex

> > +};
> > +
> > +module_driver(vfio_cdx_driver, cdx_driver_register, cdx_driver_unregister);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("VFIO for CDX devices - User Level meta-driver");
> > diff --git a/drivers/vfio/cdx/private.h b/drivers/vfio/cdx/private.h
> > new file mode 100644
> > index 000000000000..8bdc117ea88e
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/private.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > + */
> > +
> > +#ifndef VFIO_CDX_PRIVATE_H
> > +#define VFIO_CDX_PRIVATE_H
> > +
> > +#define VFIO_CDX_OFFSET_SHIFT    40
> > +
> > +static inline u64 vfio_cdx_index_to_offset(u32 index)
> > +{
> > +	return ((u64)(index) << VFIO_CDX_OFFSET_SHIFT);
> > +}
> > +
> > +struct vfio_cdx_region {
> > +	u32			flags;
> > +	u32			type;
> > +	u64			addr;
> > +	resource_size_t		size;
> > +};
> > +
> > +struct vfio_cdx_device {
> > +	struct vfio_device	vdev;
> > +	struct vfio_cdx_region	*regions;
> > +};
> > +
> > +#endif /* VFIO_CDX_PRIVATE_H */
> > diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
> > index 35ef41d8a61a..bead71b7bc73 100644
> > --- a/include/linux/cdx/cdx_bus.h
> > +++ b/include/linux/cdx/cdx_bus.h
> > @@ -14,7 +14,6 @@
> >  #include <linux/mod_devicetable.h>
> >  
> >  #define MAX_CDX_DEV_RESOURCES	4
> > -#define CDX_ANY_ID (0xFFFF)
> >  #define CDX_CONTROLLER_ID_SHIFT 4
> >  #define CDX_BUS_NUM_MASK 0xF
> >  
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index ccaaeda792c0..ccf017353bb6 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -912,6 +912,12 @@ struct ishtp_device_id {
> >  	kernel_ulong_t driver_data;
> >  };
> >  
> > +#define CDX_ANY_ID (0xFFFF)
> > +
> > +enum {
> > +	CDX_ID_F_VFIO_DRIVER_OVERRIDE = 1,
> > +};
> > +
> >  /**
> >   * struct cdx_device_id - CDX device identifier
> >   * @vendor: Vendor ID
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 0552e8dcf0cb..8e91aaf973e7 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -213,6 +213,7 @@ struct vfio_device_info {
> >  #define VFIO_DEVICE_FLAGS_AP	(1 << 5)	/* vfio-ap device */
> >  #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6)	/* vfio-fsl-mc device */
> >  #define VFIO_DEVICE_FLAGS_CAPS	(1 << 7)	/* Info supports caps */
> > +#define VFIO_DEVICE_FLAGS_CDX	(1 << 8)	/* vfio-cdx device */
> >  	__u32	num_regions;	/* Max region index + 1 */
> >  	__u32	num_irqs;	/* Max IRQ index + 1 */
> >  	__u32   cap_offset;	/* Offset within info struct of first cap */
> > diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> > index 62dc988df84d..abe65f8968dd 100644
> > --- a/scripts/mod/devicetable-offsets.c
> > +++ b/scripts/mod/devicetable-offsets.c
> > @@ -265,6 +265,7 @@ int main(void)
> >  	DEVID(cdx_device_id);
> >  	DEVID_FIELD(cdx_device_id, vendor);
> >  	DEVID_FIELD(cdx_device_id, device);
> > +	DEVID_FIELD(cdx_device_id, override_only);
> >  
> >  	return 0;
> >  }
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index 28da34ba4359..38120f932b0d 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1458,8 +1458,23 @@ static int do_cdx_entry(const char *filename, void *symval,
> >  {
> >  	DEF_FIELD(symval, cdx_device_id, vendor);
> >  	DEF_FIELD(symval, cdx_device_id, device);
> > +	DEF_FIELD(symval, cdx_device_id, override_only);
> >  
> > -	sprintf(alias, "cdx:v%08Xd%08Xd", vendor, device);
> > +	switch (override_only) {
> > +	case 0:
> > +		strcpy(alias, "cdx:");
> > +		break;
> > +	case CDX_ID_F_VFIO_DRIVER_OVERRIDE:
> > +		strcpy(alias, "vfio_cdx:");
> > +		break;
> > +	default:
> > +		warn("Unknown CDX driver_override alias %08X\n",
> > +		     override_only);
> > +		return 0;
> > +	}
> > +
> > +	ADD(alias, "v", vendor != CDX_ANY_ID, vendor);
> > +	ADD(alias, "d", device != CDX_ANY_ID, device);
> >  	return 1;
> >  }
> >    
> 

Powered by blists - more mailing lists