[Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Lucas Meneghel Rodrigues]

Add a test device which supports the kvmctl ioports,
for running the KVM test suite. This is a straight
port from the latest version of the test device present
on qemu-kvm, using the APIs currently in use by qemu.

With this we aim for daily execution of
the KVM unittests to capture any problems with the
KVM interface.

Usage:

  qemu
     -chardev file,path=/log/file/some/where,id=testlog
     -device testdev,chardev=testlog

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
---
 Makefile.target |    1 +
 hw/testdev.c    |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+), 0 deletions(-)
 create mode 100644 hw/testdev.c

diff --git a/Makefile.target b/Makefile.target
index e280bf6..e095dd5 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -232,6 +232,7 @@ obj-i386-y += debugcon.o multiboot.o
 obj-i386-y += pc_piix.o
 obj-i386-$(CONFIG_KVM) += kvmclock.o
 obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
+obj-i386-y += testdev.o
 
 # shared objects
 obj-ppc-y = ppc.o
diff --git a/hw/testdev.c b/hw/testdev.c
new file mode 100644
index 0000000..e38c20e
--- /dev/null
+++ b/hw/testdev.c
@@ -0,0 +1,140 @@
+#include <sys/mman.h>
+#include "hw.h"
+#include "qdev.h"
+#include "isa.h"
+
+struct testdev {
+    ISADevice dev;
+    CharDriverState *chr;
+};
+
+static void test_device_serial_write(void *opaque, uint32_t addr, uint32_t data)
+{
+    struct testdev *dev = opaque;
+    uint8_t buf[1] = { data };
+
+    if (dev->chr) {
+        qemu_chr_fe_write(dev->chr, buf, 1);
+    }
+}
+
+static void test_device_exit(void *opaque, uint32_t addr, uint32_t data)
+{
+    exit(data);
+}
+
+static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
+{
+    return ram_size;
+}
+
+static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
+{
+    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
+}
+
+static uint32 test_device_ioport_data;
+
+static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
+{
+    test_device_ioport_data = data;
+}
+
+static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
+{
+    return test_device_ioport_data;
+}
+
+static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
+{
+    target_phys_addr_t len = 4096;
+    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
+
+    mprotect(a, 4096, PROT_NONE);
+    mprotect(a, 4096, PROT_READ|PROT_WRITE);
+    cpu_physical_memory_unmap(a, len, 0, 0);
+}
+
+static char *iomem_buf;
+
+static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
+{
+    return iomem_buf[addr];
+}
+
+static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
+{
+    return *(uint16_t*)(iomem_buf + addr);
+}
+
+static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
+{
+    return *(uint32_t*)(iomem_buf + addr);
+}
+
+static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    iomem_buf[addr] = val;
+}
+
+static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    *(uint16_t*)(iomem_buf + addr) = val;
+}
+
+static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    *(uint32_t*)(iomem_buf + addr) = val;
+}
+
+static CPUReadMemoryFunc * const test_iomem_read[3] = {
+    test_iomem_readb,
+    test_iomem_readw,
+    test_iomem_readl,
+};
+
+static CPUWriteMemoryFunc * const test_iomem_write[3] = {
+    test_iomem_writeb,
+    test_iomem_writew,
+    test_iomem_writel,
+};
+
+static int init_test_device(ISADevice *isa)
+{
+    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
+    int iomem;
+
+    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
+    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
+    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
+    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
+    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
+    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
+    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
+    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
+    iomem_buf = g_malloc0(0x10000);
+    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
+                                   DEVICE_NATIVE_ENDIAN);
+    cpu_register_physical_memory(0xff000000, 0x10000, iomem);
+    return 0;
+}
+
+static ISADeviceInfo testdev_info = {
+    .qdev.name  = "testdev",
+    .qdev.size  = sizeof(struct testdev),
+    .init       = init_test_device,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_CHR("chardev", struct testdev, chr),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void testdev_register_devices(void)
+{
+    isa_qdev_register(&testdev_info);
+}
+
+device_init(testdev_register_devices)
-- 
1.7.6

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Anthony Liguori]

On 08/26/2011 03:04 PM, Lucas Meneghel Rodrigues wrote:
> Add a test device which supports the kvmctl ioports,
> for running the KVM test suite. This is a straight
> port from the latest version of the test device present
> on qemu-kvm, using the APIs currently in use by qemu.
>
> With this we aim for daily execution of
> the KVM unittests to capture any problems with the
> KVM interface.

I know this has come up before so apologies if this is redundant.

>
> Usage:
>
>    qemu
>       -chardev file,path=/log/file/some/where,id=testlog
>       -device testdev,chardev=testlog
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> Signed-off-by: Avi Kivity<avi@redhat.com>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> Signed-off-by: Lucas Meneghel Rodrigues<lmr@redhat.com>
> ---
>   Makefile.target |    1 +
>   hw/testdev.c    |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 141 insertions(+), 0 deletions(-)
>   create mode 100644 hw/testdev.c
>
> diff --git a/Makefile.target b/Makefile.target
> index e280bf6..e095dd5 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -232,6 +232,7 @@ obj-i386-y += debugcon.o multiboot.o
>   obj-i386-y += pc_piix.o
>   obj-i386-$(CONFIG_KVM) += kvmclock.o
>   obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> +obj-i386-y += testdev.o
>
>   # shared objects
>   obj-ppc-y = ppc.o
> diff --git a/hw/testdev.c b/hw/testdev.c
> new file mode 100644
> index 0000000..e38c20e
> --- /dev/null
> +++ b/hw/testdev.c
> @@ -0,0 +1,140 @@
> +#include<sys/mman.h>
> +#include "hw.h"
> +#include "qdev.h"
> +#include "isa.h"
> +
> +struct testdev {
> +    ISADevice dev;
> +    CharDriverState *chr;
> +};
> +
> +static void test_device_serial_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    struct testdev *dev = opaque;
> +    uint8_t buf[1] = { data };
> +
> +    if (dev->chr) {
> +        qemu_chr_fe_write(dev->chr, buf, 1);
> +    }
> +}

I think I posted patches at some point for kvm unittests to use a 
standard UART.  Was there any reason not to do use a UART?

> +static void test_device_exit(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    exit(data);
> +}

Port 501 can do this.

> +
> +static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
> +{
> +    return ram_size;
> +}

This can be read through fw_cfg, any reason to do PIO for this?

> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
> +}
> +
> +static uint32 test_device_ioport_data;
> +
> +static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    test_device_ioport_data = data;
> +}
> +
> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
> +{
> +    return test_device_ioport_data;
> +}

Would be nicer to do this via an opaque.

> +static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    target_phys_addr_t len = 4096;
> +    void *a = cpu_physical_memory_map(data&  ~0xffful,&len, 0);
> +
> +    mprotect(a, 4096, PROT_NONE);
> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
> +    cpu_physical_memory_unmap(a, len, 0, 0);

This hard codes page size (get it via sysconf).  I think mprotect 
probably isn't available on windows either.

> +}
> +
> +static char *iomem_buf;
> +
> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return iomem_buf[addr];
> +}
> +
> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint16_t*)(iomem_buf + addr);
> +}
> +
> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint32_t*)(iomem_buf + addr);
> +}
> +
> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    iomem_buf[addr] = val;
> +}
> +
> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint16_t*)(iomem_buf + addr) = val;
> +}
> +
> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint32_t*)(iomem_buf + addr) = val;
> +}
> +
> +static CPUReadMemoryFunc * const test_iomem_read[3] = {
> +    test_iomem_readb,
> +    test_iomem_readw,
> +    test_iomem_readl,
> +};
> +
> +static CPUWriteMemoryFunc * const test_iomem_write[3] = {
> +    test_iomem_writeb,
> +    test_iomem_writew,
> +    test_iomem_writel,
> +};
> +
> +static int init_test_device(ISADevice *isa)
> +{
> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> +    int iomem;
> +
> +    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
> +    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
> +    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
> +    iomem_buf = g_malloc0(0x10000);
> +    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
> +                                   DEVICE_NATIVE_ENDIAN);
> +    cpu_register_physical_memory(0xff000000, 0x10000, iomem);
> +    return 0;
> +}
> +
> +static ISADeviceInfo testdev_info = {
> +    .qdev.name  = "testdev",
> +    .qdev.size  = sizeof(struct testdev),
> +    .init       = init_test_device,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_CHR("chardev", struct testdev, chr),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};

Should this use MemoryRegion?

Regards,

Anthony Liguori

> +
> +static void testdev_register_devices(void)
> +{
> +    isa_qdev_register(&testdev_info);
> +}
> +
> +device_init(testdev_register_devices)

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6468 bytes --]

On 2011-08-26 22:04, Lucas Meneghel Rodrigues wrote:
> Add a test device which supports the kvmctl ioports,
> for running the KVM test suite. This is a straight
> port from the latest version of the test device present
> on qemu-kvm, using the APIs currently in use by qemu.
> 
> With this we aim for daily execution of
> the KVM unittests to capture any problems with the
> KVM interface.

In principle this works with TCG as well.

> 
> Usage:
> 
>   qemu
>      -chardev file,path=/log/file/some/where,id=testlog
>      -device testdev,chardev=testlog
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
> ---
>  Makefile.target |    1 +
>  hw/testdev.c    |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 141 insertions(+), 0 deletions(-)
>  create mode 100644 hw/testdev.c
> 
> diff --git a/Makefile.target b/Makefile.target
> index e280bf6..e095dd5 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -232,6 +232,7 @@ obj-i386-y += debugcon.o multiboot.o
>  obj-i386-y += pc_piix.o
>  obj-i386-$(CONFIG_KVM) += kvmclock.o
>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> +obj-i386-y += testdev.o

The name testdev is a bit too generic for this thing in its current
form. I would suggest x86-testdev or pc-testdev - or a generalization
that allows a PIO-free registration against the system bus.

>  
>  # shared objects
>  obj-ppc-y = ppc.o
> diff --git a/hw/testdev.c b/hw/testdev.c
> new file mode 100644
> index 0000000..e38c20e
> --- /dev/null
> +++ b/hw/testdev.c
> @@ -0,0 +1,140 @@
> +#include <sys/mman.h>
> +#include "hw.h"
> +#include "qdev.h"
> +#include "isa.h"
> +
> +struct testdev {
> +    ISADevice dev;
> +    CharDriverState *chr;
> +};
> +
> +static void test_device_serial_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    struct testdev *dev = opaque;
> +    uint8_t buf[1] = { data };
> +
> +    if (dev->chr) {
> +        qemu_chr_fe_write(dev->chr, buf, 1);
> +    }
> +}
> +
> +static void test_device_exit(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    exit(data);
> +}
> +
> +static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
> +{
> +    return ram_size;
> +}
> +
> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);

Note that qemu-kvm retrieves (due to a hack) GSIs via isa_get_irq while
QEMU is and will remain confined to true ISA IRQs, thus provides no
access to IRQs 16-23. May break existing tests. So we likely have to
introduce a cleaner interface now if GSI is what you need here.

> +}
> +
> +static uint32 test_device_ioport_data;
> +
> +static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    test_device_ioport_data = data;
> +}
> +
> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
> +{
> +    return test_device_ioport_data;
> +}
> +
> +static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    target_phys_addr_t len = 4096;
> +    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
> +
> +    mprotect(a, 4096, PROT_NONE);
> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
> +    cpu_physical_memory_unmap(a, len, 0, 0);
> +}
> +
> +static char *iomem_buf;
> +
> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return iomem_buf[addr];
> +}
> +
> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint16_t*)(iomem_buf + addr);
> +}
> +
> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint32_t*)(iomem_buf + addr);
> +}
> +
> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    iomem_buf[addr] = val;
> +}
> +
> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint16_t*)(iomem_buf + addr) = val;
> +}
> +
> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint32_t*)(iomem_buf + addr) = val;
> +}
> +
> +static CPUReadMemoryFunc * const test_iomem_read[3] = {
> +    test_iomem_readb,
> +    test_iomem_readw,
> +    test_iomem_readl,
> +};
> +
> +static CPUWriteMemoryFunc * const test_iomem_write[3] = {
> +    test_iomem_writeb,
> +    test_iomem_writew,
> +    test_iomem_writel,
> +};
> +
> +static int init_test_device(ISADevice *isa)
> +{
> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> +    int iomem;
> +
> +    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
> +    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
> +    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
> +    iomem_buf = g_malloc0(0x10000);
> +    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
> +                                   DEVICE_NATIVE_ENDIAN);
> +    cpu_register_physical_memory(0xff000000, 0x10000, iomem);

We have MemoryRegion for MMIO and PIO now.

> +    return 0;
> +}
> +
> +static ISADeviceInfo testdev_info = {
> +    .qdev.name  = "testdev",
> +    .qdev.size  = sizeof(struct testdev),
> +    .init       = init_test_device,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_CHR("chardev", struct testdev, chr),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void testdev_register_devices(void)
> +{
> +    isa_qdev_register(&testdev_info);
> +}
> +
> +device_init(testdev_register_devices)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Edgar E. Iglesias]

On Fri, Aug 26, 2011 at 04:22:22PM -0500, Anthony Liguori wrote:
> On 08/26/2011 03:04 PM, Lucas Meneghel Rodrigues wrote:
> >Add a test device which supports the kvmctl ioports,
> >for running the KVM test suite. This is a straight
> >port from the latest version of the test device present
> >on qemu-kvm, using the APIs currently in use by qemu.
> >
> >With this we aim for daily execution of
> >the KVM unittests to capture any problems with the
> >KVM interface.
> 
> I know this has come up before so apologies if this is redundant.
> 
> >
> >Usage:
> >
> >   qemu
> >      -chardev file,path=/log/file/some/where,id=testlog
> >      -device testdev,chardev=testlog
> >
> >Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> >Signed-off-by: Avi Kivity<avi@redhat.com>
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >Signed-off-by: Lucas Meneghel Rodrigues<lmr@redhat.com>
> >---
> >  Makefile.target |    1 +
> >  hw/testdev.c    |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 141 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/testdev.c
> >
> >diff --git a/Makefile.target b/Makefile.target
> >index e280bf6..e095dd5 100644
> >--- a/Makefile.target
> >+++ b/Makefile.target
> >@@ -232,6 +232,7 @@ obj-i386-y += debugcon.o multiboot.o
> >  obj-i386-y += pc_piix.o
> >  obj-i386-$(CONFIG_KVM) += kvmclock.o
> >  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >+obj-i386-y += testdev.o
> >
> >  # shared objects
> >  obj-ppc-y = ppc.o
> >diff --git a/hw/testdev.c b/hw/testdev.c
> >new file mode 100644
> >index 0000000..e38c20e
> >--- /dev/null
> >+++ b/hw/testdev.c
> >@@ -0,0 +1,140 @@
> >+#include<sys/mman.h>
> >+#include "hw.h"
> >+#include "qdev.h"
> >+#include "isa.h"
> >+
> >+struct testdev {
> >+    ISADevice dev;
> >+    CharDriverState *chr;
> >+};
> >+
> >+static void test_device_serial_write(void *opaque, uint32_t addr, uint32_t data)
> >+{
> >+    struct testdev *dev = opaque;
> >+    uint8_t buf[1] = { data };
> >+
> >+    if (dev->chr) {
> >+        qemu_chr_fe_write(dev->chr, buf, 1);
> >+    }
> >+}
> 
> I think I posted patches at some point for kvm unittests to use a
> standard UART.  Was there any reason not to do use a UART?
> 
> >+static void test_device_exit(void *opaque, uint32_t addr, uint32_t data)
> >+{
> >+    exit(data);
> >+}
> 
> Port 501 can do this.
> 
> >+
> >+static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
> >+{
> >+    return ram_size;
> >+}
> 
> This can be read through fw_cfg, any reason to do PIO for this?
> 
> >+static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> >+{
> >+    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
> >+}
> >+
> >+static uint32 test_device_ioport_data;
> >+
> >+static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> >+{
> >+    test_device_ioport_data = data;
> >+}
> >+
> >+static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
> >+{
> >+    return test_device_ioport_data;
> >+}
> 
> Would be nicer to do this via an opaque.
> 
> >+static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
> >+{
> >+    target_phys_addr_t len = 4096;
> >+    void *a = cpu_physical_memory_map(data&  ~0xffful,&len, 0);
> >+
> >+    mprotect(a, 4096, PROT_NONE);
> >+    mprotect(a, 4096, PROT_READ|PROT_WRITE);
> >+    cpu_physical_memory_unmap(a, len, 0, 0);
> 
> This hard codes page size (get it via sysconf).  I think mprotect
> probably isn't available on windows either.
> 
> >+}
> >+
> >+static char *iomem_buf;
> >+
> >+static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> >+{
> >+    return iomem_buf[addr];
> >+}
> >+
> >+static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> >+{
> >+    return *(uint16_t*)(iomem_buf + addr);
> >+}
> >+
> >+static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> >+{
> >+    return *(uint32_t*)(iomem_buf + addr);
> >+}
> >+
> >+static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> >+{
> >+    iomem_buf[addr] = val;
> >+}
> >+
> >+static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> >+{
> >+    *(uint16_t*)(iomem_buf + addr) = val;
> >+}
> >+
> >+static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> >+{
> >+    *(uint32_t*)(iomem_buf + addr) = val;
> >+}
> >+
> >+static CPUReadMemoryFunc * const test_iomem_read[3] = {
> >+    test_iomem_readb,
> >+    test_iomem_readw,
> >+    test_iomem_readl,
> >+};
> >+
> >+static CPUWriteMemoryFunc * const test_iomem_write[3] = {
> >+    test_iomem_writeb,
> >+    test_iomem_writew,
> >+    test_iomem_writel,
> >+};
> >+
> >+static int init_test_device(ISADevice *isa)
> >+{
> >+    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> >+    int iomem;
> >+
> >+    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
> >+    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
> >+    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
> >+    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> >+    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> >+    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> >+    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> >+    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> >+    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> >+    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> >+    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
> >+    iomem_buf = g_malloc0(0x10000);
> >+    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
> >+                                   DEVICE_NATIVE_ENDIAN);
> >+    cpu_register_physical_memory(0xff000000, 0x10000, iomem);
> >+    return 0;
> >+}
> >+
> >+static ISADeviceInfo testdev_info = {
> >+    .qdev.name  = "testdev",
> >+    .qdev.size  = sizeof(struct testdev),
> >+    .init       = init_test_device,
> >+    .qdev.props = (Property[]) {
> >+        DEFINE_PROP_CHR("chardev", struct testdev, chr),
> >+        DEFINE_PROP_END_OF_LIST(),
> >+    },
> >+};
> 
> Should this use MemoryRegion?

Yes. And what is the reason for using IO ports?
There are archs that dont have ioport connections out from the CPU.

If we are adding virtual devices for tests, they should preferably work for
all archs.

Cheers

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Blue Swirl]

On Fri, Aug 26, 2011 at 8:04 PM, Lucas Meneghel Rodrigues
<lmr@redhat.com> wrote:
> Add a test device which supports the kvmctl ioports,
> for running the KVM test suite. This is a straight
> port from the latest version of the test device present
> on qemu-kvm, using the APIs currently in use by qemu.

Or rather before recent conversions.

>
> With this we aim for daily execution of
> the KVM unittests to capture any problems with the
> KVM interface.
>
> Usage:
>
>  qemu
>     -chardev file,path=/log/file/some/where,id=testlog
>     -device testdev,chardev=testlog
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
> ---
>  Makefile.target |    1 +
>  hw/testdev.c    |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 141 insertions(+), 0 deletions(-)
>  create mode 100644 hw/testdev.c
>
> diff --git a/Makefile.target b/Makefile.target
> index e280bf6..e095dd5 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -232,6 +232,7 @@ obj-i386-y += debugcon.o multiboot.o
>  obj-i386-y += pc_piix.o
>  obj-i386-$(CONFIG_KVM) += kvmclock.o
>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> +obj-i386-y += testdev.o
>
>  # shared objects
>  obj-ppc-y = ppc.o
> diff --git a/hw/testdev.c b/hw/testdev.c
> new file mode 100644
> index 0000000..e38c20e
> --- /dev/null
> +++ b/hw/testdev.c
> @@ -0,0 +1,140 @@
> +#include <sys/mman.h>
> +#include "hw.h"
> +#include "qdev.h"
> +#include "isa.h"
> +
> +struct testdev {
> +    ISADevice dev;
> +    CharDriverState *chr;
> +};

Please read CODING_STYLE: typedef and struct TestDev(ice).

> +
> +static void test_device_serial_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    struct testdev *dev = opaque;
> +    uint8_t buf[1] = { data };
> +
> +    if (dev->chr) {
> +        qemu_chr_fe_write(dev->chr, buf, 1);
> +    }
> +}
> +
> +static void test_device_exit(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    exit(data);
> +}
> +
> +static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
> +{
> +    return ram_size;
> +}
> +
> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);

Where does 0x2000 come from?

> +}
> +
> +static uint32 test_device_ioport_data;
> +
> +static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    test_device_ioport_data = data;
> +}
> +
> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
> +{
> +    return test_device_ioport_data;
> +}
> +
> +static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    target_phys_addr_t len = 4096;
> +    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
> +
> +    mprotect(a, 4096, PROT_NONE);
> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
> +    cpu_physical_memory_unmap(a, len, 0, 0);
> +}
> +
> +static char *iomem_buf;

Please move this to state.

> +
> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return iomem_buf[addr];
> +}
> +
> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint16_t*)(iomem_buf + addr);
> +}

This and the other functions assume that the memory is available and
the guest and the host are of same endianness.

This looks like pure RAM, so MMIO is not the best way to do this.
Please just map more RAM.

What's the use of this anyway, it doesn't affect QEMU in any way? Scratch space?

> +
> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint32_t*)(iomem_buf + addr);
> +}
> +
> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    iomem_buf[addr] = val;
> +}
> +
> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint16_t*)(iomem_buf + addr) = val;
> +}
> +
> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint32_t*)(iomem_buf + addr) = val;
> +}
> +
> +static CPUReadMemoryFunc * const test_iomem_read[3] = {
> +    test_iomem_readb,
> +    test_iomem_readw,
> +    test_iomem_readl,
> +};
> +
> +static CPUWriteMemoryFunc * const test_iomem_write[3] = {
> +    test_iomem_writeb,
> +    test_iomem_writew,
> +    test_iomem_writel,
> +};
> +
> +static int init_test_device(ISADevice *isa)
> +{
> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> +    int iomem;
> +
> +    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
> +    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
> +    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);

24? Doesn't ISA have only 16? Enums for all constants would be more readable.

> +    iomem_buf = g_malloc0(0x10000);
> +    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
> +                                   DEVICE_NATIVE_ENDIAN);
> +    cpu_register_physical_memory(0xff000000, 0x10000, iomem);

Devices may not map themselves, this should be done at board level.
Doesn't this address also conflict with PCI for PC?

> +    return 0;
> +}
> +
> +static ISADeviceInfo testdev_info = {
> +    .qdev.name  = "testdev",
> +    .qdev.size  = sizeof(struct testdev),
> +    .init       = init_test_device,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_CHR("chardev", struct testdev, chr),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void testdev_register_devices(void)
> +{
> +    isa_qdev_register(&testdev_info);
> +}
> +
> +device_init(testdev_register_devices)
> --
> 1.7.6
>
>
>

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Blue Swirl]

On Sat, Aug 27, 2011 at 10:07 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Fri, Aug 26, 2011 at 04:22:22PM -0500, Anthony Liguori wrote:
>> On 08/26/2011 03:04 PM, Lucas Meneghel Rodrigues wrote:
>> >Add a test device which supports the kvmctl ioports,
>> >for running the KVM test suite. This is a straight
>> >port from the latest version of the test device present
>> >on qemu-kvm, using the APIs currently in use by qemu.
>> >
>> >With this we aim for daily execution of
>> >the KVM unittests to capture any problems with the
>> >KVM interface.
>>
>> I know this has come up before so apologies if this is redundant.
>>
>> >
>> >Usage:
>> >
>> >   qemu
>> >      -chardev file,path=/log/file/some/where,id=testlog
>> >      -device testdev,chardev=testlog
>> >
>> >Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>> >Signed-off-by: Avi Kivity<avi@redhat.com>
>> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>> >Signed-off-by: Lucas Meneghel Rodrigues<lmr@redhat.com>
>> >---
>> >  Makefile.target |    1 +
>> >  hw/testdev.c    |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 141 insertions(+), 0 deletions(-)
>> >  create mode 100644 hw/testdev.c
>> >
>> >diff --git a/Makefile.target b/Makefile.target
>> >index e280bf6..e095dd5 100644
>> >--- a/Makefile.target
>> >+++ b/Makefile.target
>> >@@ -232,6 +232,7 @@ obj-i386-y += debugcon.o multiboot.o
>> >  obj-i386-y += pc_piix.o
>> >  obj-i386-$(CONFIG_KVM) += kvmclock.o
>> >  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>> >+obj-i386-y += testdev.o
>> >
>> >  # shared objects
>> >  obj-ppc-y = ppc.o
>> >diff --git a/hw/testdev.c b/hw/testdev.c
>> >new file mode 100644
>> >index 0000000..e38c20e
>> >--- /dev/null
>> >+++ b/hw/testdev.c
>> >@@ -0,0 +1,140 @@
>> >+#include<sys/mman.h>
>> >+#include "hw.h"
>> >+#include "qdev.h"
>> >+#include "isa.h"
>> >+
>> >+struct testdev {
>> >+    ISADevice dev;
>> >+    CharDriverState *chr;
>> >+};
>> >+
>> >+static void test_device_serial_write(void *opaque, uint32_t addr, uint32_t data)
>> >+{
>> >+    struct testdev *dev = opaque;
>> >+    uint8_t buf[1] = { data };
>> >+
>> >+    if (dev->chr) {
>> >+        qemu_chr_fe_write(dev->chr, buf, 1);
>> >+    }
>> >+}
>>
>> I think I posted patches at some point for kvm unittests to use a
>> standard UART.  Was there any reason not to do use a UART?
>>
>> >+static void test_device_exit(void *opaque, uint32_t addr, uint32_t data)
>> >+{
>> >+    exit(data);
>> >+}
>>
>> Port 501 can do this.
>>
>> >+
>> >+static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
>> >+{
>> >+    return ram_size;
>> >+}
>>
>> This can be read through fw_cfg, any reason to do PIO for this?
>>
>> >+static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
>> >+{
>> >+    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
>> >+}
>> >+
>> >+static uint32 test_device_ioport_data;
>> >+
>> >+static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>> >+{
>> >+    test_device_ioport_data = data;
>> >+}
>> >+
>> >+static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
>> >+{
>> >+    return test_device_ioport_data;
>> >+}
>>
>> Would be nicer to do this via an opaque.
>>
>> >+static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
>> >+{
>> >+    target_phys_addr_t len = 4096;
>> >+    void *a = cpu_physical_memory_map(data&  ~0xffful,&len, 0);
>> >+
>> >+    mprotect(a, 4096, PROT_NONE);
>> >+    mprotect(a, 4096, PROT_READ|PROT_WRITE);
>> >+    cpu_physical_memory_unmap(a, len, 0, 0);
>>
>> This hard codes page size (get it via sysconf).  I think mprotect
>> probably isn't available on windows either.
>>
>> >+}
>> >+
>> >+static char *iomem_buf;
>> >+
>> >+static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
>> >+{
>> >+    return iomem_buf[addr];
>> >+}
>> >+
>> >+static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
>> >+{
>> >+    return *(uint16_t*)(iomem_buf + addr);
>> >+}
>> >+
>> >+static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
>> >+{
>> >+    return *(uint32_t*)(iomem_buf + addr);
>> >+}
>> >+
>> >+static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
>> >+{
>> >+    iomem_buf[addr] = val;
>> >+}
>> >+
>> >+static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
>> >+{
>> >+    *(uint16_t*)(iomem_buf + addr) = val;
>> >+}
>> >+
>> >+static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>> >+{
>> >+    *(uint32_t*)(iomem_buf + addr) = val;
>> >+}
>> >+
>> >+static CPUReadMemoryFunc * const test_iomem_read[3] = {
>> >+    test_iomem_readb,
>> >+    test_iomem_readw,
>> >+    test_iomem_readl,
>> >+};
>> >+
>> >+static CPUWriteMemoryFunc * const test_iomem_write[3] = {
>> >+    test_iomem_writeb,
>> >+    test_iomem_writew,
>> >+    test_iomem_writel,
>> >+};
>> >+
>> >+static int init_test_device(ISADevice *isa)
>> >+{
>> >+    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
>> >+    int iomem;
>> >+
>> >+    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
>> >+    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
>> >+    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
>> >+    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
>> >+    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
>> >+    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
>> >+    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
>> >+    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
>> >+    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
>> >+    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
>> >+    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
>> >+    iomem_buf = g_malloc0(0x10000);
>> >+    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
>> >+                                   DEVICE_NATIVE_ENDIAN);
>> >+    cpu_register_physical_memory(0xff000000, 0x10000, iomem);
>> >+    return 0;
>> >+}
>> >+
>> >+static ISADeviceInfo testdev_info = {
>> >+    .qdev.name  = "testdev",
>> >+    .qdev.size  = sizeof(struct testdev),
>> >+    .init       = init_test_device,
>> >+    .qdev.props = (Property[]) {
>> >+        DEFINE_PROP_CHR("chardev", struct testdev, chr),
>> >+        DEFINE_PROP_END_OF_LIST(),
>> >+    },
>> >+};
>>
>> Should this use MemoryRegion?
>
> Yes. And what is the reason for using IO ports?
> There are archs that dont have ioport connections out from the CPU.
>
> If we are adding virtual devices for tests, they should preferably work for
> all archs.

Fully agree.

In a way fw_cfg would be more suitable, but a separate device that
combines all debug/test facilities is better since it can be removed
from production builds.

I'd also remove port 501 since this makes it redundant, before 501
becomes too entrenched.

But I fear this kind of device may become over time a huge monster
with all kinds of bells and whistles. There's no hardware
specification that would limit the growth of these kind of hacks.
Next: poke port 0xf2 for buzzer sound to wake up the test person, port
0xf3 for the vibrator and finally port 0xf4 gives electrical shocks.
In 2012, a comprehensive test report with charts and graphs with error
bars in PDF format can be downloaded via port 0x3001, dial
0x3000+intl. country code for localized Mandarin Chinese version.
Would that matter though?

At least, instead of using so many ports, please use only one or two
and introduce some kind of multiplexing protocol like fw_cfg does.

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Avi Kivity]

On 08/27/2011 12:22 AM, Anthony Liguori wrote:
>
> I think I posted patches at some point for kvm unittests to use a 
> standard UART.  Was there any reason not to do use a UART?
>

No.

>> +static void test_device_exit(void *opaque, uint32_t addr, uint32_t 
>> data)
>> +{
>> +    exit(data);
>> +}
>
> Port 501 can do this.

Right.

>
>> +
>> +static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
>> +{
>> +    return ram_size;
>> +}
>
> This can be read through fw_cfg, any reason to do PIO for this?

Legacy.  We even have a fw_cfg driver in k-u-t.git.

>> +static void test_device_flush_page(void *opaque, uint32_t addr, 
>> uint32_t data)
>> +{
>> +    target_phys_addr_t len = 4096;
>> +    void *a = cpu_physical_memory_map(data&  ~0xffful,&len, 0);
>> +
>> +    mprotect(a, 4096, PROT_NONE);
>> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
>> +    cpu_physical_memory_unmap(a, len, 0, 0);
>
> This hard codes page size (get it via sysconf). 

(or getpagesize())

> I think mprotect probably isn't available on windows either.

Google thinks it is.

> Should this use MemoryRegion?
>

Yes.


Lucas, do you want to tackle these yourself?  I'll help with any 
questions.  Otherwise I'll do it.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Avi Kivity]

On 08/27/2011 01:07 PM, Edgar E. Iglesias wrote:
> >  >+
> >  >+static ISADeviceInfo testdev_info = {
> >  >+    .qdev.name  = "testdev",
> >  >+    .qdev.size  = sizeof(struct testdev),
> >  >+    .init       = init_test_device,
> >  >+    .qdev.props = (Property[]) {
> >  >+        DEFINE_PROP_CHR("chardev", struct testdev, chr),
> >  >+        DEFINE_PROP_END_OF_LIST(),
> >  >+    },
> >  >+};
> >
> >  Should this use MemoryRegion?
>
> Yes. And what is the reason for using IO ports?

Mostly for ease of use.  The tests were originally run under a separate 
kvm userspace that didn't emulate a full machine.

> There are archs that dont have ioport connections out from the CPU.
>
> If we are adding virtual devices for tests, they should preferably work for
> all archs.
>

I think all the functionallity here (apart from that which Anthony 
pointed out is available by other means) is x86 specific.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Avi Kivity]

On 08/27/2011 01:26 AM, Jan Kiszka wrote:
> >  +
> >  +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> >  +{
> >  +    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
>
> Note that qemu-kvm retrieves (due to a hack) GSIs via isa_get_irq while
> QEMU is and will remain confined to true ISA IRQs, thus provides no
> access to IRQs 16-23. May break existing tests. So we likely have to
> introduce a cleaner interface now if GSI is what you need here.

IIRC, gsi is wanted so we can hit on unallocated gsis.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Avi Kivity]

On 08/27/2011 07:22 PM, Blue Swirl wrote:
> >  +
> >  +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> >  +{
> >  +    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
>
> Where does 0x2000 come from?

The base address of this range.

> >  +
> >  +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> >  +{
> >  +    return *(uint16_t*)(iomem_buf + addr);
> >  +}
>
> This and the other functions assume that the memory is available and
> the guest and the host are of same endianness.
>
> This looks like pure RAM, so MMIO is not the best way to do this.
> Please just map more RAM.

The intent is to get the guest to fault and exercise kvm's instruction 
emulator.  That won't happen with RAM.

(well, with a sub-page mapping, it should)


> What's the use of this anyway, it doesn't affect QEMU in any way? Scratch space?
>
>
> >  +static int init_test_device(ISADevice *isa)
> >  +{
> >  +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> >  +    int iomem;
> >  +
> >  +    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
> >  +    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
> >  +    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
> >  +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> >  +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> >  +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> >  +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> >  +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> >  +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> >  +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> >  +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
>
> 24? Doesn't ISA have only 16? Enums for all constants would be more readable.

GSI space - the ioapic pins.  It's really a motherboard device.

>
> >  +    iomem_buf = g_malloc0(0x10000);
> >  +    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
> >  +                                   DEVICE_NATIVE_ENDIAN);
> >  +    cpu_register_physical_memory(0xff000000, 0x10000, iomem);
>
> Devices may not map themselves, this should be done at board level.
> Doesn't this address also conflict with PCI for PC?

Probably.  The whole thing is a hack!

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Lucas Meneghel Rodrigues]

On 08/26/2011 06:22 PM, Anthony Liguori wrote:
> On 08/26/2011 03:04 PM, Lucas Meneghel Rodrigues wrote:
>> Add a test device which supports the kvmctl ioports,
>> for running the KVM test suite. This is a straight
>> port from the latest version of the test device present
>> on qemu-kvm, using the APIs currently in use by qemu.
>>
>> With this we aim for daily execution of
>> the KVM unittests to capture any problems with the
>> KVM interface.
>
> I know this has come up before so apologies if this is redundant.
>
>>
>> Usage:
>>
>> qemu
>> -chardev file,path=/log/file/some/where,id=testlog
>> -device testdev,chardev=testlog
>>
>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>> Signed-off-by: Avi Kivity<avi@redhat.com>
>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>> Signed-off-by: Lucas Meneghel Rodrigues<lmr@redhat.com>
>> ---
>> Makefile.target | 1 +
>> hw/testdev.c | 140
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 141 insertions(+), 0 deletions(-)
>> create mode 100644 hw/testdev.c
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index e280bf6..e095dd5 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -232,6 +232,7 @@ obj-i386-y += debugcon.o multiboot.o
>> obj-i386-y += pc_piix.o
>> obj-i386-$(CONFIG_KVM) += kvmclock.o
>> obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>> +obj-i386-y += testdev.o
>>
>> # shared objects
>> obj-ppc-y = ppc.o
>> diff --git a/hw/testdev.c b/hw/testdev.c
>> new file mode 100644
>> index 0000000..e38c20e
>> --- /dev/null
>> +++ b/hw/testdev.c
>> @@ -0,0 +1,140 @@
>> +#include<sys/mman.h>
>> +#include "hw.h"
>> +#include "qdev.h"
>> +#include "isa.h"
>> +
>> +struct testdev {
>> + ISADevice dev;
>> + CharDriverState *chr;
>> +};
>> +
>> +static void test_device_serial_write(void *opaque, uint32_t addr,
>> uint32_t data)
>> +{
>> + struct testdev *dev = opaque;
>> + uint8_t buf[1] = { data };
>> +
>> + if (dev->chr) {
>> + qemu_chr_fe_write(dev->chr, buf, 1);
>> + }
>> +}
>
> I think I posted patches at some point for kvm unittests to use a
> standard UART. Was there any reason not to do use a UART?

I will look for your patches and adapt them to the current conditions.

>> +static void test_device_exit(void *opaque, uint32_t addr, uint32_t data)
>> +{
>> + exit(data);
>> +}
>
> Port 501 can do this.
>
>> +
>> +static uint32_t test_device_memsize_read(void *opaque, uint32_t addr)
>> +{
>> + return ram_size;
>> +}
>
> This can be read through fw_cfg, any reason to do PIO for this?

He already explained this, but this was an adaptation of an independent 
userspace program to run the unittests. I will convert to use fw_cfg then.

>> +static void test_device_irq_line(void *opaque, uint32_t addr,
>> uint32_t data)
>> +{
>> + qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
>> +}
>> +
>> +static uint32 test_device_ioport_data;
>> +
>> +static void test_device_ioport_write(void *opaque, uint32_t addr,
>> uint32_t data)
>> +{
>> + test_device_ioport_data = data;
>> +}
>> +
>> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
>> +{
>> + return test_device_ioport_data;
>> +}
>
> Would be nicer to do this via an opaque.

Ok.

>> +static void test_device_flush_page(void *opaque, uint32_t addr,
>> uint32_t data)
>> +{
>> + target_phys_addr_t len = 4096;
>> + void *a = cpu_physical_memory_map(data& ~0xffful,&len, 0);
>> +
>> + mprotect(a, 4096, PROT_NONE);
>> + mprotect(a, 4096, PROT_READ|PROT_WRITE);
>> + cpu_physical_memory_unmap(a, len, 0, 0);
>
> This hard codes page size (get it via sysconf). I think mprotect
> probably isn't available on windows either.

Sure, good point, will look into doing it with getpagesize

>> +}
>> +
>> +static char *iomem_buf;
>> +
>> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
>> +{
>> + return iomem_buf[addr];
>> +}
>> +
>> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
>> +{
>> + return *(uint16_t*)(iomem_buf + addr);
>> +}
>> +
>> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
>> +{
>> + return *(uint32_t*)(iomem_buf + addr);
>> +}
>> +
>> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> +{
>> + iomem_buf[addr] = val;
>> +}
>> +
>> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> +{
>> + *(uint16_t*)(iomem_buf + addr) = val;
>> +}
>> +
>> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> +{
>> + *(uint32_t*)(iomem_buf + addr) = val;
>> +}
>> +
>> +static CPUReadMemoryFunc * const test_iomem_read[3] = {
>> + test_iomem_readb,
>> + test_iomem_readw,
>> + test_iomem_readl,
>> +};
>> +
>> +static CPUWriteMemoryFunc * const test_iomem_write[3] = {
>> + test_iomem_writeb,
>> + test_iomem_writew,
>> + test_iomem_writel,
>> +};
>> +
>> +static int init_test_device(ISADevice *isa)
>> +{
>> + struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
>> + int iomem;
>> +
>> + register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
>> + register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
>> + register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
>> + register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
>> + register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
>> + register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
>> + register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
>> + register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
>> + register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
>> + register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
>> + register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
>> + iomem_buf = g_malloc0(0x10000);
>> + iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
>> + DEVICE_NATIVE_ENDIAN);
>> + cpu_register_physical_memory(0xff000000, 0x10000, iomem);
>> + return 0;
>> +}
>> +
>> +static ISADeviceInfo testdev_info = {
>> + .qdev.name = "testdev",
>> + .qdev.size = sizeof(struct testdev),
>> + .init = init_test_device,
>> + .qdev.props = (Property[]) {
>> + DEFINE_PROP_CHR("chardev", struct testdev, chr),
>> + DEFINE_PROP_END_OF_LIST(),
>> + },
>> +};
>
> Should this use MemoryRegion?

Yep, will look into converting it.

> Regards,

Thanks!

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Blue Swirl]

On Mon, Aug 29, 2011 at 5:57 AM, Avi Kivity <avi@redhat.com> wrote:
> On 08/27/2011 07:22 PM, Blue Swirl wrote:
>>
>> >  +
>> >  +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t
>> > data)
>> >  +{
>> >  +    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
>>
>> Where does 0x2000 come from?
>
> The base address of this range.
>
>> >  +
>> >  +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t
>> > addr)
>> >  +{
>> >  +    return *(uint16_t*)(iomem_buf + addr);
>> >  +}
>>
>> This and the other functions assume that the memory is available and
>> the guest and the host are of same endianness.
>>
>> This looks like pure RAM, so MMIO is not the best way to do this.
>> Please just map more RAM.
>
> The intent is to get the guest to fault and exercise kvm's instruction
> emulator.  That won't happen with RAM.
>
> (well, with a sub-page mapping, it should)
>
>
>> What's the use of this anyway, it doesn't affect QEMU in any way? Scratch
>> space?
>>
>>
>> >  +static int init_test_device(ISADevice *isa)
>> >  +{
>> >  +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
>> >  +    int iomem;
>> >  +
>> >  +    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
>> >  +    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
>> >  +    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
>> >  +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
>> >  +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
>> >  +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
>> >  +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
>> >  +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
>> >  +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
>> >  +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
>> >  +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
>>
>> 24? Doesn't ISA have only 16? Enums for all constants would be more
>> readable.
>
> GSI space - the ioapic pins.  It's really a motherboard device.
>
>>
>> >  +    iomem_buf = g_malloc0(0x10000);
>> >  +    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write,
>> > NULL,
>> >  +                                   DEVICE_NATIVE_ENDIAN);
>> >  +    cpu_register_physical_memory(0xff000000, 0x10000, iomem);
>>
>> Devices may not map themselves, this should be done at board level.
>> Doesn't this address also conflict with PCI for PC?
>
> Probably.  The whole thing is a hack!

The use case is valid, to help with testing and debugging. But this is
already a bit baroque and I foresee an endless stream of hacks in this
area.

Maybe the device should be as simple as possible but then connect to
some kind of external test program or plugin? Then that one could
include a beowulf cluster of PDF test report generators or even
Eclipse and we wouldn't care less here.

Even better, taking the instrumentation approach, could the test
device be left out completely, just use guest invisible methods (like
watchpoints) to interact with the guest?

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Lluís]

Blue Swirl writes:

> Even better, taking the instrumentation approach, could the test
> device be left out completely, just use guest invisible methods (like
> watchpoints) to interact with the guest?

I don't get it. Sorry but I've not been closely following the thread,
and a quick look at it gave me no clues about what you mean.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Blue Swirl]

On Tue, Aug 30, 2011 at 7:36 PM, Lluís <xscript@gmx.net> wrote:
> Blue Swirl writes:
>
>> Even better, taking the instrumentation approach, could the test
>> device be left out completely, just use guest invisible methods (like
>> watchpoints) to interact with the guest?
>
> I don't get it. Sorry but I've not been closely following the thread,
> and a quick look at it gave me no clues about what you mean.

The use case for the proposed test device is that guest and host can
interact during testing and debugging, preferably not for production.
But I think instrumentation needs are not unlike this case. I think I
proposed earlier to you that instead of intrusive code changes, guest
invisible methods, for example watchpoints and virtual hardware could
be used. Maybe we could be able to solve several problems at once? If
the test device would provide a generic way to connect to an external
program, wouldn't that be useful also for instrumentation?

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Lluís]

Blue Swirl writes:

> On Tue, Aug 30, 2011 at 7:36 PM, Lluís <xscript@gmx.net> wrote:
>> Blue Swirl writes:
>> 
>>> Even better, taking the instrumentation approach, could the test
>>> device be left out completely, just use guest invisible methods (like
>>> watchpoints) to interact with the guest?
>> 
>> I don't get it. Sorry but I've not been closely following the thread,
>> and a quick look at it gave me no clues about what you mean.

> The use case for the proposed test device is that guest and host can
> interact during testing and debugging, preferably not for production.
> But I think instrumentation needs are not unlike this case. I think I
> proposed earlier to you that instead of intrusive code changes, guest
> invisible methods, for example watchpoints and virtual hardware could
> be used. Maybe we could be able to solve several problems at once? If
> the test device would provide a generic way to connect to an external
> program, wouldn't that be useful also for instrumentation?

Ah, now I get it. Well, guest-host interaction is what I call the
backdoor channel, which from my point of view is orthogonal to
instrumentation. Of course, you could implement the former on top of the
latter.

But I still believe using watchpoints is too heavyweight for the kind of
instrumentation that I want (watchpoints go through the slow memory
access path, while I instrument the guest during TCG code generation),
and does not support all the kinds of information that I want to gather
(i.e., the registers that a specific guest instruction is using).

On the other hand, you really convinced me that using virtual devices is
the right thing to do to implement a backdoor channel, as opposed to my
previous approach of adding per-target special instructions. Thus it
will work in all modes (TCG and KVM) and in all targets.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

[Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Lucas Meneghel Rodrigues]

Add a test device which supports the kvmctl ioports,
so one can run the KVM unittest suite [1].

Usage:

qemu -device testdev

1) Removed port 0xf1, since now kvm-unit-tests use
   serial

2) Removed exit code port 0xf4, since that can be
   replaced by

-device isa-debugexit,iobase=0xf4,access-size=2

3) Removed ram size port 0xd1, since guest memory
   size can be retrieved from firmware, there's a
   patch for kvm-unit-tests including an API to
   retrieve that value.

[1] Preliminary versions of this patch were posted
to the mailing list about a year ago, I re-read the
comments of the thread, and had guidance from
Paolo about which ports to remove from the test
device.

CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
---
 hw/i386/Makefile.objs |   1 +
 hw/testdev.c          | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+)
 create mode 100644 hw/testdev.c

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 8c764bb..64d2787 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -11,5 +11,6 @@ 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 += testdev.o
 
 obj-y := $(addprefix ../,$(obj-y))
diff --git a/hw/testdev.c b/hw/testdev.c
new file mode 100644
index 0000000..44070f2
--- /dev/null
+++ b/hw/testdev.c
@@ -0,0 +1,131 @@
+#include <sys/mman.h>
+#include "hw.h"
+#include "qdev.h"
+#include "isa.h"
+
+struct testdev {
+    ISADevice dev;
+    MemoryRegion iomem;
+    CharDriverState *chr;
+};
+
+#define TYPE_TESTDEV "testdev"
+#define TESTDEV(obj) \
+     OBJECT_CHECK(struct testdev, (obj), TYPE_TESTDEV)
+
+static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
+{
+    struct testdev *dev = opaque;
+
+    qemu_set_irq(isa_get_irq(&dev->dev, addr - 0x2000), !!data);
+}
+
+static uint32 test_device_ioport_data;
+
+static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
+{
+    test_device_ioport_data = data;
+}
+
+static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
+{
+    return test_device_ioport_data;
+}
+
+static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
+{
+    target_phys_addr_t len = 4096;
+    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
+
+    mprotect(a, 4096, PROT_NONE);
+    mprotect(a, 4096, PROT_READ|PROT_WRITE);
+    cpu_physical_memory_unmap(a, len, 0, 0);
+}
+
+static char *iomem_buf;
+
+static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
+{
+    return iomem_buf[addr];
+}
+
+static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
+{
+    return *(uint16_t*)(iomem_buf + addr);
+}
+
+static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
+{
+    return *(uint32_t*)(iomem_buf + addr);
+}
+
+static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    iomem_buf[addr] = val;
+}
+
+static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    *(uint16_t*)(iomem_buf + addr) = val;
+}
+
+static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    *(uint32_t*)(iomem_buf + addr) = val;
+}
+
+static const MemoryRegionOps test_iomem_ops = {
+    .old_mmio = {
+        .read = { test_iomem_readb, test_iomem_readw, test_iomem_readl, },
+        .write = { test_iomem_writeb, test_iomem_writew, test_iomem_writel, },
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static int init_test_device(ISADevice *isa)
+{
+    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
+
+    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
+    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
+    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
+    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
+    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
+    iomem_buf = g_malloc0(0x10000);
+    memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
+                          "testdev", 0x10000);
+    memory_region_add_subregion(isa_address_space(&dev->dev), 0xff000000,
+                                                  &dev->iomem);
+    return 0;
+}
+
+static Property testdev_isa_properties[] = {
+    DEFINE_PROP_CHR("chardev", struct testdev, chr),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void testdev_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *k = ISA_DEVICE_CLASS(klass);
+
+    k->init = init_test_device;
+    dc->props = testdev_isa_properties;
+}
+
+static TypeInfo testdev_info = {
+    .name           = "testdev",
+    .parent         = TYPE_ISA_DEVICE,
+    .instance_size  = sizeof(struct testdev),
+    .class_init     = testdev_class_init,
+};
+
+static void testdev_register_types(void)
+{
+    type_register_static(&testdev_info);
+}
+
+type_init(testdev_register_types)
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Lucas Meneghel Rodrigues]

On 10/04/2012 12:49 AM, Lucas Meneghel Rodrigues wrote:
> Add a test device which supports the kvmctl ioports,
> so one can run the KVM unittest suite [1].
>
> Usage:
>
> qemu -device testdev
>
> 1) Removed port 0xf1, since now kvm-unit-tests use
>     serial
>
> 2) Removed exit code port 0xf4, since that can be
>     replaced by
>
> -device isa-debugexit,iobase=0xf4,access-size=2

I forgot to mention that this would work *if* the isa-debugexit device 
gets upstream. Paolo pointed this thread:

http://lists.gnu.org/archive/html/qemu-devel/2012-07/msg00818.html

But it appears that no consensus was reached.

> 3) Removed ram size port 0xd1, since guest memory
>     size can be retrieved from firmware, there's a
>     patch for kvm-unit-tests including an API to
>     retrieve that value.
>
> [1] Preliminary versions of this patch were posted
> to the mailing list about a year ago, I re-read the
> comments of the thread, and had guidance from
> Paolo about which ports to remove from the test
> device.
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
> ---
>   hw/i386/Makefile.objs |   1 +
>   hw/testdev.c          | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 132 insertions(+)
>   create mode 100644 hw/testdev.c
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 8c764bb..64d2787 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -11,5 +11,6 @@ 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 += testdev.o
>
>   obj-y := $(addprefix ../,$(obj-y))
> diff --git a/hw/testdev.c b/hw/testdev.c
> new file mode 100644
> index 0000000..44070f2
> --- /dev/null
> +++ b/hw/testdev.c
> @@ -0,0 +1,131 @@
> +#include <sys/mman.h>
> +#include "hw.h"
> +#include "qdev.h"
> +#include "isa.h"
> +
> +struct testdev {
> +    ISADevice dev;
> +    MemoryRegion iomem;
> +    CharDriverState *chr;
> +};
> +
> +#define TYPE_TESTDEV "testdev"
> +#define TESTDEV(obj) \
> +     OBJECT_CHECK(struct testdev, (obj), TYPE_TESTDEV)
> +
> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    struct testdev *dev = opaque;
> +
> +    qemu_set_irq(isa_get_irq(&dev->dev, addr - 0x2000), !!data);
> +}
> +
> +static uint32 test_device_ioport_data;
> +
> +static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    test_device_ioport_data = data;
> +}
> +
> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
> +{
> +    return test_device_ioport_data;
> +}
> +
> +static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    target_phys_addr_t len = 4096;
> +    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
> +
> +    mprotect(a, 4096, PROT_NONE);
> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
> +    cpu_physical_memory_unmap(a, len, 0, 0);
> +}
> +
> +static char *iomem_buf;
> +
> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return iomem_buf[addr];
> +}
> +
> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint16_t*)(iomem_buf + addr);
> +}
> +
> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint32_t*)(iomem_buf + addr);
> +}
> +
> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    iomem_buf[addr] = val;
> +}
> +
> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint16_t*)(iomem_buf + addr) = val;
> +}
> +
> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint32_t*)(iomem_buf + addr) = val;
> +}
> +
> +static const MemoryRegionOps test_iomem_ops = {
> +    .old_mmio = {
> +        .read = { test_iomem_readb, test_iomem_readw, test_iomem_readl, },
> +        .write = { test_iomem_writeb, test_iomem_writew, test_iomem_writel, },
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static int init_test_device(ISADevice *isa)
> +{
> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> +
> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
> +    iomem_buf = g_malloc0(0x10000);
> +    memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
> +                          "testdev", 0x10000);
> +    memory_region_add_subregion(isa_address_space(&dev->dev), 0xff000000,
> +                                                  &dev->iomem);
> +    return 0;
> +}
> +
> +static Property testdev_isa_properties[] = {
> +    DEFINE_PROP_CHR("chardev", struct testdev, chr),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void testdev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *k = ISA_DEVICE_CLASS(klass);
> +
> +    k->init = init_test_device;
> +    dc->props = testdev_isa_properties;
> +}
> +
> +static TypeInfo testdev_info = {
> +    .name           = "testdev",
> +    .parent         = TYPE_ISA_DEVICE,
> +    .instance_size  = sizeof(struct testdev),
> +    .class_init     = testdev_class_init,
> +};
> +
> +static void testdev_register_types(void)
> +{
> +    type_register_static(&testdev_info);
> +}
> +
> +type_init(testdev_register_types)
>

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Paolo Bonzini]

Il 04/10/2012 05:49, Lucas Meneghel Rodrigues ha scritto:
> Add a test device which supports the kvmctl ioports,
> so one can run the KVM unittest suite [1].
> 
> Usage:
> 
> qemu -device testdev
> 
> 1) Removed port 0xf1, since now kvm-unit-tests use
>    serial
> 
> 2) Removed exit code port 0xf4, since that can be
>    replaced by
> 
> -device isa-debugexit,iobase=0xf4,access-size=2

Can you include this in your series?

> 3) Removed ram size port 0xd1, since guest memory
>    size can be retrieved from firmware, there's a
>    patch for kvm-unit-tests including an API to
>    retrieve that value.
> 
> [1] Preliminary versions of this patch were posted
> to the mailing list about a year ago, I re-read the
> comments of the thread, and had guidance from
> Paolo about which ports to remove from the test
> device.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
> ---
>  hw/i386/Makefile.objs |   1 +
>  hw/testdev.c          | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 132 insertions(+)
>  create mode 100644 hw/testdev.c
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 8c764bb..64d2787 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -11,5 +11,6 @@ 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 += testdev.o
>  
>  obj-y := $(addprefix ../,$(obj-y))
> diff --git a/hw/testdev.c b/hw/testdev.c
> new file mode 100644
> index 0000000..44070f2
> --- /dev/null
> +++ b/hw/testdev.c
> @@ -0,0 +1,131 @@
> +#include <sys/mman.h>
> +#include "hw.h"
> +#include "qdev.h"
> +#include "isa.h"
> +
> +struct testdev {
> +    ISADevice dev;
> +    MemoryRegion iomem;
> +    CharDriverState *chr;

This member is not necessary anymore.

Paolo

> +};
> +
> +#define TYPE_TESTDEV "testdev"
> +#define TESTDEV(obj) \
> +     OBJECT_CHECK(struct testdev, (obj), TYPE_TESTDEV)
> +
> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    struct testdev *dev = opaque;
> +
> +    qemu_set_irq(isa_get_irq(&dev->dev, addr - 0x2000), !!data);
> +}
> +
> +static uint32 test_device_ioport_data;
> +
> +static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    test_device_ioport_data = data;
> +}
> +
> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
> +{
> +    return test_device_ioport_data;
> +}
> +
> +static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    target_phys_addr_t len = 4096;
> +    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
> +
> +    mprotect(a, 4096, PROT_NONE);
> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
> +    cpu_physical_memory_unmap(a, len, 0, 0);
> +}
> +
> +static char *iomem_buf;
> +
> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return iomem_buf[addr];
> +}
> +
> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint16_t*)(iomem_buf + addr);
> +}
> +
> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint32_t*)(iomem_buf + addr);
> +}
> +
> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    iomem_buf[addr] = val;
> +}
> +
> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint16_t*)(iomem_buf + addr) = val;
> +}
> +
> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint32_t*)(iomem_buf + addr) = val;
> +}
> +
> +static const MemoryRegionOps test_iomem_ops = {
> +    .old_mmio = {
> +        .read = { test_iomem_readb, test_iomem_readw, test_iomem_readl, },
> +        .write = { test_iomem_writeb, test_iomem_writew, test_iomem_writel, },
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static int init_test_device(ISADevice *isa)
> +{
> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> +
> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
> +    iomem_buf = g_malloc0(0x10000);
> +    memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
> +                          "testdev", 0x10000);
> +    memory_region_add_subregion(isa_address_space(&dev->dev), 0xff000000,
> +                                                  &dev->iomem);
> +    return 0;
> +}
> +
> +static Property testdev_isa_properties[] = {
> +    DEFINE_PROP_CHR("chardev", struct testdev, chr),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void testdev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *k = ISA_DEVICE_CLASS(klass);
> +
> +    k->init = init_test_device;
> +    dc->props = testdev_isa_properties;
> +}
> +
> +static TypeInfo testdev_info = {
> +    .name           = "testdev",
> +    .parent         = TYPE_ISA_DEVICE,
> +    .instance_size  = sizeof(struct testdev),
> +    .class_init     = testdev_class_init,
> +};
> +
> +static void testdev_register_types(void)
> +{
> +    type_register_static(&testdev_info);
> +}
> +
> +type_init(testdev_register_types)
> 

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Peter Maydell]

On 4 October 2012 04:49, Lucas Meneghel Rodrigues <lmr@redhat.com> wrote:
> Add a test device which supports the kvmctl ioports,
> so one can run the KVM unittest suite [1].
>
> Usage:
>
> qemu -device testdev
>
> 1) Removed port 0xf1, since now kvm-unit-tests use
>    serial
>
> 2) Removed exit code port 0xf4, since that can be
>    replaced by
>
> -device isa-debugexit,iobase=0xf4,access-size=2
>
> 3) Removed ram size port 0xd1, since guest memory
>    size can be retrieved from firmware, there's a
>    patch for kvm-unit-tests including an API to
>    retrieve that value.
>
> [1] Preliminary versions of this patch were posted
> to the mailing list about a year ago, I re-read the
> comments of the thread, and had guidance from
> Paolo about which ports to remove from the test
> device.

General remark: there's no documentation anywhere in
this patch. I don't necessarily mean user-facing docs,
but at least a descriptive comment saying what the
heck this device is and what it does would be helpful.


>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
> ---
>  hw/i386/Makefile.objs |   1 +
>  hw/testdev.c          | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 132 insertions(+)
>  create mode 100644 hw/testdev.c
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 8c764bb..64d2787 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -11,5 +11,6 @@ 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 += testdev.o

...the device is useful even in non-KVM configs, then?

>  obj-y := $(addprefix ../,$(obj-y))
> diff --git a/hw/testdev.c b/hw/testdev.c
> new file mode 100644
> index 0000000..44070f2
> --- /dev/null
> +++ b/hw/testdev.c
> @@ -0,0 +1,131 @@
> +#include <sys/mman.h>

This file needs a leading comment with the usual copyright/license/
description of what the file does.

> +#include "hw.h"
> +#include "qdev.h"
> +#include "isa.h"
> +
> +struct testdev {
> +    ISADevice dev;
> +    MemoryRegion iomem;
> +    CharDriverState *chr;
> +};
> +
> +#define TYPE_TESTDEV "testdev"
> +#define TESTDEV(obj) \
> +     OBJECT_CHECK(struct testdev, (obj), TYPE_TESTDEV)
> +
> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    struct testdev *dev = opaque;
> +
> +    qemu_set_irq(isa_get_irq(&dev->dev, addr - 0x2000), !!data);
> +}
> +
> +static uint32 test_device_ioport_data;
> +
> +static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    test_device_ioport_data = data;
> +}
> +
> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
> +{
> +    return test_device_ioport_data;
> +}
> +
> +static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    target_phys_addr_t len = 4096;
> +    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
> +
> +    mprotect(a, 4096, PROT_NONE);
> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
> +    cpu_physical_memory_unmap(a, len, 0, 0);
> +}
> +
> +static char *iomem_buf;
> +
> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return iomem_buf[addr];
> +}
> +
> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint16_t*)(iomem_buf + addr);
> +}
> +
> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint32_t*)(iomem_buf + addr);
> +}
> +
> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    iomem_buf[addr] = val;
> +}
> +
> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint16_t*)(iomem_buf + addr) = val;
> +}
> +
> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint32_t*)(iomem_buf + addr) = val;
> +}
> +
> +static const MemoryRegionOps test_iomem_ops = {
> +    .old_mmio = {
> +        .read = { test_iomem_readb, test_iomem_readw, test_iomem_readl, },
> +        .write = { test_iomem_writeb, test_iomem_writew, test_iomem_writel, },
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static int init_test_device(ISADevice *isa)
> +{
> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> +
> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
> +    iomem_buf = g_malloc0(0x10000);
> +    memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
> +                          "testdev", 0x10000);
> +    memory_region_add_subregion(isa_address_space(&dev->dev), 0xff000000,
> +                                                  &dev->iomem);
> +    return 0;
> +}
> +
> +static Property testdev_isa_properties[] = {
> +    DEFINE_PROP_CHR("chardev", struct testdev, chr),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void testdev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *k = ISA_DEVICE_CLASS(klass);
> +
> +    k->init = init_test_device;
> +    dc->props = testdev_isa_properties;
> +}
> +
> +static TypeInfo testdev_info = {
> +    .name           = "testdev",

Overly generic name?

> +    .parent         = TYPE_ISA_DEVICE,
> +    .instance_size  = sizeof(struct testdev),
> +    .class_init     = testdev_class_init,
> +};

Can this be generalised to not be specifically an ISA
device? (that's rather an x86-ism). Would the device be
useful for unit tests of other KVM architectures? Or
are we providing it purely for a legacy x86 testsuite?

> +
> +static void testdev_register_types(void)
> +{
> +    type_register_static(&testdev_info);
> +}
> +
> +type_init(testdev_register_types)
> --
> 1.7.11.4
>
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Paolo Bonzini]

Il 04/10/2012 10:02, Peter Maydell ha scritto:
> On 4 October 2012 04:49, Lucas Meneghel Rodrigues <lmr@redhat.com> wrote:
>> Add a test device which supports the kvmctl ioports,
>> so one can run the KVM unittest suite [1].
>>
>> Usage:
>>
>> qemu -device testdev
>>
>> 1) Removed port 0xf1, since now kvm-unit-tests use
>>    serial
>>
>> 2) Removed exit code port 0xf4, since that can be
>>    replaced by
>>
>> -device isa-debugexit,iobase=0xf4,access-size=2
>>
>> 3) Removed ram size port 0xd1, since guest memory
>>    size can be retrieved from firmware, there's a
>>    patch for kvm-unit-tests including an API to
>>    retrieve that value.
>>
>> [1] Preliminary versions of this patch were posted
>> to the mailing list about a year ago, I re-read the
>> comments of the thread, and had guidance from
>> Paolo about which ports to remove from the test
>> device.
> 
> General remark: there's no documentation anywhere in
> this patch. I don't necessarily mean user-facing docs,
> but at least a descriptive comment saying what the
> heck this device is and what it does would be helpful.
> 
> 
>>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
>> ---
>>  hw/i386/Makefile.objs |   1 +
>>  hw/testdev.c          | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 132 insertions(+)
>>  create mode 100644 hw/testdev.c
>>
>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>> index 8c764bb..64d2787 100644
>> --- a/hw/i386/Makefile.objs
>> +++ b/hw/i386/Makefile.objs
>> @@ -11,5 +11,6 @@ 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 += testdev.o
> 
> ...the device is useful even in non-KVM configs, then?
> 
>>  obj-y := $(addprefix ../,$(obj-y))
>> diff --git a/hw/testdev.c b/hw/testdev.c
>> new file mode 100644
>> index 0000000..44070f2
>> --- /dev/null
>> +++ b/hw/testdev.c
>> @@ -0,0 +1,131 @@
>> +#include <sys/mman.h>
> 
> This file needs a leading comment with the usual copyright/license/
> description of what the file does.
> 
>> +#include "hw.h"
>> +#include "qdev.h"
>> +#include "isa.h"
>> +
>> +struct testdev {
>> +    ISADevice dev;
>> +    MemoryRegion iomem;
>> +    CharDriverState *chr;
>> +};
>> +
>> +#define TYPE_TESTDEV "testdev"
>> +#define TESTDEV(obj) \
>> +     OBJECT_CHECK(struct testdev, (obj), TYPE_TESTDEV)
>> +
>> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
>> +{
>> +    struct testdev *dev = opaque;
>> +
>> +    qemu_set_irq(isa_get_irq(&dev->dev, addr - 0x2000), !!data);
>> +}
>> +
>> +static uint32 test_device_ioport_data;
>> +
>> +static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>> +{
>> +    test_device_ioport_data = data;
>> +}
>> +
>> +static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
>> +{
>> +    return test_device_ioport_data;
>> +}
>> +
>> +static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
>> +{
>> +    target_phys_addr_t len = 4096;
>> +    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
>> +
>> +    mprotect(a, 4096, PROT_NONE);
>> +    mprotect(a, 4096, PROT_READ|PROT_WRITE);
>> +    cpu_physical_memory_unmap(a, len, 0, 0);
>> +}
>> +
>> +static char *iomem_buf;
>> +
>> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
>> +{
>> +    return iomem_buf[addr];
>> +}
>> +
>> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
>> +{
>> +    return *(uint16_t*)(iomem_buf + addr);
>> +}
>> +
>> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
>> +{
>> +    return *(uint32_t*)(iomem_buf + addr);
>> +}
>> +
>> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
>> +{
>> +    iomem_buf[addr] = val;
>> +}
>> +
>> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
>> +{
>> +    *(uint16_t*)(iomem_buf + addr) = val;
>> +}
>> +
>> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>> +{
>> +    *(uint32_t*)(iomem_buf + addr) = val;
>> +}
>> +
>> +static const MemoryRegionOps test_iomem_ops = {
>> +    .old_mmio = {
>> +        .read = { test_iomem_readb, test_iomem_readw, test_iomem_readl, },
>> +        .write = { test_iomem_writeb, test_iomem_writew, test_iomem_writel, },
>> +    },
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static int init_test_device(ISADevice *isa)
>> +{
>> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
>> +
>> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
>> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
>> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
>> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
>> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
>> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
>> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
>> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
>> +    iomem_buf = g_malloc0(0x10000);
>> +    memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
>> +                          "testdev", 0x10000);
>> +    memory_region_add_subregion(isa_address_space(&dev->dev), 0xff000000,
>> +                                                  &dev->iomem);
>> +    return 0;
>> +}
>> +
>> +static Property testdev_isa_properties[] = {
>> +    DEFINE_PROP_CHR("chardev", struct testdev, chr),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void testdev_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ISADeviceClass *k = ISA_DEVICE_CLASS(klass);
>> +
>> +    k->init = init_test_device;
>> +    dc->props = testdev_isa_properties;
>> +}
>> +
>> +static TypeInfo testdev_info = {
>> +    .name           = "testdev",
> 
> Overly generic name?
> 
>> +    .parent         = TYPE_ISA_DEVICE,
>> +    .instance_size  = sizeof(struct testdev),
>> +    .class_init     = testdev_class_init,
>> +};
> 
> Can this be generalised to not be specifically an ISA
> device? (that's rather an x86-ism).

The IRQ parts are specifically ISA.

> Would the device be
> useful for unit tests of other KVM architectures? Or
> are we providing it purely for a legacy x86 testsuite?

It's not a legacy testsuite, but it's mostly x86-specific, testing
features that are exclusive to the x86 port (such as emulation, power
management, interrupt routing, etc.).

Paolo

>> +
>> +static void testdev_register_types(void)
>> +{
>> +    type_register_static(&testdev_info);
>> +}
>> +
>> +type_init(testdev_register_types)
>> --
>> 1.7.11.4
>>
>>
> 
> thanks
> -- PMM
>