From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Subject: Re: [PATCH RFC 14/15] pci: Add pci-testdev PCI bus test device Date: Mon, 23 May 2016 17:17:16 +0200 Message-ID: <20160523151716.ivqzu5f4gscmifbs@hawk.localdomain> References: <20160422172350.6jhc42cohxgiehlb@hawk.localdomain> <20160523080239.GA2051@dhcp-27-118.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Thomas Huth To: Alexander Gordeev Return-path: Received: from mx1.redhat.com ([209.132.183.28]:49454 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753728AbcEWPRV (ORCPT ); Mon, 23 May 2016 11:17:21 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7EACF50F4E for ; Mon, 23 May 2016 15:17:20 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20160523080239.GA2051@dhcp-27-118.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > > > Cc: Andrew Jones > > > Signed-off-by: Alexander Gordeev > > > --- > > > 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 > > > + * > > > + * 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@vger.kernel.org > > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html