On Fri, Mar 16, 2018 at 06:38:19PM +0530, Srivatsa Vaddagiri wrote:
> When overlaying onto a base blob which itself has unresolved symbols as
> mentioned in its __fixups__ section, its possible that not all external
> references specified in overlay blob's __fixups__ section are satisfied by base
> blob. Ignore such unresolved references (rather than aborting the whole overlay
> process upon the first instance of unresolved symbol found).
>
> Further for any external reference satisfied by base blob, delete the external
> reference found in overlay blob's __fixups__ section and add that reference in
> __local_fixups__ section of base blob. This will be because __fixups__ section
> of both blobs will need to be merged eventually and need to reflect only
> unresolved external references of the combined blob.
>
> Signed-off-by: Srivatsa Vaddagiri <vatsa@xxxxxxxxxxxxxx>
I could go through with detailed comments here, but at this point I
don't think it's worth it. These patches need a *lot* of work before
they're ready.
Please at least fix the following, then repost for another round of
review:
* Use a separate entry point for overlay merging
* Don't rely on malloc() / realloc()
* Don't use global variables
> ---
> libfdt/fdt_overlay.c | 256 +++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 250 insertions(+), 6 deletions(-)
>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 556551e..560bb27 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -145,6 +145,46 @@ static int fdt_set_name_memcheck(void **fdt, int offset,
> return fdt_set_name(*fdt, offset, name);
> }
>
> +static int new_node_len(const char *name)
> +{
> + /* FDT_BEGIN_NODE, node name in off_struct and FDT_END_NODE */
> + return sizeof(struct fdt_node_header) + ALIGN(strlen(name) + 1)
> + + FDT_TAGSIZE;
> +}
> +
> +static int fdt_add_subnode_memcheck(void **fdt, int nodeoffset,
> + const char *nodename)
> +{
> + int offset;
> +
> + offset = fdt_add_subnode(*fdt, nodeoffset, nodename);
> + if (offset > 0)
> + return offset;
> +
> + if (offset < 0 && offset != -FDT_ERR_NOSPACE)
> + return offset;
> +
> + *fdt = realloc_fdt(*fdt, new_node_len(nodename));
> +
> + return fdt_add_subnode(*fdt, nodeoffset, nodename);
> +}
> +
> +static int fdt_setprop_u32_memcheck(void **fdt, int offset,
> + const char *propname, int proplen, uint32_t val)
> +{
> + int err;
> +
> + err = fdt_setprop_u32(*fdt, offset, propname, val);
> + if (!err)
> + return 0;
> +
> + if (err != -FDT_ERR_NOSPACE)
> + return err;
> +
> + *fdt = realloc_fdt(*fdt, proplen);
> + return fdt_setprop_u32(*fdt, offset, propname, val);
> +}
> +
> /**
> * fdt_setprop_placeholder_memcheck - Set property value
> *
> @@ -573,6 +613,207 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> sizeof(phandle_prop));
> };
>
> +char **del_property;
> +int del_prop_size = 100 * sizeof(char *);
> +int del_idx;
> +
> +/*
> + * In case of base blob itself having unresolved external references (non-empty
> + * fixup section), we want to merge the fixup sections of all blobs, excluding
> + * those entries that could be resolved during overlay process.
> + * 'mark_for_deletion' marks those properties in fixup section that were
> + * successfully resolved. 'delete_fixed_properties' removes the resolved entries
> + * from 'fixup' section of overlay blob (so that when we finally merge 'fixups'
> + * sections of all blobs, the resolved entries don't appear in final fixups
> + * section) and adds them to 'local_fixup' section of base blob
> + */
> +static int delete_fixed_properties(void **fdt, void *fdto, int fixups_off)
> +{
> + int idx, localfixup_off, rc, del_prop = 0;
> +
> + localfixup_off = fdt_path_offset(*fdt, "/__local_fixups__");
> + if ((localfixup_off < 0 && (localfixup_off != -FDT_ERR_NOTFOUND))) {
> + rc = localfixup_off;
> + goto cleanup;
> + }
> +
> + if (localfixup_off == -FDT_ERR_NOTFOUND) {
> + localfixup_off = fdt_add_subnode_memcheck(fdt, 0, "__local_fixups__");
> + if (localfixup_off < 0) {
> + rc = localfixup_off;
> + goto cleanup;
> + }
> + }
> +
> + for (idx = 0; idx < del_idx; ++idx) {
> + const char *value;
> + int len, pathlen, proplen;
> +
> + value = fdt_getprop(fdto, fixups_off, del_property[idx], &len);
> + if (!value) {
> + rc = len;
> + goto cleanup;
> + }
> +
> + /*
> + * Properties found in __fixups__ section are typically one of
> + * these:
> + *
> + * abc = "/fragment@2:target:0"; OR
> + * abc = "/fragment@2/__overlay__:xyz:0"
> + *
> + * Only in case of latter, we would have inserted a phandle
> + * reference in some node's property, which needs to be
> + * reflected in combined blob's __local_fixup__ section
> + */
> +
> + dprintf("property %s is being added to local_fixups\n",
> + del_property[idx]);
> + while (len > 0) {
> + const char *path, *fixup_end, *prop;
> + const char *fixup_str = value;
> + uint32_t clen;
> + uint32_t fixup_len;
> + char *sep, *endptr;
> + const char *c;
> + int poffset;
> + int nodeoffset;
> + char propname[PATH_MAX];
> +
> + fixup_end = memchr(value, '\0', len);
> + if (!fixup_end)
> + break;
> +
> + fixup_len = fixup_end - fixup_str;
> +
> + len -= (fixup_len + 1);
> + value += fixup_len + 1;
> +
> + c = path = fixup_str;
> + sep = memchr(c, ':', fixup_len);
> + if (!sep || *sep != ':') {
> + rc = -FDT_ERR_BADOVERLAY;
> + goto cleanup;
> + }
> + pathlen = sep - path;
> + if (pathlen == (fixup_len - 1)) {
> + rc = -FDT_ERR_BADOVERLAY;
> + goto cleanup;
> + }
> +
> + fixup_len -= (pathlen + 1);
> + c = path + pathlen + 1;
> +
> + sep = memchr(c, ':', fixup_len);
> + if (!sep || *sep != ':') {
> + rc = -FDT_ERR_BADOVERLAY;
> + goto cleanup;
> + }
> +
> + prop = c;
> + proplen = sep - c;
> +
> + if (proplen >= PATH_MAX) {
> + rc = -FDT_ERR_BADOVERLAY;
> + goto cleanup;
> + }
> +
> + if (!strncmp(prop, "target", proplen))
> + continue;
> +
> + memcpy(propname, prop, proplen);
> + propname[proplen] = 0;
> +
> + fixup_len -= (proplen + 1);
> + c = prop + proplen + 1;
> + poffset = strtoul(c, &endptr, 10);
> +
> + nodeoffset = localfixup_off;
> +
> + c = path;
> + clen = pathlen;
> +
> + if (*c == '/') {
> + c++;
> + clen -= 1;
> + }
> +
> + while (clen > 0) {
> + char nodename[PATH_MAX];
> + int nodelen;
> +
> + sep = memchr(c, '/', clen);
> + if (!sep)
> + nodelen = clen;
> + else
> + nodelen = sep - c;
> +
> + if (nodelen + 1 >= PATH_MAX) {
> + dprintf("Very large node name %s\n", c);
> + rc = -FDT_ERR_BADSTRUCTURE;
> + goto cleanup;
> + }
> + memcpy(nodename, c, nodelen);
> + nodename[nodelen] = 0;
> +
> + nodeoffset = fdt_add_subnode_memcheck(fdt, nodeoffset, nodename);
> + if (nodeoffset < 0) {
> + rc = nodeoffset;
> + goto cleanup;
> + }
> +
> + c += nodelen;
> + clen -= nodelen;
> +
> + if (*c == '/') {
> + c++;
> + clen -= 1;
> + }
> + }
> +
> + rc = fdt_setprop_u32_memcheck(fdt, nodeoffset, propname, proplen, poffset);
> + if (rc < 0)
> + goto cleanup;
> + }
> + }
> +
> + del_prop = 1;
> + rc = 0;
> +
> +cleanup:
> + for (idx = 0; idx < del_idx; ++idx) {
> + if (del_prop)
> + rc = fdt_delprop(fdto, fixups_off, del_property[idx]);
> +
> + free(del_property[idx]);
> + }
> +
> + /* Reset index before the next blob gets overlaid */
> + del_idx = 0;
> +
> + return rc;
> +}
> +
> +static void mark_for_deletion(const char *name)
> +{
> + int len = strlen(name) + 1;
> +
> + if (!del_property)
> + del_property = xmalloc(del_prop_size);
> +
> + /*
> + * Perhaps we can note down the offset of prop to be deleted rather than
> + * its name? Will avoid the need for malloc()
> + */
> + del_property[del_idx] = xmalloc(len);
> + memcpy(del_property[del_idx], name, len);
> +
> + if (++del_idx >= del_prop_size/sizeof(char *)) {
> + del_property = xrealloc(del_property, del_prop_size + 512);
> + del_prop_size += 512;
> + }
> +}
> +
> /**
> * overlay_fixup_phandle - Set an overlay phandle to the base one
> * @fdt: Base Device Tree blob
> @@ -654,6 +895,9 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> return ret;
> } while (len > 0);
>
> + if (base_has_fixups)
> + mark_for_deletion(label);
> +
> return 0;
> }
>
> @@ -674,7 +918,7 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> * 0 on success
> * Negative error code on failure
> */
> -static int overlay_fixup_phandles(void *fdt, void *fdto)
> +static int overlay_fixup_phandles(void **fdt, void *fdto)
> {
> int fixups_off, symbols_off;
> int property;
> @@ -687,19 +931,19 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
> return fixups_off;
>
> /* And base DTs without symbols */
> - symbols_off = fdt_path_offset(fdt, "/__symbols__");
> + symbols_off = fdt_path_offset(*fdt, "/__symbols__");
> if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND)))
> return symbols_off;
>
> fdt_for_each_property_offset(property, fdto, fixups_off) {
> int ret;
>
> - ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> - if (ret)
> + ret = overlay_fixup_phandle(*fdt, fdto, symbols_off, property);
> + if (ret && (!base_has_fixups || ret != -FDT_ERR_NOTFOUND))
> return ret;
> }
>
> - return 0;
> + return delete_fixed_properties(fdt, fdto, fixups_off);
> }
>
> /**
> @@ -1283,7 +1527,7 @@ int fdt_overlay_apply(void **fdt, void **fdto)
> goto err;
>
> dprintf("Fixing phandles\n");
> - ret = overlay_fixup_phandles(*fdt, *fdto);
> + ret = overlay_fixup_phandles(fdt, *fdto);
> if (ret)
> goto err;
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
