Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index dc8d3a13f5..2eb5734f8e 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -54,6 +54,7 @@ obj-y += wait.o
> obj-bin-y += warning.init.o
> obj-$(CONFIG_XENOPROF) += xenoprof.o
> obj-y += xmalloc_tlsf.o
> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o

I think the entries are in alphabetical order, this should be added after += 
domain.o

> +/*
> + * overlay_get_nodes_info will get the all node's full name with path. This 
> is
> + * useful when checking node for duplication i.e. dtbo tries to add nodes 
> which
> + * already exists in device tree.
> + */
> +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path,
> +                                  unsigned int num_overlay_nodes)
> +{
> +    int fragment;
> +    unsigned int node_num = 0;
> +
> +    *nodes_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char *));

Here you can use xzalloc_bytes and...

> +
> +    if ( *nodes_full_path == NULL )
> +        return -ENOMEM;
> +    memset(*nodes_full_path, 0x0, num_overlay_nodes * sizeof(char *));

Get rid of this memset

> +
> +/* Remove nodes from dt_host. */
> +static int remove_nodes(char **full_dt_node_path, int **nodes_irq,
> +                        int *node_num_irq, unsigned int num_nodes)
> +{
> +    struct domain *d = hardware_domain;
> +    int rc = 0;
> +    struct dt_device_node *overlay_node;
> +    unsigned int naddr;
> +    unsigned int i, j, nirq;
> +    u64 addr, size;
> +    domid_t domid = 0;
> +
> +    for ( j = 0; j < num_nodes; j++ )
> +    {
> +        dt_dprintk("Finding node %s in the dt_host\n", full_dt_node_path[j]);
> +
> +        overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
> +
> +        if ( overlay_node == NULL )
> +        {
> +            printk(XENLOG_ERR "Device %s is not present in the tree. 
> Removing nodes failed\n",
> +                   full_dt_node_path[j]);
> +            return -EINVAL;
> +        }
> +
> +        domid = dt_device_used_by(overlay_node);
> +
> +        dt_dprintk("Checking if node %s is used by any domain\n",
> +                   full_dt_node_path[j]);
> +
> +        /* Remove the node iff it's assigned to domain 0 or domain io. */
> +        if ( domid != 0 && domid != DOMID_IO )
> +        {
> +            printk(XENLOG_ERR "Device %s as it is being used by domain %d. 
> Removing nodes failed\n",
> +                   full_dt_node_path[j], domid);
> +            return -EINVAL;
> +        }
> +
> +        dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
> +
> +        nirq = node_num_irq[j];
> +
> +        /* Remove IRQ permission */
> +        for ( i = 0; i < nirq; i++ )
> +        {
> +            rc = nodes_irq[j][i];
> +            /*
> +             * TODO: We don't handle shared IRQs for now. So, it is assumed 
> that
> +             * the IRQs was not shared with another domain.
> +             */
> +            rc = irq_deny_access(d, rc);
> +            if ( rc )
> +            {
> +                printk(XENLOG_ERR "unable to revoke access for irq %u for 
> %s\n",
> +                       i, dt_node_full_name(overlay_node));
> +                return rc;
> +            }
> +        }
> +
> +        rc = iommu_remove_dt_device(overlay_node);
> +        if ( rc != 0 && rc != -ENXIO )
> +            return rc;
> +
> +        naddr = dt_number_of_address(overlay_node);
> +
> +        /* Remove mmio access. */
> +        for ( i = 0; i < naddr; i++ )
> +        {
> +            rc = dt_device_get_address(overlay_node, i, &addr, &size);
> +            if ( rc )
> +            {
> +                printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> +                       i, dt_node_full_name(overlay_node));
> +                return rc;
> +            }
> +
> +            rc = iomem_deny_access(d, paddr_to_pfn(addr),
> +                                   paddr_to_pfn(PAGE_ALIGN(addr + size - 
> 1)));
> +            if ( rc )
> +            {
> +                printk(XENLOG_ERR "Unable to remove dom%d access to"
> +                        " 0x%"PRIx64" - 0x%"PRIx64"\n",
> +                        d->domain_id,
> +                        addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);

NIT: here in each line under XENLOG_ERR, there is an extra space, these lines
Could be aligned to XENLOG_ERR, just for code style purpose.

> +                return rc;
> +            }
> +        }
> +
> +        rc = dt_overlay_remove_node(overlay_node);
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return rc;
> +}
> +
> 
> +long dt_sysctl(struct xen_sysctl *op)
> +{
> +    long ret = 0;
> +    void *overlay_fdt;
> +    char **nodes_full_path = NULL;
> +    unsigned int num_overlay_nodes = 0;
> +
> +    if ( op->u.dt_overlay.overlay_fdt_size <= 0 )
> +        return -EINVAL;
> +
> +    overlay_fdt = xmalloc_bytes(op->u.dt_overlay.overlay_fdt_size);
> +
> +    if ( overlay_fdt == NULL )
> +        return -ENOMEM;
> +
> +    ret = copy_from_guest(overlay_fdt, op->u.dt_overlay.overlay_fdt,
> +                         op->u.dt_overlay.overlay_fdt_size);
> +    if ( ret )
> +    {
> +        gprintk(XENLOG_ERR, "copy from guest failed\n");
> +        xfree(overlay_fdt);
> +
> +        return -EFAULT;
> +    }
> +
> +    switch ( op->u.dt_overlay.overlay_op )
> +    {
> +    case XEN_SYSCTL_DT_OVERLAY_REMOVE:
> +        ret = check_overlay_fdt(overlay_fdt,
> +                                op->u.dt_overlay.overlay_fdt_size);
> +        if ( ret )
> +        {
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        num_overlay_nodes = overlay_node_count(overlay_fdt);
> +        if ( num_overlay_nodes == 0 )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        ret = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> +                                     num_overlay_nodes);
> +        if ( ret )
> +             break;
> +
> +        ret = handle_remove_overlay_nodes(nodes_full_path,
> +                                          num_overlay_nodes);
> +        break;
> +
> +    default:
> +        break;
> +    }
> +
> +    if ( nodes_full_path != NULL )
> +    {
> +        int I;

unsigned int

> +        for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; 
> i++ )
> +        {
> +            xfree(nodes_full_path[i]);
> +        }
> +        xfree(nodes_full_path);
> +    }
> +
> +    return ret;
> +}
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index fc4a0b31d6..d685c07159 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -29,6 +29,10 @@
> #include <xen/livepatch.h>
> #include <xen/coverage.h>
> 
> +#ifdef CONFIG_OVERLAY_DTB
> +#include <xen/dt_overlay.h>
> +#endif

Maybe this header can be included anyway, removing ifdefs, and...

> +
> long cf_check do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
>     long ret = 0;
> @@ -482,6 +486,12 @@ long cf_check 
> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>             copyback = 1;
>         break;
> 
> +#ifdef CONFIG_OVERLAY_DTB
> +    case XEN_SYSCTL_overlay:
> +        ret = dt_sysctl(op);
> +        break;
> +#endif

Also here you can remove ifdefs and use the header to switch between the real 
implementation
or a static inline returning error if CONFIG_OVERLAY_DTB is not enabled, take a 
look in
livepatch_op(struct xen_sysctl_livepatch_op *op).

dt_sysctl can take struct xen_sysctl_dt_overlay* as input.

> +
>     default:
>         ret = arch_do_sysctl(op, u_sysctl);
>         copyback = 0;
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 55252e97f2..e256aeb7c6 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1069,6 +1069,22 @@ typedef struct xen_sysctl_cpu_policy 
> xen_sysctl_cpu_policy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> #endif
> 
> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
> +
> +/*
> + * XEN_SYSCTL_dt_overlay
> + * Performs addition/removal of device tree nodes under parent node using 
> dtbo.
> + * This does in three steps:
> + *  - Adds/Removes the nodes from dt_host.
> + *  - Adds/Removes IRQ permission for the nodes.
> + *  - Adds/Removes MMIO accesses.
> + */
> +struct xen_sysctl_dt_overlay {
> +    XEN_GUEST_HANDLE_64(void) overlay_fdt;
> +    uint32_t overlay_fdt_size;  /* Overlay dtb size. */
> +    uint8_t overlay_op; /* Add or remove. */
> +};
> +
> struct xen_sysctl {
>     uint32_t cmd;
> #define XEN_SYSCTL_readconsole                    1
> @@ -1099,6 +1115,7 @@ struct xen_sysctl {
> #define XEN_SYSCTL_livepatch_op                  27
> /* #define XEN_SYSCTL_set_parameter              28 */
> #define XEN_SYSCTL_get_cpu_policy                29
> +#define XEN_SYSCTL_dt_overlay                    30
>     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>     union {
>         struct xen_sysctl_readconsole       readconsole;
> @@ -1129,6 +1146,7 @@ struct xen_sysctl {
> #if defined(__i386__) || defined(__x86_64__)
>         struct xen_sysctl_cpu_policy        cpu_policy;
> #endif
> +        struct xen_sysctl_dt_overlay        dt_overlay;

Here I would need an opinion from someone more experienced, but I think when a 
change
is made in this structure, XEN_SYSCTL_INTERFACE_VERSION should be bumped?

>         uint8_t                             pad[128];
>     } u;
> };
> diff --git a/xen/include/xen/dt_overlay.h b/xen/include/xen/dt_overlay.h
> new file mode 100644
> index 0000000000..525818b77c
> --- /dev/null
> +++ b/xen/include/xen/dt_overlay.h
> @@ -0,0 +1,47 @@
> +/*
> + * xen/common/dt_overlay.c

Typo: dt_overlay.h

> + *
> + * Device tree overlay suppoert in Xen.

Typo: support

> + *
> + * Copyright (c) 2021 Xilinx Inc.
> + * Written by Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * 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 General Public License for more details.
> + */
> +#ifndef __XEN_DT_SYSCTL_H__
> +#define __XEN_DT_SYSCTL_H__
> +
> +#include <xen/list.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/device_tree.h>
> +#include <public/sysctl.h>

In case you decide to pass struct xen_sysctl_dt_overlay to dt_sysctl, you can 
remove
#include <public/sysctl.h> and use a forward declaration to struct 
xen_sysctl_dt_overlay
instead.

> +
> +/*
> + * overlay_node_track describes information about added nodes through dtbo.
> + * @entry: List pointer.
> + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated 
> fdt'.
> + * @fdt: Stores the fdt.
> + * @nodes_fullname: Stores the full name of nodes.
> + * @nodes_irq: Stores the IRQ added from overlay dtb.
> + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
> + * @num_nodes: Stores total number of nodes in overlay dtb.
> + */
> +struct overlay_track {
> +    struct list_head entry;
> +    struct dt_device_node *dt_host_new;
> +    void *fdt;
> +    char **nodes_fullname;
> +    int **nodes_irq;
> +    int *node_num_irq;
> +    unsigned int num_nodes;
> +};
> +
> +long dt_sysctl(struct xen_sysctl *op);
> +#endif
>