pci-assign: Host IRQ sharing suppport + some fixes and cleanups

From: Jan Kiszka <[email protected]>

Use rages_overlap and proper constants to match the access range against regions that need special handling. This also fixes yet uncaught high-byte write access to the command register. Moreover, use more constants instead of magic numbers.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 10 deletions(-)

hw/device-assignment.c +29 -10

index 50c6408..bc3a57b 100644
@@ -438,13 +438,20 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
         return assigned_device_pci_cap_write_config(d, address, val, len);
     }
 
-    if (address == 0x4) {
+    if (ranges_overlap(address, len, PCI_COMMAND, 2)) {
         pci_default_write_config(d, address, val, len);
         /* Continue to program the card */
     }
 
-    if ((address >= 0x10 && address <= 0x24) || address == 0x30 ||
-        address == 0x34 || address == 0x3c || address == 0x3d) {
+    /*
+     * Catch access to
+     *  - base address registers
+     *  - ROM base address & capability pointer
+     *  - interrupt line & pin
+     */
+    if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 8) ||
+        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
         /* used for update-mappings (BAR emulation) */
         pci_default_write_config(d, address, val, len);
         return;

@@ -484,9 +491,20 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
         return val;
     }
 
-    if (address < 0x4 || (pci_dev->need_emulate_cmd && address == 0x4) ||
-	(address >= 0x10 && address <= 0x24) || address == 0x30 ||
-        address == 0x34 || address == 0x3c || address == 0x3d) {
+    /*
+     * Catch access to
+     *  - vendor & device ID
+     *  - command register (if emulation needed)
+     *  - base address registers
+     *  - ROM base address & capability pointer
+     *  - interrupt line & pin
+     */
+    if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
+        (pci_dev->need_emulate_cmd &&
+         ranges_overlap(address, len, PCI_COMMAND, 2)) ||
+        ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 8) ||
+        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
         val = pci_default_read_config(d, address, len);
         DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
               (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);

@@ -517,10 +535,11 @@ do_log:
 
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
-        if (address == 4 && len == 4)
-            val &= ~0x100000;
-        else if (address == 6)
-            val &= ~0x10;
+        if (address == PCI_COMMAND && len == 4) {
+            val &= ~(PCI_STATUS_CAP_LIST << 16);
+        } else if (address == PCI_STATUS) {
+            val &= ~PCI_STATUS_CAP_LIST;
+        }
     }
 
     return val;

From: Jan Kiszka <[email protected]>

If we emulate the command register, we must only read its content from the shadow config space. For dword read of both PCI_COMMAND and PCI_STATUS, at least the latter must be read from the device.

For simplicity reasons and as the code path is not considered performance critical for the affected SRIOV devices, the fix performes device access to the command word unconditionally, even if emulation is enabled and only that word is read.

Signed-off-by Jan Kiszka <[email protected]>

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

hw/device-assignment.c +4 -3

index bc3a57b..6ff1456 100644
@@ -494,14 +494,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
     /*
      * Catch access to
      *  - vendor & device ID
-     *  - command register (if emulation needed)
      *  - base address registers
      *  - ROM base address & capability pointer
      *  - interrupt line & pin
      */
     if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
-        (pci_dev->need_emulate_cmd &&
-         ranges_overlap(address, len, PCI_COMMAND, 2)) ||
         ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
         ranges_overlap(address, len, PCI_ROM_ADDRESS, 8) ||
         ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {

@@ -533,6 +530,17 @@ do_log:
     DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
           (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
 
+    if (pci_dev->need_emulate_cmd &&
+        ranges_overlap(address, len, PCI_COMMAND, 2)) {
+        if (address == PCI_COMMAND) {
+            val &= 0xffff0000;

From: Jan Kiszka <[email protected]>

No one can remember where this came from, and it looks very hacky anyway (we return 0 for config space address 0xfc of _every_ assigned device, not only vga as the comment claims). So better remove it and wait for the underlying issue to reappear.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/device-assignment.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

hw/device-assignment.c +0 -5

index 6ff1456..ef045f4 100644
@@ -508,10 +508,6 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
         return val;
     }
 
-    /* vga specific, remove later */
-    if (address == 0xFC)
-        goto do_log;
-
     fd = pci_dev->real_device.config_fd;
 
 again:

@@ -526,7 +522,6 @@ again:
 	exit(1);
     }
 
-do_log:
     DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
           (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
 

From: Jan Kiszka <[email protected]>

Define a mask of PCI command register bits that need to be emulated, i.e. read back from their shadow state. We will need this for selectively emulating the INTx mask bit.

Note: No initialization of emulate_cmd_mask to zero needed, the device state is already zero-initialized.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/device-assignment.c |   18 ++++++++++--------
 hw/device-assignment.h |    2 +-
 2 files changed, 11 insertions(+), 9 deletions(-)

hw/device-assignment.c +2 -3

index ef045f4..26d3bd7 100644
@@ -525,14 +525,17 @@ again:
     DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
           (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
 
-    if (pci_dev->need_emulate_cmd &&
+    if (pci_dev->emulate_cmd_mask &&
         ranges_overlap(address, len, PCI_COMMAND, 2)) {
         if (address == PCI_COMMAND) {
-            val &= 0xffff0000;
-            val |= pci_default_read_config(d, PCI_COMMAND, 2);
+            val &= ~pci_dev->emulate_cmd_mask;

hw/device-assignment.c +3 -4

@@ -800,10 +803,9 @@ again:
 
     /* dealing with virtual function device */
     snprintf(name, sizeof(name), "%sphysfn/", dir);
-    if (!stat(name, &statbuf))
-	    pci_dev->need_emulate_cmd = 1;
-    else
-	    pci_dev->need_emulate_cmd = 0;
+    if (!stat(name, &statbuf)) {
+        pci_dev->emulate_cmd_mask = 0xffff;
+    }
 
     dev->region_number = r;
     return 0;

hw/device-assignment.h +1 -1

index c94a730..9ead022 100644
@@ -109,7 +109,7 @@ typedef struct AssignedDevice {
     void *msix_table_page;
     target_phys_addr_t msix_table_addr;
     int mmio_index;
-    int need_emulate_cmd;
+    uint32_t emulate_cmd_mask;
     char *configfd_name;
     QLIST_ENTRY(AssignedDevice) next;
 } AssignedDevice;

Quoted 30 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

Define a mask of PCI command register bits that need to be emulated, i.e. read back from their shadow state. We will need this for selectively emulating the INTx mask bit.

Note: No initialization of emulate_cmd_mask to zero needed, the device state is already zero-initialized.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/device-assignment.c |   18 ++++++++++--------
 hw/device-assignment.h |    2 +-
 2 files changed, 11 insertions(+), 9 deletions(-)

hw/device-assignment.c +2 -3

index ef045f4..26d3bd7 100644
@@ -525,14 +525,17 @@ again:
     DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
           (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
 
-    if (pci_dev->need_emulate_cmd &&
+    if (pci_dev->emulate_cmd_mask &&
         ranges_overlap(address, len, PCI_COMMAND, 2)) {
         if (address == PCI_COMMAND) {
-            val &= 0xffff0000;
-            val |= pci_default_read_config(d, PCI_COMMAND, 2);
+            val &= ~pci_dev->emulate_cmd_mask;

hw/device-assignment.c again: · line 540

Quoted 10 lines

hw/device-assignment.c +5 -1

+            val |= pci_default_read_config(d, PCI_COMMAND, 2) &
+                        pci_dev->emulate_cmd_mask;
         } else {
             /* high-byte access */
-            val = pci_default_read_config(d, PCI_COMMAND+1, 1);
+            val &= ~(pci_dev->emulate_cmd_mask >> 8);
+            val |= pci_default_read_config(d, PCI_COMMAND+1, 1) &
+                        (pci_dev->emulate_cmd_mask >> 8);
         }
     }

We should definitely be using merge_bits here, this is the sort of thing I had in mind for it:

val = merge_bits(val, pci_default_read_config(d, address, len), PCI_COMMAND, pci_dev->emulate_cmd_mask);

Quoted 27 lines

hw/device-assignment.c +3 -4

@@ -800,10 +803,9 @@ again:
 
     /* dealing with virtual function device */
     snprintf(name, sizeof(name), "%sphysfn/", dir);
-    if (!stat(name, &statbuf))
-	    pci_dev->need_emulate_cmd = 1;
-    else
-	    pci_dev->need_emulate_cmd = 0;
+    if (!stat(name, &statbuf)) {
+        pci_dev->emulate_cmd_mask = 0xffff;
+    }
 
     dev->region_number = r;
     return 0;

hw/device-assignment.h +1 -1

index c94a730..9ead022 100644
@@ -109,7 +109,7 @@ typedef struct AssignedDevice {
     void *msix_table_page;
     target_phys_addr_t msix_table_addr;
     int mmio_index;
-    int need_emulate_cmd;
+    uint32_t emulate_cmd_mask;
     char *configfd_name;
     QLIST_ENTRY(AssignedDevice) next;
 } AssignedDevice;

From: Jan Kiszka <[email protected]>

Enable the new KVM feature that allows legacy interrupt sharing for PCI-2.3-compliant devices. This requires to synchronize any guest change of the INTx mask bit to the kernel.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/device-assignment.c |   38 +++++++++++++++++++++++++++++++++-----
 qemu-kvm.c             |    8 ++++++++
 qemu-kvm.h             |    3 +++
 3 files changed, 44 insertions(+), 5 deletions(-)

hw/device-assignment.c +9 -0

index 26d3bd7..cf75c52 100644
@@ -423,12 +423,21 @@ static uint8_t pci_find_cap_offset(PCIDevice *d, uint8_t cap, uint8_t start)
     return 0;
 }
 
+static uint32_t calc_assigned_dev_id(uint16_t seg, uint8_t bus, uint8_t devfn)
+{
+    return (uint32_t)seg << 16 | (uint32_t)bus << 8 | (uint32_t)devfn;
+}
+
 static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
                                           uint32_t val, int len)
 {
     int fd;
     ssize_t ret;
     AssignedDevice *pci_dev = container_of(d, AssignedDevice, dev);
+    struct kvm_assigned_pci_dev assigned_dev_data;
+#ifdef KVM_CAP_PCI_2_3
+    bool intx_masked, update_intx_mask;
+#endif /* KVM_CAP_PCI_2_3 */
 
     DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
           ((d->devfn >> 3) & 0x1F), (d->devfn & 0x7),

@@ -439,6 +448,26 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
     }
 

hw/device-assignment.c +15 -5

+        if (update_intx_mask) {
+            memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));
+            assigned_dev_data.assigned_dev_id  =
+                calc_assigned_dev_id(pci_dev->h_segnr, pci_dev->h_busnr,
+                                     pci_dev->h_devfn);
+            if (intx_masked) {
+                assigned_dev_data.flags = KVM_DEV_ASSIGN_MASK_INTX;
+            }
+            kvm_assign_set_intx_mask(kvm_context, &assigned_dev_data);
+        }
+#endif /* KVM_CAP_PCI_2_3 */
         pci_default_write_config(d, address, val, len);
         /* Continue to program the card */
     }

@@ -876,11 +905,6 @@ static void free_assigned_device(AssignedDevice *dev)
     }
 }
 
-static uint32_t calc_assigned_dev_id(uint16_t seg, uint8_t bus, uint8_t devfn)
-{
-    return (uint32_t)seg << 16 | (uint32_t)bus << 8 | (uint32_t)devfn;
-}
-
 static void assign_failed_examine(AssignedDevice *dev)
 {
     char name[PATH_MAX], dir[PATH_MAX], driver[PATH_MAX] = {}, *ns;

@@ -971,6 +995,10 @@ static int assign_device(AssignedDevice *dev)
                 "cause host memory corruption if the device issues DMA write "
                 "requests!\n");
     }
+#ifdef KVM_CAP_PCI_2_3
+    assigned_dev_data.flags |= KVM_DEV_ASSIGN_PCI_2_3;
+    dev->emulate_cmd_mask |= PCI_COMMAND_INTX_DISABLE;
+#endif /* KVM_CAP_PCI_2_3 */
 
     r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
     if (r < 0) {

qemu-kvm.c +8 -0

index 471306b..8157b4f 100644
@@ -740,6 +740,14 @@ int kvm_deassign_pci_device(kvm_context_t kvm,
 }
 #endif
 
+#ifdef KVM_CAP_PCI_2_3
+int kvm_assign_set_intx_mask(kvm_context_t kvm,
+                             struct kvm_assigned_pci_dev *assigned_dev)
+{
+    return kvm_vm_ioctl(kvm_state, KVM_ASSIGN_SET_INTX_MASK, assigned_dev);
+}
+#endif
+
 int kvm_reinject_control(kvm_context_t kvm, int pit_reinject)
 {
 #ifdef KVM_CAP_REINJECT_CONTROL

qemu-kvm.h +3 -0

index 7e6edfb..522b1b2 100644
@@ -602,6 +602,9 @@ int kvm_assign_set_msix_entry(kvm_context_t kvm,
                               struct kvm_assigned_msix_entry *entry);
 #endif
 
+int kvm_assign_set_intx_mask(kvm_context_t kvm,
+                             struct kvm_assigned_pci_dev *assigned_dev);
+
 #else                           /* !CONFIG_KVM */
 
 typedef struct kvm_context *kvm_context_t;

Quoted 42 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

Enable the new KVM feature that allows legacy interrupt sharing for PCI-2.3-compliant devices. This requires to synchronize any guest change of the INTx mask bit to the kernel.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/device-assignment.c |   38 +++++++++++++++++++++++++++++++++-----
 qemu-kvm.c             |    8 ++++++++
 qemu-kvm.h             |    3 +++
 3 files changed, 44 insertions(+), 5 deletions(-)

hw/device-assignment.c +9 -0

index 26d3bd7..cf75c52 100644
@@ -423,12 +423,21 @@ static uint8_t pci_find_cap_offset(PCIDevice *d, uint8_t cap, uint8_t start)
     return 0;
 }
 
+static uint32_t calc_assigned_dev_id(uint16_t seg, uint8_t bus, uint8_t devfn)
+{
+    return (uint32_t)seg << 16 | (uint32_t)bus << 8 | (uint32_t)devfn;
+}
+
 static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
                                           uint32_t val, int len)
 {
     int fd;
     ssize_t ret;
     AssignedDevice *pci_dev = container_of(d, AssignedDevice, dev);
+    struct kvm_assigned_pci_dev assigned_dev_data;
+#ifdef KVM_CAP_PCI_2_3
+    bool intx_masked, update_intx_mask;
+#endif /* KVM_CAP_PCI_2_3 */
 
     DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
           ((d->devfn >> 3) & 0x1F), (d->devfn & 0x7),

@@ -439,6 +448,26 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
     }
 

hw/device-assignment.c static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, · line 459

Quoted 10 lines

hw/device-assignment.c +9 -0

     if (ranges_overlap(address, len, PCI_COMMAND, 2)) {
+#ifdef KVM_CAP_PCI_2_3
+        update_intx_mask = false;
+        if (address == PCI_COMMAND+1) {
+            intx_masked = val & (PCI_COMMAND_INTX_DISABLE >> 8);
+            update_intx_mask = true;
+        } else if (len >= 2) {
+            intx_masked = val & PCI_COMMAND_INTX_DISABLE;
+            update_intx_mask = true;
+        }

I wonder if this might be a little cleaner as something like this.

    if (ranges_overlap(address, len, PCI_COMMAND + 1, 1) {
        update_intx_mask = true;
        intx_masked = (len == 1 ? val << 8 : val) & PCI_COMMAND_INTX_DISABLE;
    }