Re: [PATCH kvm-unit-tests 06/17] pci: introduce struct pci_dev

[Peter Xu <peterx@redhat.com>]

On Tue, Nov 08, 2016 at 11:16:11AM +0100, Andrew Jones wrote:
> On Mon, Nov 07, 2016 at 02:42:29PM -0500, Peter Xu wrote:
> > On Mon, Nov 07, 2016 at 07:02:12PM +0100, Andrew Jones wrote:
> > 
> > [...]
> > 
> > > > I thought pcidevaddr_t means bdf? That's a wild guess of me from its
> > > > size, as it's defined as:
> > > > 
> > > >   typedef uint16_t pcidevaddr_t;
> > > > 
> > > > uint16_t makes it looks more like bdf rather than devfn. For devfn,
> > > > uint8_t suites better.
> > > > 
> > > > And what I mean here is to pass in the bdf of the device, not devfn
> > > > only (in our case, devfn is always bdf though, since we are only
> > > > supporting bus number zero).
> > > 
> > > see lib/x86/asm/pci.h:pci_config_read how 'dev' gets used, it's only dev.
> > 
> > But shouldn't "dev" the same as "bdf" here?
> 
> Hmm, yes. We're only shifting it by 8, not 11, so you're right. The word
> 'dev' in pcidevaddr_t threw me off and I didn't bother looking up CF8's
> bits until just now. OK, let's try to get consistent naming going on,
> and we need better comments (comments at all). I'd like to see one
> briefly describing 'bdf' and then use that for the primary name, i.e.
> rename pcidevaddr_t to something like 'pcibdfaddr_t' or whatever.

Yes, indeed the naming is a little bit confusing here. I was always
trying to use bdf/devfn in the whole series to avoid misunderstanding.
Regarding to the naming of pcidevaddr_t: that's okay for me, but I
agree that bdf is better and clearer. If you would not mind, I'll keep
it as it is for now for this series.

Thanks,

-- peterx