Re: [PATCH] vhost-net: fix reversed logic in mask notifiers

[Juan Quintela <quintela@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> When guest notifier is assigned, we set mask notifier,
> which will assign kvm irqfd.
> When guest notifier is unassigned, mask notifier is unset,
> which should unassign kvm irqfd.
>
> The way to do this is to call mask notifier telling it to mask the vector.
> This, unless vector is already masked which unassigns irqfd already.
>
> The logic in unassign was reversed, which left kvm irqfd assigned.
>
> This patch is qemu-kvm only as irqfd is not upstream.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reported-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/msix.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 8f9a621..1398680 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -617,6 +617,7 @@ int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque)
>      assert(opaque);
>      assert(!dev->msix_mask_notifier_opaque[vector]);
>  
> +    /* Unmask the new notifier unless vector is masked. */
>      if (msix_is_masked(dev, vector)) {
>          return 0;
>      }
> @@ -638,12 +639,13 @@ int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
>      assert(dev->msix_mask_notifier);
>      assert(dev->msix_mask_notifier_opaque[vector]);
>  
> +    /* Mask the old notifier unless it is already masked. */
>      if (msix_is_masked(dev, vector)) {
>          return 0;
>      }
>      r = dev->msix_mask_notifier(dev, vector,
>                                  dev->msix_mask_notifier_opaque[vector],
> -                                msix_is_masked(dev, vector));
> +                                !msix_is_masked(dev, vector));

Why don't put just a 1 here?

we have:

if (msix_is_masked())
   return 0
r = msix_mask_notifier(....., !msix_is_masked());

i.e. at that point msix_is_masked() is false, or we really, really needs
locking.

Puttting a !foo, when we know that it needs to be an 1 looks strange.

Later, Juan.

PD.  Yes, I already asked in a previous version to just have two
methods, mask/unmask.  we now at call time which one we need.