[PATCH 0/5] qemu-kvm: MSI-X fixes

[PATCH 0/5] qemu-kvm: MSI-X fixes

[Jan Kiszka]

These are likely uncontroversial fixes to the MSI-X support in qemu-kvm,
broken out from my larger series. The last two patches is also upstream
material but don't apply due to kvm hooks around. They will be posted
separately for upstream again.

Jan Kiszka (5):
  qemu-kvm: msix: Don't fire notifier spuriously on set/unset
  qemu-kvm: msix: Only invoke msix_handle_mask_update on changes
  qemu-kvm: msix: Fire mask notifier on global mask changes
  msix: Don't process table changes while disabled
  msix: Prevent bogus mask updates on MMIO accesses

 hw/msix.c       |   74 ++++++++++++++++++++++++++++++++++--------------------
 hw/msix.h       |    2 +-
 hw/virtio-pci.c |    3 +-
 3 files changed, 49 insertions(+), 30 deletions(-)

-- 
1.7.3.4

[PATCH 1/5] qemu-kvm: msix: Don't fire notifier spuriously on set/unset

[Jan Kiszka]

If MSI-X is disabled or the global mask is set, don't fire the notifier
during registration or removal, reporting a wrong state.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msix.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 60d6d1e..6493937 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -563,10 +563,13 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
     int r, n;
     assert(!dev->msix_mask_notifier);
     dev->msix_mask_notifier = f;
-    for (n = 0; n < dev->msix_entries_nr; ++n) {
-        r = msix_set_mask_notifier_for_vector(dev, n);
-        if (r < 0) {
-            goto undo;
+    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);
+            if (r < 0) {
+                goto undo;
+            }
         }
     }
     return 0;
@@ -583,10 +586,13 @@ int msix_unset_mask_notifier(PCIDevice *dev)
 {
     int r, n;
     assert(dev->msix_mask_notifier);
-    for (n = 0; n < dev->msix_entries_nr; ++n) {
-        r = msix_unset_mask_notifier_for_vector(dev, n);
-        if (r < 0) {
-            goto undo;
+    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);
+            if (r < 0) {
+                goto undo;
+            }
         }
     }
     dev->msix_mask_notifier = NULL;
-- 
1.7.3.4

[PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Jan Kiszka]

Reorganize msix_mmio_writel so that msix_handle_mask_update is only
called on mask changes. Pass previous config space value to
msix_write_config so that it can check if a mask change took place.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msix.c       |   39 +++++++++++++++++++++++----------------
 hw/msix.h       |    2 +-
 hw/virtio-pci.c |    3 ++-
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 6493937..e1cc026 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -212,12 +212,12 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
 }
 
-static int msix_function_masked(PCIDevice *dev)
+static bool msix_function_masked(PCIDevice *dev)
 {
     return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
 }
 
-static int msix_is_masked(PCIDevice *dev, int vector)
+static bool msix_is_masked(PCIDevice *dev, int vector)
 {
     unsigned offset =
         vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
@@ -235,9 +235,10 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
 
 /* Handle MSI-X capability config write. */
 void msix_write_config(PCIDevice *dev, uint32_t addr,
-                       uint32_t val, int len)
+                       uint32_t old_val, int len)
 {
     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
+    bool was_masked;
     int vector;
 
     if (!range_covers_byte(addr, len, enable_pos)) {
@@ -250,12 +251,13 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
 
     pci_device_deassert_intx(dev);
 
-    if (msix_function_masked(dev)) {
-        return;
-    }
-
-    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
-        msix_handle_mask_update(dev, vector);
+    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)) {
+        for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
+            msix_handle_mask_update(dev, vector);
+        }
     }
 }
 
@@ -265,17 +267,22 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
     PCIDevice *dev = opaque;
     unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
     int vector = offset / PCI_MSIX_ENTRY_SIZE;
-    int was_masked = msix_is_masked(dev, vector);
+    bool was_masked = msix_is_masked(dev, vector);
+    int r;
+
     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 (was_masked != msix_is_masked(dev, vector) && dev->msix_mask_notifier) {
-        int r = dev->msix_mask_notifier(dev, vector,
-					msix_is_masked(dev, vector));
-        assert(r >= 0);
+
+    if (was_masked != msix_is_masked(dev, vector)) {
+        if (dev->msix_mask_notifier) {
+            r = dev->msix_mask_notifier(dev, vector,
+                                        msix_is_masked(dev, vector));
+            assert(r >= 0);
+        }
+        msix_handle_mask_update(dev, vector);
     }
-    msix_handle_mask_update(dev, vector);
 }
 
 static const MemoryRegionOps msix_mmio_ops = {
@@ -305,7 +312,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
     for (vector = 0; vector < nentries; ++vector) {
         unsigned offset =
             vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-        int was_masked = msix_is_masked(dev, vector);
+        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) {
diff --git a/hw/msix.h b/hw/msix.h
index 189bb3f..84a7e7f 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -9,7 +9,7 @@ int msix_init(PCIDevice *pdev, unsigned short nentries,
               unsigned bar_nr, unsigned bar_size);
 
 void msix_write_config(PCIDevice *pci_dev, uint32_t address,
-                       uint32_t val, int len);
+                       uint32_t old_val, int len);
 
 int msix_uninit(PCIDevice *d, MemoryRegion *bar);
 
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 6582099..858ca3f 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -493,6 +493,7 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
                                 uint32_t val, int len)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+    uint32_t old_val = pci_default_read_config(pci_dev, address, len);
 
     pci_default_write_config(pci_dev, address, val, len);
 
@@ -504,7 +505,7 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
                           proxy->vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
     }
 
-    msix_write_config(pci_dev, address, val, len);
+    msix_write_config(pci_dev, address, old_val, len);
 }
 
 static unsigned virtio_pci_get_features(void *opaque)
-- 
1.7.3.4

[PATCH 3/5] qemu-kvm: msix: Fire mask notifier on global mask changes

[Jan Kiszka]

Also invoke the mask notifier if the global MSI-X mask is modified. For
this purpose, we push the notifier call from the per-vector mask update
to the central msix_handle_mask_update.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msix.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index e1cc026..e351c68 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -227,7 +227,14 @@ static bool msix_is_masked(PCIDevice *dev, int vector)
 
 static void msix_handle_mask_update(PCIDevice *dev, int vector)
 {
-    if (!msix_is_masked(dev, vector) && msix_is_pending(dev, vector)) {
+    bool masked = msix_is_masked(dev, vector);
+    int ret;
+
+    if (dev->msix_mask_notifier) {
+        ret = dev->msix_mask_notifier(dev, vector, masked);
+        assert(ret >= 0);
+    }
+    if (!masked && msix_is_pending(dev, vector)) {
         msix_clr_pending(dev, vector);
         msix_notify(dev, vector);
     }
@@ -268,7 +275,6 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
     unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
     int vector = offset / PCI_MSIX_ENTRY_SIZE;
     bool was_masked = msix_is_masked(dev, vector);
-    int r;
 
     pci_set_long(dev->msix_table_page + offset, val);
     if (kvm_enabled() && kvm_irqchip_in_kernel()) {
@@ -276,11 +282,6 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
     }
 
     if (was_masked != msix_is_masked(dev, vector)) {
-        if (dev->msix_mask_notifier) {
-            r = dev->msix_mask_notifier(dev, vector,
-                                        msix_is_masked(dev, vector));
-            assert(r >= 0);
-        }
         msix_handle_mask_update(dev, vector);
     }
 }
-- 
1.7.3.4

[PATCH 4/5] msix: Don't process table changes while disabled

[Jan Kiszka]

As long as MSI-X is disabled, it's incorrect to invoke
msix_handle_mask_update on per-vector mask changes. That may misguide
the config notifier callback or spuriously trigger an MSI event.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msix.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index e351c68..f5c8d08 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -277,12 +277,16 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
     bool was_masked = msix_is_masked(dev, vector);
 
     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 (was_masked != msix_is_masked(dev, vector)) {
-        msix_handle_mask_update(dev, vector);
+    if (msix_enabled(dev)) {
+        if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+            kvm_msix_update(dev, vector, was_masked,
+                            msix_is_masked(dev, vector));
+        }
+
+        if (was_masked != msix_is_masked(dev, vector)) {
+            msix_handle_mask_update(dev, vector);
+        }
     }
 }
 
-- 
1.7.3.4

[PATCH 5/5] msix: Prevent bogus mask updates on MMIO accesses

[Jan Kiszka]

Only accesses to the MSI-X table must trigger a call to
msix_handle_mask_update or a notifier invocation.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msix.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index f5c8d08..0073ee5 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -278,7 +278,7 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
 
     pci_set_long(dev->msix_table_page + offset, val);
 
-    if (msix_enabled(dev)) {
+    if (msix_enabled(dev) && vector < dev->msix_entries_nr) {
         if (kvm_enabled() && kvm_irqchip_in_kernel()) {
             kvm_msix_update(dev, vector, was_masked,
                             msix_is_masked(dev, vector));
-- 
1.7.3.4

Re: [PATCH 1/5] qemu-kvm: msix: Don't fire notifier spuriously on set/unset

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 09:50:50AM +0200, Jan Kiszka wrote:
> If MSI-X is disabled or the global mask is set, don't fire the notifier
> during registration or removal, reporting a wrong state.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Hmm.  Could you please describe how does one trigger the incorrect behaviour?
Looking at the upstream code, I notice that msix_notify_if_unmasked
already calls msix_is_masked which checks the function mask.  And it
also checks msix_entry_used which, ATM, is only set when msix is
enabled.

> ---
>  hw/msix.c |   22 ++++++++++++++--------
>  1 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 60d6d1e..6493937 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -563,10 +563,13 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
>      int r, n;
>      assert(!dev->msix_mask_notifier);
>      dev->msix_mask_notifier = f;
> -    for (n = 0; n < dev->msix_entries_nr; ++n) {
> -        r = msix_set_mask_notifier_for_vector(dev, n);
> -        if (r < 0) {
> -            goto undo;
> +    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);
> +            if (r < 0) {
> +                goto undo;
> +            }
>          }
>      }
>      return 0;
> @@ -583,10 +586,13 @@ int msix_unset_mask_notifier(PCIDevice *dev)
>  {
>      int r, n;
>      assert(dev->msix_mask_notifier);
> -    for (n = 0; n < dev->msix_entries_nr; ++n) {
> -        r = msix_unset_mask_notifier_for_vector(dev, n);
> -        if (r < 0) {
> -            goto undo;
> +    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);
> +            if (r < 0) {
> +                goto undo;
> +            }
>          }
>      }
>      dev->msix_mask_notifier = NULL;
> -- 
> 1.7.3.4

Re: [PATCH 4/5] msix: Don't process table changes while disabled

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 09:50:53AM +0200, Jan Kiszka wrote:
> As long as MSI-X is disabled, it's incorrect to invoke
> msix_handle_mask_update on per-vector mask changes. That may misguide
> the config notifier callback or spuriously trigger an MSI event.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Same question as on the previous patch. At the moment, the only user
(virtio) seems to check msix_enabled before registering
notifiers. See virtio_pci_query_guest_notifiers.
Is there some way you see to trigger incorrect behaviour?

> ---
>  hw/msix.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index e351c68..f5c8d08 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -277,12 +277,16 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
>      bool was_masked = msix_is_masked(dev, vector);
>  
>      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 (was_masked != msix_is_masked(dev, vector)) {
> -        msix_handle_mask_update(dev, vector);
> +    if (msix_enabled(dev)) {
> +        if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +            kvm_msix_update(dev, vector, was_masked,
> +                            msix_is_masked(dev, vector));
> +        }
> +
> +        if (was_masked != msix_is_masked(dev, vector)) {
> +            msix_handle_mask_update(dev, vector);
> +        }
>      }
>  }
>  
> -- 
> 1.7.3.4

Re: [PATCH 4/5] msix: Don't process table changes while disabled

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 09:50:53AM +0200, Jan Kiszka wrote:
> As long as MSI-X is disabled, it's incorrect to invoke
> msix_handle_mask_update on per-vector mask changes. That may misguide
> the config notifier callback or spuriously trigger an MSI event.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

And msix_is_pending is cleared on vector unuse, so
how can a spurious MSI event happen?

> ---
>  hw/msix.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index e351c68..f5c8d08 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -277,12 +277,16 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
>      bool was_masked = msix_is_masked(dev, vector);
>  
>      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 (was_masked != msix_is_masked(dev, vector)) {
> -        msix_handle_mask_update(dev, vector);
> +    if (msix_enabled(dev)) {
> +        if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +            kvm_msix_update(dev, vector, was_masked,
> +                            msix_is_masked(dev, vector));
> +        }
> +
> +        if (was_masked != msix_is_masked(dev, vector)) {
> +            msix_handle_mask_update(dev, vector);
> +        }
>      }
>  }
>  
> -- 
> 1.7.3.4

Re: [PATCH 1/5] qemu-kvm: msix: Don't fire notifier spuriously on set/unset

[Jan Kiszka]

On 2011-10-18 13:06, Michael S. Tsirkin wrote:
> On Tue, Oct 18, 2011 at 09:50:50AM +0200, Jan Kiszka wrote:
>> If MSI-X is disabled or the global mask is set, don't fire the notifier
>> during registration or removal, reporting a wrong state.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Hmm.  Could you please describe how does one trigger the incorrect behaviour?
> Looking at the upstream code, I notice that msix_notify_if_unmasked
> already calls msix_is_masked which checks the function mask.  And it
> also checks msix_entry_used which, ATM, is only set when msix is
> enabled.

Right, that's not yet an issue. Will be required when we get rid of used
vector tracking. Will adjust the commit log accordingly and move it back
into that context.

Jan

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

Re: [PATCH 4/5] msix: Don't process table changes while disabled

[Jan Kiszka]

On 2011-10-18 13:18, Michael S. Tsirkin wrote:
> On Tue, Oct 18, 2011 at 09:50:53AM +0200, Jan Kiszka wrote:
>> As long as MSI-X is disabled, it's incorrect to invoke
>> msix_handle_mask_update on per-vector mask changes. That may misguide
>> the config notifier callback or spuriously trigger an MSI event.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Same question as on the previous patch. At the moment, the only user
> (virtio) seems to check msix_enabled before registering
> notifiers. See virtio_pci_query_guest_notifiers.
> Is there some way you see to trigger incorrect behaviour?

Then it's not an urgent fix, just a conceptual one (the user should not
have to worry about enable/disabled, we want the core to do the boring
jobs).

Jan

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

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
> called on mask changes. Pass previous config space value to
> msix_write_config so that it can check if a mask change took place.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

What we did in other cases is track the old value in device state.
This makes the API easier to use correctly.
I'm testing the following as a replacement - any comments?

diff --git a/hw/msix.c b/hw/msix.c
index b15bafc..655a600 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
     /* Make flags bit writable. */
     pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
 	    MSIX_MASKALL_MASK;
+    pdev->msix_function_masked = false;
     return 0;
 }
 
@@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
 }
 
-static int msix_function_masked(PCIDevice *dev)
-{
-    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
-}
-
 static int msix_is_masked(PCIDevice *dev, int vector)
 {
     unsigned offset =
         vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-    return msix_function_masked(dev) ||
+    return dev->msix_function_masked ||
 	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
 }
 
@@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
 {
     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     int vector;
+    bool fmsk;
 
     if (!range_covers_byte(addr, len, enable_pos)) {
         return;
@@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
 
     pci_device_deassert_intx(dev);
 
-    if (msix_function_masked(dev)) {
+    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
+    if (dev->msix_function_masked == fmsk) {
         return;
     }
+    dev->msix_function_masked = fmsk;
 
     for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
         msix_handle_mask_update(dev, vector);
     }
diff --git a/hw/pci.h b/hw/pci.h
index 86a81c8..5b67a6b 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -178,6 +178,8 @@ struct PCIDevice {
     unsigned *msix_entry_used;
     /* Region including the MSI-X table */
     uint32_t msix_bar_size;
+    /* MSIX function mask */
+    bool msix_function_masked;
     /* Version id needed for VMState */
     int32_t version_id;
 

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Jan Kiszka]

On 2011-10-18 13:43, Michael S. Tsirkin wrote:
> On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
>> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
>> called on mask changes. Pass previous config space value to
>> msix_write_config so that it can check if a mask change took place.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> What we did in other cases is track the old value in device state.
> This makes the API easier to use correctly.
> I'm testing the following as a replacement - any comments?

No concerns about caching the mask state, but...

> 
> diff --git a/hw/msix.c b/hw/msix.c
> index b15bafc..655a600 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>      /* Make flags bit writable. */
>      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>  	    MSIX_MASKALL_MASK;
> +    pdev->msix_function_masked = false;
>      return 0;
>  }
>  
> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>      *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
>  }
>  
> -static int msix_function_masked(PCIDevice *dev)
> -{
> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> -}
> -
>  static int msix_is_masked(PCIDevice *dev, int vector)
>  {
>      unsigned offset =
>          vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> -    return msix_function_masked(dev) ||
> +    return dev->msix_function_masked ||
>  	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  }
>  
> @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>  {
>      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
>      int vector;
> +    bool fmsk;
>  
>      if (!range_covers_byte(addr, len, enable_pos)) {
>          return;
> @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>  
>      pci_device_deassert_intx(dev);
>  
> -    if (msix_function_masked(dev)) {
> +    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> +    if (dev->msix_function_masked == fmsk) {

...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) ->
MSIX_ENABLE_MASK.

OTOH, there will only be one user of msix_write_config remaining:
pci_default_write_config. So there are not that many users to check for
correct API use.

Jan

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

Re: [PATCH 4/5] msix: Don't process table changes while disabled

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 01:35:37PM +0200, Jan Kiszka wrote:
> On 2011-10-18 13:18, Michael S. Tsirkin wrote:
> > On Tue, Oct 18, 2011 at 09:50:53AM +0200, Jan Kiszka wrote:
> >> As long as MSI-X is disabled, it's incorrect to invoke
> >> msix_handle_mask_update on per-vector mask changes. That may misguide
> >> the config notifier callback or spuriously trigger an MSI event.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > Same question as on the previous patch. At the moment, the only user
> > (virtio) seems to check msix_enabled before registering
> > notifiers. See virtio_pci_query_guest_notifiers.
> > Is there some way you see to trigger incorrect behaviour?
> 
> Then it's not an urgent fix, just a conceptual one (the user should not
> have to worry about enable/disabled, we want the core to do the boring
> jobs).
> 
> Jan

Yes, I agree the callback APIs could use some cleanup.
Need to make sure the patch does not cause
unbalanced mask/unmask calls. I need to look closer
to check that's still the case.

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

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 01:54:23PM +0200, Jan Kiszka wrote:
> On 2011-10-18 13:43, Michael S. Tsirkin wrote:
> > On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
> >> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
> >> called on mask changes. Pass previous config space value to
> >> msix_write_config so that it can check if a mask change took place.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > What we did in other cases is track the old value in device state.
> > This makes the API easier to use correctly.
> > I'm testing the following as a replacement - any comments?
> 
> No concerns about caching the mask state, but...
> 
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index b15bafc..655a600 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> >      /* Make flags bit writable. */
> >      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> >  	    MSIX_MASKALL_MASK;
> > +    pdev->msix_function_masked = false;
> >      return 0;
> >  }
> >  
> > @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >      *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
> >  }
> >  
> > -static int msix_function_masked(PCIDevice *dev)
> > -{
> > -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> > -}
> > -
> >  static int msix_is_masked(PCIDevice *dev, int vector)
> >  {
> >      unsigned offset =
> >          vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> > -    return msix_function_masked(dev) ||
> > +    return dev->msix_function_masked ||
> >  	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >  }
> >  
> > @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >  {
> >      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> >      int vector;
> > +    bool fmsk;
> >  
> >      if (!range_covers_byte(addr, len, enable_pos)) {
> >          return;
> > @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >  
> >      pci_device_deassert_intx(dev);
> >  
> > -    if (msix_function_masked(dev)) {
> > +    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> > +    if (dev->msix_function_masked == fmsk) {
> 
> ...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) ->
> MSIX_ENABLE_MASK.
> 

Could you clarify please?
    if (!msix_enabled(dev)) {
        return;
    }
is at start of this function so nothing happens if
MSIX is disabled.

> OTOH, there will only be one user of msix_write_config remaining:
> pci_default_write_config. So there are not that many users to check for
> correct API use.
> 
> Jan

I'm looking to fix bugs first, add features second.

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

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Jan Kiszka]

On 2011-10-18 14:11, Michael S. Tsirkin wrote:
> On Tue, Oct 18, 2011 at 01:54:23PM +0200, Jan Kiszka wrote:
>> On 2011-10-18 13:43, Michael S. Tsirkin wrote:
>>> On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
>>>> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
>>>> called on mask changes. Pass previous config space value to
>>>> msix_write_config so that it can check if a mask change took place.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> What we did in other cases is track the old value in device state.
>>> This makes the API easier to use correctly.
>>> I'm testing the following as a replacement - any comments?
>>
>> No concerns about caching the mask state, but...
>>
>>>
>>> diff --git a/hw/msix.c b/hw/msix.c
>>> index b15bafc..655a600 100644
>>> --- a/hw/msix.c
>>> +++ b/hw/msix.c
>>> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>>>      /* Make flags bit writable. */
>>>      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>>>  	    MSIX_MASKALL_MASK;
>>> +    pdev->msix_function_masked = false;
>>>      return 0;
>>>  }
>>>  
>>> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>>>      *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
>>>  }
>>>  
>>> -static int msix_function_masked(PCIDevice *dev)
>>> -{
>>> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
>>> -}
>>> -
>>>  static int msix_is_masked(PCIDevice *dev, int vector)
>>>  {
>>>      unsigned offset =
>>>          vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
>>> -    return msix_function_masked(dev) ||
>>> +    return dev->msix_function_masked ||
>>>  	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
>>>  }
>>>  
>>> @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>>>  {
>>>      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
>>>      int vector;
>>> +    bool fmsk;
>>>  
>>>      if (!range_covers_byte(addr, len, enable_pos)) {
>>>          return;
>>> @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>>>  
>>>      pci_device_deassert_intx(dev);
>>>  
>>> -    if (msix_function_masked(dev)) {
>>> +    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
>>> +    if (dev->msix_function_masked == fmsk) {
>>
>> ...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) ->
>> MSIX_ENABLE_MASK.
>>
> 
> Could you clarify please?
>     if (!msix_enabled(dev)) {
>         return;
>     }
> is at start of this function so nothing happens if
> MSIX is disabled.

OK, missed that. But let's have a look again:

ENABLE  -> 0 => msix_function_masked := false
0 -> ENABLE  => msix_function_masked remains false, thus no firing
(while we did fire in the past and should do so in the future)

Jan

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

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 02:22:38PM +0200, Jan Kiszka wrote:
> On 2011-10-18 14:11, Michael S. Tsirkin wrote:
> > On Tue, Oct 18, 2011 at 01:54:23PM +0200, Jan Kiszka wrote:
> >> On 2011-10-18 13:43, Michael S. Tsirkin wrote:
> >>> On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
> >>>> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
> >>>> called on mask changes. Pass previous config space value to
> >>>> msix_write_config so that it can check if a mask change took place.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>> What we did in other cases is track the old value in device state.
> >>> This makes the API easier to use correctly.
> >>> I'm testing the following as a replacement - any comments?
> >>
> >> No concerns about caching the mask state, but...
> >>
> >>>
> >>> diff --git a/hw/msix.c b/hw/msix.c
> >>> index b15bafc..655a600 100644
> >>> --- a/hw/msix.c
> >>> +++ b/hw/msix.c
> >>> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> >>>      /* Make flags bit writable. */
> >>>      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> >>>  	    MSIX_MASKALL_MASK;
> >>> +    pdev->msix_function_masked = false;
> >>>      return 0;
> >>>  }
> >>>  
> >>> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >>>      *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
> >>>  }
> >>>  
> >>> -static int msix_function_masked(PCIDevice *dev)
> >>> -{
> >>> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> >>> -}
> >>> -
> >>>  static int msix_is_masked(PCIDevice *dev, int vector)
> >>>  {
> >>>      unsigned offset =
> >>>          vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> >>> -    return msix_function_masked(dev) ||
> >>> +    return dev->msix_function_masked ||
> >>>  	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >>>  }
> >>>  
> >>> @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >>>  {
> >>>      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> >>>      int vector;
> >>> +    bool fmsk;
> >>>  
> >>>      if (!range_covers_byte(addr, len, enable_pos)) {
> >>>          return;
> >>> @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >>>  
> >>>      pci_device_deassert_intx(dev);
> >>>  
> >>> -    if (msix_function_masked(dev)) {
> >>> +    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> >>> +    if (dev->msix_function_masked == fmsk) {
> >>
> >> ...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) ->
> >> MSIX_ENABLE_MASK.
> >>
> > 
> > Could you clarify please?
> >     if (!msix_enabled(dev)) {
> >         return;
> >     }
> > is at start of this function so nothing happens if
> > MSIX is disabled.
> 
> OK, missed that. But let's have a look again:
> 
> ENABLE  -> 0 => msix_function_masked := false
> 0 -> ENABLE  => msix_function_masked remains false, thus no firing
> (while we did fire in the past and should do so in the future)
> 
> Jan

I'm not sure I get it. You mean mask was set when we enabled?
Then we can't send any interrupts so handlers do not fire.
What's wrong?


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

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Jan Kiszka]

On 2011-10-18 14:37, Michael S. Tsirkin wrote:
> On Tue, Oct 18, 2011 at 02:22:38PM +0200, Jan Kiszka wrote:
>> On 2011-10-18 14:11, Michael S. Tsirkin wrote:
>>> On Tue, Oct 18, 2011 at 01:54:23PM +0200, Jan Kiszka wrote:
>>>> On 2011-10-18 13:43, Michael S. Tsirkin wrote:
>>>>> On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
>>>>>> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
>>>>>> called on mask changes. Pass previous config space value to
>>>>>> msix_write_config so that it can check if a mask change took place.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> What we did in other cases is track the old value in device state.
>>>>> This makes the API easier to use correctly.
>>>>> I'm testing the following as a replacement - any comments?
>>>>
>>>> No concerns about caching the mask state, but...
>>>>
>>>>>
>>>>> diff --git a/hw/msix.c b/hw/msix.c
>>>>> index b15bafc..655a600 100644
>>>>> --- a/hw/msix.c
>>>>> +++ b/hw/msix.c
>>>>> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>>>>>      /* Make flags bit writable. */
>>>>>      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>>>>>  	    MSIX_MASKALL_MASK;
>>>>> +    pdev->msix_function_masked = false;
>>>>>      return 0;
>>>>>  }
>>>>>  
>>>>> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>>>>>      *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
>>>>>  }
>>>>>  
>>>>> -static int msix_function_masked(PCIDevice *dev)
>>>>> -{
>>>>> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
>>>>> -}
>>>>> -
>>>>>  static int msix_is_masked(PCIDevice *dev, int vector)
>>>>>  {
>>>>>      unsigned offset =
>>>>>          vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
>>>>> -    return msix_function_masked(dev) ||
>>>>> +    return dev->msix_function_masked ||
>>>>>  	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
>>>>>  }
>>>>>  
>>>>> @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>>>>>  {
>>>>>      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
>>>>>      int vector;
>>>>> +    bool fmsk;
>>>>>  
>>>>>      if (!range_covers_byte(addr, len, enable_pos)) {
>>>>>          return;
>>>>> @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>>>>>  
>>>>>      pci_device_deassert_intx(dev);
>>>>>  
>>>>> -    if (msix_function_masked(dev)) {
>>>>> +    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
>>>>> +    if (dev->msix_function_masked == fmsk) {
>>>>
>>>> ...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) ->
>>>> MSIX_ENABLE_MASK.
>>>>
>>>
>>> Could you clarify please?
>>>     if (!msix_enabled(dev)) {
>>>         return;
>>>     }
>>> is at start of this function so nothing happens if
>>> MSIX is disabled.
>>
>> OK, missed that. But let's have a look again:
>>
>> ENABLE  -> 0 => msix_function_masked := false
>> 0 -> ENABLE  => msix_function_masked remains false, thus no firing
>> (while we did fire in the past and should do so in the future)
>>
>> Jan
> 
> I'm not sure I get it. You mean mask was set when we enabled?
> Then we can't send any interrupts so handlers do not fire.
> What's wrong?

I mean mask was NOT set when disabling, and will remain unset when
enabling. Then we should fire the notifier although the mask bit was not
changed physically. There should be no vectors pending at this point as
they are supposed to be cleared when we disable MSI-X (as far as I
understand the spec).

Jan

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

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 02:44:12PM +0200, Jan Kiszka wrote:
> On 2011-10-18 14:37, Michael S. Tsirkin wrote:
> > On Tue, Oct 18, 2011 at 02:22:38PM +0200, Jan Kiszka wrote:
> >> On 2011-10-18 14:11, Michael S. Tsirkin wrote:
> >>> On Tue, Oct 18, 2011 at 01:54:23PM +0200, Jan Kiszka wrote:
> >>>> On 2011-10-18 13:43, Michael S. Tsirkin wrote:
> >>>>> On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
> >>>>>> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
> >>>>>> called on mask changes. Pass previous config space value to
> >>>>>> msix_write_config so that it can check if a mask change took place.
> >>>>>>
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>
> >>>>> What we did in other cases is track the old value in device state.
> >>>>> This makes the API easier to use correctly.
> >>>>> I'm testing the following as a replacement - any comments?
> >>>>
> >>>> No concerns about caching the mask state, but...
> >>>>
> >>>>>
> >>>>> diff --git a/hw/msix.c b/hw/msix.c
> >>>>> index b15bafc..655a600 100644
> >>>>> --- a/hw/msix.c
> >>>>> +++ b/hw/msix.c
> >>>>> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> >>>>>      /* Make flags bit writable. */
> >>>>>      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> >>>>>  	    MSIX_MASKALL_MASK;
> >>>>> +    pdev->msix_function_masked = false;
> >>>>>      return 0;
> >>>>>  }
> >>>>>  
> >>>>> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >>>>>      *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
> >>>>>  }
> >>>>>  
> >>>>> -static int msix_function_masked(PCIDevice *dev)
> >>>>> -{
> >>>>> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> >>>>> -}
> >>>>> -
> >>>>>  static int msix_is_masked(PCIDevice *dev, int vector)
> >>>>>  {
> >>>>>      unsigned offset =
> >>>>>          vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> >>>>> -    return msix_function_masked(dev) ||
> >>>>> +    return dev->msix_function_masked ||
> >>>>>  	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >>>>>  }
> >>>>>  
> >>>>> @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >>>>>  {
> >>>>>      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> >>>>>      int vector;
> >>>>> +    bool fmsk;
> >>>>>  
> >>>>>      if (!range_covers_byte(addr, len, enable_pos)) {
> >>>>>          return;
> >>>>> @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >>>>>  
> >>>>>      pci_device_deassert_intx(dev);
> >>>>>  
> >>>>> -    if (msix_function_masked(dev)) {
> >>>>> +    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> >>>>> +    if (dev->msix_function_masked == fmsk) {
> >>>>
> >>>> ...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) ->
> >>>> MSIX_ENABLE_MASK.
> >>>>
> >>>
> >>> Could you clarify please?
> >>>     if (!msix_enabled(dev)) {
> >>>         return;
> >>>     }
> >>> is at start of this function so nothing happens if
> >>> MSIX is disabled.
> >>
> >> OK, missed that. But let's have a look again:
> >>
> >> ENABLE  -> 0 => msix_function_masked := false
> >> 0 -> ENABLE  => msix_function_masked remains false, thus no firing
> >> (while we did fire in the past and should do so in the future)
> >>
> >> Jan
> > 
> > I'm not sure I get it. You mean mask was set when we enabled?
> > Then we can't send any interrupts so handlers do not fire.
> > What's wrong?
> 
> I mean mask was NOT set when disabling, and will remain unset when
> enabling. Then we should fire the notifier although the mask bit was not
> changed physically.
> There should be no vectors pending at this point as
> they are supposed to be cleared when we disable MSI-X (as far as I
> understand the spec).
> 
> Jan
> 

Ah. Right. Good catch. It's probably best to just make that
state be !enabled || masked. Like the below? (compiled only):

--->

msix: track function masked in pci device state

Only go over the table when function is masked.
This optimization is not really important for qemu.git but helps
qemu-kvm.git.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/msix.c b/hw/msix.c
index b15bafc..63b41b9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
     /* Make flags bit writable. */
     pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
 	    MSIX_MASKALL_MASK;
+    pdev->msix_function_masked = true;
     return 0;
 }
 
@@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
 }
 
-static int msix_function_masked(PCIDevice *dev)
-{
-    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
-}
-
 static int msix_is_masked(PCIDevice *dev, int vector)
 {
     unsigned offset =
         vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-    return msix_function_masked(dev) ||
+    return dev->msix_function_masked ||
 	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
 }
 
@@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
     }
 }
 
+static void msix_update_function_masked(PCIDevice *dev)
+{
+    dev->msix_function_masked = !msix_enabled(dev) ||
+        (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK);
+}
+
 /* Handle MSI-X capability config write. */
 void msix_write_config(PCIDevice *dev, uint32_t addr,
                        uint32_t val, int len)
 {
     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     int vector;
+    bool was_masked;
 
     if (!range_covers_byte(addr, len, enable_pos)) {
         return;
     }
 
+    was_masked = dev->msix_function_masked;
+    msix_update_function_masked(dev);
+
     if (!msix_enabled(dev)) {
         return;
     }
 
     pci_device_deassert_intx(dev);
 
-    if (msix_function_masked(dev)) {
+    if (dev->msix_function_masked == was_masked) {
         return;
     }
 
@@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
     msix_free_irq_entries(dev);
     qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
     qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
+    msix_update_function_masked(dev);
 }
 
 /* Does device support MSI-X? */
diff --git a/hw/pci.h b/hw/pci.h
index 86a81c8..da4c9c3 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -178,6 +178,8 @@ struct PCIDevice {
     unsigned *msix_entry_used;
     /* Region including the MSI-X table */
     uint32_t msix_bar_size;
+    /* MSIX function mask set or MSIX disabled */
+    bool msix_function_masked;
     /* Version id needed for VMState */
     int32_t version_id;
 

-- 
MST

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Michael S. Tsirkin]

On Tue, Oct 18, 2011 at 03:20:43PM +0200, Michael S. Tsirkin wrote:
> Ah. Right. Good catch. It's probably best to just make that
> state be !enabled || masked. Like the below? (compiled only):
> 
> --->
> 
> msix: track function masked in pci device state
> 
> Only go over the table when function is masked.
> This optimization is not really important for qemu.git but helps
> qemu-kvm.git.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

...

> diff --git a/hw/pci.h b/hw/pci.h
> index 86a81c8..da4c9c3 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -178,6 +178,8 @@ struct PCIDevice {
>      unsigned *msix_entry_used;
>      /* Region including the MSI-X table */
>      uint32_t msix_bar_size;
> +    /* MSIX function mask set or MSIX disabled */
> +    bool msix_function_masked;
>      /* Version id needed for VMState */
>      int32_t version_id;

BTW, since code checks msix_function_masked consistently,
this also covers the case your other patch addressed,
where there's a table access while msix is disabled, right?

-- 
MST

Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

[Jan Kiszka]

On 2011-10-18 15:49, Michael S. Tsirkin wrote:
> On Tue, Oct 18, 2011 at 03:20:43PM +0200, Michael S. Tsirkin wrote:
>> Ah. Right. Good catch. It's probably best to just make that
>> state be !enabled || masked. Like the below? (compiled only):
>>
>> --->
>>
>> msix: track function masked in pci device state
>>
>> Only go over the table when function is masked.
>> This optimization is not really important for qemu.git but helps
>> qemu-kvm.git.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ...
> 
>> diff --git a/hw/pci.h b/hw/pci.h
>> index 86a81c8..da4c9c3 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -178,6 +178,8 @@ struct PCIDevice {
>>      unsigned *msix_entry_used;
>>      /* Region including the MSI-X table */
>>      uint32_t msix_bar_size;
>> +    /* MSIX function mask set or MSIX disabled */
>> +    bool msix_function_masked;
>>      /* Version id needed for VMState */
>>      int32_t version_id;
> 
> BTW, since code checks msix_function_masked consistently,
> this also covers the case your other patch addressed,
> where there's a table access while msix is disabled, right?

Looks like. Not sure about potential side effects of this change to
msi_is_masked, but conceptually there should be none.

Jan

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