[Qemu-devel] [PATCH 1/5] msix: fix mask bit state after reset

[Qemu-devel] [PATCH 1/5] msix: fix mask bit state after reset

[Michael S. Tsirkin]

PCI spec states that mask bit must be 1 after reset.
Make it so.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index d499441..45f83dd 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -217,6 +217,15 @@ void msix_mmio_map(PCIDevice *d, int region_num,
                                  d->msix_mmio_index);
 }
 
+static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
+{
+    int vector;
+    for (vector = 0; vector < nentries; ++vector) {
+        unsigned offset = vector * MSIX_ENTRY_SIZE + MSIX_VECTOR_CTRL;
+        dev->msix_table_page[offset] |= MSIX_VECTOR_MASK;
+    }
+}
+
 /* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
  * modified, it should be retrieved with msix_bar_size. */
 int msix_init(struct PCIDevice *dev, unsigned short nentries,
@@ -234,6 +243,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
                                         sizeof *dev->msix_entry_used);
 
     dev->msix_table_page = qemu_mallocz(MSIX_PAGE_SIZE);
+    msix_mask_all(dev, nentries);
 
     dev->msix_mmio_index = cpu_register_io_memory(msix_mmio_read,
                                                   msix_mmio_write, dev);
@@ -353,6 +363,7 @@ void msix_reset(PCIDevice *dev)
     msix_free_irq_entries(dev);
     dev->config[dev->msix_cap + MSIX_ENABLE_OFFSET] &= MSIX_ENABLE_MASK;
     memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
+    msix_mask_all(dev, dev->msix_entries_nr);
 }
 
 /* PCI spec suggests that devices make it possible for software to configure
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH 2/5] msix: fix reset value for enable bit

[Michael S. Tsirkin]

On reset, we currently clear all bits in msix control register *except*
enable bit.  This is wrong: the spec says we should clear writeable
bits: function mask and enable bit.
Correct this.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 45f83dd..785e097 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -361,7 +361,8 @@ void msix_reset(PCIDevice *dev)
     if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
         return;
     msix_free_irq_entries(dev);
-    dev->config[dev->msix_cap + MSIX_ENABLE_OFFSET] &= MSIX_ENABLE_MASK;
+    dev->config[dev->msix_cap + MSIX_ENABLE_OFFSET] &=
+	    ~dev->wmask[dev->msix_cap + MSIX_ENABLE_OFFSET];
     memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
     msix_mask_all(dev, dev->msix_entries_nr);
 }
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH 3/5] msix: macro rename for function mask support

[Michael S. Tsirkin]

rename ENABLE_OFFSET -> CONTROL_OFFSET, since
same byte includes function mask.
This is in preparation for function mask support.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 785e097..07111d0 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -27,8 +27,8 @@
 #define MSIX_PBA_OFFSET 8
 #define MSIX_CAP_LENGTH 12
 
-/* MSI enable bit is in byte 1 in FLAGS register */
-#define MSIX_ENABLE_OFFSET (PCI_MSIX_FLAGS + 1)
+/* MSI enable bit and maskall bit are in byte 1 in FLAGS register */
+#define MSIX_CONTROL_OFFSET (PCI_MSIX_FLAGS + 1)
 #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
 
 /* MSI-X table format */
@@ -101,7 +101,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
                  bar_nr);
     pdev->msix_cap = config_offset;
     /* Make flags bit writeable. */
-    pdev->wmask[config_offset + MSIX_ENABLE_OFFSET] |= MSIX_ENABLE_MASK;
+    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK;
     return 0;
 }
 
@@ -117,7 +117,7 @@ static void msix_free_irq_entries(PCIDevice *dev)
 void msix_write_config(PCIDevice *dev, uint32_t addr,
                        uint32_t val, int len)
 {
-    unsigned enable_pos = dev->msix_cap + MSIX_ENABLE_OFFSET;
+    unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     if (addr + len <= enable_pos || addr > enable_pos)
         return;
 
@@ -325,7 +325,7 @@ int msix_present(PCIDevice *dev)
 int msix_enabled(PCIDevice *dev)
 {
     return (dev->cap_present & QEMU_PCI_CAP_MSIX) &&
-        (dev->config[dev->msix_cap + MSIX_ENABLE_OFFSET] &
+        (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
          MSIX_ENABLE_MASK);
 }
 
@@ -361,8 +361,8 @@ void msix_reset(PCIDevice *dev)
     if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
         return;
     msix_free_irq_entries(dev);
-    dev->config[dev->msix_cap + MSIX_ENABLE_OFFSET] &=
-	    ~dev->wmask[dev->msix_cap + MSIX_ENABLE_OFFSET];
+    dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
+	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
     memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
     msix_mask_all(dev, dev->msix_entries_nr);
 }
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH 4/5] msix: function mask support

[Michael S. Tsirkin]

Function mask is a mandatory feature in MSIX
spec so not implementing it is a spec violation.
Implement.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c |   64 +++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 07111d0..a60ea95 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -20,6 +20,7 @@
 #define  PCI_MSIX_FLAGS 2     /* Table at lower 11 bits */
 #define  PCI_MSIX_FLAGS_QSIZE	0x7FF
 #define  PCI_MSIX_FLAGS_ENABLE	(1 << 15)
+#define  PCI_MSIX_FLAGS_MASKALL	(1 << 14)
 #define  PCI_MSIX_FLAGS_BIRMASK	(7 << 0)
 
 /* MSI-X capability structure */
@@ -30,6 +31,7 @@
 /* MSI enable bit and maskall bit are in byte 1 in FLAGS register */
 #define MSIX_CONTROL_OFFSET (PCI_MSIX_FLAGS + 1)
 #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
+#define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
 
 /* MSI-X table format */
 #define MSIX_MSG_ADDR 0
@@ -101,7 +103,8 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
                  bar_nr);
     pdev->msix_cap = config_offset;
     /* Make flags bit writeable. */
-    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK;
+    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
+	    MSIX_MASKALL_MASK;
     return 0;
 }
 
@@ -113,18 +116,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
         dev->msix_entry_used[vector] = 0;
 }
 
-/* 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;
-    if (addr + len <= enable_pos || addr > enable_pos)
-        return;
-
-    if (msix_enabled(dev))
-        qemu_set_irq(dev->irq[0], 0);
-}
-
 static uint32_t msix_mmio_readl(void *opaque, target_phys_addr_t addr)
 {
     PCIDevice *dev = opaque;
@@ -165,10 +156,50 @@ 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 * MSIX_ENTRY_SIZE + MSIX_VECTOR_CTRL;
-    return dev->msix_table_page[offset] & MSIX_VECTOR_MASK;
+    return msix_function_masked(dev) ||
+	   dev->msix_table_page[offset] & MSIX_VECTOR_MASK;
+}
+
+static void msix_handle_mask_update(PCIDevice *dev, int vector)
+{
+    if (!msix_is_masked(dev, vector) && msix_is_pending(dev, vector)) {
+        msix_clr_pending(dev, vector);
+        msix_notify(dev, vector);
+    }
+}
+
+/* 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;
+
+    if (addr + len <= enable_pos || addr > enable_pos) {
+        return;
+    }
+
+    if (!msix_enabled(dev)) {
+        return;
+    }
+
+    qemu_set_irq(dev->irq[0], 0);
+
+    if (msix_function_masked(dev)) {
+        return;
+    }
+
+    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
+        msix_handle_mask_update(dev, vector);
+    }
 }
 
 static void msix_mmio_writel(void *opaque, target_phys_addr_t addr,
@@ -178,10 +209,7 @@ static void msix_mmio_writel(void *opaque, target_phys_addr_t addr,
     unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
     int vector = offset / MSIX_ENTRY_SIZE;
     pci_set_long(dev->msix_table_page + offset, val);
-    if (!msix_is_masked(dev, vector) && msix_is_pending(dev, vector)) {
-        msix_clr_pending(dev, vector);
-        msix_notify(dev, vector);
-    }
+    msix_handle_mask_update(dev, vector);
 }
 
 static void msix_mmio_write_unallowed(void *opaque, target_phys_addr_t addr,
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH 5/5] msix: clear pending bit of an unused vector

[Michael S. Tsirkin]

PCI spec states:
if a masked vector has its Pending bit set, and the associated
underlying interrupt events are somehow satisfied (usually by software
though the exact manner is function-specific), the function must clear
the Pending bit, to avoid sending a spurious interrupt message later
when software unmasks the vector.

In our case this happens if vector becomes unused.
Clear pending bit in this case.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c |   27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index a60ea95..0baedef 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -108,14 +108,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
     return 0;
 }
 
-static void msix_free_irq_entries(PCIDevice *dev)
-{
-    int vector;
-
-    for (vector = 0; vector < dev->msix_entries_nr; ++vector)
-        dev->msix_entry_used[vector] = 0;
-}
-
 static uint32_t msix_mmio_readl(void *opaque, target_phys_addr_t addr)
 {
     PCIDevice *dev = opaque;
@@ -299,6 +291,16 @@ err_index:
     return ret;
 }
 
+static void msix_free_irq_entries(PCIDevice *dev)
+{
+    int vector;
+
+    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
+        dev->msix_entry_used[vector] = 0;
+        msix_clr_pending(dev, vector);
+    }
+}
+
 /* Clean up resources for the device. */
 int msix_uninit(PCIDevice *dev)
 {
@@ -415,8 +417,13 @@ int msix_vector_use(PCIDevice *dev, unsigned vector)
 /* Mark vector as unused. */
 void msix_vector_unuse(PCIDevice *dev, unsigned vector)
 {
-    if (vector < dev->msix_entries_nr && dev->msix_entry_used[vector])
-        --dev->msix_entry_used[vector];
+    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) {
+        return;
+    }
+    if (--dev->msix_entry_used[vector]) {
+        return;
+    }
+    msix_clr_pending(dev, vector);
 }
 
 void msix_unuse_all_vectors(PCIDevice *dev)
-- 
1.6.5.2.143.g8cc62