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

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

[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