| Message ID | 20221207061815.7404-6-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 07.12.2022 07:18, Vikram Garhwal wrote: > --- 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 > +#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 > @@ -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; For now your additions are Arm-only, aren't they? You want to use #ifdef-ary similar to what you see in context in this last hunk then, to avoid undue exposure. Jan
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@amd.com> > --- > 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@amd.com> > + * > + * 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@amd.com> > + * > + * 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
Hi Jan, On 12/7/22 12:41 AM, Jan Beulich wrote: > On 07.12.2022 07:18, Vikram Garhwal wrote: >> --- 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 >> +#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 >> @@ -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; > For now your additions are Arm-only, aren't they? You want to use > #ifdef-ary similar to what you see in context in this last hunk then, > to avoid undue exposure. In v2 there was a comment regarding "No CONFIG_* dependencies in public headers". So, i remove the ifdef. Will add "if defined(__arm__) || defined (__aarch64__)" > > Jan
Hi Michal, On 1/24/23 1:01 AM, Michal Orzel wrote: > 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@amd.com> >> --- >> 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@amd.com> >> + * >> + * 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)? The main purpose of this series to address first part of dynamic programming i.e. making Xen aware of new device tree node which means updating the dt_host with overlay node information. Here we are adding/removing node from dt_host, and checking/set IOMMU and IRQ permission but never mapping them to any domain. Right now, mapping/Un-mapping will happen only when a new domU is created/destroyed using "xl create". To map IOREQ and IOMMU during runtime, there will be another small series after this one where we will do the actual IOMMU and IRQ mapping do a running domain and will call unmap_mmio_regions() to remove the mapping. Here is the patch for that series: https://github.com/Xilinx/xen/commit/76fcd5defc1c7c375cb99ac73a4d1138aa87d474 I will clean it and send once current series is done. Also, If you see addition part of this series, we put "skip_mapping = true" which means map_range_to_domain()->map_range_p2mt() is never called. The only thing function which will be called is iomem_permit_access() while adding the node and here we are calling opposite function to remove the access. Please let me know if my explanation is not clear. I addressed rest of your comments in next series. Regards, Vikram > >> + >> + } >> + >> + 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@amd.com> >> + * >> + * 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
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 + * + * Device tree overlay support in Xen. + * + * Copyright (c) 2022 AMD Inc. + * Written by Vikram Garhwal <vikram.garhwal@amd.com> + * + * 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); + 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; + unsigned int naddr, len; + unsigned int i, nirq; + u64 addr, size; + + 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) ) + { + + 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; + } + + } + + 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; + void *overlay_fdt; + + if ( op->overlay_fdt_size <= 0 || op->overlay_fdt_size > 500000 ) + return -EINVAL; + + overlay_fdt = xmalloc_bytes(op->overlay_fdt_size); + + 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; +} 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: + 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 +#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 @@ -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 @@ +/* + * xen/common/dt_overlay.h + * + * Device tree overlay support in Xen. + * + * Copyright (c) 2022 AMD Inc. + * Written by Vikram Garhwal <vikram.garhwal@amd.com> + * + * 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
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@amd.com> --- 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