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:   "Tian, Kevin" <kevin.tian@intel.com>
To:     "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>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>
CC:     "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>,
        "Liu, Jing2" <jing2.liu@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 V4 10/11] vfio/pci: Support dynamic MSI-X
Thread-Topic: [PATCH V4 10/11] vfio/pci: Support dynamic MSI-X
Thread-Index: AQHZeS7NjeBSeWLT50GaZQKxGXZCA69ARxSQ
Date:   Fri, 28 Apr 2023 06:50:01 +0000
Message-ID: <BN9PR11MB5276B67702AACB0B5BF1EC0A8C6B9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <cover.1682615447.git.reinette.chatre@intel.com>
 <c4c582970fbeaf4b6000845c400aa4c6b7bb2f13.1682615447.git.reinette.chatre@intel.com>
In-Reply-To: <c4c582970fbeaf4b6000845c400aa4c6b7bb2f13.1682615447.git.reinette.chatre@intel.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?4ONkRR2gxpkwrdOWzKUfljuLf+rNGxbMyhk04uARta4dNIPp3g+qyzYJVNJy?=
 =?us-ascii?Q?AEgCyGApfYJF5wrL+yZGD03Piww5ooYHzX3DeVyEdwNBDT32pU+VPoc+F3O2?=
 =?us-ascii?Q?qDF+x0HZXDEJhasrZz8mq7P612i5wSit5Zln1uwXRH15tNxOZ4gRGjsuvWDq?=
 =?us-ascii?Q?nU9HMi5wlbDlmHrVnMUmK8IMedisPkp2yyfCy/EHMazQMMffKsAKs+4V3h25?=
 =?us-ascii?Q?0yzxS8zNqgGe9qKL5Mn+JuuQ7JnBHj9IrCM+oWJ6rjOPOhbuvJ75F6YlwqhX?=
 =?us-ascii?Q?oif7mqhG0GRVHACkdjDtDj6IWsLBa6uuCkCnK9HSgqUCkwdRhRjgKuiVq5jV?=
 =?us-ascii?Q?7BBkORfew9KkXaOAPar0Op4SLMRMF1j4V60j72HtJNekFf+SNRB4Id+1A11g?=
 =?us-ascii?Q?ft8IJxLlGn68zWF0BVkcULCDMR5UGruElbxLl9BY5+idbwSdek7Cj1gaRyzy?=
 =?us-ascii?Q?rY2ExpCm+UsKYdlxN8glfyMKknWzdufhk/L0CyYTNuS/p9Opg4jCJiRjspYd?=
 =?us-ascii?Q?mWQZpE/Gu7CCExMM0e8cFZkHE3nRcRwX2s6EczMUGJxhHk4mxWUrlcnOrXAo?=
 =?us-ascii?Q?0zJK30m1/1U51JUIpPXGooMeLJsrqdV/NV5mFI332MT5xif3VPtkpmDzxY7m?=
 =?us-ascii?Q?JOlVL8OqWHsT0/lhlHBuByt/ha4HKMA/3VSgePyj/4MbOBf2ZvHtfOnB2iZX?=
 =?us-ascii?Q?/QBgiQ3IzkxbXE/5BfYH3Fg+7KwVehPmwzlknG7r7cK1uKkaicF5bcIZZv6d?=
 =?us-ascii?Q?I3wSL0dYSuYPA7To4nj+B0Eec59BERWTSVWKaPNdsiubNELXlwoiw/jpHMLB?=
 =?us-ascii?Q?S1FAsYnyF+rjKf3DRlzt4ghj+FE2M4jBgAh3UdOi6YSp5gLKkUzuBEYKfVfT?=
 =?us-ascii?Q?YPK6MyG2iPffnPvdWL8lnrqJ+RG9N+cr05lYLFDKRz46MrrdKPz6W8ui88Eb?=
 =?us-ascii?Q?Ff1POB/pxmHgF2HJp0i7cDlLOEDflLlgMkMA4HKRincHUlqD/TjP8anDhQIS?=
 =?us-ascii?Q?zY/zRBP2Qz2DZC5VVKJl7b2WsLXDgoLKxGmGEz1bMRXQxSAAKYoWxD093OHF?=
 =?us-ascii?Q?D/55dA2n9e3KzSg2IQg0A7hXCgqSAD1PRCF5CTrQ884cGQFljwQidc9HR+6Z?=
 =?us-ascii?Q?jMjyH93roAH6EIH47tMGQH18l77mhQmLS2REaj+46utUW0CZnc2FxfzTa51Y?=
 =?us-ascii?Q?mZl1KpTPzd7NHbiOduVSdce/61On+8etGy1srfFB0V/fJURuoZV9PRRjbWk5?=
 =?us-ascii?Q?UFWp2esdANytiXL0/C93w1Nc4jEh+utrWXfilxuVrLoM8M1bzepB7vn37K5k?=
 =?us-ascii?Q?nwKSwY9G6uiqvp7/iBxsMqJkcBqYwvdxO4FPOjWv0g53L68z21QnggvnAVLX?=
 =?us-ascii?Q?fmsubEDLGAAtki00Dk1dxWPnOn3o0J3v//vOm03uUyQEcXmMRyYT3OvkPPGb?=
 =?us-ascii?Q?oo5SpK6ivovvBySZCzOBCYrycYUeVM8dGoCRAJVYUoNcz7Hkp+9wasqF/UIZ?=
 =?us-ascii?Q?r0fNgH3NF+79r7zXwq/P7PVIE1EuvF05SlvH5dN1NnZr6UqRS/eIAuOiq0qV?=
 =?us-ascii?Q?MTxkix9mxLsNYuWyG8OxgrQrHnYknMonM2bOJIWx?=
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: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a77df410-10c4-4b9d-71c6-08db47b4ca00
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Apr 2023 06:50:01.1731
 (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: H3/QHHhogde/o4KjVTld8AT6OaLwauhGAKx0ONq/aXXmpVdtJM51wS/6WTmS7v46PMsrqnC4ELZYJptgU/pI7A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR11MB8126
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham 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: Chatre, Reinette <reinette.chatre@intel.com>
> Sent: Friday, April 28, 2023 1:36 AM
>=20
> pci_msix_alloc_irq_at() enables an individual MSI-X interrupt to be
> allocated after MSI-X enabling.
>=20
> Use dynamic MSI-X (if supported by the device) to allocate an interrupt
> after MSI-X is enabled. An MSI-X interrupt is dynamically allocated at
> the time a valid eventfd is assigned. This is different behavior from
> a range provided during MSI-X enabling where interrupts are allocated
> for the entire range whether a valid eventfd is provided for each
> interrupt or not.
>=20
> The PCI-MSIX API requires that some number of irqs are allocated for
> an initial set of vectors when enabling MSI-X on the device. When
> dynamic MSIX allocation is not supported, the vector table, and thus
> the allocated irq set can only be resized by disabling and re-enabling
> MSI-X with a different range. In that case the irq allocation is
> essentially a cache for configuring vectors within the previously
> allocated vector range. When dynamic MSI-X allocation is supported,
> the API still requires some initial set of irqs to be allocated, but
> also supports allocating and freeing specific irq vectors both
> within and beyond the initially allocated range.
>=20
> For consistency between modes, as well as to reduce latency and improve
> reliability of allocations, and also simplicity, this implementation
> only releases irqs via pci_free_irq_vectors() when either the interrupt
> mode changes or the device is released.

It improves the reliability of allocations from the calling device p.o.v.

But system-wide this is not efficient use of irqs and not releasing them
timely may affect the reliability of allocations for other devices.

Should this behavior be something configurable?

>=20
> +/*
> + * Return Linux IRQ number of an MSI or MSI-X device interrupt vector.
> + * If a Linux IRQ number is not available then a new interrupt will be
> + * allocated if dynamic MSI-X is supported.
> + */
> +static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
> +			      unsigned int vector, bool msix)
> +{
> +	struct pci_dev *pdev =3D vdev->pdev;
> +	struct msi_map map;
> +	int irq;
> +	u16 cmd;
> +
> +	irq =3D pci_irq_vector(pdev, vector);
> +	if (irq > 0 || !msix || !vdev->has_dyn_msix)
> +		return irq;

if (irq >=3D 0 || ...)

> +
> +/*
> + * Where is vfio_msi_free_irq() ?
> + *
> + * Allocated interrupts are maintained, essentially forming a cache that
> + * subsequent allocations can draw from. Interrupts are freed using
> + * pci_free_irq_vectors() when MSI/MSI-X is disabled.
> + */

Probably merge it with the comment of vfio_msi_alloc_irq()?

> @@ -401,6 +430,12 @@ static int vfio_msi_set_vector_signal(struct
> vfio_pci_core_device *vdev,
>  	if (fd < 0)
>  		return 0;
>=20
> +	if (irq =3D=3D -EINVAL) {
> +		irq =3D vfio_msi_alloc_irq(vdev, vector, msix);
> +		if (irq < 0)
> +			return irq;
> +	}
> +
>  	ctx =3D vfio_irq_ctx_alloc(vdev, vector);
>  	if (!ctx)
>  		return -ENOMEM;

This doesn't read clean that an irq is allocated but not released
in the error unwind.