From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter Xu <peterx@redhat.com>
Subject: Re: [PATCH kvm-unit-tests 06/17] pci: introduce struct pci_dev
Date: Tue, 8 Nov 2016 14:59:17 -0500
Message-ID: <20161108195917.GA19059@pxdev.xzpeter.org>
References: <1477468040-21034-1-git-send-email-peterx@redhat.com>
 <1477468040-21034-7-git-send-email-peterx@redhat.com>
 <20161104164101.ymyxzkjbbpsr4mbr@kamzik.brq.redhat.com>
 <20161108122730.GH10472@agordeev.lab.eng.brq.redhat.com>
 <20161108154832.GF2793@pxdev.xzpeter.org>
 <20161108173554.mxzmva2qsa6gu5e4@kamzik.brq.redhat.com>
 <20161108175416.GD10352@agordeev.lab.eng.brq.redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Cc: Andrew Jones <drjones@redhat.com>, kvm@vger.kernel.org,
        rkrcmar@redhat.com, jan.kiszka@web.de, pbonzini@redhat.com
To: Alexander Gordeev <agordeev@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:44758 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752433AbcKHT7U (ORCPT <rfc822;kvm@vger.kernel.org>);
        Tue, 8 Nov 2016 14:59:20 -0500
Content-Disposition: inline
In-Reply-To: <20161108175416.GD10352@agordeev.lab.eng.brq.redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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