Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 31 Mar 2023 07:51:22 -0600
From:   Alex Williamson <alex.williamson@redhat.com>
To:     "Liu, Jing2" <jing2.liu@intel.com>
Cc:     "Chatre, Reinette" <reinette.chatre@intel.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "yishaih@nvidia.com" <yishaih@nvidia.com>,
        "shameerali.kolothum.thodi@huawei.com" 
        <shameerali.kolothum.thodi@huawei.com>,
        "Tian, Kevin" <kevin.tian@intel.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "darwi@linutronix.de" <darwi@linutronix.de>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        "Raj, Ashok" <ashok.raj@intel.com>,
        "Yu, Fenghua" <fenghua.yu@intel.com>,
        "tom.zanussi@linux.intel.com" <tom.zanussi@linux.intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2 7/8] vfio/pci: Support dynamic MSI-x
Message-ID: <20230331075122.736bdb98.alex.williamson@redhat.com>
In-Reply-To: <MWHPR11MB12456636D269F997D1812FF8A98F9@MWHPR11MB1245.namprd11.prod.outlook.com>
References: <cover.1680038771.git.reinette.chatre@intel.com>
        <419f3ba2f732154d8ae079b3deb02d0fdbe3e258.1680038771.git.reinette.chatre@intel.com>
        <MWHPR11MB12456636D269F997D1812FF8A98F9@MWHPR11MB1245.namprd11.prod.outlook.com>
Organization: Red Hat
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 31 Mar 2023 10:02:32 +0000
"Liu, Jing2" <jing2.liu@intel.com> 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