Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Peter Xu <peterx@redhat.com>]

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?

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