Re: [RFC PATCH 7/8] vfio/pci: Support dynamic MSI-x

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Alex,

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

...
 
>> +static void vfio_irq_ctx_free(struct vfio_pci_core_device *vdev,
>> +			      unsigned long index)
>> +{
>> +	struct vfio_pci_irq_ctx *ctx;
>> +
>> +	ctx = xa_load(&vdev->ctx, index);
>> +	if (ctx) {
>> +		xa_erase(&vdev->ctx, index);
>> +		kfree(ctx);
>> +	}
>> +}
> 
> The only places calling this have a known valid ctx, so it seems
> redundant that we xa_load it again.  Should ctx be a function arg to
> reduce this to simply xa_erase() + kfree()?

Good point. Will do.

...

>> +	if (!ctx) {
>> +		ret = vfio_irq_ctx_alloc_single(vdev, vector);
>> +		if (ret)
>> +			return ret;
>> +		ctx = vfio_irq_ctx_get(vdev, vector);
> 
> This suggests vfio_irq_ctx_alloc_single() should return ctx.
> 

Thank you. Yes, will do.

>> @@ -464,25 +506,38 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  		goto out_free_name;
>>  	}
>>  
>> -	/*
>> -	 * The MSIx vector table resides in device memory which may be cleared
>> -	 * via backdoor resets. We don't allow direct access to the vector
>> -	 * table so even if a userspace driver attempts to save/restore around
>> -	 * such a reset it would be unsuccessful. To avoid this, restore the
>> -	 * cached value of the message prior to enabling.
>> -	 */
>>  	cmd = vfio_pci_memory_lock_and_enable(vdev);
>>  	if (msix) {
>> -		struct msi_msg msg;
>> -
>> -		get_cached_msi_msg(irq, &msg);
>> -		pci_write_msi_msg(irq, &msg);
>> +		if (irq == -EINVAL) {
>> +			msix_map = pci_msix_alloc_irq_at(pdev, vector, NULL);
> 
> It looks to me like we need to support MSI-X with both NORESIZE
> behavior and dynamic allocation based on pci_msix_can_alloc_dyn().
> It's not entirely clear to me where this is and isn't supported, but
> the existence of the test helper suggests we can't assume support.

As I mentioned in my other response ([1]) I cannot see how pci_msix_can_alloc_dyn()
can return false. Even so, yes, I can rework this series to support both the
original and dynamic MSI-x allocation mechanisms.

>> +			if (msix_map.index < 0) {
>> +				vfio_pci_memory_unlock_and_restore(vdev, cmd);
>> +				ret = msix_map.index;
>> +				goto out_put_eventfd_ctx;
>> +			}
>> +			irq = msix_map.virq;
>> +		} else {
>> +			/*
>> +			 * The MSIx vector table resides in device memory which
>> +			 * may be cleared via backdoor resets. We don't allow
>> +			 * direct access to the vector table so even if a
>> +			 * userspace driver attempts to save/restore around
>> +			 * such a reset it would be unsuccessful. To avoid
>> +			 * this, restore the cached value of the message prior
>> +			 * to enabling.
>> +			 */
>> +			struct msi_msg msg;
>> +
>> +			get_cached_msi_msg(irq, &msg);
>> +			pci_write_msi_msg(irq, &msg);
>> +		}
> 
> I don't follow when this latter branch is ever taken in the new flow.
> It's stated earlier that ctx and irq are coupled, and I believe so is
> trigger.  So if we had a previous ctx and irq (and trigger), we removed
> it and irq is now always -EINVAL here.  Thanks,

From what I understand MSI-X can be enabled without providing any triggers.
That will result in the ctx and irq existing, but not trigger. When a trigger
is assigned later, it will run the latter branch.

Thank you very much

Reinette


[1] https://lore.kernel.org/lkml/61296e93-6268-05cd-e876-680e07645a16@intel.com/