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

[Nipun Gupta]

On 4/5/2023 9:03 PM, Jason Gunthorpe wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Mon, Apr 03, 2023 at 07:55:25PM +0530, Nipun Gupta wrote:
>
> > +enum {
> > +     CDX_ID_F_VFIO_DRIVER_OVERRIDE = 1,
> > +};
>
> This seems to be missing the file2alias part.

Sure. Will add this.

> > +static void vfio_cdx_regions_cleanup(struct vfio_cdx_device *vdev)
> > +{
> > +     kfree(vdev->regions);
> > +}
> > +
> > +static int vfio_cdx_reset_device(struct vfio_cdx_device *vdev)
> > +{
> > +     return cdx_dev_reset(&vdev->cdx_dev->dev);
> > +}
>
> Wrapper functions should be avoided.

Agree

> > +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);
> > +     int ret;
> > +
> > +     vfio_cdx_regions_cleanup(vdev);
> > +
> > +     /* reset the device before cleaning up the interrupts */
> > +     ret = vfio_cdx_reset_device(vdev);
> > +     if (WARN_ON(ret))
> > +             dev_warn(core_vdev->dev,
> > +                      "VFIO_CDX: reset device has failed (%d)\n", ret);
>
> This is pretty problematic.. if the reset can fail the device is
> returned to the system in an unknown state and it seems pretty likely
> that it can be a way to attack the kernel.

We will update the code to disable the device in case of failures.

> > +     case VFIO_DEVICE_RESET:
> > +     {
> > +             return vfio_cdx_reset_device(vdev);
> > +     }
>
> What happens to MMIO access during this reset?

There is no config space here; and access to mmap space will go as usual 
but user may not see the expected behavior as the device is being reset. 
It is responsibility of user of VFIO device to serialize such access and 
reset.

> > +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)
> > +             return -EINVAL;
> > +
> > +     vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
> > +     vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>
> pgprot_device

Will update..

> > +     return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > +                            size, vma->vm_page_prot);
>
> io_remap_pfn_range
> > +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 = vdev->cdx_dev;
> > +     unsigned int index;
> > +
> > +     index = vma->vm_pgoff >> (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT);
> > +
> > +     if (vma->vm_end < vma->vm_start)
> > +             return -EINVAL;
> > +     if (vma->vm_start & ~PAGE_MASK)
> > +             return -EINVAL;
> > +     if (vma->vm_end & ~PAGE_MASK)
> > +             return -EINVAL;
>
> The core code already assures these checks.

Sure will remove these unnecessary checks.

> > +     if (!(vma->vm_flags & VM_SHARED))
> > +             return -EINVAL;
> > +     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;
> > +
> > +     vma->vm_private_data = cdx_dev;
>
> not needed

Okay

> > diff --git a/drivers/vfio/cdx/vfio_cdx_private.h b/drivers/vfio/cdx/vfio_cdx_private.h
> > new file mode 100644
> > index 000000000000..8b6f1ee3f5cd
> > --- /dev/null
> > +++ b/drivers/vfio/cdx/vfio_cdx_private.h
> > @@ -0,0 +1,32 @@
> > +/* 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
> > +#define VFIO_CDX_OFFSET_MASK (((u64)(1) << VFIO_CDX_OFFSET_SHIFT) - 1)
> > +
> > +#define VFIO_CDX_OFFSET_TO_INDEX(off) ((off) >> VFIO_CDX_OFFSET_SHIFT)
> > +
> > +#define VFIO_CDX_INDEX_TO_OFFSET(index)      \
> > +     ((u64)(index) << VFIO_CDX_OFFSET_SHIFT)
>
> use static inlines for function-line macros

Okay. Will update the code.

> > +struct vfio_cdx_region {
> > +     u32                     flags;
> > +     u32                     type;
> > +     u64                     addr;
> > +     resource_size_t         size;
> > +     void __iomem            *ioaddr;
> > +};
> > +
> > +struct vfio_cdx_device {
> > +     struct vfio_device      vdev;
> > +     struct cdx_device       *cdx_dev;
> > +     struct device           *dev;
> > +     struct vfio_cdx_region  *regions;
> > +};
>
> This header file does not seem necessary right now

There is work in progress which would add support of MSI in VFIO-CDX 
driver, and where would be additional file/s. So we would like to keep 
the header file separate.

Thanks,
Nipun

> Jason