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

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

[Jan Kiszka]

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not
only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
Moreover, it contains an additional patch to refresh the MSI IRQ routes
after vmload.



Jan Kiszka (9):
  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
  qemu-kvm: Refresh MSI settings after vmload
  qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

 hw/hpet.c  |   71 ++++++++++++++++++++++++++++++++++-
 hw/msi.c   |  124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/msi.h   |    1 +
 hw/msix.c  |   73 ++++++++++++------------------------
 hw/pci.c   |    2 +
 hw/pci.h   |   16 +++-----
 kvm.h      |   20 ++++++----
 qemu-kvm.c |   57 ++++++++++++++++++----------
 8 files changed, 276 insertions(+), 88 deletions(-)

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

[Jan Kiszka]

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 v2 2/9] qemu-kvm: Rename kvm_msix_message to KVMMsiMessage

[Jan Kiszka]

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 v2 3/9] qemu-kvm: Refactor MSI core API of KVM

[Jan Kiszka]

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 v2 4/9] qemu-kvm: Fix and clean up msix vector use/unuse hooks

[Jan Kiszka]

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 v2 5/9] qemu-kvm: Move gsi bits from kvm_msix_vector_add to kvm_msi_add_message

[Jan Kiszka]

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 v2 6/9] qemu-kvm: Move entry comparison into kvm_msi_update_message

[Jan Kiszka]

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 v2 7/9] qemu-kvm: Add in-kernel irqchip support for MSI

[Jan Kiszka]

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

[PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Jan Kiszka]

Establish a post-load notification for the MSI subsystem so that KVM can
refresh its IRQ routing after vmload.

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

diff --git a/hw/msi.c b/hw/msi.c
index 18f683b..0cbff17 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
     return msi_nr_vectors(flags);
 }
+
+void msi_post_load(PCIDevice *dev)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_msi_free(dev);
+
+        if (flags & PCI_MSI_FLAGS_ENABLE) {
+            kvm_msi_update(dev);
+        }
+    }
+}
diff --git a/hw/msi.h b/hw/msi.h
index 5766018..6ff0607 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev);
 void msi_notify(PCIDevice *dev, unsigned int vector);
 void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
 unsigned int msi_nr_vectors_allocated(const PCIDevice *dev);
+void msi_post_load(PCIDevice *dev);
 
 static inline bool msi_present(const PCIDevice *dev)
 {
diff --git a/hw/pci.c b/hw/pci.c
index 82e0300..07ec4f9 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 
     pci_update_mappings(s);
+    msi_post_load(s);
 
     qemu_free(config);
     return 0;
-- 
1.7.1

[PATCH v2 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Jan Kiszka]

Just like PCI devices, the HPET requires special care to route MSI
events to the in-kernel irqchip if enabled.

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

diff --git a/hw/hpet.c b/hw/hpet.c
index 6ce07bc..e773ed8 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -31,6 +31,7 @@
 #include "hpet_emul.h"
 #include "sysbus.h"
 #include "mc146818rtc.h"
+#include "kvm.h"
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -55,6 +56,7 @@ typedef struct HPETTimer {  /* timers */
     uint8_t wrap_flag;      /* timer pop will indicate wrap for one-shot 32-bit
                              * mode. Next pop will be actual timer expiration.
                              */
+    KVMMsiMessage kmm;
 } HPETTimer;
 
 typedef struct HPETState {
@@ -141,11 +143,59 @@ static int deactivating_bit(uint64_t old, uint64_t new, uint64_t mask)
     return ((old & mask) && !(new & mask));
 }
 
+static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
+{
+    return (old ^ new) & mask;
+}
+
 static uint64_t hpet_get_ticks(HPETState *s)
 {
     return ns_to_ticks(qemu_get_clock_ns(vm_clock) + s->hpet_offset);
 }
 
+static void kvm_hpet_msi_update(HPETTimer *t)
+{
+    KVMMsiMessage new_entry;
+    int ret = 0;
+
+    if (!kvm_enabled() || !kvm_irqchip_in_kernel()) {
+        return;
+    }
+
+    if (timer_fsb_route(t)) {
+        new_entry.addr_hi = 0;
+        new_entry.addr_lo = t->fsb >> 32;
+        new_entry.data = t->fsb & 0xffffffff;
+        if (t->kmm.gsi == -1) {
+            kvm_msi_message_add(&new_entry);
+            ret = 1;
+        } else {
+            ret = kvm_msi_message_update(&t->kmm, &new_entry);
+        }
+        if (ret > 0) {
+            t->kmm = new_entry;
+            kvm_commit_irq_routes();
+        }
+    } else if (t->kmm.gsi != -1) {
+        kvm_msi_message_del(&t->kmm);
+        t->kmm.gsi = -1;
+    }
+}
+
+static void kvm_hpet_msi_free(HPETState *s)
+{
+    int i;
+
+    for (i = 0; i < s->num_timers; i++) {
+        HPETTimer *timer = &s->timer[i];
+
+        if (timer->kmm.gsi != -1) {
+            kvm_msi_message_del(&timer->kmm);
+            timer->kmm.gsi = -1;
+        }
+    }
+}
+
 /*
  * calculate diff between comparator value and current ticks
  */
@@ -192,7 +242,11 @@ static void update_irq(struct HPETTimer *timer, int set)
             qemu_irq_lower(s->irqs[route]);
         }
     } else if (timer_fsb_route(timer)) {
-        stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+        if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+            kvm_set_irq(timer->kmm.gsi, 1, NULL);
+        } else {
+            stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+        }
     } else if (timer->config & HPET_TN_TYPE_LEVEL) {
         s->isr |= mask;
         qemu_irq_raise(s->irqs[route]);
@@ -214,6 +268,8 @@ static int hpet_pre_load(void *opaque)
 {
     HPETState *s = opaque;
 
+    kvm_hpet_msi_free(s);
+
     /* version 1 only supports 3, later versions will load the actual value */
     s->num_timers = HPET_MIN_TIMERS;
     return 0;
@@ -222,6 +278,7 @@ static int hpet_pre_load(void *opaque)
 static int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;
+    int i;
 
     /* Recalculate the offset between the main counter and guest time */
     s->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_get_clock_ns(vm_clock);
@@ -241,6 +298,10 @@ static int hpet_post_load(void *opaque, int version_id)
         hpet_pit_disable();
     }
 
+    for (i = 0; i < s->num_timers; i++) {
+        kvm_hpet_msi_update(&s->timer[i]);
+    }
+
     return 0;
 }
 
@@ -485,6 +546,9 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
             } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE)) {
                 hpet_del_timer(timer);
             }
+            if (modifying_bit(old_val, new_val, HPET_TN_FSB_ENABLE)) {
+                kvm_hpet_msi_update(timer);
+            }
             break;
         case HPET_TN_CFG + 4: // Interrupt capabilities
             DPRINTF("qemu: invalid HPET_TN_CFG+4 write\n");
@@ -533,9 +597,11 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                 break;
         case HPET_TN_ROUTE:
             timer->fsb = (timer->fsb & 0xffffffff00000000ULL) | new_val;
+            kvm_hpet_msi_update(timer);
             break;
         case HPET_TN_ROUTE + 4:
             timer->fsb = (new_val << 32) | (timer->fsb & 0xffffffff);
+            kvm_hpet_msi_update(timer);
             break;
         default:
             DPRINTF("qemu: invalid hpet_ram_writel\n");
@@ -667,6 +733,8 @@ static void hpet_reset(DeviceState *d)
     hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
     hpet_cfg.hpet[s->hpet_id].address = sysbus_from_qdev(d)->mmio[0].addr;
     count = 1;
+
+    kvm_hpet_msi_free(s);
 }
 
 static void hpet_handle_rtc_irq(void *opaque, int n, int level)
@@ -711,6 +779,7 @@ static int hpet_init(SysBusDevice *dev)
         timer->qemu_timer = qemu_new_timer_ns(vm_clock, hpet_timer, timer);
         timer->tn = i;
         timer->state = s;
+        timer->kmm.gsi = -1;
     }
 
     /* 64-bit main counter; LegacyReplacementRoute. */
-- 
1.7.1

Re: [PATCH v2 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Michael Tokarev]

26.04.2011 17:19, Jan Kiszka wrote:

>  hw/hpet.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-

> +static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
> +{
> +    return (old ^ new) & mask;
> +}

Such constructs always look suspicious.  I'm not even sure anymore
(after using C for over 20 years ;) that this works... how about

 return (old ^ new) & mask  ? 1 : 0;

instead, or something along this?  I mean, if sizeof(int)==4, how
`return 1ULL<<32' will be interpreted in this context?  Tiny test
program tells me it will return 0...

/mjt

Re: [PATCH v2 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Jan Kiszka]

On 2011-04-26 15:30, Michael Tokarev wrote:
> 26.04.2011 17:19, Jan Kiszka wrote:
> 
>>  hw/hpet.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 
>> +static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
>> +{
>> +    return (old ^ new) & mask;
>> +}
> 
> Such constructs always look suspicious.  I'm not even sure anymore
> (after using C for over 20 years ;) that this works... how about
> 
>  return (old ^ new) & mask  ? 1 : 0;
> 
> instead, or something along this?  I mean, if sizeof(int)==4, how
> `return 1ULL<<32' will be interpreted in this context?  Tiny test
> program tells me it will return 0...

Good catch, will fix (doesn't bite use here, flags fit into 32 bit, but
nevertheless).

Jan

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

Re: [PATCH v2 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Avi Kivity]

On 04/26/2011 04:55 PM, Jan Kiszka wrote:
> On 2011-04-26 15:30, Michael Tokarev wrote:
> >  26.04.2011 17:19, Jan Kiszka wrote:
> >
> >>   hw/hpet.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >
> >>  +static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
> >>  +{
> >>  +    return (old ^ new)&  mask;
> >>  +}
> >
> >  Such constructs always look suspicious.  I'm not even sure anymore
> >  (after using C for over 20 years ;) that this works... how about
> >
> >   return (old ^ new)&  mask  ? 1 : 0;
> >
> >  instead, or something along this?  I mean, if sizeof(int)==4, how
> >  `return 1ULL<<32' will be interpreted in this context?  Tiny test
> >  program tells me it will return 0...
>
> Good catch, will fix (doesn't bite use here, flags fit into 32 bit, but
> nevertheless).
>

Note, a bool return type works here.

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

Re: [PATCH v2 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Jan Kiszka]

On 2011-04-26 15:56, Avi Kivity wrote:
> On 04/26/2011 04:55 PM, Jan Kiszka wrote:
>> On 2011-04-26 15:30, Michael Tokarev wrote:
>>>  26.04.2011 17:19, Jan Kiszka wrote:
>>>
>>>>   hw/hpet.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>
>>>>  +static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
>>>>  +{
>>>>  +    return (old ^ new)&  mask;
>>>>  +}
>>>
>>>  Such constructs always look suspicious.  I'm not even sure anymore
>>>  (after using C for over 20 years ;) that this works... how about
>>>
>>>   return (old ^ new)&  mask  ? 1 : 0;
>>>
>>>  instead, or something along this?  I mean, if sizeof(int)==4, how
>>>  `return 1ULL<<32' will be interpreted in this context?  Tiny test
>>>  program tells me it will return 0...
>>
>> Good catch, will fix (doesn't bite use here, flags fit into 32 bit, but
>> nevertheless).
>>
> 
> Note, a bool return type works here.

Yes, that was my favorite as well.

Jan

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

[PATCH v3 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Jan Kiszka]

Just like PCI devices, the HPET requires special care to route MSI
events to the in-kernel irqchip if enabled.

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

v3: Let modifying_bit return bool to avoid unexpected results.

diff --git a/hw/hpet.c b/hw/hpet.c
index 6ce07bc..ed8018d 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -31,6 +31,7 @@
 #include "hpet_emul.h"
 #include "sysbus.h"
 #include "mc146818rtc.h"
+#include "kvm.h"
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -55,6 +56,7 @@ typedef struct HPETTimer {  /* timers */
     uint8_t wrap_flag;      /* timer pop will indicate wrap for one-shot 32-bit
                              * mode. Next pop will be actual timer expiration.
                              */
+    KVMMsiMessage kmm;
 } HPETTimer;
 
 typedef struct HPETState {
@@ -141,11 +143,59 @@ static int deactivating_bit(uint64_t old, uint64_t new, uint64_t mask)
     return ((old & mask) && !(new & mask));
 }
 
+static bool modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
+{
+    return (old ^ new) & mask;
+}
+
 static uint64_t hpet_get_ticks(HPETState *s)
 {
     return ns_to_ticks(qemu_get_clock_ns(vm_clock) + s->hpet_offset);
 }
 
+static void kvm_hpet_msi_update(HPETTimer *t)
+{
+    KVMMsiMessage new_entry;
+    int ret = 0;
+
+    if (!kvm_enabled() || !kvm_irqchip_in_kernel()) {
+        return;
+    }
+
+    if (timer_fsb_route(t)) {
+        new_entry.addr_hi = 0;
+        new_entry.addr_lo = t->fsb >> 32;
+        new_entry.data = t->fsb & 0xffffffff;
+        if (t->kmm.gsi == -1) {
+            kvm_msi_message_add(&new_entry);
+            ret = 1;
+        } else {
+            ret = kvm_msi_message_update(&t->kmm, &new_entry);
+        }
+        if (ret > 0) {
+            t->kmm = new_entry;
+            kvm_commit_irq_routes();
+        }
+    } else if (t->kmm.gsi != -1) {
+        kvm_msi_message_del(&t->kmm);
+        t->kmm.gsi = -1;
+    }
+}
+
+static void kvm_hpet_msi_free(HPETState *s)
+{
+    int i;
+
+    for (i = 0; i < s->num_timers; i++) {
+        HPETTimer *timer = &s->timer[i];
+
+        if (timer->kmm.gsi != -1) {
+            kvm_msi_message_del(&timer->kmm);
+            timer->kmm.gsi = -1;
+        }
+    }
+}
+
 /*
  * calculate diff between comparator value and current ticks
  */
@@ -192,7 +242,11 @@ static void update_irq(struct HPETTimer *timer, int set)
             qemu_irq_lower(s->irqs[route]);
         }
     } else if (timer_fsb_route(timer)) {
-        stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+        if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+            kvm_set_irq(timer->kmm.gsi, 1, NULL);
+        } else {
+            stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+        }
     } else if (timer->config & HPET_TN_TYPE_LEVEL) {
         s->isr |= mask;
         qemu_irq_raise(s->irqs[route]);
@@ -214,6 +268,8 @@ static int hpet_pre_load(void *opaque)
 {
     HPETState *s = opaque;
 
+    kvm_hpet_msi_free(s);
+
     /* version 1 only supports 3, later versions will load the actual value */
     s->num_timers = HPET_MIN_TIMERS;
     return 0;
@@ -222,6 +278,7 @@ static int hpet_pre_load(void *opaque)
 static int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;
+    int i;
 
     /* Recalculate the offset between the main counter and guest time */
     s->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_get_clock_ns(vm_clock);
@@ -241,6 +298,10 @@ static int hpet_post_load(void *opaque, int version_id)
         hpet_pit_disable();
     }
 
+    for (i = 0; i < s->num_timers; i++) {
+        kvm_hpet_msi_update(&s->timer[i]);
+    }
+
     return 0;
 }
 
@@ -485,6 +546,9 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
             } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE)) {
                 hpet_del_timer(timer);
             }
+            if (modifying_bit(old_val, new_val, HPET_TN_FSB_ENABLE)) {
+                kvm_hpet_msi_update(timer);
+            }
             break;
         case HPET_TN_CFG + 4: // Interrupt capabilities
             DPRINTF("qemu: invalid HPET_TN_CFG+4 write\n");
@@ -533,9 +597,11 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                 break;
         case HPET_TN_ROUTE:
             timer->fsb = (timer->fsb & 0xffffffff00000000ULL) | new_val;
+            kvm_hpet_msi_update(timer);
             break;
         case HPET_TN_ROUTE + 4:
             timer->fsb = (new_val << 32) | (timer->fsb & 0xffffffff);
+            kvm_hpet_msi_update(timer);
             break;
         default:
             DPRINTF("qemu: invalid hpet_ram_writel\n");
@@ -667,6 +733,8 @@ static void hpet_reset(DeviceState *d)
     hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
     hpet_cfg.hpet[s->hpet_id].address = sysbus_from_qdev(d)->mmio[0].addr;
     count = 1;
+
+    kvm_hpet_msi_free(s);
 }
 
 static void hpet_handle_rtc_irq(void *opaque, int n, int level)
@@ -711,6 +779,7 @@ static int hpet_init(SysBusDevice *dev)
         timer->qemu_timer = qemu_new_timer_ns(vm_clock, hpet_timer, timer);
         timer->tn = i;
         timer->state = s;
+        timer->kmm.gsi = -1;
     }
 
     /* 64-bit main counter; LegacyReplacementRoute. */
-- 
1.7.1

Re: [PATCH v3 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Christoph Hellwig]

On Tue, Apr 26, 2011 at 04:01:00PM +0200, Jan Kiszka wrote:
> +static bool modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
> +{
> +    return (old ^ new) & mask;
> +}
> +

A more usual name would be toggle_bit.  But you're passing in a mask
to be modified, so it would be more a toggle_bits or toggle_mask.

Re: [PATCH v3 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Jan Kiszka]

On 2011-04-26 18:06, Christoph Hellwig wrote:
> On Tue, Apr 26, 2011 at 04:01:00PM +0200, Jan Kiszka wrote:
>> +static bool modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
>> +{
>> +    return (old ^ new) & mask;
>> +}
>> +
> 
> A more usual name would be toggle_bit.  But you're passing in a mask
> to be modified, so it would be more a toggle_bits or toggle_mask.
> 

This function is checking for a change. But I've no problem renaming it
to toggling_bit or so if that's preferred.

Jan

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

Re: [PATCH v3 9/9] qemu-kvm: hpet: Add MSI support for in-kernel irqchip mode

[Christoph Hellwig]

On Tue, Apr 26, 2011 at 07:06:34PM +0200, Jan Kiszka wrote:
> On 2011-04-26 18:06, Christoph Hellwig wrote:
> > On Tue, Apr 26, 2011 at 04:01:00PM +0200, Jan Kiszka wrote:
> >> +static bool modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
> >> +{
> >> +    return (old ^ new) & mask;
> >> +}
> >> +
> > 
> > A more usual name would be toggle_bit.  But you're passing in a mask
> > to be modified, so it would be more a toggle_bits or toggle_mask.
> > 
> 
> This function is checking for a change. But I've no problem renaming it
> to toggling_bit or so if that's preferred.

Err, you're right, I read it as doing a ^=.  So keeping your naming
scheme sounds fine.

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

[Avi Kivity]

On 04/26/2011 04:19 PM, Jan Kiszka wrote:
> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
> that can wait until we go upstream.
>
> This version still makes classic MSI usable in irqchip mode, now not
> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
> Moreover, it contains an additional patch to refresh the MSI IRQ routes
> after vmload.
>

Patches 1-8 applied, thanks.  I'm not sure about 9 (hpet kvm msi 
integration) - it seems very intrusive to do this to every 
msi-supporting device.  At least for pci we get all pci devices done in 
one shot.

We could do this transparently in hw/apic.c.  When the message is sent 
for the first time we look it up, fail, and update the kvm routing 
entry.  Next time the lookup succeeds and we just use KVM_IRQ_LINE, 
until the message changes and we need to update the irq entry again.

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

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

[Jan Kiszka]

On 2011-04-27 09:27, Avi Kivity wrote:
> On 04/26/2011 04:19 PM, Jan Kiszka wrote:
>> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
>> that can wait until we go upstream.
>>
>> This version still makes classic MSI usable in irqchip mode, now not
>> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
>> Moreover, it contains an additional patch to refresh the MSI IRQ routes
>> after vmload.
>>
> 
> Patches 1-8 applied, thanks.  I'm not sure about 9 (hpet kvm msi 
> integration) - it seems very intrusive to do this to every 
> msi-supporting device.  At least for pci we get all pci devices done in 
> one shot.

Right, it is a but intrusive, but I do not see any real alternative.

> 
> We could do this transparently in hw/apic.c.  When the message is sent 
> for the first time we look it up, fail, and update the kvm routing 
> entry.  Next time the lookup succeeds and we just use KVM_IRQ_LINE, 
> until the message changes and we need to update the irq entry again.

I thought about this, also for PCI devices that aren't assigned or
vhost-driven, but we would quickly end up with unused and never freed
IRQ routing entries. We still need to track the vector configurations.

What would help at least in the HPET case is a new DELIVER_MSI syscall
that completely skips the IRQ routing thing. Actually, we only need
routing for IRQs that shall be injected directly at kernel level. OTOH,
this service would not be available on existing kernels, and we would
not be able to simplify the PCI code that way (due to vhost
requirements). So I dropped this idea as well and accepted that IRQ
routing is the way to go.

Jan

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

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

[Avi Kivity]

On 04/27/2011 12:00 PM, Jan Kiszka wrote:
> On 2011-04-27 09:27, Avi Kivity wrote:
> >  On 04/26/2011 04:19 PM, Jan Kiszka wrote:
> >>  I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
> >>  that can wait until we go upstream.
> >>
> >>  This version still makes classic MSI usable in irqchip mode, now not
> >>  only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
> >>  Moreover, it contains an additional patch to refresh the MSI IRQ routes
> >>  after vmload.
> >>
> >
> >  Patches 1-8 applied, thanks.  I'm not sure about 9 (hpet kvm msi
> >  integration) - it seems very intrusive to do this to every
> >  msi-supporting device.  At least for pci we get all pci devices done in
> >  one shot.
>
> Right, it is a but intrusive, but I do not see any real alternative.
>
> >
> >  We could do this transparently in hw/apic.c.  When the message is sent
> >  for the first time we look it up, fail, and update the kvm routing
> >  entry.  Next time the lookup succeeds and we just use KVM_IRQ_LINE,
> >  until the message changes and we need to update the irq entry again.
>
> I thought about this, also for PCI devices that aren't assigned or
> vhost-driven, but we would quickly end up with unused and never freed
> IRQ routing entries. We still need to track the vector configurations.

We can simply drop all route entries that are used exclusively in qemu 
(i.e. not bound to an irqfd) and let the cache rebuild itself.

> What would help at least in the HPET case is a new DELIVER_MSI syscall
> that completely skips the IRQ routing thing.

It would only help users of 2.6.40 kernels.

> Actually, we only need
> routing for IRQs that shall be injected directly at kernel level. OTOH,
> this service would not be available on existing kernels, and we would
> not be able to simplify the PCI code that way (due to vhost
> requirements). So I dropped this idea as well and accepted that IRQ
> routing is the way to go.

I think that with the cache cleanup as outlined above it can work, no?

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

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

[Jan Kiszka]

On 2011-04-27 11:04, Avi Kivity wrote:
> On 04/27/2011 12:00 PM, Jan Kiszka wrote:
>> On 2011-04-27 09:27, Avi Kivity wrote:
>>>  On 04/26/2011 04:19 PM, Jan Kiszka wrote:
>>>>  I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
>>>>  that can wait until we go upstream.
>>>>
>>>>  This version still makes classic MSI usable in irqchip mode, now not
>>>>  only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
>>>>  Moreover, it contains an additional patch to refresh the MSI IRQ routes
>>>>  after vmload.
>>>>
>>>
>>>  Patches 1-8 applied, thanks.  I'm not sure about 9 (hpet kvm msi
>>>  integration) - it seems very intrusive to do this to every
>>>  msi-supporting device.  At least for pci we get all pci devices done in
>>>  one shot.
>>
>> Right, it is a but intrusive, but I do not see any real alternative.
>>
>>>
>>>  We could do this transparently in hw/apic.c.  When the message is sent
>>>  for the first time we look it up, fail, and update the kvm routing
>>>  entry.  Next time the lookup succeeds and we just use KVM_IRQ_LINE,
>>>  until the message changes and we need to update the irq entry again.
>>
>> I thought about this, also for PCI devices that aren't assigned or
>> vhost-driven, but we would quickly end up with unused and never freed
>> IRQ routing entries. We still need to track the vector configurations.
> 
> We can simply drop all route entries that are used exclusively in qemu 
> (i.e. not bound to an irqfd) and let the cache rebuild itself.

When should they be dropped?

> 
>> What would help at least in the HPET case is a new DELIVER_MSI syscall
>> that completely skips the IRQ routing thing.
> 
> It would only help users of 2.6.40 kernels.

Exactly.

> 
>> Actually, we only need
>> routing for IRQs that shall be injected directly at kernel level. OTOH,
>> this service would not be available on existing kernels, and we would
>> not be able to simplify the PCI code that way (due to vhost
>> requirements). So I dropped this idea as well and accepted that IRQ
>> routing is the way to go.
> 
> I think that with the cache cleanup as outlined above it can work, no?
> 

I don't see yet how.

Jan

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

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

[Avi Kivity]

On 04/27/2011 12:06 PM, Jan Kiszka wrote:
> >
> >  We can simply drop all route entries that are used exclusively in qemu
> >  (i.e. not bound to an irqfd) and let the cache rebuild itself.
>
> When should they be dropped?

Whenever we need to allocate a new routing entry, but cannot because it 
is full.

def kvm_send_msi_message(addr, val):
       gsi = route_cache.get((addr, val), None)
       if gsi is None:
           expire_volatile_route_cache_entries_if_full()
           gsi = alloc_gsi_cache_entry()
           route_cache[(addr, val)] = gsi
           update_route_cache()
        kvm_irq_line(gsi, 1)
        kvm_irq_line(gsi, 0)

The code would have to be in kvm.c, where it can track whether an entry 
is volatile or persistent.

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

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

[Jan Kiszka]

On 2011-04-27 11:14, Avi Kivity wrote:
> On 04/27/2011 12:06 PM, Jan Kiszka wrote:
>>>
>>>  We can simply drop all route entries that are used exclusively in qemu
>>>  (i.e. not bound to an irqfd) and let the cache rebuild itself.
>>
>> When should they be dropped?
> 
> Whenever we need to allocate a new routing entry, but cannot because it 
> is full.
> 
> def kvm_send_msi_message(addr, val):
>        gsi = route_cache.get((addr, val), None)
>        if gsi is None:
>            expire_volatile_route_cache_entries_if_full()
>            gsi = alloc_gsi_cache_entry()
>            route_cache[(addr, val)] = gsi
>            update_route_cache()
>         kvm_irq_line(gsi, 1)
>         kvm_irq_line(gsi, 0)
> 
> The code would have to be in kvm.c, where it can track whether an entry 
> is volatile or persistent.

Yeah, what I forgot is the other side of this caching: Looking up the
GSI from the MSI addr/data tuple. That's more complex than the current
O(1) way via device.msitable[vector]. Well, we could use some hash
table. But is it worth it? IMHO only if we could radically simplify the
PCI device hooking as well (HPET is negligible compared to that). But
I'm not yet sure if we can given what vhost needs.

Jan

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

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

[Avi Kivity]

On 04/27/2011 02:21 PM, Jan Kiszka wrote:
> On 2011-04-27 11:14, Avi Kivity wrote:
> >  On 04/27/2011 12:06 PM, Jan Kiszka wrote:
> >>>
> >>>   We can simply drop all route entries that are used exclusively in qemu
> >>>   (i.e. not bound to an irqfd) and let the cache rebuild itself.
> >>
> >>  When should they be dropped?
> >
> >  Whenever we need to allocate a new routing entry, but cannot because it
> >  is full.
> >
> >  def kvm_send_msi_message(addr, val):
> >         gsi = route_cache.get((addr, val), None)
> >         if gsi is None:
> >             expire_volatile_route_cache_entries_if_full()
> >             gsi = alloc_gsi_cache_entry()
> >             route_cache[(addr, val)] = gsi
> >             update_route_cache()
> >          kvm_irq_line(gsi, 1)
> >          kvm_irq_line(gsi, 0)
> >
> >  The code would have to be in kvm.c, where it can track whether an entry
> >  is volatile or persistent.
>
> Yeah, what I forgot is the other side of this caching: Looking up the
> GSI from the MSI addr/data tuple. That's more complex than the current
> O(1) way via device.msitable[vector]. Well, we could use some hash
> table. But is it worth it? IMHO only if we could radically simplify the
> PCI device hooking as well (HPET is negligible compared to that). But
> I'm not yet sure if we can given what vhost needs.

A hash table is indeed overcomplicated for this.

How about a replacement for stl_phys() for the MSI case:

-             stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+           msi_stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff, 
&timer->msi_cache);

msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)
{
     if (kvm_msi_enabled() && addr & MSI_ADDR_MASK == msi_base_addr) {
          if (cache->addr != addr || cache->data != data) {
              kvm_update_msi_cache(cache, addr, data);
          }
          kvm_irq_line(cache->gsi, 1);
          kvm_irq_line(cache->gsi, 0);
          return;
     }
     stl_phys(addr, data);
}

This bit of code would need to be updated for IOMMU and interrupt 
remapping, but at least it means that devices don't need significant 
change for kvm support.  We could also allocate a single gsi for use in 
hw/apic.c so hacks like using DMA to generate an MSI will work (will be 
slow, though).

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

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

[Jan Kiszka]

On 2011-04-27 14:12, Avi Kivity wrote:
> On 04/27/2011 02:21 PM, Jan Kiszka wrote:
>> On 2011-04-27 11:14, Avi Kivity wrote:
>>>  On 04/27/2011 12:06 PM, Jan Kiszka wrote:
>>>>>
>>>>>   We can simply drop all route entries that are used exclusively in qemu
>>>>>   (i.e. not bound to an irqfd) and let the cache rebuild itself.
>>>>
>>>>  When should they be dropped?
>>>
>>>  Whenever we need to allocate a new routing entry, but cannot because it
>>>  is full.
>>>
>>>  def kvm_send_msi_message(addr, val):
>>>         gsi = route_cache.get((addr, val), None)
>>>         if gsi is None:
>>>             expire_volatile_route_cache_entries_if_full()
>>>             gsi = alloc_gsi_cache_entry()
>>>             route_cache[(addr, val)] = gsi
>>>             update_route_cache()
>>>          kvm_irq_line(gsi, 1)
>>>          kvm_irq_line(gsi, 0)
>>>
>>>  The code would have to be in kvm.c, where it can track whether an entry
>>>  is volatile or persistent.
>>
>> Yeah, what I forgot is the other side of this caching: Looking up the
>> GSI from the MSI addr/data tuple. That's more complex than the current
>> O(1) way via device.msitable[vector]. Well, we could use some hash
>> table. But is it worth it? IMHO only if we could radically simplify the
>> PCI device hooking as well (HPET is negligible compared to that). But
>> I'm not yet sure if we can given what vhost needs.
> 
> A hash table is indeed overcomplicated for this.
> 
> How about a replacement for stl_phys() for the MSI case:
> 
> -             stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
> +           msi_stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff, 
> &timer->msi_cache);
> 
> msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)
> {
>      if (kvm_msi_enabled() && addr & MSI_ADDR_MASK == msi_base_addr) {
>           if (cache->addr != addr || cache->data != data) {
>               kvm_update_msi_cache(cache, addr, data);
>           }
>           kvm_irq_line(cache->gsi, 1);
>           kvm_irq_line(cache->gsi, 0);
>           return;
>      }
>      stl_phys(addr, data);
> }

I was planning for a MSI short-path anyway. Also for TCG, it's pointless
to go through lengthy stl_phys if we know it's supposed to be an MSI
message.

> 
> This bit of code would need to be updated for IOMMU and interrupt 
> remapping,

Quite a bit updates required for that anyway.

> but at least it means that devices don't need significant 
> change for kvm support.  We could also allocate a single gsi for use in 
> hw/apic.c so hacks like using DMA to generate an MSI will work (will be 
> slow, though).

Needs some thoughts, maybe it will work. Though, it's not yet clear to
me if we can drop the kvm hooks from msi/msix.c and still support
vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't
think it's worth the effort.

Jan

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

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

[Avi Kivity]

On 04/27/2011 04:31 PM, Jan Kiszka wrote:
> >  A hash table is indeed overcomplicated for this.
> >
> >  How about a replacement for stl_phys() for the MSI case:
> >
> >  -             stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff);
> >  +           msi_stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff,
> >  &timer->msi_cache);
> >
> >  msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)
> >  {
> >       if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
> >            if (cache->addr != addr || cache->data != data) {
> >                kvm_update_msi_cache(cache, addr, data);
> >            }
> >            kvm_irq_line(cache->gsi, 1);
> >            kvm_irq_line(cache->gsi, 0);
> >            return;
> >       }
> >       stl_phys(addr, data);
> >  }
>
> I was planning for a MSI short-path anyway. Also for TCG, it's pointless
> to go through lengthy stl_phys if we know it's supposed to be an MSI
> message.

I don't think tcg will see much benefit; the decoding path through 
hw/apic.c isn't complicated.

> >  but at least it means that devices don't need significant
> >  change for kvm support.  We could also allocate a single gsi for use in
> >  hw/apic.c so hacks like using DMA to generate an MSI will work (will be
> >  slow, though).
>
> Needs some thoughts, maybe it will work. Though, it's not yet clear to
> me if we can drop the kvm hooks from msi/msix.c and still support
> vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't
> think it's worth the effort.

Right.  Do we have other users of MSI besides PCI?

Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a 
neutral name) into msi.c and have hpet call it whenever things change.  
So hpet.c isn't aware of kvm directly.

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

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

[Jan Kiszka]

On 2011-04-27 15:39, Avi Kivity wrote:
> On 04/27/2011 04:31 PM, Jan Kiszka wrote:
>>>  A hash table is indeed overcomplicated for this.
>>>
>>>  How about a replacement for stl_phys() for the MSI case:
>>>
>>>  -             stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff);
>>>  +           msi_stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff,
>>>  &timer->msi_cache);
>>>
>>>  msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)
>>>  {
>>>       if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
>>>            if (cache->addr != addr || cache->data != data) {
>>>                kvm_update_msi_cache(cache, addr, data);
>>>            }
>>>            kvm_irq_line(cache->gsi, 1);
>>>            kvm_irq_line(cache->gsi, 0);
>>>            return;
>>>       }
>>>       stl_phys(addr, data);
>>>  }
>>
>> I was planning for a MSI short-path anyway. Also for TCG, it's pointless
>> to go through lengthy stl_phys if we know it's supposed to be an MSI
>> message.
> 
> I don't think tcg will see much benefit; the decoding path through 
> hw/apic.c isn't complicated.

stl_phys itself is non-trivial, e.g. due to phys_page_find.

> 
>>>  but at least it means that devices don't need significant
>>>  change for kvm support.  We could also allocate a single gsi for use in
>>>  hw/apic.c so hacks like using DMA to generate an MSI will work (will be
>>>  slow, though).
>>
>> Needs some thoughts, maybe it will work. Though, it's not yet clear to
>> me if we can drop the kvm hooks from msi/msix.c and still support
>> vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't
>> think it's worth the effort.
> 
> Right.  Do we have other users of MSI besides PCI?

And HPET? Not yet. IOMMU will become another user. Maybe future chipsets
will include more non-PCI devices that issue MSIs.

> 
> Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a 
> neutral name) into msi.c and have hpet call it whenever things change.  
> So hpet.c isn't aware of kvm directly.

Without caching, you need per-vector tracking to refresh or drop routes.
That's what the hooks are about.

Intermediate solutions (like hacking msi.c now) are one thing. We also
have to know where we can go to long-term.

Jan

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

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

[Avi Kivity]

On 04/27/2011 04:54 PM, Jan Kiszka wrote:
> >>
> >>  I was planning for a MSI short-path anyway. Also for TCG, it's pointless
> >>  to go through lengthy stl_phys if we know it's supposed to be an MSI
> >>  message.
> >
> >  I don't think tcg will see much benefit; the decoding path through
> >  hw/apic.c isn't complicated.
>
> stl_phys itself is non-trivial, e.g. due to phys_page_find.

That is true.  But consider that MSI delivery has to emulate interrupt 
injection through the IDT...

> >
> >  Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a
> >  neutral name) into msi.c and have hpet call it whenever things change.
> >  So hpet.c isn't aware of kvm directly.
>
> Without caching, you need per-vector tracking to refresh or drop routes.
> That's what the hooks are about.

Right, my proposal doesn't really change your code, it simply moves the 
kvm specific parts away from hpet.c.  Instead of a general cache, the 
caller is supposed to provide a unique KVMMsiMessage per msi vector, so 
there is no cache management.

> Intermediate solutions (like hacking msi.c now) are one thing. We also
> have to know where we can go to long-term.

The really general case (allow generating an MSI via DMA, or allow the 
device to write to non-MSI addresses via MSI generation) needs the full 
blown cache.  But while I've seen these techniques actually used, I hope 
they're rare.

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

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

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 05:01:09PM +0300, Avi Kivity wrote:
> On 04/27/2011 04:54 PM, Jan Kiszka wrote:
> >>>
> >>>  I was planning for a MSI short-path anyway. Also for TCG, it's pointless
> >>>  to go through lengthy stl_phys if we know it's supposed to be an MSI
> >>>  message.
> >>
> >>  I don't think tcg will see much benefit; the decoding path through
> >>  hw/apic.c isn't complicated.
> >
> >stl_phys itself is non-trivial, e.g. due to phys_page_find.
> 
> That is true.  But consider that MSI delivery has to emulate
> interrupt injection through the IDT...
> 
> >>
> >>  Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a
> >>  neutral name) into msi.c and have hpet call it whenever things change.
> >>  So hpet.c isn't aware of kvm directly.
> >
> >Without caching, you need per-vector tracking to refresh or drop routes.
> >That's what the hooks are about.
> 
> Right, my proposal doesn't really change your code, it simply moves
> the kvm specific parts away from hpet.c.  Instead of a general
> cache, the caller is supposed to provide a unique KVMMsiMessage per
> msi vector, so there is no cache management.
> 
> >Intermediate solutions (like hacking msi.c now) are one thing. We also
> >have to know where we can go to long-term.
> 
> The really general case (allow generating an MSI via DMA, or allow
> the device to write to non-MSI addresses via MSI generation) needs
> the full blown cache.  But while I've seen these techniques actually
> used, I hope they're rare.

You have? What uses these?

As you pointed out earlier, we can make DMA to MSI work, slowly, by
using a single gsi per apic.
For the reverse, it's probably easiest to make it work by
handling MSI outside the APIC range as a store in the kvm
module in kernel.

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

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

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 03:54:36PM +0200, Jan Kiszka wrote:
> On 2011-04-27 15:39, Avi Kivity wrote:
> > On 04/27/2011 04:31 PM, Jan Kiszka wrote:
> >>>  A hash table is indeed overcomplicated for this.
> >>>
> >>>  How about a replacement for stl_phys() for the MSI case:
> >>>
> >>>  -             stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff);
> >>>  +           msi_stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff,
> >>>  &timer->msi_cache);
> >>>
> >>>  msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)
> >>>  {
> >>>       if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
> >>>            if (cache->addr != addr || cache->data != data) {
> >>>                kvm_update_msi_cache(cache, addr, data);
> >>>            }
> >>>            kvm_irq_line(cache->gsi, 1);
> >>>            kvm_irq_line(cache->gsi, 0);
> >>>            return;
> >>>       }
> >>>       stl_phys(addr, data);
> >>>  }
> >>
> >> I was planning for a MSI short-path anyway. Also for TCG, it's pointless
> >> to go through lengthy stl_phys if we know it's supposed to be an MSI
> >> message.
> > 
> > I don't think tcg will see much benefit; the decoding path through 
> > hw/apic.c isn't complicated.
> 
> stl_phys itself is non-trivial, e.g. due to phys_page_find.
> 
> > 
> >>>  but at least it means that devices don't need significant
> >>>  change for kvm support.  We could also allocate a single gsi for use in
> >>>  hw/apic.c so hacks like using DMA to generate an MSI will work (will be
> >>>  slow, though).
> >>
> >> Needs some thoughts, maybe it will work. Though, it's not yet clear to
> >> me if we can drop the kvm hooks from msi/msix.c and still support
> >> vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't
> >> think it's worth the effort.
> > 
> > Right.  Do we have other users of MSI besides PCI?
> 
> And HPET? Not yet. IOMMU will become another user. Maybe future chipsets
> will include more non-PCI devices that issue MSIs.

In theory any device can issue writes into the memory range that
apic maps to MSI and I don't think apic can tell what caused this.

Another issue is the reverse: regular memory address can be put
in the MSIX/MSI field and the result should be a regular memory
write.

> > 
> > Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a 
> > neutral name) into msi.c and have hpet call it whenever things change.  
> > So hpet.c isn't aware of kvm directly.
> 
> Without caching, you need per-vector tracking to refresh or drop routes.
> That's what the hooks are about.
> 
> Intermediate solutions (like hacking msi.c now) are one thing. We also
> have to know where we can go to long-term.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

On 2011-04-27 16:02, Michael S. Tsirkin wrote:
> On Wed, Apr 27, 2011 at 03:54:36PM +0200, Jan Kiszka wrote:
>> On 2011-04-27 15:39, Avi Kivity wrote:
>>> On 04/27/2011 04:31 PM, Jan Kiszka wrote:
>>>>>  A hash table is indeed overcomplicated for this.
>>>>>
>>>>>  How about a replacement for stl_phys() for the MSI case:
>>>>>
>>>>>  -             stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff);
>>>>>  +           msi_stl_phys(timer->fsb>>  32, timer->fsb&  0xffffffff,
>>>>>  &timer->msi_cache);
>>>>>
>>>>>  msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)
>>>>>  {
>>>>>       if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
>>>>>            if (cache->addr != addr || cache->data != data) {
>>>>>                kvm_update_msi_cache(cache, addr, data);
>>>>>            }
>>>>>            kvm_irq_line(cache->gsi, 1);
>>>>>            kvm_irq_line(cache->gsi, 0);
>>>>>            return;
>>>>>       }
>>>>>       stl_phys(addr, data);
>>>>>  }
>>>>
>>>> I was planning for a MSI short-path anyway. Also for TCG, it's pointless
>>>> to go through lengthy stl_phys if we know it's supposed to be an MSI
>>>> message.
>>>
>>> I don't think tcg will see much benefit; the decoding path through 
>>> hw/apic.c isn't complicated.
>>
>> stl_phys itself is non-trivial, e.g. due to phys_page_find.
>>
>>>
>>>>>  but at least it means that devices don't need significant
>>>>>  change for kvm support.  We could also allocate a single gsi for use in
>>>>>  hw/apic.c so hacks like using DMA to generate an MSI will work (will be
>>>>>  slow, though).
>>>>
>>>> Needs some thoughts, maybe it will work. Though, it's not yet clear to
>>>> me if we can drop the kvm hooks from msi/msix.c and still support
>>>> vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't
>>>> think it's worth the effort.
>>>
>>> Right.  Do we have other users of MSI besides PCI?
>>
>> And HPET? Not yet. IOMMU will become another user. Maybe future chipsets
>> will include more non-PCI devices that issue MSIs.
> 
> In theory any device can issue writes into the memory range that
> apic maps to MSI and I don't think apic can tell what caused this.

Not the APIC, but the PCI bridge and specifically the IOMMU can and do.
But that does not help us here.

> 
> Another issue is the reverse: regular memory address can be put
> in the MSIX/MSI field and the result should be a regular memory
> write.

Yes, that's a separate issue: Requests issued by the CPUs have to be
told apart from those issued by devices. I'm trying to address this for
a while (to clean up the APIC page mappings), but it's hairy and
incomplete yet.

Jan

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

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

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 04:10:04PM +0200, Jan Kiszka wrote:
> > Another issue is the reverse: regular memory address can be put
> > in the MSIX/MSI field and the result should be a regular memory
> > write.
> 
> Yes, that's a separate issue: Requests issued by the CPUs have to be
> told apart from those issued by devices.

Seems to be the reverse: in qemu msi writes are implemented as regular
writes so you can stick a non msi address there and it works.
in kvm they are handled differently and that's broken ...

-- 
MST

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

[Jan Kiszka]

On 2011-04-27 16:14, Michael S. Tsirkin wrote:
> On Wed, Apr 27, 2011 at 04:10:04PM +0200, Jan Kiszka wrote:
>>> Another issue is the reverse: regular memory address can be put
>>> in the MSIX/MSI field and the result should be a regular memory
>>> write.
>>
>> Yes, that's a separate issue: Requests issued by the CPUs have to be
>> told apart from those issued by devices.
> 
> Seems to be the reverse: in qemu msi writes are implemented as regular
> writes so you can stick a non msi address there and it works.
> in kvm they are handled differently and that's broken ...

Right, that's _also_ an issue.

Jan

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

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

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 03:31:16PM +0200, Jan Kiszka wrote:
> On 2011-04-27 14:12, Avi Kivity wrote:
> > On 04/27/2011 02:21 PM, Jan Kiszka wrote:
> >> On 2011-04-27 11:14, Avi Kivity wrote:
> >>>  On 04/27/2011 12:06 PM, Jan Kiszka wrote:
> >>>>>
> >>>>>   We can simply drop all route entries that are used exclusively in qemu
> >>>>>   (i.e. not bound to an irqfd) and let the cache rebuild itself.
> >>>>
> >>>>  When should they be dropped?
> >>>
> >>>  Whenever we need to allocate a new routing entry, but cannot because it
> >>>  is full.
> >>>
> >>>  def kvm_send_msi_message(addr, val):
> >>>         gsi = route_cache.get((addr, val), None)
> >>>         if gsi is None:
> >>>             expire_volatile_route_cache_entries_if_full()
> >>>             gsi = alloc_gsi_cache_entry()
> >>>             route_cache[(addr, val)] = gsi
> >>>             update_route_cache()
> >>>          kvm_irq_line(gsi, 1)
> >>>          kvm_irq_line(gsi, 0)
> >>>
> >>>  The code would have to be in kvm.c, where it can track whether an entry
> >>>  is volatile or persistent.
> >>
> >> Yeah, what I forgot is the other side of this caching: Looking up the
> >> GSI from the MSI addr/data tuple. That's more complex than the current
> >> O(1) way via device.msitable[vector]. Well, we could use some hash
> >> table. But is it worth it? IMHO only if we could radically simplify the
> >> PCI device hooking as well (HPET is negligible compared to that). But
> >> I'm not yet sure if we can given what vhost needs.
> > 
> > A hash table is indeed overcomplicated for this.
> > 
> > How about a replacement for stl_phys() for the MSI case:
> > 
> > -             stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
> > +           msi_stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff, 
> > &timer->msi_cache);
> > 
> > msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)

Let's try to use uint64_t for addresses. This is what msi deals with
anyway.

> > {
> >      if (kvm_msi_enabled() && addr & MSI_ADDR_MASK == msi_base_addr) {
> >           if (cache->addr != addr || cache->data != data) {
> >               kvm_update_msi_cache(cache, addr, data);
> >           }
> >           kvm_irq_line(cache->gsi, 1);
> >           kvm_irq_line(cache->gsi, 0);

I think this second ioctl isn't needed on latest kernels.
Was it needed for older ones? Is there a way to detect such?

> >           return;
> >      }
> >      stl_phys(addr, data);
> > }
> 
> I was planning for a MSI short-path anyway. Also for TCG, it's pointless
> to go through lengthy stl_phys if we know it's supposed to be an MSI
> message.

Well, in theory we don't. All MSI does is a store, it could
just go into memory.  For PV (virtio) which is what I coded up msix
originally for, the assumption that it will go into the apic is fine I
think, but I'm a bit less sure with emulated devices.

> > 
> > This bit of code would need to be updated for IOMMU and interrupt 
> > remapping,
> 
> Quite a bit updates required for that anyway.

Yes, I don't think we need to worry about that just yet.

> > but at least it means that devices don't need significant 
> > change for kvm support.  We could also allocate a single gsi for use in 
> > hw/apic.c so hacks like using DMA to generate an MSI will work (will be 
> > slow, though).
> 
> Needs some thoughts, maybe it will work. Though, it's not yet clear to
> me if we can drop the kvm hooks from msi/msix.c and still support
> vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't
> think it's worth the effort.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

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

[Avi Kivity]

On 04/26/2011 04:19 PM, Jan Kiszka wrote:
> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
> that can wait until we go upstream.
>
> This version still makes classic MSI usable in irqchip mode, now not
> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
> Moreover, it contains an additional patch to refresh the MSI IRQ routes
> after vmload.
>

Immediately after migration:

Program terminated with signal 11, Segmentation fault.
#0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
../bswap.h:178
178        return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
Missing separate debuginfos, use: debuginfo-install 
SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 
cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 
glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 
keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 
libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 
libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 
libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 
libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 
libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 
libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 
libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 
ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 
nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 
nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 
openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
(gdb) bt
#0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
../bswap.h:178
#1  pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at 
/build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
#2  kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized 
out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
#3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
/build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
#4  0x000000000041e29b in get_pci_config_device (f=0x2466380, 
pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346
#5  0x000000000049c36c in vmstate_load_state (f=0x2466380, 
vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374
#6  0x000000000049c323 in vmstate_load_state (f=0x2466380, 
vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372
#7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
#8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
#9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 
out>) at migration.c:66
#10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value 
optimized out>) at migration-tcp.c:163
#11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, 
writefds=0x7fff56dc03b0, xfds=<value optimized out>,
     ret=<value optimized out>) at iohandler.c:120
#12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized 
out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
#13 0x0000000000433a97 in kvm_main_loop () at 
/build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
#14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, 
argv=<value optimized out>, envp=<value optimized out>)
     at /build/home/tlv/akivity/qemu-kvm/vl.c:1369
#15 main (argc=<value optimized out>, argv=<value optimized out>, 
envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257

(gdb) fr
#3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
/build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
(gdb) p dev.msi_irq_entries
$10 = (struct KVMMsiMessage *) 0x0

dev points to the i440fx chipset device.

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

[PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Jan Kiszka]

On 2011-04-27 11:34, Avi Kivity wrote:
> On 04/26/2011 04:19 PM, Jan Kiszka wrote:
>> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
>> that can wait until we go upstream.
>>
>> This version still makes classic MSI usable in irqchip mode, now not
>> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
>> Moreover, it contains an additional patch to refresh the MSI IRQ routes
>> after vmload.
>>
> 
> Immediately after migration:
> 
> Program terminated with signal 11, Segmentation fault.
> #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
> ../bswap.h:178
> 178        return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
> Missing separate debuginfos, use: debuginfo-install 
> SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 
> cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 
> glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 
> keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 
> libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 
> libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 
> libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 
> libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 
> libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 
> libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 
> libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 
> ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 
> nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 
> nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 
> openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
> (gdb) bt
> #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
> ../bswap.h:178
> #1  pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at 
> /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
> #2  kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized 
> out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
> #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
> #4  0x000000000041e29b in get_pci_config_device (f=0x2466380, 
> pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346
> #5  0x000000000049c36c in vmstate_load_state (f=0x2466380, 
> vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374
> #6  0x000000000049c323 in vmstate_load_state (f=0x2466380, 
> vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372
> #7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
> #8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
> #9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 
> out>) at migration.c:66
> #10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value 
> optimized out>) at migration-tcp.c:163
> #11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, 
> writefds=0x7fff56dc03b0, xfds=<value optimized out>,
>      ret=<value optimized out>) at iohandler.c:120
> #12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized 
> out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
> #13 0x0000000000433a97 in kvm_main_loop () at 
> /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
> #14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, 
> argv=<value optimized out>, envp=<value optimized out>)
>      at /build/home/tlv/akivity/qemu-kvm/vl.c:1369
> #15 main (argc=<value optimized out>, argv=<value optimized out>, 
> envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257
> 
> (gdb) fr
> #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
> (gdb) p dev.msi_irq_entries
> $10 = (struct KVMMsiMessage *) 0x0
> 
> dev points to the i440fx chipset device.
> 

Yeah, better use this version of patch 8.

Jan

----8<-----

Establish a post-load notification for the MSI subsystem so that KVM can
refresh its IRQ routing after vmload.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

v3: Fix null-pointer deref after vmload by checking for availability of
    msi routing entries.

 hw/msi.c |   13 +++++++++++++
 hw/msi.h |    1 +
 hw/pci.c |    2 ++
 3 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index 18f683b..725b2b7 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
     return msi_nr_vectors(flags);
 }
+
+void msi_post_load(PCIDevice *dev)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+
+    if (kvm_enabled() && dev->msi_irq_entries) {
+        kvm_msi_free(dev);
+
+        if (flags & PCI_MSI_FLAGS_ENABLE) {
+            kvm_msi_update(dev);
+        }
+    }
+}
diff --git a/hw/msi.h b/hw/msi.h
index 5766018..6ff0607 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev);
 void msi_notify(PCIDevice *dev, unsigned int vector);
 void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
 unsigned int msi_nr_vectors_allocated(const PCIDevice *dev);
+void msi_post_load(PCIDevice *dev);
 
 static inline bool msi_present(const PCIDevice *dev)
 {
diff --git a/hw/pci.c b/hw/pci.c
index 82e0300..07ec4f9 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 
     pci_update_mappings(s);
+    msi_post_load(s);
 
     qemu_free(config);
     return 0;
-- 
1.7.1

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Avi Kivity]

On 04/27/2011 02:39 PM, Jan Kiszka wrote:
> Yeah, better use this version of patch 8.
>

Replaced, going through the mill again.

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 01:39:05PM +0200, Jan Kiszka wrote:
> On 2011-04-27 11:34, Avi Kivity wrote:
> > On 04/26/2011 04:19 PM, Jan Kiszka wrote:
> >> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
> >> that can wait until we go upstream.
> >>
> >> This version still makes classic MSI usable in irqchip mode, now not
> >> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
> >> Moreover, it contains an additional patch to refresh the MSI IRQ routes
> >> after vmload.
> >>
> > 
> > Immediately after migration:
> > 
> > Program terminated with signal 11, Segmentation fault.
> > #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
> > ../bswap.h:178
> > 178        return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
> > Missing separate debuginfos, use: debuginfo-install 
> > SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 
> > cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 
> > glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 
> > keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 
> > libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 
> > libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 
> > libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 
> > libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 
> > libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 
> > libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 
> > libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 
> > ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 
> > nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 
> > nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 
> > openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
> > (gdb) bt
> > #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
> > ../bswap.h:178
> > #1  pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at 
> > /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
> > #2  kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized 
> > out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
> > #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
> > /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
> > #4  0x000000000041e29b in get_pci_config_device (f=0x2466380, 
> > pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346
> > #5  0x000000000049c36c in vmstate_load_state (f=0x2466380, 
> > vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374
> > #6  0x000000000049c323 in vmstate_load_state (f=0x2466380, 
> > vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372
> > #7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
> > #8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
> > #9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 
> > out>) at migration.c:66
> > #10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value 
> > optimized out>) at migration-tcp.c:163
> > #11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, 
> > writefds=0x7fff56dc03b0, xfds=<value optimized out>,
> >      ret=<value optimized out>) at iohandler.c:120
> > #12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized 
> > out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
> > #13 0x0000000000433a97 in kvm_main_loop () at 
> > /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
> > #14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, 
> > argv=<value optimized out>, envp=<value optimized out>)
> >      at /build/home/tlv/akivity/qemu-kvm/vl.c:1369
> > #15 main (argc=<value optimized out>, argv=<value optimized out>, 
> > envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257
> > 
> > (gdb) fr
> > #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
> > /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
> > (gdb) p dev.msi_irq_entries
> > $10 = (struct KVMMsiMessage *) 0x0
> > 
> > dev points to the i440fx chipset device.
> > 
> 
> Yeah, better use this version of patch 8.
> 
> Jan
> 
> ----8<-----
> 
> Establish a post-load notification for the MSI subsystem so that KVM can
> refresh its IRQ routing after vmload.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> v3: Fix null-pointer deref after vmload by checking for availability of
>     msi routing entries.
> 
>  hw/msi.c |   13 +++++++++++++
>  hw/msi.h |    1 +
>  hw/pci.c |    2 ++
>  3 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 18f683b..725b2b7 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>      return msi_nr_vectors(flags);
>  }
> +
> +void msi_post_load(PCIDevice *dev)
> +{
> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> +
> +    if (kvm_enabled() && dev->msi_irq_entries) {
> +        kvm_msi_free(dev);
> +
> +        if (flags & PCI_MSI_FLAGS_ENABLE) {
> +            kvm_msi_update(dev);
> +        }
> +    }
> +}
> diff --git a/hw/msi.h b/hw/msi.h
> index 5766018..6ff0607 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev);
>  void msi_notify(PCIDevice *dev, unsigned int vector);
>  void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
>  unsigned int msi_nr_vectors_allocated(const PCIDevice *dev);
> +void msi_post_load(PCIDevice *dev);
>  
>  static inline bool msi_present(const PCIDevice *dev)
>  {
> diff --git a/hw/pci.c b/hw/pci.c
> index 82e0300..07ec4f9 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -34,6 +34,7 @@
>  #include "device-assignment.h"
>  #include "qemu-objects.h"
>  #include "range.h"
> +#include "msi.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
>      memcpy(s->config, config, size);
>  
>      pci_update_mappings(s);
> +    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in
pci.c and all capabilities in separate files.
msix has msix_load, msi will just need one too,
and let all devices call that.

>  
>      qemu_free(config);
>      return 0;
> -- 
> 1.7.1

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Jan Kiszka]

On 2011-04-27 16:16, Michael S. Tsirkin wrote:
> On Wed, Apr 27, 2011 at 01:39:05PM +0200, Jan Kiszka wrote:
>> On 2011-04-27 11:34, Avi Kivity wrote:
>>> On 04/26/2011 04:19 PM, Jan Kiszka wrote:
>>>> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
>>>> that can wait until we go upstream.
>>>>
>>>> This version still makes classic MSI usable in irqchip mode, now not
>>>> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
>>>> Moreover, it contains an additional patch to refresh the MSI IRQ routes
>>>> after vmload.
>>>>
>>>
>>> Immediately after migration:
>>>
>>> Program terminated with signal 11, Segmentation fault.
>>> #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
>>> ../bswap.h:178
>>> 178        return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
>>> Missing separate debuginfos, use: debuginfo-install 
>>> SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 
>>> cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 
>>> glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 
>>> keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 
>>> libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 
>>> libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 
>>> libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 
>>> libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 
>>> libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 
>>> libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 
>>> libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 
>>> ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 
>>> nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 
>>> nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 
>>> openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
>>> (gdb) bt
>>> #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
>>> ../bswap.h:178
>>> #1  pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at 
>>> /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
>>> #2  kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized 
>>> out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
>>> #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
>>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
>>> #4  0x000000000041e29b in get_pci_config_device (f=0x2466380, 
>>> pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346
>>> #5  0x000000000049c36c in vmstate_load_state (f=0x2466380, 
>>> vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374
>>> #6  0x000000000049c323 in vmstate_load_state (f=0x2466380, 
>>> vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372
>>> #7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
>>> #8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
>>> #9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 
>>> out>) at migration.c:66
>>> #10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value 
>>> optimized out>) at migration-tcp.c:163
>>> #11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, 
>>> writefds=0x7fff56dc03b0, xfds=<value optimized out>,
>>>      ret=<value optimized out>) at iohandler.c:120
>>> #12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized 
>>> out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
>>> #13 0x0000000000433a97 in kvm_main_loop () at 
>>> /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
>>> #14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, 
>>> argv=<value optimized out>, envp=<value optimized out>)
>>>      at /build/home/tlv/akivity/qemu-kvm/vl.c:1369
>>> #15 main (argc=<value optimized out>, argv=<value optimized out>, 
>>> envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257
>>>
>>> (gdb) fr
>>> #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
>>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
>>> (gdb) p dev.msi_irq_entries
>>> $10 = (struct KVMMsiMessage *) 0x0
>>>
>>> dev points to the i440fx chipset device.
>>>
>>
>> Yeah, better use this version of patch 8.
>>
>> Jan
>>
>> ----8<-----
>>
>> Establish a post-load notification for the MSI subsystem so that KVM can
>> refresh its IRQ routing after vmload.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> v3: Fix null-pointer deref after vmload by checking for availability of
>>     msi routing entries.
>>
>>  hw/msi.c |   13 +++++++++++++
>>  hw/msi.h |    1 +
>>  hw/pci.c |    2 ++
>>  3 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 18f683b..725b2b7 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>      return msi_nr_vectors(flags);
>>  }
>> +
>> +void msi_post_load(PCIDevice *dev)
>> +{
>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>> +
>> +    if (kvm_enabled() && dev->msi_irq_entries) {
>> +        kvm_msi_free(dev);
>> +
>> +        if (flags & PCI_MSI_FLAGS_ENABLE) {
>> +            kvm_msi_update(dev);
>> +        }
>> +    }
>> +}
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 5766018..6ff0607 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev);
>>  void msi_notify(PCIDevice *dev, unsigned int vector);
>>  void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
>>  unsigned int msi_nr_vectors_allocated(const PCIDevice *dev);
>> +void msi_post_load(PCIDevice *dev);
>>  
>>  static inline bool msi_present(const PCIDevice *dev)
>>  {
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 82e0300..07ec4f9 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -34,6 +34,7 @@
>>  #include "device-assignment.h"
>>  #include "qemu-objects.h"
>>  #include "range.h"
>> +#include "msi.h"
>>  
>>  //#define DEBUG_PCI
>>  #ifdef DEBUG_PCI
>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
>>      memcpy(s->config, config, size);
>>  
>>      pci_update_mappings(s);
>> +    msi_post_load(s);
> 
> Pls don't do this: I'm trying to keep just the core in
> pci.c and all capabilities in separate files.
> msix has msix_load, msi will just need one too,
> and let all devices call that.
> 

Preferred alternatives are...? Registering a vmstate for msi?

Jan

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 04:28:56PM +0200, Jan Kiszka wrote:
> On 2011-04-27 16:16, Michael S. Tsirkin wrote:
> > On Wed, Apr 27, 2011 at 01:39:05PM +0200, Jan Kiszka wrote:
> >> On 2011-04-27 11:34, Avi Kivity wrote:
> >>> On 04/26/2011 04:19 PM, Jan Kiszka wrote:
> >>>> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but
> >>>> that can wait until we go upstream.
> >>>>
> >>>> This version still makes classic MSI usable in irqchip mode, now not
> >>>> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on).
> >>>> Moreover, it contains an additional patch to refresh the MSI IRQ routes
> >>>> after vmload.
> >>>>
> >>>
> >>> Immediately after migration:
> >>>
> >>> Program terminated with signal 11, Segmentation fault.
> >>> #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
> >>> ../bswap.h:178
> >>> 178        return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
> >>> Missing separate debuginfos, use: debuginfo-install 
> >>> SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 
> >>> cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 
> >>> glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 
> >>> keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 
> >>> libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 
> >>> libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 
> >>> libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 
> >>> libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 
> >>> libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 
> >>> libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 
> >>> libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 
> >>> ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 
> >>> nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 
> >>> nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 
> >>> openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
> >>> (gdb) bt
> >>> #0  le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at 
> >>> ../bswap.h:178
> >>> #1  pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at 
> >>> /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
> >>> #2  kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized 
> >>> out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
> >>> #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
> >>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
> >>> #4  0x000000000041e29b in get_pci_config_device (f=0x2466380, 
> >>> pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346
> >>> #5  0x000000000049c36c in vmstate_load_state (f=0x2466380, 
> >>> vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374
> >>> #6  0x000000000049c323 in vmstate_load_state (f=0x2466380, 
> >>> vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372
> >>> #7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
> >>> #8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
> >>> #9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 
> >>> out>) at migration.c:66
> >>> #10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value 
> >>> optimized out>) at migration-tcp.c:163
> >>> #11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, 
> >>> writefds=0x7fff56dc03b0, xfds=<value optimized out>,
> >>>      ret=<value optimized out>) at iohandler.c:120
> >>> #12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized 
> >>> out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
> >>> #13 0x0000000000433a97 in kvm_main_loop () at 
> >>> /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
> >>> #14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, 
> >>> argv=<value optimized out>, envp=<value optimized out>)
> >>>      at /build/home/tlv/akivity/qemu-kvm/vl.c:1369
> >>> #15 main (argc=<value optimized out>, argv=<value optimized out>, 
> >>> envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257
> >>>
> >>> (gdb) fr
> >>> #3  0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at 
> >>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
> >>> (gdb) p dev.msi_irq_entries
> >>> $10 = (struct KVMMsiMessage *) 0x0
> >>>
> >>> dev points to the i440fx chipset device.
> >>>
> >>
> >> Yeah, better use this version of patch 8.
> >>
> >> Jan
> >>
> >> ----8<-----
> >>
> >> Establish a post-load notification for the MSI subsystem so that KVM can
> >> refresh its IRQ routing after vmload.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>
> >> v3: Fix null-pointer deref after vmload by checking for availability of
> >>     msi routing entries.
> >>
> >>  hw/msi.c |   13 +++++++++++++
> >>  hw/msi.h |    1 +
> >>  hw/pci.c |    2 ++
> >>  3 files changed, 16 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hw/msi.c b/hw/msi.c
> >> index 18f683b..725b2b7 100644
> >> --- a/hw/msi.c
> >> +++ b/hw/msi.c
> >> @@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
> >>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> >>      return msi_nr_vectors(flags);
> >>  }
> >> +
> >> +void msi_post_load(PCIDevice *dev)
> >> +{
> >> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> >> +
> >> +    if (kvm_enabled() && dev->msi_irq_entries) {
> >> +        kvm_msi_free(dev);
> >> +
> >> +        if (flags & PCI_MSI_FLAGS_ENABLE) {
> >> +            kvm_msi_update(dev);
> >> +        }
> >> +    }
> >> +}
> >> diff --git a/hw/msi.h b/hw/msi.h
> >> index 5766018..6ff0607 100644
> >> --- a/hw/msi.h
> >> +++ b/hw/msi.h
> >> @@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev);
> >>  void msi_notify(PCIDevice *dev, unsigned int vector);
> >>  void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
> >>  unsigned int msi_nr_vectors_allocated(const PCIDevice *dev);
> >> +void msi_post_load(PCIDevice *dev);
> >>  
> >>  static inline bool msi_present(const PCIDevice *dev)
> >>  {
> >> diff --git a/hw/pci.c b/hw/pci.c
> >> index 82e0300..07ec4f9 100644
> >> --- a/hw/pci.c
> >> +++ b/hw/pci.c
> >> @@ -34,6 +34,7 @@
> >>  #include "device-assignment.h"
> >>  #include "qemu-objects.h"
> >>  #include "range.h"
> >> +#include "msi.h"
> >>  
> >>  //#define DEBUG_PCI
> >>  #ifdef DEBUG_PCI
> >> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
> >>      memcpy(s->config, config, size);
> >>  
> >>      pci_update_mappings(s);
> >> +    msi_post_load(s);
> > 
> > Pls don't do this: I'm trying to keep just the core in
> > pci.c and all capabilities in separate files.
> > msix has msix_load, msi will just need one too,
> > and let all devices call that.
> > 
> 
> Preferred alternatives are...? Registering a vmstate for msi?
> 
> Jan

Add msi_load and call that from devices that need it.
Like msix_load does now.

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Jan Kiszka]

On 2011-04-27 16:30, Michael S. Tsirkin wrote:
>>>> --- a/hw/pci.c
>>>> +++ b/hw/pci.c
>>>> @@ -34,6 +34,7 @@
>>>>  #include "device-assignment.h"
>>>>  #include "qemu-objects.h"
>>>>  #include "range.h"
>>>> +#include "msi.h"
>>>>  
>>>>  //#define DEBUG_PCI
>>>>  #ifdef DEBUG_PCI
>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
>>>>      memcpy(s->config, config, size);
>>>>  
>>>>      pci_update_mappings(s);
>>>> +    msi_post_load(s);
>>>
>>> Pls don't do this: I'm trying to keep just the core in
>>> pci.c and all capabilities in separate files.
>>> msix has msix_load, msi will just need one too,
>>> and let all devices call that.
>>>
>>
>> Preferred alternatives are...? Registering a vmstate for msi?
>>
>> Jan
> 
> Add msi_load and call that from devices that need it.
> Like msix_load does now.
> 

msix_load/save are refactoring candidates IMHO. MSI-X has a real need
for storing additional state information, so it should register its own
subsection. I don't want to offload this burden to the devices also for
MSI. From the devices' POV, why shouldn't msi_init suffice?

Jan

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote:
> On 2011-04-27 16:30, Michael S. Tsirkin wrote:
> >>>> --- a/hw/pci.c
> >>>> +++ b/hw/pci.c
> >>>> @@ -34,6 +34,7 @@
> >>>>  #include "device-assignment.h"
> >>>>  #include "qemu-objects.h"
> >>>>  #include "range.h"
> >>>> +#include "msi.h"
> >>>>  
> >>>>  //#define DEBUG_PCI
> >>>>  #ifdef DEBUG_PCI
> >>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
> >>>>      memcpy(s->config, config, size);
> >>>>  
> >>>>      pci_update_mappings(s);
> >>>> +    msi_post_load(s);
> >>>
> >>> Pls don't do this: I'm trying to keep just the core in
> >>> pci.c and all capabilities in separate files.
> >>> msix has msix_load, msi will just need one too,
> >>> and let all devices call that.
> >>>
> >>
> >> Preferred alternatives are...? Registering a vmstate for msi?
> >>
> >> Jan
> > 
> > Add msi_load and call that from devices that need it.
> > Like msix_load does now.
> > 
> 
> msix_load/save are refactoring candidates IMHO. MSI-X has a real need
> for storing additional state information, so it should register its own
> subsection.

That's an implementation detail though, isn't it.

> I don't want to offload this burden to the devices also for
> MSI.
> From the devices' POV, why shouldn't msi_init suffice?
> 
> Jan

One can also claim this about config writes:
    pci_bridge_write_config(d, address, val, len);
    pcie_cap_flr_write_config(d, address, val, len);
    pcie_cap_slot_write_config(d, address, val, len);
    msi_write_config(d, address, val, len);
    pcie_aer_write_config(d, address, val, len);
which arguably just duplicates the initialization sequence.

What I'm trying to do though is to keep it modular and
keep module inter-dependencies to a minimum,
so that pci is the core and msix depends on it
but not the other way around.

What I think we should do is to add a pci subdirectory, move all
of the stuff there, move pci.c to pci/core.c and
add a high level module that depends on them all
and deals with all the capabilities.

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Jan Kiszka]

On 2011-04-27 17:09, Michael S. Tsirkin wrote:
> On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote:
>> On 2011-04-27 16:30, Michael S. Tsirkin wrote:
>>>>>> --- a/hw/pci.c
>>>>>> +++ b/hw/pci.c
>>>>>> @@ -34,6 +34,7 @@
>>>>>>  #include "device-assignment.h"
>>>>>>  #include "qemu-objects.h"
>>>>>>  #include "range.h"
>>>>>> +#include "msi.h"
>>>>>>  
>>>>>>  //#define DEBUG_PCI
>>>>>>  #ifdef DEBUG_PCI
>>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
>>>>>>      memcpy(s->config, config, size);
>>>>>>  
>>>>>>      pci_update_mappings(s);
>>>>>> +    msi_post_load(s);
>>>>>
>>>>> Pls don't do this: I'm trying to keep just the core in
>>>>> pci.c and all capabilities in separate files.
>>>>> msix has msix_load, msi will just need one too,
>>>>> and let all devices call that.
>>>>>
>>>>
>>>> Preferred alternatives are...? Registering a vmstate for msi?
>>>>
>>>> Jan
>>>
>>> Add msi_load and call that from devices that need it.
>>> Like msix_load does now.
>>>
>>
>> msix_load/save are refactoring candidates IMHO. MSI-X has a real need
>> for storing additional state information, so it should register its own
>> subsection.
> 
> That's an implementation detail though, isn't it.
> 
>> I don't want to offload this burden to the devices also for
>> MSI.
>> From the devices' POV, why shouldn't msi_init suffice?
>>
>> Jan
> 
> One can also claim this about config writes:
>     pci_bridge_write_config(d, address, val, len);
>     pcie_cap_flr_write_config(d, address, val, len);
>     pcie_cap_slot_write_config(d, address, val, len);
>     msi_write_config(d, address, val, len);
>     pcie_aer_write_config(d, address, val, len);
> which arguably just duplicates the initialization sequence.
> 
> What I'm trying to do though is to keep it modular and
> keep module inter-dependencies to a minimum,
> so that pci is the core and msix depends on it
> but not the other way around.

I still don't see the bigger benefit in saving a single bidirectional
dependency at core level vs. saving additional callbacks at each and
every MSI user. The latter is also a source for bugs.

Jan

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 05:21:43PM +0200, Jan Kiszka wrote:
> On 2011-04-27 17:09, Michael S. Tsirkin wrote:
> > On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote:
> >> On 2011-04-27 16:30, Michael S. Tsirkin wrote:
> >>>>>> --- a/hw/pci.c
> >>>>>> +++ b/hw/pci.c
> >>>>>> @@ -34,6 +34,7 @@
> >>>>>>  #include "device-assignment.h"
> >>>>>>  #include "qemu-objects.h"
> >>>>>>  #include "range.h"
> >>>>>> +#include "msi.h"
> >>>>>>  
> >>>>>>  //#define DEBUG_PCI
> >>>>>>  #ifdef DEBUG_PCI
> >>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
> >>>>>>      memcpy(s->config, config, size);
> >>>>>>  
> >>>>>>      pci_update_mappings(s);
> >>>>>> +    msi_post_load(s);
> >>>>>
> >>>>> Pls don't do this: I'm trying to keep just the core in
> >>>>> pci.c and all capabilities in separate files.
> >>>>> msix has msix_load, msi will just need one too,
> >>>>> and let all devices call that.
> >>>>>
> >>>>
> >>>> Preferred alternatives are...? Registering a vmstate for msi?
> >>>>
> >>>> Jan
> >>>
> >>> Add msi_load and call that from devices that need it.
> >>> Like msix_load does now.
> >>>
> >>
> >> msix_load/save are refactoring candidates IMHO. MSI-X has a real need
> >> for storing additional state information, so it should register its own
> >> subsection.
> > 
> > That's an implementation detail though, isn't it.
> > 
> >> I don't want to offload this burden to the devices also for
> >> MSI.
> >> From the devices' POV, why shouldn't msi_init suffice?
> >>
> >> Jan
> > 
> > One can also claim this about config writes:
> >     pci_bridge_write_config(d, address, val, len);
> >     pcie_cap_flr_write_config(d, address, val, len);
> >     pcie_cap_slot_write_config(d, address, val, len);
> >     msi_write_config(d, address, val, len);
> >     pcie_aer_write_config(d, address, val, len);
> > which arguably just duplicates the initialization sequence.
> > 
> > What I'm trying to do though is to keep it modular and
> > keep module inter-dependencies to a minimum,
> > so that pci is the core and msix depends on it
> > but not the other way around.
> 
> I still don't see the bigger benefit in saving a single bidirectional
> dependency at core level vs. saving additional callbacks at each and
> every MSI user. The latter is also a source for bugs.
> 
> Jan

Yes but let us be consistent with how e.g. config writes are
handled. As I said

> > What I think we should do is to add a pci subdirectory, move all
> > of the stuff there, move pci.c to pci/core.c and
> > add a high level module that depends on them all
> > and deals with all the capabilities.

Which will solve both issues without need for tradeoffs.

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Jan Kiszka]

On 2011-04-27 18:02, Michael S. Tsirkin wrote:
> On Wed, Apr 27, 2011 at 05:21:43PM +0200, Jan Kiszka wrote:
>> On 2011-04-27 17:09, Michael S. Tsirkin wrote:
>>> On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote:
>>>> On 2011-04-27 16:30, Michael S. Tsirkin wrote:
>>>>>>>> --- a/hw/pci.c
>>>>>>>> +++ b/hw/pci.c
>>>>>>>> @@ -34,6 +34,7 @@
>>>>>>>>  #include "device-assignment.h"
>>>>>>>>  #include "qemu-objects.h"
>>>>>>>>  #include "range.h"
>>>>>>>> +#include "msi.h"
>>>>>>>>  
>>>>>>>>  //#define DEBUG_PCI
>>>>>>>>  #ifdef DEBUG_PCI
>>>>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
>>>>>>>>      memcpy(s->config, config, size);
>>>>>>>>  
>>>>>>>>      pci_update_mappings(s);
>>>>>>>> +    msi_post_load(s);
>>>>>>>
>>>>>>> Pls don't do this: I'm trying to keep just the core in
>>>>>>> pci.c and all capabilities in separate files.
>>>>>>> msix has msix_load, msi will just need one too,
>>>>>>> and let all devices call that.
>>>>>>>
>>>>>>
>>>>>> Preferred alternatives are...? Registering a vmstate for msi?
>>>>>>
>>>>>> Jan
>>>>>
>>>>> Add msi_load and call that from devices that need it.
>>>>> Like msix_load does now.
>>>>>
>>>>
>>>> msix_load/save are refactoring candidates IMHO. MSI-X has a real need
>>>> for storing additional state information, so it should register its own
>>>> subsection.
>>>
>>> That's an implementation detail though, isn't it.
>>>
>>>> I don't want to offload this burden to the devices also for
>>>> MSI.
>>>> From the devices' POV, why shouldn't msi_init suffice?
>>>>
>>>> Jan
>>>
>>> One can also claim this about config writes:
>>>     pci_bridge_write_config(d, address, val, len);
>>>     pcie_cap_flr_write_config(d, address, val, len);
>>>     pcie_cap_slot_write_config(d, address, val, len);
>>>     msi_write_config(d, address, val, len);
>>>     pcie_aer_write_config(d, address, val, len);
>>> which arguably just duplicates the initialization sequence.
>>>
>>> What I'm trying to do though is to keep it modular and
>>> keep module inter-dependencies to a minimum,
>>> so that pci is the core and msix depends on it
>>> but not the other way around.
>>
>> I still don't see the bigger benefit in saving a single bidirectional
>> dependency at core level vs. saving additional callbacks at each and
>> every MSI user. The latter is also a source for bugs.
>>
>> Jan
> 
> Yes but let us be consistent with how e.g. config writes are
> handled. As I said
> 
>>> What I think we should do is to add a pci subdirectory, move all
>>> of the stuff there, move pci.c to pci/core.c and
>>> add a high level module that depends on them all
>>> and deals with all the capabilities.
> 
> Which will solve both issues without need for tradeoffs.

OK, but this patch is not about doing a pci refactoring. And it should
not touch any device without a need. So what is your suggestion again?

Jan

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

Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

[Michael S. Tsirkin]

On Wed, Apr 27, 2011 at 06:20:52PM +0200, Jan Kiszka wrote:
> On 2011-04-27 18:02, Michael S. Tsirkin wrote:
> > On Wed, Apr 27, 2011 at 05:21:43PM +0200, Jan Kiszka wrote:
> >> On 2011-04-27 17:09, Michael S. Tsirkin wrote:
> >>> On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote:
> >>>> On 2011-04-27 16:30, Michael S. Tsirkin wrote:
> >>>>>>>> --- a/hw/pci.c
> >>>>>>>> +++ b/hw/pci.c
> >>>>>>>> @@ -34,6 +34,7 @@
> >>>>>>>>  #include "device-assignment.h"
> >>>>>>>>  #include "qemu-objects.h"
> >>>>>>>>  #include "range.h"
> >>>>>>>> +#include "msi.h"
> >>>>>>>>  
> >>>>>>>>  //#define DEBUG_PCI
> >>>>>>>>  #ifdef DEBUG_PCI
> >>>>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
> >>>>>>>>      memcpy(s->config, config, size);
> >>>>>>>>  
> >>>>>>>>      pci_update_mappings(s);
> >>>>>>>> +    msi_post_load(s);
> >>>>>>>
> >>>>>>> Pls don't do this: I'm trying to keep just the core in
> >>>>>>> pci.c and all capabilities in separate files.
> >>>>>>> msix has msix_load, msi will just need one too,
> >>>>>>> and let all devices call that.
> >>>>>>>
> >>>>>>
> >>>>>> Preferred alternatives are...? Registering a vmstate for msi?
> >>>>>>
> >>>>>> Jan
> >>>>>
> >>>>> Add msi_load and call that from devices that need it.
> >>>>> Like msix_load does now.
> >>>>>
> >>>>
> >>>> msix_load/save are refactoring candidates IMHO. MSI-X has a real need
> >>>> for storing additional state information, so it should register its own
> >>>> subsection.
> >>>
> >>> That's an implementation detail though, isn't it.
> >>>
> >>>> I don't want to offload this burden to the devices also for
> >>>> MSI.
> >>>> From the devices' POV, why shouldn't msi_init suffice?
> >>>>
> >>>> Jan
> >>>
> >>> One can also claim this about config writes:
> >>>     pci_bridge_write_config(d, address, val, len);
> >>>     pcie_cap_flr_write_config(d, address, val, len);
> >>>     pcie_cap_slot_write_config(d, address, val, len);
> >>>     msi_write_config(d, address, val, len);
> >>>     pcie_aer_write_config(d, address, val, len);
> >>> which arguably just duplicates the initialization sequence.
> >>>
> >>> What I'm trying to do though is to keep it modular and
> >>> keep module inter-dependencies to a minimum,
> >>> so that pci is the core and msix depends on it
> >>> but not the other way around.
> >>
> >> I still don't see the bigger benefit in saving a single bidirectional
> >> dependency at core level vs. saving additional callbacks at each and
> >> every MSI user. The latter is also a source for bugs.
> >>
> >> Jan
> > 
> > Yes but let us be consistent with how e.g. config writes are
> > handled. As I said
> > 
> >>> What I think we should do is to add a pci subdirectory, move all
> >>> of the stuff there, move pci.c to pci/core.c and
> >>> add a high level module that depends on them all
> >>> and deals with all the capabilities.
> > 
> > Which will solve both issues without need for tradeoffs.
> 
> OK, but this patch is not about doing a pci refactoring. And it should
> not touch any device without a need. So what is your suggestion again?
> 
> Jan

Either add msi_load call in 4 msi users or keep your patch as is. I'll fix
it up when I do the refactoring.

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