Message-ID: <4d72821b-36a2-70c9-ee58-f7a21b13510f@intel.com>
Date:   Thu, 16 Mar 2023 16:38:29 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.8.0
Subject: Re: [RFC PATCH 0/8] vfio/pci: Support dynamic allocation of MSI-X
 interrupts
Content-Language: en-US
To:     Alex Williamson <alex.williamson@redhat.com>
CC:     <jgg@nvidia.com>, <yishaih@nvidia.com>,
        <shameerali.kolothum.thodi@huawei.com>, <kevin.tian@intel.com>,
        <tglx@linutronix.de>, <darwi@linutronix.de>, <kvm@vger.kernel.org>,
        <dave.jiang@intel.com>, <jing2.liu@intel.com>,
        <ashok.raj@intel.com>, <fenghua.yu@intel.com>,
        <tom.zanussi@linux.intel.com>, <linux-kernel@vger.kernel.org>
References: <cover.1678911529.git.reinette.chatre@intel.com>
 <20230316155646.07ae266f.alex.williamson@redhat.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230316155646.07ae266f.alex.williamson@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OUFYTVh6eWhpazZsck0zQVBoSTdMUjl2SllwTUpBVGgzZ1NwWTE3YTFKdndl?=
 =?utf-8?B?N0c3bTFnSFo1YU9VSDhRamRKMXc2TkxFNUdVVFRxWnR1c1RaWk5zZVJrTG5F?=
 =?utf-8?B?MFVBbDBybFE4VEs0R3AxeTFVMlBEUzFwQ3BlTkk2RWNiVUNwVkl6aUtFOFFU?=
 =?utf-8?B?Q3YwMHE5ZnZmOUNTVzVsT3QvTTl2REt2THl3a2N3WFZ2SUk1NXkxT3VTa2xI?=
 =?utf-8?B?Q0tSTGVJRUpvc0lMRGtPbXgxSlNzT2hxSThXTnNienZQaXlETVRmNVFvaVh5?=
 =?utf-8?B?dERSYmNxcGtLRkxaaGNNVGh5eFhvVUk0ekhlbTBMcktOT20vN1FReHkzNzhV?=
 =?utf-8?B?OXNPQXZkOVd6T0Nvd3VFVlNXVkFDbkRlbXlEQmpRUjVkMVdIdG5Lb2dINXhX?=
 =?utf-8?B?TnRoWXdDSWIxemxZSFN1MmlNRkQxREdFSFJYbU5mREMyYmd6UkRwNHR4clFy?=
 =?utf-8?B?b2JtcGcweWdlcUFQMmNlaXNzZVN5OXZ6bDdBR081L3REMGxDR1ZTemJoRVpU?=
 =?utf-8?B?ZUdVQ05MSGpWOVFZWEVhV1hvRzFjYUdCWmVsZk5DKzkwQkttZVVBWXRTenhn?=
 =?utf-8?B?TU8rcnBBR0k4T0dJOTdFTzE3TU1RTjkrYkI5bmEyeCtRazNaS0JVV21VUWtP?=
 =?utf-8?B?SURDZU9Ib1MrK3prVnJtQ1R3eHg1WFFrZVp0WCtLZm9CRXRDTEhjdGI3ODI0?=
 =?utf-8?B?UVRjVFZNU0VwclFldGRFOVA1bkJmOTd6QS9hRGNNZTBJT3paUXNRMVhPQkUr?=
 =?utf-8?B?dTBERWNuWUVXUGlETDQ5SWR2L05QbzJSaTBYVE4wY3YreFJPMUZNN09rZ2pq?=
 =?utf-8?B?bDkza1o4WU05OE50TUUrY0NtUjdxc3ZwOUkvb3FETGtUaEplWE03N1BpU1p4?=
 =?utf-8?B?OXBIQVZMT0lScmlXd0NkVUNVOU84bVI3ZkdnTllqek5BdlpsTXh2OGlaUXVq?=
 =?utf-8?B?dkREd2lZVnRVQWRyV1EwZy9VL2ttWmFaOFFIL2F0WkczZlpLNlhKTTVHQ2FN?=
 =?utf-8?B?Yjg4c2FrSXZuN05xcFVnWXJvVzNoQTZVc1FNZWZaSWFSdmF3dmtGSkFCRWJ3?=
 =?utf-8?B?M1dveUNkNVY3ZU5ZQjIwa3lLZzlCOTlBNjUzRklGdkE5RE0vNnUvbHF0ZHJm?=
 =?utf-8?B?VkRKdGprNkd1K2oxU3pFYUNWTzVCZ0djWE9iNGFLNElaVXU3MVlLMkVxU1A3?=
 =?utf-8?B?SFZsTVo2WDRra2dqdTR5bElMTkVUWUNjRnpQMU83Uk5seEhEVnNreit0WWhG?=
 =?utf-8?B?NUdjYU9BaE9rM2FJd21GRnl4QU54WE9yYnRrV01pRzliTVhBTEp2MUlZN3Rp?=
 =?utf-8?B?SkhLbXYwdUw2VVd0T2lQS3pwWjFOcFB1MFMwUkZ5QlRQcGZacXFnRk9tckxP?=
 =?utf-8?B?dEVSMitlRTJ0U1pJbFdlVTFCMEwzWlo4c25iL0JvSW5raU1VMkxmYjdUQkRI?=
 =?utf-8?B?L1g5TklIZkp5Y1FScGVyNEN2Zk9TVVI3ZmNONHdzaFdyRHplMXloVzd3eDNC?=
 =?utf-8?B?a0VCNXNVUnlhV1VTTFozZW5neGc1S1VVR1huR3VVdno2QU5jU25vaHp0RWhV?=
 =?utf-8?B?dkFOSjJVc09ENTJuWUdOTm9MNFhmODFyMWEwK1hjZHpuUjgwSTJMdEg2UFFt?=
 =?utf-8?B?OEJ1MEs4Wk9xWm1OV09YYW1zWXE5SHBvOGsrdkwydGNhVHRnNUpwRXRmS01Y?=
 =?utf-8?B?VkVEL1BrekwzT0dnSjg5cVlqMHVOVGpSU1c4enJORDRMcnZvOE8wVjdqVDFQ?=
 =?utf-8?B?b0VmZ1ZqTEg2emkwWlBPU3UxOTY2RUZiT3NwQnJkd2NKcXJsRUExdGpuNkJE?=
 =?utf-8?B?V1JNQmFaNCtCRFJLMnJSengyVS9McTBIclF4NjlTMGRmTWhhYWltS3gyQ1pi?=
 =?utf-8?B?SGxVOERJMEFONGpGSU1qMEhrWm9jWGJtS2dtMmhmWHdINlVRZG9UaVF2Tk03?=
 =?utf-8?B?RTU1ODk3eUxxNjVLUk5vK2wybm1BS2dTYitYU0hHNGFEZTJWR1poVWs3Tzkw?=
 =?utf-8?B?Vmc2ZzlSK2k3VTVZZE9GWWo2aTh5ZSt4RCtnbGhLVGpJSkdycVVraEVKZlVT?=
 =?utf-8?B?cGZ5VnB0dmdMYnQwdWNaREpqWHFxKzZUTUZpdTNMcUxRbFFpUjUrNXEwMjRV?=
 =?utf-8?B?TCtnVk9KYU5mT3dnTC8yemlDY1lCSitEMCtIblU4YzRiSE5JOG5JeXpkd2NH?=
 =?utf-8?B?WWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e7818b05-bf6e-45b5-8187-08db26778de6
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Mar 2023 23:38:33.2183
 (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: mlygoSbuK516zKK+biYl0hUKOBzPmnyeVLlof+B1cLTTsyfERkXU18fbJd8oXJN5tS5YN+6h4yzscOSMJOwwOoLm1emvdgraRW4+fbLYcUg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB8097
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Alex,

On 3/16/2023 2:56 PM, Alex Williamson wrote:
> On Wed, 15 Mar 2023 13:59:20 -0700 Reinette Chatre
> <reinette.chatre@intel.com> wrote:
> 

...

>> == Why is this an RFC ? ==
>> 
>> vfio support for dynamic MSI-X needs to work with existing user
>> space as well as upcoming user space that takes advantage of this
>> feature. I would appreciate guidance on the expectations and
>> requirements surrounding error handling when considering existing
>> user space.
>> 
>> For example, consider the following scenario: Start: Consider a
>> passthrough device that supports 10 MSI-X interrupts (0 to 9) and
>> existing Qemu allocated interrupts 0 to 4.
>> 
>> Scenario: Qemu (hypothetically) attempts to associate a new action
>> to interrupts 0 to 7. Early checking of this range in 
>> vfio_set_irqs_validate_and_prepare() will pass since it is a valid
>> range for the device. What happens after the early checking is
>> considered next:
>> 
>> Current behavior (before this series): Since the provided range, 0
>> - 7, exceeds the allocated range, no action will be taken on
>> existing allocated interrupts 0 - 4 and the Qemu request will fail
>> without making any state changes.
> 
> I must be missing something, it was described correctly earlier that 
> QEMU will disable MSI-X and re-enable with a new vector count.  Not 
> only does QEMU not really have a way to fail this change, pretty
> much nothing would work if we did.

Thank you very much for confirming Qemu behavior. 

One of my goals is to ensure that these kernel changes do not break
existing user space in any way. The only area I could find where
existing user space may encounter new behavior is if user space makes
the (pre dynamic MSI-X) mistake of attempting to associate triggers
to interrupts that have not been allocated. Thank you for confirming
that this is not a valid scenario for Qemu.

> What happens in this case is that the QEMU vfio-pci driver gets a 
> vector_use callback for one of these new vectors {5,6,7}, 
> vfio_msix_vector_do_use() checks that's greater than we have
> enabled, disables MSI-X, and re-enables it for the new vector count.
> Worst case we'll do that 3 times if the vectors are presented in
> ascending order.

Indeed. While testing this work I modified Qemu's vfio_msix_vector_do_use() to
consider VFIO_IRQ_INFO_NORESIZE. I saw the vector_use callback arriving
for each new vector and having Qemu just associate a new action with the
new vector (call vfio_set_irq_signaling()) instead of disabling MSI-X followed
by enabling all vectors worked well with the changes I present here. A single
interrupt was dynamically allocated and enabled without impacting others.

> Based on above, there really can never be an error if we expect the 
> device to work, so I think there's a misread of the current status. 
> Dynamic MSI-X support should simply reduce the disruption and chance
> of lost interrupts at the device, but the points where we risk that
> the host cannot provide the configuration we need are the same.

Thank you very much Alex. In this case, please do consider this
submission as a submission for inclusion. I'd be happy to resubmit
without the "RFC" prefix if that is preferred.

Reinette