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

[Peter Lieven]

Am 27.12.2010 um 04:51 schrieb Stefan Hajnoczi:

> On Sun, Dec 26, 2010 at 9:21 PM, Peter Lieven <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>> 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 <address@hidden>
>>> 
>>> :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