[RFC,23/45] qemu-kvm: Rework MSI-X mask notifier to generic MSI config notifiers

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 <jan.kiszka@siemens.com>
---
 hw/msix.c       |  119 +++++++++++++++++++++++++++++++++++++-----------------
 hw/msix.h       |    6 ++-
 hw/pci.h        |    8 ++-
 hw/virtio-pci.c |   24 ++++++------
 4 files changed, 102 insertions(+), 55 deletions(-)

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 <jan.kiszka@siemens.com>

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

On 2011-10-17 13:40, Michael S. Tsirkin wrote:
> 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 <jan.kiszka@siemens.com>
> 
> Passing message, always, does not seem to make sense: message is only
> valid if it is unmasked.

If we go from unmasked to masked, the consumer could just ignore the
message.

> 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.

That's in fact a possible optimization (only invoke the callback on mask
transitions). Not sure if that applies to MSI as well, probably not. To
have common types, I would prefer to stay with vector config notifiers
as name then.

Jan

On Mon, Oct 17, 2011 at 01:45:04PM +0200, Jan Kiszka wrote:
> On 2011-10-17 13:40, Michael S. Tsirkin wrote:
> > 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 <jan.kiszka@siemens.com>
> > 
> > Passing message, always, does not seem to make sense: message is only
> > valid if it is unmasked.
> 
> If we go from unmasked to masked, the consumer could just ignore the
> message.

Why don't we let the consumer get the message if it needs it?

> > 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.
> 
> That's in fact a possible optimization (only invoke the callback on mask
> transitions).

Further, it is one that is already implemented.
So I would prefer not to add work by removing it :)

> Not sure if that applies to MSI as well, probably not.

Probably not. However, if per vector masking is
supported, and while vector is masked, the address/
data values might not make any sense.

So I think even msi users needs to know about masked state.

> To
> have common types, I would prefer to stay with vector config notifiers
> as name then.
> 
> Jan

So we pass in nonsense values and ask all users to know about MSIX rules.
Ugh.

I do realize msi might change the vector without masking.
We can either artificially call mask before value change
and unmask after, or use 3 notifiers: mask,unmask,config.
Add a comment that config is invoked when configuration
for an unmasked vector is changed, and that
it can only happen for msi, not msix.

This also removes the need to handle enable/disable specially:
you simply pretend all vectors are masked.


> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

On 2011-10-17 14:39, Michael S. Tsirkin wrote:
> On Mon, Oct 17, 2011 at 01:45:04PM +0200, Jan Kiszka wrote:
>> On 2011-10-17 13:40, Michael S. Tsirkin wrote:
>>> 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 <jan.kiszka@siemens.com>
>>>
>>> Passing message, always, does not seem to make sense: message is only
>>> valid if it is unmasked.
>>
>> If we go from unmasked to masked, the consumer could just ignore the
>> message.
> 
> Why don't we let the consumer get the message if it needs it?

Because most consumer will need and because I want to keep the API simple.

> 
>>> 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.
>>
>> That's in fact a possible optimization (only invoke the callback on mask
>> transitions).
> 
> Further, it is one that is already implemented.
> So I would prefer not to add work by removing it :)

Generalization to cover MSI requires some changes. Unneeded behavioral
changes back and forth should and will of course be avoided. I will
rework this.

> 
>> Not sure if that applies to MSI as well, probably not.
> 
> Probably not. However, if per vector masking is
> supported, and while vector is masked, the address/
> data values might not make any sense.
> 
> So I think even msi users needs to know about masked state.

Yes, and they get this information via the config notifier.

> 
>> To
>> have common types, I would prefer to stay with vector config notifiers
>> as name then.
>>
>> Jan
> 
> So we pass in nonsense values and ask all users to know about MSIX rules.
> Ugh.
> 
> I do realize msi might change the vector without masking.
> We can either artificially call mask before value change
> and unmask after, or use 3 notifiers: mask,unmask,config.
> Add a comment that config is invoked when configuration
> for an unmasked vector is changed, and that
> it can only happen for msi, not msix.

I see no need in complicating the API like this. MSI-X still needs the
config information on unmask, so let's just consistently pass it via the
unified config notifier instead of forcing the consumers to create yet
two more handlers. I really do not see the benefit for the consumer.

Jan

On Mon, Oct 17, 2011 at 09:08:58PM +0200, Jan Kiszka wrote:
> On 2011-10-17 14:39, Michael S. Tsirkin wrote:
> > On Mon, Oct 17, 2011 at 01:45:04PM +0200, Jan Kiszka wrote:
> >> On 2011-10-17 13:40, Michael S. Tsirkin wrote:
> >>> 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 <jan.kiszka@siemens.com>
> >>>
> >>> Passing message, always, does not seem to make sense: message is only
> >>> valid if it is unmasked.
> >>
> >> If we go from unmasked to masked, the consumer could just ignore the
> >> message.
> > 
> > Why don't we let the consumer get the message if it needs it?
> 
> Because most consumer will need and because I want to keep the API simple.

The API seems to get more complex, as you are passing
in fields which are only valid sometimes.

> > 
> >>> 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.
> >>
> >> That's in fact a possible optimization (only invoke the callback on mask
> >> transitions).
> > 
> > Further, it is one that is already implemented.
> > So I would prefer not to add work by removing it :)
> 
> Generalization to cover MSI requires some changes. Unneeded behavioral
> changes back and forth should and will of course be avoided. I will
> rework this.
> 
> > 
> >> Not sure if that applies to MSI as well, probably not.
> > 
> > Probably not. However, if per vector masking is
> > supported, and while vector is masked, the address/
> > data values might not make any sense.
> > 
> > So I think even msi users needs to know about masked state.
> 
> Yes, and they get this information via the config notifier.
> 
> > 
> >> To
> >> have common types, I would prefer to stay with vector config notifiers
> >> as name then.
> >>
> >> Jan
> > 
> > So we pass in nonsense values and ask all users to know about MSIX rules.
> > Ugh.
> > 
> > I do realize msi might change the vector without masking.
> > We can either artificially call mask before value change
> > and unmask after, or use 3 notifiers: mask,unmask,config.
> > Add a comment that config is invoked when configuration
> > for an unmasked vector is changed, and that
> > it can only happen for msi, not msix.
> 
> I see no need in complicating the API like this. MSI-X still needs the
> config information on unmask, so let's just consistently pass it via the
> unified config notifier instead of forcing the consumers to create yet
> two more handlers. I really do not see the benefit for the consumer.
> 
> Jan
> 

The benefit is a clearer API, where all parameters you get are valid,
so you do not need to go read the spec to see what is OK to use.
Generally, encoding events in flags is more error
prone than using different notifiers for different events.

E.g. _unmask and _mask make
it obvious that they are called on mask and on unmask
respectively.
OTOH _config_change(bool mask) is unclear: is 'mask' the new
state or the old state?

It might be just my taste, but I usually prefer multiple
functions doing one thing each rather than a single
function doing multiple things. It shouldn't be too hard ...

On 2011-10-18 15:46, Michael S. Tsirkin wrote:
>>>> To
>>>> have common types, I would prefer to stay with vector config notifiers
>>>> as name then.
>>>>
>>>> Jan
>>>
>>> So we pass in nonsense values and ask all users to know about MSIX rules.
>>> Ugh.
>>>
>>> I do realize msi might change the vector without masking.
>>> We can either artificially call mask before value change
>>> and unmask after, or use 3 notifiers: mask,unmask,config.
>>> Add a comment that config is invoked when configuration
>>> for an unmasked vector is changed, and that
>>> it can only happen for msi, not msix.
>>
>> I see no need in complicating the API like this. MSI-X still needs the
>> config information on unmask, so let's just consistently pass it via the
>> unified config notifier instead of forcing the consumers to create yet
>> two more handlers. I really do not see the benefit for the consumer.
>>
>> Jan
>>
> 
> The benefit is a clearer API, where all parameters you get are valid,
> so you do not need to go read the spec to see what is OK to use.
> Generally, encoding events in flags is more error
> prone than using different notifiers for different events.
> 
> E.g. _unmask and _mask make
> it obvious that they are called on mask and on unmask
> respectively.
> OTOH _config_change(bool mask) is unclear: is 'mask' the new
> state or the old state?
> 
> It might be just my taste, but I usually prefer multiple
> functions doing one thing each rather than a single
> function doing multiple things. It shouldn't be too hard ...

The impact on the user side (device models) will be larger while the
work in those different handlers will be widely the same (check my
series, e.g. the virtio handler). But it's still just a guess of mine as
well.

Jan