Re: [XEN][RFC PATCH v2 11/12] tools/libs/light: Implement new libxl functions for device tree overlay ops

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Nov 08, 2021 at 11:02:26PM -0800, Vikram Garhwal wrote:
> Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> ---
>  tools/include/libxl.h            |  5 ++++
>  tools/libs/light/Makefile        |  3 ++
>  tools/libs/light/libxl_overlay.c | 65 
> ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+)
>  create mode 100644 tools/libs/light/libxl_overlay.c
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 2e8679d..3dcb3e7 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -2406,6 +2406,11 @@ 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);
>  
> +#if defined (CONFIG_OVERLAY_DTB)
> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
> +                     int overlay_size, uint8_t op);
> +#endif
> +

This won't work. "libxl.h" is a public header to be installed, so
CONFIG_OVERLAY_DTB won't mean anything to other application making use
of libxl.

Instead, can you always provide libxl_dt_overlay() but which would
return ENOSYS when libxl is built without CONFIG_OVERLAY_DTB?


>  /*
>   * 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 194bc5f..0fffa93 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -117,6 +117,9 @@ SRCS-y += libxl_genid.c
>  SRCS-y += _libxl_types.c
>  SRCS-y += libxl_flask.c
>  SRCS-y += _libxl_types_internal.c
> +ifeq ($(CONFIG_OVERLAY_DTB),y)
> +SRCS-y += libxl_overlay.o

FYI, the "-y" is so that you can do
    SRCS-$(CONFIG_OVERLAY_DTB) += libxl_overlay.o
;-)

> diff --git a/tools/libs/light/libxl_overlay.c 
> b/tools/libs/light/libxl_overlay.c
> new file mode 100644
> index 0000000..d965aee
> --- /dev/null
> +++ b/tools/libs/light/libxl_overlay.c
> @@ -0,0 +1,65 @@
> +/*
> + * 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)

Does "fdt" can have a better type than a plain pointer?

There is a "size" provided, is it the size of the blob pointed by "fdt"?
If so, can the size been the first thing to be check? Surely there is a
minimum size for *fdt.

> +{
> +    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,
> +                     uint8_t op)

What is "op"? What the function is supposed to do beside calling libxc?

> +{
> +    int rc = 0;
> +    GC_INIT(ctx);

The function should have "GC_FREE;" before returning.

> +
> +    if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
> +        LOG(ERROR, "Overlay DTB check failed\n");
> +        return ERROR_FAIL;
> +    } else
> +        LOG(DEBUG, "Overlay DTB check passed\n");

FYI, there is no need for "\n" when using LOG().

> +
> +    /* We don't need to do  xc_interface_open here. */
> +    rc = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, op);

"rc" is reserved for libxl error code, xc_* return value should be
stored in "r" instead.

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

Thanks,

-- 
Anthony PERARD