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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Anthony,

On 5/1/2024 11:09 PM, Anthony PERARD wrote:
> 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.
Sure, I will remove the "\n".

> > +            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.'
Thanks for spotting this. Will change it to `r`.

> > +                                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.
Great suggestion! I will do this way.

> > +                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.
Will correct it.

> > +
> > +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.
Sure, I will simply use return because I don't really think there is 
anything to be cleaned up.
> > +}
> > +
> >   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?
I will add corresponding error handling code here. Thanks!

Kind regards,
Henry

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