Re: [Qemu-devel] [RFC][PATCH 23/45] qemu-kvm: Rework MSI-X mask notifier to generic MSI config notifiers

[Michael S. Tsirkin]

On Mon, Oct 17, 2011 at 11:27:57AM +0200, Jan Kiszka wrote:
> MSI config notifiers are supposed to be triggered on every relevant
> configuration change of MSI vectors or if MSI is enabled/disabled.
> 
> Two notifiers are established, one for vector changes and one for general
> enabling. The former notifier additionally passes the currently active
> MSI message.
> This will allow to update potential in-kernel IRQ routes on
> changes. The latter notifier is optional and will only be used by a
> subset of clients.
> 
> These notifiers are currently only available for MSI-X but will be
> extended to legacy MSI as well.
> 
> Signed-off-by: Jan Kiszka <address@hidden>

Passing message, always, does not seem to make sense: message is only
valid if it is unmasked.
Further, IIRC the spec requires any changes to be done while
message is masked. So mask notifier makes more sense to me:
it does the same thing using one notifier that you do
using two notifiers.


> ---
>  hw/msix.c       |  119 +++++++++++++++++++++++++++++++++++++-----------------
>  hw/msix.h       |    6 ++-
>  hw/pci.h        |    8 ++-
>  hw/virtio-pci.c |   24 ++++++------
>  4 files changed, 102 insertions(+), 55 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 247b255..176bc76 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -219,16 +219,24 @@ static bool msix_is_masked(PCIDevice *dev, int vector)
>          dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  }
>  
> -static void msix_handle_mask_update(PCIDevice *dev, int vector)
> +static void msix_fire_vector_config_notifier(PCIDevice *dev,
> +                                             unsigned int vector, bool 
> masked)
>  {
> -    bool masked = msix_is_masked(dev, vector);
> +    MSIMessage msg;
>      int ret;
>  
> -    if (dev->msix_mask_notifier) {
> -        ret = dev->msix_mask_notifier(dev, vector,
> -                                      msix_is_masked(dev, vector));
> +    if (dev->msix_vector_config_notifier) {
> +        msix_message_from_vector(dev, vector, &msg);
> +        ret = dev->msix_vector_config_notifier(dev, vector, &msg, masked);
>          assert(ret >= 0);
>      }
> +}
> +
> +static void msix_handle_mask_update(PCIDevice *dev, int vector)
> +{
> +    bool masked = msix_is_masked(dev, vector);
> +
> +    msix_fire_vector_config_notifier(dev, vector, masked);
>      if (!masked && msix_is_pending(dev, vector)) {
>          msix_clr_pending(dev, vector);
>          msix_notify(dev, vector);
> @@ -240,20 +248,27 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>                         uint32_t old_val, int len)
>  {
>      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> -    bool was_masked;
> +    bool was_masked, was_enabled, is_enabled;
>      int vector;
>  
>      if (!msix_present(dev) || !range_covers_byte(addr, len, enable_pos)) {
>          return;
>      }
>  
> -    if (!msix_enabled(dev)) {
> +    old_val >>= (enable_pos - addr) * 8;
> +
> +    was_enabled = old_val & MSIX_ENABLE_MASK;
> +    is_enabled = msix_enabled(dev);
> +    if (was_enabled != is_enabled && dev->msix_enable_notifier) {
> +        dev->msix_enable_notifier(dev, is_enabled);
> +    }
> +
> +    if (!is_enabled) {
>          return;
>      }
>  
>      pci_device_deassert_intx(dev);
>  
> -    old_val >>= (enable_pos - addr) * 8;
>      was_masked =
>          (old_val & (MSIX_MASKALL_MASK | MSIX_ENABLE_MASK)) != 
> MSIX_ENABLE_MASK;
>      if (was_masked != msix_function_masked(dev)) {
> @@ -270,15 +285,20 @@ static void msix_mmio_write(void *opaque, 
> target_phys_addr_t addr,
>      unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
>      unsigned int vector = offset / PCI_MSIX_ENTRY_SIZE;
>      bool was_masked = msix_is_masked(dev, vector);
> +    bool is_masked;
>  
>      pci_set_long(dev->msix_table_page + offset, val);
>      if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>          kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, 
> vector));
>      }
>  
> -    if (vector < dev->msix_entries_nr &&
> -        was_masked != msix_is_masked(dev, vector)) {
> -        msix_handle_mask_update(dev, vector);
> +    if (vector < dev->msix_entries_nr) {
> +        is_masked = msix_is_masked(dev, vector);
> +        if (was_masked != is_masked) {
> +            msix_handle_mask_update(dev, vector);
> +        } else {
> +            msix_fire_vector_config_notifier(dev, vector, is_masked);
> +        }
>      }
>  }
>  
> @@ -305,17 +325,17 @@ static void msix_mmio_setup(PCIDevice *d, MemoryRegion 
> *bar)
>  
>  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>  {
> -    int vector, r;
> +    int vector;
> +
>      for (vector = 0; vector < nentries; ++vector) {
>          unsigned offset =
>              vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
>          bool was_masked = msix_is_masked(dev, vector);
> +
>          dev->msix_table_page[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> -        if (was_masked != msix_is_masked(dev, vector) &&
> -            dev->msix_mask_notifier) {
> -            r = dev->msix_mask_notifier(dev, vector,
> -                                        msix_is_masked(dev, vector));
> -            assert(r >= 0);
> +
> +        if (!was_masked) {
> +            msix_handle_mask_update(dev, vector);
>          }
>      }
>  }
> @@ -337,7 +357,6 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> nentries,
>      if (nentries > MSIX_MAX_ENTRIES)
>          return -EINVAL;
>  
> -    dev->msix_mask_notifier = NULL;
>      dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
>                                          sizeof *dev->msix_entry_used);
>  
> @@ -529,36 +548,50 @@ void msix_unuse_all_vectors(PCIDevice *dev)
>  }
>  
>  /* Invoke the notifier if vector entry is used and unmasked. */
> -static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, int 
> masked)
> +static int
> +msix_notify_if_unmasked(PCIDevice *dev, unsigned int vector, bool masked)
>  {
> -    assert(dev->msix_mask_notifier);
> +    MSIMessage msg;
> +
> +    assert(dev->msix_vector_config_notifier);
> +
>      if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
>          return 0;
>      }
> -    return dev->msix_mask_notifier(dev, vector, masked);
> +    msix_message_from_vector(dev, vector, &msg);
> +    return dev->msix_vector_config_notifier(dev, vector, &msg, masked);
>  }
>  
> -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +static int
> +msix_set_config_notifier_for_vector(PCIDevice *dev, unsigned int vector)
>  {
> -     /* Notifier has been set. Invoke it on unmasked vectors. */
> -     return msix_notify_if_unmasked(dev, vector, 0);
> +    /* Notifier has been set. Invoke it on unmasked vectors. */
> +    return msix_notify_if_unmasked(dev, vector, false);
>  }
>  
> -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned 
> vector)
> +static int
> +msix_unset_config_notifier_for_vector(PCIDevice *dev, unsigned int vector)
>  {
> -     /* Notifier will be unset. Invoke it to mask unmasked entries. */
> -     return msix_notify_if_unmasked(dev, vector, 1);
> +    /* Notifier will be unset. Invoke it to mask unmasked entries. */
> +    return msix_notify_if_unmasked(dev, vector, true);
>  }
>  
> -int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> +int msix_set_config_notifiers(PCIDevice *dev,
> +                              MSIEnableNotifier enable_notifier,
> +                              MSIVectorConfigNotifier vector_config_notifier)
>  {
>      int r, n;
> -    assert(!dev->msix_mask_notifier);
> -    dev->msix_mask_notifier = f;
> +
> +    dev->msix_enable_notifier = enable_notifier;
> +    dev->msix_vector_config_notifier = vector_config_notifier;
> +
> +    if (enable_notifier && msix_enabled(dev)) {
> +        enable_notifier(dev, true);
> +    }
>      if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
>          (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
>          for (n = 0; n < dev->msix_entries_nr; ++n) {
> -            r = msix_set_mask_notifier_for_vector(dev, n);
> +            r = msix_set_config_notifier_for_vector(dev, n);
>              if (r < 0) {
>                  goto undo;
>              }
> @@ -568,31 +601,41 @@ int msix_set_mask_notifier(PCIDevice *dev, 
> msix_mask_notifier_func f)
>  
>  undo:
>      while (--n >= 0) {
> -        msix_unset_mask_notifier_for_vector(dev, n);
> +        msix_unset_config_notifier_for_vector(dev, n);
>      }
> -    dev->msix_mask_notifier = NULL;
> +    if (enable_notifier && msix_enabled(dev)) {
> +        enable_notifier(dev, false);
> +    }
> +    dev->msix_enable_notifier = NULL;
> +    dev->msix_vector_config_notifier = NULL;
>      return r;
>  }
>  
> -int msix_unset_mask_notifier(PCIDevice *dev)
> +int msix_unset_config_notifiers(PCIDevice *dev)
>  {
>      int r, n;
> -    assert(dev->msix_mask_notifier);
> +
> +    assert(dev->msix_vector_config_notifier);
> +
>      if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
>          (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
>          for (n = 0; n < dev->msix_entries_nr; ++n) {
> -            r = msix_unset_mask_notifier_for_vector(dev, n);
> +            r = msix_unset_config_notifier_for_vector(dev, n);
>              if (r < 0) {
>                  goto undo;
>              }
>          }
>      }
> -    dev->msix_mask_notifier = NULL;
> +    if (dev->msix_enable_notifier && msix_enabled(dev)) {
> +        dev->msix_enable_notifier(dev, false);
> +    }
> +    dev->msix_enable_notifier = NULL;
> +    dev->msix_vector_config_notifier = NULL;
>      return 0;
>  
>  undo:
>      while (--n >= 0) {
> -        msix_set_mask_notifier_for_vector(dev, n);
> +        msix_set_config_notifier_for_vector(dev, n);
>      }
>      return r;
>  }
> diff --git a/hw/msix.h b/hw/msix.h
> index 685dbe2..978f417 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -29,6 +29,8 @@ void msix_notify(PCIDevice *dev, unsigned vector);
>  
>  void msix_reset(PCIDevice *dev);
>  
> -int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func);
> -int msix_unset_mask_notifier(PCIDevice *dev);
> +int msix_set_config_notifiers(PCIDevice *dev,
> +                              MSIEnableNotifier enable_notifier,
> +                              MSIVectorConfigNotifier 
> vector_config_notifier);
> +int msix_unset_config_notifiers(PCIDevice *dev);
>  #endif
> diff --git a/hw/pci.h b/hw/pci.h
> index 0177df4..4249c6a 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -127,8 +127,9 @@ enum {
>      QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
>  };
>  
> -typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
> -                                    int masked);
> +typedef void (*MSIEnableNotifier)(PCIDevice *dev, bool enabled);
> +typedef int (*MSIVectorConfigNotifier)(PCIDevice *dev, unsigned int vector,
> +                                       MSIMessage *msg, bool masked);
>  
>  struct PCIDevice {
>      DeviceState qdev;
> @@ -210,7 +211,8 @@ struct PCIDevice {
>       * on the rest of the region. */
>      target_phys_addr_t msix_page_size;
>  
> -    msix_mask_notifier_func msix_mask_notifier;
> +    MSIEnableNotifier msix_enable_notifier;
> +    MSIVectorConfigNotifier msix_vector_config_notifier;
>  };
>  
>  PCIDevice *pci_register_device(PCIBus *bus, const char *name,
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index ad6a002..6718945 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -520,8 +520,8 @@ static void virtio_pci_guest_notifier_read(void *opaque)
>      }
>  }
>  
> -static int virtio_pci_mask_vq(PCIDevice *dev, unsigned vector,
> -                              VirtQueue *vq, int masked)
> +static int virtio_pci_mask_vq(PCIDevice *dev, unsigned int vector,
> +                              VirtQueue *vq, bool masked)
>  {
>      EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
>      int r = kvm_msi_irqfd_set(&dev->msix_cache[vector],
> @@ -540,8 +540,8 @@ static int virtio_pci_mask_vq(PCIDevice *dev, unsigned 
> vector,
>      return 0;
>  }
>  
> -static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
> -                                    int masked)
> +static int virtio_pci_msi_vector_config(PCIDevice *dev, unsigned int vector,
> +                                        MSIMessage *msg, bool masked)
>  {
>      VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
>      VirtIODevice *vdev = proxy->vdev;
> @@ -608,11 +608,11 @@ static int virtio_pci_set_guest_notifiers(void *opaque, 
> bool assign)
>      VirtIODevice *vdev = proxy->vdev;
>      int r, n;
>  
> -    /* Must unset mask notifier while guest notifier
> +    /* Must unset vector config notifier while guest notifier
>       * is still assigned */
>      if (!assign) {
> -         r = msix_unset_mask_notifier(&proxy->pci_dev);
> -            assert(r >= 0);
> +        r = msix_unset_config_notifiers(&proxy->pci_dev);
> +        assert(r >= 0);
>      }
>  
>      for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> @@ -626,11 +626,11 @@ static int virtio_pci_set_guest_notifiers(void *opaque, 
> bool assign)
>          }
>      }
>  
> -    /* Must set mask notifier after guest notifier
> +    /* Must set vector config notifier after guest notifier
>       * has been assigned */
>      if (assign) {
> -        r = msix_set_mask_notifier(&proxy->pci_dev,
> -                                   virtio_pci_mask_notifier);
> +        r = msix_set_config_notifiers(&proxy->pci_dev, NULL,
> +                                      virtio_pci_msi_vector_config);
>          if (r < 0) {
>              goto assign_error;
>          }
> @@ -645,8 +645,8 @@ assign_error:
>      }
>  
>      if (!assign) {
> -        msix_set_mask_notifier(&proxy->pci_dev,
> -                               virtio_pci_mask_notifier);
> +        msix_set_config_notifiers(&proxy->pci_dev, NULL,
> +                                  virtio_pci_msi_vector_config);
>      }
>      return r;
>  }
> -- 
> 1.7.3.4