hw/mips/malta: Fix the malta machine on big endian hosts

[PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Thomas Huth 1 year, 1 month ago

Booting a Linux kernel with the malta machine is currently broken
on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
for little endian targets only, but uses the wrong way to do this:
cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
it by using the same ways on both, big and little endian hosts.

Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR registers")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 I've checked that both, the kernel from
 https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/mipsel.tgz
 and the kernel from
 https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/mips.tgz
 now boot fine on both, a little endian (x86) and a big endian (s390x) host.

 hw/mips/malta.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index af9021316d..b26ed1fc9a 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -629,9 +629,9 @@ static void bl_setup_gt64120_jump_kernel(void **p, uint64_t run_addr,
 
     /* Bus endianess is always reversed */
 #if TARGET_BIG_ENDIAN
-#define cpu_to_gt32 cpu_to_le32
+#define cpu_to_gt32(x) (x)
 #else
-#define cpu_to_gt32 cpu_to_be32
+#define cpu_to_gt32(x) bswap32(x)
 #endif
 
     /* setup MEM-to-PCI0 mapping as done by YAMON */
-- 
2.31.1

Re: [PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Michael Tokarev 1 year, 1 month ago

30.03.2023 18:26, Thomas Huth wrote:
> Booting a Linux kernel with the malta machine is currently broken
> on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
> for little endian targets only, but uses the wrong way to do this:
> cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
> it by using the same ways on both, big and little endian hosts.
> 
> Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR registers")
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Has this been forgotten?

Thanks,

/mjt

Re: [PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Peter Maydell 1 year, 1 month ago

On Thu, 13 Apr 2023 at 17:08, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 30.03.2023 18:26, Thomas Huth wrote:
> > Booting a Linux kernel with the malta machine is currently broken
> > on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
> > for little endian targets only, but uses the wrong way to do this:
> > cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
> > it by using the same ways on both, big and little endian hosts.
> >
> > Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR registers")
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
>
> Has this been forgotten?

Looks like it. Too late for 8.0 now (and it wasn't a regression
since it looks like it was broken in 7.2 as well); will have to
be fixed in 8.1.

thanks
-- PMM

Re: [PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Peter Maydell 1 year ago

On Thu, 13 Apr 2023 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 13 Apr 2023 at 17:08, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >
> > 30.03.2023 18:26, Thomas Huth wrote:
> > > Booting a Linux kernel with the malta machine is currently broken
> > > on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
> > > for little endian targets only, but uses the wrong way to do this:
> > > cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
> > > it by using the same ways on both, big and little endian hosts.
> > >
> > > Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR registers")
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> >
> > Has this been forgotten?
>
> Looks like it. Too late for 8.0 now (and it wasn't a regression
> since it looks like it was broken in 7.2 as well); will have to
> be fixed in 8.1.

Philippe -- looks like this patch still hasn't been queued ?
(It could probably use a Cc: qemu-stable@nongnu.org at this point.)

It can have my
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Thomas Huth 11 months, 3 weeks ago

On 09/05/2023 20.44, Peter Maydell wrote:
> On Thu, 13 Apr 2023 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Thu, 13 Apr 2023 at 17:08, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>>
>>> 30.03.2023 18:26, Thomas Huth wrote:
>>>> Booting a Linux kernel with the malta machine is currently broken
>>>> on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
>>>> for little endian targets only, but uses the wrong way to do this:
>>>> cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
>>>> it by using the same ways on both, big and little endian hosts.
>>>>
>>>> Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR registers")
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Has this been forgotten?
>>
>> Looks like it. Too late for 8.0 now (and it wasn't a regression
>> since it looks like it was broken in 7.2 as well); will have to
>> be fixed in 8.1.
> 
> Philippe -- looks like this patch still hasn't been queued ?
> (It could probably use a Cc: qemu-stable@nongnu.org at this point.)
> 
> It can have my
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

*ping*

Philippe, can you please comment? I think this should be good enough at 
least for a temporary fix, even if you have more clean ups in this area in 
mind later...

  Thomas

Re: [PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Thomas Huth 11 months, 2 weeks ago

On 31/05/2023 09.13, Thomas Huth wrote:
> On 09/05/2023 20.44, Peter Maydell wrote:
>> On Thu, 13 Apr 2023 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> On Thu, 13 Apr 2023 at 17:08, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>>>
>>>> 30.03.2023 18:26, Thomas Huth wrote:
>>>>> Booting a Linux kernel with the malta machine is currently broken
>>>>> on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
>>>>> for little endian targets only, but uses the wrong way to do this:
>>>>> cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
>>>>> it by using the same ways on both, big and little endian hosts.
>>>>>
>>>>> Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR 
>>>>> registers")
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>
>>>> Has this been forgotten?
>>>
>>> Looks like it. Too late for 8.0 now (and it wasn't a regression
>>> since it looks like it was broken in 7.2 as well); will have to
>>> be fixed in 8.1.
>>
>> Philippe -- looks like this patch still hasn't been queued ?
>> (It could probably use a Cc: qemu-stable@nongnu.org at this point.)
>>
>> It can have my
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> *ping*
> 
> Philippe, can you please comment? I think this should be good enough at 
> least for a temporary fix, even if you have more clean ups in this area in 
> mind later...

Philippe, if you don't mind, I'll take this through my s390x tree since this 
fixes a problem on the big-endian s390x hosts.

  Thomas

Re: [PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Peter Maydell 1 year, 1 month ago

On Thu, 30 Mar 2023 at 16:27, Thomas Huth <thuth@redhat.com> wrote:
>
> Booting a Linux kernel with the malta machine is currently broken
> on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
> for little endian targets only, but uses the wrong way to do this:
> cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
> it by using the same ways on both, big and little endian hosts.
>
> Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR registers")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  I've checked that both, the kernel from
>  https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/mipsel.tgz
>  and the kernel from
>  https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/mips.tgz
>  now boot fine on both, a little endian (x86) and a big endian (s390x) host.
>
>  hw/mips/malta.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index af9021316d..b26ed1fc9a 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -629,9 +629,9 @@ static void bl_setup_gt64120_jump_kernel(void **p, uint64_t run_addr,
>
>      /* Bus endianess is always reversed */
>  #if TARGET_BIG_ENDIAN
> -#define cpu_to_gt32 cpu_to_le32
> +#define cpu_to_gt32(x) (x)
>  #else
> -#define cpu_to_gt32 cpu_to_be32
> +#define cpu_to_gt32(x) bswap32(x)
>  #endif

So if we:
 * do nothing to the value on a BE host
 * swap the value on an LE host

isn't that the same as cpu_to_be32() in both cases?

thanks
-- PMM

Re: [PATCH] hw/mips/malta: Fix the malta machine on big endian hosts

Posted by Thomas Huth 1 year, 1 month ago

On 30/03/2023 17.33, Peter Maydell wrote:
> On Thu, 30 Mar 2023 at 16:27, Thomas Huth <thuth@redhat.com> wrote:
>>
>> Booting a Linux kernel with the malta machine is currently broken
>> on big endian hosts. The cpu_to_gt32 macro wants to byteswap a value
>> for little endian targets only, but uses the wrong way to do this:
>> cpu_to_[lb]e32 works the other way round on big endian hosts! Fix
>> it by using the same ways on both, big and little endian hosts.
>>
>> Fixes: 0c8427baf0 ("hw/mips/malta: Use bootloader helper to set BAR registers")
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   I've checked that both, the kernel from
>>   https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/mipsel.tgz
>>   and the kernel from
>>   https://landley.net/toybox/downloads/binaries/mkroot/0.8.9/mips.tgz
>>   now boot fine on both, a little endian (x86) and a big endian (s390x) host.
>>
>>   hw/mips/malta.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>> index af9021316d..b26ed1fc9a 100644
>> --- a/hw/mips/malta.c
>> +++ b/hw/mips/malta.c
>> @@ -629,9 +629,9 @@ static void bl_setup_gt64120_jump_kernel(void **p, uint64_t run_addr,
>>
>>       /* Bus endianess is always reversed */
>>   #if TARGET_BIG_ENDIAN
>> -#define cpu_to_gt32 cpu_to_le32
>> +#define cpu_to_gt32(x) (x)
>>   #else
>> -#define cpu_to_gt32 cpu_to_be32
>> +#define cpu_to_gt32(x) bswap32(x)
>>   #endif
> 
> So if we:
>   * do nothing to the value on a BE host
>   * swap the value on an LE host
> 
> isn't that the same as cpu_to_be32() in both cases?

No, it's about the *target*, not the host:

* do nothing to the value for a BE *target*
* swap the value for LE *targets*

It's quite weird and it also took me a while to understand this, but this 
seems to be the way it's working right with all combinations.

  Thomas