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

Commit Message

Jan Kiszka May 2, 2011, 10:29 a.m. UTC

On 2011-04-29 22:41, Alex Williamson wrote:
> On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote:
>> Ensure that accesses exceeding PCI_CAPABILITY_LIST and
>> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not
>> virtualize. Again, we do not optimize these checks and accesses a lot,
>> they are considered to be slow paths.
>>
>> Signed-off-by: Jan Kiszka <[e-mail]>
>> ---
>>  hw/device-assignment.c |   34 +++++++++++++++++++++++++++++-----
>>  1 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index cea072e..37c77e3 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
>>          ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
>>          /* used for update-mappings (BAR emulation) */
>>          pci_default_write_config(d, address, val, len);
>> -        return;
>> +
>> +        /* Ensure that writes to overlapping areas we don't virtualize still
>> +         * hit the device. */
>> +        switch (address) {
>> +        case PCI_CAPABILITY_LIST:
>> +            if (len > 1) {
>> +                len -= 1;
>> +                address += 1;
>> +                val >>= 8;
>> +                break; /* continue writing to the device */
>> +            }
>> +            return;
>> +        case PCI_INTERRUPT_LINE:
>> +            if (len > 2) {
>> +                len -= 2;
>> +                address += 2;
>> +                val >>= 16;
>> +                break; /* continue writing to the device */
>> +            }
>> +            return;
>> +        default:
>> +            return;
>> +        }
>>      }
> 
> It seems like we could be more symmetric with the below read.  Maybe
> something like:
> 
> if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
>     ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
>     pci_default_write_config(d, address, val, len);
>     return;
> } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
>            ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
>     uint32_t real_val =  assigned_dev_pci_read(d, address, len);
>     pci_default_write_config(d, address, val, len);
>     val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff);
>     val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
> }
> 
> We might write out to real hardware when we could avoid it, but I don't
> think it matters.  Thanks,
> 

Yep, makes sense. Find such a version below.

Thanks,
Jan


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

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

Signed-off-by: Jan Kiszka <[e-mail]>
---
 hw/device-assignment.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

Comments

Alex Williamson May 2, 2011, 2:09 p.m. UTC | #1

On Mon, 2011-05-02 at 12:29 +0200, Jan Kiszka wrote:
> On 2011-04-29 22:41, Alex Williamson wrote:
> > On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote:
> >> Ensure that accesses exceeding PCI_CAPABILITY_LIST and
> >> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not
> >> virtualize. Again, we do not optimize these checks and accesses a lot,
> >> they are considered to be slow paths.
> >>
> >> Signed-off-by: Jan Kiszka <[e-mail]>
> >> ---
> >>  hw/device-assignment.c |   34 +++++++++++++++++++++++++++++-----
> >>  1 files changed, 29 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> >> index cea072e..37c77e3 100644
> >> --- a/hw/device-assignment.c
> >> +++ b/hw/device-assignment.c
> >> @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
> >>          ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> >>          /* used for update-mappings (BAR emulation) */
> >>          pci_default_write_config(d, address, val, len);
> >> -        return;
> >> +
> >> +        /* Ensure that writes to overlapping areas we don't virtualize still
> >> +         * hit the device. */
> >> +        switch (address) {
> >> +        case PCI_CAPABILITY_LIST:
> >> +            if (len > 1) {
> >> +                len -= 1;
> >> +                address += 1;
> >> +                val >>= 8;
> >> +                break; /* continue writing to the device */
> >> +            }
> >> +            return;
> >> +        case PCI_INTERRUPT_LINE:
> >> +            if (len > 2) {
> >> +                len -= 2;
> >> +                address += 2;
> >> +                val >>= 16;
> >> +                break; /* continue writing to the device */
> >> +            }
> >> +            return;
> >> +        default:
> >> +            return;
> >> +        }
> >>      }
> > 
> > It seems like we could be more symmetric with the below read.  Maybe
> > something like:
> > 
> > if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
> >     ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
> >     pci_default_write_config(d, address, val, len);
> >     return;
> > } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
> >            ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> >     uint32_t real_val =  assigned_dev_pci_read(d, address, len);
> >     pci_default_write_config(d, address, val, len);
> >     val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff);
> >     val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
> > }
> > 
> > We might write out to real hardware when we could avoid it, but I don't
> > think it matters.  Thanks,
> > 
> 
> Yep, makes sense. Find such a version below.
> 
> Thanks,
> Jan
> 
> 
> ------8<-------
> 
> Ensure that accesses exceeding PCI_CAPABILITY_LIST and
> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not
> virtualize. Again, we do not optimize these checks and accesses a lot,
> they are considered to be slow paths.
> 
> Signed-off-by: Jan Kiszka <[e-mail]>

Acked-by: Alex Williamson <[e-mail]>

> ---
>  hw/device-assignment.c |   34 ++++++++++++++++++++++++++--------
>  1 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index cea072e..8e95730 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -438,11 +438,22 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
>       *  - interrupt line & pin
>       */
>      if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
> -        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
> -        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> -        /* used for update-mappings (BAR emulation) */
> +        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
>          pci_default_write_config(d, address, val, len);
>          return;
> +    } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
> +               ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> +        uint32_t real_val;
> +
> +        pci_default_write_config(d, address, val, len);
> +
> +        /* Ensure that writes to overlapping areas we don't virtualize still
> +         * hit the device. */
> +        real_val = assigned_dev_pci_read(d, address, len);
> +        val = merge_bits(val, real_val, address, len,
> +                         PCI_CAPABILITY_LIST, 0xff);
> +        val = merge_bits(val, real_val, address, len,
> +                         PCI_INTERRUPT_LINE, 0xffff);
>      }
>  
>      DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",
> @@ -467,7 +478,7 @@ again:
>  static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
>                                               int len)
>  {
> -    uint32_t val = 0;
> +    uint32_t val = 0, virt_val;
>      int fd;
>      ssize_t ret;
>      AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);
> @@ -483,13 +494,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
>       * Catch access to
>       *  - vendor & device ID
>       *  - base address registers
> -     *  - ROM base address & capability pointer
> -     *  - interrupt line & pin
> +     *  - ROM base address
>       */
>      if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
>          ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
> -        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
> -        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> +        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
>          val = pci_default_read_config(d, address, len);
>          DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
>                (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
> @@ -523,6 +532,15 @@ do_log:
>                           address, len, PCI_COMMAND, 0xffff);
>      }
>  
> +    /*
> +     * Merge bits from virtualized
> +     *  - capability pointer
> +     *  - interrupt line & pin
> +     */
> +    virt_val = pci_default_read_config(d, address, len);
> +    val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
> +    val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
> +
>      if (!pci_dev->cap.available) {
>          /* kill the special capabilities */
>          if (address == PCI_COMMAND && len == 4) {



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index cea072e..8e95730 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -438,11 +438,22 @@  static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
      *  - interrupt line & pin
      */
     if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
-        /* used for update-mappings (BAR emulation) */
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         pci_default_write_config(d, address, val, len);
         return;
+    } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
+               ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        uint32_t real_val;
+
+        pci_default_write_config(d, address, val, len);
+
+        /* Ensure that writes to overlapping areas we don't virtualize still
+         * hit the device. */
+        real_val = assigned_dev_pci_read(d, address, len);
+        val = merge_bits(val, real_val, address, len,
+                         PCI_CAPABILITY_LIST, 0xff);
+        val = merge_bits(val, real_val, address, len,
+                         PCI_INTERRUPT_LINE, 0xffff);
     }
 
     DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",
@@ -467,7 +478,7 @@  again:
 static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
                                              int len)
 {
-    uint32_t val = 0;
+    uint32_t val = 0, virt_val;
     int fd;
     ssize_t ret;
     AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);
@@ -483,13 +494,11 @@  static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
      * Catch access to
      *  - vendor & device ID
      *  - base address registers
-     *  - ROM base address & capability pointer
-     *  - interrupt line & pin
+     *  - ROM base address
      */
     if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
         ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
-        ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
-        ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
+        ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
         val = pci_default_read_config(d, address, len);
         DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
               (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
@@ -523,6 +532,15 @@  do_log:
                          address, len, PCI_COMMAND, 0xffff);
     }
 
+    /*
+     * Merge bits from virtualized
+     *  - capability pointer
+     *  - interrupt line & pin
+     */
+    virt_val = pci_default_read_config(d, address, len);
+    val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
+    val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
+
     if (!pci_dev->cap.available) {
         /* kill the special capabilities */
         if (address == PCI_COMMAND && len == 4) {