Re: [PATCH 4/6] hw/mips/boston: Support dumpdtb monitor commands

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Peter,

On 6/2/25 16:12, Peter Maydell wrote:
> The boston machine doesn't set MachineState::fdt to the DTB blob that
> it has loaded or created, which means that the QMP/HMP dumpdtb
> monitor commands don't work.
> 
> Setting MachineState::fdt is easy in the non-FIT codepath: we can
> simply do so immediately before loading the DTB into guest memory.
> The FIT codepath is a bit more awkward as currently the FIT loader
> throws away the memory that the FDT was in after it loads it into
> guest memory.  So we add a void *pfdt argument to load_fit() for it
> to store the FDT pointer into.
> 
> There is some readjustment required of the pointer handling in
> loader-fit.c, so that it applies 'const' only where it should (e.g.
> the data pointer we get back from fdt_getprop() is const, because
> it's into the middle of the input FDT data, but the pointer that
> fit_load_image_alloc() should not be const, because it's freshly
> allocated memory that the caller can change if it likes).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/loader-fit.h | 21 ++++++++++++++++++---
>   hw/core/loader-fit.c    | 38 +++++++++++++++++++++-----------------
>   hw/mips/boston.c        | 11 +++++++----
>   3 files changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/include/hw/loader-fit.h b/include/hw/loader-fit.h
> index 0832e379dc9..9a43490ed63 100644
> --- a/include/hw/loader-fit.h
> +++ b/include/hw/loader-fit.h
> @@ -30,12 +30,27 @@ struct fit_loader_match {
>   struct fit_loader {
>       const struct fit_loader_match *matches;
>       hwaddr (*addr_to_phys)(void *opaque, uint64_t addr);
> -    const void *(*fdt_filter)(void *opaque, const void *fdt,
> -                              const void *match_data, hwaddr *load_addr);
> +    void *(*fdt_filter)(void *opaque, const void *fdt,
> +                        const void *match_data, hwaddr *load_addr);
>       const void *(*kernel_filter)(void *opaque, const void *kernel,
>                                    hwaddr *load_addr, hwaddr *entry_addr);
>   };
>   
> -int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque);
> +/**
> + * load_fit: load a FIT format image
> + * @ldr: structure defining board specific properties and hooks
> + * @filename: image to load
> + * @pfdt: pointer to update with address of FDT blob

It is not clear this field is optional or mandatory.

Looking at other docstrings, optional is not always precised,
and code often consider optional if not precised. Mandatory is
mentioned explicitly.

> + * @opaque: opaque value passed back to the hook functions in @ldr
> + * Returns: 0 on success, or a negative errno on failure
> + *
> + * @pfdt is used to tell the caller about the FDT blob. On return, it
> + * has been set to point to the FDT blob, and it is now the caller's
> + * responsibility to free that memory with g_free(). Usually the caller
> + * will want to pass in &machine->fdt here, to record the FDT blob for
> + * the dumpdtb option and QMP/HMP commands.
> + */
> +int load_fit(const struct fit_loader *ldr, const char *filename, void **pfdt,
> +             void *opaque);


>   static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>                           int cfg, void *opaque, const void *match_data,
> -                        hwaddr kernel_end, Error **errp)
> +                        hwaddr kernel_end, void **pfdt, Error **errp)
>   {
>       ERRP_GUARD();
>       Error *err = NULL;
>       const char *name;
> -    const void *data;
> -    const void *load_data;
> +    void *data;
>       hwaddr load_addr;
>       int img_off;
>       size_t sz;
> @@ -194,7 +193,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>           return 0;
>       }
>   
> -    load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
> +    data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
>       if (!data) {
>           error_prepend(errp, "unable to load FDT image from FIT: ");
>           return -EINVAL;
> @@ -211,19 +210,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>       }
>   
>       if (ldr->fdt_filter) {
> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
> +        void *filtered_data;
> +
> +        filtered_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
> +        if (filtered_data != data) {
> +            g_free(data);
> +            data = filtered_data;
> +        }
>       }
>   
>       load_addr = ldr->addr_to_phys(opaque, load_addr);
> -    sz = fdt_totalsize(load_data);
> -    rom_add_blob_fixed(name, load_data, sz, load_addr);
> +    sz = fdt_totalsize(data);
> +    rom_add_blob_fixed(name, data, sz, load_addr);
>   
> -    ret = 0;
> +    *pfdt = data;

Here pfdt is assumed to be non-NULL, so a mandatory field.

Could you update the documentation? Otherwise consider adding
a non-NULL check.

Either ways:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    return 0;
>   out:
>       g_free((void *) data);
> -    if (data != load_data) {
> -        g_free((void *) load_data);
> -    }
>       return ret;
>   }
>   
> @@ -259,7 +262,8 @@ out:
>       return ret;
>   }