| Message ID | 20230826090633.239342-5-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Allow for removal of DT nodes and properties | expand |
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@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. Best regards Heinrich > > if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) { > ret = efi_tcg2_measure_dtb(fdt);
On Sat, 26 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@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@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. -sughosh > > Best regards > > Heinrich > > > > > if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) { > > ret = efi_tcg2_measure_dtb(fdt); >
On 28.08.23 11:32, Sughosh Ganu wrote: > On Sat, 26 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@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@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. 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 Best regards Heinrich
Hi Heinrich, On Mon, 28 Aug 2023 at 05:02, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 28.08.23 11:32, Sughosh Ganu wrote: > > On Sat, 26 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@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@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
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); if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) { ret = efi_tcg2_measure_dtb(fdt);
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@linaro.org> --- cmd/bootefi.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)