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@xxxxxxxxx> 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@xxxxxxxxx/
