| Message ID | 1314389053-30841-1-git-send-email-lmr@redhat.com |
|---|---|
| State | New |
| Headers | show |
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)
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
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
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 > > >
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.
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.
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.
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.
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!
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!
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?
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
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?
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
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)
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