qemu-kvm: pci-assign: Some more cleanups

PATCH Applied Jan Kiszka 3 participants 12 messages Apr 29, 2011 – May 3, 2011 KVM

Share & tools

Jan Kiszka

author

2011-04-29 09:05 UTC #0 lore raw

v2 cover qemu-kvm: pci-assign: Some more cleanups

This round addresses the review comments and also add another fix to ensure that non-virtualized config areas always hit the real device.

Jan Kiszka (6):
pci-assign: Clean up assigned_dev_pci_read/write_config
pci-assign: Move merge_bits
pci-assign: Fix dword read at PCI_COMMAND
pci-assign: Properly handle more overlapping accesses
pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config
pci-assign: Convert need_emulate_cmd into a bitmask

 hw/device-assignment.c |  125 +++++++++++++++++++++++++++++++----------------
 hw/device-assignment.h |    2 +-
 2 files changed, 83 insertions(+), 44 deletions(-)

Jan Kiszka

author

2011-04-29 09:05 UTC #1 lore raw

PATCH v2 1/6 pci-assign: Clean up assigned_dev_pci_read/write_config

Use ranges_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 606d725..0bf93b1 100644
@@ -404,13 +404,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, 5) ||
+        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
         /* used for update-mappings (BAR emulation) */
         pci_default_write_config(d, address, val, len);
         return;

@@ -450,9 +457,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, 5) ||
+        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);

@@ -483,10 +501,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;

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-04-29 09:05 UTC #2 lore raw

PATCH v2 2/6 pci-assign: Move merge_bits

In reply to Jan Kiszka

We will need it earlier in the file, so move it unmodified to the top.

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

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

hw/device-assignment.c +22 ~~-22~~

index 0bf93b1..ea1d7f1 100644
@@ -71,6 +71,28 @@ static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev,
 static uint32_t assigned_device_pci_cap_read_config(PCIDevice *pci_dev,
                                                     uint32_t address, int len);
 
+/* Merge the bits set in mask from mval into val.  Both val and mval are
+ * at the same addr offset, pos is the starting offset of the mask. */
+static uint32_t merge_bits(uint32_t val, uint32_t mval, uint8_t addr,
+                           int len, uint8_t pos, uint32_t mask)
+{
+    if (!ranges_overlap(addr, len, pos, 4)) {
+        return val;
+    }
+
+    if (addr >= pos) {
+        mask >>= (addr - pos) * 8;
+    } else {
+        mask <<= (pos - addr) * 8;
+    }
+    mask &= 0xffffffffU >> (4 - len) * 8;
+
+    val &= ~mask;
+    val |= (mval & mask);
+
+    return val;
+}
+
 static uint32_t assigned_dev_ioport_rw(AssignedDevRegion *dev_region,
                                        uint32_t addr, int len, uint32_t *val)
 {

@@ -1278,28 +1300,6 @@ static uint8_t find_vndr_start(PCIDevice *pci_dev, uint32_t address)
     return cap;
 }
 
-/* Merge the bits set in mask from mval into val.  Both val and mval are
- * at the same addr offset, pos is the starting offset of the mask. */
-static uint32_t merge_bits(uint32_t val, uint32_t mval, uint8_t addr,
-                           int len, uint8_t pos, uint32_t mask)
-{
-    if (!ranges_overlap(addr, len, pos, 4)) {
-        return val;
-    }
-
-    if (addr >= pos) {
-        mask >>= (addr - pos) * 8;
-    } else {
-        mask <<= (pos - addr) * 8;
-    }
-    mask &= 0xffffffffU >> (4 - len) * 8;
-
-    val &= ~mask;
-    val |= (mval & mask);
-
-    return val;
-}
-
 static uint32_t assigned_device_pci_cap_read_config(PCIDevice *pci_dev,
                                                     uint32_t address, int len)
 {

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-04-29 09:05 UTC #3 lore raw

PATCH v2 3/6 pci-assign: Fix dword read at PCI_COMMAND

In reply to Jan Kiszka

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 |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

hw/device-assignment.c +5 -3

index ea1d7f1..cea072e 100644
@@ -482,14 +482,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, 5) ||
         ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {

@@ -521,6 +518,11 @@ 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) {
+        val = merge_bits(val, pci_default_read_config(d, address, len),
+                         address, len, PCI_COMMAND, 0xffff);
+    }
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-04-29 09:05 UTC #4 lore raw

PATCH v2 4/6 pci-assign: Properly handle more overlapping accesses

In reply to Jan Kiszka

Ensure that accesses exceeding PCI_CAPABILITY_LIST and PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not virtualize. Again, we do not optimize these checks and accesses a lot, they are considered to be slow paths.

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

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

hw/device-assignment.c +29 -5

index cea072e..37c77e3 100644
@@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
         ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
         /* used for update-mappings (BAR emulation) */
         pci_default_write_config(d, address, val, len);
-        return;
+
+        /* Ensure that writes to overlapping areas we don't virtualize still
+         * hit the device. */
+        switch (address) {
+        case PCI_CAPABILITY_LIST:
+            if (len > 1) {
+                len -= 1;
+                address += 1;
+                val >>= 8;
+                break; /* continue writing to the device */
+            }
+            return;
+        case PCI_INTERRUPT_LINE:
+            if (len > 2) {
+                len -= 2;
+                address += 2;
+                val >>= 16;
+                break; /* continue writing to the device */
+            }
+            return;
+        default:
+            return;
+        }
     }
 
     DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",

@@ -467,7 +489,7 @@ again:
 static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
                                              int len)
 {
-    uint32_t val = 0;
+    uint32_t val = 0, virt_val;
     int fd;
     ssize_t ret;
     AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);

@@ -484,12 +506,10 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
      *  - vendor & device ID
      *  - base address registers
      *  - ROM base address & capability pointer
-     *  - interrupt line & pin
      */
     if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
         ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         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);

@@ -523,6 +543,10 @@ do_log:
                          address, len, PCI_COMMAND, 0xffff);
     }
 
+    virt_val = pci_default_read_config(d, address, len);
+    val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
+    val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {

Signature · 1 line

1.7.1

Alex Williamson

2011-04-29 20:41 UTC #5 lore raw

Re: [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses

Quoted 33 lines · Jan Kiszka

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

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

hw/device-assignment.c +14 -1

index cea072e..37c77e3 100644
@@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
         ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
         /* used for update-mappings (BAR emulation) */
         pci_default_write_config(d, address, val, len);
-        return;
+
+        /* Ensure that writes to overlapping areas we don't virtualize still
+         * hit the device. */
+        switch (address) {
+        case PCI_CAPABILITY_LIST:
+            if (len > 1) {
+                len -= 1;
+                address += 1;
+                val >>= 8;
+                break; /* continue writing to the device */
+            }
+            return;
+        case PCI_INTERRUPT_LINE:
+            if (len > 2) {

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

Quoted 10 lines

hw/device-assignment.c +9 -0

+                len -= 2;
+                address += 2;
+                val >>= 16;
+                break; /* continue writing to the device */
+            }
+            return;
+        default:
+            return;
+        }
     }

It seems like we could be more symmetric with the below read. Maybe something like:

if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||

    ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
    pci_default_write_config(d, address, val, len);
    return;
} else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
           ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
    uint32_t real_val =  assigned_dev_pci_read(d, address, len);
    pci_default_write_config(d, address, val, len);
    val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff);
    val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
}

We might write out to real hardware when we could avoid it, but I don't think it matters. Thanks,

Alex

Quoted 35 lines

hw/device-assignment.c +6 -4

     DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",

@@ -467,7 +489,7 @@ again:
 static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
                                              int len)
 {
-    uint32_t val = 0;
+    uint32_t val = 0, virt_val;
     int fd;
     ssize_t ret;
     AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);

@@ -484,12 +506,10 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
      *  - vendor & device ID
      *  - base address registers
      *  - ROM base address & capability pointer
-     *  - interrupt line & pin
      */
     if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
         ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         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);

@@ -523,6 +543,10 @@ do_log:
                          address, len, PCI_COMMAND, 0xffff);
     }
 
+    virt_val = pci_default_read_config(d, address, len);
+    val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
+    val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {

Jan Kiszka

author

2011-05-02 10:29 UTC #6 lore raw

PATCH v3 4/6 pci-assign: Properly handle more overlapping accesses

Quoted 60 lines · Alex Williamson

Quoted 33 lines · Jan Kiszka

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

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

hw/device-assignment.c +14 -1

index cea072e..37c77e3 100644
@@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
         ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
         /* used for update-mappings (BAR emulation) */
         pci_default_write_config(d, address, val, len);
-        return;
+
+        /* Ensure that writes to overlapping areas we don't virtualize still
+         * hit the device. */
+        switch (address) {
+        case PCI_CAPABILITY_LIST:
+            if (len > 1) {
+                len -= 1;
+                address += 1;
+                val >>= 8;
+                break; /* continue writing to the device */
+            }
+            return;
+        case PCI_INTERRUPT_LINE:
+            if (len > 2) {

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

Quoted 10 lines

hw/device-assignment.c +9 -0

+                len -= 2;
+                address += 2;
+                val >>= 16;
+                break; /* continue writing to the device */
+            }
+            return;
+        default:
+            return;
+        }
     }

It seems like we could be more symmetric with the below read. Maybe something like:

if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||

    ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
    pci_default_write_config(d, address, val, len);
    return;
} else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
           ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
    uint32_t real_val =  assigned_dev_pci_read(d, address, len);
    pci_default_write_config(d, address, val, len);
    val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff);
    val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
}

Quoted 3 lines

We might write out to real hardware when we could avoid it, but I don't think it matters. Thanks,

Yep, makes sense. Find such a version below.

Thanks,
Jan

------8<-------

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

---
 hw/device-assignment.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

hw/device-assignment.c +26 -8

index cea072e..8e95730 100644
@@ -438,11 +438,22 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
      *  - interrupt line & pin
      */
     if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
-        /* used for update-mappings (BAR emulation) */
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         pci_default_write_config(d, address, val, len);
         return;
+    } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
+               ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        uint32_t real_val;
+
+        pci_default_write_config(d, address, val, len);
+
+        /* Ensure that writes to overlapping areas we don't virtualize still
+         * hit the device. */
+        real_val = assigned_dev_pci_read(d, address, len);
+        val = merge_bits(val, real_val, address, len,
+                         PCI_CAPABILITY_LIST, 0xff);
+        val = merge_bits(val, real_val, address, len,
+                         PCI_INTERRUPT_LINE, 0xffff);
     }
 
     DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",

@@ -467,7 +478,7 @@ again:
 static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
                                              int len)
 {
-    uint32_t val = 0;
+    uint32_t val = 0, virt_val;
     int fd;
     ssize_t ret;
     AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);

@@ -483,13 +494,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
      * Catch access to
      *  - vendor & device ID
      *  - base address registers
-     *  - ROM base address & capability pointer
-     *  - interrupt line & pin
+     *  - ROM base address
      */
     if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
         ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         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);

@@ -523,6 +532,15 @@ do_log:
                          address, len, PCI_COMMAND, 0xffff);
     }
 
+    /*
+     * Merge bits from virtualized
+     *  - capability pointer
+     *  - interrupt line & pin
+     */
+    virt_val = pci_default_read_config(d, address, len);
+    val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
+    val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {

Signature · 1 line

1.7.1

Alex Williamson

Acked-by on v3 4/6

2011-05-02 14:09 UTC #7 lore raw

Re: [PATCH v3 4/6] pci-assign: Properly handle more overlapping accesses

Quoted 71 lines · Jan Kiszka

Quoted 60 lines · Alex Williamson

Quoted 33 lines · Jan Kiszka

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

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

hw/device-assignment.c +14 -1

index cea072e..37c77e3 100644
@@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
         ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
         /* used for update-mappings (BAR emulation) */
         pci_default_write_config(d, address, val, len);
-        return;
+
+        /* Ensure that writes to overlapping areas we don't virtualize still
+         * hit the device. */
+        switch (address) {
+        case PCI_CAPABILITY_LIST:
+            if (len > 1) {
+                len -= 1;
+                address += 1;
+                val >>= 8;
+                break; /* continue writing to the device */
+            }
+            return;
+        case PCI_INTERRUPT_LINE:
+            if (len > 2) {

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

Quoted 10 lines

hw/device-assignment.c +9 -0

+                len -= 2;
+                address += 2;
+                val >>= 16;
+                break; /* continue writing to the device */
+            }
+            return;
+        default:
+            return;
+        }
     }

It seems like we could be more symmetric with the below read. Maybe something like:

if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||

    ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
    pci_default_write_config(d, address, val, len);
    return;
} else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
           ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
    uint32_t real_val =  assigned_dev_pci_read(d, address, len);
    pci_default_write_config(d, address, val, len);
    val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff);
    val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
}

Quoted 3 lines

We might write out to real hardware when we could avoid it, but I don't think it matters. Thanks,

Yep, makes sense. Find such a version below.

Thanks,
Jan

Quoted 8 lines

------8<-------
Ensure that accesses exceeding PCI_CAPABILITY_LIST and PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not virtualize. Again, we do not optimize these checks and accesses a lot, they are considered to be slow paths.
Signed-off-by Jan Kiszka <[email protected]>

Acked-by Alex Williamson <[email protected]>

Quoted 75 lines

---
 hw/device-assignment.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

hw/device-assignment.c +26 -8

index cea072e..8e95730 100644
@@ -438,11 +438,22 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
      *  - interrupt line & pin
      */
     if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
-        /* used for update-mappings (BAR emulation) */
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         pci_default_write_config(d, address, val, len);
         return;
+    } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
+               ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        uint32_t real_val;
+
+        pci_default_write_config(d, address, val, len);
+
+        /* Ensure that writes to overlapping areas we don't virtualize still
+         * hit the device. */
+        real_val = assigned_dev_pci_read(d, address, len);
+        val = merge_bits(val, real_val, address, len,
+                         PCI_CAPABILITY_LIST, 0xff);
+        val = merge_bits(val, real_val, address, len,
+                         PCI_INTERRUPT_LINE, 0xffff);
     }
 
     DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",

@@ -467,7 +478,7 @@ again:
 static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
                                              int len)
 {
-    uint32_t val = 0;
+    uint32_t val = 0, virt_val;
     int fd;
     ssize_t ret;
     AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);

@@ -483,13 +494,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
      * Catch access to
      *  - vendor & device ID
      *  - base address registers
-     *  - ROM base address & capability pointer
-     *  - interrupt line & pin
+     *  - ROM base address
      */
     if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
         ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         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);

@@ -523,6 +532,15 @@ do_log:
                          address, len, PCI_COMMAND, 0xffff);
     }
 
+    /*
+     * Merge bits from virtualized
+     *  - capability pointer
+     *  - interrupt line & pin
+     */
+    virt_val = pci_default_read_config(d, address, len);
+    val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
+    val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {

Jan Kiszka

author

2011-04-29 09:05 UTC #8 lore raw

PATCH v2 5/6 pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config

In reply to Jan Kiszka

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 37c77e3..7db34c4 100644
@@ -516,10 +516,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:

@@ -534,7 +530,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);

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-04-29 09:05 UTC #9 lore raw

PATCH v2 6/6 pci-assign: Convert need_emulate_cmd into a bitmask

In reply to Jan Kiszka

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 |   11 +++++------
 hw/device-assignment.h |    2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

hw/device-assignment.c +5 -6

index 7db34c4..4625527 100644
@@ -533,9 +533,9 @@ 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) {
         val = merge_bits(val, pci_default_read_config(d, address, len),
-                         address, len, PCI_COMMAND, 0xffff);
+                         address, len, PCI_COMMAND, pci_dev->emulate_cmd_mask);
     }
 
     virt_val = pci_default_read_config(d, address, len);

@@ -802,10 +802,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 86af0a9..ae1bc58 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;
     int32_t bootindex;
     QLIST_ENTRY(AssignedDevice) next;

Signature · 1 line

1.7.1

Alex Williamson

Acked-by on the v2 series

2011-05-02 14:11 UTC #10 lore raw

In reply to Jan Kiszka

Quoted 11 lines · Jan Kiszka

This round addresses the review comments and also add another fix to ensure that non-virtualized config areas always hit the real device.
Jan Kiszka (6):
pci-assign: Clean up assigned_dev_pci_read/write_config
pci-assign: Move merge_bits
pci-assign: Fix dword read at PCI_COMMAND
pci-assign: Properly handle more overlapping accesses
pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config
pci-assign: Convert need_emulate_cmd into a bitmask

Quoted 4 lines

 hw/device-assignment.c |  125 +++++++++++++++++++++++++++++++----------------
 hw/device-assignment.h |    2 +-
 2 files changed, 83 insertions(+), 44 deletions(-)

For patches 1-3,5,6:

Acked-by Alex Williamson <[email protected]>

v3 version of 4 acked separately

Marcelo Tosatti

Applied

2011-05-03 19:20 UTC #11 lore raw

In reply to Jan Kiszka

Quoted 10 lines · Jan Kiszka

This round addresses the review comments and also add another fix to ensure that non-virtualized config areas always hit the real device.
Jan Kiszka (6):
pci-assign: Clean up assigned_dev_pci_read/write_config
pci-assign: Move merge_bits
pci-assign: Fix dword read at PCI_COMMAND
pci-assign: Properly handle more overlapping accesses
pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config
pci-assign: Convert need_emulate_cmd into a bitmask

Applied, thanks.