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

[Peter Xu <peterx@redhat.com>]

On Tue, Nov 08, 2016 at 06:54:16PM +0100, Alexander Gordeev wrote:

[...]

> > > IMHO actually it'll be nicer with:
> > > 
> > >   struct pci_dev *pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> > 
> > Yes, I was going to suggest this too, but then...
> > 
> > > 
> > > But this needs dynamic allocation of memory, which might be an
> > > overkill for kvm-unit-test. So I chose to allocate the pci_dev on
> > > caller stack.
> > 
> > ...saw the value of avoiding the alloc. That said, we do have precedent
> > for allocating in lib/* code already (virtio_bind), and I am currently
> > reworking x86's memory management code in order to enable malloc/calloc
> > use within lib code for x86. So we could go the alloc way as well...
> > 
> > > 
> > > Anyway, I take the interface issue as a matter of taste.
> > 
> > Not quite. I see Alex's reasoning. Perhaps users want to simply query
> > if a device is there (pci_find_dev), but not actually init a pci_dev
> > struct each time they call it. Alex's proposal keeps pci_find_dev more
> > generally useful.

Sounds reasonable.

> 
> Yep. So in the end of the day I would kept pci_find_dev() and added
> pci_dev_alloc()/pci_dev_free() helpers (not sure about naming though).
> The former would have to initialize pci_dev data to be consistent
> and safely used by any other PCI API that takes pci_dev pointer.

Then I'll keep the old pci_find_dev() interface. And then when init a
PCI device, it will look like:

    struct pci_dev;
    pcidevaddr_t dev_addr;

    dev_addr = pci_find_dev(VENDOR_ID, DEVICE_ID);
    if (dev_addr == PCIDEVADDR_INVALID)
        goto fail;
    pci_dev_init(&pci_dev, dev_addr);
    pci_enable_defaults(&pci_dev);

Introducing pci_dev_alloc()/free() is a good idea as well. Since that
can be a work upon current codes, I'll avoid doing it in this series
to make the series shorter (it's already longer than I thought it
should be :-).

Thanks,

-- peterx