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,
