Re: [kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning

[Andrew Jones <drjones@redhat.com>]

On Tue, Feb 02, 2016 at 10:34:01AM +0100, Alexander Gordeev wrote:
> On Wed, Jan 13, 2016 at 04:58:46PM +0100, Andrew Jones wrote:
> > On Sat, Jan 09, 2016 at 01:22:49PM +0100, Alexander Gordeev wrote:
> > > Scan bus 0 and function 0 only for now
> > > 
> > > Cc: Andrew Jones <drjones@redhat.com>
> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > > ---
> > >  arm/pci-test.c         |  4 +++
> > >  lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/pci-host-generic.h |  1 +
> > >  lib/pci.h              |  1 +
> > >  4 files changed, 73 insertions(+)
> > > 
> > > diff --git a/arm/pci-test.c b/arm/pci-test.c
> > > index 8629c89..1539d3e 100644
> > > --- a/arm/pci-test.c
> > > +++ b/arm/pci-test.c
> > > @@ -11,6 +11,7 @@
> > >  int main(void)
> > >  {
> > >  	struct pci *pci;
> > > +	int ret;
> > >  
> > >  	pci = pci_dt_probe();
> > >  
> > > @@ -18,6 +19,9 @@ int main(void)
> > >  	if (!pci)
> > >  		goto done;
> > >  
> > > +	ret = pci_bus_scan(pci);
> > > +	report("PCI bus scanning detected %d devices", true, ret);
> > 
> > How many devices are expected in this test? You can pass the expected
> > number in on the command line, and then make this a "real" test report
> > with
> > 
> >   report(..., ret == atol(argv[0]), ret)
> 
> What if a machine's PCI hierarchy changed? Such test would break then.

How would the number *of* detected devices change if we never pass more
or less devices on the qemu command line? I realize the PCI addresses
may change.

> 
> Also, this report is not entirely dummy. It ensures PCI configuration
> space access operates, devices do respond and generally the bus is fine.
> 
> > > +
> > >  	pci_shutdown(pci);
> > >  
> > >  done:
> > > diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> > > index 62b5662..e479989 100644
> > > --- a/lib/pci-host-generic.c
> > > +++ b/lib/pci-host-generic.c
> > > @@ -14,6 +14,49 @@
> > >  #include "pci-host-generic.h"
> > >  #include <linux/pci_regs.h>
> > >  
> > > +#define for_each_pci_dev(pci, dev, conf)		\
> > > +	for (dev = find_next_dev(pci, -1, &conf);	\
> > > +	     dev >= 0;					\
> > > +	     dev = find_next_dev(pci, dev, &conf))
> > 
> > Why do we need to pass both a dev index and a conf
> > pointer? I see below you can get the later from the
> > former easily enough.
> 
> No particular reason apart from the fact each for_each_pci_dev()
> would be followed by dev_conf() call - so I paired it to save
> few lines. I have no preference here, though.

Then please drop one or the other.

> 
> > > +
> > > +static u8 pci_config_readb(const void *conf, int off)
> > > +{
> > > +	return readb(conf + off);
> > > +}
> > > +
> > > +static u32 pci_config_readl(const void *conf, int off)
> > > +{
> > > +	return readl(conf + off);
> > > +}
> > > +
> > > +static void pci_config_writel(u32 val, void *conf, int off)
> > > +{
> > > +	writel(val, conf + off);
> > > +}
> > 
> > These read/write wrappers don't look all that useful to me. They would
> > be more useful if they took a 'struct pci[_dev] like Linux's
> > pci_read_config_byte/word/dword do. In fact, stealing those from Linux
> > makes sense.
> 
> I put those ones only to emphasize PCI reads/writes as opposed to
> direct read*/write* accesses. Matching the Linux names is probably
> confusing.
> 
> Bringing Linux-like pci_read_config_byte/word/dword seems as an
> overkill to me as it entails pulling the whole data hierarchy
> pci_host_bridge->pci_bus->pci_dev.
> 
> By contrast, if we do not pull the whole hierarchy then we could
> introduce our own minimalistic data structure(s) and get off with
> it easily.

Yes, that's what I was thinking. We already do have our own minimalistic
data structure 'struct pci[_dev]'. However, now that I've made x86 pci
code common you'll either just need to create a lib/arm/asm/pci.h with
a pci_config_read like in lib/x86/asm/pci.h, or perhaps expand on that
if you need to read different sizes, by adding the _byte/word/...
variants.

> 
> > > +
> > > +static void* dev_conf(struct pci *pci, int dev)
> > > +{
> > > +	struct pci_host_bridge *host = pci->sysdata;
> > > +	return (void*)host->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;
> > > +}
> > > +
> > > +/* We scan bus 0 only for now */
> > > +static int find_next_dev(struct pci *pci, int dev, void **pconf)
> > > +{
> > > +	struct pci_host_bridge *host = pci->sysdata;
> > > +	int limit = MIN(32u, host->cpu_range.size / PCI_ECAM_CONFIG_SIZE);
> > 
> > 32 is the pci max devices per bus limit, right? Assuming a sane DT, is it
> > possible to have host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32?
> 
> The point here is not host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32
> but rather host->cpu_range.size/PCI_ECAM_CONFIG_SIZE < 32.
> 
> But you are right - a bus with less than 32 device slots is likely
> insane.
> 
> > > +
> > > +	for (dev++; dev < limit; dev++) {
> > > +		void *conf = dev_conf(pci, dev);
> > > +		if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0)) {
> > > +			*pconf = conf;
> > > +			return dev;
> > > +		}
> > > +	}
> > > +
> > > +	return -1;
> > > +}
> > > +
> > >  static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
> > >  				     u32 cells[], int nr_range_cells)
> > >  {
> > > @@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
> > >  	return host;
> > >  }
> > >  
> > > +int pci_bus_scan(struct pci *pci)
> > > +{
> > > +	void *conf;
> > > +	int dev;
> > > +	int nr_dev = 0;
> > > +
> > > +	for_each_pci_dev(pci, dev, conf) {
> > > +		/* We are only interested in normal PCI devices */
> > > +		if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
> > > +					   PCI_HEADER_TYPE_NORMAL)
> > > +			continue;
> > > +		pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
> > 
> > Shouldn't the driver (unit test) be the one that writes SERR? Also,
> > doesn't this clear everything in the Command register except SERR?
> 
> I do not thing a driver is in charge. It is a minimal devices
> initialization that is expected from firmware/BIOS AFAICT. A driver
> should inherit the properly initialized bus with allocated io/memory
> buffers to use.

OK, does QEMU not provide that already?

> 
> > If a scan gets run again after driver/unittest has started using a device
> > then this write may mess things up.
> 
> PCI bus scan is normally done once to initialize hierarchy. I am
> struggling to imagine a usecase when it is called repeatedly,
> especially in such a small framework.

A unit test can do whatever it likes. I'd like the initial setup to be
useful, but minimal, and easily modifiable.

> 
> > > +		nr_dev++;
> > > +	}
> > > +
> > > +	return nr_dev;
> > > +}
> > > +
> > >  struct pci *pci_dt_probe(void)
> > >  {
> > >  	struct pci_host_bridge *host;
> > > @@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)
> > >  
> > >  void pci_shutdown(struct pci *pci)
> > >  {
> > > +	void *conf;
> > > +	int dev;
> > > +
> > > +	for_each_pci_dev(pci, dev, conf)
> > > +		pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);
> > 
> > I think I know why this is here, but I don't think that kvm-unit-tests
> > needs to bother with it.
> 
> Well, it is not about interrupts, rather about disabling devices
> access to access io/memory buffers, which is writing 0 to the
> command regiester. PCI_COMMAND_INTX_DISABLE is just a good one
> to write as well.
> 
> BTW, this is the reason I named this routine pci_shutdown(), not
> pci_free().

I'm currently thinking any sort of shutdown is overkill for
kvm-unit-tests, but I'll accept anything you can make a good
case for :-)

> 
> > > +
> > >  	free(pci->sysdata);
> > >  	free(pci);
> > >  }
> > > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> > > index 097ac2d..c14d32d 100644
> > > --- a/lib/pci-host-generic.h
> > > +++ b/lib/pci-host-generic.h
> > > @@ -22,5 +22,6 @@ struct pci_host_bridge {
> > >  };
> > >  
> > >  #define PCI_ECAM_BUS_SIZE		(1 << 20)
> > > +#define PCI_ECAM_CONFIG_SIZE		(1 << 12)
> > 
> > Are these also defined in the Linux kernel? I'm too lazy to pull up the
> > PCI spec right now, so I'd rather confirm the sizes by looking them up
> > in Linux. If they are defined there somewhere, then please use the same
> > names here. Otherwise, I guess references to some documentation or
> > something would be nice for pci dummies like me.
> > 
> > >  
> > >  #endif
> > > diff --git a/lib/pci.h b/lib/pci.h
> > > index 22b8e31..bc4f2d2 100644
> > > --- a/lib/pci.h
> > > +++ b/lib/pci.h
> > > @@ -8,6 +8,7 @@ struct pci {
> > >  };
> > >  
> > >  extern struct pci *pci_dt_probe(void);
> > > +extern int pci_bus_scan(struct pci *pci);
> > >  extern void pci_shutdown(struct pci *pci);
> > >  
> > >  #endif
> > > -- 
> > > 1.8.3.1
> > >