[PATCH 0/5] pci-assign: Host IRQ sharing suppport + some fixes and cleanups

[PATCH 0/5] pci-assign: Host IRQ sharing suppport + some fixes and cleanups

[Jan Kiszka]

This series includes cleanups of the PCI config access of assigned
devices, fixes a corner case in this area, removes that suspicious VGA
hunk from assigned_dev_pci_read_config, and finally enables support for
the latest host IRQ sharing support via PCI-2.3 interrupt masking. See
the patches for details.

Jan Kiszka (5):
  pci-assign: Clean up assigned_dev_pci_read/write_config
  pci-assign: Fix dword read at PCI_COMMAND
  pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config
  pci-assign: Convert need_emulate_cmd into a bitmask
  pci-assign: Use PCI-2.3-based shared legacy interrupts

 hw/device-assignment.c |  100 ++++++++++++++++++++++++++++++++++++-----------
 hw/device-assignment.h |    2 +-
 qemu-kvm.c             |    8 ++++
 qemu-kvm.h             |    3 +
 4 files changed, 88 insertions(+), 25 deletions(-)

[PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 50c6408..bc3a57b 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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;
-- 
1.7.1

[PATCH 2/5] pci-assign: Fix dword read at PCI_COMMAND

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 hw/device-assignment.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index bc3a57b..6ff1456 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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;
+            val |= pci_default_read_config(d, PCI_COMMAND, 2);
+        } else {
+            /* high-byte access */
+            val = pci_default_read_config(d, PCI_COMMAND+1, 1);
+        }
+    }
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {
-- 
1.7.1

[PATCH 3/5] pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 hw/device-assignment.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 6ff1456..ef045f4 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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);
 
-- 
1.7.1

[PATCH 4/5] pci-assign: Convert need_emulate_cmd into a bitmask

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 hw/device-assignment.c |   18 ++++++++++--------
 hw/device-assignment.h |    2 +-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index ef045f4..26d3bd7 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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;
+            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);
         }
     }
 
@@ -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;
diff --git a/hw/device-assignment.h b/hw/device-assignment.h
index c94a730..9ead022 100644
--- a/hw/device-assignment.h
+++ b/hw/device-assignment.h
@@ -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;
-- 
1.7.1

[PATCH 5/5] pci-assign: Use PCI-2.3-based shared legacy interrupts

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 hw/device-assignment.c |   38 +++++++++++++++++++++++++++++++++-----
 qemu-kvm.c             |    8 ++++++++
 qemu-kvm.h             |    3 +++
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 26d3bd7..cf75c52 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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,
     }
 
     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;
+        }
+        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) {
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 471306b..8157b4f 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -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
diff --git a/qemu-kvm.h b/qemu-kvm.h
index 7e6edfb..522b1b2 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -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;
-- 
1.7.1

Re: [PATCH 2/5] pci-assign: Fix dword read at PCI_COMMAND

[Alex Williamson]

On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
>  hw/device-assignment.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index bc3a57b..6ff1456 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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;
> +            val |= pci_default_read_config(d, PCI_COMMAND, 2);
> +        } else {
> +            /* high-byte access */
> +            val = pci_default_read_config(d, PCI_COMMAND+1, 1);
> +        }
> +    }
> +
>      if (!pci_dev->cap.available) {
>          /* kill the special capabilities */
>          if (address == PCI_COMMAND && len == 4) {

We might be able to use the merge_bits function that I just added for
capability support, perhaps something like:

if (pci_dev->need_emulate_cmd) {
    val = merge_bits(val, pci_default_read_config(d, address, len), PCI_COMMAND, 0xffff)
}

Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Alex Williamson]

On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
>  hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
>  1 files changed, 29 insertions(+), 10 deletions(-)

A long overdue cleanup, looks good.

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

> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 50c6408..bc3a57b 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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;

Re: [PATCH 3/5] pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config

[Alex Williamson]

On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
>  hw/device-assignment.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)

Yay!

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

> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 6ff1456..ef045f4 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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);
>  

Re: [PATCH 4/5] pci-assign: Convert need_emulate_cmd into a bitmask

[Alex Williamson]

On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
>  hw/device-assignment.c |   18 ++++++++++--------
>  hw/device-assignment.h |    2 +-
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index ef045f4..26d3bd7 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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;
> +            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);

> @@ -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;
> diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> index c94a730..9ead022 100644
> --- a/hw/device-assignment.h
> +++ b/hw/device-assignment.h
> @@ -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;

Re: [PATCH 5/5] pci-assign: Use PCI-2.3-based shared legacy interrupts

[Alex Williamson]

On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
>  hw/device-assignment.c |   38 +++++++++++++++++++++++++++++++++-----
>  qemu-kvm.c             |    8 ++++++++
>  qemu-kvm.h             |    3 +++
>  3 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 26d3bd7..cf75c52 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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,
>      }
>  
>      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;
    }

> +        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) {
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 471306b..8157b4f 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -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
> diff --git a/qemu-kvm.h b/qemu-kvm.h
> index 7e6edfb..522b1b2 100644
> --- a/qemu-kvm.h
> +++ b/qemu-kvm.h
> @@ -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;

Re: [PATCH 5/5] pci-assign: Use PCI-2.3-based shared legacy interrupts

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2601 bytes --]

Am 14.12.2010 01:16, Alex Williamson wrote:
> On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 <jan.kiszka@siemens.com>
>> ---
>>  hw/device-assignment.c |   38 +++++++++++++++++++++++++++++++++-----
>>  qemu-kvm.c             |    8 ++++++++
>>  qemu-kvm.h             |    3 +++
>>  3 files changed, 44 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index 26d3bd7..cf75c52 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -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,
>>      }
>>  
>>      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;
>     }

That should even obsolete update_intx_mask - will look into this, and
also the merge bits thing.

Thanks!
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH 4/5] pci-assign: Convert need_emulate_cmd into a bitmask

[Michael S. Tsirkin]

On Mon, Dec 13, 2010 at 04:56:29PM -0700, Alex Williamson wrote:
> On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > 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 <jan.kiszka@siemens.com>
> > ---
> >  hw/device-assignment.c |   18 ++++++++++--------
> >  hw/device-assignment.h |    2 +-
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index ef045f4..26d3bd7 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -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;
> > +            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);


Not a comment on this patch at all, rather on
assigned-devices support code generally:

	I think code will become cleaner if we add a mask array mapping the
	config space, along the lines of wmask and friends in pci.c

	I think this will make it possible to mostly get rid of
	merge_bits and tricky range check hacks.


> > @@ -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;
> > diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> > index c94a730..9ead022 100644
> > --- a/hw/device-assignment.h
> > +++ b/hw/device-assignment.h
> > @@ -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;
> 
> 

[PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Jan Kiszka]

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 <jan.kiszka@siemens.com>
---
 hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 606d725..3481c93 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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, 8) ||
+        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, 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);
@@ -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;
-- 
1.7.1

Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Alex Williamson]

On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
> Use rages_overlap and proper constants to match the access range against
      ^^^^^ typo - only if you resend

> 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 <jan.kiszka@siemens.com>
> ---
>  hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
>  1 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 606d725..3481c93 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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, 8) ||

Should this be 5 bytes instead of 8?  I'm not sure why we'd add catching
these reserved fields, but not those immediately after this range.

> +        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, 8) ||

Same here.

> +        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;

Thanks,

Alex

Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Jan Kiszka]

On 2011-04-28 16:29, Alex Williamson wrote:
> On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
>> Use rages_overlap and proper constants to match the access range against
>       ^^^^^ typo - only if you resend
> 
>> 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 <jan.kiszka@siemens.com>
>> ---
>>  hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
>>  1 files changed, 29 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index 606d725..3481c93 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -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, 8) ||
> 
> Should this be 5 bytes instead of 8?  I'm not sure why we'd add catching
> these reserved fields, but not those immediately after this range.

Yes, that's asking for clarification: Should we allow direct access to
the complete reserved space or virtualize it? Depending on this, the
proper value should be 5 or 14 (the latter would also save one
ranges_overlap).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Alex Williamson]

On Thu, 2011-04-28 at 16:46 +0200, Jan Kiszka wrote:
> On 2011-04-28 16:29, Alex Williamson wrote:
> > On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
> >> Use rages_overlap and proper constants to match the access range against
> >       ^^^^^ typo - only if you resend
> > 
> >> 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 <jan.kiszka@siemens.com>
> >> ---
> >>  hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
> >>  1 files changed, 29 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> >> index 606d725..3481c93 100644
> >> --- a/hw/device-assignment.c
> >> +++ b/hw/device-assignment.c
> >> @@ -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, 8) ||
> > 
> > Should this be 5 bytes instead of 8?  I'm not sure why we'd add catching
> > these reserved fields, but not those immediately after this range.
> 
> Yes, that's asking for clarification: Should we allow direct access to
> the complete reserved space or virtualize it? Depending on this, the
> proper value should be 5 or 14 (the latter would also save one
> ranges_overlap).

I vote for 5 here since a cleanup patch shouldn't have behavior changes
hidden in it.  I don't see any great value in virtualizing reserved
bits.  It seems like it could only make things not work if a vendor was
stupid enough to hide something in there.  Thanks,

Alex

Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Jan Kiszka]

On 2011-04-28 16:54, Alex Williamson wrote:
> On Thu, 2011-04-28 at 16:46 +0200, Jan Kiszka wrote:
>> On 2011-04-28 16:29, Alex Williamson wrote:
>>> On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
>>>> Use rages_overlap and proper constants to match the access range against
>>>       ^^^^^ typo - only if you resend
>>>
>>>> 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 <jan.kiszka@siemens.com>
>>>> ---
>>>>  hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
>>>>  1 files changed, 29 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>>>> index 606d725..3481c93 100644
>>>> --- a/hw/device-assignment.c
>>>> +++ b/hw/device-assignment.c
>>>> @@ -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, 8) ||
>>>
>>> Should this be 5 bytes instead of 8?  I'm not sure why we'd add catching
>>> these reserved fields, but not those immediately after this range.
>>
>> Yes, that's asking for clarification: Should we allow direct access to
>> the complete reserved space or virtualize it? Depending on this, the
>> proper value should be 5 or 14 (the latter would also save one
>> ranges_overlap).
> 
> I vote for 5 here since a cleanup patch shouldn't have behavior changes
> hidden in it.  I don't see any great value in virtualizing reserved
> bits.  It seems like it could only make things not work if a vendor was
> stupid enough to hide something in there.  Thanks,

Yeah, and as we properly restore the config space now, it should be
Mostly Harmless. Will update.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config

[Jan Kiszka]

On 2011-04-28 16:54, Alex Williamson wrote:
> On Thu, 2011-04-28 at 16:46 +0200, Jan Kiszka wrote:
>> On 2011-04-28 16:29, Alex Williamson wrote:
>>> On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
>>>> Use rages_overlap and proper constants to match the access range against
>>>       ^^^^^ typo - only if you resend
>>>
>>>> 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 <jan.kiszka@siemens.com>
>>>> ---
>>>>  hw/device-assignment.c |   39 +++++++++++++++++++++++++++++----------
>>>>  1 files changed, 29 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>>>> index 606d725..3481c93 100644
>>>> --- a/hw/device-assignment.c
>>>> +++ b/hw/device-assignment.c
>>>> @@ -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, 8) ||
>>>
>>> Should this be 5 bytes instead of 8?  I'm not sure why we'd add catching
>>> these reserved fields, but not those immediately after this range.
>>
>> Yes, that's asking for clarification: Should we allow direct access to
>> the complete reserved space or virtualize it? Depending on this, the
>> proper value should be 5 or 14 (the latter would also save one
>> ranges_overlap).
> 
> I vote for 5 here since a cleanup patch shouldn't have behavior changes
> hidden in it.  I don't see any great value in virtualizing reserved
> bits.  It seems like it could only make things not work if a vendor was
> stupid enough to hide something in there.  Thanks,

Thinking about this and the other sub-dword matches again, there is a
problem in my approach:

I redirect the complete write to the virtualized space once there is an
overlap. So a dword write at PCI_INTERRUPT_LINE would not touch the real
PCI_MIN_GNT & PCI_MAX_LAT. But writing directly to those without overlap
would. The same applies to our reserved space, though this inconsistency
is not critical here - but still undesirable.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux