[XEN][PATCH v9 00/19] dynamic node programming using overlay dtbo

[XEN][PATCH v9 00/19] dynamic node programming using overlay dtbo

[Vikram Garhwal]

Hi,
This patch series is for introducing dynamic programming i.e. add/remove the
devices during run time. Using "xl dt_overlay" a device can be added/removed
with dtbo.

For adding a node using dynamic programming:
    1. flatten device tree overlay node will be added to a fdt
    2. Updated fdt will be unflattened to a new dt_host_new
    3. Extract the newly added node information from dt_host_new
    4. Add the added node under correct parent in original dt_host.
    3. Map/Permit interrupt and iomem region as required.

For removing a node:
    1. Find the node with given path.
    2. Check if the node is used by any of domus. Removes the node only when
        it's not used by any domain.
    3. Removes IRQ permissions and MMIO access.
    5. Find the node in dt_host and delete the device node entry from dt_host.
    6. Free the overlay_tracker entry which means free dt_host_new also(created
in adding node step).

The main purpose of this series to address first part of the 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 to a running domain
and will call unmap_mmio_regions() to remove the mapping.

Change Log:
 v5 -> v6:
    Add separate patch for memory allocation failure in __unflatten_device_tree().
    Move __unflatten_device_tree() function type changes to single patch.
    Add error propagation for failures in unflatten_dt_node.
    Change CONFIG_OVERLAY_DTB status to "ARM: Tech Preview".
    xen/smmu: Add remove_device callback for smmu_iommu ops:
        Added check to see if device is currently used.
    common/device_tree: Add rwlock for dt_host:
        Addressed feedback from Henry to rearrange code.
    xen/arm: Implement device tree node removal functionalities:
        Changed file name to dash format.
        Addressed Michal's comments.
    Rectified formatting related errors pointed by Michal.

 v4 -> v5:
    Split patch 01/16 to two patches. One with function type changes and another
        with changes inside unflatten_device_tree().
    Change dt_overlay xl command to dt-overlay.
    Protect overlay functionality with CONFIG(arm).
    Fix rwlock issues.
    Move include "device_tree.h" to c file where arch_cpu_init() is called and
        forward declare dt_device_node. This was done to avoid circular deps b/w
        device_tree.h and rwlock.h
    Address Michal's comment on coding style.

 v3 -> v4:
    Add support for adding node's children.
    Add rwlock to dt_host functions.
    Corrected fdt size issue when applying overlay into it.
    Add memory allocation fail handling for unflatten_device_tree().
    Changed xl overlay to xl dt_overlay.
    Correct commit messages.
    Addressed code issue from v3 review.

 v2 -> v3:
    Moved overlay functionalities to dt_overlay.c file.
    Renamed XEN_SYSCTL_overlay to XEN_SYSCTL_dt_overlay.
    Add dt_* prefix to overlay_add/remove_nodes.
    Added dtdevs_lock to protect iommu_add_dt_device().
    For iommu, moved spin_lock to caller.
    Address code issue from v2 review.

 v1 -> v2:
    Add support for multiple node addition/removal using dtbo.
    Replaced fpga-add and fpga-remove with one hypercall overlay_op.
    Moved common domain_build.c function to device.c
    Add OVERLAY_DTB configuration.
    Renamed overlay_get_target() to fdt_overlay_get_target().
    Split remove_device patch into two patches.
    Moved overlay_add/remove code to sysctl and changed it from domctl to sysctl.
    Added all overlay code under CONFIG_OVERLAY_DTB
    Renamed all tool domains fpga function to overlay
    Addressed code issues from v1 review.

Regards,
Vikram

Vikram Garhwal (19):
  common/device_tree: handle memory allocation failure in
    __unflatten_device_tree()
  common/device_tree.c: unflatten_device_tree() propagate errors
  xen/arm/device: Remove __init from function type
  common/device_tree: Export __unflatten_device_tree()
  xen/arm: Add CONFIG_OVERLAY_DTB
  libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
  libfdt: overlay: change overlay_get_target()
  xen/device-tree: Add device_tree_find_node_by_path() to find nodes in
    device tree
  xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
  xen/iommu: protect iommu_add_dt_device() with dtdevs_lock
  xen/iommu: Introduce iommu_remove_dt_device()
  xen/smmu: Add remove_device callback for smmu_iommu ops
  asm/smp.h: Fix circular dependency for device_tree.h and rwlock.h
  common/device_tree: Add rwlock for dt_host
  xen/arm: Implement device tree node removal functionalities
  xen/arm: Implement device tree node addition functionalities
  tools/libs/ctrl: Implement new xc interfaces for dt overlay
  tools/libs/light: Implement new libxl functions for device tree
    overlay ops
  tools/xl: Add new xl command overlay for device tree overlay support

 CHANGELOG.md                            |    3 +-
 MAINTAINERS                             |    1 +
 SUPPORT.md                              |    6 +
 tools/include/libxl.h                   |   11 +
 tools/include/xenctrl.h                 |    5 +
 tools/libs/ctrl/Makefile.common         |    1 +
 tools/libs/ctrl/xc_dt_overlay.c         |   51 ++
 tools/libs/light/Makefile               |    3 +
 tools/libs/light/libxl_dt_overlay.c     |   71 ++
 tools/xl/xl.h                           |    1 +
 tools/xl/xl_cmdtable.c                  |    6 +
 tools/xl/xl_vmcontrol.c                 |   52 ++
 xen/arch/arm/Kconfig                    |    5 +
 xen/arch/arm/device.c                   |  149 ++++
 xen/arch/arm/domain_build.c             |  147 ----
 xen/arch/arm/include/asm/domain_build.h |    2 -
 xen/arch/arm/include/asm/setup.h        |    6 +
 xen/arch/arm/include/asm/smp.h          |    4 +-
 xen/arch/arm/smpboot.c                  |    1 +
 xen/arch/arm/sysctl.c                   |   16 +-
 xen/common/Makefile                     |    1 +
 xen/common/device_tree.c                |   62 +-
 xen/common/dt-overlay.c                 | 1001 +++++++++++++++++++++++
 xen/common/libfdt/Makefile              |    4 +
 xen/common/libfdt/fdt_overlay.c         |   29 +-
 xen/common/libfdt/version.lds           |    1 +
 xen/drivers/passthrough/arm/smmu.c      |   63 ++
 xen/drivers/passthrough/device_tree.c   |   94 ++-
 xen/include/public/sysctl.h             |   24 +
 xen/include/xen/device_tree.h           |   36 +-
 xen/include/xen/dt-overlay.h            |   63 ++
 xen/include/xen/iommu-private.h         |   28 +
 xen/include/xen/iommu.h                 |    1 +
 xen/include/xen/libfdt/libfdt.h         |   18 +
 34 files changed, 1762 insertions(+), 204 deletions(-)
 create mode 100644 tools/libs/ctrl/xc_dt_overlay.c
 create mode 100644 tools/libs/light/libxl_dt_overlay.c
 create mode 100644 xen/common/dt-overlay.c
 create mode 100644 xen/include/xen/dt-overlay.h
 create mode 100644 xen/include/xen/iommu-private.h

-- 
2.17.1

[XEN][PATCH v9 01/19] common/device_tree: handle memory allocation failure in __unflatten_device_tree()

[Vikram Garhwal]

Change __unflatten_device_tree() return type to integer so it can propagate
memory allocation failure. Add panic() in dt_unflatten_host_device_tree() for
memory allocation failure during boot.

Fixes: fb97eb614acf ("xen/arm: Create a hierarchical device tree")
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 xen/common/device_tree.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 0522fdf976..c91d54c493 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -2092,8 +2092,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
  * @fdt: The fdt to expand
  * @mynodes: The device_node tree created by the call
  */
-static void __init __unflatten_device_tree(const void *fdt,
-                                           struct dt_device_node **mynodes)
+static int __init __unflatten_device_tree(const void *fdt,
+                                          struct dt_device_node **mynodes)
 {
     unsigned long start, mem, size;
     struct dt_device_node **allnextp = mynodes;
@@ -2114,6 +2114,8 @@ static void __init __unflatten_device_tree(const void *fdt,
 
     /* Allocate memory for the expanded device tree */
     mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node));
+    if ( !mem )
+        return -ENOMEM;
 
     ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeefU);
 
@@ -2131,6 +2133,8 @@ static void __init __unflatten_device_tree(const void *fdt,
     *allnextp = NULL;
 
     dt_dprintk(" <- unflatten_device_tree()\n");
+
+    return 0;
 }
 
 static void dt_alias_add(struct dt_alias_prop *ap,
@@ -2215,7 +2219,11 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
 
 void __init dt_unflatten_host_device_tree(void)
 {
-    __unflatten_device_tree(device_tree_flattened, &dt_host);
+    int error = __unflatten_device_tree(device_tree_flattened, &dt_host);
+
+    if ( error )
+        panic("__unflatten_device_tree failed with error %d\n", error);
+
     dt_alias_scan();
 }
 
-- 
2.17.1

[XEN][PATCH v9 02/19] common/device_tree.c: unflatten_device_tree() propagate errors

[Vikram Garhwal]

This will be useful in dynamic node programming when new dt nodes are unflattend
during runtime. Invalid device tree node related errors should be propagated
back to the caller.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v7:
    Free allocated memory in case of errors when calling unflatten_dt_node.
---
---
 xen/common/device_tree.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index c91d54c493..cd9a6a5c93 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -2108,6 +2108,9 @@ static int __init __unflatten_device_tree(const void *fdt,
     /* First pass, scan for size */
     start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
     size = unflatten_dt_node(fdt, 0, &start, NULL, NULL, 0);
+    if ( !size )
+        return -EINVAL;
+
     size = (size | 3) + 1;
 
     dt_dprintk("  size is %#lx allocating...\n", size);
@@ -2125,11 +2128,21 @@ static int __init __unflatten_device_tree(const void *fdt,
     start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
     unflatten_dt_node(fdt, mem, &start, NULL, &allnextp, 0);
     if ( be32_to_cpup((__be32 *)start) != FDT_END )
-        printk(XENLOG_WARNING "Weird tag at end of tree: %08x\n",
+    {
+        printk(XENLOG_ERR "Weird tag at end of tree: %08x\n",
                   *((u32 *)start));
+        xfree((__be64 *)mem);
+        return -EINVAL;
+    }
+
     if ( be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeefU )
-        printk(XENLOG_WARNING "End of tree marker overwritten: %08x\n",
+    {
+        printk(XENLOG_ERR "End of tree marker overwritten: %08x\n",
                   be32_to_cpu(((__be32 *)mem)[size / 4]));
+        xfree((__be64 *)mem);
+        return -EINVAL;
+    }
+
     *allnextp = NULL;
 
     dt_dprintk(" <- unflatten_device_tree()\n");
-- 
2.17.1

[XEN][PATCH v9 03/19] xen/arm/device: Remove __init from function type

[Vikram Garhwal]

Remove __init from following function to access during runtime:
    1. map_irq_to_domain()
    2. handle_device_interrupts()
    3. map_range_to_domain()
    4. unflatten_dt_node()

Move map_irq_to_domain() prototype from domain_build.h to setup.h.

To avoid breaking the build, following changes are also done:
1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain()
    to device.c. After removing __init type,  these functions are not specific
    to domain building, so moving them out of domain_build.c to device.c.
2. Remove static type from handle_device_interrupt().

Overall, these changes are done to support the dynamic programming of a nodes
where an overlay node will be added to fdt and unflattened node will be added to
dt_host. Furthermore, IRQ and mmio mapping will be done for the added node.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/device.c                   | 149 ++++++++++++++++++++++++
 xen/arch/arm/domain_build.c             | 147 -----------------------
 xen/arch/arm/include/asm/domain_build.h |   2 -
 xen/arch/arm/include/asm/setup.h        |   6 +
 xen/common/device_tree.c                |  12 +-
 5 files changed, 161 insertions(+), 155 deletions(-)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index ca8539dee5..1652d765bd 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -9,8 +9,10 @@
  */
 
 #include <asm/device.h>
+#include <asm/setup.h>
 #include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/iocap.h>
 #include <xen/lib.h>
 
 extern const struct device_desc _sdevice[], _edevice[];
@@ -75,6 +77,153 @@ enum device_class device_get_class(const struct dt_device_node *dev)
     return DEVICE_UNKNOWN;
 }
 
+int map_irq_to_domain(struct domain *d, unsigned int irq,
+                      bool need_mapping, const char *devname)
+{
+    int res;
+
+    res = irq_permit_access(d, irq);
+    if ( res )
+    {
+        printk(XENLOG_ERR "Unable to permit to %pd access to IRQ %u\n", d, irq);
+        return res;
+    }
+
+    if ( need_mapping )
+    {
+        /*
+         * Checking the return of vgic_reserve_virq is not
+         * necessary. It should not fail except when we try to map
+         * the IRQ twice. This can legitimately happen if the IRQ is shared
+         */
+        vgic_reserve_virq(d, irq);
+
+        res = route_irq_to_guest(d, irq, irq, devname);
+        if ( res < 0 )
+        {
+            printk(XENLOG_ERR "Unable to map IRQ%u to %pd\n", irq, d);
+            return res;
+        }
+    }
+
+    dt_dprintk("  - IRQ: %u\n", irq);
+    return 0;
+}
+
+int map_range_to_domain(const struct dt_device_node *dev,
+                        uint64_t addr, uint64_t len, void *data)
+{
+    struct map_range_data *mr_data = data;
+    struct domain *d = mr_data->d;
+    int res;
+
+    if ( (addr != (paddr_t)addr) || (((paddr_t)~0 - addr) < len) )
+    {
+        printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the maximum allowed PA width (%u bits)",
+               dt_node_full_name(dev), addr, (addr + len), PADDR_BITS);
+        return -ERANGE;
+    }
+
+    /*
+     * reserved-memory regions are RAM carved out for a special purpose.
+     * They are not MMIO and therefore a domain should not be able to
+     * manage them via the IOMEM interface.
+     */
+    if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
+                     strlen("/reserved-memory/")) != 0 )
+    {
+        res = iomem_permit_access(d, paddr_to_pfn(addr),
+                paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to permit to dom%d access to"
+                    " 0x%"PRIx64" - 0x%"PRIx64"\n",
+                    d->domain_id,
+                    addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
+            return res;
+        }
+    }
+
+    if ( !mr_data->skip_mapping )
+    {
+        res = map_regions_p2mt(d,
+                               gaddr_to_gfn(addr),
+                               PFN_UP(len),
+                               maddr_to_mfn(addr),
+                               mr_data->p2mt);
+
+        if ( res < 0 )
+        {
+            printk(XENLOG_ERR "Unable to map 0x%"PRIx64
+                   " - 0x%"PRIx64" in domain %d\n",
+                   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
+                   d->domain_id);
+            return res;
+        }
+    }
+
+    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
+               addr, addr + len, mr_data->p2mt);
+
+    return 0;
+}
+
+/*
+ * handle_device_interrupts retrieves the interrupts configuration from
+ * a device tree node and maps those interrupts to the target domain.
+ *
+ * Returns:
+ *   < 0 error
+ *   0   success
+ */
+int handle_device_interrupts(struct domain *d,
+                             struct dt_device_node *dev,
+                             bool need_mapping)
+{
+    unsigned int i, nirq;
+    int res;
+    struct dt_raw_irq rirq;
+
+    nirq = dt_number_of_irq(dev);
+
+    /* Give permission and map IRQs */
+    for ( i = 0; i < nirq; i++ )
+    {
+        res = dt_device_get_raw_irq(dev, i, &rirq);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
+                   i, dt_node_full_name(dev));
+            return res;
+        }
+
+        /*
+         * Don't map IRQ that have no physical meaning
+         * ie: IRQ whose controller is not the GIC
+         */
+        if ( rirq.controller != dt_interrupt_controller )
+        {
+            dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
+                      i, dt_node_full_name(rirq.controller));
+            continue;
+        }
+
+        res = platform_get_irq(dev, i);
+        if ( res < 0 )
+        {
+            printk(XENLOG_ERR "Unable to get irq %u for %s\n",
+                   i, dt_node_full_name(dev));
+            return res;
+        }
+
+        res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
+        if ( res )
+            return res;
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 39b4ee03a5..57b6a43314 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2293,39 +2293,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo)
     return res;
 }
 
-int __init map_irq_to_domain(struct domain *d, unsigned int irq,
-                             bool need_mapping, const char *devname)
-{
-    int res;
-
-    res = irq_permit_access(d, irq);
-    if ( res )
-    {
-        printk(XENLOG_ERR "Unable to permit to %pd access to IRQ %u\n", d, irq);
-        return res;
-    }
-
-    if ( need_mapping )
-    {
-        /*
-         * Checking the return of vgic_reserve_virq is not
-         * necessary. It should not fail except when we try to map
-         * the IRQ twice. This can legitimately happen if the IRQ is shared
-         */
-        vgic_reserve_virq(d, irq);
-
-        res = route_irq_to_guest(d, irq, irq, devname);
-        if ( res < 0 )
-        {
-            printk(XENLOG_ERR "Unable to map IRQ%u to %pd\n", irq, d);
-            return res;
-        }
-    }
-
-    dt_dprintk("  - IRQ: %u\n", irq);
-    return 0;
-}
-
 static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
                                        const struct dt_irq *dt_irq,
                                        void *data)
@@ -2355,64 +2322,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
     return res;
 }
 
-int __init map_range_to_domain(const struct dt_device_node *dev,
-                               uint64_t addr, uint64_t len, void *data)
-{
-    struct map_range_data *mr_data = data;
-    struct domain *d = mr_data->d;
-    int res;
-
-    if ( (addr != (paddr_t)addr) || (((paddr_t)~0 - addr) < len) )
-    {
-        printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the maximum allowed PA width (%u bits)",
-               dt_node_full_name(dev), addr, (addr + len), PADDR_BITS);
-        return -ERANGE;
-    }
-
-    /*
-     * reserved-memory regions are RAM carved out for a special purpose.
-     * They are not MMIO and therefore a domain should not be able to
-     * manage them via the IOMEM interface.
-     */
-    if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
-                     strlen("/reserved-memory/")) != 0 )
-    {
-        res = iomem_permit_access(d, paddr_to_pfn(addr),
-                paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
-        if ( res )
-        {
-            printk(XENLOG_ERR "Unable to permit to dom%d access to"
-                    " 0x%"PRIx64" - 0x%"PRIx64"\n",
-                    d->domain_id,
-                    addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
-            return res;
-        }
-    }
-
-    if ( !mr_data->skip_mapping )
-    {
-        res = map_regions_p2mt(d,
-                               gaddr_to_gfn(addr),
-                               PFN_UP(len),
-                               maddr_to_mfn(addr),
-                               mr_data->p2mt);
-
-        if ( res < 0 )
-        {
-            printk(XENLOG_ERR "Unable to map 0x%"PRIx64
-                   " - 0x%"PRIx64" in domain %d\n",
-                   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
-                   d->domain_id);
-            return res;
-        }
-    }
-
-    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
-               addr, addr + len, mr_data->p2mt);
-
-    return 0;
-}
-
 /*
  * For a node which describes a discoverable bus (such as a PCI bus)
  * then we may need to perform additional mappings in order to make
@@ -2440,62 +2349,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
     return 0;
 }
 
-/*
- * handle_device_interrupts retrieves the interrupts configuration from
- * a device tree node and maps those interrupts to the target domain.
- *
- * Returns:
- *   < 0 error
- *   0   success
- */
-static int __init handle_device_interrupts(struct domain *d,
-                                           struct dt_device_node *dev,
-                                           bool need_mapping)
-{
-    unsigned int i, nirq;
-    int res;
-    struct dt_raw_irq rirq;
-
-    nirq = dt_number_of_irq(dev);
-
-    /* Give permission and map IRQs */
-    for ( i = 0; i < nirq; i++ )
-    {
-        res = dt_device_get_raw_irq(dev, i, &rirq);
-        if ( res )
-        {
-            printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
-                   i, dt_node_full_name(dev));
-            return res;
-        }
-
-        /*
-         * Don't map IRQ that have no physical meaning
-         * ie: IRQ whose controller is not the GIC
-         */
-        if ( rirq.controller != dt_interrupt_controller )
-        {
-            dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
-                      i, dt_node_full_name(rirq.controller));
-            continue;
-        }
-
-        res = platform_get_irq(dev, i);
-        if ( res < 0 )
-        {
-            printk(XENLOG_ERR "Unable to get irq %u for %s\n",
-                   i, dt_node_full_name(dev));
-            return res;
-        }
-
-        res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
-        if ( res )
-            return res;
-    }
-
-    return 0;
-}
-
 /*
  * For a given device node:
  *  - Give permission to the guest to manage IRQ and MMIO range
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index 34ceddc995..b9329c9ee0 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -4,8 +4,6 @@
 #include <xen/sched.h>
 #include <asm/kernel.h>
 
-int map_irq_to_domain(struct domain *d, unsigned int irq,
-                      bool need_mapping, const char *devname);
 int make_chosen_node(const struct kernel_info *kinfo);
 void evtchn_allocate(struct domain *d);
 
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 19dc637d55..f532332d6c 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -165,9 +165,15 @@ void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
 u32 device_tree_get_u32(const void *fdt, int node,
                         const char *prop_name, u32 dflt);
 
+int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
+                             bool need_mapping);
+
 int map_range_to_domain(const struct dt_device_node *dev,
                         uint64_t addr, uint64_t len, void *data);
 
+int map_irq_to_domain(struct domain *d, unsigned int irq,
+                      bool need_mapping, const char *devname);
+
 extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
 
 #ifdef CONFIG_ARM_64
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index cd9a6a5c93..d70b6a7ac9 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1847,12 +1847,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
  * @allnextpp: pointer to ->allnext from last allocated device_node
  * @fpsize: Size of the node path up at the current depth.
  */
-static unsigned long __init unflatten_dt_node(const void *fdt,
-                                              unsigned long mem,
-                                              unsigned long *p,
-                                              struct dt_device_node *dad,
-                                              struct dt_device_node ***allnextpp,
-                                              unsigned long fpsize)
+static unsigned long unflatten_dt_node(const void *fdt,
+                                       unsigned long mem,
+                                       unsigned long *p,
+                                       struct dt_device_node *dad,
+                                       struct dt_device_node ***allnextpp,
+                                       unsigned long fpsize)
 {
     struct dt_device_node *np;
     struct dt_property *pp, **prev_pp = NULL;
-- 
2.17.1

[XEN][PATCH v9 04/19] common/device_tree: Export __unflatten_device_tree()

[Vikram Garhwal]

Following changes are done to __unflatten_device_tree():
    1. __unflatten_device_tree() is renamed to unflatten_device_tree().
    2. Remove __init and static function type.

The changes are done to make this function useable for dynamic node programming
where new device tree overlay nodes are added to fdt and further unflattend to
update xen device tree during runtime.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v7:
    Update the commit with why unflatten_device_tree() is changed.
    Move function documentation to device_tree.h
---
---
 xen/common/device_tree.c      | 17 +++--------------
 xen/include/xen/device_tree.h | 12 ++++++++++++
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index d70b6a7ac9..67e9b6de3e 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -2082,18 +2082,7 @@ static unsigned long unflatten_dt_node(const void *fdt,
     return mem;
 }
 
-/**
- * __unflatten_device_tree - create tree of device_nodes from flat blob
- *
- * unflattens a device-tree, creating the
- * tree of struct device_node. It also fills the "name" and "type"
- * pointers of the nodes so the normal device-tree walking functions
- * can be used.
- * @fdt: The fdt to expand
- * @mynodes: The device_node tree created by the call
- */
-static int __init __unflatten_device_tree(const void *fdt,
-                                          struct dt_device_node **mynodes)
+int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
 {
     unsigned long start, mem, size;
     struct dt_device_node **allnextp = mynodes;
@@ -2232,10 +2221,10 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
 
 void __init dt_unflatten_host_device_tree(void)
 {
-    int error = __unflatten_device_tree(device_tree_flattened, &dt_host);
+    int error = unflatten_device_tree(device_tree_flattened, &dt_host);
 
     if ( error )
-        panic("__unflatten_device_tree failed with error %d\n", error);
+        panic("unflatten_device_tree failed with error %d\n", error);
 
     dt_alias_scan();
 }
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 1d79e23b28..5941599eff 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -178,6 +178,18 @@ int device_tree_for_each_node(const void *fdt, int node,
  */
 void dt_unflatten_host_device_tree(void);
 
+/**
+ * unflatten_device_tree - create tree of device_nodes from flat blob
+ *
+ * unflattens a device-tree, creating the
+ * tree of struct device_node. It also fills the "name" and "type"
+ * pointers of the nodes so the normal device-tree walking functions
+ * can be used.
+ * @fdt: The fdt to expand
+ * @mynodes: The device_node tree created by the call
+ */
+int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
+
 /**
  * IRQ translation callback
  * TODO: For the moment we assume that we only have ONE
-- 
2.17.1

[XEN][PATCH v9 05/19] xen/arm: Add CONFIG_OVERLAY_DTB

[Vikram Garhwal]

Introduce a config option where the user can enable support for adding/removing
device tree nodes using a device tree binary overlay.

Update SUPPORT.md and CHANGELOG.md to state the Device Tree Overlays support for
Arm.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Acked-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

---
Changes from v7:
    Add this feature as "experimental support" in CHANGELOG.md
---
---
 CHANGELOG.md         | 3 ++-
 SUPPORT.md           | 6 ++++++
 xen/arch/arm/Kconfig | 5 +++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7d7e0590f8..47098dbfca 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,7 +24,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  - xl/libxl can customize SMBIOS strings for HVM guests.
  - Add support for AVX512-FP16 on x86.
  - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
-
+ - On Arm, experimental support for dynamic addition/removal of Xen device tree
+   nodes using a device tree overlay binary(.dtbo).
 
 ## [4.17.0](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.17.0) - 2022-12-12
 
diff --git a/SUPPORT.md b/SUPPORT.md
index 35a6249e03..8eb006565c 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -844,6 +844,12 @@ No support for QEMU backends in a 16K or 64K domain.
 
     Status: Supported
 
+### Device Tree Overlays
+
+Add/Remove device tree nodes using a device tree overlay binary(.dtbo).
+
+    Status, ARM: Experimental
+
 ### ARM: Guest ACPI support
 
     Status: Supported
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index fd57a82dd2..02c4796438 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -92,6 +92,11 @@ config HAS_ITS
         bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
         depends on GICV3 && !NEW_VGIC && !ARM_32
 
+config OVERLAY_DTB
+	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
+	help
+	  Dynamic addition/removal of Xen device tree nodes using a dtbo.
+
 config HVM
         def_bool y
 
-- 
2.17.1

[XEN][PATCH v9 06/19] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.

[Vikram Garhwal]

This is done to access fdt library function which are required for adding device
tree overlay nodes for dynamic programming of nodes.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 xen/common/libfdt/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
index 75aaefa2e3..d50487aa6e 100644
--- a/xen/common/libfdt/Makefile
+++ b/xen/common/libfdt/Makefile
@@ -1,7 +1,11 @@
 include $(src)/Makefile.libfdt
 
 SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
+
+# For CONFIG_OVERLAY_DTB, libfdt functionalities will be needed during runtime.
+ifneq ($(CONFIG_OVERLAY_DTB),y)
 OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s))
+endif
 
 obj-y += libfdt.o
 nocov-y += libfdt.o
-- 
2.17.1

[XEN][PATCH v9 07/19] libfdt: overlay: change overlay_get_target()

[Vikram Garhwal]

Rename overlay_get_target() to fdt_overlay_target_offset() and remove static
function type.

This is done to get the target path for the overlay nodes which is very useful
in many cases. For example, Xen hypervisor needs it when applying overlays
because Xen needs to do further processing of the overlay nodes, e.g. mapping of
resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.

Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Message-Id: <1637204036-382159-2-git-send-email-fnu.vikram@xilinx.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Origin: git://git.kernel.org/pub/scm/utils/dtc/dtc.git 45f3d1a095dd

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Henry Wang <Henry.Wang@arm.com>
Acked-by: Juline Grall <jgrall@amazon.com>
---
 xen/common/libfdt/fdt_overlay.c | 29 +++++++----------------------
 xen/common/libfdt/version.lds   |  1 +
 xen/include/xen/libfdt/libfdt.h | 18 ++++++++++++++++++
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/xen/common/libfdt/fdt_overlay.c b/xen/common/libfdt/fdt_overlay.c
index 7b95e2b639..acf0c4c2a6 100644
--- a/xen/common/libfdt/fdt_overlay.c
+++ b/xen/common/libfdt/fdt_overlay.c
@@ -41,37 +41,22 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
 	return fdt32_to_cpu(*val);
 }
 
-/**
- * overlay_get_target - retrieves the offset of a fragment's target
- * @fdt: Base device tree blob
- * @fdto: Device tree overlay blob
- * @fragment: node offset of the fragment in the overlay
- * @pathp: pointer which receives the path of the target (or NULL)
- *
- * overlay_get_target() retrieves the target offset in the base
- * device tree of a fragment, no matter how the actual targeting is
- * done (through a phandle or a path)
- *
- * returns:
- *      the targeted node offset in the base device tree
- *      Negative error code on error
- */
-static int overlay_get_target(const void *fdt, const void *fdto,
-			      int fragment, char const **pathp)
+int fdt_overlay_target_offset(const void *fdt, const void *fdto,
+			      int fragment_offset, char const **pathp)
 {
 	uint32_t phandle;
 	const char *path = NULL;
 	int path_len = 0, ret;
 
 	/* Try first to do a phandle based lookup */
-	phandle = overlay_get_target_phandle(fdto, fragment);
+	phandle = overlay_get_target_phandle(fdto, fragment_offset);
 	if (phandle == (uint32_t)-1)
 		return -FDT_ERR_BADPHANDLE;
 
 	/* no phandle, try path */
 	if (!phandle) {
 		/* And then a path based lookup */
-		path = fdt_getprop(fdto, fragment, "target-path", &path_len);
+		path = fdt_getprop(fdto, fragment_offset, "target-path", &path_len);
 		if (path)
 			ret = fdt_path_offset(fdt, path);
 		else
@@ -638,7 +623,7 @@ static int overlay_merge(void *fdt, void *fdto)
 		if (overlay < 0)
 			return overlay;
 
-		target = overlay_get_target(fdt, fdto, fragment, NULL);
+		target = fdt_overlay_target_offset(fdt, fdto, fragment, NULL);
 		if (target < 0)
 			return target;
 
@@ -781,7 +766,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
 			return -FDT_ERR_BADOVERLAY;
 
 		/* get the target of the fragment */
-		ret = overlay_get_target(fdt, fdto, fragment, &target_path);
+		ret = fdt_overlay_target_offset(fdt, fdto, fragment, &target_path);
 		if (ret < 0)
 			return ret;
 		target = ret;
@@ -803,7 +788,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
 
 		if (!target_path) {
 			/* again in case setprop_placeholder changed it */
-			ret = overlay_get_target(fdt, fdto, fragment, &target_path);
+			ret = fdt_overlay_target_offset(fdt, fdto, fragment, &target_path);
 			if (ret < 0)
 				return ret;
 			target = ret;
diff --git a/xen/common/libfdt/version.lds b/xen/common/libfdt/version.lds
index 7ab85f1d9d..cbce5d4a8b 100644
--- a/xen/common/libfdt/version.lds
+++ b/xen/common/libfdt/version.lds
@@ -77,6 +77,7 @@ LIBFDT_1.2 {
 		fdt_appendprop_addrrange;
 		fdt_setprop_inplace_namelen_partial;
 		fdt_create_with_flags;
+		fdt_overlay_target_offset;
 	local:
 		*;
 };
diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
index c71689e2be..fabddbee8c 100644
--- a/xen/include/xen/libfdt/libfdt.h
+++ b/xen/include/xen/libfdt/libfdt.h
@@ -2109,6 +2109,24 @@ int fdt_del_node(void *fdt, int nodeoffset);
  */
 int fdt_overlay_apply(void *fdt, void *fdto);
 
+/**
+ * fdt_overlay_target_offset - retrieves the offset of a fragment's target
+ * @fdt: Base device tree blob
+ * @fdto: Device tree overlay blob
+ * @fragment_offset: node offset of the fragment in the overlay
+ * @pathp: pointer which receives the path of the target (or NULL)
+ *
+ * fdt_overlay_target_offset() retrieves the target offset in the base
+ * device tree of a fragment, no matter how the actual targeting is
+ * done (through a phandle or a path)
+ *
+ * returns:
+ *      the targeted node offset in the base device tree
+ *      Negative error code on error
+ */
+int fdt_overlay_target_offset(const void *fdt, const void *fdto,
+			      int fragment_offset, char const **pathp);
+
 /**********************************************************************/
 /* Debugging / informational functions                                */
 /**********************************************************************/
-- 
2.17.1

[XEN][PATCH v9 08/19] xen/device-tree: Add device_tree_find_node_by_path() to find nodes in device tree

[Vikram Garhwal]

Add device_tree_find_node_by_path() to find a matching node with path for a
dt_device_node.

Reason behind this function:
    Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
    device_tree_flattened) is created and updated with overlay nodes. This
    updated fdt is further unflattened to a dt_host_new. Next, we need to find
    the overlay nodes in dt_host_new, find the overlay node's parent in dt_host
    and add the nodes as child under their parent in the dt_host. Thus we need
    this function to search for node in different unflattened device trees.

Also, make dt_find_node_by_path() static inline.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v7:
    Rename device_tree_find_node_by_path() to dt_find_node_by_path_from().
    Fix indentation.
---
---
 xen/common/device_tree.c      |  5 +++--
 xen/include/xen/device_tree.h | 18 ++++++++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 67e9b6de3e..0f10037745 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -358,11 +358,12 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
     return np;
 }
 
-struct dt_device_node *dt_find_node_by_path(const char *path)
+struct dt_device_node *dt_find_node_by_path_from(struct dt_device_node *from,
+                                                 const char *path)
 {
     struct dt_device_node *np;
 
-    dt_for_each_device_node(dt_host, np)
+    dt_for_each_device_node(from, np)
         if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
             break;
 
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5941599eff..e507658b23 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -568,13 +568,27 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
 struct dt_device_node *dt_find_node_by_alias(const char *alias);
 
 /**
- * dt_find_node_by_path - Find a node matching a full DT path
+ * dt_find_node_by_path_from - Generic function to find a node matching the
+ * full DT path for any given unflatten device tree
+ * @from: The device tree node to start searching from
  * @path: The full path to match
  *
  * Returns a node pointer.
  */
-struct dt_device_node *dt_find_node_by_path(const char *path);
+struct dt_device_node *
+                    dt_find_node_by_path_from(struct dt_device_node *from,
+                                                  const char *path);
 
+/**
+ * dt_find_node_by_path - Find a node matching a full DT path in dt_host
+ * @path: The full path to match
+ *
+ * Returns a node pointer.
+ */
+static inline struct dt_device_node *dt_find_node_by_path(const char *path)
+{
+    return dt_find_node_by_path_from(dt_host, path);
+}
 
 /**
  * dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the
-- 
2.17.1

[XEN][PATCH v9 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Vikram Garhwal]

Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
Remove static type so this can also be used by SMMU drivers to check if the
device is being used before removing.

Moving spin_lock to caller was done to prevent the concurrent access to
iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
patches in this series introduces node add/remove feature.

Also, caller is required hold the correct lock so moved the function prototype
to a private header.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v7:
    Update commit message.
    Add ASSERT().
---
---
 xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
 xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)
 create mode 100644 xen/include/xen/iommu-private.h

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 1c32d7b50c..5796ee1f93 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -18,6 +18,7 @@
 #include <xen/device_tree.h>
 #include <xen/guest_access.h>
 #include <xen/iommu.h>
+#include <xen/iommu-private.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xsm/xsm.h>
@@ -83,16 +84,16 @@ fail:
     return rc;
 }
 
-static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
 {
     bool_t assigned = 0;
 
+    ASSERT(spin_is_locked(&dtdevs_lock));
+
     if ( !dt_device_is_protected(dev) )
         return 0;
 
-    spin_lock(&dtdevs_lock);
     assigned = !list_empty(&dev->domain_list);
-    spin_unlock(&dtdevs_lock);
 
     return assigned;
 }
@@ -213,27 +214,44 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
         if ( (d && d->is_dying) || domctl->u.assign_device.flags )
             break;
 
+        /*
+         * To ensure that the dev doesn't disappear between the time we look it
+         * up with dt_find_node_by_gpath() and we check the assignment later.
+         */
+        spin_lock(&dtdevs_lock);
+
         ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
                                     domctl->u.assign_device.u.dt.size,
                                     &dev);
         if ( ret )
+        {
+            spin_unlock(&dtdevs_lock);
             break;
+        }
 
         ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
         if ( ret )
+        {
+            spin_unlock(&dtdevs_lock);
             break;
+        }
 
         if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
         {
-            if ( iommu_dt_device_is_assigned(dev) )
+
+            if ( iommu_dt_device_is_assigned_locked(dev) )
             {
                 printk(XENLOG_G_ERR "%s already assigned.\n",
                        dt_node_full_name(dev));
                 ret = -EINVAL;
             }
+
+            spin_unlock(&dtdevs_lock);
             break;
         }
 
+        spin_unlock(&dtdevs_lock);
+
         if ( d == dom_io )
             return -EINVAL;
 
diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h
new file mode 100644
index 0000000000..bb5c94e7a6
--- /dev/null
+++ b/xen/include/xen/iommu-private.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/iommu-private.h
+ */
+#ifndef __XEN_IOMMU_PRIVATE_H__
+#define __XEN_IOMMU_PRIVATE_H__
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+#include <xen/device_tree.h>
+
+/*
+ * Checks if dt_device_node is assigned to a domain or not. This function
+ * expects to be called with dtdevs_lock acquired by caller.
+ */
+bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
+#endif
+
+#endif /* __XEN_IOMMU_PRIVATE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.1

[XEN][PATCH v9 10/19] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock

[Vikram Garhwal]

Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access
to add/remove/assign/deassign.
With addition of dynamic programming feature(follow-up patches in this series),
this function can be concurrently access by pci device assign/deassign and also
by dynamic node add/remove using device tree overlays.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

---
    Changes from v7:
        Update commit message and fix indent.
---
---
 xen/drivers/passthrough/device_tree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 5796ee1f93..096ef2dd68 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -148,6 +148,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
     if ( dev_iommu_fwspec_get(dev) )
         return 0;
 
+    spin_lock(&dtdevs_lock);
+
     /*
      * According to the Documentation/devicetree/bindings/iommu/iommu.txt
      * from Linux.
@@ -160,7 +162,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
          * these callback implemented.
          */
         if ( !ops->add_device || !ops->dt_xlate )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            goto fail;
+        }
 
         if ( !dt_device_is_available(iommu_spec.np) )
             break;
@@ -191,6 +196,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
     if ( rc < 0 )
         iommu_fwspec_free(dev);
 
+ fail:
+    spin_unlock(&dtdevs_lock);
     return rc;
 }
 
-- 
2.17.1

[XEN][PATCH v9 11/19] xen/iommu: Introduce iommu_remove_dt_device()

[Vikram Garhwal]

Remove master device from the IOMMU. This will be helpful when removing the
overlay nodes using dynamic programming during run time.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Changes from v7:
    Add check if IOMMU is enabled.
    Fix indentation of fail.
---
---
 xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++
 xen/include/xen/iommu.h               |  1 +
 2 files changed, 45 insertions(+)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 096ef2dd68..4cb32dc0b3 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d)
     return 0;
 }
 
+int iommu_remove_dt_device(struct dt_device_node *np)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    struct device *dev = dt_to_dev(np);
+    int rc;
+
+    if ( !iommu_enabled )
+        return 1;
+
+    if ( !ops )
+        return -EOPNOTSUPP;
+
+    spin_lock(&dtdevs_lock);
+
+    if ( iommu_dt_device_is_assigned_locked(np) )
+    {
+        rc = -EBUSY;
+        goto fail;
+    }
+
+    /*
+     * The driver which supports generic IOMMU DT bindings must have this
+     * callback implemented.
+     */
+    if ( !ops->remove_device )
+    {
+        rc = -EOPNOTSUPP;
+        goto fail;
+    }
+
+    /*
+     * Remove master device from the IOMMU if latter is present and available.
+     * The driver is responsible for removing is_protected flag.
+     */
+    rc = ops->remove_device(0, dev);
+
+    if ( !rc )
+        iommu_fwspec_free(dev);
+
+ fail:
+    spin_unlock(&dtdevs_lock);
+    return rc;
+}
+
 int iommu_add_dt_device(struct dt_device_node *np)
 {
     const struct iommu_ops *ops = iommu_get_ops();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 110693c59f..a8e9bc9a2d 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np);
 
 int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
+int iommu_remove_dt_device(struct dt_device_node *np);
 
 #endif /* HAS_DEVICE_TREE */
 
-- 
2.17.1

[XEN][PATCH v9 12/19] xen/smmu: Add remove_device callback for smmu_iommu ops

[Vikram Garhwal]

Add remove_device callback for removing the device entry from smmu-master using
following steps:
1. Find if SMMU master exists for the device node.
2. Check if device is currently in use.
3. Remove the SMMU master.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

---
 Changes from v7:
        Added comments on arm_smmu_dt_remove_device_generic().
---
---
 xen/drivers/passthrough/arm/smmu.c | 63 ++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index c37fa9af13..e1e8e4528d 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -42,6 +42,7 @@
 #include <xen/errno.h>
 #include <xen/err.h>
 #include <xen/irq.h>
+#include <xen/iommu-private.h>
 #include <xen/lib.h>
 #include <xen/list.h>
 #include <xen/mm.h>
@@ -815,6 +816,19 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 	return 0;
 }
 
+static int remove_smmu_master(struct arm_smmu_device *smmu,
+			      struct arm_smmu_master *master)
+{
+	if (!smmu->masters.rb_node) {
+		ASSERT_UNREACHABLE();
+		return -ENOENT;
+	}
+
+	rb_erase(&master->node, &smmu->masters);
+
+	return 0;
+}
+
 static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
 					 struct device *dev,
 					 struct iommu_fwspec *fwspec)
@@ -852,6 +866,34 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
 	return insert_smmu_master(smmu, master);
 }
 
+static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
+					 struct device *dev)
+{
+	struct arm_smmu_master *master;
+	struct device_node *dev_node = dev_get_dev_node(dev);
+	int ret;
+
+	master = find_smmu_master(smmu, dev_node);
+	if (master == NULL) {
+		dev_err(dev,
+			"No registrations found for master device %s\n",
+			dev_node->name);
+		return -EINVAL;
+	}
+
+	if (iommu_dt_device_is_assigned_locked(dev_to_dt(dev)))
+		return -EBUSY;
+
+	ret = remove_smmu_master(smmu, master);
+	if (ret)
+		return ret;
+
+	dev_node->is_protected = false;
+
+	kfree(master);
+	return 0;
+}
+
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
 				struct of_phandle_args *masterspec)
@@ -875,6 +917,26 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 					     fwspec);
 }
 
+/*
+ * The driver which supports generic IOMMU DT bindings must have this
+ * callback implemented.
+ */
+static int arm_smmu_dt_remove_device_generic(u8 devfn, struct device *dev)
+{
+	struct arm_smmu_device *smmu;
+	struct iommu_fwspec *fwspec;
+
+	fwspec = dev_iommu_fwspec_get(dev);
+	if (fwspec == NULL)
+		return -ENXIO;
+
+	smmu = find_smmu(fwspec->iommu_dev);
+	if (smmu == NULL)
+		return -ENXIO;
+
+	return arm_smmu_dt_remove_device_legacy(smmu, dev);
+}
+
 static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -2859,6 +2921,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arch_iommu_hwdom_init,
     .add_device = arm_smmu_dt_add_device_generic,
+    .remove_device = arm_smmu_dt_remove_device_generic,
     .teardown = arm_smmu_iommu_domain_teardown,
     .iotlb_flush = arm_smmu_iotlb_flush,
     .assign_device = arm_smmu_assign_dev,
-- 
2.17.1

[XEN][PATCH v9 13/19] asm/smp.h: Fix circular dependency for device_tree.h and rwlock.h

[Vikram Garhwal]

Dynamic programming ops will modify the dt_host and there might be other
function which are browsing the dt_host at the same time. To avoid the race
conditions, we will need to add a rwlock to protect access to the dt_host.
However, adding rwlock in device_tree.h causes following circular dependency:
    device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h

To fix this, removed the "#include <xen/device_tree.h> and forward declared
"struct dt_device_node".

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

---
    Changes from v7:
        Move struct dt_device_node declaration just above arch_cpu_init().
---
---
 xen/arch/arm/include/asm/smp.h | 4 +++-
 xen/arch/arm/smpboot.c         | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h
index a37ca55bff..4fabdf5310 100644
--- a/xen/arch/arm/include/asm/smp.h
+++ b/xen/arch/arm/include/asm/smp.h
@@ -3,7 +3,6 @@
 
 #ifndef __ASSEMBLY__
 #include <xen/cpumask.h>
-#include <xen/device_tree.h>
 #include <asm/current.h>
 #endif
 
@@ -23,6 +22,9 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
 extern void noreturn stop_cpu(void);
 
 extern int arch_smp_init(void);
+
+struct dt_device_node;
+
 extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
 extern int arch_cpu_up(int cpu);
 extern void arch_cpu_up_finish(void);
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index e107b86b7b..eeb76cd551 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -10,6 +10,7 @@
 #include <xen/cpu.h>
 #include <xen/cpumask.h>
 #include <xen/delay.h>
+#include <xen/device_tree.h>
 #include <xen/domain_page.h>
 #include <xen/errno.h>
 #include <xen/init.h>
-- 
2.17.1

[XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal]

 Dynamic programming ops will modify the dt_host and there might be other
 function which are browsing the dt_host at the same time. To avoid the race
 conditions, adding rwlock for browsing the dt_host during runtime. dt_host
 writer will be added in the follow-up patch titled "xen/arm: Implement device
 tree node addition functionalities."

 Reason behind adding rwlock instead of spinlock:
    For now, dynamic programming is the sole modifier of dt_host in Xen during
        run time. All other access functions like iommu_release_dt_device() are
        just reading the dt_host during run-time. So, there is a need to protect
        others from browsing the dt_host while dynamic programming is modifying
        it. rwlock is better suitable for this task as spinlock won't be able to
        differentiate between read and write access.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v7:
    Keep one lock for dt_host instead of lock for each node under dt_host.
---
---
 xen/common/device_tree.c              |  5 +++++
 xen/drivers/passthrough/device_tree.c | 15 +++++++++++++++
 xen/include/xen/device_tree.h         |  6 ++++++
 3 files changed, 26 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 0f10037745..6b934fe036 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -31,6 +31,7 @@ dt_irq_xlate_func dt_irq_xlate;
 struct dt_device_node *dt_host;
 /* Interrupt controller node*/
 const struct dt_device_node *dt_interrupt_controller;
+rwlock_t dt_host_lock;
 
 /**
  * struct dt_alias_prop - Alias property in 'aliases' node
@@ -2137,7 +2138,11 @@ int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
 
     dt_dprintk(" <- unflatten_device_tree()\n");
 
+    /* Init r/w lock for host device tree. */
+    rwlock_init(&dt_host_lock);
+
     return 0;
+
 }
 
 static void dt_alias_add(struct dt_alias_prop *ap,
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 4cb32dc0b3..31815d2b60 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -114,6 +114,8 @@ int iommu_release_dt_devices(struct domain *d)
     if ( !is_iommu_enabled(d) )
         return 0;
 
+    read_lock(&dt_host_lock);
+
     list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list)
     {
         rc = iommu_deassign_dt_device(d, dev);
@@ -121,10 +123,14 @@ int iommu_release_dt_devices(struct domain *d)
         {
             dprintk(XENLOG_ERR, "Failed to deassign %s in domain %u\n",
                     dt_node_full_name(dev), d->domain_id);
+
+            read_unlock(&dt_host_lock);
             return rc;
         }
     }
 
+    read_unlock(&dt_host_lock);
+
     return 0;
 }
 
@@ -251,6 +257,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
     int ret;
     struct dt_device_node *dev;
 
+    read_lock(&dt_host_lock);
+
     switch ( domctl->cmd )
     {
     case XEN_DOMCTL_assign_device:
@@ -304,7 +312,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
         spin_unlock(&dtdevs_lock);
 
         if ( d == dom_io )
+        {
+            read_unlock(&dt_host_lock);
             return -EINVAL;
+        }
 
         ret = iommu_add_dt_device(dev);
         if ( ret < 0 )
@@ -342,7 +353,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
             break;
 
         if ( d == dom_io )
+        {
+            read_unlock(&dt_host_lock);
             return -EINVAL;
+        }
 
         ret = iommu_deassign_dt_device(d, dev);
 
@@ -357,5 +371,6 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
         break;
     }
 
+    read_unlock(&dt_host_lock);
     return ret;
 }
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index e507658b23..8191f30197 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -18,6 +18,7 @@
 #include <xen/string.h>
 #include <xen/types.h>
 #include <xen/list.h>
+#include <xen/rwlock.h>
 
 #define DEVICE_TREE_MAX_DEPTH 16
 
@@ -216,6 +217,11 @@ extern struct dt_device_node *dt_host;
  */
 extern const struct dt_device_node *dt_interrupt_controller;
 
+/*
+ * Lock that protects r/w updates to unflattened device tree i.e. dt_host.
+ */
+extern rwlock_t dt_host_lock;
+
 /**
  * Find the interrupt controller
  * For the moment we handle only one interrupt controller: the first
-- 
2.17.1

[XEN][PATCH v9 15/19] xen/arm: Implement device tree node removal functionalities

[Vikram Garhwal]

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.
Also, this is an experimental feature so it is expected from user to make sure
correct device tree overlays are used when adding nodes and making sure devices
are not being used by other domain before removing them from Xen tree.
Partially added/removed i.e. failures while removing the overlay may cause other
failures and might need a system reboot.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v8:
    Remove IRQs and IOMMU entries using rangesets instead of parsing each node.
Changes from v7:
    Add dt-overlay.c in MAINTAINERS.
    Add comments for dt_overlay_remove_node.
    Rename handle_remove_irq_iommu() to remove_resources().
    Add comment regarding false mapping flag for reason behind not removing the
    mapping..
    Remove irq_access_premitted() check.
    Add error handling for remove_all_descendant_nodes
    Change read_lock with write_lock.
    Remove check_overlay_fdt() call from handle_remove_overlay_nodes().
    Re-organize dt_sysctl and reutnr -EOPNOSTSUPP for error cases. Also, renamed
        this function to dt_overlay_sysctl.
    Remove unnecessary header includes in dt-overlay.h
    Correct indentation and make func   tion inputs const wherever possible.
    Make overlay_fdt const_void inside xen_sysctl_dt_overlay{}.
    Add comment regarding why we not removing IRQ and MMIO mappings.
    Move overlay_node_count() out of this patch as it's not being used here
        anymore.
Changes from v6:
    Add explicit padding for xen_system_dt_overlay{}
    Update license.
    Rearrange xfree in dt_sysctl()
    Update overlay_track struct comment with relevant message.
    Fix missing xen/errno.h for builds without CONFIG_OVERLAY_DTB cases.
    Fix header formatting.
---
---
 MAINTAINERS                  |   1 +
 xen/arch/arm/sysctl.c        |  16 +-
 xen/common/Makefile          |   1 +
 xen/common/dt-overlay.c      | 402 +++++++++++++++++++++++++++++++++++
 xen/include/public/sysctl.h  |  24 +++
 xen/include/xen/dt-overlay.h |  63 ++++++
 6 files changed, 506 insertions(+), 1 deletion(-)
 create mode 100644 xen/common/dt-overlay.c
 create mode 100644 xen/include/xen/dt-overlay.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a0805d35cd..c41a7c5440 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -301,6 +301,7 @@ M:	Julien Grall <julien@xen.org>
 S:	Supported
 F:	xen/common/libfdt/
 F:	xen/common/device_tree.c
+F:	xen/common/dt-overlay.c
 F:	xen/include/xen/libfdt/
 F:	xen/include/xen/device_tree.h
 F:	xen/drivers/passthrough/device_tree.c
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index e9a0661146..5cda0dc674 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 <asm/arm64/sve.h>
@@ -25,7 +26,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;
+
+    switch ( sysctl->cmd )
+    {
+    case XEN_SYSCTL_dt_overlay:
+        ret = dt_overlay_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..60bedf0852
--- /dev/null
+++ b/xen/common/dt-overlay.c
@@ -0,0 +1,402 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/common/dt-overlay.c
+ *
+ * Device tree overlay support in Xen.
+ *
+ * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
+ * Written by Vikram Garhwal <vikram.garhwal@amd.com>
+ *
+ */
+#include <asm/domain_build.h>
+#include <xen/dt-overlay.h>
+#include <xen/guest_access.h>
+#include <xen/iocap.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/xmalloc.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(const 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 *last_descendant = device_node->child;
+
+    parent_node = device_node->parent;
+
+    /* Check if we are trying to remove "/" i.e. root node. */
+    if ( parent_node == NULL )
+    {
+        dt_dprintk("%s's parent node not found\n", device_node->name);
+        return -EFAULT;
+    }
+
+    /* Sanity check for linking between parent and child node. */
+    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 a node, we need to remove all it's descendants too.
+         * Reason behind finding last_descendant:
+         * If device_node has multiple children, device_node->allnext will point
+         * to first_child and first_child->allnext will be a sibling. When the
+         * device_node and it's all children are removed, parent_node->allnext
+         * should point to node next to last children.
+         */
+        if ( last_descendant )
+        {
+            last_descendant = find_last_descendants_node(device_node);
+            parent_node->allnext = 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 ( last_descendant )
+                last_descendant = find_last_descendants_node(device_node);
+
+            if ( last_descendant )
+                np->allnext = 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;
+}
+
+static int irq_remove_cb(unsigned long s, unsigned long e, void *dom,
+                         unsigned long *c)
+{
+    int rc;
+    struct domain *d = dom;
+
+    /*
+     * TODO: We don't handle shared IRQs for now. So, it is assumed that
+     * the IRQs was not shared with another devices.
+     * TODO: Undo the IRQ routing.
+     */
+    rc = irq_deny_access(d, s);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "unable to revoke access for irq %lu\n", s);
+    }
+    else
+        *c += e - s + 1;
+
+    return rc;
+
+}
+
+static int iomem_remove_cb(unsigned long s, unsigned long e, void *dom,
+                           unsigned long *c)
+{
+    int rc;
+    struct domain *d = dom;
+
+    /*
+    * Remove mmio access.
+    * TODO: Support for remove/add the mapping in P2M.
+    */
+    rc = iomem_deny_access(d, s, e);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Unable to remove dom%d access to"
+               " 0x%"PRIx64" - 0x%"PRIx64"\n",
+               d->domain_id,
+               s & PAGE_MASK, PAGE_ALIGN(e) - 1);
+    }
+    else
+        *c += e - s + 1;
+
+    return rc;
+}
+
+static int remove_resources(struct dt_device_node *device_node)
+{
+    int rc = 0;
+    domid_t domid;
+    unsigned int len;
+
+    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 if only it's assigned to hardware domain or domain io. */
+    if ( domid != hardware_domain->domain_id && domid != DOMID_IO )
+    {
+        printk(XENLOG_ERR "Device %s is being used by domain %u. Removing nodes failed\n",
+               device_node->full_name, domid);
+        return -EINVAL;
+    }
+
+    dt_dprintk("Removing node: %s\n", device_node->full_name);
+
+    /* Check if iommu property exists. */
+    if ( dt_get_property(device_node, "iommus", &len) )
+    {
+        rc = iommu_remove_dt_device(device_node);
+        if ( rc )
+            return rc;
+    }
+
+    return rc;
+}
+
+/* Removes all descendants of the given node. */
+static int remove_all_descendant_nodes(const 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 )
+        {
+            rc = remove_all_descendant_nodes(child_node);
+            if ( rc )
+                return rc;
+        }
+
+        rc = remove_resources(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;
+    struct domain *d = hardware_domain;
+
+    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);
+        if ( rc )
+            return rc;
+
+        /* All children nodes are unmapped. Now remove the node itself. */
+        rc = remove_resources(overlay_node);
+        if ( rc )
+            return rc;
+
+        write_lock(&dt_host_lock);
+
+        rc = dt_overlay_remove_node(overlay_node);
+        if ( rc )
+        {
+            write_unlock(&dt_host_lock);
+            return rc;
+        }
+
+        write_unlock(&dt_host_lock);
+    }
+
+    /* Remove IRQ access. */
+    if ( tracker->irq_ranges )
+    {
+        rc = rangeset_consume_ranges(tracker->irq_ranges, irq_remove_cb, d);
+        if ( rc )
+            return rc;
+    }
+
+   /* Remove mmio access. */
+    if ( tracker->iomem_ranges )
+    {
+        rc = rangeset_consume_ranges(tracker->iomem_ranges, iomem_remove_cb, d);
+        if ( rc )
+            return rc;
+    }
+
+    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(const void *overlay_fdt,
+                                        uint32_t overlay_fdt_size)
+{
+    int rc;
+    struct overlay_track *entry, *temp, *track;
+    bool found_entry = false;
+
+    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+    if ( rc )
+        return rc;
+
+    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 )
+    {
+        rc = -EINVAL;
+
+        printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
+               " Removing nodes is supported only for prior added dtbo.\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);
+
+    rangeset_destroy(entry->irq_ranges);
+    rangeset_destroy(entry->iomem_ranges);
+
+    xfree(entry);
+
+ out:
+    spin_unlock(&overlay_lock);
+    return rc;
+}
+
+long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
+{
+    long ret;
+    void *overlay_fdt;
+
+    if ( op->overlay_op != XEN_SYSCTL_DT_OVERLAY_ADD &&
+         op->overlay_op != XEN_SYSCTL_DT_OVERLAY_REMOVE )
+        return -EOPNOTSUPP;
+
+    if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
+        return -EINVAL;
+
+    if ( op->pad[0] || op->pad[1] || op->pad[2] )
+        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;
+    }
+
+    if ( op->overlay_op == XEN_SYSCTL_DT_OVERLAY_REMOVE )
+        ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+
+    xfree(overlay_fdt);
+
+    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 fa7147de47..900239133a 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1059,6 +1059,25 @@ 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(const_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. */
+    uint8_t pad[3];                         /* IN: Must be zero. */
+};
+#endif
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -1089,6 +1108,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;
@@ -1119,6 +1139,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..c0567741ee
--- /dev/null
+++ b/xen/include/xen/dt-overlay.h
@@ -0,0 +1,63 @@
+ /* SPDX-License-Identifier: GPL-2.0-only */
+ /*
+ * 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/device_tree.h>
+#include <xen/list.h>
+#include <xen/rangeset.h>
+
+/*
+ * overlay_track describes information about added nodes through dtbo.
+ * @entry: List pointer.
+ * @dt_host_new: Pointer to the updated dt_host_new which is unflattened from
+    the 'updated fdt'.
+ * @fdt: Stores the fdt.
+ * @overlay_fdt: Stores a copy of input overlay_fdt.
+ * @nodes_address: Stores each overlay_node's address.
+ * @num_nodes: Total number of nodes in overlay dtb.
+ * @iomem_ranges: Range set to keep track of all IOMEMs.
+ * @irq_ranges: Range set to keep track of all added IRQs.
+ */
+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 rangeset *iomem_ranges;
+    struct rangeset *irq_ranges;
+};
+
+struct xen_sysctl_dt_overlay;
+
+#ifdef CONFIG_OVERLAY_DTB
+long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
+#else
+#include <xen/errno.h>
+static inline long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
+{
+    return -EOPNOTSUPP;
+}
+#endif
+
+#endif /* __XEN_DT_OVERLAY_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.1

[XEN][PATCH v9 16/19] xen/arm: Implement device tree node addition functionalities

[Vikram Garhwal]

Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition
using device tree overlay.

xl dt-overlay add file.dtbo:
    Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
    device_tree_flattened) is created and updated with overlay nodes. This
    updated fdt is further unflattened to a dt_host_new. Next, it checks if any
    of the overlay nodes already exists in the dt_host. If overlay nodes doesn't
    exist then find the overlay nodes in dt_host_new, find the overlay node's
    parent in dt_host and add the nodes as child under their parent in the
    dt_host. The node is attached as the last node under target parent.

    Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the
    overlay node.

When a node is added using overlay, a new entry is allocated in the
overlay_track to keep the track of memory allocation due to addition of overlay
node. This is helpful for freeing the memory allocated when a device tree node
is removed.

The main purpose of this 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/setting 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".

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

---
Changes from v8:
    Add rangeset to keep IRQs and IOMEM information.
Changes from v7:
    Move overlay_node_count() in this patch.
    Fix indent with goto statements.
    Rename handle_add_irq_iommu() to add_resources().
Changes from v6:
    Fix comment style and add comment regarding false flag in irq mapping.
    Move malloc for nodes_full_path to handle_add_overlay_nodes.
    Move node_num define to start of overlay_get_nodes_info().
    Remove "domain *d" from handle_add_irq_iommu().
    Fix error handling for handle_add_irq_iommu().
    Split handle_add_overlay_nodes to two functions.
    Create a separate function for freeing nodes_full_path.
    Fix xfree for dt_sysctl.
---
---
 xen/common/dt-overlay.c | 599 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 599 insertions(+)

diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 60bedf0852..8b47e41491 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -34,6 +34,25 @@ find_last_descendants_node(const struct dt_device_node *device_node)
     return child_node;
 }
 
+/*
+ * Returns next node to the input node. If node has children then return
+ * last descendant's next node.
+*/
+static struct dt_device_node *
+dt_find_next_node(struct dt_device_node *dt, const struct dt_device_node *node)
+{
+    struct dt_device_node *np;
+
+    dt_for_each_device_node(dt, np)
+        if ( np == node )
+            break;
+
+    if ( np->child )
+        np = find_last_descendants_node(np);
+
+    return np->allnext;
+}
+
 static int dt_overlay_remove_node(struct dt_device_node *device_node)
 {
     struct dt_device_node *np;
@@ -111,6 +130,78 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node)
     return 0;
 }
 
+static int dt_overlay_add_node(struct dt_device_node *device_node,
+                               const char *parent_node_path)
+{
+    struct dt_device_node *parent_node;
+    struct dt_device_node *next_node;
+
+    parent_node = dt_find_node_by_path(parent_node_path);
+
+    if ( parent_node == NULL )
+    {
+        dt_dprintk("Parent node %s not found. Overlay node will not be added\n",
+                   parent_node_path);
+        return -EINVAL;
+    }
+
+    /* If parent has no child. */
+    if ( parent_node->child == NULL )
+    {
+        next_node = parent_node->allnext;
+        device_node->parent = parent_node;
+        parent_node->allnext = device_node;
+        parent_node->child = device_node;
+    }
+    else
+    {
+        struct dt_device_node *np;
+        /*
+         * If parent has at least one child node.
+         * Iterate to the last child node of parent.
+         */
+        for ( np = parent_node->child; np->sibling != NULL; np = np->sibling );
+
+        /* Iterate over all child nodes of np node. */
+        if ( np->child )
+        {
+            struct dt_device_node *np_last_descendant;
+
+            np_last_descendant = find_last_descendants_node(np);
+
+            next_node = np_last_descendant->allnext;
+            np_last_descendant->allnext = device_node;
+        }
+        else
+        {
+            next_node = np->allnext;
+            np->allnext = device_node;
+        }
+
+        device_node->parent = parent_node;
+        np->sibling = device_node;
+        np->sibling->sibling = NULL;
+    }
+
+    /* Iterate over all child nodes of device_node to add children too. */
+    if ( device_node->child )
+    {
+        struct dt_device_node *device_node_last_descendant;
+
+        device_node_last_descendant = find_last_descendants_node(device_node);
+
+        /* Plug next_node at the end of last children of device_node. */
+        device_node_last_descendant->allnext = next_node;
+    }
+    else
+    {
+        /* Now plug next_node at the end of device_node. */
+        device_node->allnext = next_node;
+    }
+
+    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)
 {
@@ -171,6 +262,102 @@ static int iomem_remove_cb(unsigned long s, unsigned long e, void *dom,
     return rc;
 }
 
+/* Count number of nodes till one level of __overlay__ tag. */
+static unsigned int overlay_node_count(const void *overlay_fdt)
+{
+    unsigned int num_overlay_nodes = 0;
+    int fragment;
+
+    fdt_for_each_subnode(fragment, overlay_fdt, 0)
+    {
+        int subnode;
+        int overlay;
+
+        overlay = fdt_subnode_offset(overlay_fdt, 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, overlay_fdt, overlay)
+        {
+            num_overlay_nodes++;
+        }
+    }
+
+    return num_overlay_nodes;
+}
+
+/*
+ * overlay_get_nodes_info gets full name with path for all the nodes which
+ * are in one level of __overlay__ tag. This is useful when checking node for
+ * duplication i.e. dtbo tries to add nodes which already exists in device tree.
+ */
+static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
+{
+    int fragment;
+    unsigned int node_num = 0;
+
+    fdt_for_each_subnode(fragment, fdto, 0)
+    {
+        int target;
+        int overlay;
+        int subnode;
+        const char *target_path;
+
+        target = fdt_overlay_target_offset(device_tree_flattened, fdto,
+                                           fragment, &target_path);
+        if ( target < 0 )
+            return target;
+
+        if ( target_path == NULL )
+            return -EINVAL;
+
+        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)
+        {
+            const char *node_name = NULL;
+            int node_name_len;
+            unsigned int target_path_len = strlen(target_path);
+            unsigned int node_full_name_len;
+
+            node_name = fdt_get_name(fdto, subnode, &node_name_len);
+
+            if ( node_name == NULL )
+                return node_name_len;
+
+            /*
+             * Magic number 2 is for adding '/' and '\0'. This is done to keep
+             * the node_full_path in the correct full node name format.
+             */
+            node_full_name_len = target_path_len + node_name_len + 2;
+
+            nodes_full_path[node_num] = xmalloc_bytes(node_full_name_len);
+
+            if ( nodes_full_path[node_num] == NULL )
+                return -ENOMEM;
+
+            memcpy(nodes_full_path[node_num], target_path, target_path_len);
+
+            nodes_full_path[node_num][target_path_len] = '/';
+
+            memcpy(nodes_full_path[node_num] + target_path_len + 1,
+                    node_name, node_name_len);
+
+            nodes_full_path[node_num][node_full_name_len - 1] = '\0';
+
+            node_num++;
+        }
+    }
+
+    return 0;
+}
+
 static int remove_resources(struct dt_device_node *device_node)
 {
     int rc = 0;
@@ -355,6 +542,416 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
     return rc;
 }
 
+/*
+ * Handles IRQ and IOMMU mapping for the overlay_node and all descendants of the
+ * overlay_node.
+ */
+static int add_resources(struct overlay_track *tr,
+                         struct dt_device_node *overlay_node)
+{
+    int rc;
+    unsigned int naddr, nirq, irq, i, len;
+    struct dt_device_node *np;
+
+    /*
+     * First let's handle the interrupts.
+     * For now, need_mapping is set to false which means it will only permit IRQ
+     * access to hardware_domain using irq_permit_access() but will never route
+     * as route_irq_to_guest() will not be called with false flag.
+     */
+    rc = handle_device_interrupts(hardware_domain, overlay_node, false);
+    if ( rc < 0 )
+    {
+        printk(XENLOG_ERR "Failed to retrieve interrupts configuration\n");
+        return rc;
+    }
+
+    nirq = dt_number_of_irq(overlay_node);
+
+    for ( i = 0; i < nirq; i++ )
+    {
+        irq = platform_get_irq(overlay_node, i);
+        if ( irq < 0 )
+        {
+            printk(XENLOG_ERR "Unable to get irq %u for %s\n",
+                   i, dt_node_full_name(overlay_node));
+            return irq;
+        }
+
+        rc = rangeset_add_singleton(tr->irq_ranges, irq);
+        if ( rc )
+            return rc;
+    }
+
+    /* Check if iommu property exists. */
+    if ( dt_get_property(overlay_node, "iommus", &len) )
+    {
+        /* Add device to IOMMUs. */
+        rc = iommu_add_dt_device(overlay_node);
+        if ( rc < 0 )
+        {
+            printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
+                   dt_node_full_name(overlay_node));
+            return rc;
+        }
+    }
+
+    /* Set permissions. */
+    naddr = dt_number_of_address(overlay_node);
+
+    dt_dprintk("%s naddr = %u\n", dt_node_full_name(overlay_node), naddr);
+
+    /* Give permission to map MMIOs */
+    for ( i = 0; i < naddr; i++ )
+    {
+        uint64_t addr, size;
+
+        /*
+         * For now, we skip_mapping which means it will only permit iomem access
+         * to hardware_domain using iomem_permit_access() but will never map as
+         * map_range_p2mt() will not be called.
+         */
+        struct map_range_data mr_data = { .d = hardware_domain,
+                                          .p2mt = p2m_mmio_direct_c,
+                                          .skip_mapping = true
+                                        };
+
+        rc = dt_device_get_address(overlay_node, i, &addr, &size);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+                   i, dt_node_full_name(overlay_node));
+            return rc;
+        }
+
+        rc = map_range_to_domain(overlay_node, addr, size, &mr_data);
+        if ( rc )
+            return rc;
+
+        rc = rangeset_add_range(tr->iomem_ranges, paddr_to_pfn(addr),
+                                paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+        if ( rc )
+            return rc;
+    }
+
+    /* Map IRQ and IOMMU for overlay_node's children. */
+    for ( np = overlay_node->child; np != NULL; np = np->sibling )
+    {
+        rc = add_resources(tr, np);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
+            return rc;
+        }
+    }
+
+    return rc;
+}
+
+static void free_nodes_full_path(int num_nodes, char **nodes_full_path)
+{
+    int i;
+
+    if ( nodes_full_path != NULL )
+    {
+        for ( i = 0; i < num_nodes && nodes_full_path[i] != NULL;
+              i++ )
+        {
+            xfree(nodes_full_path[i]);
+        }
+        xfree(nodes_full_path);
+    }
+
+    return;
+}
+
+static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
+
+{
+    int j, rc;
+    struct dt_device_node *overlay_node;
+
+    for ( j = 0; j < tr->num_nodes; j++ )
+    {
+        struct dt_device_node *prev_node, *next_node;
+
+        dt_dprintk("Adding node: %s\n", nodes_full_path[j]);
+
+        /* Find the newly added node in tr->dt_host_new by it's full path. */
+        overlay_node = dt_find_node_by_path_from(tr->dt_host_new,
+                                                 nodes_full_path[j]);
+        if ( overlay_node == NULL )
+        {
+            /* Sanity check. But code will never come here. */
+            ASSERT_UNREACHABLE();
+            return -EFAULT;
+        }
+
+        /*
+         * Find previous and next node to overlay_node in dt_host_new. We will
+         * need these nodes to fix the dt_host_new mapping. When overlay_node is
+         * take out of dt_host_new tree and added to dt_host, link between
+         * previous node and next_node is broken. We will need to refresh
+         * dt_host_new with correct linking for any other overlay nodes
+         * extraction in future.
+         */
+        dt_for_each_device_node(tr->dt_host_new, prev_node)
+            if ( prev_node->allnext == overlay_node )
+                break;
+
+        next_node = dt_find_next_node(tr->dt_host_new, overlay_node);
+
+        write_lock(&dt_host_lock);
+
+        /* Add the node to dt_host. */
+        rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name);
+        if ( rc )
+        {
+            write_unlock(&dt_host_lock);
+
+            /* Node not added in dt_host. */
+            return rc;
+        }
+
+        write_unlock(&dt_host_lock);
+
+        prev_node->allnext = next_node;
+
+        overlay_node = dt_find_node_by_path(overlay_node->full_name);
+        if ( overlay_node == NULL )
+        {
+            /* Sanity check. But code will never come here. */
+            ASSERT_UNREACHABLE();
+            return -EFAULT;
+        }
+
+        rc = add_resources(tr, overlay_node);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
+            return rc;
+        }
+
+        /* Keep overlay_node address in tracker. */
+        tr->nodes_address[j] = (unsigned long)overlay_node;
+    }
+
+    return 0;
+}
+/*
+ * Adds device tree nodes under target node.
+ * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This
+ * is done to avoid the removal of device_tree generation, iomem regions mapping
+ * to hardware domain done by handle_node().
+ */
+static long handle_add_overlay_nodes(void *overlay_fdt,
+                                     uint32_t overlay_fdt_size)
+{
+    int rc, j;
+    struct dt_device_node *overlay_node;
+    struct overlay_track *tr = NULL;
+    char **nodes_full_path = NULL;
+    unsigned int new_fdt_size;
+
+    tr = xzalloc(struct overlay_track);
+    if ( tr == NULL )
+        return -ENOMEM;
+
+    new_fdt_size = fdt_totalsize(device_tree_flattened) +
+                                 fdt_totalsize(overlay_fdt);
+
+    tr->fdt = xzalloc_bytes(new_fdt_size);
+    if ( tr->fdt == NULL )
+    {
+        xfree(tr);
+        return -ENOMEM;
+    }
+
+    tr->num_nodes = overlay_node_count(overlay_fdt);
+    if ( tr->num_nodes == 0 )
+    {
+        xfree(tr->fdt);
+        xfree(tr);
+        return -ENOMEM;
+    }
+
+    tr->nodes_address = xzalloc_bytes(tr->num_nodes * sizeof(unsigned long));
+    if ( tr->nodes_address == NULL )
+    {
+        xfree(tr->fdt);
+        xfree(tr);
+        return -ENOMEM;
+    }
+
+    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+    if ( rc )
+    {
+        xfree(tr->nodes_address);
+        xfree(tr->fdt);
+        xfree(tr);
+        return rc;
+    }
+
+    /*
+     * Keep a copy of overlay_fdt as fdt_overlay_apply will change the input
+     * overlay's content(magic) when applying overlay.
+     */
+    tr->overlay_fdt = xzalloc_bytes(overlay_fdt_size);
+    if ( tr->overlay_fdt == NULL )
+    {
+        xfree(tr->nodes_address);
+        xfree(tr->fdt);
+        xfree(tr);
+        return -ENOMEM;
+    }
+
+    memcpy(tr->overlay_fdt, overlay_fdt, overlay_fdt_size);
+
+    spin_lock(&overlay_lock);
+
+    memcpy(tr->fdt, device_tree_flattened,
+           fdt_totalsize(device_tree_flattened));
+
+    /* Open tr->fdt with more space to accommodate the overlay_fdt. */
+    rc = fdt_open_into(tr->fdt, tr->fdt, new_fdt_size);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Increasing fdt size to accommodate overlay_fdt failed with error %d\n",
+               rc);
+        goto err;
+    }
+
+    nodes_full_path = xzalloc_bytes(tr->num_nodes * sizeof(char *));
+    if ( nodes_full_path == NULL )
+    {
+        rc = -ENOMEM;
+        goto err;
+    }
+
+    /*
+     * overlay_get_nodes_info is called to get the node information from dtbo.
+     * This is done before fdt_overlay_apply() because the overlay apply will
+     * erase the magic of overlay_fdt.
+     */
+    rc = overlay_get_nodes_info(overlay_fdt, nodes_full_path);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Getting nodes information failed with error %d\n",
+               rc);
+        goto err;
+    }
+
+    rc = fdt_overlay_apply(tr->fdt, overlay_fdt);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc);
+        goto err;
+    }
+
+    /*
+     * Check if any of the node already exists in dt_host. If node already exits
+     * we can return here as this overlay_fdt is not suitable for overlay ops.
+     */
+    for ( j = 0; j < tr->num_nodes; j++ )
+    {
+        overlay_node = dt_find_node_by_path(nodes_full_path[j]);
+        if ( overlay_node != NULL )
+        {
+            printk(XENLOG_ERR "node %s exists in device tree\n",
+                   nodes_full_path[j]);
+            rc = -EINVAL;
+            goto err;
+        }
+    }
+
+    /*
+    * Unflatten the tr->fdt into a new dt_host.
+    * TODO: Check and add alias_scan() if it's needed for overlay in future.
+    */
+    rc = unflatten_device_tree(tr->fdt, &tr->dt_host_new);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "unflatten_device_tree failed with error %d\n", rc);
+        goto err;
+    }
+
+    tr->irq_ranges = rangeset_new(hardware_domain, "Overlays: Interrupts", 0);
+    if (tr->irq_ranges == NULL)
+    {
+        printk(XENLOG_ERR "Creating IRQ rangeset failed");
+        goto err;
+    }
+
+    tr->iomem_ranges = rangeset_new(hardware_domain, "Overlay: I/O Memory", 0);
+    if (tr->iomem_ranges == NULL)
+    {
+        printk(XENLOG_ERR "Creating IOMMU rangeset failed");
+        goto err;
+    }
+
+    rc = add_nodes(tr, nodes_full_path);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Adding nodes failed. Removing the partially added nodes.\n");
+        goto remove_node;
+    }
+
+    INIT_LIST_HEAD(&tr->entry);
+    list_add_tail(&tr->entry, &overlay_tracker);
+
+    spin_unlock(&overlay_lock);
+
+    free_nodes_full_path(tr->num_nodes, nodes_full_path);
+
+    return rc;
+
+/*
+ * Failure case. We need to remove the nodes, free tracker(if tr exists) and
+ * tr->dt_host_new.
+ */
+ remove_node:
+    tr->num_nodes = j;
+    rc = remove_nodes(tr);
+
+    if ( rc )
+    {
+        /*
+         * User needs to provide right overlay. Incorrect node information
+         * example parent node doesn't exist in dt_host etc can cause memory
+         * leaks as removing_nodes() will fail and this means nodes memory is
+         * not freed from tracker. Which may cause memory leaks. Ideally, these
+         * device tree related mistakes will be caught by fdt_overlay_apply()
+         * but given that we don't manage that code keeping this warning message
+         * is better here.
+         */
+        printk(XENLOG_ERR "Removing node failed.\n");
+        spin_unlock(&overlay_lock);
+
+        free_nodes_full_path(tr->num_nodes, nodes_full_path);
+
+        return rc;
+    }
+
+ err:
+    spin_unlock(&overlay_lock);
+
+    if ( tr->dt_host_new )
+        xfree(tr->dt_host_new);
+
+    xfree(tr->overlay_fdt);
+    xfree(tr->nodes_address);
+    xfree(tr->fdt);
+
+    free_nodes_full_path(tr->num_nodes, nodes_full_path);
+
+    rangeset_destroy(tr->irq_ranges);
+    rangeset_destroy(tr->iomem_ranges);
+
+    xfree(tr);
+
+    return rc;
+}
+
 long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
 {
     long ret;
@@ -386,6 +983,8 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
 
     if ( op->overlay_op == XEN_SYSCTL_DT_OVERLAY_REMOVE )
         ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+    else
+        ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
 
     xfree(overlay_fdt);
 
-- 
2.17.1

[XEN][PATCH v9 17/19] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Vikram Garhwal]

xc_dt_overlay() sends the device tree binary overlay, size of .dtbo and overlay
operation type i.e. add or remove to xen.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 tools/include/xenctrl.h         |  5 ++++
 tools/libs/ctrl/Makefile.common |  1 +
 tools/libs/ctrl/xc_dt_overlay.c | 51 +++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 tools/libs/ctrl/xc_dt_overlay.c

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index faec1dd824..3da602586c 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2643,6 +2643,11 @@ int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint32
 int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
                          xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
 
+#if defined(__arm__) || defined(__aarch64__)
+int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
+                  uint32_t overlay_fdt_size, uint8_t overlay_op);
+#endif
+
 /* Compat shims */
 #include "xenctrl_compat.h"
 
diff --git a/tools/libs/ctrl/Makefile.common b/tools/libs/ctrl/Makefile.common
index 0a09c28fd3..247afbe5f9 100644
--- a/tools/libs/ctrl/Makefile.common
+++ b/tools/libs/ctrl/Makefile.common
@@ -24,6 +24,7 @@ OBJS-y       += xc_hcall_buf.o
 OBJS-y       += xc_foreign_memory.o
 OBJS-y       += xc_kexec.o
 OBJS-y       += xc_resource.o
+OBJS-$(CONFIG_ARM)  += xc_dt_overlay.o
 OBJS-$(CONFIG_X86) += xc_psr.o
 OBJS-$(CONFIG_X86) += xc_pagetab.o
 OBJS-$(CONFIG_Linux) += xc_linux.o
diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
new file mode 100644
index 0000000000..58283b9ef6
--- /dev/null
+++ b/tools/libs/ctrl/xc_dt_overlay.c
@@ -0,0 +1,51 @@
+/*
+ *
+ * Device Tree Overlay functions.
+ * Copyright (C) 2021 Xilinx Inc.
+ * Author Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "xc_private.h"
+
+int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
+                  uint32_t overlay_fdt_size, uint8_t overlay_op)
+{
+    int err;
+    DECLARE_SYSCTL;
+
+    DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+    if ( (err = xc_hypercall_bounce_pre(xch, overlay_fdt)) )
+        goto err;
+
+    sysctl.cmd = XEN_SYSCTL_dt_overlay;
+    sysctl.u.dt_overlay.overlay_op = overlay_op;
+    sysctl.u.dt_overlay.overlay_fdt_size = overlay_fdt_size;
+    sysctl.u.dt_overlay.pad[0]= 0;
+    sysctl.u.dt_overlay.pad[1]= 0;
+    sysctl.u.dt_overlay.pad[2]= 0;
+
+    set_xen_guest_handle(sysctl.u.dt_overlay.overlay_fdt, overlay_fdt);
+
+    if ( (err = do_sysctl(xch, &sysctl)) != 0 )
+        PERROR("%s failed", __func__);
+
+err:
+    xc_hypercall_bounce_post(xch, overlay_fdt);
+
+    return err;
+}
-- 
2.17.1

[XEN][PATCH v9 18/19] tools/libs/light: Implement new libxl functions for device tree overlay ops

[Vikram Garhwal]

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/include/libxl.h               | 11 +++++
 tools/libs/light/Makefile           |  3 ++
 tools/libs/light/libxl_dt_overlay.c | 71 +++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100644 tools/libs/light/libxl_dt_overlay.c

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index aa5b1c98d4..b4f2a69b34 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -259,6 +259,12 @@
  */
 #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
 
+#if defined(__arm__) || defined(__aarch64__)
+/**
+ * This means Device Tree Overlay is supported.
+ */
+#define LIBXL_HAVE_DT_OVERLAY 1
+#endif
 /*
  * libxl_domain_build_info has device_model_user to specify the user to
  * run the device model with. See docs/misc/qemu-deprivilege.txt.
@@ -2493,6 +2499,11 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
                                         int *num);
 void libxl_device_pci_list_free(libxl_device_pci* list, int num);
 
+#if defined(__arm__) || defined(__aarch64__)
+int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
+                     uint32_t overlay_size, uint8_t overlay_op);
+#endif
+
 /*
  * Turns the current process into a backend device service daemon
  * for a driver domain.
diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index 5d7ff94b05..ba4c1b7933 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -112,6 +112,9 @@ OBJS-y += _libxl_types.o
 OBJS-y += libxl_flask.o
 OBJS-y += _libxl_types_internal.o
 
+# Device tree overlay is enabled only for ARM architecture.
+OBJS-$(CONFIG_ARM) += libxl_dt_overlay.o
+
 ifeq ($(CONFIG_LIBNL),y)
 CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
 endif
diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
new file mode 100644
index 0000000000..a6c709a6dc
--- /dev/null
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2021 Xilinx Inc.
+ * Author Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+#include <libfdt.h>
+#include <xenctrl.h>
+
+static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
+{
+    int r;
+
+    if (fdt_magic(fdt) != FDT_MAGIC) {
+        LOG(ERROR, "Overlay FDT is not a valid Flat Device Tree");
+        return ERROR_FAIL;
+    }
+
+    r = fdt_check_header(fdt);
+    if (r) {
+        LOG(ERROR, "Failed to check the overlay FDT (%d)", r);
+        return ERROR_FAIL;
+    }
+
+    if (fdt_totalsize(fdt) > size) {
+        LOG(ERROR, "Overlay FDT totalsize is too big");
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
+                     uint8_t overlay_op)
+{
+    int rc;
+    int r;
+    GC_INIT(ctx);
+
+    if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
+        LOG(ERROR, "Overlay DTB check failed");
+        rc = ERROR_FAIL;
+        goto out;
+    } else {
+        LOG(DEBUG, "Overlay DTB check passed");
+        rc = 0;
+    }
+
+    r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
+
+    if (r) {
+        LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
+        rc = ERROR_FAIL;
+    }
+
+out:
+    GC_FREE;
+    return rc;
+}
+
-- 
2.17.1

[XEN][PATCH v9 19/19] tools/xl: Add new xl command overlay for device tree overlay support

[Vikram Garhwal]

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/xl/xl.h           |  1 +
 tools/xl/xl_cmdtable.c  |  6 +++++
 tools/xl/xl_vmcontrol.c | 52 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 72538d6a81..a923daccd3 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -138,6 +138,7 @@ int main_shutdown(int argc, char **argv);
 int main_reboot(int argc, char **argv);
 int main_list(int argc, char **argv);
 int main_vm_list(int argc, char **argv);
+int main_dt_overlay(int argc, char **argv);
 int main_create(int argc, char **argv);
 int main_config_update(int argc, char **argv);
 int main_button_press(int argc, char **argv);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 67604e9536..2463521156 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -631,6 +631,12 @@ const struct cmd_spec cmd_table[] = {
       "Issue a qemu monitor command to the device model of a domain",
       "<Domain> <Command>",
     },
+    { "dt-overlay",
+      &main_dt_overlay, 0, 1,
+      "Add/Remove a device tree overlay",
+      "add/remove <.dtbo>"
+      "-h print this help\n"
+    },
 };
 
 const int cmdtable_len = ARRAY_SIZE(cmd_table);
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 03971927e9..cea5b4a88e 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv)
     return 0;
 }
 
+int main_dt_overlay(int argc, char **argv)
+{
+    const char *overlay_ops = NULL;
+    const char *overlay_config_file = NULL;
+    void *overlay_dtb = NULL;
+    int rc;
+    uint8_t op;
+    int overlay_dtb_size = 0;
+    const int overlay_add_op = 1;
+    const int overlay_remove_op = 2;
+
+    if (argc < 2) {
+        help("dt_overlay");
+        return EXIT_FAILURE;
+    }
+
+    overlay_ops = argv[1];
+    overlay_config_file = argv[2];
+
+    if (strcmp(overlay_ops, "add") == 0)
+        op = overlay_add_op;
+    else if (strcmp(overlay_ops, "remove") == 0)
+        op = overlay_remove_op;
+    else {
+        fprintf(stderr, "Invalid dt overlay operation\n");
+        return EXIT_FAILURE;
+    }
+
+    if (overlay_config_file) {
+        rc = libxl_read_file_contents(ctx, overlay_config_file,
+                                      &overlay_dtb, &overlay_dtb_size);
+
+        if (rc) {
+            fprintf(stderr, "failed to read the overlay device tree file %s\n",
+                    overlay_config_file);
+            free(overlay_dtb);
+            return ERROR_FAIL;
+        }
+    } else {
+        fprintf(stderr, "overlay dtbo file not provided\n");
+        return ERROR_FAIL;
+    }
+
+    rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+
+    free(overlay_dtb);
+
+    if (rc)
+        return EXIT_FAILURE;
+
+    return rc;
+}
 /*
  * Local variables:
  * mode: C
-- 
2.17.1

Re: [XEN][PATCH v9 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Jan Beulich]

On 19.08.2023 02:28, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> Remove static type so this can also be used by SMMU drivers to check if the
> device is being used before removing.
> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> patches in this series introduces node add/remove feature.
> 
> Also, caller is required hold the correct lock so moved the function prototype
> to a private header.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> ---
> Changes from v7:
>     Update commit message.
>     Add ASSERT().
> ---
> ---
>  xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
>  xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
>  create mode 100644 xen/include/xen/iommu-private.h

I find it odd for new versions to be sent when discussion on the prior
version hasn't settled yet, and when you then also don't incorporate
the feedback given. I also find it odd to now be Cc-ed on the entire
series. Imo instead of that patches which aren't self-explanatory /
self-consistent simply need their descriptions improved (in the case
here to mention what further use of a function is intended). Yet
better would be to add declarations (and remove static) only at the
point that's actually needed. This then eliminates the risk of
subsequent changes in response to feedback (like Julien's here)
resulting in the declaration no longer being needed, thus leading to
a Misra violation (besides the general tidiness aspect).

Jan

Re: [XEN][PATCH v9 17/19] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Anthony PERARD]

On Fri, Aug 18, 2023 at 05:28:48PM -0700, Vikram Garhwal wrote:
> xc_dt_overlay() sends the device tree binary overlay, size of .dtbo and overlay
> operation type i.e. add or remove to xen.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

Hi Vikram,

I've given some comments on the v7 of this patch at [1], there's been no reply
and the patch hasn't changed.

[1] https://lore.kernel.org/xen-devel/9975f41c-c149-445a-8122-c15cfe5511b0@perard/

Did this fell through the cracks?

Cheers,

-- 
Anthony PERARD

Re: [XEN][PATCH v9 17/19] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Vikram Garhwal]

Hi Anthony,
On Mon, Aug 21, 2023 at 05:18:27PM +0100, Anthony PERARD wrote:
> On Fri, Aug 18, 2023 at 05:28:48PM -0700, Vikram Garhwal wrote:
> > xc_dt_overlay() sends the device tree binary overlay, size of .dtbo and overlay
> > operation type i.e. add or remove to xen.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> Hi Vikram,
> 
> I've given some comments on the v7 of this patch at [1], there's been no reply
> and the patch hasn't changed.
> 
> [1] https://lore.kernel.org/xen-devel/9975f41c-c149-445a-8122-c15cfe5511b0@perard/
> 
> Did this fell through the cracks?
> 
Yes, you are right. It skipped through my update list. Sorry, for missing this.
Will change it in next version.

Thank you!
Vikram
> Cheers,
> 
> -- 
> Anthony PERARD

Re: [XEN][PATCH v9 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Vikram Garhwal]

Hi Jen,
On Mon, Aug 21, 2023 at 08:53:38AM +0200, Jan Beulich wrote:
> On 19.08.2023 02:28, Vikram Garhwal wrote:
> > Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> > Remove static type so this can also be used by SMMU drivers to check if the
> > device is being used before removing.
> > 
> > Moving spin_lock to caller was done to prevent the concurrent access to
> > iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> > patches in this series introduces node add/remove feature.
> > 
> > Also, caller is required hold the correct lock so moved the function prototype
> > to a private header.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > ---
> > Changes from v7:
> >     Update commit message.
> >     Add ASSERT().
> > ---
> > ---
> >  xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
> >  xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 4 deletions(-)
> >  create mode 100644 xen/include/xen/iommu-private.h
> 
> I find it odd for new versions to be sent when discussion on the prior
> version hasn't settled yet, and when you then also don't incorporate
> the feedback given. I also find it odd to now be Cc-ed on the entire
> series. Imo instead of that patches which aren't self-explanatory /
> self-consistent simply need their descriptions improved (in the case
> here to mention what further use of a function is intended). Yet
> better would be to add declarations (and remove static) only at the
> point that's actually needed. This then eliminates the risk of
> subsequent changes in response to feedback (like Julien's here)
> resulting in the declaration no longer being needed, thus leading to
> a Misra violation (besides the general tidiness aspect).
> 
Reason behind sending new version: Patch 15/19 is largest patch in terms of change
and there was a long discussion with Julien and Stefano regarding a big change.
Last week(after v8) we agreed on change and Stefano and Julien were okay to send
v9 so it will be easier to review with that change.

Regarding cc-ing you to whole series, there was following comment on v8 09/1
"I don't think private headers should live in include/xen/. Judging from only
the patches I was Cc-ed on." I thought you wanted to see the whole full series.
Looks like i misunderstood the comment and Will remove the from cc-list in next
version.

The function itself will be needed in further patches in this series.
I am okay with keeping static and other things same here to avoid MISRA violation
and change wherever they are called first time.

Regarding Julien's comment i am reply back in same thread on why this was
not taken in account.
> Jan

Re: [XEN][PATCH v9 02/19] common/device_tree.c: unflatten_device_tree() propagate errors

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> This will be useful in dynamic node programming when new dt nodes are unflattend
> during runtime. Invalid device tree node related errors should be propagated
> back to the caller.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> ---
> Changes from v7:
>      Free allocated memory in case of errors when calling unflatten_dt_node.
> ---
> ---
>   xen/common/device_tree.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index c91d54c493..cd9a6a5c93 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -2108,6 +2108,9 @@ static int __init __unflatten_device_tree(const void *fdt,
>       /* First pass, scan for size */
>       start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
>       size = unflatten_dt_node(fdt, 0, &start, NULL, NULL, 0);
> +    if ( !size )
> +        return -EINVAL;
> +
>       size = (size | 3) + 1;
>   
>       dt_dprintk("  size is %#lx allocating...\n", size);
> @@ -2125,11 +2128,21 @@ static int __init __unflatten_device_tree(const void *fdt,
>       start = ((unsigned long)fdt) + fdt_off_dt_struct(fdt);
>       unflatten_dt_node(fdt, mem, &start, NULL, &allnextp, 0);
>       if ( be32_to_cpup((__be32 *)start) != FDT_END )
> -        printk(XENLOG_WARNING "Weird tag at end of tree: %08x\n",
> +    {
> +        printk(XENLOG_ERR "Weird tag at end of tree: %08x\n",
>                     *((u32 *)start));
> +        xfree((__be64 *)mem);

I know that 'mem' is an 'unsigned long' so you need to cast it to a 
pointer. But '__be64 *' seems a rather odd choice. Why not using 'void *'?

Better would be to convert 'mem' to a 'void *' as we allow pointer 
arithmetic in Xen. But that can be dealt separately.

> +        return -EINVAL;
> +    }
> +
>       if ( be32_to_cpu(((__be32 *)mem)[size / 4]) != 0xdeadbeefU )
> -        printk(XENLOG_WARNING "End of tree marker overwritten: %08x\n",
> +    {
> +        printk(XENLOG_ERR "End of tree marker overwritten: %08x\n",
>                     be32_to_cpu(((__be32 *)mem)[size / 4]));
> +        xfree((__be64 *)mem);

Same remark here.

> +        return -EINVAL;
> +    }
> +
>       *allnextp = NULL;
>   
>       dt_dprintk(" <- unflatten_device_tree()\n");

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 03/19] xen/arm/device: Remove __init from function type

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Remove __init from following function to access during runtime:
>      1. map_irq_to_domain()
>      2. handle_device_interrupts()
>      3. map_range_to_domain()
>      4. unflatten_dt_node()

We are at v9 now, so this is more a remark for the future. In general we 
are trying to avoid modifying the same code multiple time within the 
series because this makes it more difficult to review. In this case, you 
could have removed the __init in patch #4 where you also export it.

> 
> Move map_irq_to_domain() prototype from domain_build.h to setup.h.
> 
> To avoid breaking the build, following changes are also done:

I am guessing by "breaking the build", you mean that you will get an 
error because an non-init function is implemented in domain_build.c. 
Right? If so, I think this should be spelled out.

> 1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain()
>      to device.c. After removing __init type,  these functions are not specific
>      to domain building, so moving them out of domain_build.c to device.c.
> 2. Remove static type from handle_device_interrupt().

Typo: s/interrupt/interrupts/ to match the function name. But I don't 
link the name when it is exported as it leads to think this is dealing 
with real interrupt.

Looking at the overlay code, the caller (i.e. add_resources()) is very 
similar to handle_device(). AFAICT the only differences are:
    1/ add_resources() doesn't deal with mapping (you said it will in 
the future)
    2/ You need to update some rangeset

For 1/ it means this is a superset. For 2/, I think this could be 
abstracted by adding a pointer to the rangesets in map_range_data. They 
could be NULL for the domain build case.

So can you look at abstracting? This will make easier to maintain a 
single place to parse a device node and map it.

A possible name for the function would be dt_map_resources_to_domain().

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 04/19] common/device_tree: Export __unflatten_device_tree()

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Following changes are done to __unflatten_device_tree():
>      1. __unflatten_device_tree() is renamed to unflatten_device_tree().
>      2. Remove __init and static function type.
> 
> The changes are done to make this function useable for dynamic node programming
> where new device tree overlay nodes are added to fdt and further unflattend to
> update xen device tree during runtime.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

It is not clear to me whether you saw my reply about using ERR_PTR() as 
I can't find an answer.

Anyway, I am not against this interface:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 01/19] common/device_tree: handle memory allocation failure in __unflatten_device_tree()

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Change __unflatten_device_tree() return type to integer so it can propagate
> memory allocation failure. Add panic() in dt_unflatten_host_device_tree() for
> memory allocation failure during boot.
> 
> Fixes: fb97eb614acf ("xen/arm: Create a hierarchical device tree")
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> Acked-by: Julien Grall <jgrall@amazon.com>
> ---
>   xen/common/device_tree.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 0522fdf976..c91d54c493 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -2092,8 +2092,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>    * @fdt: The fdt to expand
>    * @mynodes: The device_node tree created by the call
>    */

While looking at patch #4, I noticed that the comment wasn't updated to 
mention the meaning of the return value. Can this be done?

If you propose a new comment on the ML, I can update it while committing 
patch. IOW no need to resend a new version for this patch.

> -static void __init __unflatten_device_tree(const void *fdt,
> -                                           struct dt_device_node **mynodes)
> +static int __init __unflatten_device_tree(const void *fdt,
> +                                          struct dt_device_node **mynodes)
>   {
>       unsigned long start, mem, size;
>       struct dt_device_node **allnextp = mynodes;
> @@ -2114,6 +2114,8 @@ static void __init __unflatten_device_tree(const void *fdt,
>   
>       /* Allocate memory for the expanded device tree */
>       mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node));
> +    if ( !mem )
> +        return -ENOMEM;
>   
>       ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeefU);
>   
> @@ -2131,6 +2133,8 @@ static void __init __unflatten_device_tree(const void *fdt,
>       *allnextp = NULL;
>   
>       dt_dprintk(" <- unflatten_device_tree()\n");
> +
> +    return 0;
>   }
>   
>   static void dt_alias_add(struct dt_alias_prop *ap,
> @@ -2215,7 +2219,11 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
>   
>   void __init dt_unflatten_host_device_tree(void)
>   {
> -    __unflatten_device_tree(device_tree_flattened, &dt_host);
> +    int error = __unflatten_device_tree(device_tree_flattened, &dt_host);
> +
> +    if ( error )
> +        panic("__unflatten_device_tree failed with error %d\n", error);
> +
>       dt_alias_scan();
>   }
>   

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 05/19] xen/arm: Add CONFIG_OVERLAY_DTB

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Introduce a config option where the user can enable support for adding/removing
> device tree nodes using a device tree binary overlay.
> 
> Update SUPPORT.md and CHANGELOG.md to state the Device Tree Overlays support for
> Arm.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Acked-by: Henry Wang <Henry.Wang@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> ---
> Changes from v7:
>      Add this feature as "experimental support" in CHANGELOG.md
> ---
> ---
>   CHANGELOG.md         | 3 ++-
>   SUPPORT.md           | 6 ++++++
>   xen/arch/arm/Kconfig | 5 +++++
>   3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 7d7e0590f8..47098dbfca 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -24,7 +24,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>    - xl/libxl can customize SMBIOS strings for HVM guests.
>    - Add support for AVX512-FP16 on x86.
>    - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
> -
> + - On Arm, experimental support for dynamic addition/removal of Xen device tree
> +   nodes using a device tree overlay binary(.dtbo).

Typo: missing space before (.

>   
>   ## [4.17.0](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.17.0) - 2022-12-12
>   
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 35a6249e03..8eb006565c 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -844,6 +844,12 @@ No support for QEMU backends in a 16K or 64K domain.
>   
>       Status: Supported
>   
> +### Device Tree Overlays
> +
> +Add/Remove device tree nodes using a device tree overlay binary(.dtbo).

Same here. I don't suggest to handle it on commit because this is not 
something I want to merge without the rest of the series.

> +
> +    Status, ARM: Experimental
> +
>   ### ARM: Guest ACPI support
>   
>       Status: Supported
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index fd57a82dd2..02c4796438 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -92,6 +92,11 @@ config HAS_ITS
>           bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
>           depends on GICV3 && !NEW_VGIC && !ARM_32
>   
> +config OVERLAY_DTB
> +	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> +	help
> +	  Dynamic addition/removal of Xen device tree nodes using a dtbo.

Do we have any documentation in the tree of the limitations and how this 
works?

The reason I am asking is the wording here suggests that it would be 
possible to remove nodes from the original Device-Tree. AFAIU this is 
not possible with the implementation and you are not planning to handle 
it. Correct?

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 08/19] xen/device-tree: Add device_tree_find_node_by_path() to find nodes in device tree

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Add device_tree_find_node_by_path() to find a matching node with path for a
> dt_device_node.
> 
> Reason behind this function:
>      Each time overlay nodes are added using .dtbo, a new fdt(memcpy of

Typo: missing space before (.

>      device_tree_flattened) is created and updated with overlay nodes. This
>      updated fdt is further unflattened to a dt_host_new. Next, we need to find
>      the overlay nodes in dt_host_new, find the overlay node's parent in dt_host
>      and add the nodes as child under their parent in the dt_host. Thus we need
>      this function to search for node in different unflattened device trees.
> 
> Also, make dt_find_node_by_path() static inline.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>

> ---
> Changes from v7:
>      Rename device_tree_find_node_by_path() to dt_find_node_by_path_from().
>      Fix indentation.
> ---
> ---
>   xen/common/device_tree.c      |  5 +++--
>   xen/include/xen/device_tree.h | 18 ++++++++++++++++--
>   2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 67e9b6de3e..0f10037745 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -358,11 +358,12 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
>       return np;
>   }
>   
> -struct dt_device_node *dt_find_node_by_path(const char *path)
> +struct dt_device_node *dt_find_node_by_path_from(struct dt_device_node *from,
> +                                                 const char *path)

Any change this both 'from' and the return can be const?

>   {
>       struct dt_device_node *np;
>   
> -    dt_for_each_device_node(dt_host, np)
> +    dt_for_each_device_node(from, np)
>           if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
>               break;
>   
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 5941599eff..e507658b23 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -568,13 +568,27 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
>   struct dt_device_node *dt_find_node_by_alias(const char *alias);
>   
>   /**
> - * dt_find_node_by_path - Find a node matching a full DT path
> + * dt_find_node_by_path_from - Generic function to find a node matching the
> + * full DT path for any given unflatten device tree
> + * @from: The device tree node to start searching from
>    * @path: The full path to match
>    *
>    * Returns a node pointer.
>    */
> -struct dt_device_node *dt_find_node_by_path(const char *path);
> +struct dt_device_node *
> +                    dt_find_node_by_path_from(struct dt_device_node *from,
> +                                                  const char *path);

Coding style: The indentation look rather odd. I am not sure it will 
render properly here. But one style that is closer to the rest of the 
file and Xen is:

struct dt_device_node *dt_find_node_by_path_from(struct dt_device_node 
*from,
                                                  const char *path);

Where the return type, function name and first parameter is one line and 
the second parameter is on the second line with the type aligned with 
the type of the first parameter.

>   
> +/**
> + * dt_find_node_by_path - Find a node matching a full DT path in dt_host
> + * @path: The full path to match
> + *
> + * Returns a node pointer.
> + */
> +static inline struct dt_device_node *dt_find_node_by_path(const char *path)
> +{
> +    return dt_find_node_by_path_from(dt_host, path);
> +}
>   
>   /**
>    * dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> Remove static type so this can also be used by SMMU drivers to check if the
> device is being used before removing.

I have commented on v8. But I will comment here to make easier to keep 
track of comment.

I don't think iommu_dt_device_is_assigned_locked() should be called from 
the SMMU. If you want to check a device is assigned then it would be 
best to use the internal state of the SMMU.

This has two benefits:
   * This avoids what I view as a layer of violation
   * This confirmed that the internal state match with what we expect

> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> patches in this series introduces node add/remove feature.
> 
> Also, caller is required hold the correct lock so moved the function prototype
> to a private header.

I don't understand how requiring the caller to hold the correct lock 
means you need to move the protype in a private header. Can you clarify?

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> ---
> Changes from v7:
>      Update commit message.
>      Add ASSERT().
> ---
> ---
>   xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
>   xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+), 4 deletions(-)
>   create mode 100644 xen/include/xen/iommu-private.h
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 1c32d7b50c..5796ee1f93 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -18,6 +18,7 @@
>   #include <xen/device_tree.h>
>   #include <xen/guest_access.h>
>   #include <xen/iommu.h>
> +#include <xen/iommu-private.h>
>   #include <xen/lib.h>
>   #include <xen/sched.h>
>   #include <xsm/xsm.h>
> @@ -83,16 +84,16 @@ fail:
>       return rc;
>   }
>   
> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
>   {
>       bool_t assigned = 0;
>   
> +    ASSERT(spin_is_locked(&dtdevs_lock));
> +
>       if ( !dt_device_is_protected(dev) )
>           return 0;
>   
> -    spin_lock(&dtdevs_lock);
>       assigned = !list_empty(&dev->domain_list);
> -    spin_unlock(&dtdevs_lock);
>   
>       return assigned;
>   }
> @@ -213,27 +214,44 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>           if ( (d && d->is_dying) || domctl->u.assign_device.flags )
>               break;
>   
> +        /*
> +         * To ensure that the dev doesn't disappear between the time we look it
> +         * up with dt_find_node_by_gpath() and we check the assignment later.
> +         */
> +        spin_lock(&dtdevs_lock);

This change doesn't look to be explained in the commit message. But 
looking at the code after this series is applied, you justified the 
addition of read_lock(&dt_host_lock) to protect access to the 
device-tree. This will be held longer than dtdevs_lock. So is it 
actually necessary to move the locking earlier?

> +
>           ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>                                       domctl->u.assign_device.u.dt.size,
>                                       &dev);
>           if ( ret )
> +        {
> +            spin_unlock(&dtdevs_lock);
>               break;
> +        }
>   
>           ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
>           if ( ret )
> +        {
> +            spin_unlock(&dtdevs_lock);
>               break;
> +        }
>   
>           if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
>           {
> -            if ( iommu_dt_device_is_assigned(dev) )
> +
> +            if ( iommu_dt_device_is_assigned_locked(dev) )
>               {
>                   printk(XENLOG_G_ERR "%s already assigned.\n",
>                          dt_node_full_name(dev));
>                   ret = -EINVAL;
>               }
> +
> +            spin_unlock(&dtdevs_lock);
>               break;
>           }
>   
> +        spin_unlock(&dtdevs_lock);
> +
>           if ( d == dom_io )
>               return -EINVAL;
>   
> diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h
> new file mode 100644
> index 0000000000..bb5c94e7a6
> --- /dev/null
> +++ b/xen/include/xen/iommu-private.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * xen/iommu-private.h
> + */
> +#ifndef __XEN_IOMMU_PRIVATE_H__
> +#define __XEN_IOMMU_PRIVATE_H__
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +#include <xen/device_tree.h>
> +
> +/*
> + * Checks if dt_device_node is assigned to a domain or not. This function
> + * expects to be called with dtdevs_lock acquired by caller.
> + */
> +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
> +#endif
> +
> +#endif /* __XEN_IOMMU_PRIVATE_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 10/19] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access
> to add/remove/assign/deassign.
> With addition of dynamic programming feature(follow-up patches in this series),

Typo: missing space before '('.

> this function can be concurrently access by pci device assign/deassign and also

I couldn't find any use of this function in the PCI code. So are you 
talking about not yet upstreamed patches?

Also, typo: s/access/accessed/

> by dynamic node add/remove using device tree overlays.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

The code itself looks good to me. So I will provide my reviewed-by tag 
once my question about the commit message is answered.

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 11/19] xen/iommu: Introduce iommu_remove_dt_device()

[Julien Grall]

Hi,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Remove master device from the IOMMU. This will be helpful when removing the
> overlay nodes using dynamic programming during run time.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
> Changes from v7:
>      Add check if IOMMU is enabled.
>      Fix indentation of fail.
> ---
> ---
>   xen/drivers/passthrough/device_tree.c | 44 +++++++++++++++++++++++++++
>   xen/include/xen/iommu.h               |  1 +
>   2 files changed, 45 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 096ef2dd68..4cb32dc0b3 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -128,6 +128,50 @@ int iommu_release_dt_devices(struct domain *d)
>       return 0;
>   }
>   
> +int iommu_remove_dt_device(struct dt_device_node *np)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    struct device *dev = dt_to_dev(np);
> +    int rc;
> +
> +    if ( !iommu_enabled )
> +        return 1;

The caller doesn't seem to check if the error code is > 0. So can we 
instead return a -ERRNO?

If you want to continue to return a value > 0 then I think it should be 
documented in a comment like we did for iommu_add_dt_device().

> +
> +    if ( !ops )
> +        return -EOPNOTSUPP;
> +
> +    spin_lock(&dtdevs_lock);
> +
> +    if ( iommu_dt_device_is_assigned_locked(np) )
> +    {
> +        rc = -EBUSY;
> +        goto fail;
> +    }
> +
> +    /*
> +     * The driver which supports generic IOMMU DT bindings must have this
> +     * callback implemented.
> +     */

I have questioned this message in v7 and I still question it. I guess 
you copied the comment on top of add_device(), this was add there 
because we have a different way to add legacy device.

But here there are no such requirement. In fact, you are not adding the 
the callback to all the IOMMU drivers... Yet all of them support the 
generic IOMMU DT bindings.

> +    if ( !ops->remove_device )
> +    {
> +        rc = -EOPNOTSUPP;
> +        goto fail;
> +    }
> +
> +    /*
> +     * Remove master device from the IOMMU if latter is present and available.

I read this as this will not return an error if the device is protected. 
However, AFAICT, the implement in the SMMU driver provided in this 
series will return an error. So I would suggest to replace this sentence 
with:

de-register the device from the IOMMU driver.

> +     * The driver is responsible for removing is_protected flag.

Can you add an assert in the 'if ( !rc )' block to confirm that 
is_protected was effectively removed. Something like:

ASSERT(!dt_device_is_protected(dev));

This would help to confirm the driver is respecting what you expect.

> +     */
> +    rc = ops->remove_device(0, dev);
> +
> +    if ( !rc )
> +        iommu_fwspec_free(dev);
> +
> + fail:
> +    spin_unlock(&dtdevs_lock);
> +    return rc;
> +}
> +
>   int iommu_add_dt_device(struct dt_device_node *np)
>   {
>       const struct iommu_ops *ops = iommu_get_ops();
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 110693c59f..a8e9bc9a2d 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np);
>   
>   int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
> +int iommu_remove_dt_device(struct dt_device_node *np);
>   
>   #endif /* HAS_DEVICE_TREE */
>   

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 13/19] asm/smp.h: Fix circular dependency for device_tree.h and rwlock.h

[Julien Grall]

Hi,

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Dynamic programming ops will modify the dt_host and there might be other
> function which are browsing the dt_host at the same time. To avoid the race
> conditions, we will need to add a rwlock to protect access to the dt_host.
> However, adding rwlock in device_tree.h causes following circular dependency:
>      device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h
> 
> To fix this, removed the "#include <xen/device_tree.h> and forward declared
> "struct dt_device_node".
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host

[Julien Grall]

Hi Vikram,

On 19/08/2023 01:28, Vikram Garhwal wrote:
>   Dynamic programming ops will modify the dt_host and there might be other
>   function which are browsing the dt_host at the same time. To avoid the race

Typo: I think you want to write 'functions'

>   conditions, adding rwlock for browsing the dt_host during runtime. dt_host
>   writer will be added in the follow-up patch titled "xen/arm: Implement device
>   tree node addition functionalities."

I would prefer if we avoid mention the name of the follow-up commit. 
This will reduce the risk that the name of the commit is incorrect if we 
decide to commit this patch before the rest of the series is ready.

Also, the commit message seems to be indented. Was it intended?

> 
>   Reason behind adding rwlock instead of spinlock:
>      For now, dynamic programming is the sole modifier of dt_host in Xen during
>          run time. All other access functions like iommu_release_dt_device() are
>          just reading the dt_host during run-time. So, there is a need to protect
>          others from browsing the dt_host while dynamic programming is modifying
>          it. rwlock is better suitable for this task as spinlock won't be able to
>          differentiate between read and write access.

The indentation looks odd here as well.

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> ---
> Changes from v7:
>      Keep one lock for dt_host instead of lock for each node under dt_host.
> ---
> ---
>   xen/common/device_tree.c              |  5 +++++
>   xen/drivers/passthrough/device_tree.c | 15 +++++++++++++++
>   xen/include/xen/device_tree.h         |  6 ++++++
>   3 files changed, 26 insertions(+)

I am not sue where to put the comment. I noticed that you didn't touch 
iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the 
caller is expected to held the lock? If so, then this should be 
documented and an ASSERT() should be added.

> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 0f10037745..6b934fe036 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -31,6 +31,7 @@ dt_irq_xlate_func dt_irq_xlate;
>   struct dt_device_node *dt_host;
>   /* Interrupt controller node*/
>   const struct dt_device_node *dt_interrupt_controller;
> +rwlock_t dt_host_lock;
>   
>   /**
>    * struct dt_alias_prop - Alias property in 'aliases' node
> @@ -2137,7 +2138,11 @@ int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
>   
>       dt_dprintk(" <- unflatten_device_tree()\n");
>   
> +    /* Init r/w lock for host device tree. */
> +    rwlock_init(&dt_host_lock);

Calling rwlock_init() from unflattent_device_tree() seems to be 
incorrect as it would lead to re-initialize the lock every time we are 
create a new DT overlay.

Instead you want to replace the definition of dt_host_lock with:

DEFINE_RWLOCK(dt_host_lock)

> +
>       return 0;
> +

Spurious change?

>   }
>   
>   static void dt_alias_add(struct dt_alias_prop *ap,
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 4cb32dc0b3..31815d2b60 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -114,6 +114,8 @@ int iommu_release_dt_devices(struct domain *d)
>       if ( !is_iommu_enabled(d) )
>           return 0;
>   
> +    read_lock(&dt_host_lock);
> +
>       list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list)
>       {
>           rc = iommu_deassign_dt_device(d, dev);

So iommu_deassign_dt_device() is now called with the read lock. I am 
assuming the intention is all the caller will need to fist held the 
lock. If so, then I think this would require an ASSERT() in 
iommu_deassign_dt_device() and a comment on top of the function because 
it is exported.

I am guessing that iommu_assign_dt_device() is in the same situation.


> @@ -121,10 +123,14 @@ int iommu_release_dt_devices(struct domain *d)
>           {
>               dprintk(XENLOG_ERR, "Failed to deassign %s in domain %u\n",
>                       dt_node_full_name(dev), d->domain_id);
> +
> +            read_unlock(&dt_host_lock);

Coding style: Usually we add the newline before the return. So I would 
switch around the two lines.

>               return rc;
>           }
>       }
>   
> +    read_unlock(&dt_host_lock);
> +
>       return 0;
>   }
>   
> @@ -251,6 +257,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>       int ret;
>       struct dt_device_node *dev;
>   
> +    read_lock(&dt_host_lock);
> +
>       switch ( domctl->cmd )
>       {
>       case XEN_DOMCTL_assign_device:
> @@ -304,7 +312,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>           spin_unlock(&dtdevs_lock);
>   
>           if ( d == dom_io )
> +        {
> +            read_unlock(&dt_host_lock);
>               return -EINVAL;

NIT: Rather than adding the unlock here, you could use:

rc = -EINVAL;
break;

> +        }
>   
>           ret = iommu_add_dt_device(dev);
>           if ( ret < 0 )
> @@ -342,7 +353,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>               break;
>   
>           if ( d == dom_io )
> +        {
> +            read_unlock(&dt_host_lock);
>               return -EINVAL;
> +        }

NIT: Same here.

>   
>           ret = iommu_deassign_dt_device(d, dev);
>   
> @@ -357,5 +371,6 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>           break;
>       }
>   
> +    read_unlock(&dt_host_lock);

Coding style: Please add a newline.

>       return ret;
>   }
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index e507658b23..8191f30197 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -18,6 +18,7 @@
>   #include <xen/string.h>
>   #include <xen/types.h>
>   #include <xen/list.h>
> +#include <xen/rwlock.h>
>   
>   #define DEVICE_TREE_MAX_DEPTH 16
>   
> @@ -216,6 +217,11 @@ extern struct dt_device_node *dt_host;
>    */
>   extern const struct dt_device_node *dt_interrupt_controller;
>   
> +/*
> + * Lock that protects r/w updates to unflattened device tree i.e. dt_host.
> + */

The wording suggests that any update to any node would require to hold 
the write lock. However.. it looks like you are only holding the read 
when setting is_protected in the SMMU remove callback. Is this intended?

Or maybe you expect is_protected by to protected by dtdevs_lock? If so, 
then I think it would be good to spell it out. Possibly on top of 
is_protected.

Lastly, there are plenty of place in Xen where the lock is not taken. 
They mostly seem to be at boot, so I would mention that for boot only 
code, then lock may not be taken.

Lastly, this is a single line comment, so the coding style should be:

/* ... */

> +extern rwlock_t dt_host_lock;
> +
>   /**
>    * Find the interrupt controller
>    * For the moment we handle only one interrupt controller: the first

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 00/19] dynamic node programming using overlay dtbo

[Julien Grall]

On 19/08/2023 01:28, Vikram Garhwal wrote:
> Hi,

Hi Vikram,

> This patch series is for introducing dynamic programming i.e. add/remove the
> devices during run time. Using "xl dt_overlay" a device can be added/removed
> with dtbo.

Do you have any pointer where one can find more details about the dtbo? 
How can it be created? What's the internal device-tree format?

> 
> For adding a node using dynamic programming:
>      1. flatten device tree overlay node will be added to a fdt
>      2. Updated fdt will be unflattened to a new dt_host_new
>      3. Extract the newly added node information from dt_host_new
>      4. Add the added node under correct parent in original dt_host.
>      3. Map/Permit interrupt and iomem region as required.
> 
> For removing a node:
>      1. Find the node with given path.
>      2. Check if the node is used by any of domus. Removes the node only when
>          it's not used by any domain.
>      3. Removes IRQ permissions and MMIO access.
>      5. Find the node in dt_host and delete the device node entry from dt_host.
>      6. Free the overlay_tracker entry which means free dt_host_new also(created
> in adding node step).
> 
> The main purpose of this series to address first part of the 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".

I am ok with this restriction. However are you planning to handle it 
because this goes out of tech preview?

The reason I am asking is, AFAICT, your code will not even look at the 
xen,passthrough (which is used to indicate that only permissions will be 
given). So if the ABI becomes stable, then we would need to invent a new 
property to say "Please map the device resources".

I would rather want the behavior of the dt-overlay to act the same as 
for the host DT. IOW, the device would be mapped to dom0 by default 
unless the propery "xen,passthrough" is added.

I would be OK to say that we don't currently support DT overlay if the 
property "xen,passthrough" is not present in the top-level node.

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 03/19] xen/arm/device: Remove __init from function type

[Vikram Garhwal]

Hi Julien,
On Tue, Aug 22, 2023 at 07:59:13PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Remove __init from following function to access during runtime:
> >      1. map_irq_to_domain()
> >      2. handle_device_interrupts()
> >      3. map_range_to_domain()
> >      4. unflatten_dt_node()
> 
> We are at v9 now, so this is more a remark for the future. In general we are
> trying to avoid modifying the same code multiple time within the series
> because this makes it more difficult to review. In this case, you could have
> removed the __init in patch #4 where you also export it.
> 
> > 
> > Move map_irq_to_domain() prototype from domain_build.h to setup.h.
> > 
> > To avoid breaking the build, following changes are also done:
> 
> I am guessing by "breaking the build", you mean that you will get an error
> because an non-init function is implemented in domain_build.c. Right? If so,
> I think this should be spelled out.
Yeah, i will change the commit with right reasoning.
> 
> > 1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain()
> >      to device.c. After removing __init type,  these functions are not specific
> >      to domain building, so moving them out of domain_build.c to device.c.
> > 2. Remove static type from handle_device_interrupt().
> 
> Typo: s/interrupt/interrupts/ to match the function name. But I don't link
> the name when it is exported as it leads to think this is dealing with real
> interrupt.
With using handle_device() in overlay as your below suggestion will anyway need
this handle_device_interrupts() function here.
> 
> Looking at the overlay code, the caller (i.e. add_resources()) is very
> similar to handle_device(). AFAICT the only differences are:
>    1/ add_resources() doesn't deal with mapping (you said it will in the
> future)
>    2/ You need to update some rangeset
> 
> For 1/ it means this is a superset. For 2/, I think this could be abstracted
> by adding a pointer to the rangesets in map_range_data. They could be NULL
> for the domain build case.
> 
> So can you look at abstracting? This will make easier to maintain a single
> place to parse a device node and map it.
> 
> A possible name for the function would be dt_map_resources_to_domain().
For this part of dynamic overlay programming this function can be used.
I updated the overlay code to use handle_device() as per your suggestions. Moved
handle_device() and other relevant function out of domain_build.
About renaming the function name to dt_map_resource_to_domain(): I will see if
i can come with better name else will keep your suggestion.
Now with this case, overlay device tree needs to have "xen,passthrough" enabled
else it will try to map and fail as Xen supports irq_route and mapping only at
the domain creation. In earlier patches this worked fine as we were always skip
the routing and map. Anyway, I will add this in overlay commit message.

I will send v10 tonight. Testing a few things.
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN][PATCH v9 04/19] common/device_tree: Export __unflatten_device_tree()

[Vikram Garhwal]

Hi Julien,
On Tue, Aug 22, 2023 at 08:05:18PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Following changes are done to __unflatten_device_tree():
> >      1. __unflatten_device_tree() is renamed to unflatten_device_tree().
> >      2. Remove __init and static function type.
> > 
> > The changes are done to make this function useable for dynamic node programming
> > where new device tree overlay nodes are added to fdt and further unflattend to
> > update xen device tree during runtime.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> It is not clear to me whether you saw my reply about using ERR_PTR() as I
> can't find an answer.
> 
I saw the comment but was not able to work on implementing ERR_PRT(). Given that
I see your Acked-by, i will try to address this as separate patch after this
series.
> Anyway, I am not against this interface:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN][PATCH v9 05/19] xen/arm: Add CONFIG_OVERLAY_DTB

[Vikram Garhwal]

Hi Julien,
On Tue, Aug 22, 2023 at 08:10:05PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Introduce a config option where the user can enable support for adding/removing
> > device tree nodes using a device tree binary overlay.
> > 
> > Update SUPPORT.md and CHANGELOG.md to state the Device Tree Overlays support for
> > Arm.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > Acked-by: Henry Wang <Henry.Wang@arm.com>
> > Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> > 
> > ---
> > Changes from v7:
> >      Add this feature as "experimental support" in CHANGELOG.md
> > ---
> > ---
> >   CHANGELOG.md         | 3 ++-
> >   SUPPORT.md           | 6 ++++++
> >   xen/arch/arm/Kconfig | 5 +++++
> >   3 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/CHANGELOG.md b/CHANGELOG.md
> > index 7d7e0590f8..47098dbfca 100644
> > --- a/CHANGELOG.md
> > +++ b/CHANGELOG.md
> > @@ -24,7 +24,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
> >    - xl/libxl can customize SMBIOS strings for HVM guests.
> >    - Add support for AVX512-FP16 on x86.
> >    - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
> > -
> > + - On Arm, experimental support for dynamic addition/removal of Xen device tree
> > +   nodes using a device tree overlay binary(.dtbo).
> 
> Typo: missing space before (.
> 
> >   ## [4.17.0](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.17.0) - 2022-12-12
> > diff --git a/SUPPORT.md b/SUPPORT.md
> > index 35a6249e03..8eb006565c 100644
> > --- a/SUPPORT.md
> > +++ b/SUPPORT.md
> > @@ -844,6 +844,12 @@ No support for QEMU backends in a 16K or 64K domain.
> >       Status: Supported
> > +### Device Tree Overlays
> > +
> > +Add/Remove device tree nodes using a device tree overlay binary(.dtbo).
> 
> Same here. I don't suggest to handle it on commit because this is not
> something I want to merge without the rest of the series.
> 
> > +
> > +    Status, ARM: Experimental
> > +
> >   ### ARM: Guest ACPI support
> >       Status: Supported
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index fd57a82dd2..02c4796438 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -92,6 +92,11 @@ config HAS_ITS
> >           bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
> >           depends on GICV3 && !NEW_VGIC && !ARM_32
> > +config OVERLAY_DTB
> > +	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> > +	help
> > +	  Dynamic addition/removal of Xen device tree nodes using a dtbo.
> 
> Do we have any documentation in the tree of the limitations and how this
> works?
> 
> The reason I am asking is the wording here suggests that it would be
> possible to remove nodes from the original Device-Tree. AFAIU this is not
> possible with the implementation and you are not planning to handle it.
> Correct?
Yes, that is correct. This series doesn't remove the nodes which are not added
by overlay before.

I will add a document file. Is this needs to be in .pandoc or .txt format?
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN][PATCH v9 08/19] xen/device-tree: Add device_tree_find_node_by_path() to find nodes in device tree

[Vikram Garhwal]

Hi Julien,
On Tue, Aug 22, 2023 at 08:21:17PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Add device_tree_find_node_by_path() to find a matching node with path for a
> > dt_device_node.
> > 
> > Reason behind this function:
> >      Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
> 
> Typo: missing space before (.
> 
> >      device_tree_flattened) is created and updated with overlay nodes. This
> >      updated fdt is further unflattened to a dt_host_new. Next, we need to find
> >      the overlay nodes in dt_host_new, find the overlay node's parent in dt_host
> >      and add the nodes as child under their parent in the dt_host. Thus we need
> >      this function to search for node in different unflattened device trees.
> > 
> > Also, make dt_find_node_by_path() static inline.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> > ---
> > Changes from v7:
> >      Rename device_tree_find_node_by_path() to dt_find_node_by_path_from().
> >      Fix indentation.
> > ---
> > ---
> >   xen/common/device_tree.c      |  5 +++--
> >   xen/include/xen/device_tree.h | 18 ++++++++++++++++--
> >   2 files changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 67e9b6de3e..0f10037745 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -358,11 +358,12 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
> >       return np;
> >   }
> > -struct dt_device_node *dt_find_node_by_path(const char *path)
> > +struct dt_device_node *dt_find_node_by_path_from(struct dt_device_node *from,
> > +                                                 const char *path)
> 
> Any change this both 'from' and the return can be const?
Doing this will also need changes in dt_find_node_by_path() and
dt_for_each_device_node(). This function calls both of these.
> 
> >   {
> >       struct dt_device_node *np;
> > -    dt_for_each_device_node(dt_host, np)
> > +    dt_for_each_device_node(from, np)
> >           if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
> >               break;
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 5941599eff..e507658b23 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -568,13 +568,27 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
> >   struct dt_device_node *dt_find_node_by_alias(const char *alias);
> >   /**
> > - * dt_find_node_by_path - Find a node matching a full DT path
> > + * dt_find_node_by_path_from - Generic function to find a node matching the
> > + * full DT path for any given unflatten device tree
> > + * @from: The device tree node to start searching from
> >    * @path: The full path to match
> >    *
> >    * Returns a node pointer.
> >    */
> > -struct dt_device_node *dt_find_node_by_path(const char *path);
> > +struct dt_device_node *
> > +                    dt_find_node_by_path_from(struct dt_device_node *from,
> > +                                                  const char *path);
> 
> Coding style: The indentation look rather odd. I am not sure it will render
> properly here. But one style that is closer to the rest of the file and Xen
> is:
> 
> struct dt_device_node *dt_find_node_by_path_from(struct dt_device_node
> *from,
>                                                  const char *path);
> 
> Where the return type, function name and first parameter is one line and the
> second parameter is on the second line with the type aligned with the type
> of the first parameter.
Will do!
> 
> > +/**
> > + * dt_find_node_by_path - Find a node matching a full DT path in dt_host
> > + * @path: The full path to match
> > + *
> > + * Returns a node pointer.
> > + */
> > +static inline struct dt_device_node *dt_find_node_by_path(const char *path)
> > +{
> > +    return dt_find_node_by_path_from(dt_host, path);
> > +}
> >   /**
> >    * dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN][PATCH v9 10/19] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock

[Vikram Garhwal]

Hi Julien,
On Tue, Aug 22, 2023 at 08:47:10PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access
> > to add/remove/assign/deassign.
> > With addition of dynamic programming feature(follow-up patches in this series),
> 
> Typo: missing space before '('.
> 
> > this function can be concurrently access by pci device assign/deassign and also
> 
> I couldn't find any use of this function in the PCI code. So are you talking
> about not yet upstreamed patches?
So, this assign and deassign is also used by pci-assignable-add from xl which
"Make a device assignable for pci-passthru"
> 
> Also, typo: s/access/accessed/
> 
> > by dynamic node add/remove using device tree overlays.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> > Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> The code itself looks good to me. So I will provide my reviewed-by tag once
> my question about the commit message is answered.
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN][PATCH v9 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Vikram Garhwal]

Hi Julien,
On Tue, Aug 22, 2023 at 08:43:27PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> > Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_locked().
> > Remove static type so this can also be used by SMMU drivers to check if the
> > device is being used before removing.
> 
> I have commented on v8. But I will comment here to make easier to keep track
> of comment.
> 
> I don't think iommu_dt_device_is_assigned_locked() should be called from the
> SMMU. If you want to check a device is assigned then it would be best to use
> the internal state of the SMMU.
> 
> This has two benefits:
>   * This avoids what I view as a layer of violation
>   * This confirmed that the internal state match with what we expect
> 
> > 
> > Moving spin_lock to caller was done to prevent the concurrent access to
> > iommu_dt_device_is_assigned while doing add/remove/assign/deassign. Follow-up
> > patches in this series introduces node add/remove feature.
> > 
> > Also, caller is required hold the correct lock so moved the function prototype
> > to a private header.
> 
> I don't understand how requiring the caller to hold the correct lock means
> you need to move the protype in a private header. Can you clarify?
> 
With removal of check in smmu.c, this header is no longer required.
will remove private header too.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > ---
> > Changes from v7:
> >      Update commit message.
> >      Add ASSERT().
> > ---
> > ---
> >   xen/drivers/passthrough/device_tree.c | 26 +++++++++++++++++++++----
> >   xen/include/xen/iommu-private.h       | 28 +++++++++++++++++++++++++++
> >   2 files changed, 50 insertions(+), 4 deletions(-)
> >   create mode 100644 xen/include/xen/iommu-private.h
> > 
> > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> > index 1c32d7b50c..5796ee1f93 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -18,6 +18,7 @@
> >   #include <xen/device_tree.h>
> >   #include <xen/guest_access.h>
> >   #include <xen/iommu.h>
> > +#include <xen/iommu-private.h>
> >   #include <xen/lib.h>
> >   #include <xen/sched.h>
> >   #include <xsm/xsm.h>
> > @@ -83,16 +84,16 @@ fail:
> >       return rc;
> >   }
> > -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
> >   {
> >       bool_t assigned = 0;
> > +    ASSERT(spin_is_locked(&dtdevs_lock));
> > +
> >       if ( !dt_device_is_protected(dev) )
> >           return 0;
> > -    spin_lock(&dtdevs_lock);
> >       assigned = !list_empty(&dev->domain_list);
> > -    spin_unlock(&dtdevs_lock);
> >       return assigned;
> >   }
> > @@ -213,27 +214,44 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> >           if ( (d && d->is_dying) || domctl->u.assign_device.flags )
> >               break;
> > +        /*
> > +         * To ensure that the dev doesn't disappear between the time we look it
> > +         * up with dt_find_node_by_gpath() and we check the assignment later.
> > +         */
> > +        spin_lock(&dtdevs_lock);
> 
> This change doesn't look to be explained in the commit message. But looking
> at the code after this series is applied, you justified the addition of
> read_lock(&dt_host_lock) to protect access to the device-tree. This will be
> held longer than dtdevs_lock. So is it actually necessary to move the
> locking earlier?
I re-looked at the implementation and actually, dt_host_lock will protect the
dt related changes. I will move it down before iommu_dt_device_is_assigned_lock()
call.
> 
> > +
> >           ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
> >                                       domctl->u.assign_device.u.dt.size,
> >                                       &dev);
> >           if ( ret )
> > +        {
> > +            spin_unlock(&dtdevs_lock);
> >               break;
> > +        }
> >           ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
> >           if ( ret )
> > +        {
> > +            spin_unlock(&dtdevs_lock);
> >               break;
> > +        }
> >           if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> >           {
> > -            if ( iommu_dt_device_is_assigned(dev) )
> > +
> > +            if ( iommu_dt_device_is_assigned_locked(dev) )
> >               {
> >                   printk(XENLOG_G_ERR "%s already assigned.\n",
> >                          dt_node_full_name(dev));
> >                   ret = -EINVAL;
> >               }
> > +
> > +            spin_unlock(&dtdevs_lock);
> >               break;
> >           }
> > +        spin_unlock(&dtdevs_lock);
> > +
> >           if ( d == dom_io )
> >               return -EINVAL;
> > diff --git a/xen/include/xen/iommu-private.h b/xen/include/xen/iommu-private.h
> > new file mode 100644
> > index 0000000000..bb5c94e7a6
> > --- /dev/null
> > +++ b/xen/include/xen/iommu-private.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * xen/iommu-private.h
> > + */
> > +#ifndef __XEN_IOMMU_PRIVATE_H__
> > +#define __XEN_IOMMU_PRIVATE_H__
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +#include <xen/device_tree.h>
> > +
> > +/*
> > + * Checks if dt_device_node is assigned to a domain or not. This function
> > + * expects to be called with dtdevs_lock acquired by caller.
> > + */
> > +bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
> > +#endif
> > +
> > +#endif /* __XEN_IOMMU_PRIVATE_H__ */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal]

Hi Julien,
On Wed, Aug 23, 2023 at 11:06:59PM +0100, Julien Grall wrote:
> Hi Vikram,
> 
> On 19/08/2023 01:28, Vikram Garhwal wrote:
> >   Dynamic programming ops will modify the dt_host and there might be other
> >   function which are browsing the dt_host at the same time. To avoid the race
> 
> Typo: I think you want to write 'functions'
> 
> >   conditions, adding rwlock for browsing the dt_host during runtime. dt_host
> >   writer will be added in the follow-up patch titled "xen/arm: Implement device
> >   tree node addition functionalities."
> 
> I would prefer if we avoid mention the name of the follow-up commit. This
> will reduce the risk that the name of the commit is incorrect if we decide
> to commit this patch before the rest of the series is ready.
> 
> Also, the commit message seems to be indented. Was it intended?
> 
> > 
> >   Reason behind adding rwlock instead of spinlock:
> >      For now, dynamic programming is the sole modifier of dt_host in Xen during
> >          run time. All other access functions like iommu_release_dt_device() are
> >          just reading the dt_host during run-time. So, there is a need to protect
> >          others from browsing the dt_host while dynamic programming is modifying
> >          it. rwlock is better suitable for this task as spinlock won't be able to
> >          differentiate between read and write access.
> 
> The indentation looks odd here as well.
> 
Changed above comments in v10.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > ---
> > Changes from v7:
> >      Keep one lock for dt_host instead of lock for each node under dt_host.
> > ---
> > ---
> >   xen/common/device_tree.c              |  5 +++++
> >   xen/drivers/passthrough/device_tree.c | 15 +++++++++++++++
> >   xen/include/xen/device_tree.h         |  6 ++++++
> >   3 files changed, 26 insertions(+)
> 
> I am not sue where to put the comment. I noticed that you didn't touch
> iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the
> caller is expected to held the lock? If so, then this should be documented
> and an ASSERT() should be added.
Added ASSERT in iommu_(add,remove,assign and deassign)_dt_device(),
> 
> > 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 0f10037745..6b934fe036 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -31,6 +31,7 @@ dt_irq_xlate_func dt_irq_xlate;
> >   struct dt_device_node *dt_host;
> >   /* Interrupt controller node*/
> >   const struct dt_device_node *dt_interrupt_controller;
> > +rwlock_t dt_host_lock;
> >   /**
> >    * struct dt_alias_prop - Alias property in 'aliases' node
> > @@ -2137,7 +2138,11 @@ int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
> >       dt_dprintk(" <- unflatten_device_tree()\n");
> > +    /* Init r/w lock for host device tree. */
> > +    rwlock_init(&dt_host_lock);
> 
> Calling rwlock_init() from unflattent_device_tree() seems to be incorrect as
> it would lead to re-initialize the lock every time we are create a new DT
> overlay.
> 
> Instead you want to replace the definition of dt_host_lock with:
> 
> DEFINE_RWLOCK(dt_host_lock)
> 
Changed this. DEFINE_RWLOCK is added to device-tree.c and this is removed.
> > +
> >       return 0;
> > +
> 
> Spurious change?
> 
> >   }
> >   static void dt_alias_add(struct dt_alias_prop *ap,
> > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> > index 4cb32dc0b3..31815d2b60 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -114,6 +114,8 @@ int iommu_release_dt_devices(struct domain *d)
> >       if ( !is_iommu_enabled(d) )
> >           return 0;
> > +    read_lock(&dt_host_lock);
> > +
> >       list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list)
> >       {
> >           rc = iommu_deassign_dt_device(d, dev);
> 
> So iommu_deassign_dt_device() is now called with the read lock. I am
> assuming the intention is all the caller will need to fist held the lock. If
> so, then I think this would require an ASSERT() in
> iommu_deassign_dt_device() and a comment on top of the function because it
> is exported.
> 
> I am guessing that iommu_assign_dt_device() is in the same situation.
> 
> 
> > @@ -121,10 +123,14 @@ int iommu_release_dt_devices(struct domain *d)
> >           {
> >               dprintk(XENLOG_ERR, "Failed to deassign %s in domain %u\n",
> >                       dt_node_full_name(dev), d->domain_id);
> > +
> > +            read_unlock(&dt_host_lock);
> 
> Coding style: Usually we add the newline before the return. So I would
> switch around the two lines.
> 
> >               return rc;
> >           }
> >       }
> > +    read_unlock(&dt_host_lock);
> > +
> >       return 0;
> >   }
> > @@ -251,6 +257,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> >       int ret;
> >       struct dt_device_node *dev;
> > +    read_lock(&dt_host_lock);
> > +
> >       switch ( domctl->cmd )
> >       {
> >       case XEN_DOMCTL_assign_device:
> > @@ -304,7 +312,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> >           spin_unlock(&dtdevs_lock);
> >           if ( d == dom_io )
> > +        {
> > +            read_unlock(&dt_host_lock);
> >               return -EINVAL;
> 
> NIT: Rather than adding the unlock here, you could use:
> 
> rc = -EINVAL;
> break;
> 
> > +        }
> >           ret = iommu_add_dt_device(dev);
> >           if ( ret < 0 )
> > @@ -342,7 +353,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> >               break;
> >           if ( d == dom_io )
> > +        {
> > +            read_unlock(&dt_host_lock);
> >               return -EINVAL;
> > +        }
> 
> NIT: Same here.
> 
> >           ret = iommu_deassign_dt_device(d, dev);
> > @@ -357,5 +371,6 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> >           break;
> >       }
> > +    read_unlock(&dt_host_lock);
> 
> Coding style: Please add a newline.
> 
Changed all above coding styles.
> >       return ret;
> >   }
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index e507658b23..8191f30197 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -18,6 +18,7 @@
> >   #include <xen/string.h>
> >   #include <xen/types.h>
> >   #include <xen/list.h>
> > +#include <xen/rwlock.h>
> >   #define DEVICE_TREE_MAX_DEPTH 16
> > @@ -216,6 +217,11 @@ extern struct dt_device_node *dt_host;
> >    */
> >   extern const struct dt_device_node *dt_interrupt_controller;
> > +/*
> > + * Lock that protects r/w updates to unflattened device tree i.e. dt_host.
> > + */
> 
> The wording suggests that any update to any node would require to hold the
> write lock. However.. it looks like you are only holding the read when
> setting is_protected in the SMMU remove callback. Is this intended?
> 
> Or maybe you expect is_protected by to protected by dtdevs_lock? If so, then
> I think it would be good to spell it out. Possibly on top of is_protected.
> 
Yes, dtdevs_lock will be held to avoid concurrent calls to SMMU remove.
> Lastly, there are plenty of place in Xen where the lock is not taken. They
> mostly seem to be at boot, so I would mention that for boot only code, then
> lock may not be taken.
Updated.
> 
> Lastly, this is a single line comment, so the coding style should be:
> 
> /* ... */
> 
> > +extern rwlock_t dt_host_lock;
> > +
> >   /**
> >    * Find the interrupt controller
> >    * For the moment we handle only one interrupt controller: the first
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal]

Hi,
On Thu, Aug 24, 2023 at 11:22:00PM -0700, Vikram Garhwal wrote:
> Hi Julien,
> On Wed, Aug 23, 2023 at 11:06:59PM +0100, Julien Grall wrote:
> > Hi Vikram,
> > 
> > On 19/08/2023 01:28, Vikram Garhwal wrote:
> > >   Dynamic programming ops will modify the dt_host and there might be other
> > >   function which are browsing the dt_host at the same time. To avoid the race
> > 
> > Typo: I think you want to write 'functions'
> > 
> > >   conditions, adding rwlock for browsing the dt_host during runtime. dt_host
> > >   writer will be added in the follow-up patch titled "xen/arm: Implement device
> > >   tree node addition functionalities."
> > 
> > I would prefer if we avoid mention the name of the follow-up commit. This
> > will reduce the risk that the name of the commit is incorrect if we decide
> > to commit this patch before the rest of the series is ready.
> > 
> > Also, the commit message seems to be indented. Was it intended?
> > 
> > > 
> > >   Reason behind adding rwlock instead of spinlock:
> > >      For now, dynamic programming is the sole modifier of dt_host in Xen during
> > >          run time. All other access functions like iommu_release_dt_device() are
> > >          just reading the dt_host during run-time. So, there is a need to protect
> > >          others from browsing the dt_host while dynamic programming is modifying
> > >          it. rwlock is better suitable for this task as spinlock won't be able to
> > >          differentiate between read and write access.
> > 
> > The indentation looks odd here as well.
> > 
> Changed above comments in v10.
> > > 
> > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > > 
> > > ---
> > > Changes from v7:
> > >      Keep one lock for dt_host instead of lock for each node under dt_host.
> > > ---
> > > ---
> > >   xen/common/device_tree.c              |  5 +++++
> > >   xen/drivers/passthrough/device_tree.c | 15 +++++++++++++++
> > >   xen/include/xen/device_tree.h         |  6 ++++++
> > >   3 files changed, 26 insertions(+)
> > 
> > I am not sue where to put the comment. I noticed that you didn't touch
> > iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the
> > caller is expected to held the lock? If so, then this should be documented
> > and an ASSERT() should be added.
> Added ASSERT in iommu_(add,remove,assign and deassign)_dt_device(),
iommu_add_ and iommu_assign_ are called at boot time. Also, only other callers
are handle_device via overlays and iommu_do_dt_domctl() which will hold the
dt_host_lock. Will look into it more but for now sending v10 with ASSER in these
two functions.
> > 
> > > 
> > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > > index 0f10037745..6b934fe036 100644
> > > --- a/xen/common/device_tree.c
> > > +++ b/xen/common/device_tree.c
> > > @@ -31,6 +31,7 @@ dt_irq_xlate_func dt_irq_xlate;
> > >   struct dt_device_node *dt_host;
> > >   /* Interrupt controller node*/
> > >   const struct dt_device_node *dt_interrupt_controller;
> > > +rwlock_t dt_host_lock;
> > >   /**
> > >    * struct dt_alias_prop - Alias property in 'aliases' node
> > > @@ -2137,7 +2138,11 @@ int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
> > >       dt_dprintk(" <- unflatten_device_tree()\n");
> > > +    /* Init r/w lock for host device tree. */
> > > +    rwlock_init(&dt_host_lock);
> > 
> > Calling rwlock_init() from unflattent_device_tree() seems to be incorrect as
> > it would lead to re-initialize the lock every time we are create a new DT
> > overlay.
> > 
> > Instead you want to replace the definition of dt_host_lock with:
> > 
> > DEFINE_RWLOCK(dt_host_lock)
> > 
> Changed this. DEFINE_RWLOCK is added to device-tree.c and this is removed.
> > > +
> > >       return 0;
> > > +
> > 
> > Spurious change?
> > 
> > >   }
> > >   static void dt_alias_add(struct dt_alias_prop *ap,
> > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> > > index 4cb32dc0b3..31815d2b60 100644
> > > --- a/xen/drivers/passthrough/device_tree.c
> > > +++ b/xen/drivers/passthrough/device_tree.c
> > > @@ -114,6 +114,8 @@ int iommu_release_dt_devices(struct domain *d)
> > >       if ( !is_iommu_enabled(d) )
> > >           return 0;
> > > +    read_lock(&dt_host_lock);
> > > +
> > >       list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list)
> > >       {
> > >           rc = iommu_deassign_dt_device(d, dev);
> > 
> > So iommu_deassign_dt_device() is now called with the read lock. I am
> > assuming the intention is all the caller will need to fist held the lock. If
> > so, then I think this would require an ASSERT() in
> > iommu_deassign_dt_device() and a comment on top of the function because it
> > is exported.
> > 
> > I am guessing that iommu_assign_dt_device() is in the same situation.
> > 
> > 
> > > @@ -121,10 +123,14 @@ int iommu_release_dt_devices(struct domain *d)
> > >           {
> > >               dprintk(XENLOG_ERR, "Failed to deassign %s in domain %u\n",
> > >                       dt_node_full_name(dev), d->domain_id);
> > > +
> > > +            read_unlock(&dt_host_lock);
> > 
> > Coding style: Usually we add the newline before the return. So I would
> > switch around the two lines.
> > 
> > >               return rc;
> > >           }
> > >       }
> > > +    read_unlock(&dt_host_lock);
> > > +
> > >       return 0;
> > >   }
> > > @@ -251,6 +257,8 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> > >       int ret;
> > >       struct dt_device_node *dev;
> > > +    read_lock(&dt_host_lock);
> > > +
> > >       switch ( domctl->cmd )
> > >       {
> > >       case XEN_DOMCTL_assign_device:
> > > @@ -304,7 +312,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> > >           spin_unlock(&dtdevs_lock);
> > >           if ( d == dom_io )
> > > +        {
> > > +            read_unlock(&dt_host_lock);
> > >               return -EINVAL;
> > 
> > NIT: Rather than adding the unlock here, you could use:
> > 
> > rc = -EINVAL;
> > break;
> > 
> > > +        }
> > >           ret = iommu_add_dt_device(dev);
> > >           if ( ret < 0 )
> > > @@ -342,7 +353,10 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> > >               break;
> > >           if ( d == dom_io )
> > > +        {
> > > +            read_unlock(&dt_host_lock);
> > >               return -EINVAL;
> > > +        }
> > 
> > NIT: Same here.
> > 
> > >           ret = iommu_deassign_dt_device(d, dev);
> > > @@ -357,5 +371,6 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> > >           break;
> > >       }
> > > +    read_unlock(&dt_host_lock);
> > 
> > Coding style: Please add a newline.
> > 
> Changed all above coding styles.
> > >       return ret;
> > >   }
> > > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > > index e507658b23..8191f30197 100644
> > > --- a/xen/include/xen/device_tree.h
> > > +++ b/xen/include/xen/device_tree.h
> > > @@ -18,6 +18,7 @@
> > >   #include <xen/string.h>
> > >   #include <xen/types.h>
> > >   #include <xen/list.h>
> > > +#include <xen/rwlock.h>
> > >   #define DEVICE_TREE_MAX_DEPTH 16
> > > @@ -216,6 +217,11 @@ extern struct dt_device_node *dt_host;
> > >    */
> > >   extern const struct dt_device_node *dt_interrupt_controller;
> > > +/*
> > > + * Lock that protects r/w updates to unflattened device tree i.e. dt_host.
> > > + */
> > 
> > The wording suggests that any update to any node would require to hold the
> > write lock. However.. it looks like you are only holding the read when
> > setting is_protected in the SMMU remove callback. Is this intended?
> > 
> > Or maybe you expect is_protected by to protected by dtdevs_lock? If so, then
> > I think it would be good to spell it out. Possibly on top of is_protected.
> > 
> Yes, dtdevs_lock will be held to avoid concurrent calls to SMMU remove.
> > Lastly, there are plenty of place in Xen where the lock is not taken. They
> > mostly seem to be at boot, so I would mention that for boot only code, then
> > lock may not be taken.
> Updated.
> > 
> > Lastly, this is a single line comment, so the coding style should be:
> > 
> > /* ... */
> > 
> > > +extern rwlock_t dt_host_lock;
> > > +
> > >   /**
> > >    * Find the interrupt controller
> > >    * For the moment we handle only one interrupt controller: the first
> > 
> > Cheers,
> > 
> > -- 
> > Julien Grall

Re: [XEN][PATCH v9 03/19] xen/arm/device: Remove __init from function type

[Julien Grall]

Hi,

On 25/08/2023 01:52, Vikram Garhwal wrote:
> Hi Julien,
> On Tue, Aug 22, 2023 at 07:59:13PM +0100, Julien Grall wrote:
>> Hi Vikram,
>>
>> On 19/08/2023 01:28, Vikram Garhwal wrote:
>>> Remove __init from following function to access during runtime:
>>>       1. map_irq_to_domain()
>>>       2. handle_device_interrupts()
>>>       3. map_range_to_domain()
>>>       4. unflatten_dt_node()
>>
>> We are at v9 now, so this is more a remark for the future. In general we are
>> trying to avoid modifying the same code multiple time within the series
>> because this makes it more difficult to review. In this case, you could have
>> removed the __init in patch #4 where you also export it.
>>
>>>
>>> Move map_irq_to_domain() prototype from domain_build.h to setup.h.
>>>
>>> To avoid breaking the build, following changes are also done:
>>
>> I am guessing by "breaking the build", you mean that you will get an error
>> because an non-init function is implemented in domain_build.c. Right? If so,
>> I think this should be spelled out.
> Yeah, i will change the commit with right reasoning.
>>
>>> 1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain()
>>>       to device.c. After removing __init type,  these functions are not specific
>>>       to domain building, so moving them out of domain_build.c to device.c.
>>> 2. Remove static type from handle_device_interrupt().
>>
>> Typo: s/interrupt/interrupts/ to match the function name. But I don't link
>> the name when it is exported as it leads to think this is dealing with real
>> interrupt.
> With using handle_device() in overlay as your below suggestion will anyway need
> this handle_device_interrupts() function here.

Ah I didn't notice it was used in handle_passthrough_prop(). So it 
indeed needs to be exported. In which case, I would suggest to rename to 
"map_device_irqs_to_domain()".

>>
>> Looking at the overlay code, the caller (i.e. add_resources()) is very
>> similar to handle_device(). AFAICT the only differences are:
>>     1/ add_resources() doesn't deal with mapping (you said it will in the
>> future)
>>     2/ You need to update some rangeset
>>
>> For 1/ it means this is a superset. For 2/, I think this could be abstracted
>> by adding a pointer to the rangesets in map_range_data. They could be NULL
>> for the domain build case.
>>
>> So can you look at abstracting? This will make easier to maintain a single
>> place to parse a device node and map it.
>>
>> A possible name for the function would be dt_map_resources_to_domain().
> For this part of dynamic overlay programming this function can be used.
> I updated the overlay code to use handle_device() as per your suggestions. Moved
> handle_device() and other relevant function out of domain_build.
> About renaming the function name to dt_map_resource_to_domain(): I will see if
> i can come with better name else will keep your suggestion.
> Now with this case, overlay device tree needs to have "xen,passthrough" enabled
> else it will try to map and fail as Xen supports irq_route and mapping only at
> the domain creation. In earlier patches this worked fine as we were always skip
> the routing and map.

Right, but this also meant the behavior when parsing the DT overlay 
would not have been consistent with parsing the host DT.

You would have then had to break backward compatibility to add support 
for mapping the resources. If this was planned to be dealt before DT 
Overlay becomes supported, then this would have been fine as we don't 
guarantee stable interface for experimental/tech preview feature.

The new approach should at least prevent any backward compatibility 
breakage and would allow us to support DT overlay even before the 
resource mapping is supported. So I think this is better.

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 05/19] xen/arm: Add CONFIG_OVERLAY_DTB

[Julien Grall]

On 25/08/2023 04:17, Vikram Garhwal wrote:
> Hi Julien,
> On Tue, Aug 22, 2023 at 08:10:05PM +0100, Julien Grall wrote:
>> Hi Vikram,
>>
>> On 19/08/2023 01:28, Vikram Garhwal wrote:
>>> Introduce a config option where the user can enable support for adding/removing
>>> device tree nodes using a device tree binary overlay.
>>>
>>> Update SUPPORT.md and CHANGELOG.md to state the Device Tree Overlays support for
>>> Arm.
>>>
>>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>>> Acked-by: Henry Wang <Henry.Wang@arm.com>
>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>
>>> ---
>>> Changes from v7:
>>>       Add this feature as "experimental support" in CHANGELOG.md
>>> ---
>>> ---
>>>    CHANGELOG.md         | 3 ++-
>>>    SUPPORT.md           | 6 ++++++
>>>    xen/arch/arm/Kconfig | 5 +++++
>>>    3 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>> index 7d7e0590f8..47098dbfca 100644
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -24,7 +24,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>     - xl/libxl can customize SMBIOS strings for HVM guests.
>>>     - Add support for AVX512-FP16 on x86.
>>>     - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
>>> -
>>> + - On Arm, experimental support for dynamic addition/removal of Xen device tree
>>> +   nodes using a device tree overlay binary(.dtbo).
>>
>> Typo: missing space before (.
>>
>>>    ## [4.17.0](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.17.0) - 2022-12-12
>>> diff --git a/SUPPORT.md b/SUPPORT.md
>>> index 35a6249e03..8eb006565c 100644
>>> --- a/SUPPORT.md
>>> +++ b/SUPPORT.md
>>> @@ -844,6 +844,12 @@ No support for QEMU backends in a 16K or 64K domain.
>>>        Status: Supported
>>> +### Device Tree Overlays
>>> +
>>> +Add/Remove device tree nodes using a device tree overlay binary(.dtbo).
>>
>> Same here. I don't suggest to handle it on commit because this is not
>> something I want to merge without the rest of the series.
>>
>>> +
>>> +    Status, ARM: Experimental
>>> +
>>>    ### ARM: Guest ACPI support
>>>        Status: Supported
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index fd57a82dd2..02c4796438 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -92,6 +92,11 @@ config HAS_ITS
>>>            bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
>>>            depends on GICV3 && !NEW_VGIC && !ARM_32
>>> +config OVERLAY_DTB
>>> +	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
>>> +	help
>>> +	  Dynamic addition/removal of Xen device tree nodes using a dtbo.
>>
>> Do we have any documentation in the tree of the limitations and how this
>> works?
>>
>> The reason I am asking is the wording here suggests that it would be
>> possible to remove nodes from the original Device-Tree. AFAIU this is not
>> possible with the implementation and you are not planning to handle it.
>> Correct?
> Yes, that is correct. This series doesn't remove the nodes which are not added
> by overlay before.
> 
> I will add a document file. Is this needs to be in .pandoc or .txt format?

I think we now prefer .pandoc.

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 10/19] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock

[Julien Grall]

On 25/08/2023 05:44, Vikram Garhwal wrote:
> Hi Julien,
> On Tue, Aug 22, 2023 at 08:47:10PM +0100, Julien Grall wrote:
>> Hi Vikram,
>>
>> On 19/08/2023 01:28, Vikram Garhwal wrote:
>>> Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access
>>> to add/remove/assign/deassign.
>>> With addition of dynamic programming feature(follow-up patches in this series),
>>
>> Typo: missing space before '('.
>>
>>> this function can be concurrently access by pci device assign/deassign and also
>>
>> I couldn't find any use of this function in the PCI code. So are you talking
>> about not yet upstreamed patches?
> So, this assign and deassign is also used by pci-assignable-add from xl which
> "Make a device assignable for pci-passthru"

Hmmm... But this is not something we currently support on Arm and in 
fact this is not plumbed. So please remove any reference to PCI because 
this is misleading.

Cheers,

-- 
Julien Grall

Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host

[Julien Grall]

Hi,

On 25/08/2023 08:52, Vikram Garhwal wrote:
> Hi,
> On Thu, Aug 24, 2023 at 11:22:00PM -0700, Vikram Garhwal wrote:
>> Hi Julien,
>> On Wed, Aug 23, 2023 at 11:06:59PM +0100, Julien Grall wrote:
>>> Hi Vikram,
>>>
>>> On 19/08/2023 01:28, Vikram Garhwal wrote:
>>>>    Dynamic programming ops will modify the dt_host and there might be other
>>>>    function which are browsing the dt_host at the same time. To avoid the race
>>>
>>> Typo: I think you want to write 'functions'
>>>
>>>>    conditions, adding rwlock for browsing the dt_host during runtime. dt_host
>>>>    writer will be added in the follow-up patch titled "xen/arm: Implement device
>>>>    tree node addition functionalities."
>>>
>>> I would prefer if we avoid mention the name of the follow-up commit. This
>>> will reduce the risk that the name of the commit is incorrect if we decide
>>> to commit this patch before the rest of the series is ready.
>>>
>>> Also, the commit message seems to be indented. Was it intended?
>>>
>>>>
>>>>    Reason behind adding rwlock instead of spinlock:
>>>>       For now, dynamic programming is the sole modifier of dt_host in Xen during
>>>>           run time. All other access functions like iommu_release_dt_device() are
>>>>           just reading the dt_host during run-time. So, there is a need to protect
>>>>           others from browsing the dt_host while dynamic programming is modifying
>>>>           it. rwlock is better suitable for this task as spinlock won't be able to
>>>>           differentiate between read and write access.
>>>
>>> The indentation looks odd here as well.
>>>
>> Changed above comments in v10.
>>>>
>>>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>>>>
>>>> ---
>>>> Changes from v7:
>>>>       Keep one lock for dt_host instead of lock for each node under dt_host.
>>>> ---
>>>> ---
>>>>    xen/common/device_tree.c              |  5 +++++
>>>>    xen/drivers/passthrough/device_tree.c | 15 +++++++++++++++
>>>>    xen/include/xen/device_tree.h         |  6 ++++++
>>>>    3 files changed, 26 insertions(+)
>>>
>>> I am not sue where to put the comment. I noticed that you didn't touch
>>> iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the
>>> caller is expected to held the lock? If so, then this should be documented
>>> and an ASSERT() should be added.
>> Added ASSERT in iommu_(add,remove,assign and deassign)_dt_device(),
> iommu_add_ and iommu_assign_ are called at boot time. Also, only other callers
> are handle_device via overlays and iommu_do_dt_domctl() which will hold the
> dt_host_lock. 

The goal of the ASSERT() is to confirm that this holds true today and in 
the future.

> Will look into it more but for now sending v10 with ASSER in these
> two functions.
You could have at least written a comment on top... Regarding the boot 
function, I would consider to take the lock there too.

Otherwise, you could use:

ASSERT(system_state <= SYS_STATE_active || check lock);

Cheers,

-- 
Julien Grall