Re: [Xen-devel] [Qemu-devel] [PATCH V7 06/11] pci.c: Add pci_check_bar_overlap

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Sun, Feb 19, 2012 at 13:35, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> On Fri, Feb 17, 2012 at 05:08:40PM +0000, Anthony PERARD wrote:
>> From: Yuji Shimada <shimada-yxb@xxxxxxxxxxxxxxx>
>>
>> This function helps Xen PCI Passthrough device to check for overlap.
>>
>> Signed-off-by: Yuji Shimada <shimada-yxb@xxxxxxxxxxxxxxx>
>> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>
> 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,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel