[PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[Angelo Dureghello]

On mcf5441x, mmu enabled, using a small initramfs, the boot was
recently hanging silently in the initial phase, inside
arch/m68k/mm/init.c, m68k_setup_node()

initrd at 0x47d2f000:0x47d82f64
overlap at 1073741889 for chunk 0
overlap at 1073746176 for chunk 0
overlap at 1073746736 for chunk 0
overlap at 1073746737 for chunk 0
overlap at 1073746738 for chunk 0
overlap at 1073746739 for chunk 0
overlap at 1073746740 for chunk 0

From some debugging, at m68k_setup_node(), the 25-bits shift value
(virt_to_node_shift) seems unset, because applied to a wrong
address (address previously set from m68k_fixup).

Tried some fixes, but the more low-risk and hopefully correct seems
to just add a volatile inside __virt_to_node_shift(). This seems to
produce correct addresses for the next fixup to work. Cannot test this
on other mmu ColdFire cpu's so every test is eventually welcome.

Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
---
 arch/m68k/include/asm/page_mm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/m68k/include/asm/page_mm.h b/arch/m68k/include/asm/page_mm.h
index e6b75992192b..1eff386cc64b 100644
--- a/arch/m68k/include/asm/page_mm.h
+++ b/arch/m68k/include/asm/page_mm.h
@@ -135,7 +135,7 @@ static inline __attribute_const__ int __virt_to_node_shift(void)
 {
 	int shift;
 
-	asm (
+	asm volatile (
 		"1:	moveq	#0,%0\n"
 		m68k_fixup(%c1, 1b)
 		: "=d" (shift)
-- 
2.26.2

Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[Geert Uytterhoeven]

Hi Angelo,

Thanks for your patch!

On Thu, Jun 25, 2020 at 1:28 AM Angelo Dureghello
<angelo.dureghello@timesys.com> wrote:
> On mcf5441x, mmu enabled, using a small initramfs, the boot was
> recently hanging silently in the initial phase, inside
> arch/m68k/mm/init.c, m68k_setup_node()
>
> initrd at 0x47d2f000:0x47d82f64
> overlap at 1073741889 for chunk 0
> overlap at 1073746176 for chunk 0
> overlap at 1073746736 for chunk 0
> overlap at 1073746737 for chunk 0
> overlap at 1073746738 for chunk 0
> overlap at 1073746739 for chunk 0
> overlap at 1073746740 for chunk 0
>
> From some debugging, at m68k_setup_node(), the 25-bits shift value
> (virt_to_node_shift) seems unset, because applied to a wrong
> address (address previously set from m68k_fixup).
>
> Tried some fixes, but the more low-risk and hopefully correct seems
> to just add a volatile inside __virt_to_node_shift(). This seems to
> produce correct addresses for the next fixup to work. Cannot test this
> on other mmu ColdFire cpu's so every test is eventually welcome.
>
> Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>

> --- a/arch/m68k/include/asm/page_mm.h
> +++ b/arch/m68k/include/asm/page_mm.h
> @@ -135,7 +135,7 @@ static inline __attribute_const__ int __virt_to_node_shift(void)
>  {
>         int shift;
>
> -       asm (
> +       asm volatile (
>                 "1:     moveq   #0,%0\n"
>                 m68k_fixup(%c1, 1b)
>                 : "=d" (shift)

Perhaps we should add the volatile to the other asm statements, too?
Andreas?

Which compiler version are you using?
Have you compared the difference in generated code?

With gcc version 8.4.0 (Ubuntu 8.4.0-1ubuntu1~18.04), the above makes
no difference for m68k_setup_node(), and the compiler doesn't even
optimize away the call to __virt_to_node_shift(), which is marked
__attribute_const__.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[Angelo Dureghello]

Hi Geert !

thanks a lot for the feedbacks.

On Thu, Jun 25, 2020 at 10:13 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Angelo,
>
> Thanks for your patch!
>
> On Thu, Jun 25, 2020 at 1:28 AM Angelo Dureghello
> <angelo.dureghello@timesys.com> wrote:
> > On mcf5441x, mmu enabled, using a small initramfs, the boot was
> > recently hanging silently in the initial phase, inside
> > arch/m68k/mm/init.c, m68k_setup_node()
> >
> > initrd at 0x47d2f000:0x47d82f64
> > overlap at 1073741889 for chunk 0
> > overlap at 1073746176 for chunk 0
> > overlap at 1073746736 for chunk 0
> > overlap at 1073746737 for chunk 0
> > overlap at 1073746738 for chunk 0
> > overlap at 1073746739 for chunk 0
> > overlap at 1073746740 for chunk 0
> >
> > From some debugging, at m68k_setup_node(), the 25-bits shift value
> > (virt_to_node_shift) seems unset, because applied to a wrong
> > address (address previously set from m68k_fixup).
> >
> > Tried some fixes, but the more low-risk and hopefully correct seems
> > to just add a volatile inside __virt_to_node_shift(). This seems to
> > produce correct addresses for the next fixup to work. Cannot test this
> > on other mmu ColdFire cpu's so every test is eventually welcome.
> >
> > Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
>
> > --- a/arch/m68k/include/asm/page_mm.h
> > +++ b/arch/m68k/include/asm/page_mm.h
> > @@ -135,7 +135,7 @@ static inline __attribute_const__ int __virt_to_node_shift(void)
> >  {
> >         int shift;
> >
> > -       asm (
> > +       asm volatile (
> >                 "1:     moveq   #0,%0\n"
> >                 m68k_fixup(%c1, 1b)
> >                 : "=d" (shift)
>
> Perhaps we should add the volatile to the other asm statements, too?
> Andreas?

I see volatile is there in other similar m68k asm code chunks,
it should not hurt.

>
> Which compiler version are you using?

It's a toolchain i prepared up here years ago,
export CROSS_COMPILE=/opt/toolchains/m68k/gcc-5.2.0-nolibc/bin/m68k-linux-

If you have a link for a newer toolchain, happy to upgrade :)

> Have you compared the difference in generated code?
>
> With gcc version 8.4.0 (Ubuntu 8.4.0-1ubuntu1~18.04), the above makes
> no difference for m68k_setup_node(), and the compiler doesn't even
> optimize away the call to __virt_to_node_shift(), which is marked
> __attribute_const__.
>

Well, i only checked the assembly by objdump in object files, right now,
at least looking the m68k_fixup section in both cases, opcodes seems ok.

with volatile:

00000000 <.m68k_fixup>:
   0: 4e71           nop
...
6: R_68K_32 .init.text+0x1e
   a: 4e71           nop
   c: 4e71           nop
   e: 0000           .short 0x0000
  10: 0001           .short 0x0001
  12: 0000           .short 0x0000
12: R_68K_32 .init.text+0x22 correct, points to
                          22: 7200           moveq #0,%d1

  14: 0000           .short 0x0000
  16: 4e71           nop
  18: 4e71           nop
...
1e: R_68K_32 .init.text+0x46
  22: 4e71           nop


without

  00000000 <.m68k_fixup>:
   0: 4e71           nop
   2: 0000           .short 0x0000
   4: 0001           .short 0x0001
   6: 0000           .short 0x0000
6: R_68K_32 .init.text+0x10, corrrect, points to
              10: 7200           moveq #0,%d1

   8: 0000           .short 0x0000
   a: 4e71           nop
   c: 4e71           nop

Do you see anything strange ? Or, how can i objdump better ?

> Gr{oetje,eeting}s,
>
>                         Geert
>

Thanks, regards
angelo

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[Angelo Dureghello]

Hi Geert and all,

tested now building by kernel.org toolchain 10.1.0,
i can boot with or without the above added volatile, jfyi.

Regards,
angelo

On Fri, Jun 26, 2020 at 10:14 PM Angelo Dureghello
<angelo.dureghello@timesys.com> wrote:
>
> Hi Geert !
>
> thanks a lot for the feedbacks.
>
> On Thu, Jun 25, 2020 at 10:13 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> >
> > Hi Angelo,
> >
> > Thanks for your patch!
> >
> > On Thu, Jun 25, 2020 at 1:28 AM Angelo Dureghello
> > <angelo.dureghello@timesys.com> wrote:
> > > On mcf5441x, mmu enabled, using a small initramfs, the boot was
> > > recently hanging silently in the initial phase, inside
> > > arch/m68k/mm/init.c, m68k_setup_node()
> > >
> > > initrd at 0x47d2f000:0x47d82f64
> > > overlap at 1073741889 for chunk 0
> > > overlap at 1073746176 for chunk 0
> > > overlap at 1073746736 for chunk 0
> > > overlap at 1073746737 for chunk 0
> > > overlap at 1073746738 for chunk 0
> > > overlap at 1073746739 for chunk 0
> > > overlap at 1073746740 for chunk 0
> > >
> > > From some debugging, at m68k_setup_node(), the 25-bits shift value
> > > (virt_to_node_shift) seems unset, because applied to a wrong
> > > address (address previously set from m68k_fixup).
> > >
> > > Tried some fixes, but the more low-risk and hopefully correct seems
> > > to just add a volatile inside __virt_to_node_shift(). This seems to
> > > produce correct addresses for the next fixup to work. Cannot test this
> > > on other mmu ColdFire cpu's so every test is eventually welcome.
> > >
> > > Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
> >
> > > --- a/arch/m68k/include/asm/page_mm.h
> > > +++ b/arch/m68k/include/asm/page_mm.h
> > > @@ -135,7 +135,7 @@ static inline __attribute_const__ int __virt_to_node_shift(void)
> > >  {
> > >         int shift;
> > >
> > > -       asm (
> > > +       asm volatile (
> > >                 "1:     moveq   #0,%0\n"
> > >                 m68k_fixup(%c1, 1b)
> > >                 : "=d" (shift)
> >
> > Perhaps we should add the volatile to the other asm statements, too?
> > Andreas?
>
> I see volatile is there in other similar m68k asm code chunks,
> it should not hurt.
>
> >
> > Which compiler version are you using?
>
> It's a toolchain i prepared up here years ago,
> export CROSS_COMPILE=/opt/toolchains/m68k/gcc-5.2.0-nolibc/bin/m68k-linux-
>
> If you have a link for a newer toolchain, happy to upgrade :)
>
> > Have you compared the difference in generated code?
> >
> > With gcc version 8.4.0 (Ubuntu 8.4.0-1ubuntu1~18.04), the above makes
> > no difference for m68k_setup_node(), and the compiler doesn't even
> > optimize away the call to __virt_to_node_shift(), which is marked
> > __attribute_const__.
> >
>
> Well, i only checked the assembly by objdump in object files, right now,
> at least looking the m68k_fixup section in both cases, opcodes seems ok.
>
> with volatile:
>
> 00000000 <.m68k_fixup>:
>    0: 4e71           nop
> ...
> 6: R_68K_32 .init.text+0x1e
>    a: 4e71           nop
>    c: 4e71           nop
>    e: 0000           .short 0x0000
>   10: 0001           .short 0x0001
>   12: 0000           .short 0x0000
> 12: R_68K_32 .init.text+0x22 correct, points to
>                           22: 7200           moveq #0,%d1
>
>   14: 0000           .short 0x0000
>   16: 4e71           nop
>   18: 4e71           nop
> ...
> 1e: R_68K_32 .init.text+0x46
>   22: 4e71           nop
>
>
> without
>
>   00000000 <.m68k_fixup>:
>    0: 4e71           nop
>    2: 0000           .short 0x0000
>    4: 0001           .short 0x0001
>    6: 0000           .short 0x0000
> 6: R_68K_32 .init.text+0x10, corrrect, points to
>               10: 7200           moveq #0,%d1
>
>    8: 0000           .short 0x0000
>    a: 4e71           nop
>    c: 4e71           nop
>
> Do you see anything strange ? Or, how can i objdump better ?
>
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
>
> Thanks, regards
> angelo
>
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                 -- Linus Torvalds



-- 
Angelo Dureghello
Timesys
e. angelo.dureghello@timesys.com

Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[Greg Ungerer]

Hi Angelo,

On 14/7/20 5:41 am, Angelo Dureghello wrote:
> Hi Geert and all,
> 
> tested now building by kernel.org toolchain 10.1.0,
> i can boot with or without the above added volatile, jfyi.

So is the net out that this was a compiler/toolchain problem?

Regards
Greg


> Regards,
> angelo
> 
> On Fri, Jun 26, 2020 at 10:14 PM Angelo Dureghello
> <angelo.dureghello@timesys.com> wrote:
>>
>> Hi Geert !
>>
>> thanks a lot for the feedbacks.
>>
>> On Thu, Jun 25, 2020 at 10:13 AM Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>>
>>> Hi Angelo,
>>>
>>> Thanks for your patch!
>>>
>>> On Thu, Jun 25, 2020 at 1:28 AM Angelo Dureghello
>>> <angelo.dureghello@timesys.com> wrote:
>>>> On mcf5441x, mmu enabled, using a small initramfs, the boot was
>>>> recently hanging silently in the initial phase, inside
>>>> arch/m68k/mm/init.c, m68k_setup_node()
>>>>
>>>> initrd at 0x47d2f000:0x47d82f64
>>>> overlap at 1073741889 for chunk 0
>>>> overlap at 1073746176 for chunk 0
>>>> overlap at 1073746736 for chunk 0
>>>> overlap at 1073746737 for chunk 0
>>>> overlap at 1073746738 for chunk 0
>>>> overlap at 1073746739 for chunk 0
>>>> overlap at 1073746740 for chunk 0
>>>>
>>>>  From some debugging, at m68k_setup_node(), the 25-bits shift value
>>>> (virt_to_node_shift) seems unset, because applied to a wrong
>>>> address (address previously set from m68k_fixup).
>>>>
>>>> Tried some fixes, but the more low-risk and hopefully correct seems
>>>> to just add a volatile inside __virt_to_node_shift(). This seems to
>>>> produce correct addresses for the next fixup to work. Cannot test this
>>>> on other mmu ColdFire cpu's so every test is eventually welcome.
>>>>
>>>> Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
>>>
>>>> --- a/arch/m68k/include/asm/page_mm.h
>>>> +++ b/arch/m68k/include/asm/page_mm.h
>>>> @@ -135,7 +135,7 @@ static inline __attribute_const__ int __virt_to_node_shift(void)
>>>>   {
>>>>          int shift;
>>>>
>>>> -       asm (
>>>> +       asm volatile (
>>>>                  "1:     moveq   #0,%0\n"
>>>>                  m68k_fixup(%c1, 1b)
>>>>                  : "=d" (shift)
>>>
>>> Perhaps we should add the volatile to the other asm statements, too?
>>> Andreas?
>>
>> I see volatile is there in other similar m68k asm code chunks,
>> it should not hurt.
>>
>>>
>>> Which compiler version are you using?
>>
>> It's a toolchain i prepared up here years ago,
>> export CROSS_COMPILE=/opt/toolchains/m68k/gcc-5.2.0-nolibc/bin/m68k-linux-
>>
>> If you have a link for a newer toolchain, happy to upgrade :)
>>
>>> Have you compared the difference in generated code?
>>>
>>> With gcc version 8.4.0 (Ubuntu 8.4.0-1ubuntu1~18.04), the above makes
>>> no difference for m68k_setup_node(), and the compiler doesn't even
>>> optimize away the call to __virt_to_node_shift(), which is marked
>>> __attribute_const__.
>>>
>>
>> Well, i only checked the assembly by objdump in object files, right now,
>> at least looking the m68k_fixup section in both cases, opcodes seems ok.
>>
>> with volatile:
>>
>> 00000000 <.m68k_fixup>:
>>     0: 4e71           nop
>> ...
>> 6: R_68K_32 .init.text+0x1e
>>     a: 4e71           nop
>>     c: 4e71           nop
>>     e: 0000           .short 0x0000
>>    10: 0001           .short 0x0001
>>    12: 0000           .short 0x0000
>> 12: R_68K_32 .init.text+0x22 correct, points to
>>                            22: 7200           moveq #0,%d1
>>
>>    14: 0000           .short 0x0000
>>    16: 4e71           nop
>>    18: 4e71           nop
>> ...
>> 1e: R_68K_32 .init.text+0x46
>>    22: 4e71           nop
>>
>>
>> without
>>
>>    00000000 <.m68k_fixup>:
>>     0: 4e71           nop
>>     2: 0000           .short 0x0000
>>     4: 0001           .short 0x0001
>>     6: 0000           .short 0x0000
>> 6: R_68K_32 .init.text+0x10, corrrect, points to
>>                10: 7200           moveq #0,%d1
>>
>>     8: 0000           .short 0x0000
>>     a: 4e71           nop
>>     c: 4e71           nop
>>
>> Do you see anything strange ? Or, how can i objdump better ?
>>
>>> Gr{oetje,eeting}s,
>>>
>>>                          Geert
>>>
>>
>> Thanks, regards
>> angelo
>>
>>> --
>>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>>>
>>> In personal conversations with technical people, I call myself a hacker. But
>>> when I'm talking to journalists I just say "programmer" or something like that.
>>>                                  -- Linus Torvalds
> 
> 
> 

Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[Angelo Dureghello]

Hi Greg and all,

On Tue, Jul 14, 2020 at 3:01 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
>
> Hi Angelo,
>
> On 14/7/20 5:41 am, Angelo Dureghello wrote:
> > Hi Geert and all,
> >
> > tested now building by kernel.org toolchain 10.1.0,
> > i can boot with or without the above added volatile, jfyi.
>
> So is the net out that this was a compiler/toolchain problem?
>

yes seems so.

This is no more an issue for me, so up to you how to proceed,  anyway,
keeping volatile seems to help in case someone uses older compilers
(i was a 5.2.0 age btw, and toolchain was made by myself).

> Regards
> Greg
>

Regards,
angelo


-- 
Angelo Dureghello
Timesys
e. angelo.dureghello@timesys.com

[PATCH] m68k: mm: fix wrong m68k_fixup_info addresses

[Angelo Dureghello]

After rebasing today with mainline/master, i am hitting
this issue again:

https://www.spinics.net/lists/linux-m68k/msg15881.html

   Loading Ramdisk to 47ccc000, end 47d85544 ... OK
...
initrd at 0x47ccc000:0x47d85544
overlap at 1073741889 for chunk 0
overlap at 1073746200 for chunk 0
overlap at 1073746760 for chunk 0
overlap at 1073746761 for chunk 0
overlap at 1073746762 for chunk 0
overlap at 1073746763 for chunk 0
overlap at 1073746764 for chunk 0

This time i am using gcc-10.1.0-nolibc, but the issue
re-appeared. Add volatile again to solve the issue.

Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
---
 arch/m68k/include/asm/page_mm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/m68k/include/asm/page_mm.h b/arch/m68k/include/asm/page_mm.h
index e6b75992192b..1eff386cc64b 100644
--- a/arch/m68k/include/asm/page_mm.h
+++ b/arch/m68k/include/asm/page_mm.h
@@ -135,7 +135,7 @@ static inline __attribute_const__ int __virt_to_node_shift(void)
 {
 	int shift;
 
-	asm (
+	asm volatile (
 		"1:	moveq	#0,%0\n"
 		m68k_fixup(%c1, 1b)
 		: "=d" (shift)
-- 
2.28.0