Re: [PATCH kvm-unit-tests v5 09/14] pci: provide pci_scan_bars()

From: Alexander Gordeev <agordeev@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: kvm@vger.kernel.org, drjones@redhat.com, jan.kiszka@web.de,
	rkrcmar@redhat.com, pbonzini@redhat.com
Subject: Re: [PATCH kvm-unit-tests v5 09/14] pci: provide pci_scan_bars()
Date: Mon, 21 Nov 2016 20:24:13 +0100	[thread overview]
Message-ID: <20161121192413.GD30468@agordeev.lab.eng.brq.redhat.com> (raw)
In-Reply-To: <20161121072713.GG3692@pxdev.xzpeter.org>

On Mon, Nov 21, 2016 at 03:27:13PM +0800, Peter Xu wrote:
> On Wed, Nov 16, 2016 at 10:50:37AM +0100, Alexander Gordeev wrote:
> 
> [...]
> 
> > >  struct pci_dev {
> > >  	uint16_t bdf;
> > > +	phys_addr_t bar[PCI_BAR_NUM];
> > 
> > This questions pop up time and again. If bars are 32 or 64 bit?
> > What if bar is not 64 aligned? etc.
> 
> Could you help elaborate what does "not 64 aligned" mean here? AFAIU
> the bar can only be either 32bit or 64bit wide?

Let's use your example below.

> > I guess, it worth mentionig in comment that bar[] array here does
> > not match PCI header binary layout.
> 
> Not sure whether you mean this:
> 
>     void pci_scan_bars(struct pci_dev *dev)
>     {
>         int i, cur = 0;
> 
>         for (i = 0; i < PCI_BAR_NUM; i++) {
>             if (!pci_bar_is_valid(dev, i))
>                 continue;
>             dev->bar[cur++] = pci_bar_get_addr(dev, i);
>             if (pci_bar_is64(dev, i))
>                 i++;
>         }
>     }
> 
> Assume that we have a device with:
> 
> - bar 0: 32 bit addr A0
> - bar 1: 64 bit addr A1
> - bar 2: 32 bit addr A2
> 
> The original patch will generate:
> 
>   dev->bar[0] == A0
>   dev->bar[1] == A1
>   dev->bar[2] == 0
>   dev->bar[3] == A2
>   dev->bar[4] == 0
>   dev->bar[5] == 0
> 
> The new code (above) will generate:
> 
>   dev->bar[0] == A0
>   dev->bar[1] == A1
>   dev->bar[2] == A2
>   dev->bar[3] == 0
>   dev->bar[4] == 0
>   dev->bar[5] == 0

So in all three cases (PCI header, 6 32-bit words and
6 64-bit physical addresses) the term 'bar' is somehow
correct.

Yet it is confusing, because in the first two cases it reflects
device PCI header layout (6 32-bit words) while in the latter
case it is a physical address.

My suggestion is to rename pci_dev::bar[] to pci_dev::resource[]
to uncouple what is stored in this array (addresses) from PCI
header layout.

> Thanks,
> 
> -- peterx