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.
> 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.
>
> Thanks,
>
> -- peterx
