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)These checks are redundant
{
- /* FIXME: Fill in later */
+ if (vma->vm_end < vma->vm_start)
+ return -EINVAL;
-static int idxd_mdev_host_release(struct idxd_device *idxd)What ensures the mdev pointer is valid strictly longer than the VMA?
+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;
This needs refcounting.
Thanks. Will add. Some of the code came from the Intel i915 mdev driver.
+ vma->vm_pgoff = pgoff;Nothing validated req_size - did you copy this from the Intel RDMA
+
+ return remap_pfn_range(vma, vma->vm_start, pgoff, req_size, pg_prot);
driver? It had a huge security bug just like this.
+This would be a good place to insert a common VFIO helper library to
+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 = &vidxd->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 = &vidxd->irq_entries[index];
+ rc = idxd_mdev_get_pasid(mdev, &pasid);
+ 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 = irq_set_auxdata(irq_entry->irq, IMS_AUXDATA_CONTROL_WORD, auxval);
+ if (rc < 0)
+ return rc;
+
+ rc = request_irq(irq_entry->irq, idxd_guest_wq_completion, 0, "idxd-ims",
+ irq_entry);
+ if (rc) {
+ dev_warn(dev, "failed to request ims irq\n");
+ eventfd_ctx_put(trigger);
+ auxval = ims_ctrl_pasid_aux(0, false);
+ irq_set_auxdata(irq_entry->irq, IMS_AUXDATA_CONTROL_WORD, auxval);
+ return rc;
+ }
+ irq_entry->irq_set = true;
+ }
+
+ vidxd->vdev.msix_trigger[index] = trigger;
+ return 0;
+}
+
+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;
take care of the MSI-X emulation for IMS.
+int idxd_mdev_host_init(struct idxd_device *idxd)Huh. This is the first user of IOMMU_DEV_FEAT_AUX, why has so much
+{
+ struct device *dev = &idxd->pdev->dev;
+ int rc;
+
+ if (!test_bit(IDXD_FLAG_IMS_SUPPORTED, &idxd->flags))
+ return -EOPNOTSUPP;
+
+ if (iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)) {
+ rc = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_AUX);
dead-code infrastructure been already merged around this?
@@ -34,6 +1024,7 @@ static int idxd_mdev_aux_probe(struct auxiliary_device *auxdev,Something is being done wrong if this flag is needed
return rc;
}
+ set_bit(IDXD_FLAG_MDEV_ENABLED, &idxd->flags);
+int vidxd_send_interrupt(struct ims_irq_entry *iie)Here too, don't structure the patches like this
+{
+ /* PLACE HOLDER */
+ return 0;
+}
diff --git a/drivers/vfio/mdev/idxd/vdev.h b/drivers/vfio/mdev/idxd/vdev.hWhy are these functions special??
new file mode 100644
index 000000000000..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);
Jason