| Message ID | CF25E7B4-C481-4F95-8847-29417C809985@dlh.net |
|---|---|
| State | New |
| Headers | show |
On Sun, Dec 26, 2010 at 9:21 PM, Peter Lieven <pl@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 <pl@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 <blauwirbel@gmail.com> >> Date: Sat May 22 07:59:01 2010 +0000 >> >> 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 <blauwirbel@gmail.com> >> >> :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M Makefile.objs >> :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M Makefile.target >> :040000 040000 dd03f81a42b5162c93c40c517f45eb9f7bece93c 309f472328632319a15128a59715aa63daf4d92c M default-configs >> :040000 040000 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.000000000 +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 different. Fudging things just makes it tricky to understand. Stefan
Am 27.12.2010 um 04:51 schrieb Stefan Hajnoczi: > On Sun, Dec 26, 2010 at 9:21 PM, Peter Lieven <pl@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 <pl@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 <blauwirbel@gmail.com> >>> Date: Sat May 22 07:59:01 2010 +0000 >>> >>> 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 <blauwirbel@gmail.com> >>> >>> :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M Makefile.objs >>> :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M Makefile.target >>> :040000 040000 dd03f81a42b5162c93c40c517f45eb9f7bece93c 309f472328632319a15128a59715aa63daf4d92c M default-configs >>> :040000 040000 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.000000000 +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 different. > Fudging things just makes it tricky to understand. I agree, but in this case the ioport92 stuff should be moved back to hw/pc.c. Question: Does any other hardware than PC have an ioport 0x92? And does any other hardware have this A20 pain? Peter > > Stefan
On 27.12.2010 04:51, Stefan Hajnoczi wrote: > On Sun, Dec 26, 2010 at 9:21 PM, Peter Lieven<pl@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<pl@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<blauwirbel@gmail.com> >>> Date: Sat May 22 07:59:01 2010 +0000 >>> >>> 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<blauwirbel@gmail.com> >>> >>> :100644 100644 acbaf227455f931f3ef6dbe0bb4494c6b41f2cd9 1a33d4eb4a5624c55896871b5f4ecde78a49ff28 M Makefile.objs >>> :100644 100644 a22484e1e98355a35deeb5038a45fb8fe8685a91 ba5147fbc48e4faef072a5be6b0d69d3201c1e18 M Makefile.target >>> :040000 040000 dd03f81a42b5162c93c40c517f45eb9f7bece93c 309f472328632319a15128a59715aa63daf4d92c M default-configs >>> :040000 040000 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.000000000 +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 different. > Fudging things just makes it tricky to understand. what do you suggest? reverting the patch in total? peter > Stefan
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
On Wed, Jan 5, 2011 at 5:01 PM, Serge E. Hallyn <serge@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
--- qemu-0.13.0/hw/pckbd.c 2010-10-15 22:56:09.000000000 +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);