[PATCH 0/5] qemu-kvm: pci-assign: Some more cleanups

[PATCH 0/5] qemu-kvm: pci-assign: Some more cleanups

[Jan Kiszka]

I found these patches in my shared-INTx queue. They do not depend on
that topic (which is on my to do list, not on the top, but close), so I
decided to repost them.



Jan Kiszka (5):
  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: Remove suspicious hunk from assigned_dev_pci_read_config
  pci-assign: Convert need_emulate_cmd into a bitmask

 hw/device-assignment.c |   97 +++++++++++++++++++++++++++--------------------
 hw/device-assignment.h |    2 +-
 2 files changed, 57 insertions(+), 42 deletions(-)

[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

[PATCH 2/5] pci-assign: Move merge_bits

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/device-assignment.c |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 3481c93..f37f108 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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)
 {
-- 
1.7.1

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

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

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index f37f108..ee81434 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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, 8) ||
         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, PCI_COMMAND, 2),
+                         address, len, PCI_COMMAND, 0xffff);
+    }
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {
-- 
1.7.1

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

[Alex Williamson]

On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
> 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 |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index f37f108..ee81434 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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, 8) ||
>          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, PCI_COMMAND, 2),
> +                         address, len, PCI_COMMAND, 0xffff);

Shouldn't this be pci_default_read_config(d, address, len)?  I think
what you have works since PCI_COMMAND is at the start of a dword, but it
violates the merge_bits() assumption that val and mval are from the same
address with the same length.  Thanks,

Alex

> +    }
> +
>      if (!pci_dev->cap.available) {
>          /* kill the special capabilities */
>          if (address == PCI_COMMAND && len == 4) {

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

[Jan Kiszka]

On 2011-04-28 16:51, Alex Williamson wrote:
> On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
>> 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 |    8 +++++---
>>  1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index f37f108..ee81434 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -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, 8) ||
>>          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, PCI_COMMAND, 2),
>> +                         address, len, PCI_COMMAND, 0xffff);
> 
> Shouldn't this be pci_default_read_config(d, address, len)?  I think
> what you have works since PCI_COMMAND is at the start of a dword, but it
> violates the merge_bits() assumption that val and mval are from the same
> address with the same length.  Thanks,

This is actually a real issue, reading a byte from PCI_COMMAND+1 is broken.

Jan

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

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

[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 <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 ee81434..7ff1320 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -496,10 +496,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:
@@ -514,7 +510,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 5/5] pci-assign: Convert need_emulate_cmd into a bitmask

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

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 7ff1320..56b4832 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -513,9 +513,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, PCI_COMMAND, 2),
-                         address, len, PCI_COMMAND, 0xffff);
+                         address, len, PCI_COMMAND, pci_dev->emulate_cmd_mask);
     }
 
     if (!pci_dev->cap.available) {
@@ -778,10 +778,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 86af0a9..ae1bc58 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;
     int32_t bootindex;
     QLIST_ENTRY(AssignedDevice) next;
-- 
1.7.1

Re: [PATCH 0/5] qemu-kvm: pci-assign: Some more cleanups

[Avi Kivity]

On 04/28/2011 11:59 AM, Jan Kiszka wrote:
> I found these patches in my shared-INTx queue. They do not depend on
> that topic (which is on my to do list, not on the top, but close), so I
> decided to repost them.
>

Looks good, will apply after Alex reviews.

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

[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

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;