[RFC PATCH 4/5] bootefi: Call the EVT_FT_FIXUP event handler

[Simon Glass]

Hi Heinrich,

On Mon, 28 Aug 2023 at 05:02, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 28.08.23 11:32, Sughosh Ganu wrote:
> > On Sat, 26 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 8/26/23 11:06, Sughosh Ganu wrote:
> >>> The bootefi command passes the devicetree to the kernel through the
> >>> EFI config table. Call the event handlers for fixing the devicetree
> >>> before jumping into the kernel. This removes any devicetree nodes
> >>> and/or properties that are specific only to U-Boot, and are not to be
> >>> passed to the OS.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> >>> ---
> >>>    cmd/bootefi.c | 18 ++++++++++++++++++
> >>>    1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index f73d6eb0e2..c359a46ec4 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -237,6 +237,23 @@ static void *get_config_table(const efi_guid_t *guid)
> >>>        return NULL;
> >>>    }
> >>>
> >>> +/**
> >>> + * event_notify_dt_fixup() - call ft_fixup event
> >>> + *
> >>> + * @fdt:     address of the device tree to be passed to the kernel
> >>> + *           through the configuration table
> >>> + * Return:   None
> >>> + */
> >>> +static void event_notify_dt_fixup(void *fdt)
> >>> +{
> >>> +     int ret;
> >>> +     struct event_ft_fixup fixup = {0};
> >>> +
> >>> +     fixup.tree.fdt = fdt;
> >>> +     ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> >>> +     if (ret)
> >>> +             printf("Error: %d: FDT Fixup event failed\n", ret);
> >>> +}
> >>>    #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
> >>>
> >>>    /**
> >>> @@ -318,6 +335,7 @@ efi_status_t efi_install_fdt(void *fdt)
> >>>        efi_carve_out_dt_rsv(fdt);
> >>>
> >>>        efi_try_purge_kaslr_seed(fdt);
> >>> +     event_notify_dt_fixup(fdt);
> >>
> >> The event is already triggered in image_setup_libfdt(). Don't trigger it
> >> twice.
> >
> > The reason I put an explicit event_notify call is because the
> > image_setup_libfdt() call only calls the ft_fixup handlers if the
> > livetree is not active. So the fixup handlers would not be called on
> > platforms that enable livetree. Although I'm not sure if livetree
> > should be disabled before the ft fixup has to happen, or platforms
> > that need ft fixup should not enable OF_LIVE. Disabling the livetree
> > is not happening now, so I am not sure how the fixup event should work
> > on platforms which have OF_LIVE enabled.
>
> We still must not call the same event twice. We further must ensure that
> our fix-up is called last (e.g. by calling it zzz_something) and not
> before adding VBE nodes.
>
> Is there any need to use the event mechanism here?
>
> @Simon:
> None of the other function calls in image_setup_libfdt() depend on
> of_live_active(). I think it would be preferable to move that check into
> the listeners. If instead of of_live_active() you could use a
> configuration setting for the decision, gcc could eliminate a lot of code.

Firstly, of_live_active() should check CONFIG_IS_ENABLED(OF_LIVE)
first. I noticed this over the weekend when looking at something else.

The original code was due to a limitation with U-Boot's ofnode
implementation, in that it did not support multiple devicetrees
without OF_LIVE. We need to support at least two, since we have the
'control' DTB and the one being set up to boot the OS.

Anyway, that limitation can be removed, so long as
CONFIG_MULTI_DTB_FIT is set. In other words, we should enable
MULTI_DTB_FIT if !OF_LIVE and all will be well.

>
> We definitely need better descriptions in include/event.h and a man-page
> generated from that include:
>
> EVT_NONE
>      undescribed
> EVT_TEST
>      undescribed
> EVT_DM_POST_INIT_F
>      undescribed
> EVT_DM_POST_INIT_R
>      undescribed
> EVT_DM_PRE_PROBE
>      Device is about to be probed
> EVT_DM_POST_PROBE
>      undescribed
> EVT_DM_PRE_REMOVE
>      undescribed
> EVT_DM_POST_REMOVE
>      undescribed
> EVT_MISC_INIT_F
>      undescribed
> EVT_FPGA_LOAD
>      undescribed
> EVT_FT_FIXUP
>      undescribed
> EVT_MAIN_LOOP
>      undescribed
> EVT_COUNT
>      undescribed

Yes indeed. I added some for the latest patch in this area.

https://patchwork.ozlabs.org/project/uboot/patch/20230822031705.1340941-15-sjg@chromium.org/

Regards,
Simon