| Message ID | 20230906011631.30310-21-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 06.09.2023 03:16, Vikram Garhwal wrote: > --- a/tools/xl/xl_vmcontrol.c > +++ b/tools/xl/xl_vmcontrol.c > @@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv) > return 0; > } > > +int main_dt_overlay(int argc, char **argv) > +{ > + const char *overlay_ops = NULL; > + const char *overlay_config_file = NULL; > + void *overlay_dtb = NULL; > + int rc; > + 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"); > + return EXIT_FAILURE; > + } > + > + overlay_ops = argv[1]; > + overlay_config_file = argv[2]; > + > + if (strcmp(overlay_ops, "add") == 0) > + op = overlay_add_op; > + else if (strcmp(overlay_ops, "remove") == 0) > + op = overlay_remove_op; > + else { > + fprintf(stderr, "Invalid dt overlay operation\n"); > + return EXIT_FAILURE; > + } > + > + 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); Because of this being Arm-only (as validly pointed out by osstest), I expect the entire function here as well as its entry in cmd_table[] want to be Arm-specific, too? Of course it would be nice to not key this to __arm__ / __aarch64__, but to something that would not need touching again if the underlying infrastructure was made available to, say, RISC-V as well. But of course - right now the goal needs to be to address the CI and osstest breakage. Jan
On 06/09/2023 08:55, Jan Beulich wrote: > > > On 06.09.2023 03:16, Vikram Garhwal wrote: >> --- a/tools/xl/xl_vmcontrol.c >> +++ b/tools/xl/xl_vmcontrol.c >> @@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv) >> return 0; >> } >> >> +int main_dt_overlay(int argc, char **argv) >> +{ >> + const char *overlay_ops = NULL; >> + const char *overlay_config_file = NULL; >> + void *overlay_dtb = NULL; >> + int rc; >> + 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"); >> + return EXIT_FAILURE; >> + } >> + >> + overlay_ops = argv[1]; >> + overlay_config_file = argv[2]; >> + >> + if (strcmp(overlay_ops, "add") == 0) >> + op = overlay_add_op; >> + else if (strcmp(overlay_ops, "remove") == 0) >> + op = overlay_remove_op; >> + else { >> + fprintf(stderr, "Invalid dt overlay operation\n"); >> + return EXIT_FAILURE; >> + } >> + >> + 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); > > Because of this being Arm-only (as validly pointed out by osstest), I expect > the entire function here as well as its entry in cmd_table[] want to be > Arm-specific, too? Of course it would be nice to not key this to __arm__ / > __aarch64__, but to something that would not need touching again if the > underlying infrastructure was made available to, say, RISC-V as well. But of > course - right now the goal needs to be to address the CI and osstest > breakage. I agree. I would suggest to guard it with LIBXL_HAVE_DT_OVERLAY which is for now only defined for arm32/arm64. This way the code will not need to be modified if other arch gain support for the feature. If you agree, I can send a patch to unbreak CI unless you want to do this. ~Michal
On 06.09.2023 09:32, Michal Orzel wrote: > > > On 06/09/2023 08:55, Jan Beulich wrote: >> >> >> On 06.09.2023 03:16, Vikram Garhwal wrote: >>> --- a/tools/xl/xl_vmcontrol.c >>> +++ b/tools/xl/xl_vmcontrol.c >>> @@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv) >>> return 0; >>> } >>> >>> +int main_dt_overlay(int argc, char **argv) >>> +{ >>> + const char *overlay_ops = NULL; >>> + const char *overlay_config_file = NULL; >>> + void *overlay_dtb = NULL; >>> + int rc; >>> + 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"); >>> + return EXIT_FAILURE; >>> + } >>> + >>> + overlay_ops = argv[1]; >>> + overlay_config_file = argv[2]; >>> + >>> + if (strcmp(overlay_ops, "add") == 0) >>> + op = overlay_add_op; >>> + else if (strcmp(overlay_ops, "remove") == 0) >>> + op = overlay_remove_op; >>> + else { >>> + fprintf(stderr, "Invalid dt overlay operation\n"); >>> + return EXIT_FAILURE; >>> + } >>> + >>> + 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); >> >> Because of this being Arm-only (as validly pointed out by osstest), I expect >> the entire function here as well as its entry in cmd_table[] want to be >> Arm-specific, too? Of course it would be nice to not key this to __arm__ / >> __aarch64__, but to something that would not need touching again if the >> underlying infrastructure was made available to, say, RISC-V as well. But of >> course - right now the goal needs to be to address the CI and osstest >> breakage. > I agree. I would suggest to guard it with LIBXL_HAVE_DT_OVERLAY which is for now > only defined for arm32/arm64. This way the code will not need to be modified if other > arch gain support for the feature. Ah yes, that ought to work. While there perhaps also replace the conditional around the declaration of the function in libxl.h. (But of course Anthony may tell me/us that this isn't the way to go.) > If you agree, I can send a patch to unbreak CI unless you want to do this. Please do. Thanks, Jan
On 06/09/2023 09:57, Jan Beulich wrote: > > > On 06.09.2023 09:32, Michal Orzel wrote: >> >> >> On 06/09/2023 08:55, Jan Beulich wrote: >>> >>> >>> On 06.09.2023 03:16, Vikram Garhwal wrote: >>>> --- a/tools/xl/xl_vmcontrol.c >>>> +++ b/tools/xl/xl_vmcontrol.c >>>> @@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv) >>>> return 0; >>>> } >>>> >>>> +int main_dt_overlay(int argc, char **argv) >>>> +{ >>>> + const char *overlay_ops = NULL; >>>> + const char *overlay_config_file = NULL; >>>> + void *overlay_dtb = NULL; >>>> + int rc; >>>> + 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"); >>>> + return EXIT_FAILURE; >>>> + } >>>> + >>>> + overlay_ops = argv[1]; >>>> + overlay_config_file = argv[2]; >>>> + >>>> + if (strcmp(overlay_ops, "add") == 0) >>>> + op = overlay_add_op; >>>> + else if (strcmp(overlay_ops, "remove") == 0) >>>> + op = overlay_remove_op; >>>> + else { >>>> + fprintf(stderr, "Invalid dt overlay operation\n"); >>>> + return EXIT_FAILURE; >>>> + } >>>> + >>>> + 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); >>> >>> Because of this being Arm-only (as validly pointed out by osstest), I expect >>> the entire function here as well as its entry in cmd_table[] want to be >>> Arm-specific, too? Of course it would be nice to not key this to __arm__ / >>> __aarch64__, but to something that would not need touching again if the >>> underlying infrastructure was made available to, say, RISC-V as well. But of >>> course - right now the goal needs to be to address the CI and osstest >>> breakage. >> I agree. I would suggest to guard it with LIBXL_HAVE_DT_OVERLAY which is for now >> only defined for arm32/arm64. This way the code will not need to be modified if other >> arch gain support for the feature. > > Ah yes, that ought to work. While there perhaps also replace the conditional > around the declaration of the function in libxl.h. (But of course Anthony > may tell me/us that this isn't the way to go.) Hmm, if we change guards for libxl_dt_overlay(), what about xc_dt_overlay() for which we cannot use LIBXL guard? Is it ok in that case or better just focus on the fix. ~Michal
On 06.09.2023 10:06, Michal Orzel wrote: > > > On 06/09/2023 09:57, Jan Beulich wrote: >> >> >> On 06.09.2023 09:32, Michal Orzel wrote: >>> >>> >>> On 06/09/2023 08:55, Jan Beulich wrote: >>>> >>>> >>>> On 06.09.2023 03:16, Vikram Garhwal wrote: >>>>> --- a/tools/xl/xl_vmcontrol.c >>>>> +++ b/tools/xl/xl_vmcontrol.c >>>>> @@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv) >>>>> return 0; >>>>> } >>>>> >>>>> +int main_dt_overlay(int argc, char **argv) >>>>> +{ >>>>> + const char *overlay_ops = NULL; >>>>> + const char *overlay_config_file = NULL; >>>>> + void *overlay_dtb = NULL; >>>>> + int rc; >>>>> + 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"); >>>>> + return EXIT_FAILURE; >>>>> + } >>>>> + >>>>> + overlay_ops = argv[1]; >>>>> + overlay_config_file = argv[2]; >>>>> + >>>>> + if (strcmp(overlay_ops, "add") == 0) >>>>> + op = overlay_add_op; >>>>> + else if (strcmp(overlay_ops, "remove") == 0) >>>>> + op = overlay_remove_op; >>>>> + else { >>>>> + fprintf(stderr, "Invalid dt overlay operation\n"); >>>>> + return EXIT_FAILURE; >>>>> + } >>>>> + >>>>> + 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); >>>> >>>> Because of this being Arm-only (as validly pointed out by osstest), I expect >>>> the entire function here as well as its entry in cmd_table[] want to be >>>> Arm-specific, too? Of course it would be nice to not key this to __arm__ / >>>> __aarch64__, but to something that would not need touching again if the >>>> underlying infrastructure was made available to, say, RISC-V as well. But of >>>> course - right now the goal needs to be to address the CI and osstest >>>> breakage. >>> I agree. I would suggest to guard it with LIBXL_HAVE_DT_OVERLAY which is for now >>> only defined for arm32/arm64. This way the code will not need to be modified if other >>> arch gain support for the feature. >> >> Ah yes, that ought to work. While there perhaps also replace the conditional >> around the declaration of the function in libxl.h. (But of course Anthony >> may tell me/us that this isn't the way to go.) > Hmm, if we change guards for libxl_dt_overlay(), what about xc_dt_overlay() > for which we cannot use LIBXL guard? I'd key that to some suitable sysctl definition from the public header. If need be, the sub-op #define itself could be made conditional and then be used for that purpose. > Is it ok in that case or better just focus > on the fix. Personally I'd consider dealing with just the breakage sufficient for the moment. The libxc part then of course still wants dealing with later on, preferably in time for 4.18. Jan
diff --git a/tools/xl/xl.h b/tools/xl/xl.h index 72538d6a81..a923daccd3 100644 --- a/tools/xl/xl.h +++ b/tools/xl/xl.h @@ -138,6 +138,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 67604e9536..2463521156 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -631,6 +631,12 @@ const struct cmd_spec cmd_table[] = { "Issue a qemu monitor command to the device model of a domain", "<Domain> <Command>", }, + { "dt-overlay", + &main_dt_overlay, 0, 1, + "Add/Remove a device tree overlay", + "add/remove <.dtbo>" + "-h print this help\n" + }, }; const int cmdtable_len = ARRAY_SIZE(cmd_table); diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c index 03971927e9..cea5b4a88e 100644 --- a/tools/xl/xl_vmcontrol.c +++ b/tools/xl/xl_vmcontrol.c @@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv) return 0; } +int main_dt_overlay(int argc, char **argv) +{ + const char *overlay_ops = NULL; + const char *overlay_config_file = NULL; + void *overlay_dtb = NULL; + int rc; + 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"); + return EXIT_FAILURE; + } + + overlay_ops = argv[1]; + overlay_config_file = argv[2]; + + if (strcmp(overlay_ops, "add") == 0) + op = overlay_add_op; + else if (strcmp(overlay_ops, "remove") == 0) + op = overlay_remove_op; + else { + fprintf(stderr, "Invalid dt overlay operation\n"); + return EXIT_FAILURE; + } + + 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); + + free(overlay_dtb); + + if (rc) + return EXIT_FAILURE; + + return rc; +} /* * Local variables: * mode: C