Re: [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Wed, Apr 24, 2024 at 11:34:43AM +0800, Henry Wang wrote:
> diff --git a/tools/libs/light/libxl_dt_overlay.c 
> b/tools/libs/light/libxl_dt_overlay.c
> index cdb62b28cf..eaf11a0f9c 100644
> --- a/tools/libs/light/libxl_dt_overlay.c
> +++ b/tools/libs/light/libxl_dt_overlay.c
> @@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, 
> size_t size)
>      return 0;
>  }
>  
> +static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
> +                                   size_t size)
> +{
> +    int rc = 0;
> +    int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
> +    const struct fdt_property *fdt_prop_node = NULL;
> +    int overlay;
> +    int prop_len = 0;
> +    int subnode = 0;
> +    int fragment;
> +    const char *prop_name;
> +    const char *target_path = "/";
> +
> +    fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
> +        prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
> +                                &prop_len);
> +        if (prop_name == NULL) {
> +            LOG(ERROR, "target-path property not found\n");

LOG* macros already takes care of adding \n, no need to add an extra
one.

> +            rc = ERROR_FAIL;
> +            goto err;
> +        }
> +
> +        /* Change target path for domU dtb. */
> +        rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",

fdt_setprop_string() isn't a libxl function, store the return value in a
variable named `r` instead.

> +                                target_path);
> +        if (rc) {
> +            LOG(ERROR, "Setting interrupt parent property failed for %s\n",
> +                prop_name);
> +            goto err;
> +        }
> +
> +        overlay = fdt_subnode_offset(overlay_dt_domU, fragment, 
> "__overlay__");
> +
> +        fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
> +        {
> +            const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
> +                                                 NULL);
> +
> +            fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
> +                                        "interrupt-parent", &prop_len);
> +            if (fdt_prop_node == NULL) {
> +                LOG(DETAIL, "%s property not found for %s. Skip to next 
> node\n",
> +                    "interrupt-parent", node_name);

Why do you have "interrupt-parent" in a separate argument? Do you meant
to do something like
    const char *some_name = "interrupt-parent";
and use that in the 4 different places that this string is used? (Using
a variable mean that we (or the compiler) can make sure that they are
all spelled correctly.

> +                continue;
> +            }
> +
> +            rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
> +                                         "interrupt-parent",
> +                                         virtual_interrupt_parent);
> +            if (rc) {
> +                LOG(ERROR, "Setting interrupt parent property failed for 
> %s\n",
> +                    "interrupt-parent");
> +                goto err;
> +            }
> +        }
> +    }
> +
> +return 0;

Missed indentation.

> +
> +err:
> +    return rc;

A few things, looks like `rc` is always going to be ERROR_FAIL here,
unless you find an libxl_error code that better describe the error, so
you could forgo the `rc` variable.

Also, if you don't need to clean up anything in the function or have a
generic error message, you could simply "return " instead of using the
"goto" style.

> +}
> +
>  int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
>                       uint32_t overlay_dt_size, uint8_t overlay_op,
>                       bool auto_mode, bool domain_mapping)
> @@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void 
> *overlay_dt,
>          rc = ERROR_FAIL;
>      }
>  
> +    /*
> +     * auto_mode doesn't apply to dom0 as dom0 can get the physical
> +     * description of the hardware.
> +     */
> +    if (domid && auto_mode) {
> +        if (overlay_op == LIBXL_DT_OVERLAY_ADD)

Shouldn't libxl complain if the operation is different?

> +            rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
> +    }
> +
>  out:
>      GC_FREE;
>      return rc;

Thanks,

-- 
Anthony PERARD