Re: [PATCH v2 5/5] arm/pci: pci-testdev PCI device operation test

[Andrew Jones <drjones@redhat.com>]

On Mon, Feb 15, 2016 at 09:58:37PM +0100, Andrew Jones wrote:
> 
> Should still keep 'arm' out of this summary, because the few lines
> added to arm/pci-test.c should just be squashed into the ones in
> the first patch and posted as a separate patch.
> 
> On Thu, Feb 11, 2016 at 09:25:05AM +0100, Alexander Gordeev wrote:
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  arm/pci-test.c               |   4 +
> >  config/config-arm-common.mak |   1 +
> >  lib/pci-testdev.c            | 189 +++++++++++++++++++++++++++++++++++++++++++
> >  lib/pci.h                    |   6 ++
> >  4 files changed, 200 insertions(+)
> >  create mode 100644 lib/pci-testdev.c
> > 
> > diff --git a/arm/pci-test.c b/arm/pci-test.c
> > index db7d048..2ec6156 100644
> > --- a/arm/pci-test.c
> > +++ b/arm/pci-test.c
> > @@ -15,6 +15,10 @@ int main(void)
> >  	ret = pci_probe();
> >  	report("PCI device tree probing", ret);
> >  
> > +	ret = pci_testdev();
> > +	report("PCI test device passed %d tests",
> > +	       ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);
> 
> Why '>=', aren't we expecting exactly '=='? Wait, I think I see. If
> more tests are added to QEMU, then this will just work. Right?
> 
> > +
> >  	pci_shutdown();
> >  
> >  	return report_summary();
> > diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
> > index 06ad346..f2e0ad1 100644
> > --- a/config/config-arm-common.mak
> > +++ b/config/config-arm-common.mak
> > @@ -31,6 +31,7 @@ include config/asm-offsets.mak
> >  cflatobjs += lib/alloc.o
> >  cflatobjs += lib/devicetree.o
> >  cflatobjs += lib/pci-host-generic.o
> > +cflatobjs += lib/pci-testdev.o
> >  cflatobjs += lib/virtio.o
> >  cflatobjs += lib/virtio-mmio.o
> >  cflatobjs += lib/chr-testdev.o
> > diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> > new file mode 100644
> > index 0000000..1567228
> > --- /dev/null
> > +++ b/lib/pci-testdev.c
> > @@ -0,0 +1,189 @@
> > +/*
> > + * QEMU "pci-testdev" PCI test device
> > + *
> > + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > + */
> > +#include "pci.h"
> > +#include "asm/io.h"
> > +
> > +struct pci_testdev_ops {
> > +	u8 (*read_byte)(const volatile void *addr);
> > +	u16 (*read_word)(const volatile void *addr);
> > +	u32 (*read_long)(const volatile void *addr);
> > +	void (*write_byte)(u8 value, volatile void *addr);
> > +	void (*write_word)(u16 value, volatile void *addr);
> > +	void (*write_long)(u32 value, volatile void *addr);
> > +};
> > +
> > +static u8 ioreadb(const volatile void *addr)
> > +{
> > +	return readb(addr);
> > +}
> > +
> > +static u16 ioreadw(const volatile void *addr)
> > +{
> > +	return readw(addr);
> > +}
> > +
> > +static u32 ioreadl(const volatile void *addr)
> > +{
> > +	return readl(addr);
> > +}
> > +
> > +static void iowriteb(u8 value, volatile void *addr)
> > +{
> > +	writeb(value, addr);
> > +}
> > +
> > +static void iowritew(u16 value, volatile void *addr)
> > +{
> > +	writew(value, addr);
> > +}
> > +
> > +static void iowritel(u32 value, volatile void *addr)
> > +{
> > +	writel(value, addr);
> > +}
> 
> You can't put these MMIO reads and writes in here. How would x86 use it?
> Leave it to the unit test to define read_byte, write_byte, etc. They
> know how their arch works.
> 
> > +
> > +static struct pci_testdev_ops pci_testdev_io_ops = {
> > +	.read_byte	= ioreadb,
> > +	.read_word	= ioreadw,
> > +	.read_long	= ioreadl,
> > +	.write_byte	= iowriteb,
> > +	.write_word	= iowritew,
> > +	.write_long	= iowritel
> > +};
> 
> This should also be left to the unittest to define, and then register
> with the pci-testdev driver.
> 
> > +
> > +static u8 memreadb(const volatile void *addr)
> > +{
> > +	return *(const volatile u8 __force *)addr;
> > +}
> > +
> > +static u16 memreadw(const volatile void *addr)
> > +{
> > +	return *(const volatile u16 __force *)addr;
> > +}
> > +
> > +static u32 memreadl(const volatile void *addr)
> > +{
> > +	return *(const volatile u32 __force *)addr;
> > +}
> > +
> > +static void memwriteb(u8 value, volatile void *addr)
> > +{
> > +	*(volatile u8 __force *)addr = value;
> > +}
> > +
> > +static void memwritew(u16 value, volatile void *addr)
> > +{
> > +	*(volatile u16 __force *)addr = value;
> > +}
> > +
> > +static void memwritel(u32 value, volatile void *addr)
> > +{
> > +	*(volatile u32 __force *)addr = value;
> > +}
> > +
> > +static struct pci_testdev_ops pci_testdev_mem_ops = {
> > +	.read_byte	= memreadb,
> > +	.read_word	= memreadw,
> > +	.read_long	= memreadl,
> > +	.write_byte	= memwriteb,
> > +	.write_word	= memwritew,
> > +	.write_long	= memwritel
> > +};
> 
> These are pretty safe, since they'll most likely be the same
> across arches, but there's no reason to do it either, since
> we don't do the I/O ones.
> 
> So basically just move most of the above into arm/pci-test.c
> in the arm patch.
> 
> > +
> > +static bool pci_testdev_one(struct pci_test_dev_hdr *test,
> > +			    int test_nr,
> > +			    struct pci_testdev_ops *ops)
> > +{
> > +	u8 width;
> > +	u32 count, sig, off;
> > +	const int nr_writes = 16;
> > +	int i;
> > +
> > +	ops->write_byte(test_nr, &test->test);
> > +	count = ops->read_long(&test->count);
> > +	if (count != 0)
> > +		return false;
> > +
> > +	width = ops->read_byte(&test->width);
> > +	if ((width != 1) && (width != 2) && (width != 4))
> > +		return false;
> > +
> > +	sig = ops->read_long(&test->data);
> > +	off = ops->read_long(&test->offset);
> > +
> > +	for (i = 0; i < nr_writes; i++) {
> > +		switch (width) {
> > +			case 1: ops->write_byte(sig, (void*)test + off); break;
> > +			case 2: ops->write_word(sig, (void*)test + off); break;
> > +			case 4: ops->write_long(sig, (void*)test + off); break;
> > +		}
> > +	}
> > +
> > +	if ((int)ops->read_long(&test->count) != nr_writes)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +void pci_testdev_print(struct pci_test_dev_hdr *test,
> > +		       struct pci_testdev_ops *ops)
> 
> Why no 'static'?
> 
> > +{
> > +	bool io = (ops == &pci_testdev_io_ops);
> > +	int i;
> > +
> > +	printf("pci-testdev %3s: ", io ? "io" : "mem");
> > +	for (i = 0;; ++i) {
> > +		char c = ops->read_byte(&test->name[i]);
> > +		if (!c)
> > +			break;
> > +		printf("%c", c);
> > +	}
> > +	printf("\n");
> > +
> > +}
> > +
> > +static int pci_testdev_all(struct pci_test_dev_hdr *test,
> > +			   struct pci_testdev_ops *ops)
> > +{
> > +	int i;
> > +
> > +	for (i = 0;; i++) {
> > +		if (!pci_testdev_one(test, i, ops))
> > +			break;
> > +		pci_testdev_print(test, ops);
> > +	}
> > +
> > +	return i;
> > +}
> > +
> > +int pci_testdev(void)
> 
> So this should take pointers to the two ops structs as arguments.
> 
> > +{
> > +	unsigned long addr;
> > +	void __iomem *mem, *io;
> > +	pcidevaddr_t dev;
> > +	int nr_tests = 0;
> > +
> > +	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> > +	if (dev == PCIDEVADDR_INVALID)
> > +		return -1;
> > +
> > +	addr = pci_bar_addr(dev, 1);
> > +	if (addr == ~0ul)
> > +		return -1;
> > +	io = (void*)addr;
> > +
> > +	addr = pci_bar_addr(dev, 0);
> > +	if (addr == ~0ul)
> > +		return -1;
> > +	mem = ioremap(addr, 0);
> > +
> > +	nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops);
> > +	nr_tests += pci_testdev_all(io, &pci_testdev_io_ops);
> > +
> > +	return nr_tests;
> > +}
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 80d0d04..4a0d305 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -27,7 +27,12 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> >  #define PCI_VENDOR_ID_REDHAT		0x1b36
> >  #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
> >  
> > +/*
> > + * pci-testdev supports at least three types of tests (via mmio and
> > + * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd
> > + */
> >  #define PCI_TESTDEV_NUM_BARS		2
> > +#define PCI_TESTDEV_NUM_TESTS		3
> >  
> >  struct pci_test_dev_hdr {
> >  	uint8_t  test;
> > @@ -41,5 +46,6 @@ struct pci_test_dev_hdr {
> >  
> >  extern bool pci_probe(void);
> >  extern void pci_shutdown(void);
> > +extern int pci_testdev(void);
> >  
> >  #endif /* PCI_H */
> > -- 
> > 1.8.3.1
> >
> 
> This looks pretty good, except for needing to move the MMIO access out.

Wait. Where's the arm/run script changes needed? They should be in the
arm patch of course, but since this version of the series doesn't have
an arm patch, I'd expect them here. We need something like

pci_testdev=
if $qemu $M -device '?' 2>&1 |  grep pci-testdev > /dev/null; then
       pci_testdev="-device pci-testdev"
fi
...
command="$qemu $M -cpu $processor $chr_testdev $pci_testdev"

in arm/run.