[PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Victor Hassan]

earlycon uses fixmap to create a memory map,
So we need to close earlycon before closing fixmap,
otherwise printk will access illegal addresses.
After creating a new memory map, we open earlycon again.

Signed-off-by: Victor Hassan <victor@allwinnertech.com>
---
 arch/arm/mm/mmu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 274e4f73fd33..f3511f07a7d0 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -14,6 +14,7 @@
 #include <linux/fs.h>
 #include <linux/vmalloc.h>
 #include <linux/sizes.h>
+#include <linux/console.h>
 
 #include <asm/cp15.h>
 #include <asm/cputype.h>
@@ -1695,6 +1696,9 @@ static void __init early_fixmap_shutdown(void)
 	pmd_clear(fixmap_pmd(va));
 	local_flush_tlb_kernel_page(va);
 
+#ifdef CONFIG_FIX_EARLYCON_MEM
+	console_stop(console_drivers);
+#endif
 	for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
 		pte_t *pte;
 		struct map_desc map;
@@ -1713,6 +1717,9 @@ static void __init early_fixmap_shutdown(void)
 
 		create_mapping(&map);
 	}
+#ifdef CONFIG_FIX_EARLYCON_MEM
+	console_start(console_drivers);
+#endif
 }
 
 /*
-- 
2.29.0

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Linus Walleij]

On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan <victor@allwinnertech.com> wrote:

> earlycon uses fixmap to create a memory map,
> So we need to close earlycon before closing fixmap,
> otherwise printk will access illegal addresses.
> After creating a new memory map, we open earlycon again.
>
> Signed-off-by: Victor Hassan <victor@allwinnertech.com>

I think noone really noticed this because everyone on Arm systems
use CONFIG_DEBUG_LL, and that makes printascii hammer out
stuff on the console very early, it even accounts for whether we have
MMU on or not.

How are you using this on Arm even? What system and what serial
driver?

That said, it looks correct.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Victor Hassan]

On 18/04/2022 07:21, Linus Walleij wrote:
> On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan <victor@allwinnertech.com> wrote:
> 
>> earlycon uses fixmap to create a memory map,
>> So we need to close earlycon before closing fixmap,
>> otherwise printk will access illegal addresses.
>> After creating a new memory map, we open earlycon again.
>>
>> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
> 
> I think noone really noticed this because everyone on Arm systems
> use CONFIG_DEBUG_LL, and that makes printascii hammer out
> stuff on the console very early, it even accounts for whether we have
> MMU on or not.
> 
Hi Linus,
Thank you for the reply. I used earlycon, in early_fixmap_shutdown, the 
base address of earlycon is in the critical stage of release and 
reassignment, so early_fixmap_shutdown -> create_mapping should not call 
earlycon in this process, and create_mapping has a lot of conditions 
that trigger print, memblock=debug just makes it easier to expose problems.

> How are you using this on Arm even? What system and what serial
> driver?
I'm using serial driver 8250 on arm32, with cmdline: memblock=debug.
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8520_CONSOLE=y
> 
> That said, it looks correct.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
Thank you.
> 
> Yours,
> Linus Walleij

Sincerely,
Victor Hassan

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Victor Hassan]

On 4/18/2022 11:08 PM, Victor Hassan wrote:
> On 18/04/2022 07:21, Linus Walleij wrote:
>> On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan 
>> <victor@allwinnertech.com> wrote:
>>
>>> earlycon uses fixmap to create a memory map,
>>> So we need to close earlycon before closing fixmap,
>>> otherwise printk will access illegal addresses.
>>> After creating a new memory map, we open earlycon again.
>>>
>>> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
>>
>> I think noone really noticed this because everyone on Arm systems
>> use CONFIG_DEBUG_LL, and that makes printascii hammer out
>> stuff on the console very early, it even accounts for whether we have
>> MMU on or not.
>>
> Hi Linus,
> Thank you for the reply. I used earlycon, in early_fixmap_shutdown, the 
> base address of earlycon is in the critical stage of release and 
> reassignment, so early_fixmap_shutdown -> create_mapping should not call 
> earlycon in this process, and create_mapping has a lot of conditions 
> that trigger print, memblock=debug just makes it easier to expose problems.
> 
>> How are you using this on Arm even? What system and what serial
>> driver?
> I'm using serial driver 8250 on arm32, with cmdline: memblock=debug.
> CONFIG_SERIAL_8250=y
> CONFIG_SERIAL_8520_CONSOLE=y
>>
>> That said, it looks correct.
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Hi Linus,
Sorry to disturb. Is there any question about this issue? Thank you :)

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Linus Walleij]

On Fri, Jun 17, 2022 at 3:31 PM Victor Hassan <victor@allwinnertech.com> wrote:
> On 4/18/2022 11:08 PM, Victor Hassan wrote:
> > On 18/04/2022 07:21, Linus Walleij wrote:
> >> On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan
> >> <victor@allwinnertech.com> wrote:
> >>
> >>> earlycon uses fixmap to create a memory map,
> >>> So we need to close earlycon before closing fixmap,
> >>> otherwise printk will access illegal addresses.
> >>> After creating a new memory map, we open earlycon again.
> >>>
> >>> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
> >>
> >> I think noone really noticed this because everyone on Arm systems
> >> use CONFIG_DEBUG_LL, and that makes printascii hammer out
> >> stuff on the console very early, it even accounts for whether we have
> >> MMU on or not.
> >>
> > Hi Linus,
> > Thank you for the reply. I used earlycon, in early_fixmap_shutdown, the
> > base address of earlycon is in the critical stage of release and
> > reassignment, so early_fixmap_shutdown -> create_mapping should not call
> > earlycon in this process, and create_mapping has a lot of conditions
> > that trigger print, memblock=debug just makes it easier to expose problems.
> >
> >> How are you using this on Arm even? What system and what serial
> >> driver?
> > I'm using serial driver 8250 on arm32, with cmdline: memblock=debug.
> > CONFIG_SERIAL_8250=y
> > CONFIG_SERIAL_8520_CONSOLE=y
> >>
> >> That said, it looks correct.
> >> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> Hi Linus,
> Sorry to disturb. Is there any question about this issue? Thank you :)

No, I understand the problem, I provided my ACK.

Maybe you wanna add the patch to Russell's patch tracker so
he can apply it?
https://www.arm.linux.org.uk/developer/patches/

Yours,
Linus Walleij

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Marek Szyprowski]

Hi Victor,

On 16.03.2022 03:33, Victor Hassan wrote:
> earlycon uses fixmap to create a memory map,
> So we need to close earlycon before closing fixmap,
> otherwise printk will access illegal addresses.
> After creating a new memory map, we open earlycon again.
>
> Signed-off-by: Victor Hassan <victor@allwinnertech.com>

This patch landed in linux next-20220831 as commit a76886d117cb ("ARM: 
9223/1: mmu: fix access to illegal address when using earlycon & 
memblock=debug"). Unfortunately it breaks booting of all my test boards 
which *do not* use earlycon. It can be easily reproduced even with QEMU.

With kernel compiled from multi_v7_defconfig the following setup boots:

$ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append 
"console=ttyAMA0 earlycon" -M virt -smp 2 -m 512

while this one doesn't:

$ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append 
"console=ttyAMA0" -M virt -smp 2 -m 512


> ---
>   arch/arm/mm/mmu.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 274e4f73fd33..f3511f07a7d0 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -14,6 +14,7 @@
>   #include <linux/fs.h>
>   #include <linux/vmalloc.h>
>   #include <linux/sizes.h>
> +#include <linux/console.h>
>   
>   #include <asm/cp15.h>
>   #include <asm/cputype.h>
> @@ -1695,6 +1696,9 @@ static void __init early_fixmap_shutdown(void)
>   	pmd_clear(fixmap_pmd(va));
>   	local_flush_tlb_kernel_page(va);
>   
> +#ifdef CONFIG_FIX_EARLYCON_MEM
> +	console_stop(console_drivers);
> +#endif
>   	for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
>   		pte_t *pte;
>   		struct map_desc map;
> @@ -1713,6 +1717,9 @@ static void __init early_fixmap_shutdown(void)
>   
>   		create_mapping(&map);
>   	}
> +#ifdef CONFIG_FIX_EARLYCON_MEM
> +	console_start(console_drivers);
> +#endif
>   }
>   
>   /*

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Victor Hassan]

On 8/31/2022 7:52 PM, Marek Szyprowski wrote:
> Hi Victor,
> 
> On 16.03.2022 03:33, Victor Hassan wrote:
>> earlycon uses fixmap to create a memory map,
>> So we need to close earlycon before closing fixmap,
>> otherwise printk will access illegal addresses.
>> After creating a new memory map, we open earlycon again.
>>
>> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
> 
> This patch landed in linux next-20220831 as commit a76886d117cb ("ARM:
> 9223/1: mmu: fix access to illegal address when using earlycon &
> memblock=debug"). Unfortunately it breaks booting of all my test boards
> which *do not* use earlycon. It can be easily reproduced even with QEMU.
> 
> With kernel compiled from multi_v7_defconfig the following setup boots:
> 
> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
> "console=ttyAMA0 earlycon" -M virt -smp 2 -m 512
> 
> while this one doesn't:
> 
> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
> "console=ttyAMA0" -M virt -smp 2 -m 512
> 
> 
>> ---
>>    arch/arm/mm/mmu.c | 7 +++++++
>>    1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index 274e4f73fd33..f3511f07a7d0 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -14,6 +14,7 @@
>>    #include <linux/fs.h>
>>    #include <linux/vmalloc.h>
>>    #include <linux/sizes.h>
>> +#include <linux/console.h>
>>    
>>    #include <asm/cp15.h>
>>    #include <asm/cputype.h>
>> @@ -1695,6 +1696,9 @@ static void __init early_fixmap_shutdown(void)
>>    	pmd_clear(fixmap_pmd(va));
>>    	local_flush_tlb_kernel_page(va);
>>    
>> +#ifdef CONFIG_FIX_EARLYCON_MEM
>> +	console_stop(console_drivers);
>> +#endif
>>    	for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
>>    		pte_t *pte;
>>    		struct map_desc map;
>> @@ -1713,6 +1717,9 @@ static void __init early_fixmap_shutdown(void)
>>    
>>    		create_mapping(&map);
>>    	}
>> +#ifdef CONFIG_FIX_EARLYCON_MEM
>> +	console_start(console_drivers);
>> +#endif
>>    }
>>    
>>    /*
> 
> Best regards

Dear Marek,
Thank you for the notice. I'll figure it out and feed back to you as 
soon as possible.

Regards,
Victor

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Victor Hassan]

On 2022/8/31 20:37, Victor Hassan wrote:
> On 8/31/2022 7:52 PM, Marek Szyprowski wrote:
>> Hi Victor,
>>
>> On 16.03.2022 03:33, Victor Hassan wrote:
>>> earlycon uses fixmap to create a memory map,
>>> So we need to close earlycon before closing fixmap,
>>> otherwise printk will access illegal addresses.
>>> After creating a new memory map, we open earlycon again.
>>>
>>> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
>>
>> This patch landed in linux next-20220831 as commit a76886d117cb ("ARM:
>> 9223/1: mmu: fix access to illegal address when using earlycon &
>> memblock=debug"). Unfortunately it breaks booting of all my test boards
>> which *do not* use earlycon. It can be easily reproduced even with QEMU.
>>
>> With kernel compiled from multi_v7_defconfig the following setup boots:
>>
>> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
>> "console=ttyAMA0 earlycon" -M virt -smp 2 -m 512
>>
>> while this one doesn't:
>>
>> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
>> "console=ttyAMA0" -M virt -smp 2 -m 512
>>
>>
>>> ---
>>>    arch/arm/mm/mmu.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>>> index 274e4f73fd33..f3511f07a7d0 100644
>>> --- a/arch/arm/mm/mmu.c
>>> +++ b/arch/arm/mm/mmu.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/fs.h>
>>>    #include <linux/vmalloc.h>
>>>    #include <linux/sizes.h>
>>> +#include <linux/console.h>
>>>    #include <asm/cp15.h>
>>>    #include <asm/cputype.h>
>>> @@ -1695,6 +1696,9 @@ static void __init early_fixmap_shutdown(void)
>>>        pmd_clear(fixmap_pmd(va));
>>>        local_flush_tlb_kernel_page(va);
>>> +#ifdef CONFIG_FIX_EARLYCON_MEM
>>> +    console_stop(console_drivers);
>>> +#endif
>>>        for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
>>>            pte_t *pte;
>>>            struct map_desc map;
>>> @@ -1713,6 +1717,9 @@ static void __init early_fixmap_shutdown(void)
>>>            create_mapping(&map);
>>>        }
>>> +#ifdef CONFIG_FIX_EARLYCON_MEM
>>> +    console_start(console_drivers);
>>> +#endif
>>>    }
>>>    /*
>>
>> Best regards
> 
> Dear Marek,
> Thank you for the notice. I'll figure it out and feed back to you as 
> soon as possible.
> 
> Regards,
> Victor

Hi Marek,

Sorry, didn't take into account that console_drivers is NULL when 
earlycon is not used.

Here is the patch-v2. Please review:

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index a49f0b9..a240f38 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -14,6 +14,7 @@
  #include <linux/fs.h>
  #include <linux/vmalloc.h>
  #include <linux/sizes.h>
+#include <linux/console.h>

  #include <asm/cp15.h>
  #include <asm/cputype.h>
@@ -1730,6 +1731,10 @@
  	pmd_clear(fixmap_pmd(va));
  	local_flush_tlb_kernel_page(va);

+#ifdef CONFIG_FIX_EARLYCON_MEM
+	if (console_drivers)
+		console_stop(console_drivers);
+#endif
  	for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
  		pte_t *pte;
  		struct map_desc map;
@@ -1748,6 +1753,10 @@

  		create_mapping(&map);
  	}
+#ifdef CONFIG_FIX_EARLYCON_MEM
+	if (console_drivers)
+		console_start(console_drivers);
+#endif
  }

BTW, should I resend the patch-v2 through the site 
(https://www.armlinux.org.uk/developer/patches/add.php), or should I 
send the patch-v2 through E-mail to Linux-Mainline?

Thanks you.

Regards,
Victor

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Rob Herring]

On Thu, Sep 1, 2022 at 7:54 AM Victor Hassan <victor@allwinnertech.com> wrote:
>
>
>
> On 2022/8/31 20:37, Victor Hassan wrote:
> > On 8/31/2022 7:52 PM, Marek Szyprowski wrote:
> >> Hi Victor,
> >>
> >> On 16.03.2022 03:33, Victor Hassan wrote:
> >>> earlycon uses fixmap to create a memory map,
> >>> So we need to close earlycon before closing fixmap,
> >>> otherwise printk will access illegal addresses.

How? Due to recent changes in how printk and the consoles work or just
because create_mapping() can print? In the latter case, the only
variable input is the phys address. I think most if not all prints
cannot occur.

> >>> After creating a new memory map, we open earlycon again.
> >>>
> >>> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
> >>
> >> This patch landed in linux next-20220831 as commit a76886d117cb ("ARM:
> >> 9223/1: mmu: fix access to illegal address when using earlycon &
> >> memblock=debug"). Unfortunately it breaks booting of all my test boards
> >> which *do not* use earlycon. It can be easily reproduced even with QEMU.
> >>
> >> With kernel compiled from multi_v7_defconfig the following setup boots:
> >>
> >> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
> >> "console=ttyAMA0 earlycon" -M virt -smp 2 -m 512
> >>
> >> while this one doesn't:
> >>
> >> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
> >> "console=ttyAMA0" -M virt -smp 2 -m 512
> >>
> >>
> >>> ---
> >>>    arch/arm/mm/mmu.c | 7 +++++++
> >>>    1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> >>> index 274e4f73fd33..f3511f07a7d0 100644
> >>> --- a/arch/arm/mm/mmu.c
> >>> +++ b/arch/arm/mm/mmu.c
> >>> @@ -14,6 +14,7 @@
> >>>    #include <linux/fs.h>
> >>>    #include <linux/vmalloc.h>
> >>>    #include <linux/sizes.h>
> >>> +#include <linux/console.h>
> >>>    #include <asm/cp15.h>
> >>>    #include <asm/cputype.h>
> >>> @@ -1695,6 +1696,9 @@ static void __init early_fixmap_shutdown(void)
> >>>        pmd_clear(fixmap_pmd(va));
> >>>        local_flush_tlb_kernel_page(va);
> >>> +#ifdef CONFIG_FIX_EARLYCON_MEM
> >>> +    console_stop(console_drivers);
> >>> +#endif
> >>>        for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
> >>>            pte_t *pte;
> >>>            struct map_desc map;
> >>> @@ -1713,6 +1717,9 @@ static void __init early_fixmap_shutdown(void)
> >>>            create_mapping(&map);
> >>>        }
> >>> +#ifdef CONFIG_FIX_EARLYCON_MEM
> >>> +    console_start(console_drivers);
> >>> +#endif
> >>>    }
> >>>    /*
> >>
> >> Best regards
> >
> > Dear Marek,
> > Thank you for the notice. I'll figure it out and feed back to you as
> > soon as possible.
> >
> > Regards,
> > Victor
>
> Hi Marek,
>
> Sorry, didn't take into account that console_drivers is NULL when
> earlycon is not used.
>
> Here is the patch-v2. Please review:
>
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index a49f0b9..a240f38 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -14,6 +14,7 @@
>   #include <linux/fs.h>
>   #include <linux/vmalloc.h>
>   #include <linux/sizes.h>
> +#include <linux/console.h>
>
>   #include <asm/cp15.h>
>   #include <asm/cputype.h>
> @@ -1730,6 +1731,10 @@
>         pmd_clear(fixmap_pmd(va));
>         local_flush_tlb_kernel_page(va);
>
> +#ifdef CONFIG_FIX_EARLYCON_MEM

This is always true for CONFIG_MMU and this file is only built for
CONFIG_MMU. So you don't need it.

> +       if (console_drivers)
> +               console_stop(console_drivers);

console_drivers is a list, so you are only stopping the 1st one.
Couldn't console_lock() be used here?

Also, this should be before pmd_clear().

> +#endif
>         for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
>                 pte_t *pte;
>                 struct map_desc map;
> @@ -1748,6 +1753,10 @@
>
>                 create_mapping(&map);
>         }
> +#ifdef CONFIG_FIX_EARLYCON_MEM
> +       if (console_drivers)
> +               console_start(console_drivers);
> +#endif
>   }
>
> BTW, should I resend the patch-v2 through the site
> (https://www.armlinux.org.uk/developer/patches/add.php), or should I
> send the patch-v2 through E-mail to Linux-Mainline?
>
> Thanks you.
>
> Regards,
> Victor
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Rob Herring]

On Thu, Sep 1, 2022 at 8:21 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Sep 1, 2022 at 7:54 AM Victor Hassan <victor@allwinnertech.com> wrote:
> >
> >
> >
> > On 2022/8/31 20:37, Victor Hassan wrote:
> > > On 8/31/2022 7:52 PM, Marek Szyprowski wrote:
> > >> Hi Victor,
> > >>
> > >> On 16.03.2022 03:33, Victor Hassan wrote:
> > >>> earlycon uses fixmap to create a memory map,
> > >>> So we need to close earlycon before closing fixmap,
> > >>> otherwise printk will access illegal addresses.
>
> How? Due to recent changes in how printk and the consoles work or just
> because create_mapping() can print? In the latter case, the only
> variable input is the phys address. I think most if not all prints
> cannot occur.

Ah, I missed the memblock debug part on the subject...

Rob

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Victor Hassan]

Dear Rob,

On 2022/9/1 21:21, Rob Herring wrote:
> On Thu, Sep 1, 2022 at 7:54 AM Victor Hassan <victor@allwinnertech.com> wrote:
>>
>>
>>
>> On 2022/8/31 20:37, Victor Hassan wrote:
>>> On 8/31/2022 7:52 PM, Marek Szyprowski wrote:
>>>> Hi Victor,
>>>>
>>>> On 16.03.2022 03:33, Victor Hassan wrote:
>>>>> earlycon uses fixmap to create a memory map,
>>>>> So we need to close earlycon before closing fixmap,
>>>>> otherwise printk will access illegal addresses.
> 
> How? Due to recent changes in how printk and the consoles work or just
> because create_mapping() can print? In the latter case, the only
> variable input is the phys address. I think most if not all prints
> cannot occur.
> 
>>>>> After creating a new memory map, we open earlycon again.
>>>>>
>>>>> Signed-off-by: Victor Hassan <victor@allwinnertech.com>
>>>>
>>>> This patch landed in linux next-20220831 as commit a76886d117cb ("ARM:
>>>> 9223/1: mmu: fix access to illegal address when using earlycon &
>>>> memblock=debug"). Unfortunately it breaks booting of all my test boards
>>>> which *do not* use earlycon. It can be easily reproduced even with QEMU.
>>>>
>>>> With kernel compiled from multi_v7_defconfig the following setup boots:
>>>>
>>>> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
>>>> "console=ttyAMA0 earlycon" -M virt -smp 2 -m 512
>>>>
>>>> while this one doesn't:
>>>>
>>>> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
>>>> "console=ttyAMA0" -M virt -smp 2 -m 512
>>>>
>>>>
>>>>> ---
>>>>>     arch/arm/mm/mmu.c | 7 +++++++
>>>>>     1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>>>>> index 274e4f73fd33..f3511f07a7d0 100644
>>>>> --- a/arch/arm/mm/mmu.c
>>>>> +++ b/arch/arm/mm/mmu.c
>>>>> @@ -14,6 +14,7 @@
>>>>>     #include <linux/fs.h>
>>>>>     #include <linux/vmalloc.h>
>>>>>     #include <linux/sizes.h>
>>>>> +#include <linux/console.h>
>>>>>     #include <asm/cp15.h>
>>>>>     #include <asm/cputype.h>
>>>>> @@ -1695,6 +1696,9 @@ static void __init early_fixmap_shutdown(void)
>>>>>         pmd_clear(fixmap_pmd(va));
>>>>>         local_flush_tlb_kernel_page(va);
>>>>> +#ifdef CONFIG_FIX_EARLYCON_MEM
>>>>> +    console_stop(console_drivers);
>>>>> +#endif
>>>>>         for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
>>>>>             pte_t *pte;
>>>>>             struct map_desc map;
>>>>> @@ -1713,6 +1717,9 @@ static void __init early_fixmap_shutdown(void)
>>>>>             create_mapping(&map);
>>>>>         }
>>>>> +#ifdef CONFIG_FIX_EARLYCON_MEM
>>>>> +    console_start(console_drivers);
>>>>> +#endif
>>>>>     }
>>>>>     /*
>>>>
>>>> Best regards
>>>
>>> Dear Marek,
>>> Thank you for the notice. I'll figure it out and feed back to you as
>>> soon as possible.
>>>
>>> Regards,
>>> Victor
>>
>> Hi Marek,
>>
>> Sorry, didn't take into account that console_drivers is NULL when
>> earlycon is not used.
>>
>> Here is the patch-v2. Please review:
>>
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index a49f0b9..a240f38 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -14,6 +14,7 @@
>>    #include <linux/fs.h>
>>    #include <linux/vmalloc.h>
>>    #include <linux/sizes.h>
>> +#include <linux/console.h>
>>
>>    #include <asm/cp15.h>
>>    #include <asm/cputype.h>
>> @@ -1730,6 +1731,10 @@
>>          pmd_clear(fixmap_pmd(va));
>>          local_flush_tlb_kernel_page(va);
>>
>> +#ifdef CONFIG_FIX_EARLYCON_MEM
> 
> This is always true for CONFIG_MMU and this file is only built for
> CONFIG_MMU. So you don't need it.

Yes, you are right.

> 
>> +       if (console_drivers)
>> +               console_stop(console_drivers);
> 
> console_drivers is a list, so you are only stopping the 1st one.
> Couldn't console_lock() be used here?
> 

Thanks for the suggestion: console_lock is actually the same as 
console_stop in the test, and the code is more compact.

> Also, this should be before pmd_clear().

During the test, I found that the console failed after executing 
local_flush_tlb_kernel_page, so I think the pmd_clear function can 
output in time if there is printing. This doesn't seem possible, so 
before pmd_clear it's not bad either.

> 
>> +#endif
>>          for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
>>                  pte_t *pte;
>>                  struct map_desc map;
>> @@ -1748,6 +1753,10 @@
>>
>>                  create_mapping(&map);
>>          }
>> +#ifdef CONFIG_FIX_EARLYCON_MEM
>> +       if (console_drivers)
>> +               console_start(console_drivers);
>> +#endif
>>    }
>>
>> BTW, should I resend the patch-v2 through the site
>> (https://www.armlinux.org.uk/developer/patches/add.php), or should I
>> send the patch-v2 through E-mail to Linux-Mainline?
>>
>> Thanks you.
>>
>> Regards,
>> Victor
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Here is the patch-v3. Please review:

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index a49f0b9..57ca77f 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -14,6 +14,7 @@
  #include <linux/fs.h>
  #include <linux/vmalloc.h>
  #include <linux/sizes.h>
+#include <linux/console.h>

  #include <asm/cp15.h>
  #include <asm/cputype.h>
@@ -1727,6 +1728,7 @@
  	unsigned long va = fix_to_virt(__end_of_permanent_fixed_addresses - 1);

  	pte_offset_fixmap = pte_offset_late_fixmap;
+	console_lock();
  	pmd_clear(fixmap_pmd(va));
  	local_flush_tlb_kernel_page(va);

@@ -1748,6 +1750,7 @@

  		create_mapping(&map);
  	}
+	console_unlock();
  }

Thanks you.

Regards,
Victor

Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

[Russell King (Oracle)]

On Wed, Aug 31, 2022 at 01:52:57PM +0200, Marek Szyprowski wrote:
> Hi Victor,
> 
> On 16.03.2022 03:33, Victor Hassan wrote:
> > earlycon uses fixmap to create a memory map,
> > So we need to close earlycon before closing fixmap,
> > otherwise printk will access illegal addresses.
> > After creating a new memory map, we open earlycon again.
> >
> > Signed-off-by: Victor Hassan <victor@allwinnertech.com>
> 
> This patch landed in linux next-20220831 as commit a76886d117cb ("ARM: 
> 9223/1: mmu: fix access to illegal address when using earlycon & 
> memblock=debug"). Unfortunately it breaks booting of all my test boards 
> which *do not* use earlycon. It can be easily reproduced even with QEMU.
> 
> With kernel compiled from multi_v7_defconfig the following setup boots:
> 
> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append 
> "console=ttyAMA0 earlycon" -M virt -smp 2 -m 512
> 
> while this one doesn't:
> 
> $ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append 
> "console=ttyAMA0" -M virt -smp 2 -m 512

That's the second patch to drop from my merge yesterday... not good.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!