Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On Mon, Mar 01, 2021 at 05:48:00PM -0700, Dave Jiang wrote: > > On 3/1/2021 5:29 PM, Jason Gunthorpe wrote: > > On Mon, Mar 01, 2021 at 05:23:47PM -0700, Dave Jiang wrote: > > > So after looking at the code in vfio_pci_intrs.c, I agree that the > > > set_irqs > > > code between VFIO_PCI and this driver can be made in common. Given that > > > Alex > > > doesn't want a vfio_pci device embedded in the driver, > > idxd isn't a vfio_pci so it would be improper to do something like > > that here anyhow. > > > > > I think we'll need some sort of generic VFIO device that can be used > > > from the vfio_pci side and vfio_mdev side to pass down in order to > > > have common support library functions. > > Why do you need more layers? > > > > Just make some helper functions to manage this and build them into > > their own struct and function family. All this needs is some callback > > to for the end driver to hook in the raw device programming and some > > entry points to direct the emulation access to the module. > > > > It should be fully self contained and completely unrelated to vfio_pci > > > Maybe I'm looking at this wrong. I see a some code in vfio_pci_intrs.c that > we can reuse with some changes here and there. But, I think see where you > are getting at with just common functions for mdev side. Let me create it > just for IMS emulation and then we can go from there trying to figure if > that's the right path to go down or if we need to share code with vfio_pci. If it really is very common it could all be consolidated in a vfio_utils.c kind of thing that all the places can use. There is nothing wrong with splitting pieces of vfio_pci out. Jason
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On 3/1/2021 5:29 PM, Jason Gunthorpe wrote: On Mon, Mar 01, 2021 at 05:23:47PM -0700, Dave Jiang wrote: So after looking at the code in vfio_pci_intrs.c, I agree that the set_irqs code between VFIO_PCI and this driver can be made in common. Given that Alex doesn't want a vfio_pci device embedded in the driver, idxd isn't a vfio_pci so it would be improper to do something like that here anyhow. I think we'll need some sort of generic VFIO device that can be used from the vfio_pci side and vfio_mdev side to pass down in order to have common support library functions. Why do you need more layers? Just make some helper functions to manage this and build them into their own struct and function family. All this needs is some callback to for the end driver to hook in the raw device programming and some entry points to direct the emulation access to the module. It should be fully self contained and completely unrelated to vfio_pci Maybe I'm looking at this wrong. I see a some code in vfio_pci_intrs.c that we can reuse with some changes here and there. But, I think see where you are getting at with just common functions for mdev side. Let me create it just for IMS emulation and then we can go from there trying to figure if that's the right path to go down or if we need to share code with vfio_pci.
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On Mon, Mar 01, 2021 at 05:23:47PM -0700, Dave Jiang wrote: > > So after looking at the code in vfio_pci_intrs.c, I agree that the set_irqs > code between VFIO_PCI and this driver can be made in common. Given that Alex > doesn't want a vfio_pci device embedded in the driver, idxd isn't a vfio_pci so it would be improper to do something like that here anyhow. > I think we'll need some sort of generic VFIO device that can be used > from the vfio_pci side and vfio_mdev side to pass down in order to > have common support library functions. Why do you need more layers? Just make some helper functions to manage this and build them into their own struct and function family. All this needs is some callback to for the end driver to hook in the raw device programming and some entry points to direct the emulation access to the module. It should be fully self contained and completely unrelated to vfio_pci Jason
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On 2/10/2021 4:59 PM, Jason Gunthorpe wrote:
On Fri, Feb 05, 2021 at 01:53:24PM -0700, Dave Jiang wrote:
<-- cut for brevity -->
+static int vdcm_idxd_set_msix_trigger(struct vdcm_idxd *vidxd,
+ unsigned int index, unsigned int start,
+ unsigned int count, uint32_t flags,
+ void *data)
+{
+ int i, rc = 0;
+
+ if (count > VIDXD_MAX_MSIX_ENTRIES - 1)
+ count = VIDXD_MAX_MSIX_ENTRIES - 1;
+
+ if (count == 0 && (flags & VFIO_IRQ_SET_DATA_NONE)) {
+ /* Disable all MSIX entries */
+ for (i = 0; i < VIDXD_MAX_MSIX_ENTRIES; i++) {
+ rc = msix_trigger_unregister(vidxd, i);
+ if (rc < 0)
+ return rc;
+ }
+ return 0;
+ }
+
+ for (i = 0; i < count; i++) {
+ if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+ u32 fd = *(u32 *)(data + i * sizeof(u32));
+
+ rc = msix_trigger_register(vidxd, fd, i);
+ if (rc < 0)
+ return rc;
+ } else if (flags & VFIO_IRQ_SET_DATA_NONE) {
+ rc = msix_trigger_unregister(vidxd, i);
+ if (rc < 0)
+ return rc;
+ }
+ }
+ return rc;
+}
+
+static int idxd_vdcm_set_irqs(struct vdcm_idxd *vidxd, uint32_t flags,
+ unsigned int index, unsigned int start,
+ unsigned int count, void *data)
+{
+ int (*func)(struct vdcm_idxd *vidxd, unsigned int index,
+ unsigned int start, unsigned int count, uint32_t flags,
+ void *data) = NULL;
+ struct mdev_device *mdev = vidxd->vdev.mdev;
+ struct device *dev = mdev_dev(mdev);
+
+ switch (index) {
+ case VFIO_PCI_INTX_IRQ_INDEX:
+ dev_warn(dev, "intx interrupts not supported.\n");
+ break;
+ case VFIO_PCI_MSI_IRQ_INDEX:
+ dev_dbg(dev, "msi interrupt.\n");
+ switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
+ case VFIO_IRQ_SET_ACTION_MASK:
+ case VFIO_IRQ_SET_ACTION_UNMASK:
+ break;
+ case VFIO_IRQ_SET_ACTION_TRIGGER:
+ func = vdcm_idxd_set_msix_trigger;
This would be a good place to insert a common VFIO helper library to
take care of the MSI-X emulation for IMS.
Hi Jason,
So after looking at the code in vfio_pci_intrs.c, I agree that the
set_irqs code between VFIO_PCI and this driver can be made in common.
Given that Alex doesn't want a vfio_pci device embedded in the driver, I
think we'll need some sort of generic VFIO device that can be used from
the vfio_pci side and vfio_mdev side to pass down in order to have
common support library functions. Do you have any thoughts on how to do
this cleanly architecturally? Also, with vfio_pci common split [1] still
being worked on, do you think we can defer the work on making the
interrupt setup code common until the vfio_pci split work settles? Thanks!
[1]: https://lore.kernel.org/kvm/20210201162828.5938-1-mgurto...@nvidia.com/
RE: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
> From: Jason Gunthorpe
> Sent: Wednesday, February 17, 2021 5:33 AM
>
> On Tue, Feb 16, 2021 at 12:04:55PM -0700, Dave Jiang wrote:
>
> > > > + return remap_pfn_range(vma, vma->vm_start, pgoff, req_size,
> pg_prot);
> > > Nothing validated req_size - did you copy this from the Intel RDMA
> > > driver? It had a huge security bug just like this.
>
> > Thanks. Will add. Some of the code came from the Intel i915 mdev
> > driver.
>
> Please make sure it is fixed as well, the security bug is huge.
>
It's already been fixed 2yrs ago:
commit 51b00d8509dc69c98740da2ad07308b630d3eb7d
Author: Zhenyu Wang
Date: Fri Jan 11 13:58:53 2019 +0800
drm/i915/gvt: Fix mmap range check
This is to fix missed mmap range check on vGPU bar2 region
and only allow to map vGPU allocated GMADDR range, which means
user space should support sparse mmap to get proper offset for
mmap vGPU aperture. And this takes care of actual pgoff in mmap
request as original code always does from beginning of vGPU
aperture.
Fixes: 659643f7d814 ("drm/i915/gvt/kvmgt: add vfio/mdev support to KVMGT")
Cc: "Monroy, Rodrigo Axel"
Cc: "Orrala Contreras, Alfredo"
Cc: sta...@vger.kernel.org # v4.10+
Reviewed-by: Hang Yuan
Signed-off-by: Zhenyu Wang
Thanks
Kevin
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On Tue, Feb 16, 2021 at 12:04:55PM -0700, Dave Jiang wrote:
> > > + return remap_pfn_range(vma, vma->vm_start, pgoff, req_size, pg_prot);
> > Nothing validated req_size - did you copy this from the Intel RDMA
> > driver? It had a huge security bug just like this.
> Thanks. Will add. Some of the code came from the Intel i915 mdev
> driver.
Please make sure it is fixed as well, the security bug is huge.
> > > + unsigned int index, unsigned int start,
> > > + unsigned int count, void *data)
> > > +{
> > > + int (*func)(struct vdcm_idxd *vidxd, unsigned int index,
> > > + unsigned int start, unsigned int count, uint32_t flags,
> > > + void *data) = NULL;
> > > + struct mdev_device *mdev = vidxd->vdev.mdev;
> > > + struct device *dev = mdev_dev(mdev);
> > > +
> > > + switch (index) {
> > > + case VFIO_PCI_INTX_IRQ_INDEX:
> > > + dev_warn(dev, "intx interrupts not supported.\n");
> > > + break;
> > > + case VFIO_PCI_MSI_IRQ_INDEX:
> > > + dev_dbg(dev, "msi interrupt.\n");
> > > + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> > > + case VFIO_IRQ_SET_ACTION_MASK:
> > > + case VFIO_IRQ_SET_ACTION_UNMASK:
> > > + break;
> > > + case VFIO_IRQ_SET_ACTION_TRIGGER:
> > > + func = vdcm_idxd_set_msix_trigger;
> > This would be a good place to insert a common VFIO helper library to
> > take care of the MSI-X emulation for IMS.
>
> I took a look at the idxd version vs the VFIO version and they are somewhat
> different. Although the MSI and MSIX case can be squashed in the idxd driver
> code. I do think that the parent code block can be split out in VFIO code
> and made into a common helper function to deal with VFIO_DEVICE_SET_IRQS and
> I've done so.
Really it looks like the MSI emulation for a simple IMS device is just
mapping the MSI table to a certain irq_chip, this feels like it should
be substantially common code
> > > diff --git a/drivers/vfio/mdev/idxd/vdev.h b/drivers/vfio/mdev/idxd/vdev.h
> > > new file mode 100644
> > > index ..cc2ba6ccff7b
> > > +++ b/drivers/vfio/mdev/idxd/vdev.h
> > > @@ -0,0 +1,19 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/* Copyright(c) 2019,2020 Intel Corporation. All rights rsvd. */
> > > +
> > > +#ifndef _IDXD_VDEV_H_
> > > +#define _IDXD_VDEV_H_
> > > +
> > > +#include "mdev.h"
> > > +
> > > +int vidxd_mmio_read(struct vdcm_idxd *vidxd, u64 pos, void *buf,
> > > unsigned int size);
> > > +int vidxd_mmio_write(struct vdcm_idxd *vidxd, u64 pos, void *buf,
> > > unsigned int size);
> > > +int vidxd_cfg_read(struct vdcm_idxd *vidxd, unsigned int pos, void *buf,
> > > unsigned int count);
> > > +int vidxd_cfg_write(struct vdcm_idxd *vidxd, unsigned int pos, void
> > > *buf, unsigned int size);
> > > +void vidxd_mmio_init(struct vdcm_idxd *vidxd);
> > > +void vidxd_reset(struct vdcm_idxd *vidxd);
> > > +int vidxd_send_interrupt(struct ims_irq_entry *iie);
> > > +int vidxd_setup_ims_entries(struct vdcm_idxd *vidxd);
> > > +void vidxd_free_ims_entries(struct vdcm_idxd *vidxd);
> > Why are these functions special??
>
> I'm not sure I follow the intent of this question. The vidxd_* functions are
> split out to vdev.c because they are the emulation helper functions for the
> mdev. It seems reasonable to split them out from the mdev code to make it
> more manageable.
Why do they get their own mostly empty header file?
Jason
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On Tue, Feb 16, 2021 at 12:39:56PM -0800, Dan Williams wrote: > > >> +/* > > >> + * Check and see if the guest wants to map to the limited or > > >> unlimited portal. > > >> + * The driver will allow mapping to unlimited portal only if the > > >> the wq is a > > >> + * dedicated wq. Otherwise, it goes to limited. > > >> + */ > > >> +virt_portal = ((offset >> PAGE_SHIFT) & 0x3) == 1; > > >> +phys_portal = IDXD_PORTAL_LIMITED; > > >> +if (virt_portal == IDXD_PORTAL_UNLIMITED && wq_dedicated(wq)) > > >> +phys_portal = IDXD_PORTAL_UNLIMITED; > > >> + > > >> +/* We always map IMS portals to the guest */ > > >> +pgoff = (base + idxd_get_wq_portal_full_offset(wq->id, phys_portal, > > >> + IDXD_IRQ_IMS)) >> > > >> PAGE_SHIFT; > > >> +dev_dbg(dev, "mmap %lx %lx %lx %lx\n", vma->vm_start, pgoff, > > >> req_size, > > >> +pgprot_val(pg_prot)); > > >> +vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > >> +vma->vm_private_data = mdev; > > > What ensures the mdev pointer is valid strictly longer than the VMA? > > > This needs refcounting. > > > > Going to take a kref at open and then put_device at close. Does that > > sound reasonable or should I be calling get_device() in mmap() and then > > register a notifier for when vma is released? > > Where does this enabling ever look at vm_private_data again? So long as a PCI BAR page is mapped into a VMA the pci driver cannot be removed. Things must either wait until the fd (or at least all VMAs) are closed, or zap the VMAs before allowing the device driver to be removed. There should be some logic in this whole thing where the pci_driver destroys the mdevs which destroy the vfio's which wait for all the fds to be closed. There is enough going on in vfio_device_fops_release() that this might happen already, Dave needs to investigate and confirm the whole thing works as expected. Presumably there is no security issue with sharing these portal pages because I don't see a vma ops involved here to track when pages are freed up (ie the vm_private_data is dead code cargo-cult'd from someplace else) But this is all sufficiently tricky, and Intel has already had security bugs in their drivers here, that someone needs to audit it closely before it gets posted again. Jason
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On Tue, Feb 16, 2021 at 11:05 AM Dave Jiang wrote:
>
>
> On 2/10/2021 4:59 PM, Jason Gunthorpe wrote:
> > On Fri, Feb 05, 2021 at 01:53:24PM -0700, Dave Jiang wrote:
> >
> >> +static int check_vma(struct idxd_wq *wq, struct vm_area_struct *vma)
> >> {
> >> -/* FIXME: Fill in later */
> >> +if (vma->vm_end < vma->vm_start)
> >> +return -EINVAL;
> > These checks are redundant
>
> Thanks. Will remove.
>
> >
> >> -static int idxd_mdev_host_release(struct idxd_device *idxd)
> >> +static int idxd_vdcm_mmap(struct mdev_device *mdev, struct vm_area_struct
> >> *vma)
> >> +{
> >> +unsigned int wq_idx, rc;
> >> +unsigned long req_size, pgoff = 0, offset;
> >> +pgprot_t pg_prot;
> >> +struct vdcm_idxd *vidxd = mdev_get_drvdata(mdev);
> >> +struct idxd_wq *wq = vidxd->wq;
> >> +struct idxd_device *idxd = vidxd->idxd;
> >> +enum idxd_portal_prot virt_portal, phys_portal;
> >> +phys_addr_t base = pci_resource_start(idxd->pdev, IDXD_WQ_BAR);
> >> +struct device *dev = mdev_dev(mdev);
> >> +
> >> +rc = check_vma(wq, vma);
> >> +if (rc)
> >> +return rc;
> >> +
> >> +pg_prot = vma->vm_page_prot;
> >> +req_size = vma->vm_end - vma->vm_start;
> >> +vma->vm_flags |= VM_DONTCOPY;
> >> +
> >> +offset = (vma->vm_pgoff << PAGE_SHIFT) &
> >> + ((1ULL << VFIO_PCI_OFFSET_SHIFT) - 1);
> >> +
> >> +wq_idx = offset >> (PAGE_SHIFT + 2);
> >> +if (wq_idx >= 1) {
> >> +dev_err(dev, "mapping invalid wq %d off %lx\n",
> >> +wq_idx, offset);
> >> +return -EINVAL;
> >> +}
> >> +
> >> +/*
> >> + * Check and see if the guest wants to map to the limited or
> >> unlimited portal.
> >> + * The driver will allow mapping to unlimited portal only if the the
> >> wq is a
> >> + * dedicated wq. Otherwise, it goes to limited.
> >> + */
> >> +virt_portal = ((offset >> PAGE_SHIFT) & 0x3) == 1;
> >> +phys_portal = IDXD_PORTAL_LIMITED;
> >> +if (virt_portal == IDXD_PORTAL_UNLIMITED && wq_dedicated(wq))
> >> +phys_portal = IDXD_PORTAL_UNLIMITED;
> >> +
> >> +/* We always map IMS portals to the guest */
> >> +pgoff = (base + idxd_get_wq_portal_full_offset(wq->id, phys_portal,
> >> + IDXD_IRQ_IMS)) >>
> >> PAGE_SHIFT;
> >> +dev_dbg(dev, "mmap %lx %lx %lx %lx\n", vma->vm_start, pgoff, req_size,
> >> +pgprot_val(pg_prot));
> >> +vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> >> +vma->vm_private_data = mdev;
> > What ensures the mdev pointer is valid strictly longer than the VMA?
> > This needs refcounting.
>
> Going to take a kref at open and then put_device at close. Does that
> sound reasonable or should I be calling get_device() in mmap() and then
> register a notifier for when vma is released?
Where does this enabling ever look at vm_private_data again? It seems
to me it should be reasonable for the mdev to die out from underneath
a vma, just need some tracking to block future uses of the
vma->vm_private_data from being attempted.
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On 2/10/2021 4:59 PM, Jason Gunthorpe wrote:
On Fri, Feb 05, 2021 at 01:53:24PM -0700, Dave Jiang wrote:
+static int check_vma(struct idxd_wq *wq, struct vm_area_struct *vma)
{
- /* FIXME: Fill in later */
+ if (vma->vm_end < vma->vm_start)
+ return -EINVAL;
These checks are redundant
Thanks. Will remove.
-static int idxd_mdev_host_release(struct idxd_device *idxd)
+static int idxd_vdcm_mmap(struct mdev_device *mdev, struct vm_area_struct *vma)
+{
+ unsigned int wq_idx, rc;
+ unsigned long req_size, pgoff = 0, offset;
+ pgprot_t pg_prot;
+ struct vdcm_idxd *vidxd = mdev_get_drvdata(mdev);
+ struct idxd_wq *wq = vidxd->wq;
+ struct idxd_device *idxd = vidxd->idxd;
+ enum idxd_portal_prot virt_portal, phys_portal;
+ phys_addr_t base = pci_resource_start(idxd->pdev, IDXD_WQ_BAR);
+ struct device *dev = mdev_dev(mdev);
+
+ rc = check_vma(wq, vma);
+ if (rc)
+ return rc;
+
+ pg_prot = vma->vm_page_prot;
+ req_size = vma->vm_end - vma->vm_start;
+ vma->vm_flags |= VM_DONTCOPY;
+
+ offset = (vma->vm_pgoff << PAGE_SHIFT) &
+((1ULL << VFIO_PCI_OFFSET_SHIFT) - 1);
+
+ wq_idx = offset >> (PAGE_SHIFT + 2);
+ if (wq_idx >= 1) {
+ dev_err(dev, "mapping invalid wq %d off %lx\n",
+ wq_idx, offset);
+ return -EINVAL;
+ }
+
+ /*
+* Check and see if the guest wants to map to the limited or unlimited
portal.
+* The driver will allow mapping to unlimited portal only if the the wq
is a
+* dedicated wq. Otherwise, it goes to limited.
+*/
+ virt_portal = ((offset >> PAGE_SHIFT) & 0x3) == 1;
+ phys_portal = IDXD_PORTAL_LIMITED;
+ if (virt_portal == IDXD_PORTAL_UNLIMITED && wq_dedicated(wq))
+ phys_portal = IDXD_PORTAL_UNLIMITED;
+
+ /* We always map IMS portals to the guest */
+ pgoff = (base + idxd_get_wq_portal_full_offset(wq->id, phys_portal,
+ IDXD_IRQ_IMS)) >>
PAGE_SHIFT;
+ dev_dbg(dev, "mmap %lx %lx %lx %lx\n", vma->vm_start, pgoff, req_size,
+ pgprot_val(pg_prot));
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+ vma->vm_private_data = mdev;
What ensures the mdev pointer is valid strictly longer than the VMA?
This needs refcounting.
Going to take a kref at open and then put_device at close. Does that
sound reasonable or should I be calling get_device() in mmap() and then
register a notifier for when vma is released?
+ vma->vm_pgoff = pgoff;
+
+ return remap_pfn_range(vma, vma->vm_start, pgoff, req_size, pg_prot);
Nothing validated req_size - did you copy this from the Intel RDMA
driver? It had a huge security bug just like this.
Thanks. Will add. Some of the code came from the Intel i915 mdev driver.
+
+static int msix_trigger_unregister(struct vdcm_idxd *vidxd, int index)
+{
+ struct mdev_device *mdev = vidxd->vdev.mdev;
+ struct device *dev = mdev_dev(mdev);
+ struct ims_irq_entry *irq_entry;
+ int rc;
+
+ if (!vidxd->vdev.msix_trigger[index])
+ return 0;
+
+ dev_dbg(dev, "disable MSIX trigger %d\n", index);
+ if (index) {
+ u32 auxval;
+
+ irq_entry = >irq_entries[index];
+ if (irq_entry->irq_set) {
+ free_irq(irq_entry->irq, irq_entry);
+ irq_entry->irq_set = false;
+ }
+
+ auxval = ims_ctrl_pasid_aux(0, false);
+ rc = irq_set_auxdata(irq_entry->irq, IMS_AUXDATA_CONTROL_WORD,
auxval);
+ if (rc)
+ return rc;
+ }
+ eventfd_ctx_put(vidxd->vdev.msix_trigger[index]);
+ vidxd->vdev.msix_trigger[index] = NULL;
+
+ return 0;
+}
+
+static int msix_trigger_register(struct vdcm_idxd *vidxd, u32 fd, int index)
+{
+ struct mdev_device *mdev = vidxd->vdev.mdev;
+ struct device *dev = mdev_dev(mdev);
+ struct ims_irq_entry *irq_entry;
+ struct eventfd_ctx *trigger;
+ int rc;
+
+ if (vidxd->vdev.msix_trigger[index])
+ return 0;
+
+ dev_dbg(dev, "enable MSIX trigger %d\n", index);
+ trigger = eventfd_ctx_fdget(fd);
+ if (IS_ERR(trigger)) {
+ dev_warn(dev, "eventfd_ctx_fdget failed %d\n", index);
+ return PTR_ERR(trigger);
+ }
+
+ if (index) {
+ u32 pasid;
+ u32 auxval;
+
+ irq_entry = >irq_entries[index];
+ rc = idxd_mdev_get_pasid(mdev, );
+ if (rc < 0)
+ return rc;
+
+ /*
+* Program and enable the pasid field in the IMS entry. The
programmed pasid and
+* enabled field is checked against the pasid and enable field
for the
Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
On Fri, Feb 05, 2021 at 01:53:24PM -0700, Dave Jiang wrote:
> +static int check_vma(struct idxd_wq *wq, struct vm_area_struct *vma)
> {
> - /* FIXME: Fill in later */
> + if (vma->vm_end < vma->vm_start)
> + return -EINVAL;
These checks are redundant
> -static int idxd_mdev_host_release(struct idxd_device *idxd)
> +static int idxd_vdcm_mmap(struct mdev_device *mdev, struct vm_area_struct
> *vma)
> +{
> + unsigned int wq_idx, rc;
> + unsigned long req_size, pgoff = 0, offset;
> + pgprot_t pg_prot;
> + struct vdcm_idxd *vidxd = mdev_get_drvdata(mdev);
> + struct idxd_wq *wq = vidxd->wq;
> + struct idxd_device *idxd = vidxd->idxd;
> + enum idxd_portal_prot virt_portal, phys_portal;
> + phys_addr_t base = pci_resource_start(idxd->pdev, IDXD_WQ_BAR);
> + struct device *dev = mdev_dev(mdev);
> +
> + rc = check_vma(wq, vma);
> + if (rc)
> + return rc;
> +
> + pg_prot = vma->vm_page_prot;
> + req_size = vma->vm_end - vma->vm_start;
> + vma->vm_flags |= VM_DONTCOPY;
> +
> + offset = (vma->vm_pgoff << PAGE_SHIFT) &
> + ((1ULL << VFIO_PCI_OFFSET_SHIFT) - 1);
> +
> + wq_idx = offset >> (PAGE_SHIFT + 2);
> + if (wq_idx >= 1) {
> + dev_err(dev, "mapping invalid wq %d off %lx\n",
> + wq_idx, offset);
> + return -EINVAL;
> + }
> +
> + /*
> + * Check and see if the guest wants to map to the limited or unlimited
> portal.
> + * The driver will allow mapping to unlimited portal only if the the wq
> is a
> + * dedicated wq. Otherwise, it goes to limited.
> + */
> + virt_portal = ((offset >> PAGE_SHIFT) & 0x3) == 1;
> + phys_portal = IDXD_PORTAL_LIMITED;
> + if (virt_portal == IDXD_PORTAL_UNLIMITED && wq_dedicated(wq))
> + phys_portal = IDXD_PORTAL_UNLIMITED;
> +
> + /* We always map IMS portals to the guest */
> + pgoff = (base + idxd_get_wq_portal_full_offset(wq->id, phys_portal,
> +IDXD_IRQ_IMS)) >>
> PAGE_SHIFT;
> + dev_dbg(dev, "mmap %lx %lx %lx %lx\n", vma->vm_start, pgoff, req_size,
> + pgprot_val(pg_prot));
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + vma->vm_private_data = mdev;
What ensures the mdev pointer is valid strictly longer than the VMA?
This needs refcounting.
> + vma->vm_pgoff = pgoff;
> +
> + return remap_pfn_range(vma, vma->vm_start, pgoff, req_size, pg_prot);
Nothing validated req_size - did you copy this from the Intel RDMA
driver? It had a huge security bug just like this.
> +
> +static int msix_trigger_unregister(struct vdcm_idxd *vidxd, int index)
> +{
> + struct mdev_device *mdev = vidxd->vdev.mdev;
> + struct device *dev = mdev_dev(mdev);
> + struct ims_irq_entry *irq_entry;
> + int rc;
> +
> + if (!vidxd->vdev.msix_trigger[index])
> + return 0;
> +
> + dev_dbg(dev, "disable MSIX trigger %d\n", index);
> + if (index) {
> + u32 auxval;
> +
> + irq_entry = >irq_entries[index];
> + if (irq_entry->irq_set) {
> + free_irq(irq_entry->irq, irq_entry);
> + irq_entry->irq_set = false;
> + }
> +
> + auxval = ims_ctrl_pasid_aux(0, false);
> + rc = irq_set_auxdata(irq_entry->irq, IMS_AUXDATA_CONTROL_WORD,
> auxval);
> + if (rc)
> + return rc;
> + }
> + eventfd_ctx_put(vidxd->vdev.msix_trigger[index]);
> + vidxd->vdev.msix_trigger[index] = NULL;
> +
> + return 0;
> +}
> +
> +static int msix_trigger_register(struct vdcm_idxd *vidxd, u32 fd, int index)
> +{
> + struct mdev_device *mdev = vidxd->vdev.mdev;
> + struct device *dev = mdev_dev(mdev);
> + struct ims_irq_entry *irq_entry;
> + struct eventfd_ctx *trigger;
> + int rc;
> +
> + if (vidxd->vdev.msix_trigger[index])
> + return 0;
> +
> + dev_dbg(dev, "enable MSIX trigger %d\n", index);
> + trigger = eventfd_ctx_fdget(fd);
> + if (IS_ERR(trigger)) {
> + dev_warn(dev, "eventfd_ctx_fdget failed %d\n", index);
> + return PTR_ERR(trigger);
> + }
> +
> + if (index) {
> + u32 pasid;
> + u32 auxval;
> +
> + irq_entry = >irq_entries[index];
> + rc = idxd_mdev_get_pasid(mdev, );
> + if (rc < 0)
> + return rc;
> +
> + /*
> + * Program and enable the pasid field in the IMS entry. The
> programmed pasid and
> + * enabled field is checked against the pasid and enable field
> for the work queue
> + * configuration and the pasid for the descriptor. A mismatch
> will result in blocked
> + * IMS interrupt.
> + */
> + auxval = ims_ctrl_pasid_aux(pasid, true);
> + rc =
[PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions
Create a mediated device through the VFIO mediated device framework. The
mdev framework allows creation of an mediated device by the driver with
portion of the device's resources. The driver will emulate the slow path
such as the PCI config space, MMIO bar, and the command registers. The
descriptor submission portal(s) will be mmaped to the guest in order to
submit descriptors directly by the guest kernel or apps. The mediated
device support code in the idxd will be referred to as the Virtual
Device Composition Module (vdcm). Add basic plumbing to fill out the
mdev_parent_ops struct that VFIO mdev requires to support a mediated
device.
Signed-off-by: Dave Jiang
---
drivers/dma/idxd/device.c |1
drivers/dma/idxd/idxd.h |7
drivers/dma/idxd/init.c |2
drivers/vfio/mdev/idxd/Makefile |2
drivers/vfio/mdev/idxd/mdev.c | 1006 +++
drivers/vfio/mdev/idxd/mdev.h | 115
drivers/vfio/mdev/idxd/vdev.c | 75 +++
drivers/vfio/mdev/idxd/vdev.h | 19 +
8 files changed, 1218 insertions(+), 9 deletions(-)
create mode 100644 drivers/vfio/mdev/idxd/mdev.h
create mode 100644 drivers/vfio/mdev/idxd/vdev.c
create mode 100644 drivers/vfio/mdev/idxd/vdev.h
diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 2491b27c8125..89fa2bbe6ebf 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -265,6 +265,7 @@ int idxd_wq_disable(struct idxd_wq *wq)
dev_dbg(dev, "WQ %d disabled\n", wq->id);
return 0;
}
+EXPORT_SYMBOL_GPL(idxd_wq_disable);
void idxd_wq_drain(struct idxd_wq *wq)
{
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index f02c96164515..a271942df2be 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -133,6 +133,7 @@ struct idxd_wq {
u64 max_xfer_bytes;
u32 max_batch_size;
bool ats_dis;
+ struct list_head vdcm_list;
};
struct idxd_engine {
@@ -165,6 +166,7 @@ enum idxd_device_flag {
IDXD_FLAG_CMD_RUNNING,
IDXD_FLAG_PASID_ENABLED,
IDXD_FLAG_IMS_SUPPORTED,
+ IDXD_FLAG_MDEV_ENABLED,
};
struct idxd_device {
@@ -275,6 +277,11 @@ static inline bool device_swq_supported(struct idxd_device
*idxd)
return (support_enqcmd && device_pasid_enabled(idxd));
}
+static inline bool device_mdev_enabled(struct idxd_device *idxd)
+{
+ return test_bit(IDXD_FLAG_MDEV_ENABLED, >flags);
+}
+
enum idxd_portal_prot {
IDXD_PORTAL_UNLIMITED = 0,
IDXD_PORTAL_LIMITED,
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index fd57f39e4b7d..cc3b757d300f 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -215,7 +215,6 @@ static int idxd_setup_internals(struct idxd_device *idxd)
for (i = 0; i < idxd->max_wqs; i++) {
struct idxd_wq *wq = >wqs[i];
- int rc;
wq->id = i;
wq->idxd = idxd;
@@ -227,6 +226,7 @@ static int idxd_setup_internals(struct idxd_device *idxd)
if (!wq->wqcfg)
return -ENOMEM;
init_completion(>wq_dead);
+ INIT_LIST_HEAD(>vdcm_list);
}
for (i = 0; i < idxd->max_engines; i++) {
diff --git a/drivers/vfio/mdev/idxd/Makefile b/drivers/vfio/mdev/idxd/Makefile
index e8f45cb96117..27a08621d120 100644
--- a/drivers/vfio/mdev/idxd/Makefile
+++ b/drivers/vfio/mdev/idxd/Makefile
@@ -1,4 +1,4 @@
ccflags-y += -I$(srctree)/drivers/dma/idxd -DDEFAULT_SYMBOL_NAMESPACE=IDXD
obj-$(CONFIG_VFIO_MDEV_IDXD) += idxd_mdev.o
-idxd_mdev-y := mdev.o
+idxd_mdev-y := mdev.o vdev.o
diff --git a/drivers/vfio/mdev/idxd/mdev.c b/drivers/vfio/mdev/idxd/mdev.c
index 8b9a6adeb606..384ba5d6bc2b 100644
--- a/drivers/vfio/mdev/idxd/mdev.c
+++ b/drivers/vfio/mdev/idxd/mdev.c
@@ -1,27 +1,1017 @@
// SPDX-License-Identifier: GPL-2.0
-/* Copyright(c) 2020 Intel Corporation. All rights rsvd. */
+/* Copyright(c) 2019,2020 Intel Corporation. All rights rsvd. */
#include
#include
#include
#include
#include
-#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
#include
#include "registers.h"
#include "idxd.h"
+#include "../../vfio/pci/vfio_pci_private.h"
+#include "mdev.h"
+#include "vdev.h"
-static int idxd_mdev_host_init(struct idxd_device *idxd)
+static u64 idxd_pci_config[] = {
+ 0x00108086ULL,
+ 0x00800880ULL,
+ 0x000cULL,
+ 0x000cULL,
+ 0xULL,
+ 0x20108086ULL,
+ 0x0040ULL,
+ 0x00ffULL,
+ 0x06015011ULL, /* MSI-X capability, hardcoded 2 entries,
Encoded as N-1 */
+ 0x0700ULL,
+ 0x00920010ULL, /* PCIe capability */
+ 0xULL,
+ 0xULL,
+ 0xULL,
+
