Re: [PATCH 07/17] vfio/pci: Preserve per-interrupt contexts

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

Hi Alex,

On 2/5/2024 2:35 PM, Alex Williamson wrote:
> On Thu,  1 Feb 2024 20:57:01 -0800
> Reinette Chatre <reinette.chatre@intel.com> wrote:

...

>> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
>> index 31f73c70fcd2..7ca2b983b66e 100644
>> --- a/drivers/vfio/pci/vfio_pci_intrs.c
>> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
>> @@ -427,7 +427,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  
>>  	ctx = vfio_irq_ctx_get(vdev, vector);
>>  
>> -	if (ctx) {
>> +	if (ctx && ctx->trigger) {
>>  		irq_bypass_unregister_producer(&ctx->producer);
>>  		irq = pci_irq_vector(pdev, vector);
>>  		cmd = vfio_pci_memory_lock_and_enable(vdev);
>> @@ -435,8 +435,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  		vfio_pci_memory_unlock_and_restore(vdev, cmd);
>>  		/* Interrupt stays allocated, will be freed at MSI-X disable. */
>>  		kfree(ctx->name);
>> +		ctx->name = NULL;
> 
> Setting ctx->name = NULL is not strictly necessary and does not match
> the INTx code that we're claiming to try to emulate.  ctx->name is only
> tested immediately after allocation below, otherwise it can be inferred
> from ctx->trigger.  Thanks,

This all matches my understanding. I added ctx->name = NULL after every kfree(ctx->name)
(see below for confirmation of other instance). You are correct that the flow
infers validity of ctx->name from ctx->trigger. My motivation for
adding ctx->name = NULL is that, since the interrupt context persists, this
change ensures that there will be no pointer that points to freed memory. I
am not comfortable leaving pointers to freed memory around.

>>  		eventfd_ctx_put(ctx->trigger);
>> -		vfio_irq_ctx_free(vdev, ctx, vector);
>> +		ctx->trigger = NULL;
>>  	}
>>  
>>  	if (fd < 0)
>> @@ -449,16 +450,17 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  			return irq;
>>  	}
>>  
>> -	ctx = vfio_irq_ctx_alloc(vdev, vector);
>> -	if (!ctx)
>> -		return -ENOMEM;
>> +	/* Per-interrupt context remain allocated. */
>> +	if (!ctx) {
>> +		ctx = vfio_irq_ctx_alloc(vdev, vector);
>> +		if (!ctx)
>> +			return -ENOMEM;
>> +	}
>>  
>>  	ctx->name = kasprintf(GFP_KERNEL_ACCOUNT, "vfio-msi%s[%d](%s)",
>>  			      msix ? "x" : "", vector, pci_name(pdev));
>> -	if (!ctx->name) {
>> -		ret = -ENOMEM;
>> -		goto out_free_ctx;
>> -	}
>> +	if (!ctx->name)
>> +		return -ENOMEM;
>>  
>>  	trigger = eventfd_ctx_fdget(fd);
>>  	if (IS_ERR(trigger)) {
>> @@ -502,8 +504,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  	eventfd_ctx_put(trigger);
>>  out_free_name:
>>  	kfree(ctx->name);
>> -out_free_ctx:
>> -	vfio_irq_ctx_free(vdev, ctx, vector);
>> +	ctx->name = NULL;

Here is the other one.

Reinette