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

login
register
mail settings
Submitter Peter Lieven
Date Dec. 26, 2010, 9:21 p.m.
Message ID <CF25E7B4-C481-4F95-8847-29417C809985@dlh.net>
Download mbox | patch
Permalink /patch/76725/
State New
Headers show

Comments

Peter Lieven - Dec. 26, 2010, 9:21 p.m.
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

---
Stefan Hajnoczi - Dec. 27, 2010, 3:51 a.m.
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
Peter Lieven - Dec. 27, 2010, 7:59 a.m.
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
Peter Lieven - Dec. 28, 2010, 12:38 p.m.
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
Serge E. Hallyn - Jan. 5, 2011, 5:01 p.m.
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
Stefan Hajnoczi - Jan. 6, 2011, 11:24 a.m.
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

Patch

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