[U-Boot,RFC] Reserve ATF memory on Marvell Armdada 3700/7K/8K

Currently U-Boot doesn't make any effort to reserve the memory used by
ARM Trusted Firmware on these platforms.  The result is that the
memory is listed as available in the EFI memory map.  And as soon as a
loaded kernel tries to use this memory things explode.  I've seen this
with the OpenBSD kernel.  But I totally expect a Linux kernel to
suffer the same fate.

I'm currently using the diff below, but it is not entirely clear to me
if arch_early_init_r() is the appropriate place to do this.  I'm also
wondering whether the block should also be marked as reserved in the
FDT using fdt_add_mem_rsv().  If the latter is required this probably
needs to be done by ft_board_setup() or ft_system_setup().

The address and size of the region have been taken from Marvell's ATF
fork at

  https://github.com/MarvellEmbeddedProcessors/atf-marvell

The memory layout is defined in

  plat/marvell/a8k/common/include/platform_def.h

where there are lots of defines and a diagram that attempt to describe
the memory.  It is not entirely obvious to me what part needs to be
reserved.  But 0x0400000-0x04200000 works.

Hello,

I think I suffered from random kernel crashed due to this issue. Could
you please also submit this patch to downstream Marvell u-boot at github PR?

31.03.2018 17:13, Mark Kettenis пишет:
> Currently U-Boot doesn't make any effort to reserve the memory used by
> ARM Trusted Firmware on these platforms.  The result is that the
> memory is listed as available in the EFI memory map.  And as soon as a
> loaded kernel tries to use this memory things explode.  I've seen this
> with the OpenBSD kernel.  But I totally expect a Linux kernel to
> suffer the same fate.
> 
> I'm currently using the diff below, but it is not entirely clear to me
> if arch_early_init_r() is the appropriate place to do this.  I'm also
> wondering whether the block should also be marked as reserved in the
> FDT using fdt_add_mem_rsv().  If the latter is required this probably
> needs to be done by ft_board_setup() or ft_system_setup().
> 
> The address and size of the region have been taken from Marvell's ATF
> fork at
> 
>   https://github.com/MarvellEmbeddedProcessors/atf-marvell
> 
> The memory layout is defined in
> 
>   plat/marvell/a8k/common/include/platform_def.h
> 
> where there are lots of defines and a diagram that attempt to describe
> the memory.  It is not entirely obvious to me what part needs to be
> reserved.  But 0x0400000-0x04200000 works.
> 
> 
> 
> 
> diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
> index 3c84043a2c..895cd2852f 100644
> --- a/arch/arm/mach-mvebu/arm64-common.c
> +++ b/arch/arm/mach-mvebu/arm64-common.c
> @@ -95,5 +95,11 @@ int arch_early_init_r(void)
>  	pci_init();
>  #endif
>  
> +#ifdef CONFIG_EFI_LOADER
> +	/* Reserve trusted SRAM section */
> +	efi_add_memory_map(0x04000000, 0x00200000 >> EFI_PAGE_SHIFT,
> +			   EFI_RESERVED_MEMORY_TYPE, false);
> +#endif
> +
>  	return 0;
>  }
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

On 31.03.18 16:13, Mark Kettenis wrote:
> Currently U-Boot doesn't make any effort to reserve the memory used by
> ARM Trusted Firmware on these platforms.  The result is that the
> memory is listed as available in the EFI memory map.  And as soon as a
> loaded kernel tries to use this memory things explode.  I've seen this
> with the OpenBSD kernel.  But I totally expect a Linux kernel to
> suffer the same fate.

Please make sure to CC people you think would be interested. In this
case, I believe Stefan certainly would care. Me too :).

> I'm currently using the diff below, but it is not entirely clear to me
> if arch_early_init_r() is the appropriate place to do this.  I'm also
> wondering whether the block should also be marked as reserved in the
> FDT using fdt_add_mem_rsv().  If the latter is required this probably
> needs to be done by ft_board_setup() or ft_system_setup().
> 
> The address and size of the region have been taken from Marvell's ATF
> fork at
> 
>   https://github.com/MarvellEmbeddedProcessors/atf-marvell
> 
> The memory layout is defined in
> 
>   plat/marvell/a8k/common/include/platform_def.h
> 
> where there are lots of defines and a diagram that attempt to describe
> the memory.  It is not entirely obvious to me what part needs to be
> reserved.  But 0x0400000-0x04200000 works.

Yeah, so ATF resides in RAM and U-Boot obviously needs to propagate that
information.

I actually think for mvebu it might make sense to completely override
efi_add_known_memory(). If I read all the bdinfo logic correctly,
they're going through great lengths to reduce the amount of address
space they propagate in bi_dram:


https://github.com/u-boot/u-boot/blob/master/arch/arm/mach-mvebu/dram.c#L274

That of course means our memory map is completely bogus. So instead,
what I would do is create an override for efi_add_known_memory() where
you just walk the dram slots yourself and add all memory you find as
BOOTTIME_DATA. That way the OS will know that it can use it for itself,
but it won't be used during boot time.

Then, go through bi_dram and force all those regions as free memory.

And at last, reserve the ATF range as reserved. And yes, you want to do
that in both - EFI map and as FDT fixup.


Alex

On 31.03.18 16:13, Mark Kettenis wrote:
> Currently U-Boot doesn't make any effort to reserve the memory used by
> ARM Trusted Firmware on these platforms.  The result is that the
> memory is listed as available in the EFI memory map.  And as soon as a
> loaded kernel tries to use this memory things explode.  I've seen this
> with the OpenBSD kernel.  But I totally expect a Linux kernel to
> suffer the same fate.
> 
> I'm currently using the diff below, but it is not entirely clear to me
> if arch_early_init_r() is the appropriate place to do this.  I'm also
> wondering whether the block should also be marked as reserved in the
> FDT using fdt_add_mem_rsv().  If the latter is required this probably
> needs to be done by ft_board_setup() or ft_system_setup().
> 
> The address and size of the region have been taken from Marvell's ATF
> fork at
> 
>   https://github.com/MarvellEmbeddedProcessors/atf-marvell
> 
> The memory layout is defined in
> 
>   plat/marvell/a8k/common/include/platform_def.h
> 
> where there are lots of defines and a diagram that attempt to describe
> the memory.  It is not entirely obvious to me what part needs to be
> reserved.  But 0x0400000-0x04200000 works.
> 
> 
> 
> 
> diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
> index 3c84043a2c..895cd2852f 100644
> --- a/arch/arm/mach-mvebu/arm64-common.c
> +++ b/arch/arm/mach-mvebu/arm64-common.c
> @@ -95,5 +95,11 @@ int arch_early_init_r(void)
>  	pci_init();
>  #endif
>  
> +#ifdef CONFIG_EFI_LOADER
> +	/* Reserve trusted SRAM section */
> +	efi_add_memory_map(0x04000000, 0x00200000 >> EFI_PAGE_SHIFT,
> +			   EFI_RESERVED_MEMORY_TYPE, false);

I also forgot to comment here. 2MB is probably not enough:


https://github.com/MarvellEmbeddedProcessors/atf-marvell/blob/atf-v1.3-armada-17.10/plat/marvell/a8k/common/include/platform_def.h#L110

That sounds more like it should span 65MB (PLAT_MARVELL_TRUSTED_ROM_SIZE
plus 0x100000).

Looking at what edk2 does, it seems to use the same range as you:


https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/marvell-armada-wip/Platforms/Marvell/Armada/Armada.dsc.inc#L347

So let's also CC Marcin :). Maybe his range is too short!


Alex

On Fri, Apr 06, 2018 at 12:22:03PM +0200, Alexander Graf wrote:
> On 31.03.18 16:13, Mark Kettenis wrote:
> > Currently U-Boot doesn't make any effort to reserve the memory used by
> > ARM Trusted Firmware on these platforms.  The result is that the
> > memory is listed as available in the EFI memory map.  And as soon as a
> > loaded kernel tries to use this memory things explode.  I've seen this
> > with the OpenBSD kernel.  But I totally expect a Linux kernel to
> > suffer the same fate.
> > 
> > I'm currently using the diff below, but it is not entirely clear to me
> > if arch_early_init_r() is the appropriate place to do this.  I'm also
> > wondering whether the block should also be marked as reserved in the
> > FDT using fdt_add_mem_rsv().  If the latter is required this probably
> > needs to be done by ft_board_setup() or ft_system_setup().
> > 
> > The address and size of the region have been taken from Marvell's ATF
> > fork at
> > 
> >   https://github.com/MarvellEmbeddedProcessors/atf-marvell
> > 
> > The memory layout is defined in
> > 
> >   plat/marvell/a8k/common/include/platform_def.h
> > 
> > where there are lots of defines and a diagram that attempt to describe
> > the memory.  It is not entirely obvious to me what part needs to be
> > reserved.  But 0x0400000-0x04200000 works.
> > 
> > 
> > 
> > 
> > diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
> > index 3c84043a2c..895cd2852f 100644
> > --- a/arch/arm/mach-mvebu/arm64-common.c
> > +++ b/arch/arm/mach-mvebu/arm64-common.c
> > @@ -95,5 +95,11 @@ int arch_early_init_r(void)
> >  	pci_init();
> >  #endif
> >  
> > +#ifdef CONFIG_EFI_LOADER
> > +	/* Reserve trusted SRAM section */
> > +	efi_add_memory_map(0x04000000, 0x00200000 >> EFI_PAGE_SHIFT,
> > +			   EFI_RESERVED_MEMORY_TYPE, false);
> 
> I also forgot to comment here. 2MB is probably not enough:
> 
> 
> https://github.com/MarvellEmbeddedProcessors/atf-marvell/blob/atf-v1.3-armada-17.10/plat/marvell/a8k/common/include/platform_def.h#L110
> 
> That sounds more like it should span 65MB (PLAT_MARVELL_TRUSTED_ROM_SIZE
> plus 0x100000).
> 
> Looking at what edk2 does, it seems to use the same range as you:
> 
> 
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/marvell-armada-wip/Platforms/Marvell/Armada/Armada.dsc.inc#L347
> 
> So let's also CC Marcin :). Maybe his range is too short!
> 
> 
> Alex
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

The current ATF and EDK2 branch from Marvell seem to reserve a bit more.
I just had the same issue and am using this diff.  This then creates the
same map of usable memory as EDK2 does.

Best regards,
Patrick

diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
index 47bbf69944..56c0d3f6b9 100644
--- a/arch/arm/mach-mvebu/arm64-common.c
+++ b/arch/arm/mach-mvebu/arm64-common.c
@@ -14,6 +14,7 @@
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
 #include <asm/armv8/mmu.h>
+#include <efi_loader.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -142,5 +143,11 @@ int arch_early_init_r(void)
 	pci_init();
 #endif
 
+#ifdef CONFIG_EFI_LOADER
+	/* Reserve trusted SRAM section */
+	efi_add_memory_map(0x04000000, 0x01400000 >> EFI_PAGE_SHIFT,
+			   EFI_RESERVED_MEMORY_TYPE, false);
+#endif
+
 	return 0;
 }

On 02/04/2019 05:51 PM, Patrick Wildt wrote:
> On Fri, Apr 06, 2018 at 12:22:03PM +0200, Alexander Graf wrote:
>> On 31.03.18 16:13, Mark Kettenis wrote:
>>> Currently U-Boot doesn't make any effort to reserve the memory used by
>>> ARM Trusted Firmware on these platforms.  The result is that the
>>> memory is listed as available in the EFI memory map.  And as soon as a
>>> loaded kernel tries to use this memory things explode.  I've seen this
>>> with the OpenBSD kernel.  But I totally expect a Linux kernel to
>>> suffer the same fate.
>>>
>>> I'm currently using the diff below, but it is not entirely clear to me
>>> if arch_early_init_r() is the appropriate place to do this.  I'm also
>>> wondering whether the block should also be marked as reserved in the
>>> FDT using fdt_add_mem_rsv().  If the latter is required this probably
>>> needs to be done by ft_board_setup() or ft_system_setup().
>>>
>>> The address and size of the region have been taken from Marvell's ATF
>>> fork at
>>>
>>>    https://github.com/MarvellEmbeddedProcessors/atf-marvell
>>>
>>> The memory layout is defined in
>>>
>>>    plat/marvell/a8k/common/include/platform_def.h
>>>
>>> where there are lots of defines and a diagram that attempt to describe
>>> the memory.  It is not entirely obvious to me what part needs to be
>>> reserved.  But 0x0400000-0x04200000 works.
>>>
>>>
>>>
>>>
>>> diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
>>> index 3c84043a2c..895cd2852f 100644
>>> --- a/arch/arm/mach-mvebu/arm64-common.c
>>> +++ b/arch/arm/mach-mvebu/arm64-common.c
>>> @@ -95,5 +95,11 @@ int arch_early_init_r(void)
>>>   	pci_init();
>>>   #endif
>>>   
>>> +#ifdef CONFIG_EFI_LOADER
>>> +	/* Reserve trusted SRAM section */
>>> +	efi_add_memory_map(0x04000000, 0x00200000 >> EFI_PAGE_SHIFT,
>>> +			   EFI_RESERVED_MEMORY_TYPE, false);
>> I also forgot to comment here. 2MB is probably not enough:
>>
>>
>> https://github.com/MarvellEmbeddedProcessors/atf-marvell/blob/atf-v1.3-armada-17.10/plat/marvell/a8k/common/include/platform_def.h#L110
>>
>> That sounds more like it should span 65MB (PLAT_MARVELL_TRUSTED_ROM_SIZE
>> plus 0x100000).
>>
>> Looking at what edk2 does, it seems to use the same range as you:
>>
>>
>> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/marvell-armada-wip/Platforms/Marvell/Armada/Armada.dsc.inc#L347
>>
>> So let's also CC Marcin :). Maybe his range is too short!
>>
>>
>> Alex
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> The current ATF and EDK2 branch from Marvell seem to reserve a bit more.
> I just had the same issue and am using this diff.  This then creates the
> same map of usable memory as EDK2 does.

Could you please resend as proper patch (with SoB, separate email) with 
a reference to the edk2 code in the commit message?


Thanks,

Alex

>
> Best regards,
> Patrick
>
> diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
> index 47bbf69944..56c0d3f6b9 100644
> --- a/arch/arm/mach-mvebu/arm64-common.c
> +++ b/arch/arm/mach-mvebu/arm64-common.c
> @@ -14,6 +14,7 @@
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/soc.h>
>   #include <asm/armv8/mmu.h>
> +#include <efi_loader.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -142,5 +143,11 @@ int arch_early_init_r(void)
>   	pci_init();
>   #endif
>   
> +#ifdef CONFIG_EFI_LOADER
> +	/* Reserve trusted SRAM section */
> +	efi_add_memory_map(0x04000000, 0x01400000 >> EFI_PAGE_SHIFT,
> +			   EFI_RESERVED_MEMORY_TYPE, false);
> +#endif
> +
>   	return 0;
>   }

Hi Patrick,

You can refer to the mainline edk2 patch:
https://github.com/tianocore/edk2-platforms/commit/bf1c4a2cf8024669d1748e78c7e417433f85707e

Best regards,
Marcin

wt., 12 lut 2019 o 10:38 Alexander Graf <agraf@suse.de> napisał(a):

> On 02/04/2019 05:51 PM, Patrick Wildt wrote:
> > On Fri, Apr 06, 2018 at 12:22:03PM +0200, Alexander Graf wrote:
> >> On 31.03.18 16:13, Mark Kettenis wrote:
> >>> Currently U-Boot doesn't make any effort to reserve the memory used by
> >>> ARM Trusted Firmware on these platforms.  The result is that the
> >>> memory is listed as available in the EFI memory map.  And as soon as a
> >>> loaded kernel tries to use this memory things explode.  I've seen this
> >>> with the OpenBSD kernel.  But I totally expect a Linux kernel to
> >>> suffer the same fate.
> >>>
> >>> I'm currently using the diff below, but it is not entirely clear to me
> >>> if arch_early_init_r() is the appropriate place to do this.  I'm also
> >>> wondering whether the block should also be marked as reserved in the
> >>> FDT using fdt_add_mem_rsv().  If the latter is required this probably
> >>> needs to be done by ft_board_setup() or ft_system_setup().
> >>>
> >>> The address and size of the region have been taken from Marvell's ATF
> >>> fork at
> >>>
> >>>    https://github.com/MarvellEmbeddedProcessors/atf-marvell
> >>>
> >>> The memory layout is defined in
> >>>
> >>>    plat/marvell/a8k/common/include/platform_def.h
> >>>
> >>> where there are lots of defines and a diagram that attempt to describe
> >>> the memory.  It is not entirely obvious to me what part needs to be
> >>> reserved.  But 0x0400000-0x04200000 works.
> >>>
> >>>
> >>>
> >>>
> >>> diff --git a/arch/arm/mach-mvebu/arm64-common.c
> b/arch/arm/mach-mvebu/arm64-common.c
> >>> index 3c84043a2c..895cd2852f 100644
> >>> --- a/arch/arm/mach-mvebu/arm64-common.c
> >>> +++ b/arch/arm/mach-mvebu/arm64-common.c
> >>> @@ -95,5 +95,11 @@ int arch_early_init_r(void)
> >>>     pci_init();
> >>>   #endif
> >>>
> >>> +#ifdef CONFIG_EFI_LOADER
> >>> +   /* Reserve trusted SRAM section */
> >>> +   efi_add_memory_map(0x04000000, 0x00200000 >> EFI_PAGE_SHIFT,
> >>> +                      EFI_RESERVED_MEMORY_TYPE, false);
> >> I also forgot to comment here. 2MB is probably not enough:
> >>
> >>
> >>
> https://github.com/MarvellEmbeddedProcessors/atf-marvell/blob/atf-v1.3-armada-17.10/plat/marvell/a8k/common/include/platform_def.h#L110
> >>
> >> That sounds more like it should span 65MB (PLAT_MARVELL_TRUSTED_ROM_SIZE
> >> plus 0x100000).
> >>
> >> Looking at what edk2 does, it seems to use the same range as you:
> >>
> >>
> >>
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/marvell-armada-wip/Platforms/Marvell/Armada/Armada.dsc.inc#L347
> >>
> >> So let's also CC Marcin :). Maybe his range is too short!
> >>
> >>
> >> Alex
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot@lists.denx.de
> >> https://lists.denx.de/listinfo/u-boot
> > The current ATF and EDK2 branch from Marvell seem to reserve a bit more.
> > I just had the same issue and am using this diff.  This then creates the
> > same map of usable memory as EDK2 does.
>
> Could you please resend as proper patch (with SoB, separate email) with
> a reference to the edk2 code in the commit message?
>
>
> Thanks,
>
> Alex
>
> >
> > Best regards,
> > Patrick
> >
> > diff --git a/arch/arm/mach-mvebu/arm64-common.c
> b/arch/arm/mach-mvebu/arm64-common.c
> > index 47bbf69944..56c0d3f6b9 100644
> > --- a/arch/arm/mach-mvebu/arm64-common.c
> > +++ b/arch/arm/mach-mvebu/arm64-common.c
> > @@ -14,6 +14,7 @@
> >   #include <asm/arch/cpu.h>
> >   #include <asm/arch/soc.h>
> >   #include <asm/armv8/mmu.h>
> > +#include <efi_loader.h>
> >
> >   DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -142,5 +143,11 @@ int arch_early_init_r(void)
> >       pci_init();
> >   #endif
> >
> > +#ifdef CONFIG_EFI_LOADER
> > +     /* Reserve trusted SRAM section */
> > +     efi_add_memory_map(0x04000000, 0x01400000 >> EFI_PAGE_SHIFT,
> > +                        EFI_RESERVED_MEMORY_TYPE, false);
> > +#endif
> > +
> >       return 0;
> >   }
>
>
>