Linux-m68k Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses
@ 2020-06-24 23:34 Angelo Dureghello
  2020-06-25  8:13 ` Geert Uytterhoeven
  0 siblings, 1 reply; 6+ messages in thread
From: Angelo Dureghello @ 2020-06-24 23:34 UTC (permalink / raw)
  To: gerg; +Cc: linux-m68k, 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses
  2020-06-24 23:34 [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses Angelo Dureghello
@ 2020-06-25  8:13 ` Geert Uytterhoeven
  2020-06-26 20:14   ` Angelo Dureghello
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-06-25  8:13 UTC (permalink / raw)
  To: Angelo Dureghello; +Cc: Greg Ungerer, Linux/m68k

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses
  2020-06-25  8:13 ` Geert Uytterhoeven
@ 2020-06-26 20:14   ` Angelo Dureghello
  2020-07-13 19:41     ` Angelo Dureghello
  0 siblings, 1 reply; 6+ messages in thread
From: Angelo Dureghello @ 2020-06-26 20:14 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Greg Ungerer, Linux/m68k

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses
  2020-06-26 20:14   ` Angelo Dureghello
@ 2020-07-13 19:41     ` Angelo Dureghello
  2020-07-14 13:01       ` Greg Ungerer
  0 siblings, 1 reply; 6+ messages in thread
From: Angelo Dureghello @ 2020-07-13 19:41 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Greg Ungerer, Linux/m68k

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses
  2020-07-13 19:41     ` Angelo Dureghello
@ 2020-07-14 13:01       ` Greg Ungerer
  2020-07-14 15:41         ` Angelo Dureghello
  0 siblings, 1 reply; 6+ messages in thread
From: Greg Ungerer @ 2020-07-14 13:01 UTC (permalink / raw)
  To: Angelo Dureghello, Geert Uytterhoeven; +Cc: Linux/m68k

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses
  2020-07-14 13:01       ` Greg Ungerer
@ 2020-07-14 15:41         ` Angelo Dureghello
  0 siblings, 0 replies; 6+ messages in thread
From: Angelo Dureghello @ 2020-07-14 15:41 UTC (permalink / raw)
  To: Greg Ungerer; +Cc: Geert Uytterhoeven, Linux/m68k

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-24 23:34 [PATCH] m68k: mm: fix wrong m68k_fixup_info addresses Angelo Dureghello
2020-06-25  8:13 ` Geert Uytterhoeven
2020-06-26 20:14   ` Angelo Dureghello
2020-07-13 19:41     ` Angelo Dureghello
2020-07-14 13:01       ` Greg Ungerer
2020-07-14 15:41         ` Angelo Dureghello

Linux-m68k Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-m68k/0 linux-m68k/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-m68k linux-m68k/ https://lore.kernel.org/linux-m68k \
		linux-m68k@vger.kernel.org linux-m68k@lists.linux-m68k.org
	public-inbox-index linux-m68k

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-m68k


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git