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@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> 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@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 (*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.
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
