RE: [PATCH V2 7/8] vfio/pci: Support dynamic MSI-x

[Liu, Jing2]

> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> On Fri, 31 Mar 2023 10:02:32 +0000
> "Liu, Jing2" <jing2.liu@xxxxxxxxx> wrote:
> 
> > Hi Reinette,
> >
> > > @@ -409,33 +416,62 @@ static int vfio_msi_set_vector_signal(struct
> > > vfio_pci_core_device *vdev,  {
> > >  	struct pci_dev *pdev = vdev->pdev;
> > >  	struct vfio_pci_irq_ctx *ctx;
> > > +	struct msi_map msix_map = {};
> > > +	bool allow_dyn_alloc = false;
> > >  	struct eventfd_ctx *trigger;
> > > +	bool new_ctx = false;
> > >  	int irq, ret;
> > >  	u16 cmd;
> > >
> > > +	/* Only MSI-X allows dynamic allocation. */
> > > +	if (msix && pci_msix_can_alloc_dyn(vdev->pdev))
> > > +		allow_dyn_alloc = true;
> > > +
> > >  	ctx = vfio_irq_ctx_get(vdev, vector);
> > > -	if (!ctx)
> > > +	if (!ctx && !allow_dyn_alloc)
> > >  		return -EINVAL;
> > > +
> > >  	irq = pci_irq_vector(pdev, vector);
> > > +	/* Context and interrupt are always allocated together. */
> > > +	WARN_ON((ctx && irq == -EINVAL) || (!ctx && irq != -EINVAL));
> > >
> > > -	if (ctx->trigger) {
> > > +	if (ctx && ctx->trigger) {
> > >  		irq_bypass_unregister_producer(&ctx->producer);
> > >
> > >  		cmd = vfio_pci_memory_lock_and_enable(vdev);
> > >  		free_irq(irq, ctx->trigger);
> > > +		if (allow_dyn_alloc) {
> > > +			msix_map.index = vector;
> > > +			msix_map.virq = irq;
> > > +			pci_msix_free_irq(pdev, msix_map);
> > > +			irq = -EINVAL;
> > > +		}
> > >  		vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > >  		kfree(ctx->name);
> > >  		eventfd_ctx_put(ctx->trigger);
> > >  		ctx->trigger = NULL;
> > > +		if (allow_dyn_alloc) {
> > > +			vfio_irq_ctx_free(vdev, ctx, vector);
> > > +			ctx = NULL;
> > > +		}
> > >  	}
> > >
> > >  	if (fd < 0)
> > >  		return 0;
> > >
> >
> > While looking at this piece of code, one thought comes to me:
> > It might be possible that userspace comes twice with the same valid fd
> > for a specific vector, this vfio code will free the resource in if(ctx
> > && ctx->trigger) for the second time, and then re-alloc again for the same fd
> given by userspace.
> >
> > Would that help if we firstly check e.g. ctx->trigger with the given
> > valid fd, to see if the trigger is really changed or not?
> 
> It's rather a made-up situation, if userspace wants to avoid bouncing the vector
> when the eventfd hasn't changed they can simply test this before calling the ioctl.
> Thanks,
> 
> Alex

Thank you very much for your answer. 

The reason is I see Qemu has such behaviors sending the same valid fd for a 
specific vector twice, when it wants to disable the MSIx (switching fd from kvm bypass
to non-bypass).

So the right consideration is, userspace should be responsible for avoiding doing the
same thing several times, if it doesn't want that.

Thanks,
Jing