pci-assign: better MSI-X table support

When a guest enables MSI-X in PCI configuration space we walk through the MSI-X vector table trying to guess what might get used. We have to guess because we don't do anything with writes to the vector table, so we look for clues ahead of time to pre-enable the vectors we think will be used. This means that instead of doing the sane thing and test the mask bit, we test whether the data field contains a non-zero value. It's amazing this works as well as it does.

However, two key things missed by doing this is that we don't catch vector changes after enabling (ex. setting smp_affinity on an irq) and we don't support guests that don't touch the vector table prior to enabling the MSI-X capability (ex. freebsd). This patch fixes both of these problems.

NB we're not actually masking vectors yet with this patch as it's unclear whether we really have the ability to do this without losing interrupts.

Signed-off-by Alex Williamson <[email protected]>

---

 hw/device-assignment.c |   92 ++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 81 insertions(+), 11 deletions(-)

hw/device-assignment.c +81 -11

index 71685ee..9791ec9 100644
@@ -964,6 +964,11 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
     }
 }
 
+static bool msix_masked(MSIXTableEntry *entry)
+{
+    return (entry->ctrl & cpu_to_le32(0x1)) != 0;
+}
+
 static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
 {
     AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);

@@ -975,17 +980,19 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
 
     /* Get the usable entry number for allocating */
     for (i = 0; i < adev->msix_max; i++, entry++) {
-        /* Ignore unused entry even it's unmasked */
-        if (entry->data == 0) {
+        if (msix_masked(entry)) {
             continue;
         }
         entries_nr++;
     }
 
-    if (entries_nr == 0) {
-        fprintf(stderr, "MSI-X entry number is zero!\n");
-        return -EINVAL;
+    DEBUG("MSI-X entries: %d\n", entries_nr);
+
+    /* It's valid to enable MSI-X with all entries masked */
+    if (!entries_nr) {
+        return 0;
     }
+
     msix_nr.assigned_dev_id = calc_assigned_dev_id(adev);
     msix_nr.entry_nr = entries_nr;
     r = kvm_assign_set_msix_nr(kvm_state, &msix_nr);

@@ -1003,7 +1010,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
     msix_entry.assigned_dev_id = msix_nr.assigned_dev_id;
     entry = adev->msix_table;
     for (i = 0; i < adev->msix_max; i++, entry++) {
-        if (entry->data == 0) {
+        if (msix_masked(entry)) {
             continue;
         }
 

@@ -1075,9 +1082,12 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev)
             perror("assigned_dev_update_msix_mmio");
             return;
         }
-        if (kvm_assign_irq(kvm_state, &assigned_irq_data) < 0) {
-            perror("assigned_dev_enable_msix: assign irq");
-            return;
+
+        if (assigned_dev->irq_entries_nr) {
+            if (kvm_assign_irq(kvm_state, &assigned_irq_data) < 0) {
+                perror("assigned_dev_enable_msix: assign irq");
+                return;
+            }
         }
         assigned_dev->girq = -1;
         assigned_dev->irq_requested_type = assigned_irq_data.flags;

@@ -1438,11 +1448,71 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
                             uint64_t val, unsigned size)
 {
     AssignedDevice *adev = opaque;
+    PCIDevice *pdev = &adev->dev;
+    uint16_t ctrl;
+    MSIXTableEntry orig;
+    int i = addr >> 4;
+
+    if (i >= adev->msix_max) {
+        return; /* Drop write */
+    }
 
-    DEBUG("write to MSI-X entry table mmio offset 0x%lx, val 0x%lx\n",
-          addr, val);
+    ctrl = pci_get_word(pdev->config + pdev->msix_cap + PCI_MSIX_FLAGS);
+
+    DEBUG("write to MSI-X table offset 0x%lx, val 0x%lx\n", addr, val);
+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        orig = adev->msix_table[i];
+    }
 
     memcpy((void *)((uint8_t *)adev->msix_table + addr), &val, size);
+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        MSIXTableEntry *entry = &adev->msix_table[i];
+
+        if (!msix_masked(&orig) && msix_masked(entry)) {
+            /*
+             * Vector masked, disable it
+             *
+             * XXX It's not clear if we can or should actually attempt
+             * to mask or disable the interrupt.  KVM doesn't have
+             * support for pending bits and kvm_assign_set_msix_entry
+             * doesn't modify the device hardware mask.  Interrupts
+             * while masked are simply not injected to the guest, so
+             * are lost.  Can we get away with always injecting an
+             * interrupt on unmask?
+             */
+        } else if (msix_masked(&orig) && !msix_masked(entry)) {
+            /* Vector unmasked */
+            if (i >= adev->irq_entries_nr || !adev->entry[i].type) {
+                /* Previously unassigned vector, start from scratch */
+                assigned_dev_update_msix(pdev);
+                return;
+            } else {
+                /* Update an existing, previously masked vector */
+                struct kvm_irq_routing_entry orig = adev->entry[i];
+                int ret;
+
+                adev->entry[i].u.msi.address_lo = entry->addr_lo;
+                adev->entry[i].u.msi.address_hi = entry->addr_hi;
+                adev->entry[i].u.msi.data = entry->data;
+
+                ret = kvm_update_routing_entry(&orig, &adev->entry[i]);
+                if (ret) {
+                    fprintf(stderr,
+                            "Error updating irq routing entry (%d)\n", ret);
+                    return;
+                }
+
+                ret = kvm_commit_irq_routes();
+                if (ret) {
+                    fprintf(stderr,
+                            "Error committing irq routes (%d)\n", ret);
+                    return;
+                }
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_mmio_ops = {

Quoted 19 lines · Alex Williamson

When a guest enables MSI-X in PCI configuration space we walk through the MSI-X vector table trying to guess what might get used. We have to guess because we don't do anything with writes to the vector table, so we look for clues ahead of time to pre-enable the vectors we think will be used. This means that instead of doing the sane thing and test the mask bit, we test whether the data field contains a non-zero value. It's amazing this works as well as it does.

However, two key things missed by doing this is that we don't catch vector changes after enabling (ex. setting smp_affinity on an irq) and we don't support guests that don't touch the vector table prior to enabling the MSI-X capability (ex. freebsd). This patch fixes both of these problems.

NB we're not actually masking vectors yet with this patch as it's unclear whether we really have the ability to do this without losing interrupts.

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

Overall I think this is better than what we have now. One question though: would it make sense to use msix mask notifiers instead of parsing msix tables on your own?

Quoted 145 lines

---

 hw/device-assignment.c |   92 ++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 81 insertions(+), 11 deletions(-)

hw/device-assignment.c +81 -11

index 71685ee..9791ec9 100644
@@ -964,6 +964,11 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
     }
 }
 
+static bool msix_masked(MSIXTableEntry *entry)
+{
+    return (entry->ctrl & cpu_to_le32(0x1)) != 0;
+}
+
 static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
 {
     AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);

@@ -975,17 +980,19 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
 
     /* Get the usable entry number for allocating */
     for (i = 0; i < adev->msix_max; i++, entry++) {
-        /* Ignore unused entry even it's unmasked */
-        if (entry->data == 0) {
+        if (msix_masked(entry)) {
             continue;
         }
         entries_nr++;
     }
 
-    if (entries_nr == 0) {
-        fprintf(stderr, "MSI-X entry number is zero!\n");
-        return -EINVAL;
+    DEBUG("MSI-X entries: %d\n", entries_nr);
+
+    /* It's valid to enable MSI-X with all entries masked */
+    if (!entries_nr) {
+        return 0;
     }
+
     msix_nr.assigned_dev_id = calc_assigned_dev_id(adev);
     msix_nr.entry_nr = entries_nr;
     r = kvm_assign_set_msix_nr(kvm_state, &msix_nr);

@@ -1003,7 +1010,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
     msix_entry.assigned_dev_id = msix_nr.assigned_dev_id;
     entry = adev->msix_table;
     for (i = 0; i < adev->msix_max; i++, entry++) {
-        if (entry->data == 0) {
+        if (msix_masked(entry)) {
             continue;
         }
 

@@ -1075,9 +1082,12 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev)
             perror("assigned_dev_update_msix_mmio");
             return;
         }
-        if (kvm_assign_irq(kvm_state, &assigned_irq_data) < 0) {
-            perror("assigned_dev_enable_msix: assign irq");
-            return;
+
+        if (assigned_dev->irq_entries_nr) {
+            if (kvm_assign_irq(kvm_state, &assigned_irq_data) < 0) {
+                perror("assigned_dev_enable_msix: assign irq");
+                return;
+            }
         }
         assigned_dev->girq = -1;
         assigned_dev->irq_requested_type = assigned_irq_data.flags;

@@ -1438,11 +1448,71 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
                             uint64_t val, unsigned size)
 {
     AssignedDevice *adev = opaque;
+    PCIDevice *pdev = &adev->dev;
+    uint16_t ctrl;
+    MSIXTableEntry orig;
+    int i = addr >> 4;
+
+    if (i >= adev->msix_max) {
+        return; /* Drop write */
+    }
 
-    DEBUG("write to MSI-X entry table mmio offset 0x%lx, val 0x%lx\n",
-          addr, val);
+    ctrl = pci_get_word(pdev->config + pdev->msix_cap + PCI_MSIX_FLAGS);
+
+    DEBUG("write to MSI-X table offset 0x%lx, val 0x%lx\n", addr, val);
+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        orig = adev->msix_table[i];
+    }
 
     memcpy((void *)((uint8_t *)adev->msix_table + addr), &val, size);
+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        MSIXTableEntry *entry = &adev->msix_table[i];
+
+        if (!msix_masked(&orig) && msix_masked(entry)) {
+            /*
+             * Vector masked, disable it
+             *
+             * XXX It's not clear if we can or should actually attempt
+             * to mask or disable the interrupt.  KVM doesn't have
+             * support for pending bits and kvm_assign_set_msix_entry
+             * doesn't modify the device hardware mask.  Interrupts
+             * while masked are simply not injected to the guest, so
+             * are lost.  Can we get away with always injecting an
+             * interrupt on unmask?
+             */
+        } else if (msix_masked(&orig) && !msix_masked(entry)) {
+            /* Vector unmasked */
+            if (i >= adev->irq_entries_nr || !adev->entry[i].type) {
+                /* Previously unassigned vector, start from scratch */
+                assigned_dev_update_msix(pdev);
+                return;
+            } else {
+                /* Update an existing, previously masked vector */
+                struct kvm_irq_routing_entry orig = adev->entry[i];
+                int ret;
+
+                adev->entry[i].u.msi.address_lo = entry->addr_lo;
+                adev->entry[i].u.msi.address_hi = entry->addr_hi;
+                adev->entry[i].u.msi.data = entry->data;
+
+                ret = kvm_update_routing_entry(&orig, &adev->entry[i]);
+                if (ret) {
+                    fprintf(stderr,
+                            "Error updating irq routing entry (%d)\n", ret);
+                    return;
+                }
+
+                ret = kvm_commit_irq_routes();
+                if (ret) {
+                    fprintf(stderr,
+                            "Error committing irq routes (%d)\n", ret);
+                    return;
+                }
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_mmio_ops = {

Quoted 20 lines · Michael S. Tsirkin

Quoted 19 lines · Alex Williamson

When a guest enables MSI-X in PCI configuration space we walk through the MSI-X vector table trying to guess what might get used. We have to guess because we don't do anything with writes to the vector table, so we look for clues ahead of time to pre-enable the vectors we think will be used. This means that instead of doing the sane thing and test the mask bit, we test whether the data field contains a non-zero value. It's amazing this works as well as it does.

However, two key things missed by doing this is that we don't catch vector changes after enabling (ex. setting smp_affinity on an irq) and we don't support guests that don't touch the vector table prior to enabling the MSI-X capability (ex. freebsd). This patch fixes both of these problems.

NB we're not actually masking vectors yet with this patch as it's unclear whether we really have the ability to do this without losing interrupts.

Quoted 5 lines

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

Overall I think this is better than what we have now. One question though: would it make sense to use msix mask notifiers instead of parsing msix tables on your own?

Based on vfio, I think some minor changes would be necessary to the msix core to port to that infrastructure.

I don't think I'd want to use this
code to push those changes since it likely won't get ported to qemu.git.

Quoted 20 lines · Michael S. Tsirkin

Quoted 19 lines · Alex Williamson

When a guest enables MSI-X in PCI configuration space we walk through the MSI-X vector table trying to guess what might get used. We have to guess because we don't do anything with writes to the vector table, so we look for clues ahead of time to pre-enable the vectors we think will be used. This means that instead of doing the sane thing and test the mask bit, we test whether the data field contains a non-zero value. It's amazing this works as well as it does.

However, two key things missed by doing this is that we don't catch vector changes after enabling (ex. setting smp_affinity on an irq) and we don't support guests that don't touch the vector table prior to enabling the MSI-X capability (ex. freebsd). This patch fixes both of these problems.

NB we're not actually masking vectors yet with this patch as it's unclear whether we really have the ability to do this without losing interrupts.

Quoted 5 lines

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

Overall I think this is better than what we have now. One question though: would it make sense to use msix mask notifiers instead of parsing msix tables on your own?

Based on vfio, I think some minor changes would be necessary to the msix core to port to that infrastructure. I don't think I'd want to use this code to push those changes since it likely won't get ported to qemu.git. The parsing is pretty trivial now that we're not counting bytes anyway. Thanks,

Alex

Quoted 146 lines

Quoted 146 lines

---

 hw/device-assignment.c |   92 ++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 81 insertions(+), 11 deletions(-)

hw/device-assignment.c +81 -11

index 71685ee..9791ec9 100644
@@ -964,6 +964,11 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
     }
 }
 
+static bool msix_masked(MSIXTableEntry *entry)
+{
+    return (entry->ctrl & cpu_to_le32(0x1)) != 0;
+}
+
 static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
 {
     AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);

@@ -975,17 +980,19 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
 
     /* Get the usable entry number for allocating */
     for (i = 0; i < adev->msix_max; i++, entry++) {
-        /* Ignore unused entry even it's unmasked */
-        if (entry->data == 0) {
+        if (msix_masked(entry)) {
             continue;
         }
         entries_nr++;
     }
 
-    if (entries_nr == 0) {
-        fprintf(stderr, "MSI-X entry number is zero!\n");
-        return -EINVAL;
+    DEBUG("MSI-X entries: %d\n", entries_nr);
+
+    /* It's valid to enable MSI-X with all entries masked */
+    if (!entries_nr) {
+        return 0;
     }
+
     msix_nr.assigned_dev_id = calc_assigned_dev_id(adev);
     msix_nr.entry_nr = entries_nr;
     r = kvm_assign_set_msix_nr(kvm_state, &msix_nr);

@@ -1003,7 +1010,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
     msix_entry.assigned_dev_id = msix_nr.assigned_dev_id;
     entry = adev->msix_table;
     for (i = 0; i < adev->msix_max; i++, entry++) {
-        if (entry->data == 0) {
+        if (msix_masked(entry)) {
             continue;
         }
 

@@ -1075,9 +1082,12 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev)
             perror("assigned_dev_update_msix_mmio");
             return;
         }
-        if (kvm_assign_irq(kvm_state, &assigned_irq_data) < 0) {
-            perror("assigned_dev_enable_msix: assign irq");
-            return;
+
+        if (assigned_dev->irq_entries_nr) {
+            if (kvm_assign_irq(kvm_state, &assigned_irq_data) < 0) {
+                perror("assigned_dev_enable_msix: assign irq");
+                return;
+            }
         }
         assigned_dev->girq = -1;
         assigned_dev->irq_requested_type = assigned_irq_data.flags;

@@ -1438,11 +1448,71 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
                             uint64_t val, unsigned size)
 {
     AssignedDevice *adev = opaque;
+    PCIDevice *pdev = &adev->dev;
+    uint16_t ctrl;
+    MSIXTableEntry orig;
+    int i = addr >> 4;
+
+    if (i >= adev->msix_max) {
+        return; /* Drop write */
+    }
 
-    DEBUG("write to MSI-X entry table mmio offset 0x%lx, val 0x%lx\n",
-          addr, val);
+    ctrl = pci_get_word(pdev->config + pdev->msix_cap + PCI_MSIX_FLAGS);
+
+    DEBUG("write to MSI-X table offset 0x%lx, val 0x%lx\n", addr, val);
+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        orig = adev->msix_table[i];
+    }
 
     memcpy((void *)((uint8_t *)adev->msix_table + addr), &val, size);
+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        MSIXTableEntry *entry = &adev->msix_table[i];
+
+        if (!msix_masked(&orig) && msix_masked(entry)) {
+            /*
+             * Vector masked, disable it
+             *
+             * XXX It's not clear if we can or should actually attempt
+             * to mask or disable the interrupt.  KVM doesn't have
+             * support for pending bits and kvm_assign_set_msix_entry
+             * doesn't modify the device hardware mask.  Interrupts
+             * while masked are simply not injected to the guest, so
+             * are lost.  Can we get away with always injecting an
+             * interrupt on unmask?
+             */
+        } else if (msix_masked(&orig) && !msix_masked(entry)) {
+            /* Vector unmasked */
+            if (i >= adev->irq_entries_nr || !adev->entry[i].type) {
+                /* Previously unassigned vector, start from scratch */
+                assigned_dev_update_msix(pdev);
+                return;
+            } else {
+                /* Update an existing, previously masked vector */
+                struct kvm_irq_routing_entry orig = adev->entry[i];
+                int ret;
+
+                adev->entry[i].u.msi.address_lo = entry->addr_lo;
+                adev->entry[i].u.msi.address_hi = entry->addr_hi;
+                adev->entry[i].u.msi.data = entry->data;
+
+                ret = kvm_update_routing_entry(&orig, &adev->entry[i]);
+                if (ret) {
+                    fprintf(stderr,
+                            "Error updating irq routing entry (%d)\n", ret);
+                    return;
+                }
+
+                ret = kvm_commit_irq_routes();
+                if (ret) {
+                    fprintf(stderr,
+                            "Error committing irq routes (%d)\n", ret);
+                    return;
+                }
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_mmio_ops = {

hw/device-assignment.c +8 -1

@@ -1438,11 +1448,71 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
                             uint64_t val, unsigned size)
 {
     AssignedDevice *adev = opaque;
+    PCIDevice *pdev = &adev->dev;
+    uint16_t ctrl;
+    MSIXTableEntry orig;
+    int i = addr >> 4;
+
+    if (i >= adev->msix_max) {
+        return; /* Drop write */
+    }
 
-    DEBUG("write to MSI-X entry table mmio offset 0x%lx, val 0x%lx\n",

hw/device-assignment.c +47 -0

+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        MSIXTableEntry *entry = &adev->msix_table[i];
+
+        if (!msix_masked(&orig) && msix_masked(entry)) {
+            /*
+             * Vector masked, disable it
+             *
+             * XXX It's not clear if we can or should actually attempt
+             * to mask or disable the interrupt.  KVM doesn't have
+             * support for pending bits and kvm_assign_set_msix_entry
+             * doesn't modify the device hardware mask.  Interrupts
+             * while masked are simply not injected to the guest, so
+             * are lost.  Can we get away with always injecting an
+             * interrupt on unmask?
+             */
+        } else if (msix_masked(&orig) && !msix_masked(entry)) {
+            /* Vector unmasked */
+            if (i >= adev->irq_entries_nr || !adev->entry[i].type) {
+                /* Previously unassigned vector, start from scratch */
+                assigned_dev_update_msix(pdev);
+                return;
+            } else {
+                /* Update an existing, previously masked vector */
+                struct kvm_irq_routing_entry orig = adev->entry[i];
+                int ret;
+
+                adev->entry[i].u.msi.address_lo = entry->addr_lo;
+                adev->entry[i].u.msi.address_hi = entry->addr_hi;
+                adev->entry[i].u.msi.data = entry->data;
+
+                ret = kvm_update_routing_entry(&orig, &adev->entry[i]);
+                if (ret) {
+                    fprintf(stderr,
+                            "Error updating irq routing entry (%d)\n", ret);
+                    return;
+                }
+
+                ret = kvm_commit_irq_routes();
+                if (ret) {
+                    fprintf(stderr,
+                            "Error committing irq routes (%d)\n", ret);
+                    return;
+                }
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_mmio_ops = {

Quoted 23 lines · Alex Williamson

hw/device-assignment.c +15 -2

@@ -1438,11 +1448,71 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
                             uint64_t val, unsigned size)
 {
     AssignedDevice *adev = opaque;
+    PCIDevice *pdev = &adev->dev;
+    uint16_t ctrl;
+    MSIXTableEntry orig;
+    int i = addr >> 4;
+
+    if (i >= adev->msix_max) {
+        return; /* Drop write */
+    }
 
-    DEBUG("write to MSI-X entry table mmio offset 0x%lx, val 0x%lx\n",
-          addr, val);
+    ctrl = pci_get_word(pdev->config + pdev->msix_cap + PCI_MSIX_FLAGS);
+
+    DEBUG("write to MSI-X table offset 0x%lx, val 0x%lx\n", addr, val);
+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        orig = adev->msix_table[i];
+    }
 

hw/device-assignment.c static void msix_mmio_write(void *opaque, target_phys_addr_t addr, · line 1468

Quoted 1 line

hw/device-assignment.c +0 -0

     memcpy((void *)((uint8_t *)adev->msix_table + addr), &val, size);

Does the core guarantee alignment even if guest violates the rules? I ask because we validate i above but don't check the low bits of addr, so a misaligned call can access beyond the array boundary.

If core does not ensure alignment we must check here ourselves.

Quoted 50 lines

hw/device-assignment.c +47 -0

+
+    if (ctrl & PCI_MSIX_FLAGS_ENABLE) {
+        MSIXTableEntry *entry = &adev->msix_table[i];
+
+        if (!msix_masked(&orig) && msix_masked(entry)) {
+            /*
+             * Vector masked, disable it
+             *
+             * XXX It's not clear if we can or should actually attempt
+             * to mask or disable the interrupt.  KVM doesn't have
+             * support for pending bits and kvm_assign_set_msix_entry
+             * doesn't modify the device hardware mask.  Interrupts
+             * while masked are simply not injected to the guest, so
+             * are lost.  Can we get away with always injecting an
+             * interrupt on unmask?
+             */
+        } else if (msix_masked(&orig) && !msix_masked(entry)) {
+            /* Vector unmasked */
+            if (i >= adev->irq_entries_nr || !adev->entry[i].type) {
+                /* Previously unassigned vector, start from scratch */
+                assigned_dev_update_msix(pdev);
+                return;
+            } else {
+                /* Update an existing, previously masked vector */
+                struct kvm_irq_routing_entry orig = adev->entry[i];
+                int ret;
+
+                adev->entry[i].u.msi.address_lo = entry->addr_lo;
+                adev->entry[i].u.msi.address_hi = entry->addr_hi;
+                adev->entry[i].u.msi.data = entry->data;
+
+                ret = kvm_update_routing_entry(&orig, &adev->entry[i]);
+                if (ret) {
+                    fprintf(stderr,
+                            "Error updating irq routing entry (%d)\n", ret);
+                    return;
+                }
+
+                ret = kvm_commit_irq_routes();
+                if (ret) {
+                    fprintf(stderr,
+                            "Error committing irq routes (%d)\n", ret);
+                    return;
+                }
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_mmio_ops = {

This series enables better runtime MSI-X table support so that we can track vector updates for routing, enabling guest interrupt smp_affinity, as well as vectors setup after the MSI-X PCI capability is enabled, allowing for MSI-X on devices assigned to FreeBSD guests. Thanks,

Alex

v2:
- Dropping mem64, we can work on this elsewhere
- Fixed MemoryRegionOps to use .valid, tested 8-byte access
- Endian conversions when testing MSI-X vector mask bit
- Patch description updates
- Misc cleanups

---

Alex Williamson (8):
pci-assign: Update MSI-X config based on table writes
pci-assign: Use MSIX_PAGE_SIZE
pci-assign: Allocate entries for all MSI-X vectors
pci-assign: Proper initialization for MSI-X table
pci-assign: Only calculate maximum MSI-X vector entries once
pci-assign: Use struct for MSI-X table
pci-assign: Update MSI-X MMIO to Memory API
pci-assign: Fix warnings with DEBUG enabled

 hw/device-assignment.c |  262 +++++++++++++++++++++++++++++-------------------
 hw/device-assignment.h |   10 ++
 2 files changed, 169 insertions(+), 103 deletions(-)