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;
From:   "Liu, Jing2" <jing2.liu@intel.com>
To:     Alex Williamson <alex.williamson@redhat.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
Thread-Topic: [PATCH V2 7/8] vfio/pci: Support dynamic MSI-x
Thread-Index: AQHZYb/Rk3to+fQWNUmPjc7xA3v1Mq8UqOiQgABDWQCABYmzAA==
Date:   Tue, 4 Apr 2023 03:19:53 +0000
Message-ID: <MWHPR11MB124531FF4E6A4A2293D292D3A9939@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>
 <20230331075122.736bdb98.alex.williamson@redhat.com>
In-Reply-To: <20230331075122.736bdb98.alex.williamson@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?FXoyFnbiatkVwglOT0atBFhrGVIkOH7Xd3li7pbIvtgeHxbJKWpxOemQs4QT?=
 =?us-ascii?Q?T5Ew4dBuxB72BMdxBqKMBw84BigbmtCcTPRDnMbJhOkYyNwwBQFjxc4rONUR?=
 =?us-ascii?Q?8K54MmSunbQypUxD4Niy7PvXa6heNBrbD229m/r6KpBfefyQglDaxhwIFPzX?=
 =?us-ascii?Q?W7lUX0tmlEIR4dj9tGqyIP0LeOoBnmbaus05WY86wrwfKddme3XokXkI3fMC?=
 =?us-ascii?Q?RFPdXCu4bv2TgoYiMYnLSkd6dOS+vln3N3YFp/Yb8fdQ5/JjOsVCm3DmiPZA?=
 =?us-ascii?Q?SUvwiNgtUHqeMo8ynYSPvWFxahPALMDE/ihodQth8oKkSQA+duCalK5Jud5y?=
 =?us-ascii?Q?seNLJaZwMITdLLZBva+P7DO6LtaqaSjhzfInkQBhBRIpHTZQfFdWRUUSNqJR?=
 =?us-ascii?Q?ck+2UugbPWmtBf4QzSrXDdLhtAAlmddHUW+zTsnwaez29DCa0sbFYGGmzNCw?=
 =?us-ascii?Q?6sn40jGXkZhg9dJNq8Osdsrl0SaXILSIwqrmGh5vdeXaI9APayLp9koOV1AF?=
 =?us-ascii?Q?dboczDtdWE9H52hr6ZLNMsC2pJU0zXnDKUxzc3SCQIiLahSEGvduXgK6D9jo?=
 =?us-ascii?Q?rMwyvpVtyJLEwM58kCNGG/DZrBBVFwpNlcSiuJigGhAPw1rvUYf0HQ/kJyFS?=
 =?us-ascii?Q?gUyb5Bx5tLKeCUs0ZfUElZ3440BFJ0qUn+6kCxQbsC9GJdwCrW1uzQrmAqXp?=
 =?us-ascii?Q?v8KNBTcxtyN8W0KVWIScjh/7WKwyf8vFvHFJIxmPAW9jpFNH0y1RtdFiohZM?=
 =?us-ascii?Q?YDJ+52l/fQ6sqbPIx6ZISmQ84GHd0kArfYYmCMzYoYlXKIY7huWGpFeN4mbh?=
 =?us-ascii?Q?roAszvIjqb2zE6si78U+iXjaR+cFNvFfuftHBAk8K0m/Pb8/O1L0pgPdWYaV?=
 =?us-ascii?Q?mfbg6x+kL5bFuJbK9AVlSl6Diy1aZYuiemZkV8dltceKRUgOAWSh27/KKveP?=
 =?us-ascii?Q?FJsObxqBZEVLWqFyJ+e0sWpQaeKqE45WtSM6wFQyluGOXdpAbo08GKEZPT4A?=
 =?us-ascii?Q?3h0d1T2TipW8WhX3gNPz+d3z+A7ZZ9i/I77ee3kkMngO4pxHuwLe0R1ZnOZK?=
 =?us-ascii?Q?ck5ai57o/0BkPERtDUvvCqLF+9SewLf53PJAMYrQhzUQCLQ0EJTl4bak6yVF?=
 =?us-ascii?Q?xbJYz3BcmQTxrTDJtAa84rPJq4T6B7ut+SY3eHkZKJiiHZ39cL6GGHtc6LKo?=
 =?us-ascii?Q?2xYOdtI6QcBx+2v06wbAOMuyQ6H9MzlsKDfe8Gnm61NX62IxoWTM6S1AL1bL?=
 =?us-ascii?Q?RnsZCLi8Gi1mhCWDSiOE2AG7RgGPkUYdN+89p0uWFwO/CtXsCjJeXWfZCuAR?=
 =?us-ascii?Q?ztX7jVJWa24OIH5PmB3ZJ062Oo7czbJIC+KTHdZ4g8XZt900sYBh9nHmgdKc?=
 =?us-ascii?Q?KxC9Rh2shIQyC/XhUqmsD1W5drMVbuaDIf0S6OmEnIT4V/f8pYNvIV3vWM/E?=
 =?us-ascii?Q?I8Xlf3ZqGT76zjCHgSQ6p7sahEv2RTbsZOggzhaJI27zaDlIe7OEdzy/CwDF?=
 =?us-ascii?Q?Ggya5suQaEz8lHpKM2RzjrJcEpkdbJeIGLHV6IEvyUNvHJjdORubsjrwmrM1?=
 =?us-ascii?Q?OL8Q5BX5eSqoOLGDJH8jj4ISLU5QeanzWpUcLhmk?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB1245.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1be6c5e6-77aa-4a37-f359-08db34bb7567
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Apr 2023 03:19:53.6206
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: X7Fu3zTdolxSbu64WRisHBcL0ZSgUosXm2CEgiSI+o2gqFt1RHKbUezbNUkISr+8myjJxVwQEbBPHvNpVrI14g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5225
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=unavailable
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> From: Alex Williamson <alex.williamson@redhat.com>
> On Fri, 31 Mar 2023 10:02:32 +0000
> "Liu, Jing2" <jing2.liu@intel.com> wrote:
>=20
> > Hi Reinette,
> >
> > > @@ -409,33 +416,62 @@ static int vfio_msi_set_vector_signal(struct
> > > vfio_pci_core_device *vdev,  {
> > >  	struct pci_dev *pdev =3D vdev->pdev;
> > >  	struct vfio_pci_irq_ctx *ctx;
> > > +	struct msi_map msix_map =3D {};
> > > +	bool allow_dyn_alloc =3D false;
> > >  	struct eventfd_ctx *trigger;
> > > +	bool new_ctx =3D false;
> > >  	int irq, ret;
> > >  	u16 cmd;
> > >
> > > +	/* Only MSI-X allows dynamic allocation. */
> > > +	if (msix && pci_msix_can_alloc_dyn(vdev->pdev))
> > > +		allow_dyn_alloc =3D true;
> > > +
> > >  	ctx =3D vfio_irq_ctx_get(vdev, vector);
> > > -	if (!ctx)
> > > +	if (!ctx && !allow_dyn_alloc)
> > >  		return -EINVAL;
> > > +
> > >  	irq =3D pci_irq_vector(pdev, vector);
> > > +	/* Context and interrupt are always allocated together. */
> > > +	WARN_ON((ctx && irq =3D=3D -EINVAL) || (!ctx && irq !=3D -EINVAL));
> > >
> > > -	if (ctx->trigger) {
> > > +	if (ctx && ctx->trigger) {
> > >  		irq_bypass_unregister_producer(&ctx->producer);
> > >
> > >  		cmd =3D vfio_pci_memory_lock_and_enable(vdev);
> > >  		free_irq(irq, ctx->trigger);
> > > +		if (allow_dyn_alloc) {
> > > +			msix_map.index =3D vector;
> > > +			msix_map.virq =3D irq;
> > > +			pci_msix_free_irq(pdev, msix_map);
> > > +			irq =3D -EINVAL;
> > > +		}
> > >  		vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > >  		kfree(ctx->name);
> > >  		eventfd_ctx_put(ctx->trigger);
> > >  		ctx->trigger =3D NULL;
> > > +		if (allow_dyn_alloc) {
> > > +			vfio_irq_ctx_free(vdev, ctx, vector);
> > > +			ctx =3D 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 s=
ame 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?
>=20
> 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,
>=20
> Alex

Thank you very much for your answer.=20

The reason is I see Qemu has such behaviors sending the same valid fd for a=
=20
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