[PATCH 0/6] pci-assign: Multiple fixes and cleanups

[PATCH 0/6] pci-assign: Multiple fixes and cleanups

[Alex Williamson]

These patches are all independent.  Patch 1 & 2 fix serious
usability bugs.  Patches 3-6 are more subtle things that
Markus was able to find with Coverity.

Patch 1 fixes https://bugs.launchpad.net/qemu/+bug/875723

I also tested https://bugs.launchpad.net/qemu/+bug/877155
but I'm unable to reproduce.  An 82576 VF works just fine
in a Windows 2008 guest with this patch series.  Thanks,

Alex

---

Alex Williamson (6):
      pci-assign: Harden I/O port test
      pci-assign: Remove bogus PCIe lnkcap wmask setting
      pci-assign: Fix PCIe lnkcap
      pci-assign: Fix PCI_EXP_FLAGS_TYPE shift
      pci-assign: Fix I/O port
      pci-assign: Fix device removal


 hw/device-assignment.c |  137 ++++++++++++++++++++----------------------------
 1 files changed, 57 insertions(+), 80 deletions(-)

[PATCH 1/6] pci-assign: Fix device removal

[Alex Williamson]

We're destroying the memory container before we remove the
subregions it holds.  This fixes:

https://bugs.launchpad.net/qemu/+bug/875723

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

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

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 11efd16..cde0681 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -677,10 +677,23 @@ static void free_assigned_device(AssignedDevice *dev)
                 kvm_remove_ioport_region(region->u.r_baseport, region->r_size,
                                          dev->dev.qdev.hotplugged);
             }
+            memory_region_del_subregion(&region->container,
+                                        &region->real_iomem);
+            memory_region_destroy(&region->real_iomem);
+            memory_region_destroy(&region->container);
         } else if (pci_region->type & IORESOURCE_MEM) {
             if (region->u.r_virtbase) {
                 memory_region_del_subregion(&region->container,
                                             &region->real_iomem);
+
+                /* Remove MSI-X table subregion */
+                if (pci_region->base_addr <= dev->msix_table_addr &&
+                    pci_region->base_addr + pci_region->size >
+                    dev->msix_table_addr) {
+                    memory_region_del_subregion(&region->container,
+                                                &dev->mmio);
+                }
+
                 memory_region_destroy(&region->real_iomem);
                 memory_region_destroy(&region->container);
                 if (munmap(region->u.r_virtbase,

[PATCH 2/6] pci-assign: Fix I/O port

[Alex Williamson]

The old_portio structure seems broken.  Throw it away and
switch to the new style.  This was hitting an assert when
trying to make use of I/O port regions.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/device-assignment.c |  103 ++++++++++++++++--------------------------------
 1 files changed, 35 insertions(+), 68 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index cde0681..571a097 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -65,100 +65,76 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev);
 
 static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
 
-static uint32_t assigned_dev_ioport_rw(AssignedDevRegion *dev_region,
-                                       uint32_t addr, int len, uint32_t *val)
+static uint64_t assigned_dev_ioport_rw(AssignedDevRegion *dev_region,
+                                       target_phys_addr_t addr, int size,
+                                       uint64_t *data)
 {
-    uint32_t ret = 0;
-    uint32_t offset = addr;
+    uint64_t val = 0;
     int fd = dev_region->region->resource_fd;
 
     if (fd >= 0) {
-        if (val) {
-            DEBUG("pwrite val=%x, len=%d, e_phys=%x, offset=%x\n",
-                  *val, len, addr, offset);
-            if (pwrite(fd, val, len, offset) != len) {
+        if (data) {
+            DEBUG("pwrite data=%x, size=%d, e_phys=%x, addr=%x\n",
+                  *data, size, addr, addr);
+            if (pwrite(fd, data, size, addr) != size) {
                 fprintf(stderr, "%s - pwrite failed %s\n",
                         __func__, strerror(errno));
             }
         } else {
-            if (pread(fd, &ret, len, offset) != len) {
+            if (pread(fd, &val, size, addr) != size) {
                 fprintf(stderr, "%s - pread failed %s\n",
                         __func__, strerror(errno));
-                ret = (1UL << (len * 8)) - 1;
+                val = (1UL << (size * 8)) - 1;
             }
-            DEBUG("pread ret=%x, len=%d, e_phys=%x, offset=%x\n",
-                  ret, len, addr, offset);
+            DEBUG("pread val=%x, size=%d, e_phys=%x, addr=%x\n",
+                  val, size, addr, addr);
         }
     } else {
-        uint32_t port = offset + dev_region->u.r_baseport;
+        uint32_t port = addr + dev_region->u.r_baseport;
 
-        if (val) {
-            DEBUG("out val=%x, len=%d, e_phys=%x, host=%x\n",
-                  *val, len, addr, port);
-            switch (len) {
+        if (data) {
+            DEBUG("out data=%x, size=%d, e_phys=%x, host=%x\n",
+                  *data, size, addr, port);
+            switch (size) {
                 case 1:
-                    outb(*val, port);
+                    outb(*data, port);
                     break;
                 case 2:
-                    outw(*val, port);
+                    outw(*data, port);
                     break;
                 case 4:
-                    outl(*val, port);
+                    outl(*data, port);
                     break;
             }
         } else {
-            switch (len) {
+            switch (size) {
                 case 1:
-                    ret = inb(port);
+                    val = inb(port);
                     break;
                 case 2:
-                    ret = inw(port);
+                    val = inw(port);
                     break;
                 case 4:
-                    ret = inl(port);
+                    val = inl(port);
                     break;
             }
-            DEBUG("in val=%x, len=%d, e_phys=%x, host=%x\n",
-                  ret, len, addr, port);
+            DEBUG("in data=%x, size=%d, e_phys=%x, host=%x\n",
+                  val, size, addr, port);
         }
     }
-    return ret;
-}
-
-static void assigned_dev_ioport_writeb(void *opaque, uint32_t addr,
-                                       uint32_t value)
-{
-    assigned_dev_ioport_rw(opaque, addr, 1, &value);
-    return;
-}
-
-static void assigned_dev_ioport_writew(void *opaque, uint32_t addr,
-                                       uint32_t value)
-{
-    assigned_dev_ioport_rw(opaque, addr, 2, &value);
-    return;
-}
-
-static void assigned_dev_ioport_writel(void *opaque, uint32_t addr,
-                       uint32_t value)
-{
-    assigned_dev_ioport_rw(opaque, addr, 4, &value);
-    return;
-}
-
-static uint32_t assigned_dev_ioport_readb(void *opaque, uint32_t addr)
-{
-    return assigned_dev_ioport_rw(opaque, addr, 1, NULL);
+    return val;
 }
 
-static uint32_t assigned_dev_ioport_readw(void *opaque, uint32_t addr)
+static void assigned_dev_ioport_write(void *opaque, target_phys_addr_t addr,
+                                      uint64_t data, unsigned size)
 {
-    return assigned_dev_ioport_rw(opaque, addr, 2, NULL);
+    assigned_dev_ioport_rw(opaque, addr, size, &data);
 }
 
-static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr)
+static uint64_t assigned_dev_ioport_read(void *opaque,
+                                         target_phys_addr_t addr, unsigned size)
 {
-    return assigned_dev_ioport_rw(opaque, addr, 4, NULL);
+    return assigned_dev_ioport_rw(opaque, addr, size, NULL);
 }
 
 static uint32_t slow_bar_readb(void *opaque, target_phys_addr_t addr)
@@ -258,18 +234,9 @@ static void assigned_dev_iomem_setup(PCIDevice *pci_dev, int region_num,
     }
 }
 
-static const MemoryRegionPortio assigned_dev_old_portio[] = {
-    { 0x10000, 1, .read = assigned_dev_ioport_readb, },
-    { 0x10000, 2, .read = assigned_dev_ioport_readw, },
-    { 0x10000, 4, .read = assigned_dev_ioport_readl, },
-    { 0x10000, 1, .write = assigned_dev_ioport_writeb, },
-    { 0x10000, 2, .write = assigned_dev_ioport_writew, },
-    { 0x10000, 4, .write = assigned_dev_ioport_writel, },
-    PORTIO_END_OF_LIST()
-};
-
 static const MemoryRegionOps assigned_dev_ioport_ops = {
-    .old_portio = assigned_dev_old_portio,
+    .read = assigned_dev_ioport_read,
+    .write = assigned_dev_ioport_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 

[PATCH 3/6] pci-assign: Fix PCI_EXP_FLAGS_TYPE shift

[Alex Williamson]

Coverity found that we're doing (uint16_t)type & 0xf0 >> 8.
This is obviously always 0x0, so our attempt to filter out
some device types thinks everything is an endpoint.  Fix
shift amount.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

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

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 571a097..ec302d2 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1294,7 +1294,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
         assigned_dev_setup_cap_read(dev, pos, size);
 
         type = pci_get_word(pci_dev->config + pos + PCI_EXP_FLAGS);
-        type = (type & PCI_EXP_FLAGS_TYPE) >> 8;
+        type = (type & PCI_EXP_FLAGS_TYPE) >> 4;
         if (type != PCI_EXP_TYPE_ENDPOINT &&
             type != PCI_EXP_TYPE_LEG_END && type != PCI_EXP_TYPE_RC_END) {
             fprintf(stderr,

[PATCH 4/6] pci-assign: Fix PCIe lnkcap

[Alex Williamson]

Another Coverity found issue, lnkcap is a 32bit register and
we're masking bits 16 & 17.  Fix to uin32_t.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

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

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index ec302d2..dd92ce0 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1240,8 +1240,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
 
     if ((pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_EXP, 0))) {
         uint8_t version, size = 0;
-        uint16_t type, devctl, lnkcap, lnksta;
-        uint32_t devcap;
+        uint16_t type, devctl, lnksta;
+        uint32_t devcap, lnkcap;
 
         version = pci_get_byte(pci_dev->config + pos + PCI_EXP_FLAGS);
         version &= PCI_EXP_FLAGS_VERS;
@@ -1326,11 +1326,11 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
         pci_set_word(pci_dev->config + pos + PCI_EXP_DEVSTA, 0);
 
         /* Link capabilities, expose links and latencues, clear reporting */
-        lnkcap = pci_get_word(pci_dev->config + pos + PCI_EXP_LNKCAP);
+        lnkcap = pci_get_long(pci_dev->config + pos + PCI_EXP_LNKCAP);
         lnkcap &= (PCI_EXP_LNKCAP_SLS | PCI_EXP_LNKCAP_MLW |
                    PCI_EXP_LNKCAP_ASPMS | PCI_EXP_LNKCAP_L0SEL |
                    PCI_EXP_LNKCAP_L1EL);
-        pci_set_word(pci_dev->config + pos + PCI_EXP_LNKCAP, lnkcap);
+        pci_set_long(pci_dev->config + pos + PCI_EXP_LNKCAP, lnkcap);
         pci_set_word(pci_dev->wmask + pos + PCI_EXP_LNKCAP,
                      PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_RCB |
                      PCI_EXP_LNKCTL_CCC | PCI_EXP_LNKCTL_ES |

[PATCH 5/6] pci-assign: Remove bogus PCIe lnkcap wmask setting

[Alex Williamson]

All the fields of lnkcap are read-only and this is setting it
with mask values from LNKCTL.  Just below it, we indicate
link control is read only, so this appears to be a stray
chunk left in from development.  Trivial comment fix while
we're here.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

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

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index dd92ce0..0160de7 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1312,7 +1312,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
         pci_set_long(pci_dev->config + pos + PCI_EXP_DEVCAP, devcap);
 
         /* device control: clear all error reporting enable bits, leaving
-         *                 leaving only a few host values.  Note, these are
+         *                 only a few host values.  Note, these are
          *                 all writable, but not passed to hw.
          */
         devctl = pci_get_word(pci_dev->config + pos + PCI_EXP_DEVCTL);
@@ -1331,10 +1331,6 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
                    PCI_EXP_LNKCAP_ASPMS | PCI_EXP_LNKCAP_L0SEL |
                    PCI_EXP_LNKCAP_L1EL);
         pci_set_long(pci_dev->config + pos + PCI_EXP_LNKCAP, lnkcap);
-        pci_set_word(pci_dev->wmask + pos + PCI_EXP_LNKCAP,
-                     PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_RCB |
-                     PCI_EXP_LNKCTL_CCC | PCI_EXP_LNKCTL_ES |
-                     PCI_EXP_LNKCTL_CLKREQ_EN | PCI_EXP_LNKCTL_HAWD);
 
         /* Link control, pass existing read-only copy.  Should be writable? */
 

[PATCH 6/6] pci-assign: Harden I/O port test

[Alex Williamson]

Markus Armbruster points out that we're missing a < 0 check
from pread while trying to probe for pci-sysfs io-port
resource support.  We don't expect a short read, but we
should harden the test to abort if we get one so we're not
potentially looking at a stale errno.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

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

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 0160de7..7e6f972 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -434,8 +434,9 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
              * kernels return EIO.  New kernels only allow 1/2/4 byte reads
              * so should return EINVAL for a 3 byte read */
             ret = pread(pci_dev->v_addrs[i].region->resource_fd, &val, 3, 0);
-            if (ret == 3) {
-                fprintf(stderr, "I/O port resource supports 3 byte read?!\n");
+            if (ret >= 0) {
+                fprintf(stderr, "Unexpected return from I/O port read: %d\n",
+                        ret);
                 abort();
             } else if (errno != EINVAL) {
                 fprintf(stderr, "Using raw in/out ioport access (sysfs - %s)\n",

Re: [PATCH 0/6] pci-assign: Multiple fixes and cleanups

[Avi Kivity]

On 11/16/2011 10:45 PM, Alex Williamson wrote:
> These patches are all independent.  Patch 1 & 2 fix serious
> usability bugs.  Patches 3-6 are more subtle things that
> Markus was able to find with Coverity.
>
> Patch 1 fixes https://bugs.launchpad.net/qemu/+bug/875723
>
> I also tested https://bugs.launchpad.net/qemu/+bug/877155
> but I'm unable to reproduce.  An 82576 VF works just fine
> in a Windows 2008 guest with this patch series.  Thanks,
>

Applied, thanks.

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