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.mayd...@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é <phi...@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;
}
