Re: [Qemu-devel] [PATCH] pci-testdev: enhance to support new testcases

[Andrew Jones]

On Thu, Sep 22, 2016 at 02:15:08PM +0800, Peter Xu wrote:
> pci-testdev is used mostly in kvm-unit-test for some eventfd tests.
> However I see it a good framework for other tests as well (e.g., the
> IOMMU unit test in the future). So enhanced it to support more
> testcases.
> 
> The original memory handlers and protocol are strict and not easy to
> change (we need to keep the old behavior of pci-testdev). So I added a
> new parameter for the device, and memory ops will be dynamically handled
> depending on what testcase it is configured. To specify a new test case
> for pci-testdev, we use:
> 
>   -device pci-testdev,testcase=XXX
> 
> The default will be "eventfd", which is the original behavior for
> pci-testdev. In the future, we can just add new testcase for pci-testdev
> to achieve different goals.
> 
> Signed-off-by: Peter Xu <address@hidden>
> ---
> 
>  This is kind-of a RFC since I am not sure whether this is a good way.

I'm not either :-) I haven't looked too closely at this test device,
but I have been involved in reviewing a kvm-unit-tests series[*] that
will drive it. Please take a look at that series and maybe test with
it as well.

Thanks,
drew

[*] https://www.spinics.net/lists/kvm/msg136892.html

>  I did run the default kvm-unit-test cases on x86 to make sure it
>  didn't break anything. In case this is not good, I didn't write any
>  further test cases (e.g., emulate device DMAR/IR operations) yet. If
>  we like the idea, I can move on.
> 
>  Please kindly review. Thanks.
> 
>  hw/misc/pci-testdev.c | 80 
> ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/misc/pci-testdev.c b/hw/misc/pci-testdev.c
> index 7d59902..b25d673 100644
> --- a/hw/misc/pci-testdev.c
> +++ b/hw/misc/pci-testdev.c
> @@ -22,6 +22,19 @@
>  #include "hw/pci/pci.h"
>  #include "qemu/event_notifier.h"
>  #include "sysemu/kvm.h"
> +#include "qemu/error-report.h"
> +
> +/* Type: 0 for MMIO write, 1 for PIO write. */
> +typedef void (*pci_testdev_write_op)(void *opaque, hwaddr addr,
> +                                     uint64_t val, unsigned size, int type);
> +typedef uint64_t (*pci_testdev_read_op)(void *opaque, hwaddr addr,
> +                                        unsigned size);
> +
> +struct testcase {
> +    const char *name;
> +    pci_testdev_write_op write_op;
> +    pci_testdev_read_op read_op;
> +};
>  
>  typedef struct PCITestDevHdr {
>      uint8_t test;
> @@ -85,6 +98,8 @@ typedef struct PCITestDevState {
>      MemoryRegion portio;
>      IOTest *tests;
>      int current;
> +    char *testcase_name;
> +    struct testcase *testcase;
>  } PCITestDevState;
>  
>  #define TYPE_PCI_TEST_DEV "pci-testdev"
> @@ -200,22 +215,58 @@ pci_testdev_read(void *opaque, hwaddr addr, unsigned 
> size)
>      return buf[addr];
>  }
>  
> +/*
> + * To add a new test, we need to implement both write_op and read_op,
> + * and add a new "struct testcase" into the global pci_testcases[].
> + */
> +struct testcase pci_testcases[] = {
> +    {"eventfd", pci_testdev_write, pci_testdev_read},
> +    {NULL, NULL, NULL},
> +};
> +
> +#define FOREACH_TEST_CASE(n) for (n = &pci_testcases[0]; n->name; n++)
> +
> +static struct testcase *
> +pci_testdev_find_testcase(char *name)
> +{
> +    struct testcase *test;
> +
> +    FOREACH_TEST_CASE(test) {
> +        if (!strcmp(test->name, name)) {
> +            return test;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static uint64_t
> +pci_testdev_common_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCITestDevState *d = opaque;
> +    pci_testdev_read_op read_op = d->testcase->read_op;
> +    return read_op(opaque, addr, size);
> +}
> +
>  static void
>  pci_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>                         unsigned size)
>  {
> -    pci_testdev_write(opaque, addr, val, size, 0);
> +    PCITestDevState *d = opaque;
> +    pci_testdev_write_op write_op = d->testcase->write_op;
> +    write_op(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);
> +    PCITestDevState *d = opaque;
> +    pci_testdev_write_op write_op = d->testcase->write_op;
> +    write_op(opaque, addr, val, size, 1);
>  }
>  
>  static const MemoryRegionOps pci_testdev_mmio_ops = {
> -    .read = pci_testdev_read,
> +    .read = pci_testdev_common_read,
>      .write = pci_testdev_mmio_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .impl = {
> @@ -225,7 +276,7 @@ static const MemoryRegionOps pci_testdev_mmio_ops = {
>  };
>  
>  static const MemoryRegionOps pci_testdev_pio_ops = {
> -    .read = pci_testdev_read,
> +    .read = pci_testdev_common_read,
>      .write = pci_testdev_pio_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .impl = {
> @@ -281,6 +332,21 @@ static void pci_testdev_realize(PCIDevice *pci_dev, 
> Error **errp)
>          assert(r >= 0);
>          test->hasnotifier = true;
>      }
> +
> +    if (!d->testcase_name) {
> +        d->testcase_name = (char *)"eventfd";
> +    }
> +
> +    d->testcase = pci_testdev_find_testcase(d->testcase_name);
> +    if (!d->testcase) {
> +        struct testcase *test;
> +        error_report("Invalid test case. Currently support: {");
> +        FOREACH_TEST_CASE(test) {
> +            error_report("\t\"%s\", ", test->name);
> +        }
> +        error_report("}");
> +        exit(1);
> +    }
>  }
>  
>  static void
> @@ -305,6 +371,11 @@ static void qdev_pci_testdev_reset(DeviceState *dev)
>      pci_testdev_reset(d);
>  }
>  
> +static Property pci_testdev_properties[] = {
> +    DEFINE_PROP_STRING("testcase", PCITestDevState, testcase_name),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void pci_testdev_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -319,6 +390,7 @@ static void pci_testdev_class_init(ObjectClass *klass, 
> void *data)
>      dc->desc = "PCI Test Device";
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      dc->reset = qdev_pci_testdev_reset;
> +    dc->props = pci_testdev_properties;
>  }
>  
>  static const TypeInfo pci_testdev_info = {
> -- 
> 2.7.4
> 
>