Re: [PATCH v4 12/14] vfio-user: handle device interrupts

[Stefan Hajnoczi]

On Wed, Dec 15, 2021 at 10:35:36AM -0500, Jagannathan Raman wrote:
> @@ -62,6 +66,9 @@ void remote_iohub_set_irq(void *opaque, int pirq, int level)
>      QEMU_LOCK_GUARD(&iohub->irq_level_lock[pirq]);
>  
>      if (level) {
> +        if (iohub->intx_notify) {
> +            iohub->intx_notify(pirq, 0);
> +        }
>          if (++iohub->irq_level[pirq] == 1) {
>              event_notifier_set(&iohub->irqfds[pirq]);
>          }

Does it make sense to use iohub.c in vfio-user since these irqfds are
unused?

Instead of adding iohub->intx_notify(), can vfio-user register its own
set_irq() callback for the PCI bus?

> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> index ae375e69b9..2b28d465d5 100644
> --- a/hw/remote/vfio-user-obj.c
> +++ b/hw/remote/vfio-user-obj.c
> @@ -50,6 +50,9 @@
>  #include "hw/pci/pci.h"
>  #include "qemu/timer.h"
>  #include "hw/remote/iommu.h"
> +#include "hw/pci/msi.h"
> +#include "hw/pci/msix.h"
> +#include "hw/remote/iohub.h"
>  
>  #define TYPE_VFU_OBJECT "x-vfio-user-server"
>  OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
> @@ -81,6 +84,8 @@ struct VfuObject {
>      int vfu_poll_fd;
>  };
>  
> +static GHashTable *vfu_object_dev_table;
> +
>  static void vfu_object_init_ctx(VfuObject *o, Error **errp);
>  
>  static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
> @@ -347,6 +352,54 @@ static void vfu_object_register_bars(vfu_ctx_t *vfu_ctx, 
> PCIDevice *pdev)
>      }
>  }
>  
> +static void vfu_object_intx_notify(int pci_devfn, unsigned vector)
> +{
> +    vfu_ctx_t *vfu_ctx = g_hash_table_lookup(vfu_object_dev_table,
> +                                             (void *)(uint64_t)pci_devfn);

I'm not sure the hash table is necessary. The function arguments
currently don't contain the information we need, but that's easy to fix
because these functions are added by this patch. Add an opaque pointer
argument to ->intx_notify, ->msix_notify, and ->msi_notify in order to
pass along the VfuObject we need.

> +
> +    if (vfu_ctx) {
> +        vfu_irq_trigger(vfu_ctx, vector);
> +    }
> +}
> +
> +static void vfu_object_msi_notify(PCIDevice *pci_dev, unsigned vector)
> +{
> +    vfu_object_intx_notify(pci_dev->devfn, vector);
> +}
> +
> +static int vfu_object_setup_irqs(vfu_ctx_t *vfu_ctx, PCIDevice *pci_dev)
> +{
> +    RemoteMachineState *machine = REMOTE_MACHINE(current_machine);
> +    RemoteIOHubState *iohub = &machine->iohub;
> +    int ret;
> +
> +    ret = vfu_setup_device_nr_irqs(vfu_ctx, VFU_DEV_INTX_IRQ, 1);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    iohub->intx_notify = vfu_object_intx_notify;
> +
> +    ret = 0;
> +    if (msix_nr_vectors_allocated(pci_dev)) {
> +        ret = vfu_setup_device_nr_irqs(vfu_ctx, VFU_DEV_MSIX_IRQ,
> +                                       msix_nr_vectors_allocated(pci_dev));
> +
> +        pci_dev->msix_notify = vfu_object_msi_notify;
> +    } else if (msi_nr_vectors_allocated(pci_dev)) {
> +        ret = vfu_setup_device_nr_irqs(vfu_ctx, VFU_DEV_MSI_IRQ,
> +                                       msi_nr_vectors_allocated(pci_dev));
> +
> +        pci_dev->msi_notify = vfu_object_msi_notify;
> +    }
> +
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
>  /*
>   * TYPE_VFU_OBJECT depends on the availability of the 'socket' and 'device'
>   * properties. It also depends on devices instantiated in QEMU. These
> @@ -437,6 +490,13 @@ static void vfu_object_init_ctx(VfuObject *o, Error 
> **errp)
>  
>      vfu_object_register_bars(o->vfu_ctx, o->pci_dev);
>  
> +    ret = vfu_object_setup_irqs(o->vfu_ctx, o->pci_dev);
> +    if (ret < 0) {
> +        error_setg(errp, "vfu: Failed to setup interrupts for %s",
> +                   o->device);
> +        goto fail;
> +    }
> +
>      ret = vfu_realize_ctx(o->vfu_ctx);
>      if (ret < 0) {
>          error_setg(errp, "vfu: Failed to realize device %s- %s",
> @@ -450,6 +510,9 @@ static void vfu_object_init_ctx(VfuObject *o, Error 
> **errp)
>          goto fail;
>      }
>  
> +    g_hash_table_insert(vfu_object_dev_table,
> +                        (void *)(uint64_t)o->pci_dev->devfn, o->vfu_ctx);

vfu_object_dev_table seems like a misnomer since the values stored are
actually vfu_ctx_t, not VfuObjects. vfu_devfn_to_vfu_ctx_table?

> +
>      qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_attach_ctx, NULL, o);
>  
>      return;
> @@ -504,9 +567,18 @@ static void vfu_object_finalize(Object *obj)
>          remote_iommu_free(o->pci_dev);
>      }
>  
> +    if (o->pci_dev &&
> +            g_hash_table_lookup(vfu_object_dev_table,
> +                                (void *)(uint64_t)o->pci_dev->devfn)) {

lookup() is unnecessary since remove() is a nop if the key does not
exist.

signature.asc
Description: PGP signature