Re: FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2011-01-06 Thread Stefan Hajnoczi
On Wed, Jan 5, 2011 at 5:01 PM, Serge E. Hallyn se...@hallyn.com wrote:
 I don't see this patch in the git tree, nor a revert of the buggy
 commit.  Was any decision made on this?

Blue Swirl posted a patch a few days ago:
[PATCH] pc: move port 92 stuff back to pc.c from pckbd.c

It hasn't been merged yet but I don't see any objections to it on the
email thread.  Perhaps he's just busy.

Stefan
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2011-01-06 Thread Serge E. Hallyn
Thanks, Stefan.  That patch actually doesn't compile for me, because
it leaves references in hw/pckbd.c to both ioport92_write and
ioport92_read, which it deletes from there.  Should ioport92_read
just be renamed to outport_read instead of delted, and the remaining
references changed to {input,output}_read?

thanks,
-serge

[patch for reference]

diff --git a/hw/pc.c b/hw/pc.c
index 18a4a9f..e63b397 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -411,11 +411,71 @@ void pc_cmos_init(ram_addr_t ram_size,
ram_addr_t above_4g_mem_size,
 qemu_register_reset(pc_cmos_init_late, arg);
 }

+/* port 92 stuff: could be split off */
+typedef struct Port92State {
+ISADevice dev;
+uint8_t outport;
+qemu_irq *a20_out;
+} Port92State;
+
+static void port92_write(void *opaque, uint32_t addr, uint32_t val)
+{
+Port92State *s = opaque;
+
+DPRINTF(port92: write 0x%02x\n, val);
+s-outport = val;
+qemu_set_irq(*s-a20_out, (val  1)  1);
+if (val  1) {
+qemu_system_reset_request();
+}
+}
+
+static uint32_t port92_read(void *opaque, uint32_t addr)
+{
+Port92State *s = opaque;
+uint32_t ret;
+
+ret = s-outport;
+DPRINTF(port92: read 0x%02x\n, ret);
+return ret;
+}
+
+static void port92_init(ISADevice *dev, qemu_irq *a20_out)
+{
+Port92State *s = DO_UPCAST(Port92State, dev, dev);
+
+s-a20_out = a20_out;
+}
+
+static int port92_initfn(ISADevice *dev)
+{
+Port92State *s = DO_UPCAST(Port92State, dev, dev);
+
+register_ioport_read(0x92, 1, 1, port92_read, s);
+register_ioport_write(0x92, 1, 1, port92_write, s);
+isa_init_ioport(dev, 0x92);
+return 0;
+}
+
+static ISADeviceInfo port92_info = {
+.qdev.name = port92,
+.qdev.size = sizeof(Port92State),
+.qdev.no_user  = 1,
+.init  = port92_initfn,
+};
+
+static void port92_register(void)
+{
+isa_qdev_register(port92_info);
+}
+device_init(port92_register)
+
 static void handle_a20_line_change(void *opaque, int irq, int level)
 {
 CPUState *cpu = opaque;

 /* XXX: send to all CPUs ? */
+/* XXX: add logic to handle multiple A20 line sources */
 cpu_x86_set_a20(cpu, level);
 }

@@ -1027,7 +1087,7 @@ void pc_basic_device_init(qemu_irq *isa_irq,
 PITState *pit;
 qemu_irq rtc_irq = NULL;
 qemu_irq *a20_line;
-ISADevice *i8042;
+ISADevice *i8042, *port92;
 qemu_irq *cpu_exit_irq;

 register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
@@ -1061,10 +1121,12 @@ void pc_basic_device_init(qemu_irq *isa_irq,
 }
 }

-a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
+a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
 i8042 = isa_create_simple(i8042);
-i8042_setup_a20_line(i8042, a20_line);
+i8042_setup_a20_line(i8042, a20_line[0]);
 vmmouse_init(i8042);
+port92 = isa_create_simple(port92);
+port92_init(port92, a20_line[1]);

 cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
 DMA_init(0, cpu_exit_irq);
diff --git a/hw/pckbd.c b/hw/pckbd.c
index 863b485..958de0a 100644
--- a/hw/pckbd.c
+++ b/hw/pckbd.c
@@ -211,10 +211,8 @@ static void kbd_queue(KBDState *s, int b, int aux)
 ps2_queue(s-kbd, b);
 }

-static void ioport92_write(void *opaque, uint32_t addr, uint32_t val)
+static void outport_write(KBDState *s, uint32_t addr, uint32_t val)
 {
-KBDState *s = opaque;
-
 DPRINTF(kbd: write outport=0x%02x\n, val);
 s-outport = val;
 if (s-a20_out) {
@@ -225,16 +223,6 @@ static void ioport92_write(void *opaque, uint32_t
addr, uint32_t val)
 }
 }

-static uint32_t ioport92_read(void *opaque, uint32_t addr)
-{
-KBDState *s = opaque;
-uint32_t ret;
-
-ret = s-outport;
-DPRINTF(kbd: read outport=0x%02x\n, ret);
-return ret;
-}
-
 static void kbd_write_command(void *opaque, uint32_t addr, uint32_t val)
 {
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2011-01-06 Thread Blue Swirl
On Thu, Jan 6, 2011 at 4:41 PM, Serge E. Hallyn se...@hallyn.com wrote:
 Thanks, Stefan.  That patch actually doesn't compile for me, because
 it leaves references in hw/pckbd.c to both ioport92_write and
 ioport92_read, which it deletes from there.  Should ioport92_read
 just be renamed to outport_read instead of delted, and the remaining
 references changed to {input,output}_read?

No, the patch is meant for QEMU. I'll apply it.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2011-01-05 Thread Serge E. Hallyn
Hi,

I don't see this patch in the git tree, nor a revert of the buggy
commit.  Was any decision made on this?

thanks,
-serge
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-28 Thread Peter Lieven

 On 27.12.2010 04:51, Stefan Hajnoczi wrote:

On Sun, Dec 26, 2010 at 9:21 PM, Peter Lievenp...@dlh.net  wrote:

Am 25.12.2010 um 20:02 schrieb Peter Lieven:


Am 23.12.2010 um 03:42 schrieb Stefan Hajnoczi:


On Wed, Dec 22, 2010 at 10:02 AM, Peter Lievenp...@dlh.net  wrote:

If I start a VM with the following parameters
qemu-kvm-0.13.0 -m 2048 -smp 2 -monitor tcp:0:4014,server,nowait -vnc :14 -name 
'ubuntu.test'  -boot order=dc,menu=off  -cdrom ubuntu-10.04.1-desktop-amd64.iso 
-k de

and select memtest in the Ubuntu CD Boot Menu, the VM immediately resets. After 
this reset there happen several errors including graphic corruption or the 
qemu-kvm binary
aborting with error 134.

Exactly the same scenario on the same machine with qemu-kvm-0.12.5 works 
flawlessly.

Any ideas?

You could track down the commit which broke this using git-bisect(1).
The steps are:

$ git bisect start v0.13.0 v0.12.5

Then:

$ ./configure [...]  make
$ x86_64-softmmu/qemu-system-x86_64 -m 2048 -smp 2 -monitor
tcp:0:4014,server,nowait -vnc :14 -name 'ubuntu.test'  -boot
order=dc,menu=off  -cdrom ubuntu-10.04.1-desktop-amd64.iso -k de

If memtest runs as expected:
$ git bisect good
otherwise:
$ git bisect bad

Keep repeating this and you should end up at the commit that introduced the bug.

this was the outcome of my bisect session:

956a3e6bb7386de48b642d4fee11f7f86a2fcf9a is first bad commit
commit 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
Author: Blue Swirlblauwir...@gmail.com
Date:   Sat May 22 07:59:01 2010 +

Compile pckbd only once

Use a qemu_irq to indicate A20 line changes. Move I/O port 92
to pckbd.c.

Signed-off-by: Blue Swirlblauwir...@gmail.com

:100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 
1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M  Makefile.objs
:100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 
ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M  Makefile.target
:04 04 dd03f81a42b5162c93c40c517f45eb9f7bece93c 
309f472328632319a15128a59715aa63daf4d92c M  default-configs
:04 04 83201c4fcde2f592a771479246e0a33a8906515b 
b1192bce85f2a7129fb19cf2fe7462ef168165cb M  hw
bisect run success

I tracked down the regression to a bug in commit 
956a3e6bb7386de48b642d4fee11f7f86a2fcf9a

In the patch the outport of the keyboard controller and ioport 0x92 are made 
the same.

this cannot work:

a) both share bit 1 to enable a20_gate. 1=enable, 0=disable -  ok so far
b) both implement a fast reset option through bit 0, but with inverse logic!!!
the keyboard controller resets if bit 0 is lowered, the ioport 0x92 resets if 
bit 0 is raised.
c) all other bits have nothing in common at all.

see: http://www.brokenthorn.com/Resources/OSDev9.html

I have a proposed patch attached. Comments appreciated. The state of the A20 
Gate is still
shared between ioport 0x92 and outport of the keyboard controller, but all 
other bits are ignored.
They might be used in the future to emulate e.g. hdd led activity or other 
usage of ioport 0x92.

I have tested the attached patch. memtest works again as expected. I think it 
crashed because it uses
ioport 0x92 directly to enable the a20 gate.

Peter

---

--- qemu-0.13.0/hw/pckbd.c  2010-10-15 22:56:09.0 +0200
+++ qemu-0.13.0-fix/hw/pckbd.c  2010-12-26 19:38:35.835114033 +0100
@@ -212,13 +212,16 @@
static void ioport92_write(void *opaque, uint32_t addr, uint32_t val)
{
KBDState *s = opaque;
-
-DPRINTF(kbd: write outport=0x%02x\n, val);
-s-outport = val;
-if (s-a20_out) {
-qemu_set_irq(*s-a20_out, (val  1)  1);
+if (val  0x02) { // bit 1: enable/disable A20
+   if (s-a20_out) qemu_irq_raise(*s-a20_out);
+   s-outport |= KBD_OUT_A20;
+}
+else
+{
+   if (s-a20_out) qemu_irq_lower(*s-a20_out);
+   s-outport= ~KBD_OUT_A20;
}
-if (!(val  1)) {
+if ((val  1)) { // bit 0: raised -  fast reset
qemu_system_reset_request();
}
}
@@ -226,11 +229,8 @@
static uint32_t ioport92_read(void *opaque, uint32_t addr)
{
KBDState *s = opaque;
-uint32_t ret;
-
-ret = s-outport;
-DPRINTF(kbd: read outport=0x%02x\n, ret);
-return ret;
+return (s-outport  0x02); // only bit 1 (KBD_OUT_A20) of port 0x92 is 
identical to s-outport
+/* XXX: bit 0 is fast reset, bits 6-7 hdd activity */
}

static void kbd_write_command(void *opaque, uint32_t addr, uint32_t val)
@@ -340,7 +340,9 @@
kbd_queue(s, val, 1);
break;
case KBD_CCMD_WRITE_OUTPORT:
-ioport92_write(s, 0, val);
+ioport92_write(s, 0, (ioport92_read(s,0)  0xfc) // copy bits 2-7 of 
0x92
+ | (val  0x02) // bit 1 (enable a20)
+ | (~val  0x01)); // bit 0 (fast reset) of port 
0x92 has inverse logic
break;
case KBD_CCMD_WRITE_MOUSE:
ps2_write_mouse(s-mouse, val);



I just replied to the original thread.  I think we should separate
0x92 and the keyboard controller port since they are quite 

Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-28 Thread Blue Swirl
On Mon, Dec 27, 2010 at 3:47 AM, Stefan Hajnoczi stefa...@gmail.com wrote:
 On Sat, Dec 25, 2010 at 7:02 PM, Peter Lieven p...@dlh.net wrote:
 this was the outcome of my bisect session:

 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a is first bad commit
 commit 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
 Author: Blue Swirl blauwir...@gmail.com
 Date:   Sat May 22 07:59:01 2010 +

    Compile pckbd only once

    Use a qemu_irq to indicate A20 line changes. Move I/O port 92
    to pckbd.c.

    Signed-off-by: Blue Swirl blauwir...@gmail.com

 :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 
 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M      Makefile.objs
 :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 
 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M      Makefile.target
 :04 04 dd03f81a42b5162c93c40c517f45eb9f7bece93c 
 309f472328632319a15128a59715aa63daf4d92c M      default-configs
 :04 04 83201c4fcde2f592a771479246e0a33a8906515b 
 b1192bce85f2a7129fb19cf2fe7462ef168165cb M      hw
 bisect run success

 Nice job bisecting this!  I can reproduce the Memtest86+ V4.10 system
 reset with qemu-kvm.git and qemu.git.

 The following code path is hit when val=0x2:
 if (!(val  1)) {
    qemu_system_reset_request();
 }

 I think unifying ioport 0x92 and KBD_CCMD_WRITE_OUTPORT was incorrect.
  ioport 0x92 is the System Control Port A and resets the system if bit
 0 is 1.  The keyboard outport seems to reset if bit 0 is 0.

 Here are the links I've found describing the i8042 keyboard controller
 and System Control Port A:
 http://www.computer-engineering.org/ps2keyboard/
 http://www.win.tue.nl/~aeb/linux/kbd/A20.html

 Blue Swirl: Any thoughts on this?

SMSC LPC47S45x data sheet (p. 112) also confirms this. The proper fix
is to split the ports.

But then A20 logic becomes more complex, disabling either of keyboard
or port 92 A20 lines should disable system A20 gate.

I think we should move port 92 back to pc.c and add the missing A20
logic there as well.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-26 Thread Peter Lieven

Am 25.12.2010 um 20:02 schrieb Peter Lieven:

 
 Am 23.12.2010 um 03:42 schrieb Stefan Hajnoczi:
 
 On Wed, Dec 22, 2010 at 10:02 AM, Peter Lieven p...@dlh.net wrote:
 If I start a VM with the following parameters
 qemu-kvm-0.13.0 -m 2048 -smp 2 -monitor tcp:0:4014,server,nowait -vnc :14 
 -name 'ubuntu.test'  -boot order=dc,menu=off  -cdrom 
 ubuntu-10.04.1-desktop-amd64.iso -k de
 
 and select memtest in the Ubuntu CD Boot Menu, the VM immediately resets. 
 After this reset there happen several errors including graphic corruption 
 or the qemu-kvm binary
 aborting with error 134.
 
 Exactly the same scenario on the same machine with qemu-kvm-0.12.5 works 
 flawlessly.
 
 Any ideas?
 
 You could track down the commit which broke this using git-bisect(1).
 The steps are:
 
 $ git bisect start v0.13.0 v0.12.5
 
 Then:
 
 $ ./configure [...]  make
 $ x86_64-softmmu/qemu-system-x86_64 -m 2048 -smp 2 -monitor
 tcp:0:4014,server,nowait -vnc :14 -name 'ubuntu.test'  -boot
 order=dc,menu=off  -cdrom ubuntu-10.04.1-desktop-amd64.iso -k de
 
 If memtest runs as expected:
 $ git bisect good
 otherwise:
 $ git bisect bad
 
 Keep repeating this and you should end up at the commit that introduced the 
 bug.
 
 this was the outcome of my bisect session:
 
 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a is first bad commit
 commit 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
 Author: Blue Swirl blauwir...@gmail.com
 Date:   Sat May 22 07:59:01 2010 +
 
Compile pckbd only once
 
Use a qemu_irq to indicate A20 line changes. Move I/O port 92
to pckbd.c.
 
Signed-off-by: Blue Swirl blauwir...@gmail.com
 
 :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 
 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M  Makefile.objs
 :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 
 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M  Makefile.target
 :04 04 dd03f81a42b5162c93c40c517f45eb9f7bece93c 
 309f472328632319a15128a59715aa63daf4d92c M  default-configs
 :04 04 83201c4fcde2f592a771479246e0a33a8906515b 
 b1192bce85f2a7129fb19cf2fe7462ef168165cb M  hw
 bisect run success

I tracked down the regression to a bug in commit 
956a3e6bb7386de48b642d4fee11f7f86a2fcf9a

In the patch the outport of the keyboard controller and ioport 0x92 are made 
the same.

this cannot work:

a) both share bit 1 to enable a20_gate. 1=enable, 0=disable - ok so far
b) both implement a fast reset option through bit 0, but with inverse logic!!!
the keyboard controller resets if bit 0 is lowered, the ioport 0x92 resets if 
bit 0 is raised.
c) all other bits have nothing in common at all.

see: http://www.brokenthorn.com/Resources/OSDev9.html

I have a proposed patch attached. Comments appreciated. The state of the A20 
Gate is still
shared between ioport 0x92 and outport of the keyboard controller, but all 
other bits are ignored.
They might be used in the future to emulate e.g. hdd led activity or other 
usage of ioport 0x92.

I have tested the attached patch. memtest works again as expected. I think it 
crashed because it uses
ioport 0x92 directly to enable the a20 gate.

Peter

---

--- qemu-0.13.0/hw/pckbd.c  2010-10-15 22:56:09.0 +0200
+++ qemu-0.13.0-fix/hw/pckbd.c  2010-12-26 19:38:35.835114033 +0100
@@ -212,13 +212,16 @@
static void ioport92_write(void *opaque, uint32_t addr, uint32_t val)
{
KBDState *s = opaque;
-
-DPRINTF(kbd: write outport=0x%02x\n, val);
-s-outport = val;
-if (s-a20_out) {
-qemu_set_irq(*s-a20_out, (val  1)  1);
+if (val  0x02) { // bit 1: enable/disable A20
+   if (s-a20_out) qemu_irq_raise(*s-a20_out);
+   s-outport |= KBD_OUT_A20;
+}
+else
+{
+   if (s-a20_out) qemu_irq_lower(*s-a20_out);
+   s-outport = ~KBD_OUT_A20;
}
-if (!(val  1)) {
+if ((val  1)) { // bit 0: raised - fast reset
qemu_system_reset_request();
}
}
@@ -226,11 +229,8 @@
static uint32_t ioport92_read(void *opaque, uint32_t addr)
{
KBDState *s = opaque;
-uint32_t ret;
-
-ret = s-outport;
-DPRINTF(kbd: read outport=0x%02x\n, ret);
-return ret;
+return (s-outport  0x02); // only bit 1 (KBD_OUT_A20) of port 0x92 is 
identical to s-outport
+/* XXX: bit 0 is fast reset, bits 6-7 hdd activity */
}

static void kbd_write_command(void *opaque, uint32_t addr, uint32_t val)
@@ -340,7 +340,9 @@
kbd_queue(s, val, 1);
break;
case KBD_CCMD_WRITE_OUTPORT:
-ioport92_write(s, 0, val);
+ioport92_write(s, 0, (ioport92_read(s,0)  0xfc) // copy bits 2-7 of 
0x92 
+ | (val  0x02) // bit 1 (enable a20)
+ | (~val  0x01)); // bit 0 (fast reset) of port 
0x92 has inverse logic
break;
case KBD_CCMD_WRITE_MOUSE:
ps2_write_mouse(s-mouse, val);

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-26 Thread Stefan Hajnoczi
On Sat, Dec 25, 2010 at 7:02 PM, Peter Lieven p...@dlh.net wrote:
 this was the outcome of my bisect session:

 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a is first bad commit
 commit 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
 Author: Blue Swirl blauwir...@gmail.com
 Date:   Sat May 22 07:59:01 2010 +

    Compile pckbd only once

    Use a qemu_irq to indicate A20 line changes. Move I/O port 92
    to pckbd.c.

    Signed-off-by: Blue Swirl blauwir...@gmail.com

 :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 
 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M      Makefile.objs
 :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 
 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M      Makefile.target
 :04 04 dd03f81a42b5162c93c40c517f45eb9f7bece93c 
 309f472328632319a15128a59715aa63daf4d92c M      default-configs
 :04 04 83201c4fcde2f592a771479246e0a33a8906515b 
 b1192bce85f2a7129fb19cf2fe7462ef168165cb M      hw
 bisect run success

Nice job bisecting this!  I can reproduce the Memtest86+ V4.10 system
reset with qemu-kvm.git and qemu.git.

The following code path is hit when val=0x2:
if (!(val  1)) {
qemu_system_reset_request();
}

I think unifying ioport 0x92 and KBD_CCMD_WRITE_OUTPORT was incorrect.
 ioport 0x92 is the System Control Port A and resets the system if bit
0 is 1.  The keyboard outport seems to reset if bit 0 is 0.

Here are the links I've found describing the i8042 keyboard controller
and System Control Port A:
http://www.computer-engineering.org/ps2keyboard/
http://www.win.tue.nl/~aeb/linux/kbd/A20.html

Blue Swirl: Any thoughts on this?

Stefan
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-26 Thread Stefan Hajnoczi
On Sun, Dec 26, 2010 at 9:21 PM, Peter Lieven p...@dlh.net wrote:

 Am 25.12.2010 um 20:02 schrieb Peter Lieven:


 Am 23.12.2010 um 03:42 schrieb Stefan Hajnoczi:

 On Wed, Dec 22, 2010 at 10:02 AM, Peter Lieven p...@dlh.net wrote:
 If I start a VM with the following parameters
 qemu-kvm-0.13.0 -m 2048 -smp 2 -monitor tcp:0:4014,server,nowait -vnc :14 
 -name 'ubuntu.test'  -boot order=dc,menu=off  -cdrom 
 ubuntu-10.04.1-desktop-amd64.iso -k de

 and select memtest in the Ubuntu CD Boot Menu, the VM immediately resets. 
 After this reset there happen several errors including graphic corruption 
 or the qemu-kvm binary
 aborting with error 134.

 Exactly the same scenario on the same machine with qemu-kvm-0.12.5 works 
 flawlessly.

 Any ideas?

 You could track down the commit which broke this using git-bisect(1).
 The steps are:

 $ git bisect start v0.13.0 v0.12.5

 Then:

 $ ./configure [...]  make
 $ x86_64-softmmu/qemu-system-x86_64 -m 2048 -smp 2 -monitor
 tcp:0:4014,server,nowait -vnc :14 -name 'ubuntu.test'  -boot
 order=dc,menu=off  -cdrom ubuntu-10.04.1-desktop-amd64.iso -k de

 If memtest runs as expected:
 $ git bisect good
 otherwise:
 $ git bisect bad

 Keep repeating this and you should end up at the commit that introduced the 
 bug.

 this was the outcome of my bisect session:

 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a is first bad commit
 commit 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
 Author: Blue Swirl blauwir...@gmail.com
 Date:   Sat May 22 07:59:01 2010 +

    Compile pckbd only once

    Use a qemu_irq to indicate A20 line changes. Move I/O port 92
    to pckbd.c.

    Signed-off-by: Blue Swirl blauwir...@gmail.com

 :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 
 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M      Makefile.objs
 :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 
 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M      Makefile.target
 :04 04 dd03f81a42b5162c93c40c517f45eb9f7bece93c 
 309f472328632319a15128a59715aa63daf4d92c M      default-configs
 :04 04 83201c4fcde2f592a771479246e0a33a8906515b 
 b1192bce85f2a7129fb19cf2fe7462ef168165cb M      hw
 bisect run success

 I tracked down the regression to a bug in commit 
 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a

 In the patch the outport of the keyboard controller and ioport 0x92 are made 
 the same.

 this cannot work:

 a) both share bit 1 to enable a20_gate. 1=enable, 0=disable - ok so far
 b) both implement a fast reset option through bit 0, but with inverse logic!!!
 the keyboard controller resets if bit 0 is lowered, the ioport 0x92 resets if 
 bit 0 is raised.
 c) all other bits have nothing in common at all.

 see: http://www.brokenthorn.com/Resources/OSDev9.html

 I have a proposed patch attached. Comments appreciated. The state of the A20 
 Gate is still
 shared between ioport 0x92 and outport of the keyboard controller, but all 
 other bits are ignored.
 They might be used in the future to emulate e.g. hdd led activity or other 
 usage of ioport 0x92.

 I have tested the attached patch. memtest works again as expected. I think it 
 crashed because it uses
 ioport 0x92 directly to enable the a20 gate.

 Peter

 ---

 --- qemu-0.13.0/hw/pckbd.c      2010-10-15 22:56:09.0 +0200
 +++ qemu-0.13.0-fix/hw/pckbd.c  2010-12-26 19:38:35.835114033 +0100
 @@ -212,13 +212,16 @@
 static void ioport92_write(void *opaque, uint32_t addr, uint32_t val)
 {
    KBDState *s = opaque;
 -
 -    DPRINTF(kbd: write outport=0x%02x\n, val);
 -    s-outport = val;
 -    if (s-a20_out) {
 -        qemu_set_irq(*s-a20_out, (val  1)  1);
 +    if (val  0x02) { // bit 1: enable/disable A20
 +       if (s-a20_out) qemu_irq_raise(*s-a20_out);
 +       s-outport |= KBD_OUT_A20;
 +    }
 +    else
 +    {
 +       if (s-a20_out) qemu_irq_lower(*s-a20_out);
 +       s-outport = ~KBD_OUT_A20;
    }
 -    if (!(val  1)) {
 +    if ((val  1)) { // bit 0: raised - fast reset
        qemu_system_reset_request();
    }
 }
 @@ -226,11 +229,8 @@
 static uint32_t ioport92_read(void *opaque, uint32_t addr)
 {
    KBDState *s = opaque;
 -    uint32_t ret;
 -
 -    ret = s-outport;
 -    DPRINTF(kbd: read outport=0x%02x\n, ret);
 -    return ret;
 +    return (s-outport  0x02); // only bit 1 (KBD_OUT_A20) of port 0x92 is 
 identical to s-outport
 +    /* XXX: bit 0 is fast reset, bits 6-7 hdd activity */
 }

 static void kbd_write_command(void *opaque, uint32_t addr, uint32_t val)
 @@ -340,7 +340,9 @@
        kbd_queue(s, val, 1);
        break;
    case KBD_CCMD_WRITE_OUTPORT:
 -        ioport92_write(s, 0, val);
 +        ioport92_write(s, 0, (ioport92_read(s,0)  0xfc) // copy bits 2-7 of 
 0x92
 +                             | (val  0x02) // bit 1 (enable a20)
 +                             | (~val  0x01)); // bit 0 (fast reset) of port 
 0x92 has inverse logic
        break;
    case KBD_CCMD_WRITE_MOUSE:
        ps2_write_mouse(s-mouse, val);



I just replied to the original thread.  I think we should separate
0x92 

Re: FIXED: Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-26 Thread Peter Lieven

Am 27.12.2010 um 04:51 schrieb Stefan Hajnoczi:

 On Sun, Dec 26, 2010 at 9:21 PM, Peter Lieven p...@dlh.net wrote:
 
 Am 25.12.2010 um 20:02 schrieb Peter Lieven:
 
 
 Am 23.12.2010 um 03:42 schrieb Stefan Hajnoczi:
 
 On Wed, Dec 22, 2010 at 10:02 AM, Peter Lieven p...@dlh.net wrote:
 If I start a VM with the following parameters
 qemu-kvm-0.13.0 -m 2048 -smp 2 -monitor tcp:0:4014,server,nowait -vnc :14 
 -name 'ubuntu.test'  -boot order=dc,menu=off  -cdrom 
 ubuntu-10.04.1-desktop-amd64.iso -k de
 
 and select memtest in the Ubuntu CD Boot Menu, the VM immediately resets. 
 After this reset there happen several errors including graphic corruption 
 or the qemu-kvm binary
 aborting with error 134.
 
 Exactly the same scenario on the same machine with qemu-kvm-0.12.5 works 
 flawlessly.
 
 Any ideas?
 
 You could track down the commit which broke this using git-bisect(1).
 The steps are:
 
 $ git bisect start v0.13.0 v0.12.5
 
 Then:
 
 $ ./configure [...]  make
 $ x86_64-softmmu/qemu-system-x86_64 -m 2048 -smp 2 -monitor
 tcp:0:4014,server,nowait -vnc :14 -name 'ubuntu.test'  -boot
 order=dc,menu=off  -cdrom ubuntu-10.04.1-desktop-amd64.iso -k de
 
 If memtest runs as expected:
 $ git bisect good
 otherwise:
 $ git bisect bad
 
 Keep repeating this and you should end up at the commit that introduced 
 the bug.
 
 this was the outcome of my bisect session:
 
 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a is first bad commit
 commit 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
 Author: Blue Swirl blauwir...@gmail.com
 Date:   Sat May 22 07:59:01 2010 +
 
Compile pckbd only once
 
Use a qemu_irq to indicate A20 line changes. Move I/O port 92
to pckbd.c.
 
Signed-off-by: Blue Swirl blauwir...@gmail.com
 
 :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 
 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M  Makefile.objs
 :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 
 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M  Makefile.target
 :04 04 dd03f81a42b5162c93c40c517f45eb9f7bece93c 
 309f472328632319a15128a59715aa63daf4d92c M  default-configs
 :04 04 83201c4fcde2f592a771479246e0a33a8906515b 
 b1192bce85f2a7129fb19cf2fe7462ef168165cb M  hw
 bisect run success
 
 I tracked down the regression to a bug in commit 
 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
 
 In the patch the outport of the keyboard controller and ioport 0x92 are made 
 the same.
 
 this cannot work:
 
 a) both share bit 1 to enable a20_gate. 1=enable, 0=disable - ok so far
 b) both implement a fast reset option through bit 0, but with inverse 
 logic!!!
 the keyboard controller resets if bit 0 is lowered, the ioport 0x92 resets 
 if bit 0 is raised.
 c) all other bits have nothing in common at all.
 
 see: http://www.brokenthorn.com/Resources/OSDev9.html
 
 I have a proposed patch attached. Comments appreciated. The state of the A20 
 Gate is still
 shared between ioport 0x92 and outport of the keyboard controller, but all 
 other bits are ignored.
 They might be used in the future to emulate e.g. hdd led activity or other 
 usage of ioport 0x92.
 
 I have tested the attached patch. memtest works again as expected. I think 
 it crashed because it uses
 ioport 0x92 directly to enable the a20 gate.
 
 Peter
 
 ---
 
 --- qemu-0.13.0/hw/pckbd.c  2010-10-15 22:56:09.0 +0200
 +++ qemu-0.13.0-fix/hw/pckbd.c  2010-12-26 19:38:35.835114033 +0100
 @@ -212,13 +212,16 @@
 static void ioport92_write(void *opaque, uint32_t addr, uint32_t val)
 {
KBDState *s = opaque;
 -
 -DPRINTF(kbd: write outport=0x%02x\n, val);
 -s-outport = val;
 -if (s-a20_out) {
 -qemu_set_irq(*s-a20_out, (val  1)  1);
 +if (val  0x02) { // bit 1: enable/disable A20
 +   if (s-a20_out) qemu_irq_raise(*s-a20_out);
 +   s-outport |= KBD_OUT_A20;
 +}
 +else
 +{
 +   if (s-a20_out) qemu_irq_lower(*s-a20_out);
 +   s-outport = ~KBD_OUT_A20;
}
 -if (!(val  1)) {
 +if ((val  1)) { // bit 0: raised - fast reset
qemu_system_reset_request();
}
 }
 @@ -226,11 +229,8 @@
 static uint32_t ioport92_read(void *opaque, uint32_t addr)
 {
KBDState *s = opaque;
 -uint32_t ret;
 -
 -ret = s-outport;
 -DPRINTF(kbd: read outport=0x%02x\n, ret);
 -return ret;
 +return (s-outport  0x02); // only bit 1 (KBD_OUT_A20) of port 0x92 is 
 identical to s-outport
 +/* XXX: bit 0 is fast reset, bits 6-7 hdd activity */
 }
 
 static void kbd_write_command(void *opaque, uint32_t addr, uint32_t val)
 @@ -340,7 +340,9 @@
kbd_queue(s, val, 1);
break;
case KBD_CCMD_WRITE_OUTPORT:
 -ioport92_write(s, 0, val);
 +ioport92_write(s, 0, (ioport92_read(s,0)  0xfc) // copy bits 2-7 
 of 0x92
 + | (val  0x02) // bit 1 (enable a20)
 + | (~val  0x01)); // bit 0 (fast reset) of 
 port 0x92 has inverse logic
break;
case KBD_CCMD_WRITE_MOUSE:

Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-25 Thread Peter Lieven

Am 23.12.2010 um 03:42 schrieb Stefan Hajnoczi:

 On Wed, Dec 22, 2010 at 10:02 AM, Peter Lieven p...@dlh.net wrote:
 If I start a VM with the following parameters
 qemu-kvm-0.13.0 -m 2048 -smp 2 -monitor tcp:0:4014,server,nowait -vnc :14 
 -name 'ubuntu.test'  -boot order=dc,menu=off  -cdrom 
 ubuntu-10.04.1-desktop-amd64.iso -k de
 
 and select memtest in the Ubuntu CD Boot Menu, the VM immediately resets. 
 After this reset there happen several errors including graphic corruption or 
 the qemu-kvm binary
 aborting with error 134.
 
 Exactly the same scenario on the same machine with qemu-kvm-0.12.5 works 
 flawlessly.
 
 Any ideas?
 
 You could track down the commit which broke this using git-bisect(1).
 The steps are:
 
 $ git bisect start v0.13.0 v0.12.5
 
 Then:
 
 $ ./configure [...]  make
 $ x86_64-softmmu/qemu-system-x86_64 -m 2048 -smp 2 -monitor
 tcp:0:4014,server,nowait -vnc :14 -name 'ubuntu.test'  -boot
 order=dc,menu=off  -cdrom ubuntu-10.04.1-desktop-amd64.iso -k de
 
 If memtest runs as expected:
 $ git bisect good
 otherwise:
 $ git bisect bad
 
 Keep repeating this and you should end up at the commit that introduced the 
 bug.

this was the outcome of my bisect session:

956a3e6bb7386de48b642d4fee11f7f86a2fcf9a is first bad commit
commit 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a
Author: Blue Swirl blauwir...@gmail.com
Date:   Sat May 22 07:59:01 2010 +

Compile pckbd only once

Use a qemu_irq to indicate A20 line changes. Move I/O port 92
to pckbd.c.

Signed-off-by: Blue Swirl blauwir...@gmail.com

:100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 
1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M  Makefile.objs
:100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 
ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M  Makefile.target
:04 04 dd03f81a42b5162c93c40c517f45eb9f7bece93c 
309f472328632319a15128a59715aa63daf4d92c M  default-configs
:04 04 83201c4fcde2f592a771479246e0a33a8906515b 
b1192bce85f2a7129fb19cf2fe7462ef168165cb M  hw
bisect run success



 
 Stefan

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Qemu-devel] possible regression in qemu-kvm 0.13.0 (memtest)

2010-12-22 Thread Stefan Hajnoczi
On Wed, Dec 22, 2010 at 10:02 AM, Peter Lieven p...@dlh.net wrote:
 If I start a VM with the following parameters
 qemu-kvm-0.13.0 -m 2048 -smp 2 -monitor tcp:0:4014,server,nowait -vnc :14 
 -name 'ubuntu.test'  -boot order=dc,menu=off  -cdrom 
 ubuntu-10.04.1-desktop-amd64.iso -k de

 and select memtest in the Ubuntu CD Boot Menu, the VM immediately resets. 
 After this reset there happen several errors including graphic corruption or 
 the qemu-kvm binary
 aborting with error 134.

 Exactly the same scenario on the same machine with qemu-kvm-0.12.5 works 
 flawlessly.

 Any ideas?

You could track down the commit which broke this using git-bisect(1).
The steps are:

$ git bisect start v0.13.0 v0.12.5

Then:

$ ./configure [...]  make
$ x86_64-softmmu/qemu-system-x86_64 -m 2048 -smp 2 -monitor
tcp:0:4014,server,nowait -vnc :14 -name 'ubuntu.test'  -boot
order=dc,menu=off  -cdrom ubuntu-10.04.1-desktop-amd64.iso -k de

If memtest runs as expected:
$ git bisect good
otherwise:
$ git bisect bad

Keep repeating this and you should end up at the commit that introduced the bug.

Stefan
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html