| Message ID | 20240424033449.168398-10-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remaining patches for dynamic node programming using overlay dtbo | expand |
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,
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, >
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 @@ -17,6 +17,7 @@ #include "libxl_internal.h" #include <libfdt.h> #include <xenctrl.h> +#include <xen/device_tree_defs.h> static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size) { @@ -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"); + rc = ERROR_FAIL; + goto err; + } + + /* Change target path for domU dtb. */ + rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path", + 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); + 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; + +err: + return rc; +} + 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) + rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size); + } + out: GC_FREE; return rc;