Message-ID: <61296e93-6268-05cd-e876-680e07645a16@intel.com>
Date:   Fri, 17 Mar 2023 15:21:09 -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 8/8] vfio/pci: Clear VFIO_IRQ_INFO_NORESIZE for MSI-X
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>
 <549e6300c0ea011cdce9a2712d49de4efd3a06b7.1678911529.git.reinette.chatre@intel.com>
 <20230317150554.6bf92337.alex.williamson@redhat.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230317150554.6bf92337.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?OUdpQThxWkpVWmJoT21ya3JFbVJDVERRMUNrZExOdmk0UndtZ0REeGNkUFNF?=
 =?utf-8?B?SVpPUUhtUEQ5eWhyUnRyYU41RzlBOFNGNWwxelJZL01MVDdhSWVTN1RWTkd1?=
 =?utf-8?B?Z2E0cUhYdWlqQkpmeE9QWmtRQXBRWm1qekhEbGRhbEd6OGovZjRuMGFVWG9C?=
 =?utf-8?B?L2lZYW1QSUNqTktocC9VU0NFdUs2b1YwZVRMYjB6SGRwdldXditxWms5eXIz?=
 =?utf-8?B?U0JKQjY2WDBIR20wYVZhb2lVNlF2K2FmZG9ZckNjeExxdFNEa2ZVZmhvUDRD?=
 =?utf-8?B?VFczei9WaDlvQnpCYXJHc3JFUEtyZXlHcldTLzBGOTNSZUowYk95MTlaYzkx?=
 =?utf-8?B?ZXh5MExrUFdJR0dzYzRSZEVkZndFOWdvWnNpTnpFZVRnbXNTWWNkeFNWNmVy?=
 =?utf-8?B?MW84T3UzMlVsVEJ6T0JKbkNDbVJoZXFqc2g1YVhIOEFNRFFpY2pMRXRRYTl6?=
 =?utf-8?B?UTdxY2h4TW93RkY0NHVXOW94bXJWNUxRaXRKT0hKSUFGUTRZZDVrVEVVRWEr?=
 =?utf-8?B?MWRRWFlGbTU4YURzZWJKYmFBbllhaFpVbW5SZEF6V1Z3emFPc2Q5azZZV2NQ?=
 =?utf-8?B?VE5ZeStmU1BlVHBXT1VQVDhka0Z5bE5GOXFvSXZiTHF1Vks3dTBndWExUlNm?=
 =?utf-8?B?K28ySXVBRC9pZnhDbEhJV05QaUVCT2xDcmRUbGlnK0w5ekJrTXNQR0dsaWpO?=
 =?utf-8?B?VFFZZEVtNTM3ZldQdkM0OHNKUG0zcncybGxxYlRPa1hzazJpNGVpeFdrN05a?=
 =?utf-8?B?Y1J3WVpVT2dwaDk3R2h5UllOOVcyU1E4SnhYN21hd0NJMjZXbFRUY1VoZHRa?=
 =?utf-8?B?TXhhbk1oKzFwL1YxRFpJam96UWNMMFRJcEpUWEU3cVFsVWhMb2djVnAyMGNw?=
 =?utf-8?B?bTMzRDFlUndMd1MwYXJKREFINFdmOU9FVnovZlhKN095UVU3eVdiWDBHaUVK?=
 =?utf-8?B?YU14bXdrd2s4Q2R0elhDa3d0cDNPSGk4c0txcUZWNjBqWGM0SGpLT1o2OHh1?=
 =?utf-8?B?dWpoMnVVQjlnVDVxVytGdWQvWG82cm9Hd20vVFdUbHB4Q1IxbGlucXN0N0tF?=
 =?utf-8?B?MWszWGVZWlhURGZHRDNpaytlSUtDUVQvakt2M2RRNzNhTHJmcitITW0reFJt?=
 =?utf-8?B?cEtwUkNnNjNXWWgrbUEwcVZqRkdQbW5HeEVTUklZaUY1dlJMejBXek96MWFa?=
 =?utf-8?B?dzZHTFdxTkhGS1pSalN0VFp1ZitxeE5aOWtRb0ZXUHhmaEMzcExTYVI0ajVT?=
 =?utf-8?B?RE0yeU43bERPNmdia1k5YTZ5T3hYOWlualNueFJaZ0JxZXV3YlJOY1NySFhC?=
 =?utf-8?B?VVFmVmVFb1VTSFFrZ0paS0VvUE1oaXNOWDNYUC8rRlJpaDR2QUEyNDR1RE5q?=
 =?utf-8?B?NlRUbEwyOVlRTU8zNjVCdlVBVzNrdU5UNGpqcEhDVVFqeDNNcUd1aTYzWXF3?=
 =?utf-8?B?UmZLQ2cvbXRid0NmeDNqRXZ5UmRzd2Q4MFlFMHVrQW03RFRhdUpnRENUMXZ1?=
 =?utf-8?B?TC9KYm5VSUtMdjZUalpPT1ErOWE4ME1IL0dKREhwUFl6MVZvdzJiSS9SQk92?=
 =?utf-8?B?ZzdBZkRqZzAyNTNieXZDeDNINEkrVkkrT3VRWXdoWFQvQmxqdjJ4Q2Mxd25q?=
 =?utf-8?B?YVd0MUc5WHRVMXdZM3gxenpmVTBMa21FTm5yMlJYUHRMbUQ4NmJITGV3bzM1?=
 =?utf-8?B?c3hhTk9WZkRSRkI5ZkxJemVMYWR3bHRSQUpoVkN3YmxzUHJiSy9kNEE5b0lz?=
 =?utf-8?B?S1o0RWRER0oxd01VOVlTcVhGRlFtR3lzYm5tc3Nwd21pL050TFNiRVhSRjR1?=
 =?utf-8?B?ZmduRytCdS83WkdGWldEMGRIakowU0VCUUJzeUxqeTk1WDBkUkp6SDJ5bDZJ?=
 =?utf-8?B?L0p0ZFVnVGs5Rm5rOEE3YVRBdTFQRjNGaDM5TkdFclNyRjZxY1BNd0JzM3Ax?=
 =?utf-8?B?di84MXpIWDRLMGk2cGY3YXFsWVhiWDVsY3UyNHJDcUF6RnNVWWlLdHZ2TW5s?=
 =?utf-8?B?ajNSd21vajFqVVJwQ0srN08rcnpHZkJHWGhIK253ME9Mc1pDMFNyTHNMOFoy?=
 =?utf-8?B?UmRoN0pNOFhndVZWQWhoeisxb3g3RnMwSElmZ1BTSHpxYlNQdkNhNUg3dTdB?=
 =?utf-8?B?bHp2ejhzOXVOSW9ldVhrTlA5dDh0TGVsZlNka1Vyc0xPSG0rN1VsbHFzZEtm?=
 =?utf-8?B?Z0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 195a1488-7283-42af-3fa3-08db2735ea70
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 22:21:12.5844
 (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: 5LFpjYgciJ86/HJ4RfvRB1XRLVrJtH9wGX3ffqojNk0x0mjIBeVZLVVf9fw5QZqNN3+hq1j5jKLLaYeuLnU3TF+BJ2oAAZ7UZGPyUGIlW88=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5336
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/17/2023 2:05 PM, Alex Williamson wrote:
> On Wed, 15 Mar 2023 13:59:28 -0700
> Reinette Chatre <reinette.chatre@intel.com> wrote:
> 
>> Dynamic MSI-X is supported. Clear VFIO_IRQ_INFO_NORESIZE
>> to provide guidance to user space.
>>
>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>> ---
>>  drivers/vfio/pci/vfio_pci_core.c | 2 +-
>>  include/uapi/linux/vfio.h        | 3 +++
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index ae0e161c7fc9..1d071ee212a7 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -1111,7 +1111,7 @@ static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev,
>>  	if (info.index == VFIO_PCI_INTX_IRQ_INDEX)
>>  		info.flags |=
>>  			(VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_AUTOMASKED);
>> -	else
>> +	else if (info.index != VFIO_PCI_MSIX_IRQ_INDEX)
>>  		info.flags |= VFIO_IRQ_INFO_NORESIZE;
>>  
> 
> I think we need to check pci_msix_can_alloc_dyn(), right?  Thanks,

Can pci_msix_can_alloc_dyn() ever return false?

I cannot see how pci_msix_can_alloc_dyn() can return false when
considering the VFIO PCI MSI-X flow:

vfio_msi_enable(..., ..., msix == true) 
  pci_alloc_irq_vectors(..., ..., ..., flag == PCI_IRQ_MSIX) 
    pci_alloc_irq_vectors_affinity() 
      __pci_enable_msix_range() 
        pci_setup_msix_device_domain() 
          pci_create_device_domain(..., &pci_msix_template, ...)

The template used above, pci_msix_template, has MSI_FLAG_PCI_MSIX_ALLOC_DYN
hardcoded. This is the flag that pci_msix_can_alloc_dyn() tests for.

If indeed pci_msix_can_alloc_dyn() can return false in the VFIO PCI MSI-X
usage then this series needs to be reworked to continue supporting
existing allocation mechanism as well as dynamic MSI-X allocation. Which
allocation mechanism to use would be depend on pci_msix_can_alloc_dyn().

Alternatively, if you agree that VFIO PCI MSI-X can expect
pci_msix_can_alloc_dyn() to always return true then I should perhaps
add a test in vfio_msi_enable() that confirms this is the case. This would
cause vfio_msi_enable() to fail if dynamic MSI-X is not possible, perhaps even
complain loudly with a WARN.

Reinette