RE: [PATCH v4] vfio/cdx: add support for CDX bus

[Gupta, Nipun]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Tuesday, April 18, 2023 5:40 PM
> To: Gupta, Nipun <Nipun.Gupta@xxxxxxx>
> Cc: alex.williamson@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kvm@xxxxxxxxxxxxxxx; masahiroy@xxxxxxxxxx; nathan@xxxxxxxxxx;
> ndesaulniers@xxxxxxxxxx; nicolas@xxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>;
> Anand, Harpreet <harpreet.anand@xxxxxxx>; Jansen Van Vuuren, Pieter
> <pieter.jansen-van-vuuren@xxxxxxx>; Agarwal, Nikhil
> <nikhil.agarwal@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>
> Subject: Re: [PATCH v4] vfio/cdx: add support for CDX bus
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> On Tue, Apr 18, 2023 at 05:06:55PM +0530, Nipun Gupta wrote:
> 
> > 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"
> 
> keep sorted

Since it is not sorted as of now, should a separate patch to be created for
sorting, before adding vfio-cdx?

> 
> >  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/
> 
> keep sorted

Is there Linux guideline here on how objects and folders in Makefile are sorted,
as there are mix and match of files and folders in "drivers/vfio/Makefile".
I could not find any reference for sorting in other Makefiles as well.

> 
> > diff --git a/drivers/vfio/cdx/Makefile b/drivers/vfio/cdx/Makefile
> > new file mode 100644
> > index 000000000000..82e4ef412c0f
> > --- /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 := vfio_cdx.o
> 
> Linus has asked about "suttering" filenames before, suggest to call
> this
> 
> "vfio/cdx/main.c"

Okay, I do not any strong affiliation towards the name.
Alex, your thoughts on this please?

> 
> > +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);
> > +     struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> > +     unsigned long minsz;
> > +
> > +     switch (cmd) {
> > +     case VFIO_DEVICE_GET_INFO:
> > +     {
> > +             struct vfio_device_info info;
> > +
> > +             minsz = offsetofend(struct vfio_device_info, num_irqs);
> > +
> > +             if (copy_from_user(&info, (void __user *)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((void __user *)arg, &info, minsz) ?
> > +                     -EFAULT : 0;
> > +     }
> > +     case VFIO_DEVICE_GET_REGION_INFO:
> > +     {
> > +             struct vfio_region_info info;
> > +
> > +             minsz = offsetofend(struct vfio_region_info, offset);
> > +
> > +             if (copy_from_user(&info, (void __user *)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;
> > +
> > +             if (copy_to_user((void __user *)arg, &info, minsz))
> > +                     return -EFAULT;
> > +             return 0;
> > +     }
> > +     case VFIO_DEVICE_RESET:
> > +     {
> > +             return cdx_dev_reset(core_vdev->dev);
> > +     }
> > +     default:
> > +             return -ENOTTY;
> > +     }
> > +}
> 
> Please split this into functions, eg look at PCI

Sure, will split this into functions.

Regards,
Nipun

> 
> Jason