Patchwork hw: Add test device for unittests execution

login
register
mail settings
Submitter Lucas Meneghel Rodrigues
Date Aug. 26, 2011, 8:04 p.m.
Message ID <1314389053-30841-1-git-send-email-lmr@redhat.com>
Download mbox | patch
Permalink /patch/111835/
State New
Headers show

Comments

Lucas Meneghel Rodrigues - Aug. 26, 2011, 8:04 p.m.
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
Anthony Liguori - Aug. 26, 2011, 9:22 p.m.
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)
Jan Kiszka - Aug. 26, 2011, 10:26 p.m.
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
Edgar Iglesias - Aug. 27, 2011, 10:07 a.m.
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
Blue Swirl - Aug. 27, 2011, 4:22 p.m.
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
>
>
>
Blue Swirl - Aug. 27, 2011, 4:44 p.m.
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.
Avi Kivity - Aug. 29, 2011, 5:47 a.m.
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.
Avi Kivity - Aug. 29, 2011, 5:50 a.m.
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.
Avi Kivity - Aug. 29, 2011, 5:52 a.m.
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.
Avi Kivity - Aug. 29, 2011, 5:57 a.m.
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!
Lucas Meneghel Rodrigues - Aug. 29, 2011, 1:58 p.m.
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!
Blue Swirl - Aug. 30, 2011, 7:11 p.m.
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?
=?utf-8?Q?Llu=C3=ADs?= - Aug. 30, 2011, 7:36 p.m.
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
Blue Swirl - Aug. 30, 2011, 7:54 p.m.
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?
=?utf-8?Q?Llu=C3=ADs?= - Aug. 30, 2011, 8:20 p.m.
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

Patch

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)