Re: [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Wed, Apr 24, 2024 at 11:34:42AM +0800, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> ---
>  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,

-- 
Anthony PERARD