On Tue, Feb 28, 2017 at 10:51:52AM +0100, Alexander Gordeev wrote:
> 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?
>
> Yes - see how pci_bar_is_valid() is implemented:
>
> bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> {
> return dev->resource[bar_num] != INVALID_PHYS_ADDR;
> }
>
> On the one hand a testcase should not address a 64-bit BAR using
> high part BAR number. Particularly, when it tries to obtan BAR
> size or address. On the other hand a testcase should be able to
> access a low and high parts separately if it needs to for whatever
> reason. The rest of the API allows that as well.
>
> But we do not have any checks wrt high/low parts right now nor
> this series introduces them yet. It is really about data design.
>
> pci_dev::resource[] contains decoded 32/64-bit BAR addresses,
> not low/high parts of underlying 32-bit device BARs. Yet,
> indexes into this array correspond to raw 32-bit BARs in the
> PCI device configuration space. Thus, a question arises -
> what should be stored in array elements that correspond to
> high-parts of 64-bit BARs? Zero is particularly bad choice,
> because:
> - it is a valid address in PIO address space, so it can not
> stand for "no value" or NULL or whatever marker could be
> used to indicate a high part;
> - the high part of underlying 64-bit address is (could be)
> non-zero. So there is inconsistency also;
>
> So both implementation and data-desine wise the best solution
> I see for now is simply store the same 64-bit address for both
> indexes of a 64-bit BAR.
I see. Could I ask why we cannot just use INVALID_PHYS_ADDR for
res[bar_num+1] when bar_num is 64bit (just like what patch 2 did)?
Thanks,
-- peterx
