From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [kvm-unit-tests PATCH 09/14] pci: provide pci_set_master() Date: Mon, 24 Oct 2016 18:11:08 +0800 Message-ID: <20161024101108.GY15168@pxdev.xzpeter.org> References: <1476448852-30062-1-git-send-email-peterx@redhat.com> <1476448852-30062-10-git-send-email-peterx@redhat.com> <20161020124950.uridfndvesdoqa44@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]:47162 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754820AbcJXKLI (ORCPT ); Mon, 24 Oct 2016 06:11:08 -0400 Content-Disposition: inline In-Reply-To: <20161020124950.uridfndvesdoqa44@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Oct 20, 2016 at 02:49:50PM +0200, Andrew Jones wrote: > On Fri, Oct 14, 2016 at 08:40:47PM +0800, Peter Xu wrote: > > And set master by default for pci devices. So that DMA is allowed. > > > > Signed-off-by: Peter Xu > > --- > > lib/pci.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/lib/pci.c b/lib/pci.c > > index ef0b02a..044e4db 100644 > > --- a/lib/pci.c > > +++ b/lib/pci.c > > @@ -7,6 +7,13 @@ > > #include "pci.h" > > #include "asm/pci.h" > > > > +static void pci_set_master(pci_dev *dev, int master) > > +{ > > + uint32_t val = pci_config_read(dev->pci_addr, PCI_COMMAND); > > + val |= PCI_COMMAND_MASTER; > > + pci_config_write(dev->pci_addr, PCI_COMMAND, val); > > +} > > I agree with the API extension (well, I know next to nothing about PCI, > so I'll assume this function is correct...) But... > > > + > > /* > > * Scan bus look for a specific device. Only bus 0 scanned for now. > > * After the scan, a pci_dev is returned with correct BAR information. > > @@ -41,6 +48,8 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id) > > } > > dev->inited = true; > > > > + pci_set_master(dev, 1); > > ...I don't necessarily agree with it being enabled by default. In the > least I think you want a minimal init function available to unit tests > that they then can extend with their own custom init. Then you can also > provide an init that configures a bunch of stuff you think is a > good idea by default. For example, in my gic series I try to do that. I > have a gic_init() function which does nothing except set some base > addresses. Then, I have a gic_enable_defaults function that unit tests > can use if they don't care how we enable it, they just want something > that works. Otherwise the unit test would just call gic_init and then > write all the registers for enabling it that they want/need. Good idea. will follow. First of all, I'll introduce pci_dev_init() to init the raw structure. Then, will add one pci_enable_defaults() with all the rests. Thanks, -- peterx