|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN][RFC PATCH v4 12/16] xen/arm: Implement device tree node removal functionalities
Hi Vikram,
You received extensive feedback for v3 from others, so I will limit my review
to just coding
and general comments.
On 07/12/2022 07:18, Vikram Garhwal wrote:
>
>
> Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
> device tree overlay.
>
> xl dt_overlay remove file.dtbo:
> Removes all the nodes in a given dtbo.
> First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
> in dt_host and delete the device node entries from dt_host.
>
> The nodes get removed only if it is not used by any of dom0 or domio.
>
> Also, added overlay_track struct to keep the track of added node through
> device
> tree overlay. overlay_track has dt_host_new which is unflattened form of
> updated
> fdt and name of overlay nodes. When a node is removed, we also free the memory
> used by overlay_track for the particular overlay node.
>
> Nested overlay removal is supported in sequential manner only i.e. if
> overlay_child nests under overlay_parent, it is assumed that user first
> removes
> overlay_child and then removes overlay_parent.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> ---
> xen/common/Makefile | 1 +
> xen/common/dt_overlay.c | 411 +++++++++++++++++++++++++++++++++++
> xen/common/sysctl.c | 5 +
> xen/include/public/sysctl.h | 19 ++
> xen/include/xen/dt_overlay.h | 55 +++++
> 5 files changed, 491 insertions(+)
> create mode 100644 xen/common/dt_overlay.c
> create mode 100644 xen/include/xen/dt_overlay.h
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 3baf83d527..58a35f55b2 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
> obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> obj-$(CONFIG_IOREQ_SERVER) += dm.o
> obj-y += domain.o
> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
> obj-y += event_2l.o
> obj-y += event_channel.o
> obj-y += event_fifo.o
> diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
> new file mode 100644
> index 0000000000..477341f0aa
> --- /dev/null
> +++ b/xen/common/dt_overlay.c
> @@ -0,0 +1,411 @@
> +/*
> + * xen/common/dt_overlay.c
New files should start with SPDX comment expressing license.
> + *
> + * Device tree overlay support in Xen.
> + *
> + * Copyright (c) 2022 AMD Inc.
> + * Written by Vikram Garhwal <vikram.garhwal@xxxxxxx>
> + *
> + * 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.
> + */
> +#include <xen/iocap.h>
> +#include <xen/xmalloc.h>
> +#include <asm/domain_build.h>
> +#include <xen/dt_overlay.h>
> +#include <xen/guest_access.h>
> +
> +static LIST_HEAD(overlay_tracker);
> +static DEFINE_SPINLOCK(overlay_lock);
> +
> +/* Find last descendants of the device_node. */
> +static struct dt_device_node *find_last_descendants_node(
> + struct dt_device_node
> *device_node)
> +{
> + struct dt_device_node *child_node;
> +
> + for ( child_node = device_node->child; child_node->sibling != NULL;
> + child_node = child_node->sibling )
> + {
> + }
> +
> + /* If last child_node also have children. */
> + if ( child_node->child )
> + child_node = find_last_descendants_node(child_node);
Please add a blank line here.
> + return child_node;
> +}
> +
> +static int dt_overlay_remove_node(struct dt_device_node *device_node)
> +{
> + struct dt_device_node *np;
> + struct dt_device_node *parent_node;
> + struct dt_device_node *device_node_last_descendant = device_node->child;
> +
> + parent_node = device_node->parent;
> +
> + if ( parent_node == NULL )
> + {
> + dt_dprintk("%s's parent node not found\n", device_node->name);
> + return -EFAULT;
> + }
> +
> + np = parent_node->child;
> +
> + if ( np == NULL )
> + {
> + dt_dprintk("parent node %s's not found\n", parent_node->name);
> + return -EFAULT;
> + }
> +
> + /* If node to be removed is only child node or first child. */
> + if ( !dt_node_cmp(np->full_name, device_node->full_name) )
> + {
> + parent_node->child = np->sibling;
> +
> + /*
> + * Iterate over all child nodes of device_node. Given that we are
> + * removing parent node, we need to remove all it's descendants too.
> + */
> + if ( device_node_last_descendant )
> + {
> + device_node_last_descendant =
> +
> find_last_descendants_node(device_node);
> + parent_node->allnext = device_node_last_descendant->allnext;
> + }
> + else
> + parent_node->allnext = np->allnext;
> +
> + return 0;
> + }
> +
> + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
> + {
> + if ( !dt_node_cmp(np->sibling->full_name, device_node->full_name) )
> + {
> + /* Found the node. Now we remove it. */
> + np->sibling = np->sibling->sibling;
> +
> + if ( np->child )
> + np = find_last_descendants_node(np);
> +
> + /*
> + * Iterate over all child nodes of device_node. Given that we are
> + * removing parent node, we need to remove all it's descendants
> too.
> + */
> + if ( device_node_last_descendant )
> + device_node_last_descendant =
> +
> find_last_descendants_node(device_node);
> +
> + if ( device_node_last_descendant )
> + np->allnext = device_node_last_descendant->allnext;
> + else
> + np->allnext = np->allnext->allnext;
> +
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* Basic sanity check for the dtbo tool stack provided to Xen. */
> +static int check_overlay_fdt(const void *overlay_fdt, uint32_t
> overlay_fdt_size)
> +{
> + if ( (fdt_totalsize(overlay_fdt) != overlay_fdt_size) ||
> + fdt_check_header(overlay_fdt) )
> + {
> + printk(XENLOG_ERR "The overlay FDT is not a valid Flat Device
> Tree\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/* Count number of nodes till one level of __overlay__ tag. */
> +static unsigned int overlay_node_count(void *fdto)
> +{
> + unsigned int num_overlay_nodes = 0;
> + int fragment;
> +
> + fdt_for_each_subnode(fragment, fdto, 0)
> + {
> + int subnode;
> + int overlay;
> +
> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +
> + /*
> + * overlay value can be < 0. But fdt_for_each_subnode() loop checks
> for
> + * overlay >= 0. So, no need for a overlay>=0 check here.
> + */
> + fdt_for_each_subnode(subnode, fdto, overlay)
> + {
> + num_overlay_nodes++;
> + }
> + }
> +
> + return num_overlay_nodes;
> +}
> +
> +static int handle_remove_irq_iommu(struct dt_device_node *device_node)
> +{
> + int rc = 0;
> + struct domain *d = hardware_domain;
> + domid_t domid = 0;
No need for this assignment.
> + unsigned int naddr, len;
> + unsigned int i, nirq;
> + u64 addr, size;
We should not be using types like these anymore. Use uint64_t.
> +
> + domid = dt_device_used_by(device_node);
> +
> + dt_dprintk("Checking if node %s is used by any domain\n",
> + device_node->full_name);
> +
> + /* 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",
> + device_node->full_name, domid);
> + return -EINVAL;
> + }
> +
> + dt_dprintk("Removing node: %s\n", device_node->full_name);
> +
> + nirq = dt_number_of_irq(device_node);
> +
> + /* Remove IRQ permission */
> + for ( i = 0; i < nirq; i++ )
> + {
> + rc = platform_get_irq(device_node, i);;
> +
> + if ( irq_access_permitted(d, rc) == false )
> + {
> + printk(XENLOG_ERR "IRQ %d is not routed to domain %d\n", rc,
> + domid);
> + return -EINVAL;
> + }
> + /*
> + * TODO: We don't handle shared IRQs for now. So, it is assumed that
> + * the IRQs was not shared with another devices.
> + */
> + rc = irq_deny_access(d, rc);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
> + i, device_node->full_name);
> + return rc;
> + }
> + }
> +
> + /* Check if iommu property exists. */
> + if ( dt_get_property(device_node, "iommus", &len) )
> + {
> +
Remove extra line.
> + rc = iommu_remove_dt_device(device_node);
> + if ( rc != 0 && rc != -ENXIO )
> + return rc;
> + }
> +
> + naddr = dt_number_of_address(device_node);
> +
> + /* Remove mmio access. */
> + for ( i = 0; i < naddr; i++ )
> + {
> + rc = dt_device_get_address(device_node, i, &addr, &size);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> + i, dt_node_full_name(device_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);
> + return rc;
> + }
What about removing p2m mappings (comment from Julien on v3)?
> +
> + }
> +
> + return rc;
> +}
> +
> +/* Removes all descendants of the given node. */
> +static int remove_all_descendant_nodes(struct dt_device_node *device_node)
> +{
> + int rc = 0;
> + struct dt_device_node *child_node;
> +
> + for ( child_node = device_node->child; child_node != NULL;
> + child_node = child_node->sibling )
> + {
> + if ( child_node->child )
> + remove_all_descendant_nodes(child_node);
> +
> + rc = handle_remove_irq_iommu(child_node);
> + if ( rc )
> + return rc;
> + }
> +
> + return rc;
> +}
> +
> +/* Remove nodes from dt_host. */
> +static int remove_nodes(const struct overlay_track *tracker)
> +{
> + int rc = 0;
> + struct dt_device_node *overlay_node;
> + unsigned int j;
> +
> + for ( j = 0; j < tracker->num_nodes; j++ )
> + {
> + overlay_node = (struct dt_device_node *)tracker->nodes_address[j];
> + if ( overlay_node == NULL )
> + {
> + printk(XENLOG_ERR "Device %s is not present in the tree.
> Removing nodes failed\n",
> + overlay_node->full_name);
> + return -EINVAL;
> + }
> +
> + rc = remove_all_descendant_nodes(overlay_node);
> +
> + /* All children nodes are unmapped. Now remove the node itself. */
> + rc = handle_remove_irq_iommu(overlay_node);
> + if ( rc )
> + return rc;
> +
> + read_lock(&dt_host->lock);
> +
> + rc = dt_overlay_remove_node(overlay_node);
> + if ( rc )
> + {
> + read_unlock(&dt_host->lock);
> +
> + return rc;
> + }
> +
> + read_unlock(&dt_host->lock);
> + }
> +
> + return rc;
> +}
> +
> +/*
> + * First finds the device node to remove. Check if the device is being used
> by
> + * any dom and finally remove it from dt_host. IOMMU is already being taken
> care
> + * while destroying the domain.
> + */
> +static long handle_remove_overlay_nodes(void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> + int rc = 0;
> + struct overlay_track *entry, *temp, *track;
> + bool found_entry = false;
> +
> + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> + if ( rc )
> + return rc;
> +
> + if ( overlay_node_count(overlay_fdt) == 0 )
> + return -ENOMEM;
> +
> + spin_lock(&overlay_lock);
> +
> + /*
> + * First check if dtbo is correct i.e. it should one of the dtbo which
> was
> + * used when dynamically adding the node.
> + * Limitation: Cases with same node names but different property are not
> + * supported currently. We are relying on user to provide the same dtbo
> + * as it was used when adding the nodes.
> + */
> + list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> + {
> + if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> + {
> + track = entry;
> + found_entry = true;
> + break;
> + }
> + }
> +
> + if ( found_entry == false )
> + {
> + rc = -EINVAL;
> +
> + printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
> + " Removing nodes is supported for only prior added dtbo.
> Please"
> + " provide a valid dtbo which was used to add the nodes.\n");
> + goto out;
> +
> + }
> +
> + rc = remove_nodes(entry);
> +
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Removing node failed\n");
> + goto out;
> + }
> +
> + list_del(&entry->entry);
> +
> + xfree(entry->dt_host_new);
> + xfree(entry->fdt);
> + xfree(entry->overlay_fdt);
> +
> + xfree(entry->nodes_address);
> +
> + xfree(entry);
> +
> +out:
> + spin_unlock(&overlay_lock);
> + return rc;
> +}
> +
> +long dt_sysctl(struct xen_sysctl_dt_overlay *op)
> +{
> + long ret = 0;
No need for assignign a value that will be reassigned anyway.
> + void *overlay_fdt;
> +
> + if ( op->overlay_fdt_size <= 0 || op->overlay_fdt_size > 500000 )
FWICS, you want to limit the fdt size to 500KB which should be 512000.
Also, it would be clearer to use KB(500). Otherwise such value is a bit
ambiguous.
Also overlay_fdt_size cannot be < 0.
> + return -EINVAL;
> +
> + overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
If you alloc the bytes here and the op will not be XEN_SYSCTL_DT_OVERLAY_REMOVE,
then you will end up without freeing it.
> +
> + if ( overlay_fdt == NULL )
> + return -ENOMEM;
> +
> + ret = copy_from_guest(overlay_fdt, op->overlay_fdt,
> op->overlay_fdt_size);
> + if ( ret )
> + {
> + gprintk(XENLOG_ERR, "copy from guest failed\n");
> + xfree(overlay_fdt);
> +
> + return -EFAULT;
> + }
> +
> + switch ( op->overlay_op )
> + {
> + case XEN_SYSCTL_DT_OVERLAY_REMOVE:
> + ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
> + xfree(overlay_fdt);
> +
> + break;
> +
> + default:
> + break;
> + }
> +
> + return ret;
> +}
Don't you want to put EMACS comments block here?
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 02505ab044..bb338b7c27 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -28,6 +28,7 @@
> #include <xen/pmstat.h>
> #include <xen/livepatch.h>
> #include <xen/coverage.h>
> +#include <xen/dt_overlay.h>
>
> long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
> @@ -482,6 +483,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
> u_sysctl)
> copyback = 1;
> break;
>
> + case XEN_SYSCTL_dt_overlay:
If you protect xen_sysctl_dt_overlay with ARM ifdefery as Jan suggested,
then you should move this handling to arch_do_sysctl.
> + ret = dt_sysctl(&op->u.dt_overlay);
> + break;
> +
> 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 5672906729..4bc76bbe27 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1079,6 +1079,23 @@ 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_ADD 1
I'm not sure whether the ADD macro should be added in this patch.
> +#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;
FWICS, this is the output variable and it would be beneficial to add a comment.
Also, usually IN variables appear first.
> + uint32_t overlay_fdt_size; /* Overlay dtb size. */
> + uint8_t overlay_op; /* Add or remove. */
These are the input variables, so the comment should be e.g. /* IN: Overlay dtb
size */
> +};
> +
> struct xen_sysctl {
> uint32_t cmd;
> #define XEN_SYSCTL_readconsole 1
> @@ -1109,6 +1126,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;
> @@ -1139,6 +1157,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;
> 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..30f4b86586
> --- /dev/null
> +++ b/xen/include/xen/dt_overlay.h
> @@ -0,0 +1,55 @@
> +/*
Missing SPDX comment at the top of the files.
> + * xen/common/dt_overlay.h
Incorrect path.
> + *
> + * Device tree overlay support in Xen.
> + *
> + * Copyright (c) 2022 AMD Inc.
> + * Written by Vikram Garhwal <vikram.garhwal@xxxxxxx>
> + *
> + * 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 <xen/rangeset.h>
> +
> +/*
> + * 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;
> + void *overlay_fdt;
> + unsigned long *nodes_address;
> + unsigned int num_nodes;
> +};
> +
> +struct xen_sysctl_dt_overlay;
> +
> +#ifdef CONFIG_OVERLAY_DTB
> +long dt_sysctl(struct xen_sysctl_dt_overlay *op);
> +#else
> +static inline long dt_sysctl(struct xen_sysctl_dt_overlay *op)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +#endif
Don't you want to put EMACS comments block here?
> --
> 2.17.1
>
>
~Michal
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |