[Qemu-devel] [PATCH 0/2] Adding pc-testdev to qemu

[Qemu-devel] [PATCH 0/2] Adding pc-testdev to qemu

[Lucas Meneghel Rodrigues]

These two patches introduce:

 1) A ISA debugexit port device, that when written, causes qemu
    to exit with the exit code passed as parameter.
 2) A port of the existing testdev present on qemu-kvm to qemu

One particular comment of previous reviews wasn't addressed, but
I'd like people to see this to see if we're in the right direction.

Another thing I did notice is that with the new test device, a lot
of the kvm-unit-tests are failing. The same tests are passing fine
under qemu-kvm, meaning there are some bugs we need to address.

Example:

qemu-kvm:

$ x86_64-softmmu/qemu-system-x86_64 -device testdev -serial stdio -kernel /path/to/kvm/unittests/pcid.flat 
VNC server running on `127.0.0.1:5900'
enabling apic
CPUID consistency: PASS
Test on PCID when disabled: PASS
Test on INVPCID when disabled: PASS
3 tests, 0 failures
$ echo $?
0

qemu:

$ x86_64-softmmu/qemu-system-x86_64 -device pc-testdev -serial stdio -device isa-debugexit,iobase=0xf4,access-size=4 -kernel /path/to/kvm/unittests/pcid.flat 
VNC server running on `127.0.0.1:5900'
enabling apic
CPUID consistency: PASS
Test on PCID when disabled: FAIL
Test on INVPCID when disabled: PASS
3 tests, 1 failures
$ echo $?
3


Hervé Poussineau (1):
  debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

Lucas Meneghel Rodrigues (1):
  hw: Add test device for unittests execution v2

 hw/debugexit.c        |  68 +++++++++++++++++++++
 hw/i386/Makefile.objs |   3 +-
 hw/pc-testdev.c       | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+), 1 deletion(-)
 create mode 100644 hw/debugexit.c
 create mode 100644 hw/pc-testdev.c

-- 
1.7.11.4

[Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Lucas Meneghel Rodrigues]

From: Hervé Poussineau <hpoussin@reactos.org>

Add generic support for simple I/O port which, when written to, cause
QEMU to exit with the given written value.

There is no vmstate associated with the debugging port, simply because
the entire interface is a single, stateless, write-only port.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/debugexit.c        | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/Makefile.objs |  2 +-
 2 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 hw/debugexit.c

diff --git a/hw/debugexit.c b/hw/debugexit.c
new file mode 100644
index 0000000..5c0d726
--- /dev/null
+++ b/hw/debugexit.c
@@ -0,0 +1,68 @@
+/*
+ * QEMU debug exit port ("LGPL'ed-VGA-BIOS-style port 501/502") emulation
+ *
+ * Copyright (c) 2012 Herve Poussineau
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "isa.h"
+
+typedef struct ISADebugExitState {
+    ISADevice dev;
+    uint32_t iobase;
+    uint8_t access_size;
+} ISADebugExitState;
+
+static void debugexit_ioport_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    exit((val << 1) | 1);
+}
+
+static int debugexit_isa_initfn(ISADevice *dev)
+{
+    ISADebugExitState *isa = DO_UPCAST(ISADebugExitState, dev, dev);
+
+    register_ioport_write(isa->iobase, 1, isa->access_size,
+                          debugexit_ioport_write, NULL);
+    return 0;
+}
+
+static Property debugexit_isa_properties[] = {
+    DEFINE_PROP_HEX32("iobase", ISADebugExitState, iobase, 0x501),
+    DEFINE_PROP_UINT8("access-size", ISADebugExitState, access_size, 1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void debugexit_isa_class_initfn(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+    ic->init = debugexit_isa_initfn;
+    dc->props = debugexit_isa_properties;
+}
+
+static TypeInfo debugexit_isa_info = {
+    .name          = "isa-debugexit",
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(ISADebugExitState),
+    .class_init    = debugexit_isa_class_initfn,
+};
+
+static void debugexit_register_types(void)
+{
+    type_register_static(&debugexit_isa_info);
+}
+
+type_init(debugexit_register_types)
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 8c764bb..b34c61c 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -3,7 +3,7 @@ obj-y += apic_common.o apic.o kvmvapic.o
 obj-y += sga.o ioapic_common.o ioapic.o piix_pci.o
 obj-y += vmport.o
 obj-y += pci-hotplug.o smbios.o wdt_ib700.o
-obj-y += debugcon.o multiboot.o
+obj-y += debugcon.o debugexit.o multiboot.o
 obj-y += pc_piix.o
 obj-y += pc_sysfw.o
 obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o
-- 
1.7.11.4

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

[Lucas Meneghel Rodrigues]

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

Intended Usage:

qemu-system-x86_64 -device pc-testdev -serial stdio \
-device isa-debugexit,iobase=0xf4,access-size=4 \
-kernel /path/to/kvm/unittests/msr.flat

Where msr.flat is one of the KVM unittests, present on a
separate repo,

git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git

I still didn't figure out how to port register_ioport_read
to the memory API, but hopefully we'll get there.

Changes from v2:

1) Removed unused testdev member
2) Renamed device to a less generic name, pc-testdev

Initial changes from initial attempt:

1) Removed port 0xf1, since now kvm-unit-tests use
   serial
2) Removed exit code port 0xf4, since that can be
   replaced by

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

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

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

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index b34c61c..3ef3b4a 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -11,5 +11,6 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
 obj-y += kvm/
 obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
+obj-y += pc-testdev.o
 
 obj-y := $(addprefix ../,$(obj-y))
diff --git a/hw/pc-testdev.c b/hw/pc-testdev.c
new file mode 100644
index 0000000..0234adb
--- /dev/null
+++ b/hw/pc-testdev.c
@@ -0,0 +1,160 @@
+/*
+ * QEMU x86 ISA testdev
+ *
+ * Copyright (c) 2012 Avi Kivity, Gerd Hoffmann, Marcelo Tosatti
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+/*
+ * This device is used to test KVM features specific to the x86 port, such
+ * as emulation, power management, interrupt routing, among others. It's meant
+ * to be used like:
+ *
+ * qemu-system-x86_64 -device pc-testdev -serial stdio \
+ * -device isa-debugexit,iobase=0xf4,access-size=2 \
+ * -kernel /home/lmr/Code/virt-test.git/kvm/unittests/msr.flat
+ *
+ * Where msr.flat is one of the KVM unittests, present on a separate repo,
+ * git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git
+*/
+
+#include <sys/mman.h>
+#include "hw.h"
+#include "qdev.h"
+#include "isa.h"
+
+struct testdev {
+    ISADevice dev;
+    MemoryRegion iomem;
+};
+
+#define TYPE_TESTDEV "pc-testdev"
+#define TESTDEV(obj) \
+     OBJECT_CHECK(struct testdev, (obj), TYPE_TESTDEV)
+
+static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
+{
+    struct testdev *dev = opaque;
+
+    qemu_set_irq(isa_get_irq(&dev->dev, addr - 0x2000), !!data);
+}
+
+static uint32 test_device_ioport_data;
+
+static void test_device_ioport_write(void *opaque, uint32_t addr, uint32_t data)
+{
+    test_device_ioport_data = data;
+}
+
+static uint32_t test_device_ioport_read(void *opaque, uint32_t addr)
+{
+    return test_device_ioport_data;
+}
+
+static void test_device_flush_page(void *opaque, uint32_t addr, uint32_t data)
+{
+    target_phys_addr_t len = 4096;
+    void *a = cpu_physical_memory_map(data & ~0xffful, &len, 0);
+
+    mprotect(a, 4096, PROT_NONE);
+    mprotect(a, 4096, PROT_READ|PROT_WRITE);
+    cpu_physical_memory_unmap(a, len, 0, 0);
+}
+
+static char *iomem_buf;
+
+static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
+{
+    return iomem_buf[addr];
+}
+
+static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
+{
+    return *(uint16_t*)(iomem_buf + addr);
+}
+
+static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
+{
+    return *(uint32_t*)(iomem_buf + addr);
+}
+
+static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    iomem_buf[addr] = val;
+}
+
+static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    *(uint16_t*)(iomem_buf + addr) = val;
+}
+
+static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    *(uint32_t*)(iomem_buf + addr) = val;
+}
+
+static const MemoryRegionOps test_iomem_ops = {
+    .old_mmio = {
+        .read = { test_iomem_readb, test_iomem_readw, test_iomem_readl, },
+        .write = { test_iomem_writeb, test_iomem_writew, test_iomem_writel, },
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static int init_test_device(ISADevice *isa)
+{
+    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
+
+    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
+    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
+    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
+    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
+    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
+    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
+    iomem_buf = g_malloc0(0x10000);
+    memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
+                          "pc-testdev", 0x10000);
+    memory_region_add_subregion(isa_address_space(&dev->dev), 0xff000000,
+                                                  &dev->iomem);
+    return 0;
+}
+
+static void testdev_class_init(ObjectClass *klass, void *data)
+{
+    ISADeviceClass *k = ISA_DEVICE_CLASS(klass);
+
+    k->init = init_test_device;
+}
+
+static TypeInfo testdev_info = {
+    .name           = "pc-testdev",
+    .parent         = TYPE_ISA_DEVICE,
+    .instance_size  = sizeof(struct testdev),
+    .class_init     = testdev_class_init,
+};
+
+static void testdev_register_types(void)
+{
+    type_register_static(&testdev_info);
+}
+
+type_init(testdev_register_types)
-- 
1.7.11.4

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

[Jan Kiszka]

On 2012-10-04 19:31, Lucas Meneghel Rodrigues wrote:
> Add a test device which supports the kvmctl ioports,
> so one can run the KVM unittest suite.
> 
> Intended Usage:
> 
> qemu-system-x86_64 -device pc-testdev -serial stdio \
> -device isa-debugexit,iobase=0xf4,access-size=4 \
> -kernel /path/to/kvm/unittests/msr.flat
> 
> Where msr.flat is one of the KVM unittests, present on a
> separate repo,
> 
> git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git
> 
> I still didn't figure out how to port register_ioport_read
> to the memory API, but hopefully we'll get there.
> 
> Changes from v2:
> 
> 1) Removed unused testdev member
> 2) Renamed device to a less generic name, pc-testdev
> 
> Initial changes from initial attempt:
> 
> 1) Removed port 0xf1, since now kvm-unit-tests use
>    serial
> 2) Removed exit code port 0xf4, since that can be
>    replaced by
> 
> -device isa-debugexit,iobase=0xf4,access-size=4
> 
> 3) Removed ram size port 0xd1, since guest memory
>    size can be retrieved from firmware, there's a
>    patch for kvm-unit-tests including an API to
>    retrieve that value.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@redhat.com>
> ---
>  hw/i386/Makefile.objs |   1 +
>  hw/pc-testdev.c       | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 161 insertions(+)
>  create mode 100644 hw/pc-testdev.c
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index b34c61c..3ef3b4a 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -11,5 +11,6 @@ obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
>  obj-y += kvm/
>  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> +obj-y += pc-testdev.o
>  
>  obj-y := $(addprefix ../,$(obj-y))
> diff --git a/hw/pc-testdev.c b/hw/pc-testdev.c
> new file mode 100644
> index 0000000..0234adb
> --- /dev/null
> +++ b/hw/pc-testdev.c
> @@ -0,0 +1,160 @@
> +/*
> + * QEMU x86 ISA testdev
> + *
> + * Copyright (c) 2012 Avi Kivity, Gerd Hoffmann, Marcelo Tosatti
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +/*
> + * This device is used to test KVM features specific to the x86 port, such
> + * as emulation, power management, interrupt routing, among others. It's meant
> + * to be used like:
> + *
> + * qemu-system-x86_64 -device pc-testdev -serial stdio \
> + * -device isa-debugexit,iobase=0xf4,access-size=2 \
> + * -kernel /home/lmr/Code/virt-test.git/kvm/unittests/msr.flat
> + *
> + * Where msr.flat is one of the KVM unittests, present on a separate repo,
> + * git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git
> +*/
> +
> +#include <sys/mman.h>
> +#include "hw.h"
> +#include "qdev.h"
> +#include "isa.h"
> +
> +struct testdev {

typedef struct PCTestdev { ... } PCTestdev;

> +    ISADevice dev;
> +    MemoryRegion iomem;
> +};
> +
> +#define TYPE_TESTDEV "pc-testdev"
> +#define TESTDEV(obj) \
> +     OBJECT_CHECK(struct testdev, (obj), TYPE_TESTDEV)
> +
> +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    struct testdev *dev = opaque;
> +
> +    qemu_set_irq(isa_get_irq(&dev->dev, addr - 0x2000), !!data);
> +}
> +
> +static uint32 test_device_ioport_data;

Embed into PCTestdev.

> +
> +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;

Also this.

> +
> +static uint32_t test_iomem_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return iomem_buf[addr];
> +}
> +
> +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint16_t*)(iomem_buf + addr);
> +}
> +
> +static uint32_t test_iomem_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return *(uint32_t*)(iomem_buf + addr);
> +}
> +
> +static void test_iomem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    iomem_buf[addr] = val;
> +}
> +
> +static void test_iomem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint16_t*)(iomem_buf + addr) = val;
> +}
> +
> +static void test_iomem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    *(uint32_t*)(iomem_buf + addr) = val;
> +}
> +
> +static const MemoryRegionOps test_iomem_ops = {
> +    .old_mmio = {
> +        .read = { test_iomem_readb, test_iomem_readw, test_iomem_readl, },
> +        .write = { test_iomem_writeb, test_iomem_writew, test_iomem_writel, },
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static int init_test_device(ISADevice *isa)
> +{
> +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> +
> +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
> +    iomem_buf = g_malloc0(0x10000);
> +    memory_region_init_io(&dev->iomem, &test_iomem_ops, dev,
> +                          "pc-testdev", 0x10000);
> +    memory_region_add_subregion(isa_address_space(&dev->dev), 0xff000000,
> +                                                  &dev->iomem);
> +    return 0;
> +}
> +
> +static void testdev_class_init(ObjectClass *klass, void *data)
> +{
> +    ISADeviceClass *k = ISA_DEVICE_CLASS(klass);
> +
> +    k->init = init_test_device;
> +}
> +
> +static TypeInfo testdev_info = {
> +    .name           = "pc-testdev",
> +    .parent         = TYPE_ISA_DEVICE,
> +    .instance_size  = sizeof(struct testdev),
> +    .class_init     = testdev_class_init,
> +};
> +
> +static void testdev_register_types(void)
> +{
> +    type_register_static(&testdev_info);
> +}
> +
> +type_init(testdev_register_types)
> 

For the ioport conversion I already sent you a link. Should be
straightforward, but also needs to be applied on Hervé's patch.

What about making the port addresses configurable at this chance, just
like we do in debugcon?

I think the rest is fine.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Jan Kiszka]

[was a private thread due to typo in qemu list address]

On 2012-10-05 14:40, Anthony Liguori wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 05/10/2012 00:06, Lucas Meneghel Rodrigues ha scritto:
>>> +static void debugexit_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>> +{
>>> +    exit((val << 1) | 1);
>>> +}
>>> +
>>
>> This "| 1" might be the problem.  Anthony, are you relying on it in
>> qemu-test and/or can you work out the changes if we use just
>> "exit(val)"?
> 
> The reason for ' | 1' was to make sure that the guest couldn't trigger
> an exit(0).
> 
> If there's a compelling reason to drop '| 1', I can adjust my tests
> accordingly.

assert(val); (or hw_error)
exit(val);

I would suggest.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Paolo Bonzini]

Il 05/10/2012 14:43, Jan Kiszka ha scritto:
>>> >> This "| 1" might be the problem.  Anthony, are you relying on it in
>>> >> qemu-test and/or can you work out the changes if we use just
>>> >> "exit(val)"?
>> > 
>> > The reason for ' | 1' was to make sure that the guest couldn't trigger
>> > an exit(0).
>> > 
>> > If there's a compelling reason to drop '| 1', I can adjust my tests
>> > accordingly.
> assert(val); (or hw_error)
> exit(val);
> 
> I would suggest.

I think what the kvm_unittests want is exactly to trigger an exit(0).
Why did you rule it out?

Paolo

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Jan Kiszka]

On 2012-10-05 14:46, Paolo Bonzini wrote:
> Il 05/10/2012 14:43, Jan Kiszka ha scritto:
>>>>>> This "| 1" might be the problem.  Anthony, are you relying on it in
>>>>>> qemu-test and/or can you work out the changes if we use just
>>>>>> "exit(val)"?
>>>>
>>>> The reason for ' | 1' was to make sure that the guest couldn't trigger
>>>> an exit(0).
>>>>
>>>> If there's a compelling reason to drop '| 1', I can adjust my tests
>>>> accordingly.
>> assert(val); (or hw_error)
>> exit(val);
>>
>> I would suggest.
> 
> I think what the kvm_unittests want is exactly to trigger an exit(0).
> Why did you rule it out?

To avoid spurious exits on port scans etc. that appear like regular
ones. However, as we configure this device explicitly in, not adding it
in some default, that risk is likely low.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 05/10/2012 14:43, Jan Kiszka ha scritto:
>>>> >> This "| 1" might be the problem.  Anthony, are you relying on it in
>>>> >> qemu-test and/or can you work out the changes if we use just
>>>> >> "exit(val)"?
>>> > 
>>> > The reason for ' | 1' was to make sure that the guest couldn't trigger
>>> > an exit(0).
>>> > 
>>> > If there's a compelling reason to drop '| 1', I can adjust my tests
>>> > accordingly.
>> assert(val); (or hw_error)
>> exit(val);
>> 
>> I would suggest.
>
> I think what the kvm_unittests want is exactly to trigger an exit(0).
> Why did you rule it out?

Mainly to differientiate between an open coded exit(0)/exit(1) and
something triggered by the unit test.

The problem I tried to cope with was:

anthony@titi:~/git/qemu$ grep 'exit([01])' hw/*.c | wc -l
249

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Paolo Bonzini]

Il 05/10/2012 15:58, Anthony Liguori ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 05/10/2012 14:43, Jan Kiszka ha scritto:
>>>>>>> This "| 1" might be the problem.  Anthony, are you relying on it in
>>>>>>> qemu-test and/or can you work out the changes if we use just
>>>>>>> "exit(val)"?
>>>>>
>>>>> The reason for ' | 1' was to make sure that the guest couldn't trigger
>>>>> an exit(0).
>>>>>
>>>>> If there's a compelling reason to drop '| 1', I can adjust my tests
>>>>> accordingly.
>>> assert(val); (or hw_error)
>>> exit(val);
>>>
>>> I would suggest.
>>
>> I think what the kvm_unittests want is exactly to trigger an exit(0).
>> Why did you rule it out?
> 
> Mainly to differientiate between an open coded exit(0)/exit(1) and
> something triggered by the unit test.
> 
> The problem I tried to cope with was:
> 
> anthony@titi:~/git/qemu$ grep 'exit([01])' hw/*.c | wc -l
> 249

Understood.  The right solution is of course to fix hw/*.c.

Let's start with exit(0).

hw/eeprom93xx.c:    exit(0);           <--- should be hw_error
hw/pci.c:        exit(0);              <--- move to qemu_show_nic_models
hw/spapr.c:        exit(0);            <--- should be exit(1) !!

For exit(1), many of them should become hw_error, especially those in
this files:

hw/bonito.c
hw/ivshmem.c
hw/lsi53c895a.c
hw/pci.c
hw/pl022.c
hw/pl061.c
hw/pl110.c
hw/qxl.c
hw/smbus.c
hw/ssd0303.c
hw/ssi-sd.c
hw/stellaris_enet.c
hw/virtio-blk.c
hw/virtio.c
hw/virtio-net.c
hw/virtio-scsi.c

Any takers?

Paolo

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 05/10/2012 15:58, Anthony Liguori ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 05/10/2012 14:43, Jan Kiszka ha scritto:
>>>>>>>> This "| 1" might be the problem.  Anthony, are you relying on it in
>>>>>>>> qemu-test and/or can you work out the changes if we use just
>>>>>>>> "exit(val)"?
>>>>>>
>>>>>> The reason for ' | 1' was to make sure that the guest couldn't trigger
>>>>>> an exit(0).
>>>>>>
>>>>>> If there's a compelling reason to drop '| 1', I can adjust my tests
>>>>>> accordingly.
>>>> assert(val); (or hw_error)
>>>> exit(val);
>>>>
>>>> I would suggest.
>>>
>>> I think what the kvm_unittests want is exactly to trigger an exit(0).
>>> Why did you rule it out?
>> 
>> Mainly to differientiate between an open coded exit(0)/exit(1) and
>> something triggered by the unit test.
>> 
>> The problem I tried to cope with was:
>> 
>> anthony@titi:~/git/qemu$ grep 'exit([01])' hw/*.c | wc -l
>> 249
>
> Understood.  The right solution is of course to fix hw/*.c.

I don't think it's an awful thing for test harnesses to just use a
reserved range of exit reasons.

Should be easy enough to change kvm-unittests, no?

Regards,

Anthony Liguori

>
> Let's start with exit(0).
>
> hw/eeprom93xx.c:    exit(0);           <--- should be hw_error
> hw/pci.c:        exit(0);              <--- move to qemu_show_nic_models
> hw/spapr.c:        exit(0);            <--- should be exit(1) !!
>
> For exit(1), many of them should become hw_error, especially those in
> this files:
>
> hw/bonito.c
> hw/ivshmem.c
> hw/lsi53c895a.c
> hw/pci.c
> hw/pl022.c
> hw/pl061.c
> hw/pl110.c
> hw/qxl.c
> hw/smbus.c
> hw/ssd0303.c
> hw/ssi-sd.c
> hw/stellaris_enet.c
> hw/virtio-blk.c
> hw/virtio.c
> hw/virtio-net.c
> hw/virtio-scsi.c
>
> Any takers?
>
> Paolo

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Paolo Bonzini]

Il 05/10/2012 16:23, Anthony Liguori ha scritto:
>> > Understood.  The right solution is of course to fix hw/*.c.
> I don't think it's an awful thing for test harnesses to just use a
> reserved range of exit reasons.

But that's not what exit((val << 1) | 1) does, since it allows you to do
exit(1)...

Paolo

> Should be easy enough to change kvm-unittests, no?

Re: [Qemu-devel] [PATCH 1/2] debugexit: support for custom exit port (LGPL VGA BIOS port 0x501)

[Blue Swirl]

On Fri, Oct 5, 2012 at 2:15 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 05/10/2012 15:58, Anthony Liguori ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 05/10/2012 14:43, Jan Kiszka ha scritto:
>>>>>>>> This "| 1" might be the problem.  Anthony, are you relying on it in
>>>>>>>> qemu-test and/or can you work out the changes if we use just
>>>>>>>> "exit(val)"?
>>>>>>
>>>>>> The reason for ' | 1' was to make sure that the guest couldn't trigger
>>>>>> an exit(0).
>>>>>>
>>>>>> If there's a compelling reason to drop '| 1', I can adjust my tests
>>>>>> accordingly.
>>>> assert(val); (or hw_error)
>>>> exit(val);
>>>>
>>>> I would suggest.
>>>
>>> I think what the kvm_unittests want is exactly to trigger an exit(0).
>>> Why did you rule it out?
>>
>> Mainly to differientiate between an open coded exit(0)/exit(1) and
>> something triggered by the unit test.
>>
>> The problem I tried to cope with was:
>>
>> anthony@titi:~/git/qemu$ grep 'exit([01])' hw/*.c | wc -l
>> 249
>
> Understood.  The right solution is of course to fix hw/*.c.

While fixing, let's also avoid guest triggerable exits and use logging
for unimplemented features.

>
> Let's start with exit(0).
>
> hw/eeprom93xx.c:    exit(0);           <--- should be hw_error
> hw/pci.c:        exit(0);              <--- move to qemu_show_nic_models
> hw/spapr.c:        exit(0);            <--- should be exit(1) !!
>
> For exit(1), many of them should become hw_error, especially those in
> this files:
>
> hw/bonito.c
> hw/ivshmem.c
> hw/lsi53c895a.c
> hw/pci.c
> hw/pl022.c
> hw/pl061.c
> hw/pl110.c
> hw/qxl.c
> hw/smbus.c
> hw/ssd0303.c
> hw/ssi-sd.c
> hw/stellaris_enet.c
> hw/virtio-blk.c
> hw/virtio.c
> hw/virtio-net.c
> hw/virtio-scsi.c
>
> Any takers?
>
> Paolo
>
>