From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [PATCH kvm-unit-tests 10/17] pci: provide pci_set_master() Date: Mon, 7 Nov 2016 14:45:42 -0500 Message-ID: <20161107194542.GO3719@pxdev.xzpeter.org> References: <1477468040-21034-1-git-send-email-peterx@redhat.com> <1477468040-21034-11-git-send-email-peterx@redhat.com> <20161104170456.g2kamipgo26kziwr@kamzik.brq.redhat.com> <20161107173502.GI3719@pxdev.xzpeter.org> <20161107175930.4rky63e4kvb73tdj@kamzik.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: kvm@vger.kernel.org, rkrcmar@redhat.com, agordeev@redhat.com, jan.kiszka@web.de, pbonzini@redhat.com To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51320 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751797AbcKGTpp (ORCPT ); Mon, 7 Nov 2016 14:45:45 -0500 Content-Disposition: inline In-Reply-To: <20161107175930.4rky63e4kvb73tdj@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 07, 2016 at 06:59:30PM +0100, Andrew Jones wrote: > On Tue, Nov 08, 2016 at 01:35:02AM +0800, Peter Xu wrote: > > On Fri, Nov 04, 2016 at 06:04:56PM +0100, Andrew Jones wrote: > > > On Wed, Oct 26, 2016 at 03:47:13PM +0800, Peter Xu wrote: > > > > We need to call this whenever we want to do DMA with a device. > > > > > > > > Signed-off-by: Peter Xu > > > > --- > > > > lib/pci.c | 7 +++++++ > > > > lib/pci.h | 1 + > > > > 2 files changed, 8 insertions(+) > > > > > > > > diff --git a/lib/pci.c b/lib/pci.c > > > > index 95a6581..1495f61 100644 > > > > --- a/lib/pci.c > > > > +++ b/lib/pci.c > > > > @@ -7,6 +7,13 @@ > > > > #include "pci.h" > > > > #include "asm/pci.h" > > > > > > > > +void pci_set_master(struct pci_dev *dev, int master) > > > > > > Why take pci_dev instead of just the devid/bdf here? > > > What is the 'master' argument (which is unused and at least > > > needs an __unused to compile) here for? > > > > I think both works for me here, but when this happens, IMHO pci_dev > > struct is prefered, since as long as the caller has one *dev pointer > > (I think if we introduce pci_dev, people should always keep one object > > for each PCI device they manipulate in kvm-unit-tests), I would just > > let people to pass in "dev" rather than "dev->pci_bdf", since it's > > shorter for typing, and also it avoids referencing internal fields. > > > > Regarding to the "master" variable... Oops, I just missed that. What I > > want to do is: > > > > if (master) > > val |= PCI_COMMAND_MASTER; > > else > > val &= ~PCI_COMMAND_MASTER; > > > > Thanks for pointing out. Will fix. > > > Actually, maybe we can just drop this patch. It's just > > pci_config_writew(dev->bsd, PCI_COMMAND, > pci_config_readw(dev->bsd, PCI_COMMAND) | PCI_COMMAND_MASTER); > ^ change op as needed > > Long, but no big deal... If updating PCI_COMMAND is a common thing > to do then we should have > > pci_command_set(dev, bits) > pci_command_clr(dev, bits) > > operators instead. Sounds good to me. Actually I think it does not hurt to provide a pci_set_master(), even as a wrapper to pci_command_set(). But for now, let me drop pci_set_master() in this series. Maybe I can use one function pci_cmd_set_clr(dev, set_bits, clr_bits) for the two? Thanks, -- peterx