On Tue, Nov 01, 2016 at 07:57:26PM +0100, Alexander Gordeev wrote:
> 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?
The first one. I don't know what you mean by the second one :-)
>
> > 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.
addr isn't an input, it's an output. In general, for debugging purposes,
I think it's good practice to ensure all outputs are set to something.
As for choosing something invalid, maybe (~0) is a better choice than
zero, and both (~0) and (0) seem better to me than random stack garbage.
Thanks,
drew
>
> > > + }
> > > +
> > > + 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
--
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
