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

[Andrew Jones <drjones@redhat.com>]

On Mon, Jun 06, 2016 at 02:46:40PM +0200, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-testdev.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci.h         |   7 +++
>  2 files changed, 191 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..32a4abe117b6
> --- /dev/null
> +++ b/lib/pci-testdev.c
> @@ -0,0 +1,184 @@
> +/*
> + * 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 (*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))

nit: no need for all the ()'s

> +		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;
> +		}
> +	}
> +
> +	return (int)ops->io_readl(&test->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)
> +		return -1;
> +
> +	if (!pci_bar_is_valid(dev, 0) || !pci_bar_is_valid(dev, 1))
> +		return -1;
> +
> +	addr = pci_bar_get_addr(dev, 0);
> +	mem = ioremap(addr, 0);
> +
> +	addr = pci_bar_get_addr(dev, 1);
> +	io = (void *)(unsigned long)addr;
> +
> +	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 37b282169a4e..ee64ab315030 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -39,6 +39,8 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
>  
> +int pci_testdev(void);
> +
>  /*
>   * pci-testdev is a driver for the pci-testdev qemu pci device. The
>   * device enables testing mmio and portio exits, and measuring their
> @@ -47,7 +49,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;
> -- 
> 1.8.3.1

Just one nit. Looks great.

Reviewed-by: Andrew Jones <drjones@redhat.com>