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
<address@hidden> 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<address@hidden>
>> >Signed-off-by: Avi Kivity<address@hidden>
>> >Signed-off-by: Marcelo Tosatti<address@hidden>
>> >Signed-off-by: Lucas Meneghel Rodrigues<address@hidden>
>> >---
>> >  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.