Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 11/11/24 11:45, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Mon, 11 Nov 2024 at 10:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/25/24 13:14, Sughosh Ganuefi_bootmgr_pmem_setup wrote:
>>> The EFI HTTP boot puts the iso installer image at some location in
>>> memory which needs to be reserved in the devicetree as persistent
>>> memory (pmem). Add helper functions which add this pmem node when the
>>> EFI_DT_FIXUP protocol's fixup callback is invoked.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>> ---
>>>    boot/image-fdt.c             |  9 +++++++++
>>>    include/efi_loader.h         | 17 +++++++++++++++++
>>>    lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>>>    lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>>>    4 files changed, 59 insertions(+)
>>>
>>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>>> index 8eda521693..b39e81ad30 100644
>>> --- a/boot/image-fdt.c
>>> +++ b/boot/image-fdt.c
>>> @@ -11,6 +11,7 @@
>>>    #include <command.h>
>>>    #include <fdt_support.h>
>>>    #include <fdtdec.h>
>>> +#include <efi_loader.h>
>>>    #include <env.h>
>>>    #include <errno.h>
>>>    #include <image.h>
>>> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>>>        if (!ft_verify_fdt(blob))
>>>                goto err;
>>>
>>> +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
>>> +             fdt_ret = fdt_efi_pmem_setup(blob);
>>
>> I can see no reason why pmem setup should depend on HTTP boot.
>
> The reason is that we *know* we want to preserve image on HTTP
> installers. but we should only add it if that image is booted, not
> unconditionally if EFI_HTTP is enabled
>
>>
>> It should be possible to pass a memory block device to the kernel no
>> matter how it was created.
>
> Yes, we can. But how do we know we need to setup a pmem node? In this
> specific case, we know http installers need the image.
> If we can find similar rules (or perhaps a command line option), we
> can preserve it for all images

Simon is working on patches to track loaded images. I guess there we
would need to add the detection of image types including bare kernels,
EFI binaries, and ISOs.

>
> Thanks
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>> +             if (fdt_ret) {
>>> +                     printf("ERROR: HTTP boot pmem fixup failed\n");

Please, use log_err() and remove "ERROR: ".

>>> +                     goto err;
>>> +             }
>>> +     }
>>> +
>>>        /* after here we are using a livetree */
>>>        if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
>>>                struct event_ft_fixup fixup;
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index d450e304c6..031de18746 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>>>    efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>>>                                    efi_guid_t *guid);
>>>
>>> +/**
>>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
>>> + *
>>> + * @fdt:     Pointer to the devicetree
>>> + *
>>> + * Return:   0 on success, negative on failure
>>> + */
>>> +int fdt_efi_pmem_setup(void *fdt);
>>> +
>>>    /**
>>>     * efi_size_in_pages() - convert size in bytes to size in pages
>>>     *
>>> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>>>                                  void *load_options);
>>>    efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>>>
>>> +/**
>>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
>>> + *
>>> + * @fdt:     Pointer to the DT blob
>>> + * Return:   status code
>>> + */
>>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
>>> +
>>>    /**
>>>     * struct efi_image_regions - A list of memory regions
>>>     *
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 16f75555f6..1d9246be61 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -41,6 +41,8 @@ struct uridp_context {
>>>        efi_handle_t mem_handle;
>>>    };
>>>
>>> +static struct uridp_context *uctx;
>>> +
>>>    const efi_guid_t efi_guid_bootmenu_auto_generated =
>>>                EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>>>
>>> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>>>
>>>        efi_free_pool(ctx->loaded_dp);
>>>        free(ctx);
>>> +     uctx = NULL;
>>>
>>>        return ret == EFI_SUCCESS ? ret2 : ret;
>>>    }
>>> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>>>        EFI_EXIT(ret);
>>>    }
>>>
>>> +/**
>>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
>>> + *
>>> + * @fdt:     Pointer to the DT blob
>>> + * Return:   status code
>>> + */
>>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
>>> +{
>>> +     if (!uctx) {
>>> +             log_warning("No EFI HTTP boot context found\n");

Are we writing a warning here when loading grubriscv64.efi from disk and
executing it?

Best regards

Heinrich

>>> +             return EFI_SUCCESS;
>>> +     }
>>> +
>>> +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
>>> +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
>>> +}
>>> +
>>>    /**
>>>     * try_load_from_uri_path() - Handle the URI device path
>>>     *
>>> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>>>        if (!ctx)
>>>                return EFI_OUT_OF_RESOURCES;
>>>
>>> +     uctx = ctx;
>>>        s = env_get("loadaddr");
>>>        if (!s) {
>>>                log_err("Error: loadaddr is not set\n");
>>> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
>>> index 00167bd2a1..33cd8b9a50 100644
>>> --- a/lib/efi_loader/efi_helper.c
>>> +++ b/lib/efi_loader/efi_helper.c
>>> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>>>        return 0;
>>>    }
>>>
>>> +/**
>>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
>>> + *
>>> + * @fdt:     Pointer to the devicetree
>>> + *
>>> + * Return:   0 on success, negative on failure
>>> + */
>>> +int fdt_efi_pmem_setup(void *fdt)
>>> +{
>>> +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
>>> +}
>>> +
>>>    static int u16_tohex(u16 c)
>>>    {
>>>        if (c >= '0' && c <= '9')
>>