Re: [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Mar 08, 2022 at 11:47:03AM -0800, Vikram Garhwal wrote:
> Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> ---
>  tools/include/libxl.h            |  3 ++
>  tools/libs/light/Makefile        |  1 +
>  tools/libs/light/libxl_overlay.c | 67 ++++++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+)
>  create mode 100644 tools/libs/light/libxl_overlay.c
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 51a9b6cfac..b31e17c2ce 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -2419,6 +2419,9 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, 
> uint32_t domid,
>                                          int *num);
>  void libxl_device_pci_list_free(libxl_device_pci* list, int num);
>  
> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
> +                     int overlay_size, uint8_t overlay_op);
> +

As you are making changes to libxl's API, you are going to need to add a
"#define LIBXL_HAVE_*" in "include/libxl.h" about the availability of
the new function.

>  /*
>   * Turns the current process into a backend device service daemon
>   * for a driver domain.
> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> index 453bea0067..405115c13c 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -116,6 +116,7 @@ SRCS-y += libxl_genid.c
>  SRCS-y += _libxl_types.c
>  SRCS-y += libxl_flask.c
>  SRCS-y += _libxl_types_internal.c
> +SRCS-y += libxl_overlay.o

Building this new file unconditionally is an issue at the moment.
"./configure" doesn't check if libfdt is on the system unless we happen
to build for Arm. And libfdt is mandatory when building for Arm.

Could you build this new source file on Arm only? With a comment why.
    "SRCS-$(CONFIG_ARM) +="

Alternatively, you could try to rework configure to always check for
libfdt, but I doubt that device tree overlay are going to be useful on
x86 at the moment.


Then, libxl_dt_overlay() will need a stub that just return an error when
building on system that don't have libfdt.

>  
>  ifeq ($(CONFIG_LIBNL),y)
>  CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
> diff --git a/tools/libs/light/libxl_overlay.c 
> b/tools/libs/light/libxl_overlay.c
> new file mode 100644
> index 0000000000..e370e8cac8
> --- /dev/null
> +++ b/tools/libs/light/libxl_overlay.c

Could you rename this new file "libxl_dt_overlay.c"? There could be
other kind of "overlay".

> @@ -0,0 +1,67 @@
> +/*
> + * Copyright (C) 2021 Xilinx Inc.
> + * Author Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxl_internal.h"
> +#include <libfdt.h>
> +#include <xenguest.h>
> +#include <xenctrl.h>
> +
> +static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
> +{
> +    int r;
> +
> +    if (fdt_magic(fdt) != FDT_MAGIC) {
> +        LOG(ERROR, "Overlay FDT is not a valid Flat Device Tree");
> +        return ERROR_FAIL;
> +    }
> +
> +    r = fdt_check_header(fdt);
> +    if (r) {
> +        LOG(ERROR, "Failed to check the overlay FDT (%d)", r);
> +        return ERROR_FAIL;
> +    }
> +
> +    if (fdt_totalsize(fdt) > size) {
> +        LOG(ERROR, "Overlay FDT totalsize is too big");
> +        return ERROR_FAIL;
> +    }
> +
> +    return 0;
> +}
> +
> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, int overlay_dt_size,

I wonder whether the type of "overlay_dt_size" should be something else.
check_overlay_fdt() takes a "size_t", but the hypercall takes
"uint32_t".

> +                     uint8_t overlay_op)
> +{
> +    int rc = 0;

By CODING_STYLE, "rc" shouldn't be set when declared.

> +    GC_INIT(ctx);
> +
> +    if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
> +        LOG(ERROR, "Overlay DTB check failed\n");

No need for \n, it's already added by LOG().

> +        GC_FREE;
> +        return ERROR_FAIL;

Instead of writing GC_FREE more than once, could you set "rc" then "goto
out;"?

> +    } else
> +        LOG(DEBUG, "Overlay DTB check passed\n");

This needs to be in a {} block because the true side of the "if" uses
braces.

> +
> +    /* We don't need to do  xc_interface_open here. */

That comment isn't very useful, as it is a fact of using "ctx".

> +    rc = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);

Instead of "rc", could you store the result of xc_dt_overlay() in "r"?
"rc" is reserved for libxl error code. Also, the return value of
libxl_dt_overlay() can't be the return value of xc_dt_overlay().
There's some better explanation in the CODING_STYLE file, if needed.

> +    if (rc)
> +        LOG(ERROR, "%s: Adding/Removing overlay dtb failed.\n", __func__);
> +
> +    GC_FREE;
> +    return rc;
> +}

Thanks,

-- 
Anthony PERARD