| Message ID | 20220308194704.14061-15-fnu.vikram@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
> On 8 Mar 2022, at 19:47, Vikram Garhwal <fnu.vikram@xilinx.com> wrote: > > Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> > --- > tools/xl/xl.h | 4 ++++ > tools/xl/xl_cmdtable.c | 6 ++++++ > tools/xl/xl_vmcontrol.c | 45 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 55 insertions(+) > > diff --git a/tools/xl/xl.h b/tools/xl/xl.h > index c5c4bedbdd..604fd5bb94 100644 > --- a/tools/xl/xl.h > +++ b/tools/xl/xl.h > @@ -97,6 +97,9 @@ struct save_file_header { > > #define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL) > > +#define XL_DT_OVERLAY_ADD 1 > +#define XL_DT_OVERLAY_REMOVE 2 Maybe you can get rid of them and ... > + > void save_domain_core_begin(uint32_t domid, > int preserve_domid, > const char *override_config_file, > @@ -139,6 +142,7 @@ int main_shutdown(int argc, char **argv); > int main_reboot(int argc, char **argv); > int main_list(int argc, char **argv); > int main_vm_list(int argc, char **argv); > +int main_dt_overlay(int argc, char **argv); > int main_create(int argc, char **argv); > int main_config_update(int argc, char **argv); > int main_button_press(int argc, char **argv); > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c > index 661323d488..5812d19db8 100644 > --- a/tools/xl/xl_cmdtable.c > +++ b/tools/xl/xl_cmdtable.c > @@ -20,6 +20,12 @@ > #include "xl.h" > > const struct cmd_spec cmd_table[] = { > + { "overlay", > + &main_dt_overlay, 1, 1, > + "Add/Remove a device tree overlay", > + "add/remove <.dtbo>" > + "-h print this help\n" > + }, > { "create", > &main_create, 1, 1, > "Create a domain from config file <filename>", > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c > index 435155a033..76b969dc33 100644 > --- a/tools/xl/xl_vmcontrol.c > +++ b/tools/xl/xl_vmcontrol.c > @@ -1262,6 +1262,51 @@ int main_create(int argc, char **argv) > return 0; > } > > +int main_dt_overlay(int argc, char **argv) > +{ > + const char *overlay_ops = argv[1]; > + const char *overlay_config_file = argv[2]; > + void *overlay_dtb = NULL; > + int rc; > + uint8_t op; > + int overlay_dtb_size = 0; > + > + if (overlay_ops == NULL) { > + fprintf(stderr, "No overlay operation mode provided\n"); > + return ERROR_FAIL; > + } > + > + if (strcmp(overlay_ops, "add") == 0) > + op = XL_DT_OVERLAY_ADD; > + else if (strcmp(overlay_ops, "remove") == 0) > + op = XL_DT_OVERLAY_REMOVE; Use them there, maybe defining const int <name> = <value> Since these values are only used there
On Tue, Mar 08, 2022 at 11:47:04AM -0800, Vikram Garhwal wrote: > Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> > --- > tools/xl/xl.h | 4 ++++ > tools/xl/xl_cmdtable.c | 6 ++++++ > tools/xl/xl_vmcontrol.c | 45 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 55 insertions(+) > > diff --git a/tools/xl/xl.h b/tools/xl/xl.h > index c5c4bedbdd..604fd5bb94 100644 > --- a/tools/xl/xl.h > +++ b/tools/xl/xl.h > @@ -97,6 +97,9 @@ struct save_file_header { > > #define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL) > > +#define XL_DT_OVERLAY_ADD 1 > +#define XL_DT_OVERLAY_REMOVE 2 These value would need to be part of libxl's API, rather than been defined here. Could you create a new enum with both operation in "libxl_types.idl", then have the libxl function convert them to the hypercall operation? (So to be done in the previous patch.) > void save_domain_core_begin(uint32_t domid, > int preserve_domid, > const char *override_config_file, > @@ -139,6 +142,7 @@ int main_shutdown(int argc, char **argv); > int main_reboot(int argc, char **argv); > int main_list(int argc, char **argv); > int main_vm_list(int argc, char **argv); > +int main_dt_overlay(int argc, char **argv); > int main_create(int argc, char **argv); > int main_config_update(int argc, char **argv); > int main_button_press(int argc, char **argv); > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c > index 661323d488..5812d19db8 100644 > --- a/tools/xl/xl_cmdtable.c > +++ b/tools/xl/xl_cmdtable.c > @@ -20,6 +20,12 @@ > #include "xl.h" > > const struct cmd_spec cmd_table[] = { > + { "overlay", > + &main_dt_overlay, 1, 1, I think the first "1" needs to be a "0", because it doesn't seems that the command can do a "dry-run". > + "Add/Remove a device tree overlay", > + "add/remove <.dtbo>" > + "-h print this help\n" > + }, I don't think "overlay" is a good name for the command. Maybe "dt-overlay" ? But we seem to mostly have "*-add" "*-remove" command (or attach/detach), so maybe two new commands would be better: "dt-overlay-add" and "dt-overlay-remove" rather than using a subcommand for add/remove. Also, could you add this/those commands later in "cmd_table"? I'd rather keep "create" first when running `xl help`. So maybe at the end, or some other place that kind of make sens? > { "create", > &main_create, 1, 1, > "Create a domain from config file <filename>", > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c > index 435155a033..76b969dc33 100644 > --- a/tools/xl/xl_vmcontrol.c > +++ b/tools/xl/xl_vmcontrol.c > @@ -1262,6 +1262,51 @@ int main_create(int argc, char **argv) > return 0; > } > > +int main_dt_overlay(int argc, char **argv) > +{ > + const char *overlay_ops = argv[1]; > + const char *overlay_config_file = argv[2]; > + void *overlay_dtb = NULL; > + int rc; > + uint8_t op; > + int overlay_dtb_size = 0; > + > + if (overlay_ops == NULL) { > + fprintf(stderr, "No overlay operation mode provided\n"); > + return ERROR_FAIL; > + } > + > + if (strcmp(overlay_ops, "add") == 0) > + op = XL_DT_OVERLAY_ADD; > + else if (strcmp(overlay_ops, "remove") == 0) > + op = XL_DT_OVERLAY_REMOVE; > + else { > + fprintf(stderr, "Invalid dt overlay operation\n"); > + return ERROR_FAIL; > + } > + > + if (overlay_config_file) { > + rc = libxl_read_file_contents(ctx, overlay_config_file, > + &overlay_dtb, &overlay_dtb_size); > + > + if (rc) { > + fprintf(stderr, "failed to read the overlay device tree file %s\n", > + overlay_config_file); > + free(overlay_dtb); > + return ERROR_FAIL; > + } > + } else { > + fprintf(stderr, "overlay dtbo file not provided\n"); Instead of making out of bound access to "argv", you could check that "argc" have the expected value, and if not, print the help of the command. If you look at main_save() for example, there is: "if(argc is wrong value){help("save");} which would print the help for the command. > + return ERROR_FAIL; > + } > + > + rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op); > + if (rc) > + fprintf(stderr, "Overlay operation failed\n"); I'm not sure that this error message would be useful, as libxl should already have printed something. > + > + free(overlay_dtb); > + return rc; > +} Thanks,
diff --git a/tools/xl/xl.h b/tools/xl/xl.h index c5c4bedbdd..604fd5bb94 100644 --- a/tools/xl/xl.h +++ b/tools/xl/xl.h @@ -97,6 +97,9 @@ struct save_file_header { #define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL) +#define XL_DT_OVERLAY_ADD 1 +#define XL_DT_OVERLAY_REMOVE 2 + void save_domain_core_begin(uint32_t domid, int preserve_domid, const char *override_config_file, @@ -139,6 +142,7 @@ int main_shutdown(int argc, char **argv); int main_reboot(int argc, char **argv); int main_list(int argc, char **argv); int main_vm_list(int argc, char **argv); +int main_dt_overlay(int argc, char **argv); int main_create(int argc, char **argv); int main_config_update(int argc, char **argv); int main_button_press(int argc, char **argv); diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c index 661323d488..5812d19db8 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -20,6 +20,12 @@ #include "xl.h" const struct cmd_spec cmd_table[] = { + { "overlay", + &main_dt_overlay, 1, 1, + "Add/Remove a device tree overlay", + "add/remove <.dtbo>" + "-h print this help\n" + }, { "create", &main_create, 1, 1, "Create a domain from config file <filename>", diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c index 435155a033..76b969dc33 100644 --- a/tools/xl/xl_vmcontrol.c +++ b/tools/xl/xl_vmcontrol.c @@ -1262,6 +1262,51 @@ int main_create(int argc, char **argv) return 0; } +int main_dt_overlay(int argc, char **argv) +{ + const char *overlay_ops = argv[1]; + const char *overlay_config_file = argv[2]; + void *overlay_dtb = NULL; + int rc; + uint8_t op; + int overlay_dtb_size = 0; + + if (overlay_ops == NULL) { + fprintf(stderr, "No overlay operation mode provided\n"); + return ERROR_FAIL; + } + + if (strcmp(overlay_ops, "add") == 0) + op = XL_DT_OVERLAY_ADD; + else if (strcmp(overlay_ops, "remove") == 0) + op = XL_DT_OVERLAY_REMOVE; + else { + fprintf(stderr, "Invalid dt overlay operation\n"); + return ERROR_FAIL; + } + + if (overlay_config_file) { + rc = libxl_read_file_contents(ctx, overlay_config_file, + &overlay_dtb, &overlay_dtb_size); + + if (rc) { + fprintf(stderr, "failed to read the overlay device tree file %s\n", + overlay_config_file); + free(overlay_dtb); + return ERROR_FAIL; + } + } else { + fprintf(stderr, "overlay dtbo file not provided\n"); + return ERROR_FAIL; + } + + rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op); + if (rc) + fprintf(stderr, "Overlay operation failed\n"); + + free(overlay_dtb); + return rc; +} /* * Local variables: * mode: C
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> --- tools/xl/xl.h | 4 ++++ tools/xl/xl_cmdtable.c | 6 ++++++ tools/xl/xl_vmcontrol.c | 45 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+)