vhost-net: fix reversed logic in mask notifiers

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 <[email protected]>

Reported-by Amit Shah <[email protected]>

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 <[email protected]>

Reported-by Amit Shah <[email protected]>

---
 hw/msix.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

hw/msix.c +1 -0

index 8f9a621..1398680 100644
@@ -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);

Quoted 32 lines · 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 <[email protected]>

Reported-by Amit Shah <[email protected]>

---
 hw/msix.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

hw/msix.c +1 -0

index 8f9a621..1398680 100644
@@ -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);

hw/msix.c int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector) · line 648

Quoted 10 lines

hw/msix.c +2 -1

     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.

Quoted 13 lines · Juan Quintela

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.

Quoted 2 lines

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?

Quoted 15 lines · Michael S. Tsirkin

Quoted 13 lines · Juan Quintela

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 26 lines · Juan Quintela

Quoted 15 lines · Michael S. Tsirkin

Quoted 13 lines · Juan Quintela

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

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<[email protected]>

Reported-by Amit Shah<[email protected]>

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.