[Qemu-devel] [PATCH 0/2] pci-assign: MSI affinity support

[Qemu-devel] [PATCH 0/2] pci-assign: MSI affinity support

[Alex Williamson]

I posted these about 6 months ago and Jan felt we should implement
MSI notifiers like we have for MSI-X.  That still hasn't happened.
This is a pretty trivial addition to pci-assign and could easily
be ported to MSI notifiers if anyone cares enough down the road
to implement them.  I don't feel that this, or the vfio patch that
adds the same, warrants that kind of infrastructure.

I expect these are for qemu-1.6.  Thanks,

Alex

---

Alex Williamson (2):
      pci-assign: Refactor MSI virq array
      pci-assign: Add MSI affinity support


 hw/i386/kvm/pci-assign.c |   78 +++++++++++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 25 deletions(-)

[Qemu-devel] [PATCH 1/2] pci-assign: Refactor MSI virq array

[Alex Williamson]

Convert the msi_virq array into a struct array so we can easily add
a place to track the MSIMessage for each vector.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/i386/kvm/pci-assign.c |   51 +++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index c1e08ec..0f83a4c 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -131,8 +131,10 @@ typedef struct AssignedDevice {
     } cap;
     uint8_t emulate_config_read[PCI_CONFIG_SPACE_SIZE];
     uint8_t emulate_config_write[PCI_CONFIG_SPACE_SIZE];
-    int msi_virq_nr;
-    int *msi_virq;
+    int msi_nr;
+    struct {
+        int virq;
+    } *msi;
     MSIXTableEntry *msix_table;
     hwaddr msix_table_addr;
     uint16_t msix_max;
@@ -689,19 +691,18 @@ again:
     return 0;
 }
 
-static void free_msi_virqs(AssignedDevice *dev)
+static void free_msi(AssignedDevice *dev)
 {
     int i;
 
-    for (i = 0; i < dev->msi_virq_nr; i++) {
-        if (dev->msi_virq[i] >= 0) {
-            kvm_irqchip_release_virq(kvm_state, dev->msi_virq[i]);
-            dev->msi_virq[i] = -1;
+    for (i = 0; i < dev->msi_nr; i++) {
+        if (dev->msi[i].virq >= 0) {
+            kvm_irqchip_release_virq(kvm_state, dev->msi[i].virq);
         }
     }
-    g_free(dev->msi_virq);
-    dev->msi_virq = NULL;
-    dev->msi_virq_nr = 0;
+    g_free(dev->msi);
+    dev->msi = NULL;
+    dev->msi_nr = 0;
 }
 
 static void free_assigned_device(AssignedDevice *dev)
@@ -756,7 +757,7 @@ static void free_assigned_device(AssignedDevice *dev)
         close(dev->real_device.config_fd);
     }
 
-    free_msi_virqs(dev);
+    free_msi(dev);
 }
 
 static void assign_failed_examine(AssignedDevice *dev)
@@ -995,7 +996,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
             perror("assigned_dev_update_msi: deassign irq");
         }
 
-        free_msi_virqs(assigned_dev);
+        free_msi(assigned_dev);
 
         assigned_dev->assigned_irq_type = ASSIGNED_IRQ_NONE;
         pci_device_set_intx_routing_notifier(pci_dev, NULL);
@@ -1011,9 +1012,9 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
             return;
         }
 
-        assigned_dev->msi_virq = g_malloc(sizeof(*assigned_dev->msi_virq));
-        assigned_dev->msi_virq_nr = 1;
-        assigned_dev->msi_virq[0] = virq;
+        assigned_dev->msi = g_malloc(sizeof(*assigned_dev->msi));
+        assigned_dev->msi_nr = 1;
+        assigned_dev->msi[0].virq = virq;
         if (kvm_device_msi_assign(kvm_state, assigned_dev->dev_id, virq) < 0) {
             perror("assigned_dev_update_msi: kvm_device_msi_assign");
         }
@@ -1074,14 +1075,14 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
         return r;
     }
 
-    free_msi_virqs(adev);
+    free_msi(adev);
 
-    adev->msi_virq_nr = adev->msix_max;
-    adev->msi_virq = g_malloc(adev->msix_max * sizeof(*adev->msi_virq));
+    adev->msi_nr = adev->msix_max;
+    adev->msi = g_malloc(adev->msix_max * sizeof(*adev->msi));
 
     entry = adev->msix_table;
     for (i = 0; i < adev->msix_max; i++, entry++) {
-        adev->msi_virq[i] = -1;
+        adev->msi[i].virq = -1;
 
         if (assigned_dev_msix_skipped(entry)) {
             continue;
@@ -1093,13 +1094,13 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
         if (r < 0) {
             return r;
         }
-        adev->msi_virq[i] = r;
+        adev->msi[i].virq = r;
 
         DEBUG("MSI-X vector %d, gsi %d, addr %08x_%08x, data %08x\n", i,
               r, entry->addr_hi, entry->addr_lo, entry->data);
 
         r = kvm_device_msix_set_vector(kvm_state, adev->dev_id, i,
-                                       adev->msi_virq[i]);
+                                       adev->msi[i].virq);
         if (r) {
             error_report("fail to set MSI-X entry! %s", strerror(-r));
             break;
@@ -1127,7 +1128,7 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev)
             perror("assigned_dev_update_msix: deassign irq");
         }
 
-        free_msi_virqs(assigned_dev);
+        free_msi(assigned_dev);
 
         assigned_dev->assigned_irq_type = ASSIGNED_IRQ_NONE;
         pci_device_set_intx_routing_notifier(pci_dev, NULL);
@@ -1139,7 +1140,7 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev)
             return;
         }
 
-        if (assigned_dev->msi_virq_nr > 0) {
+        if (assigned_dev->msi_nr > 0) {
             if (kvm_device_msix_assign(kvm_state, assigned_dev->dev_id) < 0) {
                 perror("assigned_dev_enable_msix: assign irq");
                 return;
@@ -1567,7 +1568,7 @@ static void assigned_dev_msix_mmio_write(void *opaque, hwaddr addr,
         } else if (assigned_dev_msix_masked(&orig) &&
                    !assigned_dev_msix_masked(entry)) {
             /* Vector unmasked */
-            if (i >= adev->msi_virq_nr || adev->msi_virq[i] < 0) {
+            if (i >= adev->msi_nr || adev->msi[i].virq < 0) {
                 /* Previously unassigned vector, start from scratch */
                 assigned_dev_update_msix(pdev);
                 return;
@@ -1581,7 +1582,7 @@ static void assigned_dev_msix_mmio_write(void *opaque, hwaddr addr,
                 msg.data = entry->data;
 
                 ret = kvm_irqchip_update_msi_route(kvm_state,
-                                                   adev->msi_virq[i], msg);
+                                                   adev->msi[i].virq, msg);
                 if (ret) {
                     error_report("Error updating irq routing entry (%d)", ret);
                 }

[Qemu-devel] [PATCH 2/2] pci-assign: Add MSI affinity support

[Alex Williamson]

Track the last MSIMessage programmed so we can determine when it has
changed and update the routing to the guest.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/i386/kvm/pci-assign.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 0f83a4c..e09265b 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -134,6 +134,7 @@ typedef struct AssignedDevice {
     int msi_nr;
     struct {
         int virq;
+        MSIMessage msg;
     } *msi;
     MSIXTableEntry *msix_table;
     hwaddr msix_table_addr;
@@ -1015,6 +1016,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
         assigned_dev->msi = g_malloc(sizeof(*assigned_dev->msi));
         assigned_dev->msi_nr = 1;
         assigned_dev->msi[0].virq = virq;
+        assigned_dev->msi[0].msg = msg;
         if (kvm_device_msi_assign(kvm_state, assigned_dev->dev_id, virq) < 0) {
             perror("assigned_dev_update_msi: kvm_device_msi_assign");
         }
@@ -1027,6 +1029,27 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
     }
 }
 
+static void assigned_dev_update_msi_msg(PCIDevice *pci_dev)
+{
+    AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
+    uint8_t ctrl_byte = pci_get_byte(pci_dev->config + pci_dev->msi_cap +
+                                     PCI_MSI_FLAGS);
+    MSIMessage msg;
+
+    if (assigned_dev->assigned_irq_type != ASSIGNED_IRQ_MSI ||
+        !(ctrl_byte & PCI_MSI_FLAGS_ENABLE)) {
+        return;
+    }
+
+    msg = msi_get_message(pci_dev, 0);
+
+    if (msg.address != assigned_dev->msi[0].msg.address ||
+        msg.data != assigned_dev->msi[0].msg.data) {
+        kvm_irqchip_update_msi_route(kvm_state, assigned_dev->msi[0].virq, msg);
+        assigned_dev->msi[0].msg = msg;
+    }
+}
+
 static bool assigned_dev_msix_masked(MSIXTableEntry *entry)
 {
     return (entry->ctrl & cpu_to_le32(0x1)) != 0;
@@ -1202,6 +1225,10 @@ static void assigned_dev_pci_write_config(PCIDevice *pci_dev, uint32_t address,
         if (range_covers_byte(address, len,
                               pci_dev->msi_cap + PCI_MSI_FLAGS)) {
             assigned_dev_update_msi(pci_dev);
+        } else if (ranges_overlap(address, len,
+                                  pci_dev->msi_cap + PCI_MSI_ADDRESS_LO,
+                                  10 - PCI_MSI_ADDRESS_LO)) {
+            assigned_dev_update_msi_msg(pci_dev);
         }
     }
     if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {

Re: [Qemu-devel] [PATCH 0/2] pci-assign: MSI affinity support

[Jan Kiszka]

On 2013-05-09 18:35, Alex Williamson wrote:
> I posted these about 6 months ago and Jan felt we should implement
> MSI notifiers like we have for MSI-X.  That still hasn't happened.

Device assignments are the only currently known users - and you provide
this feature, so...

POWER does this nice configuration of MSI messages via a side channel.
Not that it already fires the MSI-X notifiers properly, but a generic
notifier based approach is the right way to abstract away the different
modification channels (instead of encoding them at the consumer side
like in your patches).

Moreover, having different designs for MSI and MSI-X is just ugly.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/2] pci-assign: MSI affinity support

[Alex Williamson]

On Fri, 2013-05-10 at 14:40 +0200, Jan Kiszka wrote:
> On 2013-05-09 18:35, Alex Williamson wrote:
> > I posted these about 6 months ago and Jan felt we should implement
> > MSI notifiers like we have for MSI-X.  That still hasn't happened.
> 
> Device assignments are the only currently known users - and you provide
> this feature, so...
> 
> POWER does this nice configuration of MSI messages via a side channel.
> Not that it already fires the MSI-X notifiers properly, but a generic
> notifier based approach is the right way to abstract away the different
> modification channels (instead of encoding them at the consumer side
> like in your patches).
> 
> Moreover, having different designs for MSI and MSI-X is just ugly.

OTOH, if device assignment is the only one that needs this and it's
trivial to implement there, why would we bloat the common code with
notifiers and caching vector messages.  I'm not that thrilled with the
MSI-X notifiers to go copy them into MSI anyway, they completely miss
important things like an overall device MSI-X enable or disable.

For this much code:

 hw/i386/kvm/pci-assign.c |   78 +++++++++++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 25 deletions(-)

we get a feature that's immediately useful to a guest.  Seems like a win
to me.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 2/2] pci-assign: Add MSI affinity support

[Michael S. Tsirkin]

On Thu, May 09, 2013 at 10:36:05AM -0600, Alex Williamson wrote:
> Track the last MSIMessage programmed so we can determine when it has
> changed and update the routing to the guest.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  hw/i386/kvm/pci-assign.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index 0f83a4c..e09265b 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -134,6 +134,7 @@ typedef struct AssignedDevice {
>      int msi_nr;
>      struct {
>          int virq;
> +        MSIMessage msg;
>      } *msi;
>      MSIXTableEntry *msix_table;
>      hwaddr msix_table_addr;
> @@ -1015,6 +1016,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
>          assigned_dev->msi = g_malloc(sizeof(*assigned_dev->msi));
>          assigned_dev->msi_nr = 1;
>          assigned_dev->msi[0].virq = virq;
> +        assigned_dev->msi[0].msg = msg;
>          if (kvm_device_msi_assign(kvm_state, assigned_dev->dev_id, virq) < 0) {
>              perror("assigned_dev_update_msi: kvm_device_msi_assign");
>          }
> @@ -1027,6 +1029,27 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
>      }
>  }
>  
> +static void assigned_dev_update_msi_msg(PCIDevice *pci_dev)
> +{
> +    AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> +    uint8_t ctrl_byte = pci_get_byte(pci_dev->config + pci_dev->msi_cap +
> +                                     PCI_MSI_FLAGS);
> +    MSIMessage msg;
> +
> +    if (assigned_dev->assigned_irq_type != ASSIGNED_IRQ_MSI ||
> +        !(ctrl_byte & PCI_MSI_FLAGS_ENABLE)) {
> +        return;
> +    }
> +
> +    msg = msi_get_message(pci_dev, 0);
> +
> +    if (msg.address != assigned_dev->msi[0].msg.address ||
> +        msg.data != assigned_dev->msi[0].msg.data) {
> +        kvm_irqchip_update_msi_route(kvm_state, assigned_dev->msi[0].virq, msg);
> +        assigned_dev->msi[0].msg = msg;
> +    }
> +}
> +

Okay, but why do we track the message in AssignedDevice?
To avoid extra calls to kvm_irqchip_update_msi_route?
Let's not do premature optimizations please:
just call kvm_irqchip_update_msi_route directly,
no need to track the message in AssignedDevice.

Moreover, if you can measure a performance gain
from tracking msg in AssignedDevice there's a
better way to do this: we have an outstanding patch to kvm,
which optimized the no change case away to an array
lookup. This benefits msi-x as well.

>  static bool assigned_dev_msix_masked(MSIXTableEntry *entry)
>  {
>      return (entry->ctrl & cpu_to_le32(0x1)) != 0;
> @@ -1202,6 +1225,10 @@ static void assigned_dev_pci_write_config(PCIDevice *pci_dev, uint32_t address,
>          if (range_covers_byte(address, len,
>                                pci_dev->msi_cap + PCI_MSI_FLAGS)) {
>              assigned_dev_update_msi(pci_dev);
> +        } else if (ranges_overlap(address, len,
> +                                  pci_dev->msi_cap + PCI_MSI_ADDRESS_LO,
> +                                  10 - PCI_MSI_ADDRESS_LO)) {
> +            assigned_dev_update_msi_msg(pci_dev);
>          }
>      }
>      if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {

Re: [Qemu-devel] [PATCH 0/2] pci-assign: MSI affinity support

[Michael S. Tsirkin]

On Fri, May 10, 2013 at 02:40:04PM +0200, Jan Kiszka wrote:
> On 2013-05-09 18:35, Alex Williamson wrote:
> > I posted these about 6 months ago and Jan felt we should implement
> > MSI notifiers like we have for MSI-X.  That still hasn't happened.
> 
> Device assignments are the only currently known users - and you provide
> this feature, so...
> 
> POWER does this nice configuration of MSI messages via a side channel.
> Not that it already fires the MSI-X notifiers properly, but a generic
> notifier based approach is the right way to abstract away the different
> modification channels (instead of encoding them at the consumer side
> like in your patches).
> 
> Moreover, having different designs for MSI and MSI-X is just ugly.
> 
> Jan

I agree, but it's not immediately obvious what would
a good API look like, and I think it's an important bug to fix.
We are sending interrupts to the wrong CPU in a clear
violation of the spec.
And if we drop the tracking of the message per device we
end up with a very small patch - something like the
below - untested, but just to give you the idea.
This hardly looks like a change we need to delay until
we get proper infrastructure in place, right?
It will be just as easy to replace.

pci-assign.c |   21 +++++++++++++++++++++

---

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index c1e08ec..e0061a0 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1026,6 +1026,23 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
     }
 }
 
+/* Update MSI message without touching enable/disable bits. */
+static void assigned_dev_update_msi_msg(PCIDevice *pci_dev)
+{
+    AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
+    uint8_t ctrl_byte = pci_get_byte(pci_dev->config + pci_dev->msi_cap +
+                                     PCI_MSI_FLAGS);
+
+    if (assigned_dev->assigned_irq_type != ASSIGNED_IRQ_MSI ||
+        !(ctrl_byte & PCI_MSI_FLAGS_ENABLE)) {
+        return;
+    }
+
+    assert(assigned_dev->msi_virq_nr == 1);
+    kvm_irqchip_update_msi_route(kvm_state, assigned_dev->msi_virq[0],
+                                 msi_get_message(pci_dev, 0));
+}
+
 static bool assigned_dev_msix_masked(MSIXTableEntry *entry)
 {
     return (entry->ctrl & cpu_to_le32(0x1)) != 0;
@@ -1201,6 +1218,10 @@ static void assigned_dev_pci_write_config(PCIDevice *pci_dev, uint32_t address,
         if (range_covers_byte(address, len,
                               pci_dev->msi_cap + PCI_MSI_FLAGS)) {
             assigned_dev_update_msi(pci_dev);
+        } else if (ranges_overlap(address, len,
+                                  pci_dev->msi_cap + PCI_MSI_ADDRESS_LO,
+                                  PCI_MSI_DATA_32 + 2 - PCI_MSI_ADDRESS_LO)) {
+            assigned_dev_update_msi_msg(pci_dev);
         }
     }
     if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {

Re: [Qemu-devel] [PATCH 0/2] pci-assign: MSI affinity support

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori