On Fri, Oct 21, 2016 at 06:30:21PM +0200, Andrew Jones wrote:
> > +static bool pci_alloc_resource(u64 *addr, u32 bar, u64 size)
> > +{
> > + struct pci_host_bridge *host = pci_host_bridge;
> > + struct pci_addr_space *as = &host->addr_space[0];
> > + u32 mask;
> > + int i;
> > +
> > + for (i = 0; i < host->nr_addr_spaces; i++) {
> > + if (as->type == pci_bar_type(bar))
> > + break;
> > + as++;
> > + }
> > + if (i >= host->nr_addr_spaces) {
> > + printf("No PCI resource matching a device found\n");
> > + return false;
>
> So this is the new code with v8, and it needs more work. First,
> this print message doesn't explain the problem. It should point
> out which device and which bar failed to have resources allocated
> and why.
>
> For the which, I think it should print the device number and function
> number in the lspci way %02x.%02x.%1x (bus.dev.fn) followed by the
> vendor-id and device-id, %04x:%04x. For the bar you'll want to
> refactor pci_dev_print to pull a pci_bar_print out of it that you
> can use here.
>
> But... what about the why? Is it really necessary to fail this
> allocation? How does Linux handle it with the same guest config?
Just to check we are on the same page - by "fail" you mean here
the failure to assign a resource, not bailing out due to a mis-
configuration, right?
> So I went a head and improved the print message in order to try
> to understand, and I think I do. The virtio-net-pci device that
> gets added by default wanted a MEM64/p region. We fail because
> of the '/p'. I don't know PCI, but shouldn't we be masking off
> that PCI_BASE_ADDRESS_MEM_PREFETCH bit in this check?
I guess, if we could mask /p we would not have it in th first place.
I recall some ugly side effects in case prefetch memory mismatch,
but can not tell for sure.
@Thomas, may be you know the answer here?
> Finally, I'd set *addr to some invalid address, zero? when
> returning false here.
Er.. should not we preserve input data in case a function failed?
Anyway, zero may be a legitimate address in PIO space and I can
not think of any better choice.
> > + }
> > +
> > + mask = pci_bar_mask(bar);
> > + size = ALIGN(size, ~mask + 1);
> > + assert(as->allocated + size <= as->size);
> > +
> > + *addr = as->pci_start + as->allocated;
> > + as->allocated += size;
> > +
> > + return true;
> > +}
> > +
> Thanks,
> drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
