caching BLOBLISTT_SPL_HANDOFF (was Re: [PATCH] common/board_f.c: use #ifdefs a little more consistently)

On 28/02/2020 18.35, Tom Rini wrote:
> On Fri, Feb 28, 2020 at 05:24:58PM +0000, Rasmus Villemoes wrote:

>> eliminated, and there's not an #ifdef in sight.
> 
> That sounds pretty nice actually.  If you're so inclined I'd like to see
> it.
> 

So I started looking at that, and while it's mostly mechanical, one
quickly hits the case I was worried about, some of the functions
referring to symbols or struct members that are conditionally
defined/declared, so there's no way around guarding such accesses at the
preprocessor level.

Case in point: I'd like to do


but that doesn't work because gd->spl_handoff only exists when
CONFIG_IS_ENABLED(BLOBLIST) && defined(CONFIG_SPL).

Now that particular one seems a bit fishy: Why is it ok to cache the
location of the BLOBLISTT_SPL_HANDOFF blob in gd->spl_handoff? Later in
the init sequence there's a call to reserve_bloblist, and later again
reloc_bloblist. Doesn't that leave gd->spl_handoff stale?

I'd expect that users would always have to look it up via
bloblist_find(). Simon?

Rasmus

Hi Rasmus,

On Fri, 28 Feb 2020 at 16:09, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 28/02/2020 18.35, Tom Rini wrote:
> > On Fri, Feb 28, 2020 at 05:24:58PM +0000, Rasmus Villemoes wrote:
>
> >> eliminated, and there's not an #ifdef in sight.
> >
> > That sounds pretty nice actually.  If you're so inclined I'd like to see
> > it.
> >
>
> So I started looking at that, and while it's mostly mechanical, one
> quickly hits the case I was worried about, some of the functions
> referring to symbols or struct members that are conditionally
> defined/declared, so there's no way around guarding such accesses at the
> preprocessor level.
>
> Case in point: I'd like to do
>
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -287,11 +287,9 @@ static int setup_mon_len(void)
>
>  static int setup_spl_handoff(void)
>  {
> -#if CONFIG_IS_ENABLED(HANDOFF)
>         gd->spl_handoff = bloblist_find(BLOBLISTT_SPL_HANDOFF,
>                                         sizeof(struct spl_handoff));
>         debug("Found SPL hand-off info %p\n", gd->spl_handoff);
> -#endif
>
>         return 0;
>  }
> @@ -886,7 +884,7 @@ static const init_fnc_t init_sequence_f[] = {
>  #ifdef CONFIG_BLOBLIST
>         bloblist_init,
>  #endif
> -       setup_spl_handoff,
> +       CONFIG_IS_ENABLED(HANDOFF) ? setup_spl_handoff : NULL,
>         initf_console_record,
>  #if defined(CONFIG_HAVE_FSP)
>         arch_fsp_init,
>
> but that doesn't work because gd->spl_handoff only exists when
> CONFIG_IS_ENABLED(BLOBLIST) && defined(CONFIG_SPL).
>
> Now that particular one seems a bit fishy: Why is it ok to cache the
> location of the BLOBLISTT_SPL_HANDOFF blob in gd->spl_handoff? Later in
> the init sequence there's a call to reserve_bloblist, and later again
> reloc_bloblist. Doesn't that leave gd->spl_handoff stale?

Yes it does. It is only supposed to be used in the early stages of
U-Boot (proper) init.

Actually I think that member could be dropped and we could search for
it each time:

./arch/x86/cpu/broadwell/cpu_from_spl.c

>
> I'd expect that users would always have to look it up via
> bloblist_find(). Simon?

Regards,
Simon

On 02/03/2020 20.47, Simon Glass wrote:
> Hi Rasmus,
> 
> On Fri, 28 Feb 2020 at 16:09, Rasmus Villemoes
> <rasmus.villemoes at prevas.dk> wrote:
>>

>> Now that particular one seems a bit fishy: Why is it ok to cache the
>> location of the BLOBLISTT_SPL_HANDOFF blob in gd->spl_handoff? Later in
>> the init sequence there's a call to reserve_bloblist, and later again
>> reloc_bloblist. Doesn't that leave gd->spl_handoff stale?
> 
> Yes it does. It is only supposed to be used in the early stages of
> U-Boot (proper) init.

Yes, that's what I thought - and if it's only actually used once or
twice during the early stages, there's not much point in caching it.

> Actually I think that member could be dropped and we could search for
> it each time:
> 
> ./arch/x86/cpu/broadwell/cpu_from_spl.c

Yes, there didn't seem to be many users, so it should not be that hard
to get rid of. I also think that sets a better precedent for future
bloblist users.

Rasmus

Hi Rasmus,

On Mon, 2 Mar 2020 at 13:01, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 02/03/2020 20.47, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Fri, 28 Feb 2020 at 16:09, Rasmus Villemoes
> > <rasmus.villemoes at prevas.dk> wrote:
> >>
>
> >> Now that particular one seems a bit fishy: Why is it ok to cache the
> >> location of the BLOBLISTT_SPL_HANDOFF blob in gd->spl_handoff? Later in
> >> the init sequence there's a call to reserve_bloblist, and later again
> >> reloc_bloblist. Doesn't that leave gd->spl_handoff stale?
> >
> > Yes it does. It is only supposed to be used in the early stages of
> > U-Boot (proper) init.
>
> Yes, that's what I thought - and if it's only actually used once or
> twice during the early stages, there's not much point in caching it.
>
> > Actually I think that member could be dropped and we could search for
> > it each time:
> >
> > ./arch/x86/cpu/broadwell/cpu_from_spl.c
>
> Yes, there didn't seem to be many users, so it should not be that hard
> to get rid of. I also think that sets a better precedent for future
> bloblist users.

Sounds good, thanks.

I wonder if one day we will want to support multiple bloblists in
different memory locations.

Regards,
Simon