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

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(-)

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;
>   }

On Mon, 10 Feb 2025 at 10:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 6/2/25 16:12, Peter Maydell wrote:
> > -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.

I did go back and forth while I was writing this on whether to
make it optional or not (and the versions where it is optional,
I had a note in the documentation about that). But there is exactly
one caller of this function, and that callsite wants to pass a
non-NULL pointer, so I ended up deciding that it was pointless
to make the argument optional and include the code to handle
"pfdt is NULL".

If you get it wrong you'll find out very quickly because your
code will segfault :-)

Generally we (should) say arguments are optional when they're
optional; in those cases we also should document what the behaviour
when they're omitted is. So I think "mandatory" is the default.
In this function, ldr and filename also are mandatory. If
we mark every mandatory argument as "mandatory" then we will
end up with a lot of boilerplate markup for most arguments,
I think.

> Could you update the documentation? Otherwise consider adding
> a non-NULL check.
>
> Either ways:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

thanks
-- PMM

On 10/2/25 12:09, Peter Maydell wrote:
> On Mon, 10 Feb 2025 at 10:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 6/2/25 16:12, Peter Maydell wrote:
>>> -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.
> 
> I did go back and forth while I was writing this on whether to
> make it optional or not (and the versions where it is optional,
> I had a note in the documentation about that). But there is exactly
> one caller of this function, and that callsite wants to pass a
> non-NULL pointer, so I ended up deciding that it was pointless
> to make the argument optional and include the code to handle
> "pfdt is NULL".
> 
> If you get it wrong you'll find out very quickly because your
> code will segfault :-)
> 
> Generally we (should) say arguments are optional when they're
> optional; in those cases we also should document what the behaviour
> when they're omitted is. So I think "mandatory" is the default.
> In this function, ldr and filename also are mandatory. If
> we mark every mandatory argument as "mandatory" then we will
> end up with a lot of boilerplate markup for most arguments,
> I think.

OK, fine as is then!

> 
>> Could you update the documentation? Otherwise consider adding
>> a non-NULL check.
>>
>> Either ways:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> thanks
> -- PMM