Re: [Qemu-devel] [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 <address@hidden>
>>> ---
>>>  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
>>>