| Message ID | 20241124183832.2476928-1-caleb.connolly@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | common/board_f: init malloc earlier | expand |
On Sun, 24 Nov 2024 at 20:38, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Currently the early malloc initialisation is done partially in > board_init_f_init_reserve() (on arm64 at least), which configures > gd->malloc_base. But it isn't actually usable until initf_malloc() is > called which doesn't happen until after fdtdec_setup(). > > This causes problems in a few scenarios: > > 1. when using MULTI_DTB_FIT as this needs a working malloc (especially > for compressed FIT). > 2. Some platforms may need to allocate memory as part of memory map > initialisation (e.g. Qualcomm will need this to parse the memory map > from SMEM). > > Move the initf_malloc() call earlier so that malloc is available during > fdtdec_setup(). > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > common/board_f.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/common/board_f.c b/common/board_f.c > index 98dc2591e1d0..bddfa6b992b9 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -867,15 +867,15 @@ static int initf_upl(void) > } > > static const init_fnc_t init_sequence_f[] = { > setup_mon_len, > + initf_malloc, > #ifdef CONFIG_OF_CONTROL > fdtdec_setup, > #endif > #ifdef CONFIG_TRACE_EARLY > trace_early_init, > #endif > - initf_malloc, > initf_upl, > log_init, > initf_bootstage, /* uses its own timer, so does not need DM */ > event_init, > -- > 2.47.0 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Hi Caleb, On Sun, 24 Nov 2024 at 11:38, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Currently the early malloc initialisation is done partially in > board_init_f_init_reserve() (on arm64 at least), which configures > gd->malloc_base. But it isn't actually usable until initf_malloc() is > called which doesn't happen until after fdtdec_setup(). > > This causes problems in a few scenarios: > > 1. when using MULTI_DTB_FIT as this needs a working malloc (especially > for compressed FIT). Hmmm, how does this work today? > 2. Some platforms may need to allocate memory as part of memory map > initialisation (e.g. Qualcomm will need this to parse the memory map > from SMEM). That really needs to be tidied up. When does this fixup need to be done? I imagine it is somewhere prior to setup_dest_addr() ? Perhaps we could introduce an event to do 'memory map' stuff? Regards, Simon > > Move the initf_malloc() call earlier so that malloc is available during > fdtdec_setup(). > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > common/board_f.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/common/board_f.c b/common/board_f.c > index 98dc2591e1d0..bddfa6b992b9 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -867,15 +867,15 @@ static int initf_upl(void) > } > > static const init_fnc_t init_sequence_f[] = { > setup_mon_len, > + initf_malloc, > #ifdef CONFIG_OF_CONTROL > fdtdec_setup, > #endif > #ifdef CONFIG_TRACE_EARLY > trace_early_init, > #endif > - initf_malloc, > initf_upl, > log_init, > initf_bootstage, /* uses its own timer, so does not need DM */ > event_init, > -- > 2.47.0 >
Hi Simon, On 26/11/2024 01:32, Simon Glass wrote: > Hi Caleb, > > On Sun, 24 Nov 2024 at 11:38, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> Currently the early malloc initialisation is done partially in >> board_init_f_init_reserve() (on arm64 at least), which configures >> gd->malloc_base. But it isn't actually usable until initf_malloc() is >> called which doesn't happen until after fdtdec_setup(). >> >> This causes problems in a few scenarios: >> >> 1. when using MULTI_DTB_FIT as this needs a working malloc (especially >> for compressed FIT). > > Hmmm, how does this work today? I honestly have no idea, I assume boards that make use of it do some custom board_f. > >> 2. Some platforms may need to allocate memory as part of memory map >> initialisation (e.g. Qualcomm will need this to parse the memory map >> from SMEM). > > That really needs to be tidied up. When does this fixup need to be > done? I imagine it is somewhere prior to setup_dest_addr() ? Perhaps It's necessary in order to figure out gd->ram_base/ram_size. We do it in board_fdt_blob_setup() so that we can support another use-case where U-Boot has an internal FDT (which it should use) but the memory layout is provided via an external FDT which is unavailable (although this usecase isn't enabled upstream yet). > we could introduce an event to do 'memory map' stuff? We could, but considering that on most platforms the pre-relocation malloc is fixed at build time and at a known location relative to U-Boot there is no reason for us to arbitrary decide that some codepaths at the start of board_f aren't allowed to use malloc(). Just moving the malloc initialization earlier ensures malloc() is consistently available and greatly simplifies things. fwiw, I had another variation of this patch which dropped initf_malloc() entirely and just set up gd->malloc_limit/malloc_ptr in board_init_f_init_reserve() since that's where we set malloc_base anyways. But after digging some more there seem to be quite a few other entrypoints into U-Boot that don't go through board_init_f_init_reserve() (e.g. sandbox, EFI app) and it seemed more error prone to duplicate the implementation there. Kind regards > > Regards, > Simon > > >> >> Move the initf_malloc() call earlier so that malloc is available during >> fdtdec_setup(). >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> common/board_f.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/common/board_f.c b/common/board_f.c >> index 98dc2591e1d0..bddfa6b992b9 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -867,15 +867,15 @@ static int initf_upl(void) >> } >> >> static const init_fnc_t init_sequence_f[] = { >> setup_mon_len, >> + initf_malloc, >> #ifdef CONFIG_OF_CONTROL >> fdtdec_setup, >> #endif >> #ifdef CONFIG_TRACE_EARLY >> trace_early_init, >> #endif >> - initf_malloc, >> initf_upl, >> log_init, >> initf_bootstage, /* uses its own timer, so does not need DM */ >> event_init, >> -- >> 2.47.0 >>
Hi Caleb, On Tue, 26 Nov 2024 at 05:22, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Hi Simon, > > On 26/11/2024 01:32, Simon Glass wrote: > > Hi Caleb, > > > > On Sun, 24 Nov 2024 at 11:38, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> > >> Currently the early malloc initialisation is done partially in > >> board_init_f_init_reserve() (on arm64 at least), which configures > >> gd->malloc_base. But it isn't actually usable until initf_malloc() is > >> called which doesn't happen until after fdtdec_setup(). > >> > >> This causes problems in a few scenarios: > >> > >> 1. when using MULTI_DTB_FIT as this needs a working malloc (especially > >> for compressed FIT). > > > > Hmmm, how does this work today? > > I honestly have no idea, I assume boards that make use of it do some > custom board_f. OK. > > > >> 2. Some platforms may need to allocate memory as part of memory map > >> initialisation (e.g. Qualcomm will need this to parse the memory map > >> from SMEM). > > > > That really needs to be tidied up. When does this fixup need to be > > done? I imagine it is somewhere prior to setup_dest_addr() ? Perhaps > > It's necessary in order to figure out gd->ram_base/ram_size. We do it in > board_fdt_blob_setup() so that we can support another use-case where > U-Boot has an internal FDT (which it should use) but the memory layout > is provided via an external FDT which is unavailable (although this > usecase isn't enabled upstream yet). > > > we could introduce an event to do 'memory map' stuff? > > We could, but considering that on most platforms the pre-relocation > malloc is fixed at build time and at a known location relative to U-Boot > there is no reason for us to arbitrary decide that some codepaths at the > start of board_f aren't allowed to use malloc(). > > Just moving the malloc initialization earlier ensures malloc() is > consistently available and greatly simplifies things. I'd like to see a more generic solution to this problem...I think we discussed this before? To my eye it looks like if we called an event in setup_dest_addr() we could allow ram_base to be set. > > fwiw, I had another variation of this patch which dropped initf_malloc() > entirely and just set up gd->malloc_limit/malloc_ptr in > board_init_f_init_reserve() since that's where we set malloc_base > anyways. But after digging some more there seem to be quite a few other > entrypoints into U-Boot that don't go through > board_init_f_init_reserve() (e.g. sandbox, EFI app) and it seemed more > error prone to duplicate the implementation there. Fair enough. One more thing I notice in your board_fdt_blob_setup() implementation in arch/arm/mach-snapdragon/board.c : It seems to be using either an built-in or external devicetree. It seems that we should show this in dm_announce(), i.e. with the call to fdtdec_get_srcname() ? Regards, Simon > > Kind regards > > > > > Regards, > > Simon > > > > > >> > >> Move the initf_malloc() call earlier so that malloc is available during > >> fdtdec_setup(). > >> > >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > >> --- > >> common/board_f.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/common/board_f.c b/common/board_f.c > >> index 98dc2591e1d0..bddfa6b992b9 100644 > >> --- a/common/board_f.c > >> +++ b/common/board_f.c > >> @@ -867,15 +867,15 @@ static int initf_upl(void) > >> } > >> > >> static const init_fnc_t init_sequence_f[] = { > >> setup_mon_len, > >> + initf_malloc, > >> #ifdef CONFIG_OF_CONTROL > >> fdtdec_setup, > >> #endif > >> #ifdef CONFIG_TRACE_EARLY > >> trace_early_init, > >> #endif > >> - initf_malloc, > >> initf_upl, > >> log_init, > >> initf_bootstage, /* uses its own timer, so does not need DM */ > >> event_init, > >> -- > >> 2.47.0 > >> > > -- > // Caleb (they/them) >
Hi Simon, On 26/11/2024 16:38, Simon Glass wrote: > Hi Caleb, > > On Tue, 26 Nov 2024 at 05:22, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> Hi Simon, >> >> On 26/11/2024 01:32, Simon Glass wrote: >>> Hi Caleb, >>> >>> On Sun, 24 Nov 2024 at 11:38, Caleb Connolly <caleb.connolly@linaro.org> wrote: >>>> >>>> Currently the early malloc initialisation is done partially in >>>> board_init_f_init_reserve() (on arm64 at least), which configures >>>> gd->malloc_base. But it isn't actually usable until initf_malloc() is >>>> called which doesn't happen until after fdtdec_setup(). >>>> >>>> This causes problems in a few scenarios: >>>> >>>> 1. when using MULTI_DTB_FIT as this needs a working malloc (especially >>>> for compressed FIT). >>> >>> Hmmm, how does this work today? >> >> I honestly have no idea, I assume boards that make use of it do some >> custom board_f. > > OK. > >>> >>>> 2. Some platforms may need to allocate memory as part of memory map >>>> initialisation (e.g. Qualcomm will need this to parse the memory map >>>> from SMEM). >>> >>> That really needs to be tidied up. When does this fixup need to be >>> done? I imagine it is somewhere prior to setup_dest_addr() ? Perhaps >> >> It's necessary in order to figure out gd->ram_base/ram_size. We do it in >> board_fdt_blob_setup() so that we can support another use-case where >> U-Boot has an internal FDT (which it should use) but the memory layout >> is provided via an external FDT which is unavailable (although this >> usecase isn't enabled upstream yet). >> >>> we could introduce an event to do 'memory map' stuff? >> >> We could, but considering that on most platforms the pre-relocation >> malloc is fixed at build time and at a known location relative to U-Boot >> there is no reason for us to arbitrary decide that some codepaths at the >> start of board_f aren't allowed to use malloc(). >> >> Just moving the malloc initialization earlier ensures malloc() is >> consistently available and greatly simplifies things. > > I'd like to see a more generic solution to this problem...I think we > discussed this before? I think so. I'm not sure I follow with "more generic solution"? > > To my eye it looks like if we called an event in setup_dest_addr() we > could allow ram_base to be set. Right, it's possible/likely that we could clean up how mach-snapdragon currently does memory parsing. I do think this patch would still be justified even if we ignore the Qualcomm case. > >> >> fwiw, I had another variation of this patch which dropped initf_malloc() >> entirely and just set up gd->malloc_limit/malloc_ptr in >> board_init_f_init_reserve() since that's where we set malloc_base >> anyways. But after digging some more there seem to be quite a few other >> entrypoints into U-Boot that don't go through >> board_init_f_init_reserve() (e.g. sandbox, EFI app) and it seemed more >> error prone to duplicate the implementation there. > > Fair enough. > > One more thing I notice in your board_fdt_blob_setup() implementation > in arch/arm/mach-snapdragon/board.c : > > It seems to be using either an built-in or external devicetree. It > seems that we should show this in dm_announce(), i.e. with the call to > fdtdec_get_srcname() ? Yes, the way it's implemented currently assumes that if CONFIG_OF_BOARD is set that we'd never use the built-in DT and mach-snapdragon doesn't fit this assumption. It's not a high priority but for sure something I'd like to see fixed. Kind regards, > > Regards, > Simon > > >> >> Kind regards >> >>> >>> Regards, >>> Simon >>> >>> >>>> >>>> Move the initf_malloc() call earlier so that malloc is available during >>>> fdtdec_setup(). >>>> >>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >>>> --- >>>> common/board_f.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/common/board_f.c b/common/board_f.c >>>> index 98dc2591e1d0..bddfa6b992b9 100644 >>>> --- a/common/board_f.c >>>> +++ b/common/board_f.c >>>> @@ -867,15 +867,15 @@ static int initf_upl(void) >>>> } >>>> >>>> static const init_fnc_t init_sequence_f[] = { >>>> setup_mon_len, >>>> + initf_malloc, >>>> #ifdef CONFIG_OF_CONTROL >>>> fdtdec_setup, >>>> #endif >>>> #ifdef CONFIG_TRACE_EARLY >>>> trace_early_init, >>>> #endif >>>> - initf_malloc, >>>> initf_upl, >>>> log_init, >>>> initf_bootstage, /* uses its own timer, so does not need DM */ >>>> event_init, >>>> -- >>>> 2.47.0 >>>> >> >> -- >> // Caleb (they/them) >>
Hi Caleb, On Tue, 26 Nov 2024 at 09:15, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Hi Simon, > > On 26/11/2024 16:38, Simon Glass wrote: > > Hi Caleb, > > > > On Tue, 26 Nov 2024 at 05:22, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> > >> Hi Simon, > >> > >> On 26/11/2024 01:32, Simon Glass wrote: > >>> Hi Caleb, > >>> > >>> On Sun, 24 Nov 2024 at 11:38, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >>>> > >>>> Currently the early malloc initialisation is done partially in > >>>> board_init_f_init_reserve() (on arm64 at least), which configures > >>>> gd->malloc_base. But it isn't actually usable until initf_malloc() is > >>>> called which doesn't happen until after fdtdec_setup(). > >>>> > >>>> This causes problems in a few scenarios: > >>>> > >>>> 1. when using MULTI_DTB_FIT as this needs a working malloc (especially > >>>> for compressed FIT). > >>> > >>> Hmmm, how does this work today? > >> > >> I honestly have no idea, I assume boards that make use of it do some > >> custom board_f. > > > > OK. > > > >>> > >>>> 2. Some platforms may need to allocate memory as part of memory map > >>>> initialisation (e.g. Qualcomm will need this to parse the memory map > >>>> from SMEM). > >>> > >>> That really needs to be tidied up. When does this fixup need to be > >>> done? I imagine it is somewhere prior to setup_dest_addr() ? Perhaps > >> > >> It's necessary in order to figure out gd->ram_base/ram_size. We do it in > >> board_fdt_blob_setup() so that we can support another use-case where > >> U-Boot has an internal FDT (which it should use) but the memory layout > >> is provided via an external FDT which is unavailable (although this > >> usecase isn't enabled upstream yet). > >> > >>> we could introduce an event to do 'memory map' stuff? > >> > >> We could, but considering that on most platforms the pre-relocation > >> malloc is fixed at build time and at a known location relative to U-Boot > >> there is no reason for us to arbitrary decide that some codepaths at the > >> start of board_f aren't allowed to use malloc(). > >> > >> Just moving the malloc initialization earlier ensures malloc() is > >> consistently available and greatly simplifies things. > > > > I'd like to see a more generic solution to this problem...I think we > > discussed this before? > > I think so. I'm not sure I follow with "more generic solution"? Using an event to set the RAM location, rather than repurposing the FDT-setup function. > > > > To my eye it looks like if we called an event in setup_dest_addr() we > > could allow ram_base to be set. > > Right, it's possible/likely that we could clean up how mach-snapdragon > currently does memory parsing. > > I do think this patch would still be justified even if we ignore the > Qualcomm case. It does put malloc() outside the purview of trace. There is always going to be a race between which subsystem wants to be first and we have certainly changed it several times. But until we have an actual need, I think it is better to wait. > > > >> > >> fwiw, I had another variation of this patch which dropped initf_malloc() > >> entirely and just set up gd->malloc_limit/malloc_ptr in > >> board_init_f_init_reserve() since that's where we set malloc_base > >> anyways. But after digging some more there seem to be quite a few other > >> entrypoints into U-Boot that don't go through > >> board_init_f_init_reserve() (e.g. sandbox, EFI app) and it seemed more > >> error prone to duplicate the implementation there. > > > > Fair enough. > > > > One more thing I notice in your board_fdt_blob_setup() implementation > > in arch/arm/mach-snapdragon/board.c : > > > > It seems to be using either an built-in or external devicetree. It > > seems that we should show this in dm_announce(), i.e. with the call to > > fdtdec_get_srcname() ? > > Yes, the way it's implemented currently assumes that if CONFIG_OF_BOARD > is set that we'd never use the built-in DT and mach-snapdragon doesn't > fit this assumption. It's not a high priority but for sure something I'd > like to see fixed. I'm not even sure that assumption is right, actually... [..] REgards, Simon
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d0..bddfa6b992b9 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -867,15 +867,15 @@ static int initf_upl(void) } static const init_fnc_t init_sequence_f[] = { setup_mon_len, + initf_malloc, #ifdef CONFIG_OF_CONTROL fdtdec_setup, #endif #ifdef CONFIG_TRACE_EARLY trace_early_init, #endif - initf_malloc, initf_upl, log_init, initf_bootstage, /* uses its own timer, so does not need DM */ event_init,
Currently the early malloc initialisation is done partially in board_init_f_init_reserve() (on arm64 at least), which configures gd->malloc_base. But it isn't actually usable until initf_malloc() is called which doesn't happen until after fdtdec_setup(). This causes problems in a few scenarios: 1. when using MULTI_DTB_FIT as this needs a working malloc (especially for compressed FIT). 2. Some platforms may need to allocate memory as part of memory map initialisation (e.g. Qualcomm will need this to parse the memory map from SMEM). Move the initf_malloc() call earlier so that malloc is available during fdtdec_setup(). Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- common/board_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)