Re: [kvm-unit-tests PATCH v5 11/12] pci: Add pci-testdev PCI bus test device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jul 28, 2016 at 12:30:43PM +0200, Alexander Gordeev wrote:
> On Thu, Jul 21, 2016 at 10:17:48AM +0200, Andrew Jones wrote:
> > On Tue, Jul 19, 2016 at 02:53:08PM +0200, Alexander Gordeev wrote:
> > > Cc: Thomas Huth <thuth@xxxxxxxxxx>
> > > Cc: Andrew Jones <drjones@xxxxxxxxxx>
> > > Reviewed-by: Andrew Jones <drjones@xxxxxxxxxx>
> > > Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> > > ---
> > >  lib/pci-testdev.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/pci.h         |   7 ++
> > >  2 files changed, 195 insertions(+)
> > >  create mode 100644 lib/pci-testdev.c
> > > 
> > > diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> > > new file mode 100644
> > > index 000000000000..1ee4a3ca0df8
> > > --- /dev/null
> > > +++ b/lib/pci-testdev.c
> > > @@ -0,0 +1,188 @@
> > > +/*
> > > + * QEMU "pci-testdev" PCI test device
> > > + *
> > > + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@xxxxxxxxxx>
> > > + *
> > > + * 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 (*io_readb)(const volatile void *addr);
> > > +	u16 (*io_readw)(const volatile void *addr);
> > > +	u32 (*io_readl)(const volatile void *addr);
> > > +	void (*io_writeb)(u8 value, volatile void *addr);
> > > +	void (*io_writew)(u16 value, volatile void *addr);
> > > +	void (*io_writel)(u32 value, volatile void *addr);
> > > +};
> > > +
> > > +static u8 pio_readb(const volatile void *addr)
> > > +{
> > > +	return inb((unsigned long)addr);
> > > +}
> > > +
> > > +static u16 pio_readw(const volatile void *addr)
> > > +{
> > > +	return inw((unsigned long)addr);
> > > +}
> > > +
> > > +static u32 pio_readl(const volatile void *addr)
> > > +{
> > > +	return inl((unsigned long)addr);
> > > +}
> > > +
> > > +static void pio_writeb(u8 value, volatile void *addr)
> > > +{
> > > +	outb(value, (unsigned long)addr);
> > > +}
> > > +
> > > +static void pio_writew(u16 value, volatile void *addr)
> > > +{
> > > +	outw(value, (unsigned long)addr);
> > > +}
> > > +
> > > +static void pio_writel(u32 value, volatile void *addr)
> > > +{
> > > +	outl(value, (unsigned long)addr);
> > > +}
> > > +
> > > +static struct pci_testdev_ops pci_testdev_io_ops = {
> > > +	.io_readb	= pio_readb,
> > > +	.io_readw	= pio_readw,
> > > +	.io_readl	= pio_readl,
> > > +	.io_writeb	= pio_writeb,
> > > +	.io_writew	= pio_writew,
> > > +	.io_writel	= pio_writel
> > > +};
> > > +
> > > +static u8 mmio_readb(const volatile void *addr)
> > > +{
> > > +	return *(const volatile u8 __force *)addr;
> > > +}
> > > +
> > > +static u16 mmio_readw(const volatile void *addr)
> > > +{
> > > +	return *(const volatile u16 __force *)addr;
> > > +}
> > > +
> > > +static u32 mmio_readl(const volatile void *addr)
> > > +{
> > > +	return *(const volatile u32 __force *)addr;
> > > +}
> > > +
> > > +static void mmio_writeb(u8 value, volatile void *addr)
> > > +{
> > > +	*(volatile u8 __force *)addr = value;
> > > +}
> > > +
> > > +static void mmio_writew(u16 value, volatile void *addr)
> > > +{
> > > +	*(volatile u16 __force *)addr = value;
> > > +}
> > > +
> > > +static void mmio_writel(u32 value, volatile void *addr)
> > > +{
> > > +	*(volatile u32 __force *)addr = value;
> > > +}
> > > +
> > > +static struct pci_testdev_ops pci_testdev_mem_ops = {
> > > +	.io_readb	= mmio_readb,
> > > +	.io_readw	= mmio_readw,
> > > +	.io_readl	= mmio_readl,
> > > +	.io_writeb	= mmio_writeb,
> > > +	.io_writew	= mmio_writew,
> > > +	.io_writel	= mmio_writel
> > > +};
> > > +
> > > +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->io_writeb(test_nr, &test->test);
> > > +	count = ops->io_readl(&test->count);
> > > +	if (count != 0)
> > > +		return false;
> > > +
> > > +	width = ops->io_readb(&test->width);
> > > +	if (width != 1 && width != 2 && width != 4)
> > > +		return false;
> > > +
> > > +	sig = ops->io_readl(&test->data);
> > > +	off = ops->io_readl(&test->offset);
> > > +
> > > +	for (i = 0; i < nr_writes; i++) {
> > > +		switch (width) {
> > > +		case 1: ops->io_writeb(sig, (void *)test + off); break;
> > > +		case 2: ops->io_writew(sig, (void *)test + off); break;
> > > +		case 4: ops->io_writel(sig, (void *)test + off); break;
> > > +		}
> > > +	}
> > > +
> > > +	count = ops->io_readl(&test->count);
> > > +	if (!count)
> > > +		return true;
> > > +
> > > +	return (int)count == nr_writes;
> > > +}
> > > +
> > > +void pci_testdev_print(struct pci_test_dev_hdr *test,
> > > +		       struct pci_testdev_ops *ops)
> > > +{
> > > +	bool io = (ops == &pci_testdev_io_ops);
> > > +	int i;
> > > +
> > > +	printf("pci-testdev %3s: ", io ? "io" : "mem");
> > > +	for (i = 0;; ++i) {
> > > +		char c = ops->io_readb(&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)
> > > +{
> > > +	phys_addr_t 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)
> > 
> > You should output an error message here, saying it's not found. You could
> > suggest how to provide it, i.e. '-device pci-testdev' too.
> > 
> > > +		return -1;
> > > +
> > > +	if (!pci_bar_is_valid(dev, 0) || !pci_bar_is_valid(dev, 1))
> > 
> > Needs error message. Actually, why can this happen? Can it? Shouldn't
> > this just be an assert?
> 
> I think it is a matter of interpretation. If the device does exist,
> but does not report these BARs it could mean (a) device layout failure
> or (b) device/PCI misoperation. The latter is a test failure while
> the former could be both a test failure and a fatal failure. I tend
> treating is as (b).

A single test function (pci_testdev) with multiple failures needs multiple
exit codes. If -1 means unexpected failure, and is used for multiple
conditions, then it's not useful for unit testing. You should create
more exit codes and check them; e.g.

 ret = pci_testdev();
 report("TEST1", ret != ERROR1);
 report("TEST2", ret != ERROR2);
 ...

But, if this test is quite unlikely to fail, and most likely only to fail
if the QEMU pci-testdev code breaks, then I think an assert() is the most
appropriate action.

Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux