[PATCH 0/4] kvm-unittests: add pci PORT IO and MMIO speed tests

[PATCH 0/4] kvm-unittests: add pci PORT IO and MMIO speed tests

[Michael S. Tsirkin]

These patches add a test device, useful to measure speed of MMIO versus PIO, in
different configurations. As I didn't want to reserve a hardcoded range
of memory, I added pci device for this instead.  Used together with the
kvm unittest patches I posted on kvm mailing list.

To use, simply add the device on the pci bus.
Example test output:
	mmio-no-eventfd:pci-mem 8796
	mmio-wildcard-eventfd:pci-mem 3609
	mmio-datamatch-eventfd:pci-mem 3685
	portio-no-eventfd:pci-io 5287
	portio-wildcard-eventfd:pci-io 1762
	portio-datamatch-eventfd:pci-io 1777

First interesting conclusion is that the overhead of MMIO
exit to QEMU as compared to PIO is double that of
MMIO ioeventfd as compared to PIO eventfd. Is this a known fact?

I also had to extend kvm in a minor way, making all ioeventfd
options accessible through the API. This actually needs less code
than checking that users DTRT.

What's the best way to merge this patchset?
I'm guessing the kvm tree ...

Michael S. Tsirkin (4):
  kvm: remove unused APIs
  kvm: support any size for pio eventfd
  kvm: support non datamatch ioeventfd
  pci: add pci test device

 hw/i386/Makefile.objs |   2 +-
 hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci/pci.h          |   1 +
 include/sysemu/kvm.h  |   4 -
 kvm-all.c             | 133 +++++++++++-----------
 kvm-stub.c            |  10 --
 6 files changed, 376 insertions(+), 80 deletions(-)
 create mode 100644 hw/pci-testdev.c

-- 
MST

[PATCH 1/4] kvm: remove unused APIs

[Michael S. Tsirkin]

There are only used internally now, move them
out of header and out of stub.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/sysemu/kvm.h |   4 --
 kvm-all.c            | 107 ++++++++++++++++++++++++++-------------------------
 kvm-stub.c           |  10 -----
 3 files changed, 54 insertions(+), 67 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index f2d97b5..4a65d9f 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -283,10 +283,6 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
 #endif
 
 #endif
-int kvm_set_ioeventfd_mmio(int fd, uint32_t adr, uint32_t val, bool assign,
-                           uint32_t size);
-
-int kvm_set_ioeventfd_pio_word(int fd, uint16_t adr, uint16_t val, bool assign);
 
 int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg);
 int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg);
diff --git a/kvm-all.c b/kvm-all.c
index 9b433d3..ca9775d 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -500,6 +500,60 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
     return ret;
 }
 
+static int kvm_set_ioeventfd_mmio(int fd, uint32_t addr, uint32_t val, bool assign,
+                                  uint32_t size)
+{
+    int ret;
+    struct kvm_ioeventfd iofd;
+
+    iofd.datamatch = val;
+    iofd.addr = addr;
+    iofd.len = size;
+    iofd.flags = KVM_IOEVENTFD_FLAG_DATAMATCH;
+    iofd.fd = fd;
+
+    if (!kvm_enabled()) {
+        return -ENOSYS;
+    }
+
+    if (!assign) {
+        iofd.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
+    }
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &iofd);
+
+    if (ret < 0) {
+        return -errno;
+    }
+
+    return 0;
+}
+
+static int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val,
+                                      bool assign)
+{
+    struct kvm_ioeventfd kick = {
+        .datamatch = val,
+        .addr = addr,
+        .len = 2,
+        .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
+        .fd = fd,
+    };
+    int r;
+    if (!kvm_enabled()) {
+        return -ENOSYS;
+    }
+    if (!assign) {
+        kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
+    }
+    r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick);
+    if (r < 0) {
+        return r;
+    }
+    return 0;
+}
+
+
 static int kvm_check_many_ioeventfds(void)
 {
     /* Userspace can use ioeventfd for io notification.  This requires a host
@@ -1971,59 +2025,6 @@ int kvm_set_signal_mask(CPUArchState *env, const sigset_t *sigset)
 
     return r;
 }
-
-int kvm_set_ioeventfd_mmio(int fd, uint32_t addr, uint32_t val, bool assign,
-                           uint32_t size)
-{
-    int ret;
-    struct kvm_ioeventfd iofd;
-
-    iofd.datamatch = val;
-    iofd.addr = addr;
-    iofd.len = size;
-    iofd.flags = KVM_IOEVENTFD_FLAG_DATAMATCH;
-    iofd.fd = fd;
-
-    if (!kvm_enabled()) {
-        return -ENOSYS;
-    }
-
-    if (!assign) {
-        iofd.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
-    }
-
-    ret = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &iofd);
-
-    if (ret < 0) {
-        return -errno;
-    }
-
-    return 0;
-}
-
-int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val, bool assign)
-{
-    struct kvm_ioeventfd kick = {
-        .datamatch = val,
-        .addr = addr,
-        .len = 2,
-        .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
-        .fd = fd,
-    };
-    int r;
-    if (!kvm_enabled()) {
-        return -ENOSYS;
-    }
-    if (!assign) {
-        kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
-    }
-    r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick);
-    if (r < 0) {
-        return r;
-    }
-    return 0;
-}
-
 int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
 {
     return kvm_arch_on_sigbus_vcpu(cpu, code, addr);
diff --git a/kvm-stub.c b/kvm-stub.c
index 760aadc..ef1f201 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -102,16 +102,6 @@ int kvm_set_signal_mask(CPUArchState *env, const sigset_t *sigset)
 }
 #endif
 
-int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val, bool assign)
-{
-    return -ENOSYS;
-}
-
-int kvm_set_ioeventfd_mmio(int fd, uint32_t adr, uint32_t val, bool assign, uint32_t len)
-{
-    return -ENOSYS;
-}
-
 int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
 {
     return 1;
-- 
MST

[PATCH 2/4] kvm: support any size for pio eventfd

[Michael S. Tsirkin]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 kvm-all.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index ca9775d..589e37c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -500,8 +500,8 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
     return ret;
 }
 
-static int kvm_set_ioeventfd_mmio(int fd, uint32_t addr, uint32_t val, bool assign,
-                                  uint32_t size)
+static int kvm_set_ioeventfd_mmio(int fd, uint32_t addr, uint32_t val,
+                                  bool assign, uint32_t size)
 {
     int ret;
     struct kvm_ioeventfd iofd;
@@ -529,13 +529,13 @@ static int kvm_set_ioeventfd_mmio(int fd, uint32_t addr, uint32_t val, bool assi
     return 0;
 }
 
-static int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val,
-                                      bool assign)
+static int kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint16_t val,
+                                 bool assign, uint32_t size)
 {
     struct kvm_ioeventfd kick = {
         .datamatch = val,
         .addr = addr,
-        .len = 2,
+        .len = size,
         .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
         .fd = fd,
     };
@@ -571,7 +571,7 @@ static int kvm_check_many_ioeventfds(void)
         if (ioeventfds[i] < 0) {
             break;
         }
-        ret = kvm_set_ioeventfd_pio_word(ioeventfds[i], 0, i, true);
+        ret = kvm_set_ioeventfd_pio(ioeventfds[i], 0, i, true, 2);
         if (ret < 0) {
             close(ioeventfds[i]);
             break;
@@ -582,7 +582,7 @@ static int kvm_check_many_ioeventfds(void)
     ret = i == ARRAY_SIZE(ioeventfds);
 
     while (i-- > 0) {
-        kvm_set_ioeventfd_pio_word(ioeventfds[i], 0, i, false);
+        kvm_set_ioeventfd_pio(ioeventfds[i], 0, i, false, 2);
         close(ioeventfds[i]);
     }
     return ret;
@@ -834,10 +834,10 @@ static void kvm_io_ioeventfd_add(MemoryListener *listener,
     int fd = event_notifier_get_fd(e);
     int r;
 
-    assert(match_data && section->size == 2);
+    assert(match_data && section->size <= 8);
 
-    r = kvm_set_ioeventfd_pio_word(fd, section->offset_within_address_space,
-                                   data, true);
+    r = kvm_set_ioeventfd_pio(fd, section->offset_within_address_space,
+                              data, true, section->size);
     if (r < 0) {
         abort();
     }
@@ -852,8 +852,8 @@ static void kvm_io_ioeventfd_del(MemoryListener *listener,
     int fd = event_notifier_get_fd(e);
     int r;
 
-    r = kvm_set_ioeventfd_pio_word(fd, section->offset_within_address_space,
-                                   data, false);
+    r = kvm_set_ioeventfd_pio(fd, section->offset_within_address_space,
+                              data, false, section->size);
     if (r < 0) {
         abort();
     }
-- 
MST

[PATCH 3/4] kvm: support non datamatch ioeventfd

[Michael S. Tsirkin]

Adding restrictions just adds code.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 kvm-all.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 589e37c..ce823f9 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -501,21 +501,24 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
 }
 
 static int kvm_set_ioeventfd_mmio(int fd, uint32_t addr, uint32_t val,
-                                  bool assign, uint32_t size)
+                                  bool assign, uint32_t size, bool datamatch)
 {
     int ret;
     struct kvm_ioeventfd iofd;
 
-    iofd.datamatch = val;
+    iofd.datamatch = datamatch ? val : 0;
     iofd.addr = addr;
     iofd.len = size;
-    iofd.flags = KVM_IOEVENTFD_FLAG_DATAMATCH;
+    iofd.flags = 0;
     iofd.fd = fd;
 
     if (!kvm_enabled()) {
         return -ENOSYS;
     }
 
+    if (datamatch) {
+        iofd.flags |= KVM_IOEVENTFD_FLAG_DATAMATCH;
+    }
     if (!assign) {
         iofd.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
     }
@@ -530,19 +533,22 @@ static int kvm_set_ioeventfd_mmio(int fd, uint32_t addr, uint32_t val,
 }
 
 static int kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint16_t val,
-                                 bool assign, uint32_t size)
+                                 bool assign, uint32_t size, bool datamatch)
 {
     struct kvm_ioeventfd kick = {
-        .datamatch = val,
+        .datamatch = datamatch ? val : 0,
         .addr = addr,
+        .flags = KVM_IOEVENTFD_FLAG_PIO,
         .len = size,
-        .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
         .fd = fd,
     };
     int r;
     if (!kvm_enabled()) {
         return -ENOSYS;
     }
+    if (datamatch) {
+        kick.flags |= KVM_IOEVENTFD_FLAG_DATAMATCH;
+    }
     if (!assign) {
         kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
     }
@@ -571,7 +577,7 @@ static int kvm_check_many_ioeventfds(void)
         if (ioeventfds[i] < 0) {
             break;
         }
-        ret = kvm_set_ioeventfd_pio(ioeventfds[i], 0, i, true, 2);
+        ret = kvm_set_ioeventfd_pio(ioeventfds[i], 0, i, true, 2, true);
         if (ret < 0) {
             close(ioeventfds[i]);
             break;
@@ -582,7 +588,7 @@ static int kvm_check_many_ioeventfds(void)
     ret = i == ARRAY_SIZE(ioeventfds);
 
     while (i-- > 0) {
-        kvm_set_ioeventfd_pio(ioeventfds[i], 0, i, false, 2);
+        kvm_set_ioeventfd_pio(ioeventfds[i], 0, i, false, 2, true);
         close(ioeventfds[i]);
     }
     return ret;
@@ -802,10 +808,8 @@ static void kvm_mem_ioeventfd_add(MemoryListener *listener,
     int fd = event_notifier_get_fd(e);
     int r;
 
-    assert(match_data && section->size <= 8);
-
     r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space,
-                               data, true, section->size);
+                               data, true, section->size, match_data);
     if (r < 0) {
         abort();
     }
@@ -820,7 +824,7 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener,
     int r;
 
     r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space,
-                               data, false, section->size);
+                               data, false, section->size, match_data);
     if (r < 0) {
         abort();
     }
@@ -834,10 +838,8 @@ static void kvm_io_ioeventfd_add(MemoryListener *listener,
     int fd = event_notifier_get_fd(e);
     int r;
 
-    assert(match_data && section->size <= 8);
-
     r = kvm_set_ioeventfd_pio(fd, section->offset_within_address_space,
-                              data, true, section->size);
+                              data, true, section->size, match_data);
     if (r < 0) {
         abort();
     }
@@ -853,7 +855,7 @@ static void kvm_io_ioeventfd_del(MemoryListener *listener,
     int r;
 
     r = kvm_set_ioeventfd_pio(fd, section->offset_within_address_space,
-                              data, false, section->size);
+                              data, false, section->size, match_data);
     if (r < 0) {
         abort();
     }
-- 
MST

[PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

This device is used for kvm unit tests,
currently it supports testing performance of ioeventfd.
Using updated kvm unittest, here's an example output:
        mmio-no-eventfd:pci-mem 8796
        mmio-wildcard-eventfd:pci-mem 3609
        mmio-datamatch-eventfd:pci-mem 3685
        portio-no-eventfd:pci-io 5287
        portio-wildcard-eventfd:pci-io 1762
        portio-datamatch-eventfd:pci-io 1777

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/Makefile.objs |   2 +-
 hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci/pci.h          |   1 +
 3 files changed, 308 insertions(+), 1 deletion(-)
 create mode 100644 hw/pci-testdev.c

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index a78c0b2..7497e7a 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -11,7 +11,7 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
 obj-y += kvm/
 obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
-obj-y += pc-testdev.o
+obj-y += pc-testdev.o pci-testdev.o
 
 obj-y := $(addprefix ../,$(obj-y))
 
diff --git a/hw/pci-testdev.c b/hw/pci-testdev.c
new file mode 100644
index 0000000..9486624
--- /dev/null
+++ b/hw/pci-testdev.c
@@ -0,0 +1,306 @@
+#include "hw/hw.h"
+#include "hw/pci/pci.h"
+#include "qemu/event_notifier.h"
+#include "qemu/osdep.h"
+
+typedef struct PCITestDevHdr {
+    uint8_t test;
+    uint8_t width;
+    uint8_t pad0[2];
+    uint32_t offset;
+    uint8_t data;
+    uint8_t pad1[3];
+    uint32_t count;
+    uint8_t name[];
+} PCITestDevHdr;
+
+typedef struct IOTest {
+    MemoryRegion *mr;
+    EventNotifier notifier;
+    bool hasnotifier;
+    unsigned size;
+    bool match_data;
+    PCITestDevHdr *hdr;
+    unsigned bufsize;
+} IOTest;
+
+#define IOTEST_DATAMATCH 0xFA
+#define IOTEST_NOMATCH   0xCE
+
+#define IOTEST_IOSIZE 128
+#define IOTEST_MEMSIZE 2048
+
+static const char *iotest_test[] = {
+    "no-eventfd",
+    "wildcard-eventfd",
+    "datamatch-eventfd"
+};
+
+static const char *iotest_type[] = {
+    "mmio",
+    "portio"
+};
+
+#define IOTEST_TEST(i) (iotest_test[((i) % ARRAY_SIZE(iotest_test))])
+#define IOTEST_TYPE(i) (iotest_type[((i) / ARRAY_SIZE(iotest_test))])
+#define IOTEST_MAX_TEST (ARRAY_SIZE(iotest_test))
+#define IOTEST_MAX_TYPE (ARRAY_SIZE(iotest_type))
+#define IOTEST_MAX (IOTEST_MAX_TEST * IOTEST_MAX_TYPE)
+
+enum {
+    IOTEST_ACCESS_NAME,
+    IOTEST_ACCESS_DATA,
+    IOTEST_ACCESS_MAX,
+};
+
+#define IOTEST_ACCESS_TYPE uint8_t
+#define IOTEST_ACCESS_WIDTH (sizeof(uint8_t))
+
+typedef struct PCITestDevState {
+    PCIDevice dev;
+    MemoryRegion mmio;
+    MemoryRegion portio;
+    IOTest *tests;
+    int current;
+} PCITestDevState;
+
+#define IOTEST_IS_MEM(i) (strcmp(IOTEST_TYPE(i), "portio"))
+#define IOTEST_REGION(d, i) (IOTEST_IS_MEM(i) ?  &(d)->mmio : &(d)->portio)
+#define IOTEST_SIZE(i) (IOTEST_IS_MEM(i) ? IOTEST_MEMSIZE : IOTEST_IOSIZE)
+#define IOTEST_PCI_BAR(i) (IOTEST_IS_MEM(i) ? PCI_BASE_ADDRESS_SPACE_MEMORY : \
+                           PCI_BASE_ADDRESS_SPACE_IO)
+
+static int pci_testdev_start(IOTest *test)
+{
+    test->hdr->count = 0;
+    if (!test->hasnotifier) {
+        return 0;
+    }
+    event_notifier_test_and_clear(&test->notifier);
+    memory_region_add_eventfd(test->mr,
+                              le32_to_cpu(test->hdr->offset),
+                              test->size,
+                              test->match_data,
+                              test->hdr->data,
+                              &test->notifier);
+    return 0;
+}
+
+static void pci_testdev_stop(IOTest *test)
+{
+    if (!test->hasnotifier) {
+        return;
+    }
+    memory_region_del_eventfd(test->mr,
+                              le32_to_cpu(test->hdr->offset),
+                              test->size,
+                              test->match_data,
+                              test->hdr->data,
+                              &test->notifier);
+}
+
+static void
+pci_testdev_reset(PCITestDevState *d)
+{
+    if (d->current == -1) {
+        return;
+    }
+    pci_testdev_stop(&d->tests[d->current]);
+    d->current = -1;
+}
+
+static void pci_testdev_inc(IOTest *test, unsigned inc)
+{
+    uint32_t c = le32_to_cpu(test->hdr->count);
+    test->hdr->count = cpu_to_le32(c + inc);
+}
+
+static void
+pci_testdev_write(void *opaque, hwaddr addr, uint64_t val,
+                  unsigned size, int type)
+{
+    PCITestDevState *d = opaque;
+    IOTest *test;
+    int t, r;
+
+    if (addr == offsetof(PCITestDevHdr, test)) {
+        pci_testdev_reset(d);
+        if (val >= IOTEST_MAX_TEST) {
+            return;
+        }
+        t = type * IOTEST_MAX_TEST + val;
+        r = pci_testdev_start(&d->tests[t]);
+        if (r < 0) {
+            return;
+        }
+        d->current = t;
+        return;
+    }
+    if (d->current < 0) {
+        return;
+    }
+    test = &d->tests[d->current];
+    if (addr != le32_to_cpu(test->hdr->offset)) {
+        return;
+    }
+    if (test->match_data && test->size != size) {
+        return;
+    }
+    if (test->match_data && val != test->hdr->data) {
+        return;
+    }
+    pci_testdev_inc(test, 1);
+}
+
+static uint64_t
+pci_testdev_read(void *opaque, hwaddr addr, unsigned size)
+{
+    PCITestDevState *d = opaque;
+    const char *buf;
+    IOTest *test;
+    if (d->current < 0) {
+        return 0;
+    }
+    test = &d->tests[d->current];
+    buf = (const char *)test->hdr;
+    if (addr + size >= test->bufsize) {
+        return 0;
+    }
+    if (test->hasnotifier) {
+        event_notifier_test_and_clear(&test->notifier);
+    }
+    return buf[addr];
+}
+
+static void
+pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
+                       unsigned size)
+{
+    pci_testdev_write(opaque, addr, val, size, 0);
+}
+
+static void
+pci_testdev_pio_write(void *opaque, hwaddr addr, uint64_t val,
+                       unsigned size)
+{
+    pci_testdev_write(opaque, addr, val, size, 1);
+}
+
+static const MemoryRegionOps pci_testdev_mmio_ops = {
+    .read = pci_testdev_read,
+    .write = pci_testdev_mmio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static const MemoryRegionOps pci_testdev_pio_ops = {
+    .read = pci_testdev_read,
+    .write = pci_testdev_pio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static int pci_testdev_init(PCIDevice *pci_dev)
+{
+    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, pci_dev);
+    uint8_t *pci_conf;
+    char *name;
+    int r, i;
+
+    pci_conf = d->dev.config;
+
+    pci_conf[PCI_INTERRUPT_PIN] = 0; /* no interrupt pin */
+
+    memory_region_init_io(&d->mmio, &pci_testdev_mmio_ops, d,
+                          "pci-testdev-mmio", IOTEST_MEMSIZE * 2);
+    memory_region_init_io(&d->portio, &pci_testdev_pio_ops, d,
+                          "pci-testdev-portio", IOTEST_IOSIZE * 2);
+    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
+    pci_register_bar(&d->dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
+
+    d->current = -1;
+    d->tests = g_malloc0(IOTEST_MAX * sizeof *d->tests);
+    for (i = 0; i < IOTEST_MAX; ++i) {
+        IOTest *test = &d->tests[i];
+        name = g_strdup_printf("%s-%s", IOTEST_TYPE(i), IOTEST_TEST(i));
+        test->bufsize = sizeof(PCITestDevHdr) + strlen(name) + 1;
+        test->hdr = g_malloc0(test->bufsize);
+        memcpy(test->hdr->name, name, strlen(name) + 1);
+        g_free(name);
+        test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
+        test->size = IOTEST_ACCESS_WIDTH;
+        test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
+        test->hdr->test = i;
+        test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
+        test->hdr->width = IOTEST_ACCESS_WIDTH;
+        test->mr = IOTEST_REGION(d, i);
+        if (!strcmp(IOTEST_TEST(i), "no-eventfd")) {
+            test->hasnotifier = false;
+            continue;
+        }
+        r = event_notifier_init(&test->notifier, 0);
+        assert(r >= 0);
+        test->hasnotifier = true;
+    }
+
+    return 0;
+}
+
+static void
+pci_testdev_uninit(PCIDevice *dev)
+{
+    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
+    int i;
+
+    pci_testdev_reset(d);
+    for (i = 0; i < IOTEST_MAX; ++i) {
+        if (d->tests[i].hasnotifier) {
+            event_notifier_cleanup(&d->tests[i].notifier);
+        }
+        g_free(d->tests[i].hdr);
+    }
+    g_free(d->tests);
+    memory_region_destroy(&d->mmio);
+    memory_region_destroy(&d->portio);
+}
+
+static void qdev_pci_testdev_reset(DeviceState *dev)
+{
+    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev.qdev, dev);
+    pci_testdev_reset(d);
+}
+
+static void pci_testdev_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = pci_testdev_init;
+    k->exit = pci_testdev_uninit;
+    k->vendor_id = PCI_VENDOR_ID_REDHAT;
+    k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
+    k->revision = 0x00;
+    k->class_id = PCI_CLASS_OTHERS;
+    dc->desc = "PCI Test Device";
+    dc->reset = qdev_pci_testdev_reset;
+}
+
+static const TypeInfo pci_testdev_info = {
+    .name          = "pci-testdev",
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCITestDevState),
+    .class_init    = pci_testdev_class_init,
+};
+
+static void pci_testdev_register_types(void)
+{
+    type_register_static(&pci_testdev_info);
+}
+
+type_init(pci_testdev_register_types)
diff --git a/hw/pci/pci.h b/hw/pci/pci.h
index 774369c..d81198c 100644
--- a/hw/pci/pci.h
+++ b/hw/pci/pci.h
@@ -84,6 +84,7 @@
 #define PCI_DEVICE_ID_REDHAT_SERIAL      0x0002
 #define PCI_DEVICE_ID_REDHAT_SERIAL2     0x0003
 #define PCI_DEVICE_ID_REDHAT_SERIAL4     0x0004
+#define PCI_DEVICE_ID_REDHAT_TEST        0x0005
 #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
 
 #define FMT_PCIBUS                      PRIx64
-- 
MST

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 10:59, Michael S. Tsirkin ha scritto:
> This device is used for kvm unit tests,
> currently it supports testing performance of ioeventfd.
> Using updated kvm unittest, here's an example output:
>         mmio-no-eventfd:pci-mem 8796
>         mmio-wildcard-eventfd:pci-mem 3609
>         mmio-datamatch-eventfd:pci-mem 3685
>         portio-no-eventfd:pci-io 5287
>         portio-wildcard-eventfd:pci-io 1762
>         portio-datamatch-eventfd:pci-io 1777

There is too much magic in this device, that is shared between the
testcase and the device.  I think this device should be structured
differently.  For example, you could have a single EventNotifier and 3 BARs:

1) BAR0/BAR1 are as in your device.  All they do is count writes and
report them into a register in BAR2.

2) BAR2 has a control register for BAR0 and one for BAR1, that
enables/disables ioeventfd (bit 0 = enable/disable, bit 1 =
wildcard/datamatch, bits 2-3 = log2(width)).  It also has a zero-on-read
counter that is incremented for each write to BAR0 or BAR1, and clears
the EventNotifier.

Paolo

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/Makefile.objs |   2 +-
>  hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci/pci.h          |   1 +
>  3 files changed, 308 insertions(+), 1 deletion(-)
>  create mode 100644 hw/pci-testdev.c
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index a78c0b2..7497e7a 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -11,7 +11,7 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
>  obj-y += kvm/
>  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> -obj-y += pc-testdev.o
> +obj-y += pc-testdev.o pci-testdev.o
>  
>  obj-y := $(addprefix ../,$(obj-y))
>  
> diff --git a/hw/pci-testdev.c b/hw/pci-testdev.c
> new file mode 100644
> index 0000000..9486624
> --- /dev/null
> +++ b/hw/pci-testdev.c
> @@ -0,0 +1,306 @@
> +#include "hw/hw.h"
> +#include "hw/pci/pci.h"
> +#include "qemu/event_notifier.h"
> +#include "qemu/osdep.h"
> +
> +typedef struct PCITestDevHdr {
> +    uint8_t test;
> +    uint8_t width;
> +    uint8_t pad0[2];
> +    uint32_t offset;
> +    uint8_t data;
> +    uint8_t pad1[3];
> +    uint32_t count;
> +    uint8_t name[];
> +} PCITestDevHdr;
> +
> +typedef struct IOTest {
> +    MemoryRegion *mr;
> +    EventNotifier notifier;
> +    bool hasnotifier;
> +    unsigned size;
> +    bool match_data;
> +    PCITestDevHdr *hdr;
> +    unsigned bufsize;
> +} IOTest;
> +
> +#define IOTEST_DATAMATCH 0xFA
> +#define IOTEST_NOMATCH   0xCE
> +
> +#define IOTEST_IOSIZE 128
> +#define IOTEST_MEMSIZE 2048
> +
> +static const char *iotest_test[] = {
> +    "no-eventfd",
> +    "wildcard-eventfd",
> +    "datamatch-eventfd"
> +};
> +
> +static const char *iotest_type[] = {
> +    "mmio",
> +    "portio"
> +};
> +
> +#define IOTEST_TEST(i) (iotest_test[((i) % ARRAY_SIZE(iotest_test))])
> +#define IOTEST_TYPE(i) (iotest_type[((i) / ARRAY_SIZE(iotest_test))])
> +#define IOTEST_MAX_TEST (ARRAY_SIZE(iotest_test))
> +#define IOTEST_MAX_TYPE (ARRAY_SIZE(iotest_type))
> +#define IOTEST_MAX (IOTEST_MAX_TEST * IOTEST_MAX_TYPE)
> +
> +enum {
> +    IOTEST_ACCESS_NAME,
> +    IOTEST_ACCESS_DATA,
> +    IOTEST_ACCESS_MAX,
> +};
> +
> +#define IOTEST_ACCESS_TYPE uint8_t
> +#define IOTEST_ACCESS_WIDTH (sizeof(uint8_t))
> +
> +typedef struct PCITestDevState {
> +    PCIDevice dev;
> +    MemoryRegion mmio;
> +    MemoryRegion portio;
> +    IOTest *tests;
> +    int current;
> +} PCITestDevState;
> +
> +#define IOTEST_IS_MEM(i) (strcmp(IOTEST_TYPE(i), "portio"))
> +#define IOTEST_REGION(d, i) (IOTEST_IS_MEM(i) ?  &(d)->mmio : &(d)->portio)
> +#define IOTEST_SIZE(i) (IOTEST_IS_MEM(i) ? IOTEST_MEMSIZE : IOTEST_IOSIZE)
> +#define IOTEST_PCI_BAR(i) (IOTEST_IS_MEM(i) ? PCI_BASE_ADDRESS_SPACE_MEMORY : \
> +                           PCI_BASE_ADDRESS_SPACE_IO)
> +
> +static int pci_testdev_start(IOTest *test)
> +{
> +    test->hdr->count = 0;
> +    if (!test->hasnotifier) {
> +        return 0;
> +    }
> +    event_notifier_test_and_clear(&test->notifier);
> +    memory_region_add_eventfd(test->mr,
> +                              le32_to_cpu(test->hdr->offset),
> +                              test->size,
> +                              test->match_data,
> +                              test->hdr->data,
> +                              &test->notifier);
> +    return 0;
> +}
> +
> +static void pci_testdev_stop(IOTest *test)
> +{
> +    if (!test->hasnotifier) {
> +        return;
> +    }
> +    memory_region_del_eventfd(test->mr,
> +                              le32_to_cpu(test->hdr->offset),
> +                              test->size,
> +                              test->match_data,
> +                              test->hdr->data,
> +                              &test->notifier);
> +}
> +
> +static void
> +pci_testdev_reset(PCITestDevState *d)
> +{
> +    if (d->current == -1) {
> +        return;
> +    }
> +    pci_testdev_stop(&d->tests[d->current]);
> +    d->current = -1;
> +}
> +
> +static void pci_testdev_inc(IOTest *test, unsigned inc)
> +{
> +    uint32_t c = le32_to_cpu(test->hdr->count);
> +    test->hdr->count = cpu_to_le32(c + inc);
> +}
> +
> +static void
> +pci_testdev_write(void *opaque, hwaddr addr, uint64_t val,
> +                  unsigned size, int type)
> +{
> +    PCITestDevState *d = opaque;
> +    IOTest *test;
> +    int t, r;
> +
> +    if (addr == offsetof(PCITestDevHdr, test)) {
> +        pci_testdev_reset(d);
> +        if (val >= IOTEST_MAX_TEST) {
> +            return;
> +        }
> +        t = type * IOTEST_MAX_TEST + val;
> +        r = pci_testdev_start(&d->tests[t]);
> +        if (r < 0) {
> +            return;
> +        }
> +        d->current = t;
> +        return;
> +    }
> +    if (d->current < 0) {
> +        return;
> +    }
> +    test = &d->tests[d->current];
> +    if (addr != le32_to_cpu(test->hdr->offset)) {
> +        return;
> +    }
> +    if (test->match_data && test->size != size) {
> +        return;
> +    }
> +    if (test->match_data && val != test->hdr->data) {
> +        return;
> +    }
> +    pci_testdev_inc(test, 1);
> +}
> +
> +static uint64_t
> +pci_testdev_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCITestDevState *d = opaque;
> +    const char *buf;
> +    IOTest *test;
> +    if (d->current < 0) {
> +        return 0;
> +    }
> +    test = &d->tests[d->current];
> +    buf = (const char *)test->hdr;
> +    if (addr + size >= test->bufsize) {
> +        return 0;
> +    }
> +    if (test->hasnotifier) {
> +        event_notifier_test_and_clear(&test->notifier);
> +    }
> +    return buf[addr];
> +}
> +
> +static void
> +pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
> +                       unsigned size)
> +{
> +    pci_testdev_write(opaque, addr, val, size, 0);
> +}
> +
> +static void
> +pci_testdev_pio_write(void *opaque, hwaddr addr, uint64_t val,
> +                       unsigned size)
> +{
> +    pci_testdev_write(opaque, addr, val, size, 1);
> +}
> +
> +static const MemoryRegionOps pci_testdev_mmio_ops = {
> +    .read = pci_testdev_read,
> +    .write = pci_testdev_mmio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static const MemoryRegionOps pci_testdev_pio_ops = {
> +    .read = pci_testdev_read,
> +    .write = pci_testdev_pio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static int pci_testdev_init(PCIDevice *pci_dev)
> +{
> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, pci_dev);
> +    uint8_t *pci_conf;
> +    char *name;
> +    int r, i;
> +
> +    pci_conf = d->dev.config;
> +
> +    pci_conf[PCI_INTERRUPT_PIN] = 0; /* no interrupt pin */
> +
> +    memory_region_init_io(&d->mmio, &pci_testdev_mmio_ops, d,
> +                          "pci-testdev-mmio", IOTEST_MEMSIZE * 2);
> +    memory_region_init_io(&d->portio, &pci_testdev_pio_ops, d,
> +                          "pci-testdev-portio", IOTEST_IOSIZE * 2);
> +    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> +    pci_register_bar(&d->dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> +
> +    d->current = -1;
> +    d->tests = g_malloc0(IOTEST_MAX * sizeof *d->tests);
> +    for (i = 0; i < IOTEST_MAX; ++i) {
> +        IOTest *test = &d->tests[i];
> +        name = g_strdup_printf("%s-%s", IOTEST_TYPE(i), IOTEST_TEST(i));
> +        test->bufsize = sizeof(PCITestDevHdr) + strlen(name) + 1;
> +        test->hdr = g_malloc0(test->bufsize);
> +        memcpy(test->hdr->name, name, strlen(name) + 1);
> +        g_free(name);
> +        test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
> +        test->size = IOTEST_ACCESS_WIDTH;
> +        test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
> +        test->hdr->test = i;
> +        test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
> +        test->hdr->width = IOTEST_ACCESS_WIDTH;
> +        test->mr = IOTEST_REGION(d, i);
> +        if (!strcmp(IOTEST_TEST(i), "no-eventfd")) {
> +            test->hasnotifier = false;
> +            continue;
> +        }
> +        r = event_notifier_init(&test->notifier, 0);
> +        assert(r >= 0);
> +        test->hasnotifier = true;
> +    }
> +
> +    return 0;
> +}
> +
> +static void
> +pci_testdev_uninit(PCIDevice *dev)
> +{
> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
> +    int i;
> +
> +    pci_testdev_reset(d);
> +    for (i = 0; i < IOTEST_MAX; ++i) {
> +        if (d->tests[i].hasnotifier) {
> +            event_notifier_cleanup(&d->tests[i].notifier);
> +        }
> +        g_free(d->tests[i].hdr);
> +    }
> +    g_free(d->tests);
> +    memory_region_destroy(&d->mmio);
> +    memory_region_destroy(&d->portio);
> +}
> +
> +static void qdev_pci_testdev_reset(DeviceState *dev)
> +{
> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev.qdev, dev);
> +    pci_testdev_reset(d);
> +}
> +
> +static void pci_testdev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = pci_testdev_init;
> +    k->exit = pci_testdev_uninit;
> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
> +    k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
> +    k->revision = 0x00;
> +    k->class_id = PCI_CLASS_OTHERS;
> +    dc->desc = "PCI Test Device";
> +    dc->reset = qdev_pci_testdev_reset;
> +}
> +
> +static const TypeInfo pci_testdev_info = {
> +    .name          = "pci-testdev",
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PCITestDevState),
> +    .class_init    = pci_testdev_class_init,
> +};
> +
> +static void pci_testdev_register_types(void)
> +{
> +    type_register_static(&pci_testdev_info);
> +}
> +
> +type_init(pci_testdev_register_types)
> diff --git a/hw/pci/pci.h b/hw/pci/pci.h
> index 774369c..d81198c 100644
> --- a/hw/pci/pci.h
> +++ b/hw/pci/pci.h
> @@ -84,6 +84,7 @@
>  #define PCI_DEVICE_ID_REDHAT_SERIAL      0x0002
>  #define PCI_DEVICE_ID_REDHAT_SERIAL2     0x0003
>  #define PCI_DEVICE_ID_REDHAT_SERIAL4     0x0004
> +#define PCI_DEVICE_ID_REDHAT_TEST        0x0005
>  #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
>  
>  #define FMT_PCIBUS                      PRIx64
> 

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 11:28:55AM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 10:59, Michael S. Tsirkin ha scritto:
> > This device is used for kvm unit tests,
> > currently it supports testing performance of ioeventfd.
> > Using updated kvm unittest, here's an example output:
> >         mmio-no-eventfd:pci-mem 8796
> >         mmio-wildcard-eventfd:pci-mem 3609
> >         mmio-datamatch-eventfd:pci-mem 3685
> >         portio-no-eventfd:pci-io 5287
> >         portio-wildcard-eventfd:pci-io 1762
> >         portio-datamatch-eventfd:pci-io 1777
> 
> There is too much magic in this device, that is shared between the
> testcase and the device.

Paolo, it looks like there's some misunderstanding.
There's no magic. Each BAR has the structure specified
in both test and qemu:

struct pci_test_dev_hdr {
    uint8_t test;       -- test number
    uint8_t width;      - how much to write
    uint8_t pad0[2];
    uint32_t offset;    - where to write
    uint32_t data;     - what to write
    uint32_t count;    - incremented on write
    uint8_t name[];   -- test name for debugging
};

What you propose below has a bit less features
if you add these you will get pretty much same thing.

>  I think this device should be structured
> differently.  For example, you could have a single EventNotifier and 3 BARs:
> 
> 1) BAR0/BAR1 are as in your device.  All they do is count writes and
> report them into a register in BAR2.
> 
> 2) BAR2 has a control register for BAR0 and one for BAR1, that
> enables/disables ioeventfd (bit 0 = enable/disable, bit 1 =
> wildcard/datamatch, bits 2-3 = log2(width)).  It also has a zero-on-read
> counter that is incremented for each write to BAR0 or BAR1, and clears
> the EventNotifier.
> 
> Paolo

This is pretty close to what I have, only I put control
at the beginning of each BAR, and I support arbitrary
length writes guest side, and I allow testing both
datamatch and non data match.

I also do not want guest to control things like ioeventfd
assignment. These are host side things and while you could
argue security does not matter for a test device,
I think it will set a bad example.

For this reason, this is structured like virtio:
host tells guest what to write and where.

So any new benchmark can be added pretty much on qemu
side without changing test.


> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/i386/Makefile.objs |   2 +-
> >  hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/pci/pci.h          |   1 +
> >  3 files changed, 308 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/pci-testdev.c
> > 
> > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> > index a78c0b2..7497e7a 100644
> > --- a/hw/i386/Makefile.objs
> > +++ b/hw/i386/Makefile.objs
> > @@ -11,7 +11,7 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
> >  obj-y += kvm/
> >  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> > -obj-y += pc-testdev.o
> > +obj-y += pc-testdev.o pci-testdev.o
> >  
> >  obj-y := $(addprefix ../,$(obj-y))
> >  
> > diff --git a/hw/pci-testdev.c b/hw/pci-testdev.c
> > new file mode 100644
> > index 0000000..9486624
> > --- /dev/null
> > +++ b/hw/pci-testdev.c
> > @@ -0,0 +1,306 @@
> > +#include "hw/hw.h"
> > +#include "hw/pci/pci.h"
> > +#include "qemu/event_notifier.h"
> > +#include "qemu/osdep.h"
> > +
> > +typedef struct PCITestDevHdr {
> > +    uint8_t test;
> > +    uint8_t width;
> > +    uint8_t pad0[2];
> > +    uint32_t offset;
> > +    uint8_t data;
> > +    uint8_t pad1[3];
> > +    uint32_t count;
> > +    uint8_t name[];
> > +} PCITestDevHdr;
> > +
> > +typedef struct IOTest {
> > +    MemoryRegion *mr;
> > +    EventNotifier notifier;
> > +    bool hasnotifier;
> > +    unsigned size;
> > +    bool match_data;
> > +    PCITestDevHdr *hdr;
> > +    unsigned bufsize;
> > +} IOTest;
> > +
> > +#define IOTEST_DATAMATCH 0xFA
> > +#define IOTEST_NOMATCH   0xCE
> > +
> > +#define IOTEST_IOSIZE 128
> > +#define IOTEST_MEMSIZE 2048
> > +
> > +static const char *iotest_test[] = {
> > +    "no-eventfd",
> > +    "wildcard-eventfd",
> > +    "datamatch-eventfd"
> > +};
> > +
> > +static const char *iotest_type[] = {
> > +    "mmio",
> > +    "portio"
> > +};
> > +
> > +#define IOTEST_TEST(i) (iotest_test[((i) % ARRAY_SIZE(iotest_test))])
> > +#define IOTEST_TYPE(i) (iotest_type[((i) / ARRAY_SIZE(iotest_test))])
> > +#define IOTEST_MAX_TEST (ARRAY_SIZE(iotest_test))
> > +#define IOTEST_MAX_TYPE (ARRAY_SIZE(iotest_type))
> > +#define IOTEST_MAX (IOTEST_MAX_TEST * IOTEST_MAX_TYPE)
> > +
> > +enum {
> > +    IOTEST_ACCESS_NAME,
> > +    IOTEST_ACCESS_DATA,
> > +    IOTEST_ACCESS_MAX,
> > +};
> > +
> > +#define IOTEST_ACCESS_TYPE uint8_t
> > +#define IOTEST_ACCESS_WIDTH (sizeof(uint8_t))
> > +
> > +typedef struct PCITestDevState {
> > +    PCIDevice dev;
> > +    MemoryRegion mmio;
> > +    MemoryRegion portio;
> > +    IOTest *tests;
> > +    int current;
> > +} PCITestDevState;
> > +
> > +#define IOTEST_IS_MEM(i) (strcmp(IOTEST_TYPE(i), "portio"))
> > +#define IOTEST_REGION(d, i) (IOTEST_IS_MEM(i) ?  &(d)->mmio : &(d)->portio)
> > +#define IOTEST_SIZE(i) (IOTEST_IS_MEM(i) ? IOTEST_MEMSIZE : IOTEST_IOSIZE)
> > +#define IOTEST_PCI_BAR(i) (IOTEST_IS_MEM(i) ? PCI_BASE_ADDRESS_SPACE_MEMORY : \
> > +                           PCI_BASE_ADDRESS_SPACE_IO)
> > +
> > +static int pci_testdev_start(IOTest *test)
> > +{
> > +    test->hdr->count = 0;
> > +    if (!test->hasnotifier) {
> > +        return 0;
> > +    }
> > +    event_notifier_test_and_clear(&test->notifier);
> > +    memory_region_add_eventfd(test->mr,
> > +                              le32_to_cpu(test->hdr->offset),
> > +                              test->size,
> > +                              test->match_data,
> > +                              test->hdr->data,
> > +                              &test->notifier);
> > +    return 0;
> > +}
> > +
> > +static void pci_testdev_stop(IOTest *test)
> > +{
> > +    if (!test->hasnotifier) {
> > +        return;
> > +    }
> > +    memory_region_del_eventfd(test->mr,
> > +                              le32_to_cpu(test->hdr->offset),
> > +                              test->size,
> > +                              test->match_data,
> > +                              test->hdr->data,
> > +                              &test->notifier);
> > +}
> > +
> > +static void
> > +pci_testdev_reset(PCITestDevState *d)
> > +{
> > +    if (d->current == -1) {
> > +        return;
> > +    }
> > +    pci_testdev_stop(&d->tests[d->current]);
> > +    d->current = -1;
> > +}
> > +
> > +static void pci_testdev_inc(IOTest *test, unsigned inc)
> > +{
> > +    uint32_t c = le32_to_cpu(test->hdr->count);
> > +    test->hdr->count = cpu_to_le32(c + inc);
> > +}
> > +
> > +static void
> > +pci_testdev_write(void *opaque, hwaddr addr, uint64_t val,
> > +                  unsigned size, int type)
> > +{
> > +    PCITestDevState *d = opaque;
> > +    IOTest *test;
> > +    int t, r;
> > +
> > +    if (addr == offsetof(PCITestDevHdr, test)) {
> > +        pci_testdev_reset(d);
> > +        if (val >= IOTEST_MAX_TEST) {
> > +            return;
> > +        }
> > +        t = type * IOTEST_MAX_TEST + val;
> > +        r = pci_testdev_start(&d->tests[t]);
> > +        if (r < 0) {
> > +            return;
> > +        }
> > +        d->current = t;
> > +        return;
> > +    }
> > +    if (d->current < 0) {
> > +        return;
> > +    }
> > +    test = &d->tests[d->current];
> > +    if (addr != le32_to_cpu(test->hdr->offset)) {
> > +        return;
> > +    }
> > +    if (test->match_data && test->size != size) {
> > +        return;
> > +    }
> > +    if (test->match_data && val != test->hdr->data) {
> > +        return;
> > +    }
> > +    pci_testdev_inc(test, 1);
> > +}
> > +
> > +static uint64_t
> > +pci_testdev_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +    PCITestDevState *d = opaque;
> > +    const char *buf;
> > +    IOTest *test;
> > +    if (d->current < 0) {
> > +        return 0;
> > +    }
> > +    test = &d->tests[d->current];
> > +    buf = (const char *)test->hdr;
> > +    if (addr + size >= test->bufsize) {
> > +        return 0;
> > +    }
> > +    if (test->hasnotifier) {
> > +        event_notifier_test_and_clear(&test->notifier);
> > +    }
> > +    return buf[addr];
> > +}
> > +
> > +static void
> > +pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
> > +                       unsigned size)
> > +{
> > +    pci_testdev_write(opaque, addr, val, size, 0);
> > +}
> > +
> > +static void
> > +pci_testdev_pio_write(void *opaque, hwaddr addr, uint64_t val,
> > +                       unsigned size)
> > +{
> > +    pci_testdev_write(opaque, addr, val, size, 1);
> > +}
> > +
> > +static const MemoryRegionOps pci_testdev_mmio_ops = {
> > +    .read = pci_testdev_read,
> > +    .write = pci_testdev_mmio_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .impl = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 1,
> > +    },
> > +};
> > +
> > +static const MemoryRegionOps pci_testdev_pio_ops = {
> > +    .read = pci_testdev_read,
> > +    .write = pci_testdev_pio_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .impl = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 1,
> > +    },
> > +};
> > +
> > +static int pci_testdev_init(PCIDevice *pci_dev)
> > +{
> > +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, pci_dev);
> > +    uint8_t *pci_conf;
> > +    char *name;
> > +    int r, i;
> > +
> > +    pci_conf = d->dev.config;
> > +
> > +    pci_conf[PCI_INTERRUPT_PIN] = 0; /* no interrupt pin */
> > +
> > +    memory_region_init_io(&d->mmio, &pci_testdev_mmio_ops, d,
> > +                          "pci-testdev-mmio", IOTEST_MEMSIZE * 2);
> > +    memory_region_init_io(&d->portio, &pci_testdev_pio_ops, d,
> > +                          "pci-testdev-portio", IOTEST_IOSIZE * 2);
> > +    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> > +    pci_register_bar(&d->dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> > +
> > +    d->current = -1;
> > +    d->tests = g_malloc0(IOTEST_MAX * sizeof *d->tests);
> > +    for (i = 0; i < IOTEST_MAX; ++i) {
> > +        IOTest *test = &d->tests[i];
> > +        name = g_strdup_printf("%s-%s", IOTEST_TYPE(i), IOTEST_TEST(i));
> > +        test->bufsize = sizeof(PCITestDevHdr) + strlen(name) + 1;
> > +        test->hdr = g_malloc0(test->bufsize);
> > +        memcpy(test->hdr->name, name, strlen(name) + 1);
> > +        g_free(name);
> > +        test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
> > +        test->size = IOTEST_ACCESS_WIDTH;
> > +        test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
> > +        test->hdr->test = i;
> > +        test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
> > +        test->hdr->width = IOTEST_ACCESS_WIDTH;
> > +        test->mr = IOTEST_REGION(d, i);
> > +        if (!strcmp(IOTEST_TEST(i), "no-eventfd")) {
> > +            test->hasnotifier = false;
> > +            continue;
> > +        }
> > +        r = event_notifier_init(&test->notifier, 0);
> > +        assert(r >= 0);
> > +        test->hasnotifier = true;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static void
> > +pci_testdev_uninit(PCIDevice *dev)
> > +{
> > +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
> > +    int i;
> > +
> > +    pci_testdev_reset(d);
> > +    for (i = 0; i < IOTEST_MAX; ++i) {
> > +        if (d->tests[i].hasnotifier) {
> > +            event_notifier_cleanup(&d->tests[i].notifier);
> > +        }
> > +        g_free(d->tests[i].hdr);
> > +    }
> > +    g_free(d->tests);
> > +    memory_region_destroy(&d->mmio);
> > +    memory_region_destroy(&d->portio);
> > +}
> > +
> > +static void qdev_pci_testdev_reset(DeviceState *dev)
> > +{
> > +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev.qdev, dev);
> > +    pci_testdev_reset(d);
> > +}
> > +
> > +static void pci_testdev_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > +
> > +    k->init = pci_testdev_init;
> > +    k->exit = pci_testdev_uninit;
> > +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > +    k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
> > +    k->revision = 0x00;
> > +    k->class_id = PCI_CLASS_OTHERS;
> > +    dc->desc = "PCI Test Device";
> > +    dc->reset = qdev_pci_testdev_reset;
> > +}
> > +
> > +static const TypeInfo pci_testdev_info = {
> > +    .name          = "pci-testdev",
> > +    .parent        = TYPE_PCI_DEVICE,
> > +    .instance_size = sizeof(PCITestDevState),
> > +    .class_init    = pci_testdev_class_init,
> > +};
> > +
> > +static void pci_testdev_register_types(void)
> > +{
> > +    type_register_static(&pci_testdev_info);
> > +}
> > +
> > +type_init(pci_testdev_register_types)
> > diff --git a/hw/pci/pci.h b/hw/pci/pci.h
> > index 774369c..d81198c 100644
> > --- a/hw/pci/pci.h
> > +++ b/hw/pci/pci.h
> > @@ -84,6 +84,7 @@
> >  #define PCI_DEVICE_ID_REDHAT_SERIAL      0x0002
> >  #define PCI_DEVICE_ID_REDHAT_SERIAL2     0x0003
> >  #define PCI_DEVICE_ID_REDHAT_SERIAL4     0x0004
> > +#define PCI_DEVICE_ID_REDHAT_TEST        0x0005
> >  #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
> >  
> >  #define FMT_PCIBUS                      PRIx64
> > 

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 11:45, Michael S. Tsirkin ha scritto:
> On Wed, Apr 03, 2013 at 11:28:55AM +0200, Paolo Bonzini wrote:
>> Il 03/04/2013 10:59, Michael S. Tsirkin ha scritto:
>>> This device is used for kvm unit tests,
>>> currently it supports testing performance of ioeventfd.
>>> Using updated kvm unittest, here's an example output:
>>>         mmio-no-eventfd:pci-mem 8796
>>>         mmio-wildcard-eventfd:pci-mem 3609
>>>         mmio-datamatch-eventfd:pci-mem 3685
>>>         portio-no-eventfd:pci-io 5287
>>>         portio-wildcard-eventfd:pci-io 1762
>>>         portio-datamatch-eventfd:pci-io 1777
>>
>> There is too much magic in this device, that is shared between the
>> testcase and the device.
> 
> Paolo, it looks like there's some misunderstanding.
> There's no magic. Each BAR has the structure specified
> in both test and qemu:
> 
> struct pci_test_dev_hdr {
>     uint8_t test;       -- test number
>     uint8_t width;      - how much to write
>     uint8_t pad0[2];
>     uint32_t offset;    - where to write
>     uint32_t data;     - what to write
>     uint32_t count;    - incremented on write
>     uint8_t name[];   -- test name for debugging
> };
> 
> What you propose below has a bit less features
> if you add these you will get pretty much same thing.

I think it has the same features, except the guest is in charge of
enabling/disabling ioeventfd.

>>  I think this device should be structured
>> differently.  For example, you could have a single EventNotifier and 3 BARs:
>>
>> 1) BAR0/BAR1 are as in your device.  All they do is count writes and
>> report them into a register in BAR2.
>>
>> 2) BAR2 has a control register for BAR0 and one for BAR1, that
>> enables/disables ioeventfd (bit 0 = enable/disable, bit 1 =
>> wildcard/datamatch, bits 2-3 = log2(width)).  It also has a zero-on-read
>> counter that is incremented for each write to BAR0 or BAR1, and clears
>> the EventNotifier.
> 
> This is pretty close to what I have, only I put control
> at the beginning of each BAR, and I support arbitrary
> length writes guest side, and I allow testing both
> datamatch and non data match.

Yes, all of this is possible with my design too, the differences are:
* "what to write" is a fixed value (could be 0, or all-ones, whatever).
* the offset is a fixed value too (could be 0, or could be written in
other bits of the control register)
* no test names for debugging, because the guest picks the tests

For example:

mmio-no-eventfd:pci-mem            0000 to BAR2's first control register
mmio-wildcard-eventfd:pci-mem      1001 to BAR2's first control register
mmio-datamatch-eventfd:pci-mem     1011 to BAR2's first control register
portio-no-eventfd:pci-io           0000 to BAR2's second register
portio-wildcard-eventfd:pci-io     1001 to BAR2's second register
portio-datamatch-eventfd:pci-io    1011 to BAR2's second register

> I also do not want guest to control things like ioeventfd
> assignment. These are host side things and while you could
> argue security does not matter for a test device,
> I think it will set a bad example.

Test devices exist for the purpose of doing things that you won't do in
normal devices. :)

> So any new benchmark can be added pretty much on qemu
> side without changing test.

You could say the same with the control registers (new benchmarks can be
added on the test side without changing QEMU, for example benchmarks
using different write sizes.

(BTW, and unrelated to this, please use default-configs/ and
hw/Makefile.objs instead of hw/i386/Makefile.objs to enable the device).

Paolo

> 
> 
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  hw/i386/Makefile.objs |   2 +-
>>>  hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/pci/pci.h          |   1 +
>>>  3 files changed, 308 insertions(+), 1 deletion(-)
>>>  create mode 100644 hw/pci-testdev.c
>>>
>>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>>> index a78c0b2..7497e7a 100644
>>> --- a/hw/i386/Makefile.objs
>>> +++ b/hw/i386/Makefile.objs
>>> @@ -11,7 +11,7 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
>>>  obj-y += kvm/
>>>  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>> -obj-y += pc-testdev.o
>>> +obj-y += pc-testdev.o pci-testdev.o
>>>  
>>>  obj-y := $(addprefix ../,$(obj-y))
>>>  
>>> diff --git a/hw/pci-testdev.c b/hw/pci-testdev.c
>>> new file mode 100644
>>> index 0000000..9486624
>>> --- /dev/null
>>> +++ b/hw/pci-testdev.c
>>> @@ -0,0 +1,306 @@
>>> +#include "hw/hw.h"
>>> +#include "hw/pci/pci.h"
>>> +#include "qemu/event_notifier.h"
>>> +#include "qemu/osdep.h"
>>> +
>>> +typedef struct PCITestDevHdr {
>>> +    uint8_t test;
>>> +    uint8_t width;
>>> +    uint8_t pad0[2];
>>> +    uint32_t offset;
>>> +    uint8_t data;
>>> +    uint8_t pad1[3];
>>> +    uint32_t count;
>>> +    uint8_t name[];
>>> +} PCITestDevHdr;
>>> +
>>> +typedef struct IOTest {
>>> +    MemoryRegion *mr;
>>> +    EventNotifier notifier;
>>> +    bool hasnotifier;
>>> +    unsigned size;
>>> +    bool match_data;
>>> +    PCITestDevHdr *hdr;
>>> +    unsigned bufsize;
>>> +} IOTest;
>>> +
>>> +#define IOTEST_DATAMATCH 0xFA
>>> +#define IOTEST_NOMATCH   0xCE
>>> +
>>> +#define IOTEST_IOSIZE 128
>>> +#define IOTEST_MEMSIZE 2048
>>> +
>>> +static const char *iotest_test[] = {
>>> +    "no-eventfd",
>>> +    "wildcard-eventfd",
>>> +    "datamatch-eventfd"
>>> +};
>>> +
>>> +static const char *iotest_type[] = {
>>> +    "mmio",
>>> +    "portio"
>>> +};
>>> +
>>> +#define IOTEST_TEST(i) (iotest_test[((i) % ARRAY_SIZE(iotest_test))])
>>> +#define IOTEST_TYPE(i) (iotest_type[((i) / ARRAY_SIZE(iotest_test))])
>>> +#define IOTEST_MAX_TEST (ARRAY_SIZE(iotest_test))
>>> +#define IOTEST_MAX_TYPE (ARRAY_SIZE(iotest_type))
>>> +#define IOTEST_MAX (IOTEST_MAX_TEST * IOTEST_MAX_TYPE)
>>> +
>>> +enum {
>>> +    IOTEST_ACCESS_NAME,
>>> +    IOTEST_ACCESS_DATA,
>>> +    IOTEST_ACCESS_MAX,
>>> +};
>>> +
>>> +#define IOTEST_ACCESS_TYPE uint8_t
>>> +#define IOTEST_ACCESS_WIDTH (sizeof(uint8_t))
>>> +
>>> +typedef struct PCITestDevState {
>>> +    PCIDevice dev;
>>> +    MemoryRegion mmio;
>>> +    MemoryRegion portio;
>>> +    IOTest *tests;
>>> +    int current;
>>> +} PCITestDevState;
>>> +
>>> +#define IOTEST_IS_MEM(i) (strcmp(IOTEST_TYPE(i), "portio"))
>>> +#define IOTEST_REGION(d, i) (IOTEST_IS_MEM(i) ?  &(d)->mmio : &(d)->portio)
>>> +#define IOTEST_SIZE(i) (IOTEST_IS_MEM(i) ? IOTEST_MEMSIZE : IOTEST_IOSIZE)
>>> +#define IOTEST_PCI_BAR(i) (IOTEST_IS_MEM(i) ? PCI_BASE_ADDRESS_SPACE_MEMORY : \
>>> +                           PCI_BASE_ADDRESS_SPACE_IO)
>>> +
>>> +static int pci_testdev_start(IOTest *test)
>>> +{
>>> +    test->hdr->count = 0;
>>> +    if (!test->hasnotifier) {
>>> +        return 0;
>>> +    }
>>> +    event_notifier_test_and_clear(&test->notifier);
>>> +    memory_region_add_eventfd(test->mr,
>>> +                              le32_to_cpu(test->hdr->offset),
>>> +                              test->size,
>>> +                              test->match_data,
>>> +                              test->hdr->data,
>>> +                              &test->notifier);
>>> +    return 0;
>>> +}
>>> +
>>> +static void pci_testdev_stop(IOTest *test)
>>> +{
>>> +    if (!test->hasnotifier) {
>>> +        return;
>>> +    }
>>> +    memory_region_del_eventfd(test->mr,
>>> +                              le32_to_cpu(test->hdr->offset),
>>> +                              test->size,
>>> +                              test->match_data,
>>> +                              test->hdr->data,
>>> +                              &test->notifier);
>>> +}
>>> +
>>> +static void
>>> +pci_testdev_reset(PCITestDevState *d)
>>> +{
>>> +    if (d->current == -1) {
>>> +        return;
>>> +    }
>>> +    pci_testdev_stop(&d->tests[d->current]);
>>> +    d->current = -1;
>>> +}
>>> +
>>> +static void pci_testdev_inc(IOTest *test, unsigned inc)
>>> +{
>>> +    uint32_t c = le32_to_cpu(test->hdr->count);
>>> +    test->hdr->count = cpu_to_le32(c + inc);
>>> +}
>>> +
>>> +static void
>>> +pci_testdev_write(void *opaque, hwaddr addr, uint64_t val,
>>> +                  unsigned size, int type)
>>> +{
>>> +    PCITestDevState *d = opaque;
>>> +    IOTest *test;
>>> +    int t, r;
>>> +
>>> +    if (addr == offsetof(PCITestDevHdr, test)) {
>>> +        pci_testdev_reset(d);
>>> +        if (val >= IOTEST_MAX_TEST) {
>>> +            return;
>>> +        }
>>> +        t = type * IOTEST_MAX_TEST + val;
>>> +        r = pci_testdev_start(&d->tests[t]);
>>> +        if (r < 0) {
>>> +            return;
>>> +        }
>>> +        d->current = t;
>>> +        return;
>>> +    }
>>> +    if (d->current < 0) {
>>> +        return;
>>> +    }
>>> +    test = &d->tests[d->current];
>>> +    if (addr != le32_to_cpu(test->hdr->offset)) {
>>> +        return;
>>> +    }
>>> +    if (test->match_data && test->size != size) {
>>> +        return;
>>> +    }
>>> +    if (test->match_data && val != test->hdr->data) {
>>> +        return;
>>> +    }
>>> +    pci_testdev_inc(test, 1);
>>> +}
>>> +
>>> +static uint64_t
>>> +pci_testdev_read(void *opaque, hwaddr addr, unsigned size)
>>> +{
>>> +    PCITestDevState *d = opaque;
>>> +    const char *buf;
>>> +    IOTest *test;
>>> +    if (d->current < 0) {
>>> +        return 0;
>>> +    }
>>> +    test = &d->tests[d->current];
>>> +    buf = (const char *)test->hdr;
>>> +    if (addr + size >= test->bufsize) {
>>> +        return 0;
>>> +    }
>>> +    if (test->hasnotifier) {
>>> +        event_notifier_test_and_clear(&test->notifier);
>>> +    }
>>> +    return buf[addr];
>>> +}
>>> +
>>> +static void
>>> +pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>>> +                       unsigned size)
>>> +{
>>> +    pci_testdev_write(opaque, addr, val, size, 0);
>>> +}
>>> +
>>> +static void
>>> +pci_testdev_pio_write(void *opaque, hwaddr addr, uint64_t val,
>>> +                       unsigned size)
>>> +{
>>> +    pci_testdev_write(opaque, addr, val, size, 1);
>>> +}
>>> +
>>> +static const MemoryRegionOps pci_testdev_mmio_ops = {
>>> +    .read = pci_testdev_read,
>>> +    .write = pci_testdev_mmio_write,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> +    .impl = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 1,
>>> +    },
>>> +};
>>> +
>>> +static const MemoryRegionOps pci_testdev_pio_ops = {
>>> +    .read = pci_testdev_read,
>>> +    .write = pci_testdev_pio_write,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> +    .impl = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 1,
>>> +    },
>>> +};
>>> +
>>> +static int pci_testdev_init(PCIDevice *pci_dev)
>>> +{
>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, pci_dev);
>>> +    uint8_t *pci_conf;
>>> +    char *name;
>>> +    int r, i;
>>> +
>>> +    pci_conf = d->dev.config;
>>> +
>>> +    pci_conf[PCI_INTERRUPT_PIN] = 0; /* no interrupt pin */
>>> +
>>> +    memory_region_init_io(&d->mmio, &pci_testdev_mmio_ops, d,
>>> +                          "pci-testdev-mmio", IOTEST_MEMSIZE * 2);
>>> +    memory_region_init_io(&d->portio, &pci_testdev_pio_ops, d,
>>> +                          "pci-testdev-portio", IOTEST_IOSIZE * 2);
>>> +    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>>> +    pci_register_bar(&d->dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
>>> +
>>> +    d->current = -1;
>>> +    d->tests = g_malloc0(IOTEST_MAX * sizeof *d->tests);
>>> +    for (i = 0; i < IOTEST_MAX; ++i) {
>>> +        IOTest *test = &d->tests[i];
>>> +        name = g_strdup_printf("%s-%s", IOTEST_TYPE(i), IOTEST_TEST(i));
>>> +        test->bufsize = sizeof(PCITestDevHdr) + strlen(name) + 1;
>>> +        test->hdr = g_malloc0(test->bufsize);
>>> +        memcpy(test->hdr->name, name, strlen(name) + 1);
>>> +        g_free(name);
>>> +        test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
>>> +        test->size = IOTEST_ACCESS_WIDTH;
>>> +        test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
>>> +        test->hdr->test = i;
>>> +        test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
>>> +        test->hdr->width = IOTEST_ACCESS_WIDTH;
>>> +        test->mr = IOTEST_REGION(d, i);
>>> +        if (!strcmp(IOTEST_TEST(i), "no-eventfd")) {
>>> +            test->hasnotifier = false;
>>> +            continue;
>>> +        }
>>> +        r = event_notifier_init(&test->notifier, 0);
>>> +        assert(r >= 0);
>>> +        test->hasnotifier = true;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void
>>> +pci_testdev_uninit(PCIDevice *dev)
>>> +{
>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
>>> +    int i;
>>> +
>>> +    pci_testdev_reset(d);
>>> +    for (i = 0; i < IOTEST_MAX; ++i) {
>>> +        if (d->tests[i].hasnotifier) {
>>> +            event_notifier_cleanup(&d->tests[i].notifier);
>>> +        }
>>> +        g_free(d->tests[i].hdr);
>>> +    }
>>> +    g_free(d->tests);
>>> +    memory_region_destroy(&d->mmio);
>>> +    memory_region_destroy(&d->portio);
>>> +}
>>> +
>>> +static void qdev_pci_testdev_reset(DeviceState *dev)
>>> +{
>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev.qdev, dev);
>>> +    pci_testdev_reset(d);
>>> +}
>>> +
>>> +static void pci_testdev_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> +
>>> +    k->init = pci_testdev_init;
>>> +    k->exit = pci_testdev_uninit;
>>> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>> +    k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
>>> +    k->revision = 0x00;
>>> +    k->class_id = PCI_CLASS_OTHERS;
>>> +    dc->desc = "PCI Test Device";
>>> +    dc->reset = qdev_pci_testdev_reset;
>>> +}
>>> +
>>> +static const TypeInfo pci_testdev_info = {
>>> +    .name          = "pci-testdev",
>>> +    .parent        = TYPE_PCI_DEVICE,
>>> +    .instance_size = sizeof(PCITestDevState),
>>> +    .class_init    = pci_testdev_class_init,
>>> +};
>>> +
>>> +static void pci_testdev_register_types(void)
>>> +{
>>> +    type_register_static(&pci_testdev_info);
>>> +}
>>> +
>>> +type_init(pci_testdev_register_types)
>>> diff --git a/hw/pci/pci.h b/hw/pci/pci.h
>>> index 774369c..d81198c 100644
>>> --- a/hw/pci/pci.h
>>> +++ b/hw/pci/pci.h
>>> @@ -84,6 +84,7 @@
>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL      0x0002
>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL2     0x0003
>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL4     0x0004
>>> +#define PCI_DEVICE_ID_REDHAT_TEST        0x0005
>>>  #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
>>>  
>>>  #define FMT_PCIBUS                      PRIx64
>>>

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 11:53:59AM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 11:45, Michael S. Tsirkin ha scritto:
> > On Wed, Apr 03, 2013 at 11:28:55AM +0200, Paolo Bonzini wrote:
> >> Il 03/04/2013 10:59, Michael S. Tsirkin ha scritto:
> >>> This device is used for kvm unit tests,
> >>> currently it supports testing performance of ioeventfd.
> >>> Using updated kvm unittest, here's an example output:
> >>>         mmio-no-eventfd:pci-mem 8796
> >>>         mmio-wildcard-eventfd:pci-mem 3609
> >>>         mmio-datamatch-eventfd:pci-mem 3685
> >>>         portio-no-eventfd:pci-io 5287
> >>>         portio-wildcard-eventfd:pci-io 1762
> >>>         portio-datamatch-eventfd:pci-io 1777
> >>
> >> There is too much magic in this device, that is shared between the
> >> testcase and the device.
> > 
> > Paolo, it looks like there's some misunderstanding.
> > There's no magic. Each BAR has the structure specified
> > in both test and qemu:
> > 
> > struct pci_test_dev_hdr {
> >     uint8_t test;       -- test number
> >     uint8_t width;      - how much to write
> >     uint8_t pad0[2];
> >     uint32_t offset;    - where to write
> >     uint32_t data;     - what to write
> >     uint32_t count;    - incremented on write
> >     uint8_t name[];   -- test name for debugging
> > };
> > 
> > What you propose below has a bit less features
> > if you add these you will get pretty much same thing.
> 
> I think it has the same features, except the guest is in charge of
> enabling/disabling ioeventfd.
> 
> >>  I think this device should be structured
> >> differently.  For example, you could have a single EventNotifier and 3 BARs:
> >>
> >> 1) BAR0/BAR1 are as in your device.  All they do is count writes and
> >> report them into a register in BAR2.
> >>
> >> 2) BAR2 has a control register for BAR0 and one for BAR1, that
> >> enables/disables ioeventfd (bit 0 = enable/disable, bit 1 =
> >> wildcard/datamatch, bits 2-3 = log2(width)).  It also has a zero-on-read
> >> counter that is incremented for each write to BAR0 or BAR1, and clears
> >> the EventNotifier.
> > 
> > This is pretty close to what I have, only I put control
> > at the beginning of each BAR, and I support arbitrary
> > length writes guest side, and I allow testing both
> > datamatch and non data match.
> 
> Yes, all of this is possible with my design too, the differences are:
> * "what to write" is a fixed value (could be 0, or all-ones, whatever).
> * the offset is a fixed value too (could be 0, or could be written in
> other bits of the control register)
> * no test names for debugging, because the guest picks the tests

Heh but host and guest must agree on how to trigger each test.
This means each time I add a test I need to update guest and host side.

> For example:
> 
> mmio-no-eventfd:pci-mem            0000 to BAR2's first control register
> mmio-wildcard-eventfd:pci-mem      1001 to BAR2's first control register
> mmio-datamatch-eventfd:pci-mem     1011 to BAR2's first control register
> portio-no-eventfd:pci-io           0000 to BAR2's second register
> portio-wildcard-eventfd:pci-io     1001 to BAR2's second register
> portio-datamatch-eventfd:pci-io    1011 to BAR2's second register

Exactly. I prefer a flexible structure so I can change
things host side. For example register multiple
datamatches with same address and compare speed
of access on first and last one.
Similarly for address.
But there's no less magic because you share constants.

> > I also do not want guest to control things like ioeventfd
> > assignment. These are host side things and while you could
> > argue security does not matter for a test device,
> > I think it will set a bad example.
> 
> Test devices exist for the purpose of doing things that you won't do in
> normal devices. :)

You might not care about setting bad examples for ISA
but I care about setting bad examples for PCI.
In particular this one actually is a completely normal PCI device.
And what it does it very much like what we are discussing
for virtio pci new layout.

> > So any new benchmark can be added pretty much on qemu
> > side without changing test.
> 
> You could say the same with the control registers (new benchmarks can be
> added on the test side without changing QEMU, for example benchmarks
> using different write sizes.

And then we'll get back to a layout kind of like what we have here,
except guest side. Again, I have in mind several more tests
so this is structured in a way that will make adding
them easier.

> (BTW, and unrelated to this, please use default-configs/ and
> hw/Makefile.objs instead of hw/i386/Makefile.objs to enable the device).
> 
> Paolo

Okay but why does pc-test device sit in hw/i386/Makefile.objs ?

> > 
> > 
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>  hw/i386/Makefile.objs |   2 +-
> >>>  hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  hw/pci/pci.h          |   1 +
> >>>  3 files changed, 308 insertions(+), 1 deletion(-)
> >>>  create mode 100644 hw/pci-testdev.c
> >>>
> >>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> >>> index a78c0b2..7497e7a 100644
> >>> --- a/hw/i386/Makefile.objs
> >>> +++ b/hw/i386/Makefile.objs
> >>> @@ -11,7 +11,7 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> >>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
> >>>  obj-y += kvm/
> >>>  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>> -obj-y += pc-testdev.o
> >>> +obj-y += pc-testdev.o pci-testdev.o
> >>>  
> >>>  obj-y := $(addprefix ../,$(obj-y))
> >>>  
> >>> diff --git a/hw/pci-testdev.c b/hw/pci-testdev.c
> >>> new file mode 100644
> >>> index 0000000..9486624
> >>> --- /dev/null
> >>> +++ b/hw/pci-testdev.c
> >>> @@ -0,0 +1,306 @@
> >>> +#include "hw/hw.h"
> >>> +#include "hw/pci/pci.h"
> >>> +#include "qemu/event_notifier.h"
> >>> +#include "qemu/osdep.h"
> >>> +
> >>> +typedef struct PCITestDevHdr {
> >>> +    uint8_t test;
> >>> +    uint8_t width;
> >>> +    uint8_t pad0[2];
> >>> +    uint32_t offset;
> >>> +    uint8_t data;
> >>> +    uint8_t pad1[3];
> >>> +    uint32_t count;
> >>> +    uint8_t name[];
> >>> +} PCITestDevHdr;
> >>> +
> >>> +typedef struct IOTest {
> >>> +    MemoryRegion *mr;
> >>> +    EventNotifier notifier;
> >>> +    bool hasnotifier;
> >>> +    unsigned size;
> >>> +    bool match_data;
> >>> +    PCITestDevHdr *hdr;
> >>> +    unsigned bufsize;
> >>> +} IOTest;
> >>> +
> >>> +#define IOTEST_DATAMATCH 0xFA
> >>> +#define IOTEST_NOMATCH   0xCE
> >>> +
> >>> +#define IOTEST_IOSIZE 128
> >>> +#define IOTEST_MEMSIZE 2048
> >>> +
> >>> +static const char *iotest_test[] = {
> >>> +    "no-eventfd",
> >>> +    "wildcard-eventfd",
> >>> +    "datamatch-eventfd"
> >>> +};
> >>> +
> >>> +static const char *iotest_type[] = {
> >>> +    "mmio",
> >>> +    "portio"
> >>> +};
> >>> +
> >>> +#define IOTEST_TEST(i) (iotest_test[((i) % ARRAY_SIZE(iotest_test))])
> >>> +#define IOTEST_TYPE(i) (iotest_type[((i) / ARRAY_SIZE(iotest_test))])
> >>> +#define IOTEST_MAX_TEST (ARRAY_SIZE(iotest_test))
> >>> +#define IOTEST_MAX_TYPE (ARRAY_SIZE(iotest_type))
> >>> +#define IOTEST_MAX (IOTEST_MAX_TEST * IOTEST_MAX_TYPE)
> >>> +
> >>> +enum {
> >>> +    IOTEST_ACCESS_NAME,
> >>> +    IOTEST_ACCESS_DATA,
> >>> +    IOTEST_ACCESS_MAX,
> >>> +};
> >>> +
> >>> +#define IOTEST_ACCESS_TYPE uint8_t
> >>> +#define IOTEST_ACCESS_WIDTH (sizeof(uint8_t))
> >>> +
> >>> +typedef struct PCITestDevState {
> >>> +    PCIDevice dev;
> >>> +    MemoryRegion mmio;
> >>> +    MemoryRegion portio;
> >>> +    IOTest *tests;
> >>> +    int current;
> >>> +} PCITestDevState;
> >>> +
> >>> +#define IOTEST_IS_MEM(i) (strcmp(IOTEST_TYPE(i), "portio"))
> >>> +#define IOTEST_REGION(d, i) (IOTEST_IS_MEM(i) ?  &(d)->mmio : &(d)->portio)
> >>> +#define IOTEST_SIZE(i) (IOTEST_IS_MEM(i) ? IOTEST_MEMSIZE : IOTEST_IOSIZE)
> >>> +#define IOTEST_PCI_BAR(i) (IOTEST_IS_MEM(i) ? PCI_BASE_ADDRESS_SPACE_MEMORY : \
> >>> +                           PCI_BASE_ADDRESS_SPACE_IO)
> >>> +
> >>> +static int pci_testdev_start(IOTest *test)
> >>> +{
> >>> +    test->hdr->count = 0;
> >>> +    if (!test->hasnotifier) {
> >>> +        return 0;
> >>> +    }
> >>> +    event_notifier_test_and_clear(&test->notifier);
> >>> +    memory_region_add_eventfd(test->mr,
> >>> +                              le32_to_cpu(test->hdr->offset),
> >>> +                              test->size,
> >>> +                              test->match_data,
> >>> +                              test->hdr->data,
> >>> +                              &test->notifier);
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static void pci_testdev_stop(IOTest *test)
> >>> +{
> >>> +    if (!test->hasnotifier) {
> >>> +        return;
> >>> +    }
> >>> +    memory_region_del_eventfd(test->mr,
> >>> +                              le32_to_cpu(test->hdr->offset),
> >>> +                              test->size,
> >>> +                              test->match_data,
> >>> +                              test->hdr->data,
> >>> +                              &test->notifier);
> >>> +}
> >>> +
> >>> +static void
> >>> +pci_testdev_reset(PCITestDevState *d)
> >>> +{
> >>> +    if (d->current == -1) {
> >>> +        return;
> >>> +    }
> >>> +    pci_testdev_stop(&d->tests[d->current]);
> >>> +    d->current = -1;
> >>> +}
> >>> +
> >>> +static void pci_testdev_inc(IOTest *test, unsigned inc)
> >>> +{
> >>> +    uint32_t c = le32_to_cpu(test->hdr->count);
> >>> +    test->hdr->count = cpu_to_le32(c + inc);
> >>> +}
> >>> +
> >>> +static void
> >>> +pci_testdev_write(void *opaque, hwaddr addr, uint64_t val,
> >>> +                  unsigned size, int type)
> >>> +{
> >>> +    PCITestDevState *d = opaque;
> >>> +    IOTest *test;
> >>> +    int t, r;
> >>> +
> >>> +    if (addr == offsetof(PCITestDevHdr, test)) {
> >>> +        pci_testdev_reset(d);
> >>> +        if (val >= IOTEST_MAX_TEST) {
> >>> +            return;
> >>> +        }
> >>> +        t = type * IOTEST_MAX_TEST + val;
> >>> +        r = pci_testdev_start(&d->tests[t]);
> >>> +        if (r < 0) {
> >>> +            return;
> >>> +        }
> >>> +        d->current = t;
> >>> +        return;
> >>> +    }
> >>> +    if (d->current < 0) {
> >>> +        return;
> >>> +    }
> >>> +    test = &d->tests[d->current];
> >>> +    if (addr != le32_to_cpu(test->hdr->offset)) {
> >>> +        return;
> >>> +    }
> >>> +    if (test->match_data && test->size != size) {
> >>> +        return;
> >>> +    }
> >>> +    if (test->match_data && val != test->hdr->data) {
> >>> +        return;
> >>> +    }
> >>> +    pci_testdev_inc(test, 1);
> >>> +}
> >>> +
> >>> +static uint64_t
> >>> +pci_testdev_read(void *opaque, hwaddr addr, unsigned size)
> >>> +{
> >>> +    PCITestDevState *d = opaque;
> >>> +    const char *buf;
> >>> +    IOTest *test;
> >>> +    if (d->current < 0) {
> >>> +        return 0;
> >>> +    }
> >>> +    test = &d->tests[d->current];
> >>> +    buf = (const char *)test->hdr;
> >>> +    if (addr + size >= test->bufsize) {
> >>> +        return 0;
> >>> +    }
> >>> +    if (test->hasnotifier) {
> >>> +        event_notifier_test_and_clear(&test->notifier);
> >>> +    }
> >>> +    return buf[addr];
> >>> +}
> >>> +
> >>> +static void
> >>> +pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
> >>> +                       unsigned size)
> >>> +{
> >>> +    pci_testdev_write(opaque, addr, val, size, 0);
> >>> +}
> >>> +
> >>> +static void
> >>> +pci_testdev_pio_write(void *opaque, hwaddr addr, uint64_t val,
> >>> +                       unsigned size)
> >>> +{
> >>> +    pci_testdev_write(opaque, addr, val, size, 1);
> >>> +}
> >>> +
> >>> +static const MemoryRegionOps pci_testdev_mmio_ops = {
> >>> +    .read = pci_testdev_read,
> >>> +    .write = pci_testdev_mmio_write,
> >>> +    .endianness = DEVICE_LITTLE_ENDIAN,
> >>> +    .impl = {
> >>> +        .min_access_size = 1,
> >>> +        .max_access_size = 1,
> >>> +    },
> >>> +};
> >>> +
> >>> +static const MemoryRegionOps pci_testdev_pio_ops = {
> >>> +    .read = pci_testdev_read,
> >>> +    .write = pci_testdev_pio_write,
> >>> +    .endianness = DEVICE_LITTLE_ENDIAN,
> >>> +    .impl = {
> >>> +        .min_access_size = 1,
> >>> +        .max_access_size = 1,
> >>> +    },
> >>> +};
> >>> +
> >>> +static int pci_testdev_init(PCIDevice *pci_dev)
> >>> +{
> >>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, pci_dev);
> >>> +    uint8_t *pci_conf;
> >>> +    char *name;
> >>> +    int r, i;
> >>> +
> >>> +    pci_conf = d->dev.config;
> >>> +
> >>> +    pci_conf[PCI_INTERRUPT_PIN] = 0; /* no interrupt pin */
> >>> +
> >>> +    memory_region_init_io(&d->mmio, &pci_testdev_mmio_ops, d,
> >>> +                          "pci-testdev-mmio", IOTEST_MEMSIZE * 2);
> >>> +    memory_region_init_io(&d->portio, &pci_testdev_pio_ops, d,
> >>> +                          "pci-testdev-portio", IOTEST_IOSIZE * 2);
> >>> +    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> >>> +    pci_register_bar(&d->dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> >>> +
> >>> +    d->current = -1;
> >>> +    d->tests = g_malloc0(IOTEST_MAX * sizeof *d->tests);
> >>> +    for (i = 0; i < IOTEST_MAX; ++i) {
> >>> +        IOTest *test = &d->tests[i];
> >>> +        name = g_strdup_printf("%s-%s", IOTEST_TYPE(i), IOTEST_TEST(i));
> >>> +        test->bufsize = sizeof(PCITestDevHdr) + strlen(name) + 1;
> >>> +        test->hdr = g_malloc0(test->bufsize);
> >>> +        memcpy(test->hdr->name, name, strlen(name) + 1);
> >>> +        g_free(name);
> >>> +        test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
> >>> +        test->size = IOTEST_ACCESS_WIDTH;
> >>> +        test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
> >>> +        test->hdr->test = i;
> >>> +        test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
> >>> +        test->hdr->width = IOTEST_ACCESS_WIDTH;
> >>> +        test->mr = IOTEST_REGION(d, i);
> >>> +        if (!strcmp(IOTEST_TEST(i), "no-eventfd")) {
> >>> +            test->hasnotifier = false;
> >>> +            continue;
> >>> +        }
> >>> +        r = event_notifier_init(&test->notifier, 0);
> >>> +        assert(r >= 0);
> >>> +        test->hasnotifier = true;
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static void
> >>> +pci_testdev_uninit(PCIDevice *dev)
> >>> +{
> >>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
> >>> +    int i;
> >>> +
> >>> +    pci_testdev_reset(d);
> >>> +    for (i = 0; i < IOTEST_MAX; ++i) {
> >>> +        if (d->tests[i].hasnotifier) {
> >>> +            event_notifier_cleanup(&d->tests[i].notifier);
> >>> +        }
> >>> +        g_free(d->tests[i].hdr);
> >>> +    }
> >>> +    g_free(d->tests);
> >>> +    memory_region_destroy(&d->mmio);
> >>> +    memory_region_destroy(&d->portio);
> >>> +}
> >>> +
> >>> +static void qdev_pci_testdev_reset(DeviceState *dev)
> >>> +{
> >>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev.qdev, dev);
> >>> +    pci_testdev_reset(d);
> >>> +}
> >>> +
> >>> +static void pci_testdev_class_init(ObjectClass *klass, void *data)
> >>> +{
> >>> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>> +
> >>> +    k->init = pci_testdev_init;
> >>> +    k->exit = pci_testdev_uninit;
> >>> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
> >>> +    k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
> >>> +    k->revision = 0x00;
> >>> +    k->class_id = PCI_CLASS_OTHERS;
> >>> +    dc->desc = "PCI Test Device";
> >>> +    dc->reset = qdev_pci_testdev_reset;
> >>> +}
> >>> +
> >>> +static const TypeInfo pci_testdev_info = {
> >>> +    .name          = "pci-testdev",
> >>> +    .parent        = TYPE_PCI_DEVICE,
> >>> +    .instance_size = sizeof(PCITestDevState),
> >>> +    .class_init    = pci_testdev_class_init,
> >>> +};
> >>> +
> >>> +static void pci_testdev_register_types(void)
> >>> +{
> >>> +    type_register_static(&pci_testdev_info);
> >>> +}
> >>> +
> >>> +type_init(pci_testdev_register_types)
> >>> diff --git a/hw/pci/pci.h b/hw/pci/pci.h
> >>> index 774369c..d81198c 100644
> >>> --- a/hw/pci/pci.h
> >>> +++ b/hw/pci/pci.h
> >>> @@ -84,6 +84,7 @@
> >>>  #define PCI_DEVICE_ID_REDHAT_SERIAL      0x0002
> >>>  #define PCI_DEVICE_ID_REDHAT_SERIAL2     0x0003
> >>>  #define PCI_DEVICE_ID_REDHAT_SERIAL4     0x0004
> >>> +#define PCI_DEVICE_ID_REDHAT_TEST        0x0005
> >>>  #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
> >>>  
> >>>  #define FMT_PCIBUS                      PRIx64
> >>>

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

> Exactly. I prefer a flexible structure so I can change
> things host side. For example register multiple
> datamatches with same address and compare speed
> of access on first and last one.
> Similarly for address.

Ok, this makes sense.

>> (BTW, and unrelated to this, please use default-configs/ and
>> hw/Makefile.objs instead of hw/i386/Makefile.objs to enable the device).
> 
> Okay but why does pc-test device sit in hw/i386/Makefile.objs ?

Because someone wasn't looking. :)  The hw/ reorganization patches I've
posted fix that.

Paolo

>>>
>>>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>  hw/i386/Makefile.objs |   2 +-
>>>>>  hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  hw/pci/pci.h          |   1 +
>>>>>  3 files changed, 308 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 hw/pci-testdev.c
>>>>>
>>>>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>>>>> index a78c0b2..7497e7a 100644
>>>>> --- a/hw/i386/Makefile.objs
>>>>> +++ b/hw/i386/Makefile.objs
>>>>> @@ -11,7 +11,7 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>>>>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
>>>>>  obj-y += kvm/
>>>>>  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>>> -obj-y += pc-testdev.o
>>>>> +obj-y += pc-testdev.o pci-testdev.o
>>>>>  
>>>>>  obj-y := $(addprefix ../,$(obj-y))
>>>>>  
>>>>> diff --git a/hw/pci-testdev.c b/hw/pci-testdev.c
>>>>> new file mode 100644
>>>>> index 0000000..9486624
>>>>> --- /dev/null
>>>>> +++ b/hw/pci-testdev.c
>>>>> @@ -0,0 +1,306 @@
>>>>> +#include "hw/hw.h"
>>>>> +#include "hw/pci/pci.h"
>>>>> +#include "qemu/event_notifier.h"
>>>>> +#include "qemu/osdep.h"
>>>>> +
>>>>> +typedef struct PCITestDevHdr {
>>>>> +    uint8_t test;
>>>>> +    uint8_t width;
>>>>> +    uint8_t pad0[2];
>>>>> +    uint32_t offset;
>>>>> +    uint8_t data;
>>>>> +    uint8_t pad1[3];
>>>>> +    uint32_t count;
>>>>> +    uint8_t name[];
>>>>> +} PCITestDevHdr;
>>>>> +
>>>>> +typedef struct IOTest {
>>>>> +    MemoryRegion *mr;
>>>>> +    EventNotifier notifier;
>>>>> +    bool hasnotifier;
>>>>> +    unsigned size;
>>>>> +    bool match_data;
>>>>> +    PCITestDevHdr *hdr;
>>>>> +    unsigned bufsize;
>>>>> +} IOTest;
>>>>> +
>>>>> +#define IOTEST_DATAMATCH 0xFA
>>>>> +#define IOTEST_NOMATCH   0xCE
>>>>> +
>>>>> +#define IOTEST_IOSIZE 128
>>>>> +#define IOTEST_MEMSIZE 2048
>>>>> +
>>>>> +static const char *iotest_test[] = {
>>>>> +    "no-eventfd",
>>>>> +    "wildcard-eventfd",
>>>>> +    "datamatch-eventfd"
>>>>> +};
>>>>> +
>>>>> +static const char *iotest_type[] = {
>>>>> +    "mmio",
>>>>> +    "portio"
>>>>> +};
>>>>> +
>>>>> +#define IOTEST_TEST(i) (iotest_test[((i) % ARRAY_SIZE(iotest_test))])
>>>>> +#define IOTEST_TYPE(i) (iotest_type[((i) / ARRAY_SIZE(iotest_test))])
>>>>> +#define IOTEST_MAX_TEST (ARRAY_SIZE(iotest_test))
>>>>> +#define IOTEST_MAX_TYPE (ARRAY_SIZE(iotest_type))
>>>>> +#define IOTEST_MAX (IOTEST_MAX_TEST * IOTEST_MAX_TYPE)
>>>>> +
>>>>> +enum {
>>>>> +    IOTEST_ACCESS_NAME,
>>>>> +    IOTEST_ACCESS_DATA,
>>>>> +    IOTEST_ACCESS_MAX,
>>>>> +};
>>>>> +
>>>>> +#define IOTEST_ACCESS_TYPE uint8_t
>>>>> +#define IOTEST_ACCESS_WIDTH (sizeof(uint8_t))
>>>>> +
>>>>> +typedef struct PCITestDevState {
>>>>> +    PCIDevice dev;
>>>>> +    MemoryRegion mmio;
>>>>> +    MemoryRegion portio;
>>>>> +    IOTest *tests;
>>>>> +    int current;
>>>>> +} PCITestDevState;
>>>>> +
>>>>> +#define IOTEST_IS_MEM(i) (strcmp(IOTEST_TYPE(i), "portio"))
>>>>> +#define IOTEST_REGION(d, i) (IOTEST_IS_MEM(i) ?  &(d)->mmio : &(d)->portio)
>>>>> +#define IOTEST_SIZE(i) (IOTEST_IS_MEM(i) ? IOTEST_MEMSIZE : IOTEST_IOSIZE)
>>>>> +#define IOTEST_PCI_BAR(i) (IOTEST_IS_MEM(i) ? PCI_BASE_ADDRESS_SPACE_MEMORY : \
>>>>> +                           PCI_BASE_ADDRESS_SPACE_IO)
>>>>> +
>>>>> +static int pci_testdev_start(IOTest *test)
>>>>> +{
>>>>> +    test->hdr->count = 0;
>>>>> +    if (!test->hasnotifier) {
>>>>> +        return 0;
>>>>> +    }
>>>>> +    event_notifier_test_and_clear(&test->notifier);
>>>>> +    memory_region_add_eventfd(test->mr,
>>>>> +                              le32_to_cpu(test->hdr->offset),
>>>>> +                              test->size,
>>>>> +                              test->match_data,
>>>>> +                              test->hdr->data,
>>>>> +                              &test->notifier);
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void pci_testdev_stop(IOTest *test)
>>>>> +{
>>>>> +    if (!test->hasnotifier) {
>>>>> +        return;
>>>>> +    }
>>>>> +    memory_region_del_eventfd(test->mr,
>>>>> +                              le32_to_cpu(test->hdr->offset),
>>>>> +                              test->size,
>>>>> +                              test->match_data,
>>>>> +                              test->hdr->data,
>>>>> +                              &test->notifier);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +pci_testdev_reset(PCITestDevState *d)
>>>>> +{
>>>>> +    if (d->current == -1) {
>>>>> +        return;
>>>>> +    }
>>>>> +    pci_testdev_stop(&d->tests[d->current]);
>>>>> +    d->current = -1;
>>>>> +}
>>>>> +
>>>>> +static void pci_testdev_inc(IOTest *test, unsigned inc)
>>>>> +{
>>>>> +    uint32_t c = le32_to_cpu(test->hdr->count);
>>>>> +    test->hdr->count = cpu_to_le32(c + inc);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +pci_testdev_write(void *opaque, hwaddr addr, uint64_t val,
>>>>> +                  unsigned size, int type)
>>>>> +{
>>>>> +    PCITestDevState *d = opaque;
>>>>> +    IOTest *test;
>>>>> +    int t, r;
>>>>> +
>>>>> +    if (addr == offsetof(PCITestDevHdr, test)) {
>>>>> +        pci_testdev_reset(d);
>>>>> +        if (val >= IOTEST_MAX_TEST) {
>>>>> +            return;
>>>>> +        }
>>>>> +        t = type * IOTEST_MAX_TEST + val;
>>>>> +        r = pci_testdev_start(&d->tests[t]);
>>>>> +        if (r < 0) {
>>>>> +            return;
>>>>> +        }
>>>>> +        d->current = t;
>>>>> +        return;
>>>>> +    }
>>>>> +    if (d->current < 0) {
>>>>> +        return;
>>>>> +    }
>>>>> +    test = &d->tests[d->current];
>>>>> +    if (addr != le32_to_cpu(test->hdr->offset)) {
>>>>> +        return;
>>>>> +    }
>>>>> +    if (test->match_data && test->size != size) {
>>>>> +        return;
>>>>> +    }
>>>>> +    if (test->match_data && val != test->hdr->data) {
>>>>> +        return;
>>>>> +    }
>>>>> +    pci_testdev_inc(test, 1);
>>>>> +}
>>>>> +
>>>>> +static uint64_t
>>>>> +pci_testdev_read(void *opaque, hwaddr addr, unsigned size)
>>>>> +{
>>>>> +    PCITestDevState *d = opaque;
>>>>> +    const char *buf;
>>>>> +    IOTest *test;
>>>>> +    if (d->current < 0) {
>>>>> +        return 0;
>>>>> +    }
>>>>> +    test = &d->tests[d->current];
>>>>> +    buf = (const char *)test->hdr;
>>>>> +    if (addr + size >= test->bufsize) {
>>>>> +        return 0;
>>>>> +    }
>>>>> +    if (test->hasnotifier) {
>>>>> +        event_notifier_test_and_clear(&test->notifier);
>>>>> +    }
>>>>> +    return buf[addr];
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>>>>> +                       unsigned size)
>>>>> +{
>>>>> +    pci_testdev_write(opaque, addr, val, size, 0);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +pci_testdev_pio_write(void *opaque, hwaddr addr, uint64_t val,
>>>>> +                       unsigned size)
>>>>> +{
>>>>> +    pci_testdev_write(opaque, addr, val, size, 1);
>>>>> +}
>>>>> +
>>>>> +static const MemoryRegionOps pci_testdev_mmio_ops = {
>>>>> +    .read = pci_testdev_read,
>>>>> +    .write = pci_testdev_mmio_write,
>>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>> +    .impl = {
>>>>> +        .min_access_size = 1,
>>>>> +        .max_access_size = 1,
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static const MemoryRegionOps pci_testdev_pio_ops = {
>>>>> +    .read = pci_testdev_read,
>>>>> +    .write = pci_testdev_pio_write,
>>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>> +    .impl = {
>>>>> +        .min_access_size = 1,
>>>>> +        .max_access_size = 1,
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static int pci_testdev_init(PCIDevice *pci_dev)
>>>>> +{
>>>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, pci_dev);
>>>>> +    uint8_t *pci_conf;
>>>>> +    char *name;
>>>>> +    int r, i;
>>>>> +
>>>>> +    pci_conf = d->dev.config;
>>>>> +
>>>>> +    pci_conf[PCI_INTERRUPT_PIN] = 0; /* no interrupt pin */
>>>>> +
>>>>> +    memory_region_init_io(&d->mmio, &pci_testdev_mmio_ops, d,
>>>>> +                          "pci-testdev-mmio", IOTEST_MEMSIZE * 2);
>>>>> +    memory_region_init_io(&d->portio, &pci_testdev_pio_ops, d,
>>>>> +                          "pci-testdev-portio", IOTEST_IOSIZE * 2);
>>>>> +    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>>>>> +    pci_register_bar(&d->dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
>>>>> +
>>>>> +    d->current = -1;
>>>>> +    d->tests = g_malloc0(IOTEST_MAX * sizeof *d->tests);
>>>>> +    for (i = 0; i < IOTEST_MAX; ++i) {
>>>>> +        IOTest *test = &d->tests[i];
>>>>> +        name = g_strdup_printf("%s-%s", IOTEST_TYPE(i), IOTEST_TEST(i));
>>>>> +        test->bufsize = sizeof(PCITestDevHdr) + strlen(name) + 1;
>>>>> +        test->hdr = g_malloc0(test->bufsize);
>>>>> +        memcpy(test->hdr->name, name, strlen(name) + 1);
>>>>> +        g_free(name);
>>>>> +        test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
>>>>> +        test->size = IOTEST_ACCESS_WIDTH;
>>>>> +        test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
>>>>> +        test->hdr->test = i;
>>>>> +        test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
>>>>> +        test->hdr->width = IOTEST_ACCESS_WIDTH;
>>>>> +        test->mr = IOTEST_REGION(d, i);
>>>>> +        if (!strcmp(IOTEST_TEST(i), "no-eventfd")) {
>>>>> +            test->hasnotifier = false;
>>>>> +            continue;
>>>>> +        }
>>>>> +        r = event_notifier_init(&test->notifier, 0);
>>>>> +        assert(r >= 0);
>>>>> +        test->hasnotifier = true;
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +pci_testdev_uninit(PCIDevice *dev)
>>>>> +{
>>>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
>>>>> +    int i;
>>>>> +
>>>>> +    pci_testdev_reset(d);
>>>>> +    for (i = 0; i < IOTEST_MAX; ++i) {
>>>>> +        if (d->tests[i].hasnotifier) {
>>>>> +            event_notifier_cleanup(&d->tests[i].notifier);
>>>>> +        }
>>>>> +        g_free(d->tests[i].hdr);
>>>>> +    }
>>>>> +    g_free(d->tests);
>>>>> +    memory_region_destroy(&d->mmio);
>>>>> +    memory_region_destroy(&d->portio);
>>>>> +}
>>>>> +
>>>>> +static void qdev_pci_testdev_reset(DeviceState *dev)
>>>>> +{
>>>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev.qdev, dev);
>>>>> +    pci_testdev_reset(d);
>>>>> +}
>>>>> +
>>>>> +static void pci_testdev_class_init(ObjectClass *klass, void *data)
>>>>> +{
>>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>> +
>>>>> +    k->init = pci_testdev_init;
>>>>> +    k->exit = pci_testdev_uninit;
>>>>> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>>> +    k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
>>>>> +    k->revision = 0x00;
>>>>> +    k->class_id = PCI_CLASS_OTHERS;
>>>>> +    dc->desc = "PCI Test Device";
>>>>> +    dc->reset = qdev_pci_testdev_reset;
>>>>> +}
>>>>> +
>>>>> +static const TypeInfo pci_testdev_info = {
>>>>> +    .name          = "pci-testdev",
>>>>> +    .parent        = TYPE_PCI_DEVICE,
>>>>> +    .instance_size = sizeof(PCITestDevState),
>>>>> +    .class_init    = pci_testdev_class_init,
>>>>> +};
>>>>> +
>>>>> +static void pci_testdev_register_types(void)
>>>>> +{
>>>>> +    type_register_static(&pci_testdev_info);
>>>>> +}
>>>>> +
>>>>> +type_init(pci_testdev_register_types)
>>>>> diff --git a/hw/pci/pci.h b/hw/pci/pci.h
>>>>> index 774369c..d81198c 100644
>>>>> --- a/hw/pci/pci.h
>>>>> +++ b/hw/pci/pci.h
>>>>> @@ -84,6 +84,7 @@
>>>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL      0x0002
>>>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL2     0x0003
>>>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL4     0x0004
>>>>> +#define PCI_DEVICE_ID_REDHAT_TEST        0x0005
>>>>>  #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
>>>>>  
>>>>>  #define FMT_PCIBUS                      PRIx64
>>>>>

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 12:25:02PM +0200, Paolo Bonzini wrote:
> 
> > Exactly. I prefer a flexible structure so I can change
> > things host side. For example register multiple
> > datamatches with same address and compare speed
> > of access on first and last one.
> > Similarly for address.
> 
> Ok, this makes sense.
> 
> >> (BTW, and unrelated to this, please use default-configs/ and
> >> hw/Makefile.objs instead of hw/i386/Makefile.objs to enable the device).
> > 
> > Okay but why does pc-test device sit in hw/i386/Makefile.objs ?
> 
> Because someone wasn't looking. :)  The hw/ reorganization patches I've
> posted fix that.
> 
> Paolo

Okay.
Still not sure how to merge this, if it goes in through my tree
and that's the only comment, I'll just fix it silently ...

> >>>
> >>>
> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>> ---
> >>>>>  hw/i386/Makefile.objs |   2 +-
> >>>>>  hw/pci-testdev.c      | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>  hw/pci/pci.h          |   1 +
> >>>>>  3 files changed, 308 insertions(+), 1 deletion(-)
> >>>>>  create mode 100644 hw/pci-testdev.c
> >>>>>
> >>>>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> >>>>> index a78c0b2..7497e7a 100644
> >>>>> --- a/hw/i386/Makefile.objs
> >>>>> +++ b/hw/i386/Makefile.objs
> >>>>> @@ -11,7 +11,7 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> >>>>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
> >>>>>  obj-y += kvm/
> >>>>>  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>>>> -obj-y += pc-testdev.o
> >>>>> +obj-y += pc-testdev.o pci-testdev.o
> >>>>>  
> >>>>>  obj-y := $(addprefix ../,$(obj-y))
> >>>>>  
> >>>>> diff --git a/hw/pci-testdev.c b/hw/pci-testdev.c
> >>>>> new file mode 100644
> >>>>> index 0000000..9486624
> >>>>> --- /dev/null
> >>>>> +++ b/hw/pci-testdev.c
> >>>>> @@ -0,0 +1,306 @@
> >>>>> +#include "hw/hw.h"
> >>>>> +#include "hw/pci/pci.h"
> >>>>> +#include "qemu/event_notifier.h"
> >>>>> +#include "qemu/osdep.h"
> >>>>> +
> >>>>> +typedef struct PCITestDevHdr {
> >>>>> +    uint8_t test;
> >>>>> +    uint8_t width;
> >>>>> +    uint8_t pad0[2];
> >>>>> +    uint32_t offset;
> >>>>> +    uint8_t data;
> >>>>> +    uint8_t pad1[3];
> >>>>> +    uint32_t count;
> >>>>> +    uint8_t name[];
> >>>>> +} PCITestDevHdr;
> >>>>> +
> >>>>> +typedef struct IOTest {
> >>>>> +    MemoryRegion *mr;
> >>>>> +    EventNotifier notifier;
> >>>>> +    bool hasnotifier;
> >>>>> +    unsigned size;
> >>>>> +    bool match_data;
> >>>>> +    PCITestDevHdr *hdr;
> >>>>> +    unsigned bufsize;
> >>>>> +} IOTest;
> >>>>> +
> >>>>> +#define IOTEST_DATAMATCH 0xFA
> >>>>> +#define IOTEST_NOMATCH   0xCE
> >>>>> +
> >>>>> +#define IOTEST_IOSIZE 128
> >>>>> +#define IOTEST_MEMSIZE 2048
> >>>>> +
> >>>>> +static const char *iotest_test[] = {
> >>>>> +    "no-eventfd",
> >>>>> +    "wildcard-eventfd",
> >>>>> +    "datamatch-eventfd"
> >>>>> +};
> >>>>> +
> >>>>> +static const char *iotest_type[] = {
> >>>>> +    "mmio",
> >>>>> +    "portio"
> >>>>> +};
> >>>>> +
> >>>>> +#define IOTEST_TEST(i) (iotest_test[((i) % ARRAY_SIZE(iotest_test))])
> >>>>> +#define IOTEST_TYPE(i) (iotest_type[((i) / ARRAY_SIZE(iotest_test))])
> >>>>> +#define IOTEST_MAX_TEST (ARRAY_SIZE(iotest_test))
> >>>>> +#define IOTEST_MAX_TYPE (ARRAY_SIZE(iotest_type))
> >>>>> +#define IOTEST_MAX (IOTEST_MAX_TEST * IOTEST_MAX_TYPE)
> >>>>> +
> >>>>> +enum {
> >>>>> +    IOTEST_ACCESS_NAME,
> >>>>> +    IOTEST_ACCESS_DATA,
> >>>>> +    IOTEST_ACCESS_MAX,
> >>>>> +};
> >>>>> +
> >>>>> +#define IOTEST_ACCESS_TYPE uint8_t
> >>>>> +#define IOTEST_ACCESS_WIDTH (sizeof(uint8_t))
> >>>>> +
> >>>>> +typedef struct PCITestDevState {
> >>>>> +    PCIDevice dev;
> >>>>> +    MemoryRegion mmio;
> >>>>> +    MemoryRegion portio;
> >>>>> +    IOTest *tests;
> >>>>> +    int current;
> >>>>> +} PCITestDevState;
> >>>>> +
> >>>>> +#define IOTEST_IS_MEM(i) (strcmp(IOTEST_TYPE(i), "portio"))
> >>>>> +#define IOTEST_REGION(d, i) (IOTEST_IS_MEM(i) ?  &(d)->mmio : &(d)->portio)
> >>>>> +#define IOTEST_SIZE(i) (IOTEST_IS_MEM(i) ? IOTEST_MEMSIZE : IOTEST_IOSIZE)
> >>>>> +#define IOTEST_PCI_BAR(i) (IOTEST_IS_MEM(i) ? PCI_BASE_ADDRESS_SPACE_MEMORY : \
> >>>>> +                           PCI_BASE_ADDRESS_SPACE_IO)
> >>>>> +
> >>>>> +static int pci_testdev_start(IOTest *test)
> >>>>> +{
> >>>>> +    test->hdr->count = 0;
> >>>>> +    if (!test->hasnotifier) {
> >>>>> +        return 0;
> >>>>> +    }
> >>>>> +    event_notifier_test_and_clear(&test->notifier);
> >>>>> +    memory_region_add_eventfd(test->mr,
> >>>>> +                              le32_to_cpu(test->hdr->offset),
> >>>>> +                              test->size,
> >>>>> +                              test->match_data,
> >>>>> +                              test->hdr->data,
> >>>>> +                              &test->notifier);
> >>>>> +    return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static void pci_testdev_stop(IOTest *test)
> >>>>> +{
> >>>>> +    if (!test->hasnotifier) {
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    memory_region_del_eventfd(test->mr,
> >>>>> +                              le32_to_cpu(test->hdr->offset),
> >>>>> +                              test->size,
> >>>>> +                              test->match_data,
> >>>>> +                              test->hdr->data,
> >>>>> +                              &test->notifier);
> >>>>> +}
> >>>>> +
> >>>>> +static void
> >>>>> +pci_testdev_reset(PCITestDevState *d)
> >>>>> +{
> >>>>> +    if (d->current == -1) {
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    pci_testdev_stop(&d->tests[d->current]);
> >>>>> +    d->current = -1;
> >>>>> +}
> >>>>> +
> >>>>> +static void pci_testdev_inc(IOTest *test, unsigned inc)
> >>>>> +{
> >>>>> +    uint32_t c = le32_to_cpu(test->hdr->count);
> >>>>> +    test->hdr->count = cpu_to_le32(c + inc);
> >>>>> +}
> >>>>> +
> >>>>> +static void
> >>>>> +pci_testdev_write(void *opaque, hwaddr addr, uint64_t val,
> >>>>> +                  unsigned size, int type)
> >>>>> +{
> >>>>> +    PCITestDevState *d = opaque;
> >>>>> +    IOTest *test;
> >>>>> +    int t, r;
> >>>>> +
> >>>>> +    if (addr == offsetof(PCITestDevHdr, test)) {
> >>>>> +        pci_testdev_reset(d);
> >>>>> +        if (val >= IOTEST_MAX_TEST) {
> >>>>> +            return;
> >>>>> +        }
> >>>>> +        t = type * IOTEST_MAX_TEST + val;
> >>>>> +        r = pci_testdev_start(&d->tests[t]);
> >>>>> +        if (r < 0) {
> >>>>> +            return;
> >>>>> +        }
> >>>>> +        d->current = t;
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    if (d->current < 0) {
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    test = &d->tests[d->current];
> >>>>> +    if (addr != le32_to_cpu(test->hdr->offset)) {
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    if (test->match_data && test->size != size) {
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    if (test->match_data && val != test->hdr->data) {
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    pci_testdev_inc(test, 1);
> >>>>> +}
> >>>>> +
> >>>>> +static uint64_t
> >>>>> +pci_testdev_read(void *opaque, hwaddr addr, unsigned size)
> >>>>> +{
> >>>>> +    PCITestDevState *d = opaque;
> >>>>> +    const char *buf;
> >>>>> +    IOTest *test;
> >>>>> +    if (d->current < 0) {
> >>>>> +        return 0;
> >>>>> +    }
> >>>>> +    test = &d->tests[d->current];
> >>>>> +    buf = (const char *)test->hdr;
> >>>>> +    if (addr + size >= test->bufsize) {
> >>>>> +        return 0;
> >>>>> +    }
> >>>>> +    if (test->hasnotifier) {
> >>>>> +        event_notifier_test_and_clear(&test->notifier);
> >>>>> +    }
> >>>>> +    return buf[addr];
> >>>>> +}
> >>>>> +
> >>>>> +static void
> >>>>> +pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
> >>>>> +                       unsigned size)
> >>>>> +{
> >>>>> +    pci_testdev_write(opaque, addr, val, size, 0);
> >>>>> +}
> >>>>> +
> >>>>> +static void
> >>>>> +pci_testdev_pio_write(void *opaque, hwaddr addr, uint64_t val,
> >>>>> +                       unsigned size)
> >>>>> +{
> >>>>> +    pci_testdev_write(opaque, addr, val, size, 1);
> >>>>> +}
> >>>>> +
> >>>>> +static const MemoryRegionOps pci_testdev_mmio_ops = {
> >>>>> +    .read = pci_testdev_read,
> >>>>> +    .write = pci_testdev_mmio_write,
> >>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
> >>>>> +    .impl = {
> >>>>> +        .min_access_size = 1,
> >>>>> +        .max_access_size = 1,
> >>>>> +    },
> >>>>> +};
> >>>>> +
> >>>>> +static const MemoryRegionOps pci_testdev_pio_ops = {
> >>>>> +    .read = pci_testdev_read,
> >>>>> +    .write = pci_testdev_pio_write,
> >>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
> >>>>> +    .impl = {
> >>>>> +        .min_access_size = 1,
> >>>>> +        .max_access_size = 1,
> >>>>> +    },
> >>>>> +};
> >>>>> +
> >>>>> +static int pci_testdev_init(PCIDevice *pci_dev)
> >>>>> +{
> >>>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, pci_dev);
> >>>>> +    uint8_t *pci_conf;
> >>>>> +    char *name;
> >>>>> +    int r, i;
> >>>>> +
> >>>>> +    pci_conf = d->dev.config;
> >>>>> +
> >>>>> +    pci_conf[PCI_INTERRUPT_PIN] = 0; /* no interrupt pin */
> >>>>> +
> >>>>> +    memory_region_init_io(&d->mmio, &pci_testdev_mmio_ops, d,
> >>>>> +                          "pci-testdev-mmio", IOTEST_MEMSIZE * 2);
> >>>>> +    memory_region_init_io(&d->portio, &pci_testdev_pio_ops, d,
> >>>>> +                          "pci-testdev-portio", IOTEST_IOSIZE * 2);
> >>>>> +    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> >>>>> +    pci_register_bar(&d->dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> >>>>> +
> >>>>> +    d->current = -1;
> >>>>> +    d->tests = g_malloc0(IOTEST_MAX * sizeof *d->tests);
> >>>>> +    for (i = 0; i < IOTEST_MAX; ++i) {
> >>>>> +        IOTest *test = &d->tests[i];
> >>>>> +        name = g_strdup_printf("%s-%s", IOTEST_TYPE(i), IOTEST_TEST(i));
> >>>>> +        test->bufsize = sizeof(PCITestDevHdr) + strlen(name) + 1;
> >>>>> +        test->hdr = g_malloc0(test->bufsize);
> >>>>> +        memcpy(test->hdr->name, name, strlen(name) + 1);
> >>>>> +        g_free(name);
> >>>>> +        test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
> >>>>> +        test->size = IOTEST_ACCESS_WIDTH;
> >>>>> +        test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
> >>>>> +        test->hdr->test = i;
> >>>>> +        test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
> >>>>> +        test->hdr->width = IOTEST_ACCESS_WIDTH;
> >>>>> +        test->mr = IOTEST_REGION(d, i);
> >>>>> +        if (!strcmp(IOTEST_TEST(i), "no-eventfd")) {
> >>>>> +            test->hasnotifier = false;
> >>>>> +            continue;
> >>>>> +        }
> >>>>> +        r = event_notifier_init(&test->notifier, 0);
> >>>>> +        assert(r >= 0);
> >>>>> +        test->hasnotifier = true;
> >>>>> +    }
> >>>>> +
> >>>>> +    return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static void
> >>>>> +pci_testdev_uninit(PCIDevice *dev)
> >>>>> +{
> >>>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
> >>>>> +    int i;
> >>>>> +
> >>>>> +    pci_testdev_reset(d);
> >>>>> +    for (i = 0; i < IOTEST_MAX; ++i) {
> >>>>> +        if (d->tests[i].hasnotifier) {
> >>>>> +            event_notifier_cleanup(&d->tests[i].notifier);
> >>>>> +        }
> >>>>> +        g_free(d->tests[i].hdr);
> >>>>> +    }
> >>>>> +    g_free(d->tests);
> >>>>> +    memory_region_destroy(&d->mmio);
> >>>>> +    memory_region_destroy(&d->portio);
> >>>>> +}
> >>>>> +
> >>>>> +static void qdev_pci_testdev_reset(DeviceState *dev)
> >>>>> +{
> >>>>> +    PCITestDevState *d = DO_UPCAST(PCITestDevState, dev.qdev, dev);
> >>>>> +    pci_testdev_reset(d);
> >>>>> +}
> >>>>> +
> >>>>> +static void pci_testdev_class_init(ObjectClass *klass, void *data)
> >>>>> +{
> >>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >>>>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>>>> +
> >>>>> +    k->init = pci_testdev_init;
> >>>>> +    k->exit = pci_testdev_uninit;
> >>>>> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
> >>>>> +    k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
> >>>>> +    k->revision = 0x00;
> >>>>> +    k->class_id = PCI_CLASS_OTHERS;
> >>>>> +    dc->desc = "PCI Test Device";
> >>>>> +    dc->reset = qdev_pci_testdev_reset;
> >>>>> +}
> >>>>> +
> >>>>> +static const TypeInfo pci_testdev_info = {
> >>>>> +    .name          = "pci-testdev",
> >>>>> +    .parent        = TYPE_PCI_DEVICE,
> >>>>> +    .instance_size = sizeof(PCITestDevState),
> >>>>> +    .class_init    = pci_testdev_class_init,
> >>>>> +};
> >>>>> +
> >>>>> +static void pci_testdev_register_types(void)
> >>>>> +{
> >>>>> +    type_register_static(&pci_testdev_info);
> >>>>> +}
> >>>>> +
> >>>>> +type_init(pci_testdev_register_types)
> >>>>> diff --git a/hw/pci/pci.h b/hw/pci/pci.h
> >>>>> index 774369c..d81198c 100644
> >>>>> --- a/hw/pci/pci.h
> >>>>> +++ b/hw/pci/pci.h
> >>>>> @@ -84,6 +84,7 @@
> >>>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL      0x0002
> >>>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL2     0x0003
> >>>>>  #define PCI_DEVICE_ID_REDHAT_SERIAL4     0x0004
> >>>>> +#define PCI_DEVICE_ID_REDHAT_TEST        0x0005
> >>>>>  #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
> >>>>>  
> >>>>>  #define FMT_PCIBUS                      PRIx64
> >>>>>

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 12:33, Michael S. Tsirkin ha scritto:
> > Because someone wasn't looking. :)  The hw/ reorganization patches I've
> > posted fix that.
> 
> Still not sure how to merge this, if it goes in through my tree
> and that's the only comment, I'll just fix it silently ...

Indeed.  In fact, considering my hw/ reorganization patches will be
committed soon, please put it in hw/pci (matching hw/isa/pc-testdev.c).

Another comment is that you should add a description in docs/specs.

Paolo

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 12:34:24PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 12:33, Michael S. Tsirkin ha scritto:
> > > Because someone wasn't looking. :)  The hw/ reorganization patches I've
> > > posted fix that.
> > 
> > Still not sure how to merge this, if it goes in through my tree
> > and that's the only comment, I'll just fix it silently ...
> 
> Indeed.  In fact, considering my hw/ reorganization patches will be
> committed soon, please put it in hw/pci (matching hw/isa/pc-testdev.c).

Paolo, hw/pci is pci core, I haven't looked at your reorg patches,
but please do not move devices there.
Sorting devices by connection is also wrong I think, by function
would be better.

> Another comment is that you should add a description in docs/specs.
> 
> Paolo

pc-test has none ... I'll see what I can do.

-- 
MST

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 12:38, Michael S. Tsirkin ha scritto:
> On Wed, Apr 03, 2013 at 12:34:24PM +0200, Paolo Bonzini wrote:
>> Il 03/04/2013 12:33, Michael S. Tsirkin ha scritto:
>>>> Because someone wasn't looking. :)  The hw/ reorganization patches I've
>>>> posted fix that.
>>>
>>> Still not sure how to merge this, if it goes in through my tree
>>> and that's the only comment, I'll just fix it silently ...
>>
>> Indeed.  In fact, considering my hw/ reorganization patches will be
>> committed soon, please put it in hw/pci (matching hw/isa/pc-testdev.c).
> 
> Paolo, hw/pci is pci core, I haven't looked at your reorg patches,
> but please do not move devices there.
> Sorting devices by connection is also wrong I think, by function
> would be better.

Indeed that's how most devices are sorted.  For example, PCI host
devices/bridges/etc. are in hw/pci (together with the PCI core, making
hw/pci basically all that goes in through your tree), ISA host devices
are in hw/isa, etc.

However, there are a few exceptions.  You cannot really sort out 600
files without exceptions.  All USB devices are in hw/usb, and the
existing test devices (debugexit, testdev) are in hw/isa.

There is only one exception you should care about, namely that VFIO and
ivshmem are also in hw/pci.

Here is the list of files in hw/pci:

host-apb.c
host-bonito.c
host-dec.c
host-dec.h
host-grackle.c
host-gt64xxx.c
host-piix.c
host-ppc4xx.c
host-ppce500.c
host-prep.c
host-q35.c
host-sh.c
host-spapr.c
host-uninorth.c
host-versatile.c
i82801b11.c
ioh3420.c
ioh3420.h
ivshmem.c
msi.c
msi.h
msix.c
msix.h
pam.c
pci-hotplug.c
pci-stub.c
pci.c
pci.h
pci_bridge.c
pci_bridge.h
pci_bridge_dev.c
pci_bus.h
pci_host.c
pci_host.h
pci_ids.h
pci_regs.h
pcie.c
pcie.h
pcie_aer.c
pcie_aer.h
pcie_host.c
pcie_host.h
pcie_port.c
pcie_port.h
pcie_regs.h
shpc.c
shpc.h
slotid_cap.c
slotid_cap.h
vfio.c
xio3130_downstream.c
xio3130_downstream.h
xio3130_upstream.c
xio3130_upstream.h

Paolo

> 
>> Another comment is that you should add a description in docs/specs.
>>
>> Paolo
> 
> pc-test has none ... I'll see what I can do.
> 

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 01:48:41PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 12:38, Michael S. Tsirkin ha scritto:
> > On Wed, Apr 03, 2013 at 12:34:24PM +0200, Paolo Bonzini wrote:
> >> Il 03/04/2013 12:33, Michael S. Tsirkin ha scritto:
> >>>> Because someone wasn't looking. :)  The hw/ reorganization patches I've
> >>>> posted fix that.
> >>>
> >>> Still not sure how to merge this, if it goes in through my tree
> >>> and that's the only comment, I'll just fix it silently ...
> >>
> >> Indeed.  In fact, considering my hw/ reorganization patches will be
> >> committed soon, please put it in hw/pci (matching hw/isa/pc-testdev.c).
> > 
> > Paolo, hw/pci is pci core, I haven't looked at your reorg patches,
> > but please do not move devices there.
> > Sorting devices by connection is also wrong I think, by function
> > would be better.
> 
> Indeed that's how most devices are sorted.  For example, PCI host
> devices/bridges/etc. are in hw/pci (together with the PCI core, making
> hw/pci basically all that goes in through your tree),

Well host bridges often do lots of things besides pci on the same
chip.

> ISA host devices
> are in hw/isa, etc.

what do you mean "host ISA device"?

> However, there are a few exceptions.  You cannot really sort out 600
> files without exceptions.  All USB devices are in hw/usb, and the
> existing test devices (debugexit, testdev) are in hw/isa.
> 
> There is only one exception you should care about, namely that VFIO and
> ivshmem are also in hw/pci.

This makes no sense really.
Pls add hw/misc or just keep misc stuff in hw/
We don't need to have everything in subdirectories.

> Here is the list of files in hw/pci:
> 
> host-apb.c
> host-bonito.c
> host-dec.c
> host-dec.h
> host-grackle.c
> host-gt64xxx.c
> host-piix.c
> host-ppc4xx.c
> host-ppce500.c
> host-prep.c
> host-q35.c
> host-sh.c
> host-spapr.c
> host-uninorth.c
> host-versatile.c
> i82801b11.c
> ioh3420.c
> ioh3420.h
> ivshmem.c
> msi.c
> msi.h
> msix.c
> msix.h
> pam.c
> pci-hotplug.c
> pci-stub.c
> pci.c
> pci.h
> pci_bridge.c
> pci_bridge.h
> pci_bridge_dev.c
> pci_bus.h
> pci_host.c
> pci_host.h
> pci_ids.h
> pci_regs.h
> pcie.c
> pcie.h
> pcie_aer.c
> pcie_aer.h
> pcie_host.c
> pcie_host.h
> pcie_port.c
> pcie_port.h
> pcie_regs.h
> shpc.c
> shpc.h
> slotid_cap.c
> slotid_cap.h
> vfio.c
> xio3130_downstream.c
> xio3130_downstream.h
> xio3130_upstream.c
> xio3130_upstream.h
> 
> Paolo

This messes up things.  pci core is separate from devices using it,
and it's important to me. Really just add hw/bridge/ and put all
kind of bridge devices there.

> > 
> >> Another comment is that you should add a description in docs/specs.
> >>
> >> Paolo
> > 
> > pc-test has none ... I'll see what I can do.
> > 

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 14:00, Michael S. Tsirkin ha scritto:
> On Wed, Apr 03, 2013 at 01:48:41PM +0200, Paolo Bonzini wrote:
>> Il 03/04/2013 12:38, Michael S. Tsirkin ha scritto:
>>> On Wed, Apr 03, 2013 at 12:34:24PM +0200, Paolo Bonzini wrote:
>>>> Il 03/04/2013 12:33, Michael S. Tsirkin ha scritto:
>>>>>> Because someone wasn't looking. :)  The hw/ reorganization patches I've
>>>>>> posted fix that.
>>>>>
>>>>> Still not sure how to merge this, if it goes in through my tree
>>>>> and that's the only comment, I'll just fix it silently ...
>>>>
>>>> Indeed.  In fact, considering my hw/ reorganization patches will be
>>>> committed soon, please put it in hw/pci (matching hw/isa/pc-testdev.c).
>>>
>>> Paolo, hw/pci is pci core, I haven't looked at your reorg patches,
>>> but please do not move devices there.
>>> Sorting devices by connection is also wrong I think, by function
>>> would be better.
>>
>> Indeed that's how most devices are sorted.  For example, PCI host
>> devices/bridges/etc. are in hw/pci (together with the PCI core, making
>> hw/pci basically all that goes in through your tree),
> 
> Well host bridges often do lots of things besides pci on the same
> chip.

Still the PCIness is quite important, seeing that almost all such
devices have a name that looks like *_pci.c.

>> ISA host devices
>> are in hw/isa, etc.
> 
> what do you mean "host ISA device"?

PCI-ISA bridges and SuperIO chips: piix4.c, vt82c686.c, lpc_ich9.c,
i82378.c.  PIIX3 would also be there, it needs to be split out of
piix_pci.c.

>> However, there are a few exceptions.  You cannot really sort out 600
>> files without exceptions.  All USB devices are in hw/usb, and the
>> existing test devices (debugexit, testdev) are in hw/isa.
>>
>> There is only one exception you should care about, namely that VFIO and
>> ivshmem are also in hw/pci.
> 
> This makes no sense really.
> Pls add hw/misc or just keep misc stuff in hw/

hw/misc exists already, I'll move those two there.

>> Here is the list of files in hw/pci:
>>
>> host-apb.c
>> host-bonito.c
>> host-dec.c
>> host-dec.h
>> host-grackle.c
>> host-gt64xxx.c
>> host-piix.c
>> host-ppc4xx.c
>> host-ppce500.c
>> host-prep.c
>> host-q35.c
>> host-sh.c
>> host-spapr.c
>> host-uninorth.c
>> host-versatile.c
>> i82801b11.c
>> ioh3420.c
>> ioh3420.h
>> ivshmem.c
>> msi.c
>> msi.h
>> msix.c
>> msix.h
>> pam.c
>> pci-hotplug.c
>> pci-stub.c
>> pci.c
>> pci.h
>> pci_bridge.c
>> pci_bridge.h
>> pci_bridge_dev.c
>> pci_bus.h
>> pci_host.c
>> pci_host.h
>> pci_ids.h
>> pci_regs.h
>> pcie.c
>> pcie.h
>> pcie_aer.c
>> pcie_aer.h
>> pcie_host.c
>> pcie_host.h
>> pcie_port.c
>> pcie_port.h
>> pcie_regs.h
>> shpc.c
>> shpc.h
>> slotid_cap.c
>> slotid_cap.h
>> vfio.c
>> xio3130_downstream.c
>> xio3130_downstream.h
>> xio3130_upstream.c
>> xio3130_upstream.h
>>
>> Paolo
> 
> This messes up things.  pci core is separate from devices using it,
> and it's important to me. Really just add hw/bridge/ and put all
> kind of bridge devices there.

Ok, I'll add hw/pci/bridge, and remove the "host-" prefix for host PCI
devices.

Paolo

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 02:05:32PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 14:00, Michael S. Tsirkin ha scritto:
> > On Wed, Apr 03, 2013 at 01:48:41PM +0200, Paolo Bonzini wrote:
> >> Il 03/04/2013 12:38, Michael S. Tsirkin ha scritto:
> >>> On Wed, Apr 03, 2013 at 12:34:24PM +0200, Paolo Bonzini wrote:
> >>>> Il 03/04/2013 12:33, Michael S. Tsirkin ha scritto:
> >>>>>> Because someone wasn't looking. :)  The hw/ reorganization patches I've
> >>>>>> posted fix that.
> >>>>>
> >>>>> Still not sure how to merge this, if it goes in through my tree
> >>>>> and that's the only comment, I'll just fix it silently ...
> >>>>
> >>>> Indeed.  In fact, considering my hw/ reorganization patches will be
> >>>> committed soon, please put it in hw/pci (matching hw/isa/pc-testdev.c).
> >>>
> >>> Paolo, hw/pci is pci core, I haven't looked at your reorg patches,
> >>> but please do not move devices there.
> >>> Sorting devices by connection is also wrong I think, by function
> >>> would be better.
> >>
> >> Indeed that's how most devices are sorted.  For example, PCI host
> >> devices/bridges/etc. are in hw/pci (together with the PCI core, making
> >> hw/pci basically all that goes in through your tree),
> > 
> > Well host bridges often do lots of things besides pci on the same
> > chip.
> 
> Still the PCIness is quite important, seeing that almost all such
> devices have a name that looks like *_pci.c.

Some of them do. For some of them name is not a very successful name.

> >> ISA host devices
> >> are in hw/isa, etc.
> > 
> > what do you mean "host ISA device"?
> 
> PCI-ISA bridges and SuperIO chips: piix4.c, vt82c686.c, lpc_ich9.c,
> i82378.c.  PIIX3 would also be there, it needs to be split out of
> piix_pci.c.
> 
> >> However, there are a few exceptions.  You cannot really sort out 600
> >> files without exceptions.  All USB devices are in hw/usb, and the
> >> existing test devices (debugexit, testdev) are in hw/isa.
> >>
> >> There is only one exception you should care about, namely that VFIO and
> >> ivshmem are also in hw/pci.
> > 
> > This makes no sense really.
> > Pls add hw/misc or just keep misc stuff in hw/
> 
> hw/misc exists already, I'll move those two there.
> 
> >> Here is the list of files in hw/pci:
> >>
> >> host-apb.c
> >> host-bonito.c
> >> host-dec.c
> >> host-dec.h
> >> host-grackle.c
> >> host-gt64xxx.c
> >> host-piix.c
> >> host-ppc4xx.c
> >> host-ppce500.c
> >> host-prep.c
> >> host-q35.c
> >> host-sh.c
> >> host-spapr.c
> >> host-uninorth.c
> >> host-versatile.c
> >> i82801b11.c
> >> ioh3420.c
> >> ioh3420.h
> >> ivshmem.c
> >> msi.c
> >> msi.h
> >> msix.c
> >> msix.h
> >> pam.c
> >> pci-hotplug.c
> >> pci-stub.c
> >> pci.c
> >> pci.h
> >> pci_bridge.c
> >> pci_bridge.h
> >> pci_bridge_dev.c
> >> pci_bus.h
> >> pci_host.c
> >> pci_host.h
> >> pci_ids.h
> >> pci_regs.h
> >> pcie.c
> >> pcie.h
> >> pcie_aer.c
> >> pcie_aer.h
> >> pcie_host.c
> >> pcie_host.h
> >> pcie_port.c
> >> pcie_port.h
> >> pcie_regs.h
> >> shpc.c
> >> shpc.h
> >> slotid_cap.c
> >> slotid_cap.h
> >> vfio.c
> >> xio3130_downstream.c
> >> xio3130_downstream.h
> >> xio3130_upstream.c
> >> xio3130_upstream.h
> >>
> >> Paolo
> > 
> > This messes up things.  pci core is separate from devices using it,
> > and it's important to me. Really just add hw/bridge/ and put all
> > kind of bridge devices there.
> 
> Ok, I'll add hw/pci/bridge, and remove the "host-" prefix for host PCI
> devices.
> 
> Paolo

That's too much nesting I think. hw/bridge/ and we can put isa bridges there
as well then.

-- 
MST

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 16:06, Michael S. Tsirkin ha scritto:
> > Ok, I'll add hw/pci/bridge, and remove the "host-" prefix for host PCI
> > devices.
>
> That's too much nesting I think. hw/bridge/ and we can put isa bridges there
> as well then.

You need to group similar devices for the nesting to be useful.  For
example, it should be easy to check if something is true of all ISA
bridges, or to do the same change in all of them.  ISA and PCI bridges
have too little in common for that (and why not put I2C and SPI in
hw/bridge too :)).

Paolo

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 04:08:45PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 16:06, Michael S. Tsirkin ha scritto:
> > > Ok, I'll add hw/pci/bridge, and remove the "host-" prefix for host PCI
> > > devices.
> >
> > That's too much nesting I think. hw/bridge/ and we can put isa bridges there
> > as well then.
> 
> You need to group similar devices for the nesting to be useful.  For
> example, it should be easy to check if something is true of all ISA
> bridges, or to do the same change in all of them.  ISA and PCI bridges
> have too little in common for that (and why not put I2C and SPI in
> hw/bridge too :)).
> 
> Paolo

Yes, why not. What all bridges need to share is their modeling
needs to be similar. That's one thing that practically
needs to be cleaned up I think.

-- 
MST

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 16:28, Michael S. Tsirkin ha scritto:
> > You need to group similar devices for the nesting to be useful.  For
> > example, it should be easy to check if something is true of all ISA
> > bridges, or to do the same change in all of them.  ISA and PCI bridges
> > have too little in common for that (and why not put I2C and SPI in
> > hw/bridge too :)).
> 
> Yes, why not. What all bridges need to share is their modeling
> needs to be similar. That's one thing that practically
> needs to be cleaned up I think.

Bridges are simply devices that expose their own bus, or that derive
from a class that does.  But bridge is not a universal word, some buses
use controller or adapter, it would be weird to have hw/bridge/i2c or
hw/bridge/scsi (and leave two files only in hw/scsi).

> But will this conflict with how libhw works at the moment?
> We don't want to rebuild pci for each target ...

No, we won't.  In fact, almost everything should be built once only.  As
far as PCI is concerned, if it's not it is because of some really bad
hacks.  For unmaintained boards, it's best to stash them in hw/ARCH.

If we limit the amount of files that are built per-target, it works nicely.

Paolo

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 05:46:00PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 16:28, Michael S. Tsirkin ha scritto:
> > > You need to group similar devices for the nesting to be useful.  For
> > > example, it should be easy to check if something is true of all ISA
> > > bridges, or to do the same change in all of them.  ISA and PCI bridges
> > > have too little in common for that (and why not put I2C and SPI in
> > > hw/bridge too :)).
> > 
> > Yes, why not. What all bridges need to share is their modeling
> > needs to be similar. That's one thing that practically
> > needs to be cleaned up I think.
> 
> Bridges are simply devices that expose their own bus, or that derive
> from a class that does.  But bridge is not a universal word, some buses
> use controller or adapter, it would be weird to have hw/bridge/i2c or
> hw/bridge/scsi (and leave two files only in hw/scsi).
> 
> > But will this conflict with how libhw works at the moment?
> > We don't want to rebuild pci for each target ...
> 
> No, we won't.  In fact, almost everything should be built once only.  As
> far as PCI is concerned, if it's not it is because of some really bad
> hacks.  For unmaintained boards, it's best to stash them in hw/ARCH.
> 
> If we limit the amount of files that are built per-target, it works nicely.
> 
> Paolo

Well ATM it's part of libhw and is built twice. Not sure what
do you propose here.

-- 
MST

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 19:04, Michael S. Tsirkin ha scritto:
> Well ATM it's part of libhw and is built twice. Not sure what
> do you propose here.

Things haven't been built twice for a few months.  hwaddr is
unconditionally 64-bit wide.

Paolo

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 04:08:45PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 16:06, Michael S. Tsirkin ha scritto:
> > > Ok, I'll add hw/pci/bridge, and remove the "host-" prefix for host PCI
> > > devices.
> >
> > That's too much nesting I think. hw/bridge/ and we can put isa bridges there
> > as well then.
> 
> You need to group similar devices for the nesting to be useful.  For
> example, it should be easy to check if something is true of all ISA
> bridges, or to do the same change in all of them.  ISA and PCI bridges
> have too little in common for that (and why not put I2C and SPI in
> hw/bridge too :)).
> 
> Paolo

At some level it might make sense to have pci/ and isa/
with core code at the top level. Specific
devices would go into hw/pci/ hw/isa/ ...
But will this conflict with how libhw works at the moment?
We don't want to rebuild pci for each target ...

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 04:08:45PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 16:06, Michael S. Tsirkin ha scritto:
> > > Ok, I'll add hw/pci/bridge, and remove the "host-" prefix for host PCI
> > > devices.
> >
> > That's too much nesting I think. hw/bridge/ and we can put isa bridges there
> > as well then.
> 
> You need to group similar devices for the nesting to be useful.  For
> example, it should be easy to check if something is true of all ISA
> bridges, or to do the same change in all of them.  ISA and PCI bridges
> have too little in common for that (and why not put I2C and SPI in
> hw/bridge too :)).
> 
> Paolo

Okay after some more thought.
hw/pci-host/
hw/pci-bridge/
hw/isa-bridge/

?

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

Il 03/04/2013 17:09, Michael S. Tsirkin ha scritto:
> Okay after some more thought.
> hw/pci-host/
> hw/pci-bridge/
> hw/isa-bridge/

Renaming hw/isa to hw/isa-bridge is easy.

For the rest, I would prefer hw/pci/{core,host,bridge}, but whatever you
like the most is fine for me too.

Paolo

Re: [PATCH 4/4] pci: add pci test device

[Michael S. Tsirkin]

On Wed, Apr 03, 2013 at 05:43:40PM +0200, Paolo Bonzini wrote:
> Il 03/04/2013 17:09, Michael S. Tsirkin ha scritto:
> > Okay after some more thought.
> > hw/pci-host/
> > hw/pci-bridge/
> > hw/isa-bridge/
> 
> Renaming hw/isa to hw/isa-bridge is easy.
> 
> For the rest, I would prefer hw/pci/{core,host,bridge}, but whatever you
> like the most is fine for me too.
> 
> Paolo

I'd prefer a single level as above, I get lost easily.

Re: [PATCH 4/4] pci: add pci test device

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Michael S. Tsirkin" <mst@redhat.com>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, gleb@redhat.com, mtosatti@redhat.com
> Inviato: Mercoledì, 3 aprile 2013 20:39:58
> Oggetto: Re: [PATCH 4/4] pci: add pci test device
> 
> On Wed, Apr 03, 2013 at 05:43:40PM +0200, Paolo Bonzini wrote:
> > Il 03/04/2013 17:09, Michael S. Tsirkin ha scritto:
> > > Okay after some more thought.
> > > hw/pci-host/
> > > hw/pci-bridge/
> > > hw/isa-bridge/
> > 
> > Renaming hw/isa to hw/isa-bridge is easy.
> > 
> > For the rest, I would prefer hw/pci/{core,host,bridge}, but whatever you
> > like the most is fine for me too.
> > 
> > Paolo
> 
> I'd prefer a single level as above, I get lost easily.

The reason why I prefer deep structures is that I would like
the directories to become the basis for a Kconfig menu.  But I'll
use the shallow structure, most of those symbols except in pci-bridge
would probably be select-ed by the high level "board" symbols anyway.

Paolo