On Fri, Apr 22, 2016 at 07:23:50PM +0200, Andrew Jones wrote:
> On Mon, Apr 11, 2016 at 01:04:26PM +0200, Alexander Gordeev wrote:
> > Cc: Thomas Huth <thuth@xxxxxxxxxx>
> > Cc: 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..ad89b84ca37d
> > --- /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);
> > +}
>
> Ah, so the above is why you were attempting to add the io-accessor
> wrappers around mmio-accessors to asm-generic. Rather than do that,
I also tried to unify io-accessors across archs Linux style.
> I think we can just allow the unit test to populate
> pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I
> think) like
>
> .io_readb = __raw_readb,
> .io_readw = __raw_readw,
> .io_readl = __raw_readl,
> .io_writeb = __raw_writeb,
> .io_writew = __raw_writew,
> .io_writel = __raw_writel,
>
> while an x86 unit test would use ins and outs.
>
> You could maybe even still provide the ops structs pre-populated,
> but you'll need to use some #ifdef __arm__ type of stuff then.
Out of these two options the latter makes more sense to me, since
type of access to PCI resources is derived from that resource type
rather than from unit test parameters. And that deriving could be
done "automatically".
So I would do the #ifdefs, but that will look like a worsened
version of generic io-accessors ;)
So what is your preference here?
> > +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;
> > + }
> > + }
> > +
> > + if ((int)ops->io_readl(&test->count) != nr_writes)
> > + return false;
> > +
> > + return true;
>
> Or just
>
> return (int)ops->io_readl(&test->count) != nr_writes;
>
> I know you like it :-)
>
> > +}
> > +
> > +void pci_testdev_print(struct pci_test_dev_hdr *test,
> > + struct pci_testdev_ops *ops)
> > +{
> > + bool io = (ops == &pci_testdev_io_ops);
>
> nice :)
>
> > + 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");
> > +
>
> extra blank line here
>
> > +}
> > +
> > +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_addr(dev, 1);
> > + io = (void*)addr;
> > +
> > + addr = pci_bar_addr(dev, 0);
> > + 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 36dd67e19838..03452b059412 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> > void pci_type_desc(int type, char *desc, int len);
> > void pci_print(void);
> >
> > +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
> > @@ -33,7 +35,12 @@ void pci_print(void);
> > #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
>
> you don't seem to use this define anywhere. maybe the next patch
>
> thanks,
> drew
> >
> > struct pci_test_dev_hdr {
> > uint8_t test;
> > --
> > 1.8.3.1
> >
> > --
> > 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
--
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
