Re: [kvm-unit-tests PATCH 09/14] pci: provide pci_set_master()

[Peter Xu <peterx@redhat.com>]

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 <peterx@redhat.com>
> > ---
> >  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