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

[Henry Wang <xin.wang2@xxxxxxx>]

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@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.
Ok. I can split this part to a separated commit.

> > 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.
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,