[Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Qemu-devel] ppc vga output breakage since commit c3c1bb99

[BALATON Zoltan]

Hello,

Commit c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 (exec: Respect 
as_tranlsate_internal length clamp) seems to break vga output with 
qemu-system-ppc on x86_64 host. Since this commit, the output window does 
not get to the OpenBIOS console but stays black. (Easy to reproduce by 
just starting qemu-system-ppc.)

The reason may be that this change has exposed another bug somewhere else 
but this is all quite obscure to me. Does anyone have any idea what could 
be wrong or where to look further. Any help is appreciated.

Thank you,
BALATON Zoltan

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Mark Cave-Ayland]

On 28/03/15 19:04, BALATON Zoltan wrote:

> Hello,
> 
> Commit c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 (exec: Respect
> as_tranlsate_internal length clamp) seems to break vga output with
> qemu-system-ppc on x86_64 host. Since this commit, the output window
> does not get to the OpenBIOS console but stays black. (Easy to reproduce
> by just starting qemu-system-ppc.)
> 
> The reason may be that this change has exposed another bug somewhere
> else but this is all quite obscure to me. Does anyone have any idea what
> could be wrong or where to look further. Any help is appreciated.

Hi Zoltan,

Yes indeed, this has been picked up in the last few days and is on the
list of issues to be dealt with for -rc2. So watch this space...


ATB,

Mark.

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Paolo Bonzini]

On 28/03/2015 20:19, Mark Cave-Ayland wrote:
> On 28/03/15 19:04, BALATON Zoltan wrote:
> 
>> Hello,
>>
>> Commit c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 (exec: Respect
>> as_tranlsate_internal length clamp) seems to break vga output with
>> qemu-system-ppc on x86_64 host. Since this commit, the output window
>> does not get to the OpenBIOS console but stays black. (Easy to reproduce
>> by just starting qemu-system-ppc.)
>>
>> The reason may be that this change has exposed another bug somewhere
>> else but this is all quite obscure to me. Does anyone have any idea what
>> could be wrong or where to look further. Any help is appreciated.
> 
> Hi Zoltan,
> 
> Yes indeed, this has been picked up in the last few days and is on the
> list of issues to be dealt with for -rc2. So watch this space...

This is an OpenBIOS bug; the port at 0x1ce is 1 byte wide, but OpenBIOS
is accessing it as 2 bytes.

Of course, there's a QEMU regression too and I'm thinking of how to fix it.

Paolo

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Mark Cave-Ayland]

On 30/03/15 10:51, Paolo Bonzini wrote:

> On 28/03/2015 20:19, Mark Cave-Ayland wrote:
>> On 28/03/15 19:04, BALATON Zoltan wrote:
>>
>>> Hello,
>>>
>>> Commit c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 (exec: Respect
>>> as_tranlsate_internal length clamp) seems to break vga output with
>>> qemu-system-ppc on x86_64 host. Since this commit, the output window
>>> does not get to the OpenBIOS console but stays black. (Easy to reproduce
>>> by just starting qemu-system-ppc.)
>>>
>>> The reason may be that this change has exposed another bug somewhere
>>> else but this is all quite obscure to me. Does anyone have any idea what
>>> could be wrong or where to look further. Any help is appreciated.
>>
>> Hi Zoltan,
>>
>> Yes indeed, this has been picked up in the last few days and is on the
>> list of issues to be dealt with for -rc2. So watch this space...
> 
> This is an OpenBIOS bug; the port at 0x1ce is 1 byte wide, but OpenBIOS
> is accessing it as 2 bytes.
> 
> Of course, there's a QEMU regression too and I'm thinking of how to fix it.

Hmmm that's interesting because the documentation refers to both
registers being 16-bit: http://wiki.osdev.org/Bochs_VBE_Extensions. And
indeed the pseudo-code uses outpw/inpw for accesses, even though the
index and data registers are only 1 byte apart (0x1ce and 0x1cf) in I/O
space.

Maybe OpenBIOS is getting the endian conversion incorrect for the word
access? (i.e. it's not converting to little endian).


ATB,

Mark.

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Paolo Bonzini]

On 30/03/2015 12:20, Mark Cave-Ayland wrote:
>> > 
>> > Of course, there's a QEMU regression too and I'm thinking of how to fix it.
> Hmmm that's interesting because the documentation refers to both
> registers being 16-bit: http://wiki.osdev.org/Bochs_VBE_Extensions. And
> indeed the pseudo-code uses outpw/inpw for accesses, even though the
> index and data registers are only 1 byte apart (0x1ce and 0x1cf) in I/O
> space.

Ugh, you're right.

> Maybe OpenBIOS is getting the endian conversion incorrect for the word
> access? (i.e. it's not converting to little endian).

No, that's not it.  It's basically treating the whole access as
"unassigned" because 0x1cf is not allocated (on non-x86 machines, the
DISPI data port is at 0x1d0).

Paolo

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Peter Crosthwaite]

On Mon, Mar 30, 2015 at 8:28 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 30/03/2015 12:20, Mark Cave-Ayland wrote:
>>> >
>>> > Of course, there's a QEMU regression too and I'm thinking of how to fix it.

Can the address_space_translate_address() length clamp be made
conditional on non-MMIO access as the RC fix? I submitted
c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 as I think its the right
thing to do regardless of memory type, but in reality it only fixes a
bug I encountered with RAM memory regions. The original code ignores
address_space_translate_internal() return-by-pointer length value
absolutely and the new code uses it absolutely. Should we just if the
whole thing, old vs new behaviour on MMIO vs non-MMIO?

Happy to submit that fixup if that's the accepted plan.

Regards,
Peter

>> Hmmm that's interesting because the documentation refers to both
>> registers being 16-bit: http://wiki.osdev.org/Bochs_VBE_Extensions. And
>> indeed the pseudo-code uses outpw/inpw for accesses, even though the
>> index and data registers are only 1 byte apart (0x1ce and 0x1cf) in I/O
>> space.
>
> Ugh, you're right.
>
>> Maybe OpenBIOS is getting the endian conversion incorrect for the word
>> access? (i.e. it's not converting to little endian).
>
> No, that's not it.  It's basically treating the whole access as
> "unassigned" because 0x1cf is not allocated (on non-x86 machines, the
> DISPI data port is at 0x1d0).
>
> Paolo
>

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Paolo Bonzini]

On 30/03/2015 13:45, Peter Crosthwaite wrote:
> Can the address_space_translate_address() length clamp be made
> conditional on non-MMIO access as the RC fix? I submitted
> c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 as I think its the right
> thing to do regardless of memory type, but in reality it only fixes a
> bug I encountered with RAM memory regions. The original code ignores
> address_space_translate_internal() return-by-pointer length value
> absolutely and the new code uses it absolutely. Should we just if the
> whole thing, old vs new behaviour on MMIO vs non-MMIO?
> 
> Happy to submit that fixup if that's the accepted plan.

I submitted what I think is the right fix (sorry Mark for misspelling
your email address).  I think you're patch is correct, so I'd rather not
introduce hacks for the release; we can either revert it, or fix ioport.c.

Paolo

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Mark Cave-Ayland]

On 30/03/15 12:47, Paolo Bonzini wrote:

> On 30/03/2015 13:45, Peter Crosthwaite wrote:
>> Can the address_space_translate_address() length clamp be made
>> conditional on non-MMIO access as the RC fix? I submitted
>> c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 as I think its the right
>> thing to do regardless of memory type, but in reality it only fixes a
>> bug I encountered with RAM memory regions. The original code ignores
>> address_space_translate_internal() return-by-pointer length value
>> absolutely and the new code uses it absolutely. Should we just if the
>> whole thing, old vs new behaviour on MMIO vs non-MMIO?
>>
>> Happy to submit that fixup if that's the accepted plan.
> 
> I submitted what I think is the right fix (sorry Mark for misspelling
> your email address).  I think you're patch is correct, so I'd rather not
> introduce hacks for the release; we can either revert it, or fix ioport.c.

Ah wait a second - the same patch c3c1bb99 also breaks reset on SPARC32:

$ ./qemu-system-sparc -prom-env 'auto-boot?=false' -nographic
Configuration device id QEMU version 1 machine id 32
Probing SBus slot 0 offset 0
Probing SBus slot 1 offset 0
Probing SBus slot 2 offset 0
Probing SBus slot 3 offset 0
Probing SBus slot 4 offset 0
Probing SBus slot 5 offset 0
Invalid FCode start byte
CPUs: 1 x FMI,MB86904
UUID: 00000000-0000-0000-0000-000000000000
Welcome to OpenBIOS v1.1 built on Mar 12 2015 08:08
  Type 'help' for detailed information

0 > reset-all Unhandled Exception 0x00000029
PC = 0xffd10c08 NPC = 0xffd10c0c
Stopping execution

The reset is controlled by writing a 1 to the hardware reset register at
physical address 0x71f00000 (system-control according to "info mtree")
which is handled in hw/misc/slavio_misc.c.

Given that SYSCTRL_SIZE isn't used at all (and the access size for
slavio_led_mem_ops also looks wrong), the following seems to be the
correct fix for the reset and LED access:


--- a/hw/misc/slavio_misc.c
+++ b/hw/misc/slavio_misc.c
@@ -68,6 +68,7 @@ typedef struct APCState {
 } APCState;

 #define MISC_SIZE 1
+#define LED_SIZE 2
 #define SYSCTRL_SIZE 4

 #define AUX1_TC        0x02
@@ -452,13 +453,13 @@ static int slavio_misc_init1(SysBusDevice *sbd)
     /* 16 bit registers */
     /* ss600mp diag LEDs */
     memory_region_init_io(&s->led_iomem, OBJECT(s), &slavio_led_mem_ops, s,
-                          "leds", MISC_SIZE);
+                          "leds", LED_SIZE);
     sysbus_init_mmio(sbd, &s->led_iomem);

     /* 32 bit registers */
     /* System control */
     memory_region_init_io(&s->sysctrl_iomem, OBJECT(s),
&slavio_sysctrl_mem_ops, s,
-                          "system-control", MISC_SIZE);
+                          "system-control", SYSCTRL_SIZE);
     sysbus_init_mmio(sbd, &s->sysctrl_iomem);

     /* AUX 1 (Misc System Functions) */


Then again it is getting quite late in the release cycle...


ATB,

Mark.

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Paolo Bonzini]

On 01/04/2015 00:34, Mark Cave-Ayland wrote:
> On 30/03/15 12:47, Paolo Bonzini wrote:
> 
>> On 30/03/2015 13:45, Peter Crosthwaite wrote:
>>> Can the address_space_translate_address() length clamp be made
>>> conditional on non-MMIO access as the RC fix? I submitted
>>> c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 as I think its the right
>>> thing to do regardless of memory type, but in reality it only fixes a
>>> bug I encountered with RAM memory regions. The original code ignores
>>> address_space_translate_internal() return-by-pointer length value
>>> absolutely and the new code uses it absolutely. Should we just if the
>>> whole thing, old vs new behaviour on MMIO vs non-MMIO?
>>>
>>> Happy to submit that fixup if that's the accepted plan.
>>
>> I submitted what I think is the right fix (sorry Mark for misspelling
>> your email address).  I think you're patch is correct, so I'd rather not
>> introduce hacks for the release; we can either revert it, or fix ioport.c.
> 
> Ah wait a second - the same patch c3c1bb99 also breaks reset on SPARC32:
> 
> $ ./qemu-system-sparc -prom-env 'auto-boot?=false' -nographic
> Configuration device id QEMU version 1 machine id 32
> Probing SBus slot 0 offset 0
> Probing SBus slot 1 offset 0
> Probing SBus slot 2 offset 0
> Probing SBus slot 3 offset 0
> Probing SBus slot 4 offset 0
> Probing SBus slot 5 offset 0
> Invalid FCode start byte
> CPUs: 1 x FMI,MB86904
> UUID: 00000000-0000-0000-0000-000000000000
> Welcome to OpenBIOS v1.1 built on Mar 12 2015 08:08
>   Type 'help' for detailed information
> 
> 0 > reset-all Unhandled Exception 0x00000029
> PC = 0xffd10c08 NPC = 0xffd10c0c
> Stopping execution
> 
> The reset is controlled by writing a 1 to the hardware reset register at
> physical address 0x71f00000 (system-control according to "info mtree")
> which is handled in hw/misc/slavio_misc.c.
> 
> Given that SYSCTRL_SIZE isn't used at all (and the access size for
> slavio_led_mem_ops also looks wrong), the following seems to be the
> correct fix for the reset and LED access:
> 
> 
> --- a/hw/misc/slavio_misc.c
> +++ b/hw/misc/slavio_misc.c
> @@ -68,6 +68,7 @@ typedef struct APCState {
>  } APCState;
> 
>  #define MISC_SIZE 1
> +#define LED_SIZE 2
>  #define SYSCTRL_SIZE 4
> 
>  #define AUX1_TC        0x02
> @@ -452,13 +453,13 @@ static int slavio_misc_init1(SysBusDevice *sbd)
>      /* 16 bit registers */
>      /* ss600mp diag LEDs */
>      memory_region_init_io(&s->led_iomem, OBJECT(s), &slavio_led_mem_ops, s,
> -                          "leds", MISC_SIZE);
> +                          "leds", LED_SIZE);
>      sysbus_init_mmio(sbd, &s->led_iomem);
> 
>      /* 32 bit registers */
>      /* System control */
>      memory_region_init_io(&s->sysctrl_iomem, OBJECT(s),
> &slavio_sysctrl_mem_ops, s,
> -                          "system-control", MISC_SIZE);
> +                          "system-control", SYSCTRL_SIZE);
>      sysbus_init_mmio(sbd, &s->sysctrl_iomem);
> 
>      /* AUX 1 (Misc System Functions) */
> 
> 
> Then again it is getting quite late in the release cycle...

Yes, it's better to revert for now.  Can you submit the patch above, so
I include it in 2.4 and keep bisectability?

Paolo

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Mark Cave-Ayland]

On 01/04/15 08:55, Paolo Bonzini wrote:

> On 01/04/2015 00:34, Mark Cave-Ayland wrote:
>> On 30/03/15 12:47, Paolo Bonzini wrote:
>>
>>> On 30/03/2015 13:45, Peter Crosthwaite wrote:
>>>> Can the address_space_translate_address() length clamp be made
>>>> conditional on non-MMIO access as the RC fix? I submitted
>>>> c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 as I think its the right
>>>> thing to do regardless of memory type, but in reality it only fixes a
>>>> bug I encountered with RAM memory regions. The original code ignores
>>>> address_space_translate_internal() return-by-pointer length value
>>>> absolutely and the new code uses it absolutely. Should we just if the
>>>> whole thing, old vs new behaviour on MMIO vs non-MMIO?
>>>>
>>>> Happy to submit that fixup if that's the accepted plan.
>>>
>>> I submitted what I think is the right fix (sorry Mark for misspelling
>>> your email address).  I think you're patch is correct, so I'd rather not
>>> introduce hacks for the release; we can either revert it, or fix ioport.c.
>>
>> Ah wait a second - the same patch c3c1bb99 also breaks reset on SPARC32:
>>
>> $ ./qemu-system-sparc -prom-env 'auto-boot?=false' -nographic
>> Configuration device id QEMU version 1 machine id 32
>> Probing SBus slot 0 offset 0
>> Probing SBus slot 1 offset 0
>> Probing SBus slot 2 offset 0
>> Probing SBus slot 3 offset 0
>> Probing SBus slot 4 offset 0
>> Probing SBus slot 5 offset 0
>> Invalid FCode start byte
>> CPUs: 1 x FMI,MB86904
>> UUID: 00000000-0000-0000-0000-000000000000
>> Welcome to OpenBIOS v1.1 built on Mar 12 2015 08:08
>>   Type 'help' for detailed information
>>
>> 0 > reset-all Unhandled Exception 0x00000029
>> PC = 0xffd10c08 NPC = 0xffd10c0c
>> Stopping execution
>>
>> The reset is controlled by writing a 1 to the hardware reset register at
>> physical address 0x71f00000 (system-control according to "info mtree")
>> which is handled in hw/misc/slavio_misc.c.
>>
>> Given that SYSCTRL_SIZE isn't used at all (and the access size for
>> slavio_led_mem_ops also looks wrong), the following seems to be the
>> correct fix for the reset and LED access:
>>
>>
>> --- a/hw/misc/slavio_misc.c
>> +++ b/hw/misc/slavio_misc.c
>> @@ -68,6 +68,7 @@ typedef struct APCState {
>>  } APCState;
>>
>>  #define MISC_SIZE 1
>> +#define LED_SIZE 2
>>  #define SYSCTRL_SIZE 4
>>
>>  #define AUX1_TC        0x02
>> @@ -452,13 +453,13 @@ static int slavio_misc_init1(SysBusDevice *sbd)
>>      /* 16 bit registers */
>>      /* ss600mp diag LEDs */
>>      memory_region_init_io(&s->led_iomem, OBJECT(s), &slavio_led_mem_ops, s,
>> -                          "leds", MISC_SIZE);
>> +                          "leds", LED_SIZE);
>>      sysbus_init_mmio(sbd, &s->led_iomem);
>>
>>      /* 32 bit registers */
>>      /* System control */
>>      memory_region_init_io(&s->sysctrl_iomem, OBJECT(s),
>> &slavio_sysctrl_mem_ops, s,
>> -                          "system-control", MISC_SIZE);
>> +                          "system-control", SYSCTRL_SIZE);
>>      sysbus_init_mmio(sbd, &s->sysctrl_iomem);
>>
>>      /* AUX 1 (Misc System Functions) */
>>
>>
>> Then again it is getting quite late in the release cycle...
> 
> Yes, it's better to revert for now.  Can you submit the patch above, so
> I include it in 2.4 and keep bisectability?

Sure. Is your current 2.4 branch available on github or anywhere
similar? That's just so once the patch has been applied, I have a remote
I can use to run a complete set of OpenBIOS tests.


ATB,

Mark.

Re: [Qemu-devel] ppc vga output breakage since commit c3c1bb99

[Paolo Bonzini]

On 02/04/2015 13:45, Mark Cave-Ayland wrote:
> > Yes, it's better to revert for now.  Can you submit the patch above, so
> > I include it in 2.4 and keep bisectability?
>
> Sure. Is your current 2.4 branch available on github or anywhere
> similar? That's just so once the patch has been applied, I have a remote
> I can use to run a complete set of OpenBIOS tests.

I placed it in branch "atomic-dirty" of
git://github.com/bonzini/qemu.git - for now it's only compile tested,
but I'll get to it tomorrow.  Thanks!

Paolo