diff mbox

[V8,RESEND,4/8] pci.c: Add pci_check_bar_overlap

Message ID 1331916862-20504-5-git-send-email-anthony.perard@citrix.com
State New
Headers show

Commit Message

Anthony PERARD March 16, 2012, 4:54 p.m. UTC 
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 |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci.h |    5 +++++
 2 files changed, 55 insertions(+), 0 deletions(-)

Comments

Stefano Stabellini March 19, 2012, 11:55 a.m. UTC | #1 
On Fri, 16 Mar 2012, 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>

It is probably worth mentioning that the function as it is cannot handle
bridges in the commit message too.
Other than that, ack.


>  hw/pci.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci.h |    5 +++++
>  2 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 38e1de5..f950b4e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1992,6 +1992,56 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>      return dev->bus->address_space_io;
>  }
>  
> +/* This function checks if an io_region overlap an io_region from another
> + * device.  The io_region to check is provide with (addr, size and type)
> + * A callback can be provide and will be called for every region that is
> + * overlapped.
> + * The return value indicate if the region is overlappsed */
> +bool pci_check_bar_overlap(PCIDevice *device,
> +                           pcibus_t addr, pcibus_t size, uint8_t type,
> +                           void (*c)(void *o, const PCIDevice *d, int index),
> +                           void *opaque)
> +{
> +    PCIBus *bus = device->bus;
> +    int i, j;
> +    bool rc = false;
> +
> +    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
> +        PCIDevice *d = bus->devices[i];
> +        if (!d) {
> +            continue;
> +        }
> +
> +        if (d->devfn == device->devfn) {
> +            continue;
> +        }
> +
> +        /* xxx: This ignores bridges. */
> +        for (j = 0; j < PCI_NUM_REGIONS; j++) {
> +            PCIIORegion *r = &d->io_regions[j];
> +
> +            if (!r->size) {
> +                continue;
> +            }
> +            if ((type & PCI_BASE_ADDRESS_SPACE_IO)
> +                != (r->type & PCI_BASE_ADDRESS_SPACE_IO)) {
> +                continue;
> +            }
> +
> +            if (ranges_overlap(addr, size, r->addr, r->size)) {
> +                if (c) {
> +                    c(opaque, d, j);
> +                    rc = true;
> +                } else {
> +                    return true;
> +                }
> +            }
> +        }
> +    }
> +
> +    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 4f19fdb..cbd04e1 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -628,4 +628,9 @@ extern const VMStateDescription vmstate_pci_device;
>      .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
>  }
>  
> +bool pci_check_bar_overlap(PCIDevice *dev,
> +                           pcibus_t addr, pcibus_t size, uint8_t type,
> +                           void (*c)(void *o, const PCIDevice *d, int index),
> +                           void *opaque);
> +
>  #endif
> -- 
> Anthony PERARD
>
Michael S. Tsirkin March 19, 2012, 1:15 p.m. UTC | #2 
On Fri, Mar 16, 2012 at 04:54:18PM +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>

It seems that what's called for here really is
using the new memory region infrastructure.
That handles overlap etc nicely.

That said, I don't mind, but would prefer to
keep this mess outside the pci core. See below.

> ---
>  hw/pci.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci.h |    5 +++++
>  2 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 38e1de5..f950b4e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1992,6 +1992,56 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>      return dev->bus->address_space_io;
>  }
>  
> +/* This function

Comment blocks start with /* */

> checks if an io_region overlap an io_region from another

overlaps

> + * device.  The io_region to check is provide with (addr, size and type)

provided

> + * A callback can be provide and will be called for every region that is

provided

> + * overlapped.
> + * The return value indicate if the region is overlappsed */

indicates


> +bool pci_check_bar_overlap(PCIDevice *device,
> +                           pcibus_t addr, pcibus_t size, uint8_t type,
> +                           void (*c)(void *o, const PCIDevice *d, int index),
> +                           void *opaque)

IMO this is inlikely to be needed by anyone except Xen.
How about a generic pci_foreach_device and let Xen
implement the hacks internally.

> +{
> +    PCIBus *bus = device->bus;
> +    int i, j;
> +    bool rc = false;
> +
> +    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
> +        PCIDevice *d = bus->devices[i];
> +        if (!d) {
> +            continue;
> +        }
> +
> +        if (d->devfn == device->devfn) {
> +            continue;
> +        }
> +
> +        /* xxx: This ignores bridges. */
> +        for (j = 0; j < PCI_NUM_REGIONS; j++) {
> +            PCIIORegion *r = &d->io_regions[j];
> +
> +            if (!r->size) {
> +                continue;
> +            }
> +            if ((type & PCI_BASE_ADDRESS_SPACE_IO)
> +                != (r->type & PCI_BASE_ADDRESS_SPACE_IO)) {
> +                continue;
> +            }
> +
> +            if (ranges_overlap(addr, size, r->addr, r->size)) {
> +                if (c) {
> +                    c(opaque, d, j);
> +                    rc = true;
> +                } else {
> +                    return true;
> +                }
> +            }
> +        }
> +    }
> +
> +    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 4f19fdb..cbd04e1 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -628,4 +628,9 @@ extern const VMStateDescription vmstate_pci_device;
>      .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
>  }
>  
> +bool pci_check_bar_overlap(PCIDevice *dev,
> +                           pcibus_t addr, pcibus_t size, uint8_t type,
> +                           void (*c)(void *o, const PCIDevice *d, int index),
> +                           void *opaque);
> +
>  #endif
> -- 
> Anthony PERARD
Anthony PERARD March 19, 2012, 5:22 p.m. UTC | #3 
On 19/03/12 13:15, Michael S. Tsirkin wrote:
> On Fri, Mar 16, 2012 at 04:54:18PM +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>
>
> It seems that what's called for here really is
> using the new memory region infrastructure.
> That handles overlap etc nicely.
>
> That said, I don't mind, but would prefer to
> keep this mess outside the pci core. See below.
>
>> ---
>>   hw/pci.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/pci.h |    5 +++++
>>   2 files changed, 55 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 38e1de5..f950b4e 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1992,6 +1992,56 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>>       return dev->bus->address_space_io;
>>   }
>>
>> +/* This function
>
> Comment blocks start with /* */
>
>> checks if an io_region overlap an io_region from another
>
> overlaps
>
>> + * device.  The io_region to check is provide with (addr, size and type)
>
> provided
>
>> + * A callback can be provide and will be called for every region that is
>
> provided
>
>> + * overlapped.
>> + * The return value indicate if the region is overlappsed */
>
> indicates
>
>
>> +bool pci_check_bar_overlap(PCIDevice *device,
>> +                           pcibus_t addr, pcibus_t size, uint8_t type,
>> +                           void (*c)(void *o, const PCIDevice *d, int index),
>> +                           void *opaque)
>
> IMO this is inlikely to be needed by anyone except Xen.
> How about a generic pci_foreach_device and let Xen
> implement the hacks internally.

Ok, this should be better, I'll work on that.

Thanks,

>> +{
>> +    PCIBus *bus = device->bus;
>> +    int i, j;
>> +    bool rc = false;
>> +
>> +    for (i = 0; i<  ARRAY_SIZE(bus->devices); i++) {
>> +        PCIDevice *d = bus->devices[i];
>> +        if (!d) {
>> +            continue;
>> +        }
>> +
>> +        if (d->devfn == device->devfn) {
>> +            continue;
>> +        }
>> +
>> +        /* xxx: This ignores bridges. */
>> +        for (j = 0; j<  PCI_NUM_REGIONS; j++) {
>> +            PCIIORegion *r =&d->io_regions[j];
>> +
>> +            if (!r->size) {
>> +                continue;
>> +            }
>> +            if ((type&  PCI_BASE_ADDRESS_SPACE_IO)
>> +                != (r->type&  PCI_BASE_ADDRESS_SPACE_IO)) {
>> +                continue;
>> +            }
>> +
>> +            if (ranges_overlap(addr, size, r->addr, r->size)) {
>> +                if (c) {
>> +                    c(opaque, d, j);
>> +                    rc = true;
>> +                } else {
>> +                    return true;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    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 4f19fdb..cbd04e1 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -628,4 +628,9 @@ extern const VMStateDescription vmstate_pci_device;
>>       .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
>>   }
>>
>> +bool pci_check_bar_overlap(PCIDevice *dev,
>> +                           pcibus_t addr, pcibus_t size, uint8_t type,
>> +                           void (*c)(void *o, const PCIDevice *d, int index),
>> +                           void *opaque);
>> +
>>   #endif
>> --
>> Anthony PERARD
diff mbox

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 38e1de5..f950b4e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1992,6 +1992,56 @@  MemoryRegion *pci_address_space_io(PCIDevice *dev)
     return dev->bus->address_space_io;
 }
 
+/* This function checks if an io_region overlap an io_region from another
+ * device.  The io_region to check is provide with (addr, size and type)
+ * A callback can be provide and will be called for every region that is
+ * overlapped.
+ * The return value indicate if the region is overlappsed */
+bool pci_check_bar_overlap(PCIDevice *device,
+                           pcibus_t addr, pcibus_t size, uint8_t type,
+                           void (*c)(void *o, const PCIDevice *d, int index),
+                           void *opaque)
+{
+    PCIBus *bus = device->bus;
+    int i, j;
+    bool rc = false;
+
+    for (i = 0; i < ARRAY_SIZE(bus->devices); i++) {
+        PCIDevice *d = bus->devices[i];
+        if (!d) {
+            continue;
+        }
+
+        if (d->devfn == device->devfn) {
+            continue;
+        }
+
+        /* xxx: This ignores bridges. */
+        for (j = 0; j < PCI_NUM_REGIONS; j++) {
+            PCIIORegion *r = &d->io_regions[j];
+
+            if (!r->size) {
+                continue;
+            }
+            if ((type & PCI_BASE_ADDRESS_SPACE_IO)
+                != (r->type & PCI_BASE_ADDRESS_SPACE_IO)) {
+                continue;
+            }
+
+            if (ranges_overlap(addr, size, r->addr, r->size)) {
+                if (c) {
+                    c(opaque, d, j);
+                    rc = true;
+                } else {
+                    return true;
+                }
+            }
+        }
+    }
+
+    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 4f19fdb..cbd04e1 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -628,4 +628,9 @@  extern const VMStateDescription vmstate_pci_device;
     .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
 }
 
+bool pci_check_bar_overlap(PCIDevice *dev,
+                           pcibus_t addr, pcibus_t size, uint8_t type,
+                           void (*c)(void *o, const PCIDevice *d, int index),
+                           void *opaque);
+
 #endif