On Mon, May 23, 2016 at 10:02:40AM +0200, Alexander Gordeev wrote:
> 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.
Yeah, but not everything Linux does is nice :-)
>
> > 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?
My latter suggestion, and I think I was wrong about needing ifdefs.
Anyway, let's see how it turns out.
>
> > > +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
--
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
