| Message ID | 20240424033449.168398-9-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:42AM +0800, Henry Wang wrote: > From: Vikram Garhwal <fnu.vikram@xilinx.com> > > Add domain_id and expert mode for overlay assignment. This enables dynamic > programming of nodes during runtime. > > Take the opportunity to fix the name mismatch in the xl command, the > command name should be "dt-overlay" instead of "dt_overlay". I don't like much these unrelated / opportunistic changes in a patch, I'd rather have a separate patch. And in this case, if it was on a separate patch, that separated patch could gain: Fixes: 61765a07e3d8 ("tools/xl: Add new xl command overlay for device tree overlay support") and potentially backported. > Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > tools/include/libxl.h | 8 +++++-- > tools/include/xenctrl.h | 5 +++-- > tools/libs/ctrl/xc_dt_overlay.c | 7 ++++-- > tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++---- > tools/xl/xl_vmcontrol.c | 34 ++++++++++++++++++++++++++--- > 5 files changed, 58 insertions(+), 13 deletions(-) > > diff --git a/tools/include/libxl.h b/tools/include/libxl.h > index 62cb07dea6..59a3e1b37c 100644 > --- a/tools/include/libxl.h > +++ b/tools/include/libxl.h > @@ -2549,8 +2549,12 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, > void libxl_device_pci_list_free(libxl_device_pci* list, int num); > > #if defined(__arm__) || defined(__aarch64__) > -int libxl_dt_overlay(libxl_ctx *ctx, void *overlay, > - uint32_t overlay_size, uint8_t overlay_op); > +#define LIBXL_DT_OVERLAY_ADD 1 > +#define LIBXL_DT_OVERLAY_REMOVE 2 > + > +int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay, > + uint32_t overlay_size, uint8_t overlay_op, bool auto_mode, > + bool domain_mapping); Sorry, you cannot change the API of an existing libxl function without providing something backward compatible. We have already a few example of this changes in libxl.h, e.g.: fded24ea8315 ("libxl: Make libxl_set_vcpuonline async") So, providing a wrapper called libxl_dt_overlay_0x041800() which call the new function. > #endif > > /* > diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c > index a6c709a6dc..cdb62b28cf 100644 > --- a/tools/libs/light/libxl_dt_overlay.c > +++ b/tools/libs/light/libxl_dt_overlay.c > @@ -57,10 +58,18 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size, > rc = 0; > } > > - r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op); > + /* Check if user entered a valid domain id. */ > + rc = libxl_domain_info(CTX, NULL, domid); > + if (rc == ERROR_DOMAIN_NOTFOUND) { Why do you check specifically for "domain not found", what about other error? > + LOGD(ERROR, domid, "Non-existant domain."); > + return ERROR_FAIL; Use `goto out`, and you can let the function return ERROR_DOMAIN_NOTFOUND if that the error, we can just propagate the `rc` from libxl_domain_info(). > + } > + > + r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op, > + domain_mapping); > > if (r) { > - LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__); > + LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid); You could replace the macro by LOGD, instead of handwriting "domain%d". > rc = ERROR_FAIL; > } > > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c > index 98f6bd2e76..9674383ec3 100644 > --- a/tools/xl/xl_vmcontrol.c > +++ b/tools/xl/xl_vmcontrol.c > @@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv) > { > const char *overlay_ops = NULL; > const char *overlay_config_file = NULL; > + uint32_t domain_id = 0; > void *overlay_dtb = NULL; > int rc; > + bool auto_mode = true; > + bool domain_mapping = false; > uint8_t op; > int overlay_dtb_size = 0; > const int overlay_add_op = 1; > const int overlay_remove_op = 2; > > - if (argc < 2) { > - help("dt_overlay"); > + if (argc < 3) { > + help("dt-overlay"); > return EXIT_FAILURE; > } > > + if (argc > 5) { > + fprintf(stderr, "Too many arguments\n"); > + return ERROR_FAIL; > + } > + > overlay_ops = argv[1]; > overlay_config_file = argv[2]; > > + if (!strcmp(argv[argc - 1], "-e")) > + auto_mode = false; > + > + if (argc == 4 || !auto_mode) { > + domain_id = find_domain(argv[argc-1]); > + domain_mapping = true; > + } > + > + if (argc == 5 || !auto_mode) { > + domain_id = find_domain(argv[argc-2]); > + domain_mapping = true; > + } Sorry, I can't review that changes, this needs a change in the help message of dt-overlay, and something in the man page. (and that argument parsing looks convoluted). > + > + /* User didn't prove any overlay operation. */ I guess you meant "provide" instead of prove. But the comment can be discarded, it doesn't explain anything useful that the error message doesn't already explain. > + if (overlay_ops == NULL) { > + fprintf(stderr, "No overlay operation mode provided\n"); > + return ERROR_FAIL; That should be EXIT_FAILURE instead. (and I realise that the function already return ERROR_FAIL by mistake in several places. (ERROR_FAIL is a libxl error return value of -3, which isn't really a good exit value for CLI programmes.)) Thanks,
Hi Anthony, On 5/1/2024 10:46 PM, Anthony PERARD wrote: > On Wed, Apr 24, 2024 at 11:34:42AM +0800, Henry Wang wrote: >> From: Vikram Garhwal <fnu.vikram@xilinx.com> >> >> Add domain_id and expert mode for overlay assignment. This enables dynamic >> programming of nodes during runtime. >> >> Take the opportunity to fix the name mismatch in the xl command, the >> command name should be "dt-overlay" instead of "dt_overlay". > I don't like much these unrelated / opportunistic changes in a patch, > I'd rather have a separate patch. And in this case, if it was on a > separate patch, that separated patch could gain: Fixes: 61765a07e3d8 > ("tools/xl: Add new xl command overlay for device tree overlay support") > and potentially backported. Ok. I can split this part to a separated commit. >> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >> Signed-off-by: Henry Wang <xin.wang2@amd.com> >> --- >> tools/include/libxl.h | 8 +++++-- >> tools/include/xenctrl.h | 5 +++-- >> tools/libs/ctrl/xc_dt_overlay.c | 7 ++++-- >> tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++---- >> tools/xl/xl_vmcontrol.c | 34 ++++++++++++++++++++++++++--- >> 5 files changed, 58 insertions(+), 13 deletions(-) >> >> diff --git a/tools/include/libxl.h b/tools/include/libxl.h >> index 62cb07dea6..59a3e1b37c 100644 >> --- a/tools/include/libxl.h >> +++ b/tools/include/libxl.h >> @@ -2549,8 +2549,12 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, >> void libxl_device_pci_list_free(libxl_device_pci* list, int num); >> >> #if defined(__arm__) || defined(__aarch64__) >> -int libxl_dt_overlay(libxl_ctx *ctx, void *overlay, >> - uint32_t overlay_size, uint8_t overlay_op); >> +#define LIBXL_DT_OVERLAY_ADD 1 >> +#define LIBXL_DT_OVERLAY_REMOVE 2 >> + >> +int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay, >> + uint32_t overlay_size, uint8_t overlay_op, bool auto_mode, >> + bool domain_mapping); > Sorry, you cannot change the API of an existing libxl function without > providing something backward compatible. We have already a few example > of this changes in libxl.h, e.g.: fded24ea8315 ("libxl: Make libxl_set_vcpuonline async") > So, providing a wrapper called libxl_dt_overlay_0x041800() which call > the new function. Ok, I will add an wrapper. >> #endif >> >> /* >> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c >> index a6c709a6dc..cdb62b28cf 100644 >> --- a/tools/libs/light/libxl_dt_overlay.c >> +++ b/tools/libs/light/libxl_dt_overlay.c >> @@ -57,10 +58,18 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size, >> rc = 0; >> } >> >> - r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op); >> + /* Check if user entered a valid domain id. */ >> + rc = libxl_domain_info(CTX, NULL, domid); >> + if (rc == ERROR_DOMAIN_NOTFOUND) { > Why do you check specifically for "domain not found", what about other > error? I agree this is indeed very confusing...I will rewrite this part properly in the next version. >> + LOGD(ERROR, domid, "Non-existant domain."); >> + return ERROR_FAIL; > Use `goto out`, and you can let the function return > ERROR_DOMAIN_NOTFOUND if that the error, we can just propagate the `rc` > from libxl_domain_info(). Sure, will do the suggested way. >> + } >> + >> + r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op, >> + domain_mapping); >> >> if (r) { >> - LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__); >> + LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid); > You could replace the macro by LOGD, instead of handwriting "domain%d". Great suggestion. I will use LOGD. >> rc = ERROR_FAIL; >> } >> >> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c >> index 98f6bd2e76..9674383ec3 100644 >> --- a/tools/xl/xl_vmcontrol.c >> +++ b/tools/xl/xl_vmcontrol.c >> @@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv) >> { >> const char *overlay_ops = NULL; >> const char *overlay_config_file = NULL; >> + uint32_t domain_id = 0; >> void *overlay_dtb = NULL; >> int rc; >> + bool auto_mode = true; >> + bool domain_mapping = false; >> uint8_t op; >> int overlay_dtb_size = 0; >> const int overlay_add_op = 1; >> const int overlay_remove_op = 2; >> >> - if (argc < 2) { >> - help("dt_overlay"); >> + if (argc < 3) { >> + help("dt-overlay"); >> return EXIT_FAILURE; >> } >> >> + if (argc > 5) { >> + fprintf(stderr, "Too many arguments\n"); >> + return ERROR_FAIL; >> + } >> + >> overlay_ops = argv[1]; >> overlay_config_file = argv[2]; >> >> + if (!strcmp(argv[argc - 1], "-e")) >> + auto_mode = false; >> + >> + if (argc == 4 || !auto_mode) { >> + domain_id = find_domain(argv[argc-1]); >> + domain_mapping = true; >> + } >> + >> + if (argc == 5 || !auto_mode) { >> + domain_id = find_domain(argv[argc-2]); >> + domain_mapping = true; >> + } > Sorry, I can't review that changes, this needs a change in the help > message of dt-overlay, and something in the man page. (and that argument > parsing looks convoluted). I will rework this part to see if I can make it better. >> + >> + /* User didn't prove any overlay operation. */ > I guess you meant "provide" instead of prove. But the comment can be > discarded, it doesn't explain anything useful that the error message > doesn't already explain. I think your comment is correct, I will just drop it. >> + if (overlay_ops == NULL) { >> + fprintf(stderr, "No overlay operation mode provided\n"); >> + return ERROR_FAIL; > That should be EXIT_FAILURE instead. (and I realise that the function > already return ERROR_FAIL by mistake in several places. (ERROR_FAIL is a > libxl error return value of -3, which isn't really a good exit value for > CLI programmes.)) Thanks. Will use EXIT_FAILURE. Kind regards, Henry > > Thanks, >
diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 62cb07dea6..59a3e1b37c 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -2549,8 +2549,12 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, void libxl_device_pci_list_free(libxl_device_pci* list, int num); #if defined(__arm__) || defined(__aarch64__) -int libxl_dt_overlay(libxl_ctx *ctx, void *overlay, - uint32_t overlay_size, uint8_t overlay_op); +#define LIBXL_DT_OVERLAY_ADD 1 +#define LIBXL_DT_OVERLAY_REMOVE 2 + +int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay, + uint32_t overlay_size, uint8_t overlay_op, bool auto_mode, + bool domain_mapping); #endif /* diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 2ef8b4e054..3861d0a23f 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -2654,8 +2654,9 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t domid, xen_pfn_t start_pfn, xen_pfn_t nr_pfns); #if defined(__arm__) || defined(__aarch64__) -int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, - uint32_t overlay_fdt_size, uint8_t overlay_op); +int xc_dt_overlay(xc_interface *xch, uint32_t domain_id, void *overlay_fdt, + uint32_t overlay_fdt_size, uint8_t overlay_op, + bool domain_mapping); #endif /* Compat shims */ diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c index c2224c4d15..a1dc549915 100644 --- a/tools/libs/ctrl/xc_dt_overlay.c +++ b/tools/libs/ctrl/xc_dt_overlay.c @@ -20,15 +20,18 @@ #include "xc_private.h" -int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, - uint32_t overlay_fdt_size, uint8_t overlay_op) +int xc_dt_overlay(xc_interface *xch, uint32_t domid, void *overlay_fdt, + uint32_t overlay_fdt_size, uint8_t overlay_op, + bool domain_mapping) { int err; struct xen_sysctl sysctl = { .cmd = XEN_SYSCTL_dt_overlay, .u.dt_overlay = { + .domain_id = domid, .overlay_op = overlay_op, .overlay_fdt_size = overlay_fdt_size, + .domain_mapping = domain_mapping, } }; diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c index a6c709a6dc..cdb62b28cf 100644 --- a/tools/libs/light/libxl_dt_overlay.c +++ b/tools/libs/light/libxl_dt_overlay.c @@ -41,8 +41,9 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size) return 0; } -int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size, - uint8_t overlay_op) +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) { int rc; int r; @@ -57,10 +58,18 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size, rc = 0; } - r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op); + /* Check if user entered a valid domain id. */ + rc = libxl_domain_info(CTX, NULL, domid); + if (rc == ERROR_DOMAIN_NOTFOUND) { + LOGD(ERROR, domid, "Non-existant domain."); + return ERROR_FAIL; + } + + r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op, + domain_mapping); if (r) { - LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__); + LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid); rc = ERROR_FAIL; } diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c index 98f6bd2e76..9674383ec3 100644 --- a/tools/xl/xl_vmcontrol.c +++ b/tools/xl/xl_vmcontrol.c @@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv) { const char *overlay_ops = NULL; const char *overlay_config_file = NULL; + uint32_t domain_id = 0; void *overlay_dtb = NULL; int rc; + bool auto_mode = true; + bool domain_mapping = false; uint8_t op; int overlay_dtb_size = 0; const int overlay_add_op = 1; const int overlay_remove_op = 2; - if (argc < 2) { - help("dt_overlay"); + if (argc < 3) { + help("dt-overlay"); return EXIT_FAILURE; } + if (argc > 5) { + fprintf(stderr, "Too many arguments\n"); + return ERROR_FAIL; + } + overlay_ops = argv[1]; overlay_config_file = argv[2]; + if (!strcmp(argv[argc - 1], "-e")) + auto_mode = false; + + if (argc == 4 || !auto_mode) { + domain_id = find_domain(argv[argc-1]); + domain_mapping = true; + } + + if (argc == 5 || !auto_mode) { + domain_id = find_domain(argv[argc-2]); + domain_mapping = true; + } + + /* User didn't prove any overlay operation. */ + if (overlay_ops == NULL) { + fprintf(stderr, "No overlay operation mode provided\n"); + return ERROR_FAIL; + } + if (strcmp(overlay_ops, "add") == 0) op = overlay_add_op; else if (strcmp(overlay_ops, "remove") == 0) @@ -1309,7 +1336,8 @@ int main_dt_overlay(int argc, char **argv) return ERROR_FAIL; } - rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op); + rc = libxl_dt_overlay(ctx, domain_id, overlay_dtb, overlay_dtb_size, op, + auto_mode, domain_mapping); free(overlay_dtb);