| Message ID | 1329498525-8454-7-git-send-email-anthony.perard@citrix.com |
|---|---|
| State | New |
| Headers | show |
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
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,
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