On Tue, Feb 28, 2017 at 03:02:25PM +0800, Peter Xu wrote:
> On Mon, Feb 27, 2017 at 03:12:34PM +0100, Alexander Gordeev wrote:
> > Array pci_dev::resource[] is ambiguous wrt what is
> > actually stored in its elements and how to interpret
> > the index into the array.
> >
> > It is simple in case a device has only 32-bit BARs -
> > an element pci_dev::resource[bar_num] contains the
> > decoded address of BAR # bar_num.
> >
> > But what if a device has 64-bit BAR starting at bar_num?
> >
> > Curretnly pci_dev::resource[bar_num] contains the decoded
> > address of the BAR, while pci_dev::resource[bar_num + 1]
> > contains 0. That makes meaning of (bar_num + 1) index
> > difficult to understand.
> >
> > On the one hand it is an index of high 32-bit part of
> > the 64-bit address in the device itself. But why then
> > the element contains 0, not the high part of the address
> > or INVALID_PHYS_ADDRESS for example?
> >
> > By placing the same 64-bit address in both bar_num and
> > (bar_num + 1) elements the ambiguity is less striking,
> > since:
> > - the meaning of bar_num kept consistent with the rest
> > of the functions (where it refers 32-bit BAR in terms
> > of the device configuration address space);
> > - pci_dev::resource[bar_num + 1] contains a valid address
> > rather than vague value of 0.
> > - both bar_num and (bar_num + 1) indexes refer to the
> > same 64-bit BAR and therefore return the same address;
> > The notion of low and high parts of a 64-bit address
> > is ignored, but that is fine, since pci_dev::resource[]
> > contain only full addresses;
>
> IIUC for a general PCI device driver, it should know which bars are
> used for specific device, the type of the bar (whether it would be
> 64bit), for what purpose, etc.. Then, the driver should just avoid
> touching the other bar (in our case, bar_num+1). So here I don't quite
> catch why we need to explicitly have res[bar_num+1] contain the same
> content as res[bar_num]. Do we really have a use case?
kvm-unit-test drivers can be unit tests. I prefer that common code
provide asserts allowing unit test writers to quickly determine
when their test has been incorrectly written, rather than forcing
them to read the common code in order to debug the issue. Currently
it's not possible to ensure a driver (unit test) does not attempt
to use invalid bars or 64bit bars in invalid ways. This series
prepares for some asserts to get sprinkled into the common code
and/or offer a better "is valid" API.
>
> And, looks like this patch has just overwritten previous patch (patch
> 2). IMHO I would slightly prefer that one since INVALID_PHYS_ADDR at
> least showed that we fetched something wrong.
Not really. The INVALID_PHYS_ADDR is still assigned to unused bars
in the else {} below.
Thanks,
drew
