Re: [PATCH v5 05/14] vfio/mdev: idxd: add basic mdev registration and helper functions

[Jason Gunthorpe]

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 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.

Why do they get their own mostly empty header file?

Jason