[PATCH 0/7] qemu-kvm: Clean up and enhance MSI irqchip support

[PATCH 0/7] qemu-kvm: Clean up and enhance MSI irqchip support

[Jan Kiszka]

I'm not fully happy with the cleanups yet, but I think it is a step
forward. Moreover, it adds the critical support for classic MSI in
irqchip mode. That makes AHCI and other MSI-using device models
available for the standard KVM mode. So I decided to roll this out.



Jan Kiszka (7):
  qemu-kvm: Drop unneeded kvm_irq_routing_entry declaration
  qemu-kvm: Rename kvm_msix_message to KVMMsiMessage
  qemu-kvm: Refactor MSI core API of KVM
  qemu-kvm: Fix and clean up msix vector use/unuse hooks
  qemu-kvm: Move gsi bits from kvm_msix_vector_add to
    kvm_msi_add_message
  qemu-kvm: Move entry comparison into kvm_msi_update_message
  qemu-kvm: Add in-kernel irqchip support for MSI

 hw/msi.c   |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/msix.c  |   73 +++++++++++++--------------------------
 hw/pci.h   |   16 +++-----
 kvm.h      |   20 ++++++----
 qemu-kvm.c |   57 ++++++++++++++++++++-----------
 5 files changed, 190 insertions(+), 87 deletions(-)

[PATCH 1/7] qemu-kvm: Drop unneeded kvm_irq_routing_entry declaration

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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

diff --git a/hw/pci.h b/hw/pci.h
index d584458..dd09494 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -6,8 +6,6 @@
 
 #include "qdev.h"
 
-struct kvm_irq_routing_entry;
-
 /* PCI includes legacy ISA access.  */
 #include "isa.h"
 
-- 
1.7.1

[PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This structure will be used for legacy MSI as well and will become part
of the KVM API.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msix.c |   10 +++++-----
 hw/pci.h  |   10 ++--------
 kvm.h     |    7 +++++++
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 8b68a6a..42870c5 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -51,7 +51,7 @@ int msix_supported;
 static void kvm_msix_free(PCIDevice *dev)
 {
     int vector, changed = 0;
-    struct kvm_msix_message *kmm;
+    KVMMsiMessage *kmm;
 
     for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
         if (dev->msix_entry_used[vector]) {
@@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev)
 }
 
 static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
-                                         struct kvm_msix_message *kmm)
+                                         KVMMsiMessage *kmm)
 {
     uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE;
 
@@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
 static void kvm_msix_update(PCIDevice *dev, int vector,
                             int was_masked, int is_masked)
 {
-    struct kvm_msix_message e = {}, *entry;
+    KVMMsiMessage e = {}, *entry;
     int mask_cleared = was_masked && !is_masked;
     /* It is only legal to change an entry when it is masked. Therefore, it is
      * enough to update the routing in kernel when mask is being cleared. */
@@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
 
 static int kvm_msix_add(PCIDevice *dev, unsigned vector)
 {
-    struct kvm_msix_message *kmm = dev->msix_irq_entries + vector;
+    KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
     int r;
 
     if (!kvm_has_gsi_routing()) {
@@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
 
 static void kvm_msix_del(PCIDevice *dev, unsigned vector)
 {
-    struct kvm_msix_message *kmm;
+    KVMMsiMessage *kmm;
 
     if (dev->msix_entry_used[vector]) {
         return;
diff --git a/hw/pci.h b/hw/pci.h
index dd09494..dc5df17 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -5,6 +5,7 @@
 #include "qobject.h"
 
 #include "qdev.h"
+#include "kvm.h"
 
 /* PCI includes legacy ISA access.  */
 #include "isa.h"
@@ -129,13 +130,6 @@ enum {
 typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
 				       int masked);
 
-struct kvm_msix_message {
-    uint32_t gsi;
-    uint32_t addr_lo;
-    uint32_t addr_hi;
-    uint32_t data;
-};
-
 struct PCIDevice {
     DeviceState qdev;
     /* PCI config space */
@@ -208,7 +202,7 @@ struct PCIDevice {
      * on the rest of the region. */
     target_phys_addr_t msix_page_size;
 
-    struct kvm_msix_message *msix_irq_entries;
+    KVMMsiMessage *msix_irq_entries;
 
     msix_mask_notifier_func msix_mask_notifier;
 };
diff --git a/kvm.h b/kvm.h
index b890b5d..8ece0b3 100644
--- a/kvm.h
+++ b/kvm.h
@@ -214,6 +214,13 @@ int kvm_set_irqfd(int gsi, int fd, bool assigned)
 
 int kvm_set_ioeventfd_pio_word(int fd, uint16_t adr, uint16_t val, bool assign);
 
+typedef struct KVMMsiMessage {
+    uint32_t gsi;
+    uint32_t addr_lo;
+    uint32_t addr_hi;
+    uint32_t data;
+} KVMMsiMessage;
+
 int kvm_has_gsi_routing(void);
 int kvm_get_irq_route_gsi(void);
 int kvm_add_msix(uint32_t gsi, uint32_t addr_lo,
-- 
1.7.1

[PATCH 3/7] qemu-kvm: Refactor MSI core API of KVM

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Remove MSI-X from function names - the interface is valid for MSI as
well - and use KVMMsiMessage as parameter.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msix.c  |   15 ++++-----------
 kvm.h      |   13 +++++--------
 qemu-kvm.c |   32 +++++++++++++-------------------
 3 files changed, 22 insertions(+), 38 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 42870c5..52facd4 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -51,12 +51,10 @@ int msix_supported;
 static void kvm_msix_free(PCIDevice *dev)
 {
     int vector, changed = 0;
-    KVMMsiMessage *kmm;
 
     for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
         if (dev->msix_entry_used[vector]) {
-            kmm = &dev->msix_irq_entries[vector];
-            kvm_del_msix(kmm->gsi, kmm->addr_lo, kmm->addr_hi, kmm->data);
+            kvm_msi_message_del(&dev->msix_irq_entries[vector]);
             changed = 1;
         }
     }
@@ -94,9 +92,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
     if (memcmp(entry, &e, sizeof e) != 0) {
         int r;
 
-        r = kvm_update_msix(entry->gsi, entry->addr_lo,
-                            entry->addr_hi, entry->data,
-                            e.gsi, e.addr_lo, e.addr_hi, e.data);
+        r = kvm_msi_message_update(entry, &e);
         if (r) {
             fprintf(stderr, "%s: kvm_update_msix failed: %s\n", __func__,
 		    strerror(-r));
@@ -131,7 +127,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
     }
     kmm->gsi = r;
     kvm_msix_message_from_vector(dev, vector, kmm);
-    r = kvm_add_msix(kmm->gsi, kmm->addr_lo, kmm->addr_hi, kmm->data);
+    r = kvm_msi_message_add(kmm);
     if (r < 0) {
         fprintf(stderr, "%s: kvm_add_msix failed: %s\n", __func__, strerror(-r));
         return r;
@@ -147,13 +143,10 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
 
 static void kvm_msix_del(PCIDevice *dev, unsigned vector)
 {
-    KVMMsiMessage *kmm;
-
     if (dev->msix_entry_used[vector]) {
         return;
     }
-    kmm = &dev->msix_irq_entries[vector];
-    kvm_del_msix(kmm->gsi, kmm->addr_lo, kmm->addr_hi, kmm->data);
+    kvm_msi_message_del(&dev->msix_irq_entries[vector]);
     kvm_commit_irq_routes();
 }
 
diff --git a/kvm.h b/kvm.h
index 8ece0b3..179eb7e 100644
--- a/kvm.h
+++ b/kvm.h
@@ -223,14 +223,11 @@ typedef struct KVMMsiMessage {
 
 int kvm_has_gsi_routing(void);
 int kvm_get_irq_route_gsi(void);
-int kvm_add_msix(uint32_t gsi, uint32_t addr_lo,
-                 uint32_t addr_hi, uint32_t data);
-int kvm_del_msix(uint32_t gsi, uint32_t addr_lo,
-                 uint32_t addr_hi, uint32_t data);
-int kvm_update_msix(uint32_t old_gsi, uint32_t old_addr_lo,
-                    uint32_t old_addr_hi, uint32_t old_data,
-                    uint32_t new_gsi, uint32_t new_addr_lo,
-                    uint32_t new_addr_hi, uint32_t new_data);
+
+int kvm_msi_message_add(KVMMsiMessage *msg);
+int kvm_msi_message_del(KVMMsiMessage *msg);
+int kvm_msi_message_update(KVMMsiMessage *old, KVMMsiMessage *new);
+
 int kvm_commit_irq_routes(void);
 
 int kvm_irqchip_in_kernel(void);
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 7689225..9cbc109 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -969,46 +969,40 @@ int kvm_get_irq_route_gsi(void)
     return -ENOSPC;
 }
 
-static void kvm_msix_routing_entry(struct kvm_irq_routing_entry *e,
-                                   uint32_t gsi, uint32_t addr_lo,
-                                   uint32_t addr_hi, uint32_t data)
+static void kvm_msi_routing_entry(struct kvm_irq_routing_entry *e,
+                                  KVMMsiMessage *msg)
 
 {
-    e->gsi = gsi;
+    e->gsi = msg->gsi;
     e->type = KVM_IRQ_ROUTING_MSI;
     e->flags = 0;
-    e->u.msi.address_lo = addr_lo;
-    e->u.msi.address_hi = addr_hi;
-    e->u.msi.data = data;
+    e->u.msi.address_lo = msg->addr_lo;
+    e->u.msi.address_hi = msg->addr_hi;
+    e->u.msi.data = msg->data;
 }
 
-int kvm_add_msix(uint32_t gsi, uint32_t addr_lo,
-                        uint32_t addr_hi, uint32_t data)
+int kvm_msi_message_add(KVMMsiMessage *msg)
 {
     struct kvm_irq_routing_entry e;
 
-    kvm_msix_routing_entry(&e, gsi, addr_lo, addr_hi, data);
+    kvm_msi_routing_entry(&e, msg);
     return kvm_add_routing_entry(&e);
 }
 
-int kvm_del_msix(uint32_t gsi, uint32_t addr_lo,
-                        uint32_t addr_hi, uint32_t data)
+int kvm_msi_message_del(KVMMsiMessage *msg)
 {
     struct kvm_irq_routing_entry e;
 
-    kvm_msix_routing_entry(&e, gsi, addr_lo, addr_hi, data);
+    kvm_msi_routing_entry(&e, msg);
     return kvm_del_routing_entry(&e);
 }
 
-int kvm_update_msix(uint32_t old_gsi, uint32_t old_addr_lo,
-                    uint32_t old_addr_hi, uint32_t old_data,
-                    uint32_t new_gsi, uint32_t new_addr_lo,
-                    uint32_t new_addr_hi, uint32_t new_data)
+int kvm_msi_message_update(KVMMsiMessage *old, KVMMsiMessage *new)
 {
     struct kvm_irq_routing_entry e1, e2;
 
-    kvm_msix_routing_entry(&e1, old_gsi, old_addr_lo, old_addr_hi, old_data);
-    kvm_msix_routing_entry(&e2, new_gsi, new_addr_lo, new_addr_hi, new_data);
+    kvm_msi_routing_entry(&e1, old);
+    kvm_msi_routing_entry(&e2, new);
     return kvm_update_routing_entry(&e1, &e2);
 }
 
-- 
1.7.1

[PATCH 4/7] qemu-kvm: Fix and clean up msix vector use/unuse hooks

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Remove the premature return from msix_vector_use if the vector was
already in use, this could cause usage counter imbalances. In contrast,
the check for msix_entry_used on deletion was redundant. At this chance,
rename the internal API to clarify what is added/deleted here.

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

diff --git a/hw/msix.c b/hw/msix.c
index 52facd4..1bdffb6 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -108,7 +108,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
     }
 }
 
-static int kvm_msix_add(PCIDevice *dev, unsigned vector)
+static int kvm_msix_vector_add(PCIDevice *dev, unsigned vector)
 {
     KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
     int r;
@@ -141,11 +141,8 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
     return 0;
 }
 
-static void kvm_msix_del(PCIDevice *dev, unsigned vector)
+static void kvm_msix_vector_del(PCIDevice *dev, unsigned vector)
 {
-    if (dev->msix_entry_used[vector]) {
-        return;
-    }
     kvm_msi_message_del(&dev->msix_irq_entries[vector]);
     kvm_commit_irq_routes();
 }
@@ -548,11 +545,9 @@ int msix_vector_use(PCIDevice *dev, unsigned vector)
     int ret;
     if (vector >= dev->msix_entries_nr)
         return -EINVAL;
-    if (dev->msix_entry_used[vector]) {
-        return 0;
-    }
-    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
-        ret = kvm_msix_add(dev, vector);
+    if (kvm_enabled() && kvm_irqchip_in_kernel() &&
+        !dev->msix_entry_used[vector]) {
+        ret = kvm_msix_vector_add(dev, vector);
         if (ret) {
             return ret;
         }
@@ -571,7 +566,7 @@ void msix_vector_unuse(PCIDevice *dev, unsigned vector)
         return;
     }
     if (kvm_enabled() && kvm_irqchip_in_kernel()) {
-        kvm_msix_del(dev, vector);
+        kvm_msix_vector_del(dev, vector);
     }
     msix_clr_pending(dev, vector);
 }
-- 
1.7.1

[PATCH 5/7] qemu-kvm: Move gsi bits from kvm_msix_vector_add to kvm_msi_add_message

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Testing support and allocating a GSI for an MSI message is required both
for MSI and MSI-X. At this chance, drop the aging version warning.

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

diff --git a/hw/msix.c b/hw/msix.c
index 1bdffb6..8c8bc18 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -113,19 +113,6 @@ static int kvm_msix_vector_add(PCIDevice *dev, unsigned vector)
     KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
     int r;
 
-    if (!kvm_has_gsi_routing()) {
-        fprintf(stderr, "Warning: no MSI-X support found. "
-                "At least kernel 2.6.30 is required for MSI-X support.\n"
-               );
-        return -EOPNOTSUPP;
-    }
-
-    r = kvm_get_irq_route_gsi();
-    if (r < 0) {
-        fprintf(stderr, "%s: kvm_get_irq_route_gsi failed: %s\n", __func__, strerror(-r));
-        return r;
-    }
-    kmm->gsi = r;
     kvm_msix_message_from_vector(dev, vector, kmm);
     r = kvm_msi_message_add(kmm);
     if (r < 0) {
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 9cbc109..7317f87 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -984,6 +984,17 @@ static void kvm_msi_routing_entry(struct kvm_irq_routing_entry *e,
 int kvm_msi_message_add(KVMMsiMessage *msg)
 {
     struct kvm_irq_routing_entry e;
+    int ret;
+
+    if (!kvm_has_gsi_routing()) {
+        return -EOPNOTSUPP;
+    }
+
+    ret = kvm_get_irq_route_gsi();
+    if (ret < 0) {
+        return ret;
+    }
+    msg->gsi = ret;
 
     kvm_msi_routing_entry(&e, msg);
     return kvm_add_routing_entry(&e);
-- 
1.7.1

[PATCH 6/7] qemu-kvm: Move entry comparison into kvm_msi_update_message

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Checking the the update chances the message content is a common task for
both MSI types.

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

diff --git a/hw/msix.c b/hw/msix.c
index 8c8bc18..9cee086 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -76,8 +76,10 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
 static void kvm_msix_update(PCIDevice *dev, int vector,
                             int was_masked, int is_masked)
 {
-    KVMMsiMessage e = {}, *entry;
+    KVMMsiMessage new_entry, *entry;
     int mask_cleared = was_masked && !is_masked;
+    int r;
+
     /* It is only legal to change an entry when it is masked. Therefore, it is
      * enough to update the routing in kernel when mask is being cleared. */
     if (!mask_cleared) {
@@ -86,19 +88,17 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
     if (!dev->msix_entry_used[vector]) {
         return;
     }
-    entry = dev->msix_irq_entries + vector;
-    e.gsi = entry->gsi;
-    kvm_msix_message_from_vector(dev, vector, &e);
-    if (memcmp(entry, &e, sizeof e) != 0) {
-        int r;
 
-        r = kvm_msi_message_update(entry, &e);
-        if (r) {
-            fprintf(stderr, "%s: kvm_update_msix failed: %s\n", __func__,
-		    strerror(-r));
-            exit(1);
-        }
-        *entry = e;
+    entry = dev->msix_irq_entries + vector;
+    kvm_msix_message_from_vector(dev, vector, &new_entry);
+    r = kvm_msi_message_update(entry, &new_entry);
+    if (r < 0) {
+        fprintf(stderr, "%s: kvm_update_msix failed: %s\n", __func__,
+                strerror(-r));
+        exit(1);
+    }
+    if (r > 0) {
+        *entry = new_entry;
         r = kvm_commit_irq_routes();
         if (r) {
             fprintf(stderr, "%s: kvm_commit_irq_routes failed: %s\n", __func__,
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 7317f87..e8c2009 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1011,10 +1011,22 @@ int kvm_msi_message_del(KVMMsiMessage *msg)
 int kvm_msi_message_update(KVMMsiMessage *old, KVMMsiMessage *new)
 {
     struct kvm_irq_routing_entry e1, e2;
+    int ret;
+
+    new->gsi = old->gsi;
+    if (memcmp(old, new, sizeof(KVMMsiMessage)) == 0) {
+        return 0;
+    }
 
     kvm_msi_routing_entry(&e1, old);
     kvm_msi_routing_entry(&e2, new);
-    return kvm_update_routing_entry(&e1, &e2);
+
+    ret = kvm_update_routing_entry(&e1, &e2);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return 1;
 }
 
 
-- 
1.7.1

[PATCH 7/7] qemu-kvm: Add in-kernel irqchip support for MSI

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This adds the required bits to map classic MSI vectors on KVM IRQ
routing entries and deliver them via the irqchip if enabled.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msi.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci.h |    4 ++
 2 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index 3dc3a24..18f683b 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -20,6 +20,7 @@
 
 #include "msi.h"
 #include "range.h"
+#include "kvm.h"
 
 /* Eventually those constants should go to Linux pci_regs.h */
 #define PCI_MSI_PENDING_32      0x10
@@ -109,6 +110,92 @@ bool msi_enabled(const PCIDevice *dev)
          PCI_MSI_FLAGS_ENABLE);
 }
 
+static void kvm_msi_message_from_vector(PCIDevice *dev, unsigned vector,
+                                        KVMMsiMessage *kmm)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    unsigned int nr_vectors = msi_nr_vectors(flags);
+
+    kmm->addr_lo = pci_get_long(dev->config + msi_address_lo_off(dev));
+    if (msi64bit) {
+        kmm->addr_hi = pci_get_long(dev->config + msi_address_hi_off(dev));
+    } else {
+        kmm->addr_hi = 0;
+    }
+
+    kmm->data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
+    if (nr_vectors > 1) {
+        kmm->data &= ~(nr_vectors - 1);
+        kmm->data |= vector;
+    }
+}
+
+static void kvm_msi_update(PCIDevice *dev)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    unsigned int nr_vectors = msi_nr_vectors(flags);
+    KVMMsiMessage new_entry, *entry;
+    bool changed = false;
+    unsigned int vector;
+    int r;
+
+    for (vector = 0; vector < 32; vector++) {
+        entry = dev->msi_irq_entries + vector;
+
+        if (vector >= nr_vectors) {
+            if (vector < dev->msi_entries_nr) {
+                kvm_msi_message_del(entry);
+                changed = true;
+            }
+        } else if (vector >= dev->msi_entries_nr) {
+            kvm_msi_message_from_vector(dev, vector, entry);
+            r = kvm_msi_message_add(entry);
+            if (r) {
+                fprintf(stderr, "%s: kvm_msi_add failed: %s\n", __func__,
+                        strerror(-r));
+                exit(1);
+            }
+            changed = true;
+        } else {
+            kvm_msi_message_from_vector(dev, vector, &new_entry);
+            r = kvm_msi_message_update(entry, &new_entry);
+            if (r < 0) {
+                fprintf(stderr, "%s: kvm_update_msi failed: %s\n",
+                        __func__, strerror(-r));
+                exit(1);
+            }
+            if (r > 0) {
+                *entry = new_entry;
+                changed = true;
+            }
+        }
+    }
+    dev->msi_entries_nr = nr_vectors;
+    if (changed) {
+        r = kvm_commit_irq_routes();
+        if (r) {
+            fprintf(stderr, "%s: kvm_commit_irq_routes failed: %s\n", __func__,
+                    strerror(-r));
+            exit(1);
+        }
+    }
+}
+
+/* KVM specific MSI helpers */
+static void kvm_msi_free(PCIDevice *dev)
+{
+    unsigned int vector;
+
+    for (vector = 0; vector < dev->msi_entries_nr; ++vector) {
+        kvm_msi_message_del(&dev->msi_irq_entries[vector]);
+    }
+    if (dev->msi_entries_nr > 0) {
+        kvm_commit_irq_routes();
+    }
+    dev->msi_entries_nr = 0;
+}
+
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
 {
@@ -159,6 +246,12 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
         pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
                      0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
     }
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        dev->msi_irq_entries = qemu_malloc(nr_vectors *
+                                           sizeof(*dev->msix_irq_entries));
+    }
+
     return config_offset;
 }
 
@@ -166,6 +259,11 @@ void msi_uninit(struct PCIDevice *dev)
 {
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
     uint8_t cap_size = msi_cap_sizeof(flags);
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_msi_free(dev);
+        qemu_free(dev->msi_irq_entries);
+    }
     pci_del_capability(dev, PCI_CAP_ID_MSIX, cap_size);
     MSI_DEV_PRINTF(dev, "uninit\n");
 }
@@ -175,6 +273,10 @@ void msi_reset(PCIDevice *dev)
     uint16_t flags;
     bool msi64bit;
 
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_msi_free(dev);
+    }
+
     flags = pci_get_word(dev->config + msi_flags_off(dev));
     flags &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
     msi64bit = flags & PCI_MSI_FLAGS_64BIT;
@@ -224,6 +326,11 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
         return;
     }
 
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_set_irq(dev->msi_irq_entries[vector].gsi, 1, NULL);
+        return;
+    }
+
     if (msi64bit) {
         address = pci_get_quad(dev->config + msi_address_lo_off(dev));
     } else {
@@ -312,6 +419,10 @@ void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
         pci_set_word(dev->config + msi_flags_off(dev), flags);
     }
 
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_msi_update(dev);
+    }
+
     if (!msi_per_vector_mask) {
         /* if per vector masking isn't supported,
            there is no pending interrupt. */
diff --git a/hw/pci.h b/hw/pci.h
index dc5df17..1a18bc8 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -195,6 +195,10 @@ struct PCIDevice {
     ram_addr_t rom_offset;
     uint32_t rom_bar;
 
+    /* MSI entries */
+    int msi_entries_nr;
+    struct KVMMsiMessage *msi_irq_entries;
+
     /* How much space does an MSIX table need. */
     /* The spec requires giving the table structure
      * a 4K aligned region all by itself. Align it to
-- 
1.7.1

Re: [PATCH 4/7] qemu-kvm: Fix and clean up msix vector use/unuse hooks

[Michael S. Tsirkin]

On Sat, Apr 23, 2011 at 12:23:37PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Remove the premature return from msix_vector_use if the vector was
> already in use, this could cause usage counter imbalances. In contrast,
> the check for msix_entry_used on deletion was redundant. At this chance,
> rename the internal API to clarify what is added/deleted here.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Good catch!

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

> ---
>  hw/msix.c |   17 ++++++-----------
>  1 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 52facd4..1bdffb6 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -108,7 +108,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
>      }
>  }
>  
> -static int kvm_msix_add(PCIDevice *dev, unsigned vector)
> +static int kvm_msix_vector_add(PCIDevice *dev, unsigned vector)
>  {
>      KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
>      int r;
> @@ -141,11 +141,8 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
>      return 0;
>  }
>  
> -static void kvm_msix_del(PCIDevice *dev, unsigned vector)
> +static void kvm_msix_vector_del(PCIDevice *dev, unsigned vector)
>  {
> -    if (dev->msix_entry_used[vector]) {
> -        return;
> -    }
>      kvm_msi_message_del(&dev->msix_irq_entries[vector]);
>      kvm_commit_irq_routes();
>  }
> @@ -548,11 +545,9 @@ int msix_vector_use(PCIDevice *dev, unsigned vector)
>      int ret;
>      if (vector >= dev->msix_entries_nr)
>          return -EINVAL;
> -    if (dev->msix_entry_used[vector]) {
> -        return 0;
> -    }
> -    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> -        ret = kvm_msix_add(dev, vector);
> +    if (kvm_enabled() && kvm_irqchip_in_kernel() &&
> +        !dev->msix_entry_used[vector]) {
> +        ret = kvm_msix_vector_add(dev, vector);
>          if (ret) {
>              return ret;
>          }
> @@ -571,7 +566,7 @@ void msix_vector_unuse(PCIDevice *dev, unsigned vector)
>          return;
>      }
>      if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> -        kvm_msix_del(dev, vector);
> +        kvm_msix_vector_del(dev, vector);
>      }
>      msix_clr_pending(dev, vector);
>  }
> -- 
> 1.7.1

Re: [PATCH 5/7] qemu-kvm: Move gsi bits from kvm_msix_vector_add to kvm_msi_add_message

[Michael S. Tsirkin]

On Sat, Apr 23, 2011 at 12:23:38PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Testing support and allocating a GSI for an MSI message is required both
> for MSI and MSI-X. At this chance, drop the aging version warning.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

No objection, but I do note that this means running on an old
kernel will lead to a silent failure.
stderr output is not in fact much better: I think we should
check the capability in msix_init. Care coding this up?

> ---
>  hw/msix.c  |   13 -------------
>  qemu-kvm.c |   11 +++++++++++
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 1bdffb6..8c8bc18 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -113,19 +113,6 @@ static int kvm_msix_vector_add(PCIDevice *dev, unsigned vector)
>      KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
>      int r;
>  
> -    if (!kvm_has_gsi_routing()) {
> -        fprintf(stderr, "Warning: no MSI-X support found. "
> -                "At least kernel 2.6.30 is required for MSI-X support.\n"
> -               );
> -        return -EOPNOTSUPP;
> -    }
> -
> -    r = kvm_get_irq_route_gsi();
> -    if (r < 0) {
> -        fprintf(stderr, "%s: kvm_get_irq_route_gsi failed: %s\n", __func__, strerror(-r));
> -        return r;
> -    }
> -    kmm->gsi = r;
>      kvm_msix_message_from_vector(dev, vector, kmm);
>      r = kvm_msi_message_add(kmm);
>      if (r < 0) {
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 9cbc109..7317f87 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -984,6 +984,17 @@ static void kvm_msi_routing_entry(struct kvm_irq_routing_entry *e,
>  int kvm_msi_message_add(KVMMsiMessage *msg)
>  {
>      struct kvm_irq_routing_entry e;
> +    int ret;
> +
> +    if (!kvm_has_gsi_routing()) {
> +        return -EOPNOTSUPP;
> +    }
> +
> +    ret = kvm_get_irq_route_gsi();
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    msg->gsi = ret;
>  
>      kvm_msi_routing_entry(&e, msg);
>      return kvm_add_routing_entry(&e);
> -- 
> 1.7.1

Re: [PATCH 5/7] qemu-kvm: Move gsi bits from kvm_msix_vector_add to kvm_msi_add_message

[Jan Kiszka]

On 2011-04-27 14:54, Michael S. Tsirkin wrote:
> On Sat, Apr 23, 2011 at 12:23:38PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Testing support and allocating a GSI for an MSI message is required both
>> for MSI and MSI-X. At this chance, drop the aging version warning.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> No objection, but I do note that this means running on an old
> kernel will lead to a silent failure.

Shouldn't be silent: The caller of msi_vector_use should check and
process the error (virtio should even forward it to guest IIUC).
> stderr output is not in fact much better: I think we should
> check the capability in msix_init. Care coding this up?

I think the motivation to check on vector activation is that devices and
guests without a need for MSI should not cause a failure if MSI is
unsupported by KVM.

Jan

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

Re: [PATCH 7/7] qemu-kvm: Add in-kernel irqchip support for MSI

[Michael S. Tsirkin]

On Sat, Apr 23, 2011 at 12:23:40PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This adds the required bits to map classic MSI vectors on KVM IRQ
> routing entries and deliver them via the irqchip if enabled.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/msi.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci.h |    4 ++
>  2 files changed, 115 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 3dc3a24..18f683b 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -20,6 +20,7 @@
>  
>  #include "msi.h"
>  #include "range.h"
> +#include "kvm.h"
>  
>  /* Eventually those constants should go to Linux pci_regs.h */
>  #define PCI_MSI_PENDING_32      0x10
> @@ -109,6 +110,92 @@ bool msi_enabled(const PCIDevice *dev)
>           PCI_MSI_FLAGS_ENABLE);
>  }
>  
> +static void kvm_msi_message_from_vector(PCIDevice *dev, unsigned vector,
> +                                        KVMMsiMessage *kmm)
> +{
> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> +    unsigned int nr_vectors = msi_nr_vectors(flags);
> +
> +    kmm->addr_lo = pci_get_long(dev->config + msi_address_lo_off(dev));
> +    if (msi64bit) {
> +        kmm->addr_hi = pci_get_long(dev->config + msi_address_hi_off(dev));
> +    } else {
> +        kmm->addr_hi = 0;
> +    }
> +
> +    kmm->data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> +    if (nr_vectors > 1) {
> +        kmm->data &= ~(nr_vectors - 1);
> +        kmm->data |= vector;
> +    }

msi_notify already has similar logic.
Let's add msi_get_vector_address and msi_get_vector_data


> +}
> +
> +static void kvm_msi_update(PCIDevice *dev)
> +{
> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> +    unsigned int nr_vectors = msi_nr_vectors(flags);
> +    KVMMsiMessage new_entry, *entry;
> +    bool changed = false;
> +    unsigned int vector;
> +    int r;
> +
> +    for (vector = 0; vector < 32; vector++) {

I'd say up to log_max_vecs: the array is allocated
with nr_vectors.

> +        entry = dev->msi_irq_entries + vector;
> +
> +        if (vector >= nr_vectors) {
> +            if (vector < dev->msi_entries_nr) {
> +                kvm_msi_message_del(entry);
> +                changed = true;
> +            }
> +        } else if (vector >= dev->msi_entries_nr) {
> +            kvm_msi_message_from_vector(dev, vector, entry);
> +            r = kvm_msi_message_add(entry);
> +            if (r) {
> +                fprintf(stderr, "%s: kvm_msi_add failed: %s\n", __func__,
> +                        strerror(-r));
> +                exit(1);
> +            }
> +            changed = true;
> +        } else {
> +            kvm_msi_message_from_vector(dev, vector, &new_entry);
> +            r = kvm_msi_message_update(entry, &new_entry);
> +            if (r < 0) {
> +                fprintf(stderr, "%s: kvm_update_msi failed: %s\n",
> +                        __func__, strerror(-r));
> +                exit(1);
> +            }
> +            if (r > 0) {
> +                *entry = new_entry;
> +                changed = true;
> +            }
> +        }
> +    }
> +    dev->msi_entries_nr = nr_vectors;
> +    if (changed) {
> +        r = kvm_commit_irq_routes();
> +        if (r) {
> +            fprintf(stderr, "%s: kvm_commit_irq_routes failed: %s\n", __func__,
> +                    strerror(-r));
> +            exit(1);
> +        }
> +    }
> +}
> +
> +/* KVM specific MSI helpers */
> +static void kvm_msi_free(PCIDevice *dev)
> +{
> +    unsigned int vector;
> +
> +    for (vector = 0; vector < dev->msi_entries_nr; ++vector) {
> +        kvm_msi_message_del(&dev->msi_irq_entries[vector]);
> +    }
> +    if (dev->msi_entries_nr > 0) {
> +        kvm_commit_irq_routes();
> +    }
> +    dev->msi_entries_nr = 0;
> +}
> +
>  int msi_init(struct PCIDevice *dev, uint8_t offset,
>               unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
>  {
> @@ -159,6 +246,12 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>          pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
>                       0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
>      }
> +
> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +        dev->msi_irq_entries = qemu_malloc(nr_vectors *
> +                                           sizeof(*dev->msix_irq_entries));
> +    }
> +
>      return config_offset;
>  }
>  
> @@ -166,6 +259,11 @@ void msi_uninit(struct PCIDevice *dev)
>  {
>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>      uint8_t cap_size = msi_cap_sizeof(flags);
> +
> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +        kvm_msi_free(dev);
> +        qemu_free(dev->msi_irq_entries);
> +    }
>      pci_del_capability(dev, PCI_CAP_ID_MSIX, cap_size);
>      MSI_DEV_PRINTF(dev, "uninit\n");
>  }
> @@ -175,6 +273,10 @@ void msi_reset(PCIDevice *dev)
>      uint16_t flags;
>      bool msi64bit;
>  
> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +        kvm_msi_free(dev);
> +    }
> +
>      flags = pci_get_word(dev->config + msi_flags_off(dev));
>      flags &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
>      msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> @@ -224,6 +326,11 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
>          return;
>      }
>  
> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +        kvm_set_irq(dev->msi_irq_entries[vector].gsi, 1, NULL);
> +        return;
> +    }
> +
>      if (msi64bit) {
>          address = pci_get_quad(dev->config + msi_address_lo_off(dev));
>      } else {
> @@ -312,6 +419,10 @@ void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
>          pci_set_word(dev->config + msi_flags_off(dev), flags);
>      }
>  
> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +        kvm_msi_update(dev);
> +    }
> +

Does this ignore per vector masking then?

>      if (!msi_per_vector_mask) {
>          /* if per vector masking isn't supported,
>             there is no pending interrupt. */
> diff --git a/hw/pci.h b/hw/pci.h
> index dc5df17..1a18bc8 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -195,6 +195,10 @@ struct PCIDevice {
>      ram_addr_t rom_offset;
>      uint32_t rom_bar;
>  
> +    /* MSI entries */
> +    int msi_entries_nr;
> +    struct KVMMsiMessage *msi_irq_entries;
> +
>      /* How much space does an MSIX table need. */
>      /* The spec requires giving the table structure
>       * a 4K aligned region all by itself. Align it to
> -- 
> 1.7.1

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Michael S. Tsirkin]

On Sat, Apr 23, 2011 at 12:23:35PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This structure will be used for legacy MSI as well and will become part
> of the KVM API.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I'd like KVMMSIMessage better. struct kvm_msi_message would be even
better but oh well.

> ---
>  hw/msix.c |   10 +++++-----
>  hw/pci.h  |   10 ++--------
>  kvm.h     |    7 +++++++
>  3 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 8b68a6a..42870c5 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -51,7 +51,7 @@ int msix_supported;
>  static void kvm_msix_free(PCIDevice *dev)
>  {
>      int vector, changed = 0;
> -    struct kvm_msix_message *kmm;
> +    KVMMsiMessage *kmm;
>  
>      for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
>          if (dev->msix_entry_used[vector]) {
> @@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev)
>  }
>  
>  static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
> -                                         struct kvm_msix_message *kmm)
> +                                         KVMMsiMessage *kmm)
>  {
>      uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE;
>  
> @@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
>  static void kvm_msix_update(PCIDevice *dev, int vector,
>                              int was_masked, int is_masked)
>  {
> -    struct kvm_msix_message e = {}, *entry;
> +    KVMMsiMessage e = {}, *entry;
>      int mask_cleared = was_masked && !is_masked;
>      /* It is only legal to change an entry when it is masked. Therefore, it is
>       * enough to update the routing in kernel when mask is being cleared. */
> @@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
>  
>  static int kvm_msix_add(PCIDevice *dev, unsigned vector)
>  {
> -    struct kvm_msix_message *kmm = dev->msix_irq_entries + vector;
> +    KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
>      int r;
>  
>      if (!kvm_has_gsi_routing()) {
> @@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
>  
>  static void kvm_msix_del(PCIDevice *dev, unsigned vector)
>  {
> -    struct kvm_msix_message *kmm;
> +    KVMMsiMessage *kmm;
>  
>      if (dev->msix_entry_used[vector]) {
>          return;
> diff --git a/hw/pci.h b/hw/pci.h
> index dd09494..dc5df17 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -5,6 +5,7 @@
>  #include "qobject.h"
>  
>  #include "qdev.h"
> +#include "kvm.h"

I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
is anything wrong with that?  Maybe just rename it to make it generic
for msi and leave if where it is.

>  
>  /* PCI includes legacy ISA access.  */
>  #include "isa.h"
> @@ -129,13 +130,6 @@ enum {
>  typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
>  				       int masked);
>  
> -struct kvm_msix_message {
> -    uint32_t gsi;
> -    uint32_t addr_lo;
> -    uint32_t addr_hi;
> -    uint32_t data;
> -};
> -
>  struct PCIDevice {
>      DeviceState qdev;
>      /* PCI config space */
> @@ -208,7 +202,7 @@ struct PCIDevice {
>       * on the rest of the region. */
>      target_phys_addr_t msix_page_size;
>  
> -    struct kvm_msix_message *msix_irq_entries;
> +    KVMMsiMessage *msix_irq_entries;
>  
>      msix_mask_notifier_func msix_mask_notifier;
>  };
> diff --git a/kvm.h b/kvm.h
> index b890b5d..8ece0b3 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -214,6 +214,13 @@ int kvm_set_irqfd(int gsi, int fd, bool assigned)
>  
>  int kvm_set_ioeventfd_pio_word(int fd, uint16_t adr, uint16_t val, bool assign);
>  
> +typedef struct KVMMsiMessage {
> +    uint32_t gsi;
> +    uint32_t addr_lo;
> +    uint32_t addr_hi;
> +    uint32_t data;
> +} KVMMsiMessage;
> +
>  int kvm_has_gsi_routing(void);
>  int kvm_get_irq_route_gsi(void);
>  int kvm_add_msix(uint32_t gsi, uint32_t addr_lo,
> -- 
> 1.7.1

Re: [PATCH 5/7] qemu-kvm: Move gsi bits from kvm_msix_vector_add to kvm_msi_add_message

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 03:29:15PM +0200, Jan Kiszka wrote:
> On 2011-04-27 14:54, Michael S. Tsirkin wrote:
> > On Sat, Apr 23, 2011 at 12:23:38PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Testing support and allocating a GSI for an MSI message is required both
> >> for MSI and MSI-X. At this chance, drop the aging version warning.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > No objection, but I do note that this means running on an old
> > kernel will lead to a silent failure.
> 
> Shouldn't be silent: The caller of msi_vector_use should check and
> process the error (virtio should even forward it to guest IIUC).

It does, but the msi/msix spec does not allow this so not
all guest OSes can use this extended reporting.

> > stderr output is not in fact much better: I think we should
> > check the capability in msix_init. Care coding this up?
> 
> I think the motivation to check on vector activation is that devices and
> guests without a need for MSI should not cause a failure if MSI is
> unsupported by KVM.
> 
> Jan

IMO it's better not to report MSI capability in this setup.


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

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Jan Kiszka]

On 2011-04-27 15:34, Michael S. Tsirkin wrote:
> On Sat, Apr 23, 2011 at 12:23:35PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This structure will be used for legacy MSI as well and will become part
>> of the KVM API.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I'd like KVMMSIMessage better. struct kvm_msi_message would be even
> better but oh well.

Yeah, QEMU naming convention is not designed for such use cases... Will
change it as you prefer.

> 
>> ---
>>  hw/msix.c |   10 +++++-----
>>  hw/pci.h  |   10 ++--------
>>  kvm.h     |    7 +++++++
>>  3 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/msix.c b/hw/msix.c
>> index 8b68a6a..42870c5 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -51,7 +51,7 @@ int msix_supported;
>>  static void kvm_msix_free(PCIDevice *dev)
>>  {
>>      int vector, changed = 0;
>> -    struct kvm_msix_message *kmm;
>> +    KVMMsiMessage *kmm;
>>  
>>      for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
>>          if (dev->msix_entry_used[vector]) {
>> @@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev)
>>  }
>>  
>>  static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
>> -                                         struct kvm_msix_message *kmm)
>> +                                         KVMMsiMessage *kmm)
>>  {
>>      uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE;
>>  
>> @@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
>>  static void kvm_msix_update(PCIDevice *dev, int vector,
>>                              int was_masked, int is_masked)
>>  {
>> -    struct kvm_msix_message e = {}, *entry;
>> +    KVMMsiMessage e = {}, *entry;
>>      int mask_cleared = was_masked && !is_masked;
>>      /* It is only legal to change an entry when it is masked. Therefore, it is
>>       * enough to update the routing in kernel when mask is being cleared. */
>> @@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
>>  
>>  static int kvm_msix_add(PCIDevice *dev, unsigned vector)
>>  {
>> -    struct kvm_msix_message *kmm = dev->msix_irq_entries + vector;
>> +    KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
>>      int r;
>>  
>>      if (!kvm_has_gsi_routing()) {
>> @@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
>>  
>>  static void kvm_msix_del(PCIDevice *dev, unsigned vector)
>>  {
>> -    struct kvm_msix_message *kmm;
>> +    KVMMsiMessage *kmm;
>>  
>>      if (dev->msix_entry_used[vector]) {
>>          return;
>> diff --git a/hw/pci.h b/hw/pci.h
>> index dd09494..dc5df17 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -5,6 +5,7 @@
>>  #include "qobject.h"
>>  
>>  #include "qdev.h"
>> +#include "kvm.h"
> 
> I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
> is anything wrong with that?  Maybe just rename it to make it generic
> for msi and leave if where it is.

kvm.h shall provide kvm related types, not some unrelated header. That's
even more important with MSI support for non-PCI devices (aka HPET).

Jan

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

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Avi Kivity]

On 04/27/2011 05:04 PM, Jan Kiszka wrote:
> >
> >  I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
> >  is anything wrong with that?  Maybe just rename it to make it generic
> >  for msi and leave if where it is.
>
> kvm.h shall provide kvm related types, not some unrelated header. That's
> even more important with MSI support for non-PCI devices (aka HPET).

We could have an MSIMessage type that abstracts the general facility, 
and let it embed a KVMMsiMessage that contains just the gsi.

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

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Jan Kiszka]

On 2011-04-27 16:29, Avi Kivity wrote:
> On 04/27/2011 05:04 PM, Jan Kiszka wrote:
>>>
>>>  I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
>>>  is anything wrong with that?  Maybe just rename it to make it generic
>>>  for msi and leave if where it is.
>>
>> kvm.h shall provide kvm related types, not some unrelated header. That's
>> even more important with MSI support for non-PCI devices (aka HPET).
> 
> We could have an MSIMessage type that abstracts the general facility, 
> and let it embed a KVMMsiMessage that contains just the gsi.

Yes, likely also useful for generic MSI delivery services that bypass
stl_phys.

Jan

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

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 04:04:44PM +0200, Jan Kiszka wrote:
> On 2011-04-27 15:34, Michael S. Tsirkin wrote:
> > On Sat, Apr 23, 2011 at 12:23:35PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> This structure will be used for legacy MSI as well and will become part
> >> of the KVM API.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > I'd like KVMMSIMessage better. struct kvm_msi_message would be even
> > better but oh well.
> 
> Yeah, QEMU naming convention is not designed for such use cases... Will
> change it as you prefer.
> 
> > 
> >> ---
> >>  hw/msix.c |   10 +++++-----
> >>  hw/pci.h  |   10 ++--------
> >>  kvm.h     |    7 +++++++
> >>  3 files changed, 14 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/msix.c b/hw/msix.c
> >> index 8b68a6a..42870c5 100644
> >> --- a/hw/msix.c
> >> +++ b/hw/msix.c
> >> @@ -51,7 +51,7 @@ int msix_supported;
> >>  static void kvm_msix_free(PCIDevice *dev)
> >>  {
> >>      int vector, changed = 0;
> >> -    struct kvm_msix_message *kmm;
> >> +    KVMMsiMessage *kmm;
> >>  
> >>      for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
> >>          if (dev->msix_entry_used[vector]) {
> >> @@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev)
> >>  }
> >>  
> >>  static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
> >> -                                         struct kvm_msix_message *kmm)
> >> +                                         KVMMsiMessage *kmm)
> >>  {
> >>      uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE;
> >>  
> >> @@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector,
> >>  static void kvm_msix_update(PCIDevice *dev, int vector,
> >>                              int was_masked, int is_masked)
> >>  {
> >> -    struct kvm_msix_message e = {}, *entry;
> >> +    KVMMsiMessage e = {}, *entry;
> >>      int mask_cleared = was_masked && !is_masked;
> >>      /* It is only legal to change an entry when it is masked. Therefore, it is
> >>       * enough to update the routing in kernel when mask is being cleared. */
> >> @@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector,
> >>  
> >>  static int kvm_msix_add(PCIDevice *dev, unsigned vector)
> >>  {
> >> -    struct kvm_msix_message *kmm = dev->msix_irq_entries + vector;
> >> +    KVMMsiMessage *kmm = dev->msix_irq_entries + vector;
> >>      int r;
> >>  
> >>      if (!kvm_has_gsi_routing()) {
> >> @@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector)
> >>  
> >>  static void kvm_msix_del(PCIDevice *dev, unsigned vector)
> >>  {
> >> -    struct kvm_msix_message *kmm;
> >> +    KVMMsiMessage *kmm;
> >>  
> >>      if (dev->msix_entry_used[vector]) {
> >>          return;
> >> diff --git a/hw/pci.h b/hw/pci.h
> >> index dd09494..dc5df17 100644
> >> --- a/hw/pci.h
> >> +++ b/hw/pci.h
> >> @@ -5,6 +5,7 @@
> >>  #include "qobject.h"
> >>  
> >>  #include "qdev.h"
> >> +#include "kvm.h"
> > 
> > I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
> > is anything wrong with that?  Maybe just rename it to make it generic
> > for msi and leave if where it is.
> 
> kvm.h shall provide kvm related types, not some unrelated header. That's
> even more important with MSI support for non-PCI devices (aka HPET).
> 
> Jan

Generally qemu suggests forward-declaring types so all users don't
need to pull them in. This should certainly work here:
declare the type in qemu-common.h, then msix.c and kvm are
the only ones that need to include kvm.h


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

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 05:29:10PM +0300, Avi Kivity wrote:
> On 04/27/2011 05:04 PM, Jan Kiszka wrote:
> >>
> >>  I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
> >>  is anything wrong with that?  Maybe just rename it to make it generic
> >>  for msi and leave if where it is.
> >
> >kvm.h shall provide kvm related types, not some unrelated header. That's
> >even more important with MSI support for non-PCI devices (aka HPET).
> 
> We could have an MSIMessage type that abstracts the general
> facility, and let it embed a KVMMsiMessage that contains just the
> gsi.

The whole structure is only needed for kvm though: no one
else seems to want an MSIMessage.

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

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Avi Kivity]

On 04/27/2011 05:30 PM, Jan Kiszka wrote:
> On 2011-04-27 16:29, Avi Kivity wrote:
> >  On 04/27/2011 05:04 PM, Jan Kiszka wrote:
> >>>
> >>>   I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
> >>>   is anything wrong with that?  Maybe just rename it to make it generic
> >>>   for msi and leave if where it is.
> >>
> >>  kvm.h shall provide kvm related types, not some unrelated header. That's
> >>  even more important with MSI support for non-PCI devices (aka HPET).
> >
> >  We could have an MSIMessage type that abstracts the general facility,
> >  and let it embed a KVMMsiMessage that contains just the gsi.
>
> Yes, likely also useful for generic MSI delivery services that bypass
> stl_phys.
>

Right, so you can cache the phys_page and apic lookups.

A different layer in which to accomplish this is to have a per-device 
tlb.  So we'd have

     CachedPhysicalAddress msi_fsb_tlb;

     stl_phys_tlb(&msi_fsb_cache, addr, data);

which caches the addr lookup in a 1-entry tlb.  If we have many non-msi 
repeated device writes to the same address, this might be worthwhile.  
 From a quick look at the code, I don't think we do, though.

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

Re: [PATCH 7/7] qemu-kvm: Add in-kernel irqchip support for MSI

[Jan Kiszka]

On 2011-04-27 15:31, Michael S. Tsirkin wrote:
> On Sat, Apr 23, 2011 at 12:23:40PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This adds the required bits to map classic MSI vectors on KVM IRQ
>> routing entries and deliver them via the irqchip if enabled.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/msi.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/pci.h |    4 ++
>>  2 files changed, 115 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 3dc3a24..18f683b 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -20,6 +20,7 @@
>>  
>>  #include "msi.h"
>>  #include "range.h"
>> +#include "kvm.h"
>>  
>>  /* Eventually those constants should go to Linux pci_regs.h */
>>  #define PCI_MSI_PENDING_32      0x10
>> @@ -109,6 +110,92 @@ bool msi_enabled(const PCIDevice *dev)
>>           PCI_MSI_FLAGS_ENABLE);
>>  }
>>  
>> +static void kvm_msi_message_from_vector(PCIDevice *dev, unsigned vector,
>> +                                        KVMMsiMessage *kmm)
>> +{
>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>> +    unsigned int nr_vectors = msi_nr_vectors(flags);
>> +
>> +    kmm->addr_lo = pci_get_long(dev->config + msi_address_lo_off(dev));
>> +    if (msi64bit) {
>> +        kmm->addr_hi = pci_get_long(dev->config + msi_address_hi_off(dev));
>> +    } else {
>> +        kmm->addr_hi = 0;
>> +    }
>> +
>> +    kmm->data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
>> +    if (nr_vectors > 1) {
>> +        kmm->data &= ~(nr_vectors - 1);
>> +        kmm->data |= vector;
>> +    }
> 
> msi_notify already has similar logic.
> Let's add msi_get_vector_address and msi_get_vector_data
> 

Ok.

> 
>> +}
>> +
>> +static void kvm_msi_update(PCIDevice *dev)
>> +{
>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>> +    unsigned int nr_vectors = msi_nr_vectors(flags);
>> +    KVMMsiMessage new_entry, *entry;
>> +    bool changed = false;
>> +    unsigned int vector;
>> +    int r;
>> +
>> +    for (vector = 0; vector < 32; vector++) {
> 
> I'd say up to log_max_vecs: the array is allocated
> with nr_vectors.

Right.

> 
>> +        entry = dev->msi_irq_entries + vector;
>> +
>> +        if (vector >= nr_vectors) {
>> +            if (vector < dev->msi_entries_nr) {
>> +                kvm_msi_message_del(entry);
>> +                changed = true;
>> +            }
>> +        } else if (vector >= dev->msi_entries_nr) {
>> +            kvm_msi_message_from_vector(dev, vector, entry);
>> +            r = kvm_msi_message_add(entry);
>> +            if (r) {
>> +                fprintf(stderr, "%s: kvm_msi_add failed: %s\n", __func__,
>> +                        strerror(-r));
>> +                exit(1);
>> +            }
>> +            changed = true;
>> +        } else {
>> +            kvm_msi_message_from_vector(dev, vector, &new_entry);
>> +            r = kvm_msi_message_update(entry, &new_entry);
>> +            if (r < 0) {
>> +                fprintf(stderr, "%s: kvm_update_msi failed: %s\n",
>> +                        __func__, strerror(-r));
>> +                exit(1);
>> +            }
>> +            if (r > 0) {
>> +                *entry = new_entry;
>> +                changed = true;
>> +            }
>> +        }
>> +    }
>> +    dev->msi_entries_nr = nr_vectors;
>> +    if (changed) {
>> +        r = kvm_commit_irq_routes();
>> +        if (r) {
>> +            fprintf(stderr, "%s: kvm_commit_irq_routes failed: %s\n", __func__,
>> +                    strerror(-r));
>> +            exit(1);
>> +        }
>> +    }
>> +}
>> +
>> +/* KVM specific MSI helpers */
>> +static void kvm_msi_free(PCIDevice *dev)
>> +{
>> +    unsigned int vector;
>> +
>> +    for (vector = 0; vector < dev->msi_entries_nr; ++vector) {
>> +        kvm_msi_message_del(&dev->msi_irq_entries[vector]);
>> +    }
>> +    if (dev->msi_entries_nr > 0) {
>> +        kvm_commit_irq_routes();
>> +    }
>> +    dev->msi_entries_nr = 0;
>> +}
>> +
>>  int msi_init(struct PCIDevice *dev, uint8_t offset,
>>               unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
>>  {
>> @@ -159,6 +246,12 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>>          pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
>>                       0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
>>      }
>> +
>> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>> +        dev->msi_irq_entries = qemu_malloc(nr_vectors *
>> +                                           sizeof(*dev->msix_irq_entries));
>> +    }
>> +
>>      return config_offset;
>>  }
>>  
>> @@ -166,6 +259,11 @@ void msi_uninit(struct PCIDevice *dev)
>>  {
>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>      uint8_t cap_size = msi_cap_sizeof(flags);
>> +
>> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>> +        kvm_msi_free(dev);
>> +        qemu_free(dev->msi_irq_entries);
>> +    }
>>      pci_del_capability(dev, PCI_CAP_ID_MSIX, cap_size);
>>      MSI_DEV_PRINTF(dev, "uninit\n");
>>  }
>> @@ -175,6 +273,10 @@ void msi_reset(PCIDevice *dev)
>>      uint16_t flags;
>>      bool msi64bit;
>>  
>> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>> +        kvm_msi_free(dev);
>> +    }
>> +
>>      flags = pci_get_word(dev->config + msi_flags_off(dev));
>>      flags &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
>>      msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>> @@ -224,6 +326,11 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
>>          return;
>>      }
>>  
>> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>> +        kvm_set_irq(dev->msi_irq_entries[vector].gsi, 1, NULL);
>> +        return;
>> +    }
>> +
>>      if (msi64bit) {
>>          address = pci_get_quad(dev->config + msi_address_lo_off(dev));
>>      } else {
>> @@ -312,6 +419,10 @@ void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
>>          pci_set_word(dev->config + msi_flags_off(dev), flags);
>>      }
>>  
>> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>> +        kvm_msi_update(dev);
>> +    }
>> +
> 
> Does this ignore per vector masking then?

The hook has to be placed here independently.

But, yes, I do not consider to optional mask in kvm_msi_update. Not sure
if it would buy us anything, but it could still be added on top later on.

Jan

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

Re: [PATCH 2/7] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Jan Kiszka]

On 2011-04-27 16:40, Avi Kivity wrote:
> On 04/27/2011 05:30 PM, Jan Kiszka wrote:
>> On 2011-04-27 16:29, Avi Kivity wrote:
>>>  On 04/27/2011 05:04 PM, Jan Kiszka wrote:
>>>>>
>>>>>   I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h
>>>>>   is anything wrong with that?  Maybe just rename it to make it generic
>>>>>   for msi and leave if where it is.
>>>>
>>>>  kvm.h shall provide kvm related types, not some unrelated header. That's
>>>>  even more important with MSI support for non-PCI devices (aka HPET).
>>>
>>>  We could have an MSIMessage type that abstracts the general facility,
>>>  and let it embed a KVMMsiMessage that contains just the gsi.
>>
>> Yes, likely also useful for generic MSI delivery services that bypass
>> stl_phys.
>>
> 
> Right, so you can cache the phys_page and apic lookups.
> 
> A different layer in which to accomplish this is to have a per-device 
> tlb.  So we'd have
> 
>      CachedPhysicalAddress msi_fsb_tlb;
> 
>      stl_phys_tlb(&msi_fsb_cache, addr, data);
> 
> which caches the addr lookup in a 1-entry tlb.  If we have many non-msi 
> repeated device writes to the same address, this might be worthwhile.  
>  From a quick look at the code, I don't think we do, though.

There is no caching need if the messages targets the fixed MSI address
window. We can simply pass it directly to the APIC then.

Jan

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