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

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

[Michael S. Tsirkin]

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));
     if (r < 0) {
         return r;
     }
-- 
1.7.1.12.g42b7f

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

[Amit Shah]

On (Tue) May 25 2010 [17:00:43], Michael S. Tsirkin 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>

Acked-by: Amit Shah <amit.shah@redhat.com>

		Amit

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

[Juan Quintela]

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

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

[Michael S. Tsirkin]

On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote:
> "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.


I find msix_is_masked clearer here than true since you don't need
to look up definition to understand what this 'true' stands for.
The value is clear from code above. What do you think?

-- 
MST

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote:

>> 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.
>
>
> I find msix_is_masked clearer here than true since you don't need
> to look up definition to understand what this 'true' stands for.
> The value is clear from code above. What do you think?

I preffer the change, but it is up to you.

at that point, we are using !msix_masked() to mean "true"

i.e. we know that msix_masked() is false.  What you want to do is "mask".

Later, Juan.

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

[Michael S. Tsirkin]

On Tue, May 25, 2010 at 04:58:15PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote:
> 
> >> 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.
> >
> >
> > I find msix_is_masked clearer here than true since you don't need
> > to look up definition to understand what this 'true' stands for.
> > The value is clear from code above. What do you think?
> 
> I preffer the change, but it is up to you.
> 
> at that point, we are using !msix_masked() to mean "true"
> 
> i.e. we know that msix_masked() is false.  What you want to do is "mask".
> 
> Later, Juan.

Right. I guess I'll keep it as is, when I look at it with a fresh mind
next time, I'll clean it all up.

-- 
MST

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, May 25, 2010 at 04:58:15PM +0200, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote:
>> 
>> >> 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.
>> >
>> >
>> > I find msix_is_masked clearer here than true since you don't need
>> > to look up definition to understand what this 'true' stands for.
>> > The value is clear from code above. What do you think?
>> 
>> I preffer the change, but it is up to you.
>> 
>> at that point, we are using !msix_masked() to mean "true"
>> 
>> i.e. we know that msix_masked() is false.  What you want to do is "mask".
>> 
>> Later, Juan.
>
> Right. I guess I'll keep it as is, when I look at it with a fresh mind
> next time, I'll clean it all up.

ok with me.

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

[Gerd Hoffmann]

On 05/25/10 16:00, Michael S. Tsirkin 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>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

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

[Avi Kivity]

On 05/25/2010 05:00 PM, Michael S. Tsirkin 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.
>    

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function