From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [kvm-unit-tests PATCH 10/14] pci: add bdf helpers Date: Mon, 24 Oct 2016 22:44:53 +0800 Message-ID: <20161024144453.GZ15168@pxdev.xzpeter.org> References: <1476448852-30062-1-git-send-email-peterx@redhat.com> <1476448852-30062-11-git-send-email-peterx@redhat.com> <20161020125539.6jbickrv52x3mvec@kamzik.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: kvm@vger.kernel.org, jan.kiszka@web.de, agordeev@redhat.com, rkrcmar@redhat.com, pbonzini@redhat.com To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46914 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755055AbcJXOoy (ORCPT ); Mon, 24 Oct 2016 10:44:54 -0400 Content-Disposition: inline In-Reply-To: <20161020125539.6jbickrv52x3mvec@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Oct 20, 2016 at 02:55:39PM +0200, Andrew Jones wrote: > On Fri, Oct 14, 2016 at 08:40:48PM +0800, Peter Xu wrote: > > Signed-off-by: Peter Xu > > --- > > lib/pci.h | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/lib/pci.h b/lib/pci.h > > index b8755ff..6a1c3c9 100644 > > --- a/lib/pci.h > > +++ b/lib/pci.h > > @@ -18,6 +18,12 @@ enum { > > #define PCI_BAR_NUM (6) > > #define PCI_DEVFN_MAX (256) > > > > +#define PCI_BDF_GET_DEVFN(x) ((x) & 0xff) > > +#define PCI_BDF_GET_BUS(x) (((x) >> 8) & 0xff) > > +#define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) > > +#define PCI_FUNC(devfn) ((devfn) & 0x07) > > +#define PCI_BUILD_BDF(bus, devfn) ((bus << 8) | (devfn)) > > As I mention frequently; I know nothing about PCI, but this doesn't > look right to me. DEVFN should be defined as > > #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) > > Care to explain these? Please provide the explanation in the commit > message to along with a motivation for adding them. Looks like the above lines are not conflicting? E.g., the above patch (PCI_SLOT and PCI_FUNC) provides way to abstract slot/function out of devfn value, while the one you mentioned (PCI_DEVFN) should be the builder macro that builds a devfn value from its slot and function? However... I just found that I didn't use PCI_SLOT/PCI_FUNC/PCI_BUILD_BDF macros in the following patches, so maybe I should just remove the last three of them... Thanks, -- peterx