Message ID | 63321798f8798524@bloch.sibelius.xs4all.nl |
---|---|
State | Changes Requested |
Delegated to: | Stefan Roese |
Headers | show |
Series | [U-Boot,RFC] Reserve ATF memory on Marvell Armdada 3700/7K/8K | expand |
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; > > } > > >
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; }