Date:   Tue, 16 Feb 2021 12:04:55 -0700
From:   Dave Jiang <dave.jiang@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     alex.williamson@...hat.com, kwankhede@...dia.com,
        tglx@...utronix.de, vkoul@...nel.org, megha.dey@...el.com,
        jacob.jun.pan@...el.com, ashok.raj@...el.com, yi.l.liu@...el.com,
        baolu.lu@...el.com, kevin.tian@...el.com, sanjay.k.kumar@...el.com,
        tony.luck@...el.com, dan.j.williams@...el.com,
        eric.auger@...hat.com, parav@...lanox.com, netanelg@...lanox.com,
        shahafs@...lanox.com, pbonzini@...hat.com,
        dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: 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 = &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;
> 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.


>> +int idxd_mdev_host_init(struct idxd_device *idxd)
>> +{
>> +	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);
> Huh. This is the first user of IOMMU_DEV_FEAT_AUX, why has so much
> dead-code infrastructure been already merged around this?
>
>
>> @@ -34,6 +1024,7 @@ static int idxd_mdev_aux_probe(struct auxiliary_device *auxdev,
>>   		return rc;
>>   	}
>>   
>> +	set_bit(IDXD_FLAG_MDEV_ENABLED, &idxd->flags);
> Something is being done wrong if this flag is needed

Will remove.


>
>> +int vidxd_send_interrupt(struct ims_irq_entry *iie)
>> +{
>> +	/* PLACE HOLDER */
>> +	return 0;
>> +}
> Here too, don't structure the patches like this

This is the unfortunately result of attempting to split the driver code 
into manageable patches from inherited code. Do you suggest I organize 
it such that I add the function definitions first so we don't deal with 
empty functions?

>
>> diff --git a/drivers/vfio/mdev/idxd/vdev.h b/drivers/vfio/mdev/idxd/vdev.h
>> 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);
> 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.


>
> Jason

Powered by blists - more mailing lists