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

[Nipun Gupta]

On 4/4/2023 2:58 AM, Alex Williamson wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Mon, 3 Apr 2023 19:55:25 +0530
> Nipun Gupta <nipun.gupta@xxxxxxx> wrote:
> > 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
> ...
> > +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);
> > +
> > +     return 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 = 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;
> > +     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;
> > +
> > +     return vfio_cdx_mmap_mmio(vdev->regions[index], vma);
> > +}
>
> I see discussion of MMIO_REGIONS_ENABLE controlling host access to the
> device in mc_cdx_pcol.h.  Is a user of vfio-cdx able to manipulate
> whether MMIO space of the device is enabled?  If so, what's the system
> response to accessing MMIO through the mmap while disabled?

The MMIO enable/disable has been added in mc_cdx_pcol.h from the future 
support perspective, but it is not currently supported, as all the CDX 
devices have the MMIO enable flag permanently set which cannot be 
disabled. Due to this we have not added any interface/API in the CDX bus 
to disable MMIO for the device. This is still under discussion and 
future patches will add complete support for this.

That said, if required we can add a flag currently in the "cdx_device" 
which will be set when for MMIO enabled (it would be by default enabled 
for now), and depending on this flag VFIO can return error during mmap, 
but we would prefer to defer it to be added along with the complete 
support for MMIO enable/disable in the CDX bus.

> Is MMIO
> space accessible even through calling the RESET ioctl?

Yes, MMIO region would be accessible even through calling reset, but 
user may not see the correct values as the device is being reset.


> Is there a
> public specification somewhere for CDX?  Thanks,

I am afraid there is no public specification for CDX as of now.

Thanks,
Nipun

> Alex