From: Michal Orzel <michal.orzel@amd.com>
To: Vikram Garhwal <vikram.garhwal@amd.com>,
<xen-devel@lists.xenproject.org>
Cc: <sstabellini@kernel.org>, Julien Grall <julien@xen.org>,
Bertrand Marquis <bertrand.marquis@arm.com>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Jan Beulich <jbeulich@suse.com>, Wei Liu <wl@xen.org>
Subject: Re: [XEN][PATCH v6 15/19] xen/arm: Implement device tree node removal functionalities
Date: Tue, 9 May 2023 16:30:31 +0200 [thread overview]
Message-ID: <8c96d009-bd81-66c7-fd23-1f11bc07e72f@amd.com> (raw)
In-Reply-To: <20230502233650.20121-16-vikram.garhwal@amd.com>
On 03/05/2023 01:36, 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/arch/arm/sysctl.c | 16 +-
> xen/common/Makefile | 1 +
> xen/common/dt-overlay.c | 419 +++++++++++++++++++++++++++++++++++
> xen/include/public/sysctl.h | 23 ++
> xen/include/xen/dt-overlay.h | 58 +++++
> 5 files changed, 516 insertions(+), 1 deletion(-)
> create mode 100644 xen/common/dt-overlay.c
> create mode 100644 xen/include/xen/dt-overlay.h
>
> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> index b0a78a8b10..456358166c 100644
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -9,6 +9,7 @@
>
> #include <xen/types.h>
> #include <xen/lib.h>
> +#include <xen/dt-overlay.h>
> #include <xen/errno.h>
> #include <xen/hypercall.h>
> #include <public/sysctl.h>
> @@ -21,7 +22,20 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> long arch_do_sysctl(struct xen_sysctl *sysctl,
> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
> - return -ENOSYS;
> + long ret = 0;
> +
> + switch ( sysctl->cmd )
> + {
> + case XEN_SYSCTL_dt_overlay:
> + ret = dt_sysctl(&sysctl->u.dt_overlay);
> + break;
> +
> + default:
> + ret = -ENOSYS;
> + break;
> + }
> +
> + return ret;
> }
>
> /*
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 46049eac35..e7e96b1087 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -8,6 +8,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..b89cceab84
> --- /dev/null
> +++ b/xen/common/dt-overlay.c
> @@ -0,0 +1,419 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
GPL-2.0-only according to the latest series from Andrew (GPL-2.0 is a depracated tag)
[...]
> +
> + /* Remove mmio access. */
> + for ( i = 0; i < naddr; i++ )
> + {
> + uint64_t addr, size;
> +
> + rc = dt_device_get_address(device_node, i, &addr, &size);
Given that Ayan's 32-bit series might be merged first, this will have to be changed (to use paddr_t for addr,size, etc.)
> + 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;
as always, please do not initialize variables if it is not required (this applies to all the patches)
> + 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 -EINVAL;
> +
> + 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");
This will be a quite large single line. Maybe some split?
> + goto out;
> +
> + }
> +
> + rc = remove_nodes(entry);
> +
remove this line so that a check follows assignment
> + 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;
> + void *overlay_fdt;
> +
> + if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
> + 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);
this xfree and ...
> +
> + break;
> +
> + default:
> + xfree(overlay_fdt);
this one can be removed by adding a single xfree before final return
> + break;
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 2b24d6bfd0..28f7fba98b 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1057,6 +1057,24 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> #endif
>
> +#if defined(__arm__) || defined (__aarch64__)
> +/*
> + * 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; /* IN: overlay fdt. */
> + uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
> +#define XEN_SYSCTL_DT_OVERLAY_ADD 1
> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
> + uint8_t overlay_op; /* IN: Add or remove. */
> +};
> +#endif
> +
> struct xen_sysctl {
> uint32_t cmd;
> #define XEN_SYSCTL_readconsole 1
> @@ -1087,6 +1105,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;
> @@ -1117,6 +1136,10 @@ struct xen_sysctl {
> #if defined(__i386__) || defined(__x86_64__)
> struct xen_sysctl_cpu_policy cpu_policy;
> #endif
> +
> +#if defined(__arm__) || defined (__aarch64__)
> + struct xen_sysctl_dt_overlay dt_overlay;
> +#endif
> 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..5b369f8eb7
> --- /dev/null
> +++ b/xen/include/xen/dt-overlay.h
> @@ -0,0 +1,58 @@
> + /* SPDX-License-Identifier: GPL-2.0 */
GPL-2.0-only according to the latest series from Andrew (GPL-2.0 is a depracated tag)
> + /*
> + * xen/dt-overlay.h
> + *
> + * Device tree overlay support in Xen.
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
> + * Written by Vikram Garhwal <vikram.garhwal@amd.com>
> + *
> + */
> +#ifndef __XEN_DT_OVERLAY_H__
> +#define __XEN_DT_OVERLAY_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.
structure is called overlay_track
> + * @entry: List pointer.
> + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
This reads strange, would you mind to fix it?
> + * @fdt: Stores the fdt.
From here ...
> + * @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.
... to here these params do not reflect the struct members. Please fix. Also, you can omit "Stores" word
and just write e.g. "total number of ..."
> + * @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;
no xen/errno.h included results in a compilation error on my side.
> +}
> +#endif
add empty line here
> +#endif
add /* __XEN_DT_OVERLAY_H__ */ next to last #endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
~Michal
next prev parent reply other threads:[~2023-05-09 14:31 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-02 23:36 [XEN][PATCH v6 00/19] dynamic node programming using overlay dtbo Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 01/19] xen/arm/device: Remove __init from function type Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 02/19] common/device_tree: handle memory allocation failure in __unflatten_device_tree() Vikram Garhwal
2023-05-04 3:54 ` Henry Wang
2023-05-05 9:38 ` Michal Orzel
[not found] ` <f854bfc4-1f3c-19df-ba22-89c8859cfe6d@amd.com>
2023-06-01 6:58 ` Michal Orzel
2023-05-02 23:36 ` [XEN][PATCH v6 03/19] common/device_tree: change __unflatten_device_tree() type Vikram Garhwal
2023-05-04 4:02 ` Henry Wang
2023-05-02 23:36 ` [XEN][PATCH v6 04/19] common/device_tree.c: unflatten_device_tree() propagate errors Vikram Garhwal
2023-05-04 4:08 ` Henry Wang
2023-05-02 23:36 ` [XEN][PATCH v6 05/19] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
2023-05-04 4:11 ` Henry Wang
2023-05-09 10:54 ` Michal Orzel
2023-05-02 23:36 ` [XEN][PATCH v6 06/19] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 07/19] libfdt: overlay: change overlay_get_target() Vikram Garhwal
2023-05-04 4:13 ` Henry Wang
2023-05-02 23:36 ` [XEN][PATCH v6 08/19] xen/device-tree: Add device_tree_find_node_by_path() to find nodes in device tree Vikram Garhwal
2023-05-04 4:23 ` Henry Wang
2023-05-04 5:56 ` unsubscribe Terry Yang
2023-05-09 11:29 ` [XEN][PATCH v6 08/19] xen/device-tree: Add device_tree_find_node_by_path() to find nodes in device tree Michal Orzel
2023-05-31 21:31 ` Vikram Garhwal
2023-06-01 2:02 ` Henry Wang
2023-05-02 23:36 ` [XEN][PATCH v6 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller Vikram Garhwal
2023-05-03 7:53 ` Jan Beulich
2023-05-02 23:36 ` [XEN][PATCH v6 10/19] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 11/19] xen/iommu: Introduce iommu_remove_dt_device() Vikram Garhwal
2023-05-03 7:56 ` Jan Beulich
2023-05-02 23:36 ` [XEN][PATCH v6 12/19] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 13/19] asm/smp.h: Fix circular dependency for device_tree.h and rwlock.h Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 14/19] common/device_tree: Add rwlock for dt_host Vikram Garhwal
2023-05-04 4:38 ` Henry Wang
2023-05-02 23:36 ` [XEN][PATCH v6 15/19] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
2023-05-03 7:59 ` Jan Beulich
2023-05-03 8:02 ` Jan Beulich
2023-05-09 14:30 ` Michal Orzel [this message]
2023-05-09 14:52 ` Andrew Cooper
2023-05-02 23:36 ` [XEN][PATCH v6 16/19] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
2023-05-10 10:18 ` Michal Orzel
2023-06-02 0:49 ` Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 17/19] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
2023-05-18 16:01 ` Anthony PERARD
2023-06-02 0:49 ` Vikram Garhwal
2023-05-02 23:36 ` [XEN][PATCH v6 18/19] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
2023-05-18 16:01 ` Anthony PERARD
2023-05-02 23:36 ` [XEN][PATCH v6 19/19] tools/xl: Add new xl command overlay for device tree overlay support Vikram Garhwal
2023-05-18 16:01 ` Anthony PERARD
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8c96d009-bd81-66c7-fd23-1f11bc07e72f@amd.com \
--to=michal.orzel@amd.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=sstabellini@kernel.org \
--cc=vikram.garhwal@amd.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.