Patchwork [V7,06/11] pci.c: Add pci_check_bar_overlap

login
register
mail settings
Submitter Anthony PERARD
Date Feb. 17, 2012, 5:08 p.m.
Message ID <1329498525-8454-7-git-send-email-anthony.perard@citrix.com>
Download mbox | patch
Permalink /patch/141957/
State New
Headers show

Comments

Anthony PERARD - Feb. 17, 2012, 5:08 p.m.
From: Yuji Shimada <shimada-yxb@necst.nec.co.jp>

This function helps Xen PCI Passthrough device to check for overlap.

Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/pci.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci.h |    3 +++
 2 files changed, 50 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Feb. 19, 2012, 1:35 p.m.
On Fri, Feb 17, 2012 at 05:08:40PM +0000, Anthony PERARD wrote:
> From: Yuji Shimada <shimada-yxb@necst.nec.co.jp>
> 
> This function helps Xen PCI Passthrough device to check for overlap.
> 
> Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

As far as I can see, this scans the bus a specific
device is on, looking for devices who have conflicting
BARs. Returns 1 if found, 0 if not.

Not sure what would devices do with this information, really:
patches posted just print out a warning which does not seem
too useful.


Just FYI, if you decided to e.g. disable device in
such a case that would be wrong: it is legal to have overlapping BARs as
long as there are no accesses.
So a legal thing for a guest to do is:

Assign BAR1 = abcde
Assign BAR2 = abcde <- overlaps temporarily
Assign BAR1 = 12345

And this means that you can't just check your device
has no conflicts when your device is touched:
it needs to be checked each time mappings are updated.

> ---
>  hw/pci.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci.h |    3 +++
>  2 files changed, 50 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 678a8c1..75d6529 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1985,6 +1985,53 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>      return dev->bus->address_space_io;
>  }
>  
> +int pci_check_bar_overlap(PCIDevice *dev,
> +                          pcibus_t addr, pcibus_t size, uint8_t type)

This lacks comments documentng what this does, parameters, return
values, etc.

> +{
> +    PCIBus *bus = dev->bus;
> +    PCIDevice *devices = NULL;
> +    PCIIORegion *r;
> +    int i, j;
> +    int rc = 0;
> +
> +    /* check Overlapped to Base Address */

Weird use of upper case. Intentional?
Also - what does this comment mean?

> +    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
> +        devices = bus->devices[i];
> +        if (!devices) {
> +            continue;
> +        }
> +
> +        /* skip itself */

itself?

> +        if (devices->devfn == dev->devfn) {
> +            continue;
> +        }
> +
> +        for (j = 0; j < PCI_NUM_REGIONS; j++) {

This ignores bridges.

> +            r = &devices->io_regions[j];
> +
> +            /* skip different resource type, but don't skip when
> +             * prefetch and non-prefetch memory are compared.
> +             */
> +            if (type != r->type) {
> +                if (type & PCI_BASE_ADDRESS_SPACE_IO ||
> +                    r->type & PCI_BASE_ADDRESS_SPACE_IO) {

Do you mean
	type & PCI_BASE_ADDRESS_SPACE_IO != r->type &
PCI_BASE_ADDRESS_SPACE_IO
?

This would not need a comment then.


> +                    continue;
> +                }
> +            }
> +
> +            if ((addr < (r->addr + r->size)) && ((addr + size) > r->addr)) {

Can ranges_overlap be used?


> +                printf("Overlapped to device[%02x:%02x.%x][Region:%d]"
> +                       "[Address:%"PRIx64"h][Size:%"PRIx64"h]\n",
> +                       pci_bus_num(bus), PCI_SLOT(devices->devfn),
> +                       PCI_FUNC(devices->devfn), j, r->addr, r->size);


That's not how one should report errors.

> +                rc = 1;
> +            }
> +        }
> +    }
> +
> +    return rc;
> +}
> +
>  static void pci_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *k = DEVICE_CLASS(klass);
> diff --git a/hw/pci.h b/hw/pci.h
> index 33b0b18..f05fda5 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -566,4 +566,7 @@ extern const VMStateDescription vmstate_pci_device;
>      .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
>  }
>  
> +int pci_check_bar_overlap(PCIDevice *dev,
> +                          pcibus_t addr, pcibus_t size, uint8_t type);
> +
>  #endif
> -- 
> Anthony PERARD
Anthony PERARD - Feb. 28, 2012, 11:19 a.m.
On Sun, Feb 19, 2012 at 13:35, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Feb 17, 2012 at 05:08:40PM +0000, Anthony PERARD wrote:
>> From: Yuji Shimada <shimada-yxb@necst.nec.co.jp>
>>
>> This function helps Xen PCI Passthrough device to check for overlap.
>>
>> Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
> As far as I can see, this scans the bus a specific
> device is on, looking for devices who have conflicting
> BARs. Returns 1 if found, 0 if not.
>
> Not sure what would devices do with this information, really:
> patches posted just print out a warning which does not seem
> too useful.

This function is just here to print a warning in case an overlap
happen, but we would like too keep this warning.

> Just FYI, if you decided to e.g. disable device in
> such a case that would be wrong: it is legal to have overlapping BARs as
> long as there are no accesses.
> So a legal thing for a guest to do is:
>
> Assign BAR1 = abcde
> Assign BAR2 = abcde <- overlaps temporarily
> Assign BAR1 = 12345
>
> And this means that you can't just check your device
> has no conflicts when your device is touched:
> it needs to be checked each time mappings are updated.
>
>> ---
>>  hw/pci.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/pci.h |    3 +++
>>  2 files changed, 50 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 678a8c1..75d6529 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1985,6 +1985,53 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>>      return dev->bus->address_space_io;
>>  }
>>
>> +int pci_check_bar_overlap(PCIDevice *dev,
>> +                          pcibus_t addr, pcibus_t size, uint8_t type)
>
> This lacks comments documentng what this does, parameters, return
> values, etc.

I'll fix that.

>> +{
>> +    PCIBus *bus = dev->bus;
>> +    PCIDevice *devices = NULL;
>> +    PCIIORegion *r;
>> +    int i, j;
>> +    int rc = 0;
>> +
>> +    /* check Overlapped to Base Address */
>
> Weird use of upper case. Intentional?
> Also - what does this comment mean?

Don't know, I will remove the comment, it is useless.

>> +    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
>> +        devices = bus->devices[i];
>> +        if (!devices) {
>> +            continue;
>> +        }
>> +
>> +        /* skip itself */
>
> itself?

Same here, the comment is probably useless, the next line should be
easy to understand.

>> +        if (devices->devfn == dev->devfn) {
>> +            continue;
>> +        }
>> +
>> +        for (j = 0; j < PCI_NUM_REGIONS; j++) {
>
> This ignores bridges.

I'm not familiar with brigdes. I'll try to take a look.

>> +            r = &devices->io_regions[j];
>> +
>> +            /* skip different resource type, but don't skip when
>> +             * prefetch and non-prefetch memory are compared.
>> +             */
>> +            if (type != r->type) {
>> +                if (type & PCI_BASE_ADDRESS_SPACE_IO ||
>> +                    r->type & PCI_BASE_ADDRESS_SPACE_IO) {
>
> Do you mean
>        type & PCI_BASE_ADDRESS_SPACE_IO != r->type &
> PCI_BASE_ADDRESS_SPACE_IO
> ?
>
> This would not need a comment then.

Yes, I'll replace that.

>
>> +                    continue;
>> +                }
>> +            }
>> +
>> +            if ((addr < (r->addr + r->size)) && ((addr + size) > r->addr)) {
>
> Can ranges_overlap be used?

Yes.

>> +                printf("Overlapped to device[%02x:%02x.%x][Region:%d]"
>> +                       "[Address:%"PRIx64"h][Size:%"PRIx64"h]\n",
>> +                       pci_bus_num(bus), PCI_SLOT(devices->devfn),
>> +                       PCI_FUNC(devices->devfn), j, r->addr, r->size);
>
>
> That's not how one should report errors.

A callback would be better?


Thanks,

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 678a8c1..75d6529 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1985,6 +1985,53 @@  MemoryRegion *pci_address_space_io(PCIDevice *dev)
     return dev->bus->address_space_io;
 }
 
+int pci_check_bar_overlap(PCIDevice *dev,
+                          pcibus_t addr, pcibus_t size, uint8_t type)
+{
+    PCIBus *bus = dev->bus;
+    PCIDevice *devices = NULL;
+    PCIIORegion *r;
+    int i, j;
+    int rc = 0;
+
+    /* check Overlapped to Base Address */
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        devices = bus->devices[i];
+        if (!devices) {
+            continue;
+        }
+
+        /* skip itself */
+        if (devices->devfn == dev->devfn) {
+            continue;
+        }
+
+        for (j = 0; j < PCI_NUM_REGIONS; j++) {
+            r = &devices->io_regions[j];
+
+            /* skip different resource type, but don't skip when
+             * prefetch and non-prefetch memory are compared.
+             */
+            if (type != r->type) {
+                if (type & PCI_BASE_ADDRESS_SPACE_IO ||
+                    r->type & PCI_BASE_ADDRESS_SPACE_IO) {
+                    continue;
+                }
+            }
+
+            if ((addr < (r->addr + r->size)) && ((addr + size) > r->addr)) {
+                printf("Overlapped to device[%02x:%02x.%x][Region:%d]"
+                       "[Address:%"PRIx64"h][Size:%"PRIx64"h]\n",
+                       pci_bus_num(bus), PCI_SLOT(devices->devfn),
+                       PCI_FUNC(devices->devfn), j, r->addr, r->size);
+                rc = 1;
+            }
+        }
+    }
+
+    return rc;
+}
+
 static void pci_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
diff --git a/hw/pci.h b/hw/pci.h
index 33b0b18..f05fda5 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -566,4 +566,7 @@  extern const VMStateDescription vmstate_pci_device;
     .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
 }
 
+int pci_check_bar_overlap(PCIDevice *dev,
+                          pcibus_t addr, pcibus_t size, uint8_t type);
+
 #endif