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

[Reinette Chatre]

Hi Alex,

On 3/31/2023 3:24 PM, Alex Williamson wrote:
> On Fri, 31 Mar 2023 10:49:16 -0700
> Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
>> On 3/30/2023 3:42 PM, Alex Williamson wrote:
>>> On Thu, 30 Mar 2023 16:40:50 -0600
>>> Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
>>>   
>>>> On Tue, 28 Mar 2023 14:53:34 -0700
>>>> Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
>>>>  

...

>>>>> +		msix_map.index = vector;
>>>>> +		msix_map.virq = irq;
>>>>> +		pci_msix_free_irq(pdev, msix_map);
>>>>> +	}
>>>>> +	vfio_pci_memory_unlock_and_restore(vdev, cmd);
>>>>>  out_put_eventfd_ctx:
>>>>>  	eventfd_ctx_put(trigger);
>>>>>  out_free_name:
>>>>>  	kfree(ctx->name);
>>>>>  	ctx->name = NULL;
>>>>> +out_free_ctx:
>>>>> +	if (allow_dyn_alloc && new_ctx)
>>>>> +		vfio_irq_ctx_free(vdev, ctx, vector);
>>>>>  	return ret;
>>>>>  }
>>>>>      
>>>>
>>>> Do we really need the new_ctx test in the above cases?  Thanks,  
>>
>> new_ctx is not required for correctness but instead is used to keep
>> the code symmetric. 
>> Specifically, if the user enables MSI-X without providing triggers and
>> then later assign triggers then an error path without new_ctx would unwind
>> more than done in this function, it would free the context that
>> was allocated within vfio_msi_enable(). 
> 
> Seems like we already have that asymmetry, if a trigger is unset we'll
> free the ctx allocated by vfio_msi_enable().  Tracking which are

Apologies, but could you please elaborate on where the asymmetry is? I am
not able to see a flow in this solution where the ctx allocated by
vfio_msi_enable() is freed if the trigger is unset.

> allocated where is unnecessarily complex, how about a policy that

I do not see this as tracking where allocations are made. Instead I
see it as containing/compartmentalizing state changes with the goal of
making the code easier to understand and maintain. Specifically, new_ctx
is used so that if vfio_msi_set_vector_signal() fails, the state 
before and after vfio_msi_set_vector_signal() will be the same.

I do agree that it makes vfio_msi_set_vector_signal() more complex
and I can remove new_ctx if you find that this is unnecessary after
considering the motivations behind its use. 

> devices supporting vdev->has_dyn_msix only ever have active contexts
> allocated?  Thanks,

What do you see as an "active context"? A policy that is currently enforced
is that an allocated context always has an allocated interrupt associated
with it. I do not see how this could be expanded to also require an
enabled interrupt because interrupt enabling requires a trigger that
may not be available.

Reinette