Re: [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support

[Anthony PERARD <anthony.perard@citrix.com>]

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,

-- 
Anthony PERARD