diff mbox series

[v4,7/9] xen/arm: Support device detachment from domains

Message ID 20240523074040.1611264-8-xin.wang2@amd.com (mailing list archive)
State New, archived
Headers show
Series Remaining patches for dynamic node programming using overlay dtbo | expand

Commit Message

Henry Wang May 23, 2024, 7:40 a.m. UTC 
Similarly as the device attachment from DT overlay to domain, this
commit implements the device detachment from domain. The DOMCTL
XEN_DOMCTL_dt_overlay op is extended to have the operation
XEN_DOMCTL_DT_OVERLAY_DETACH. The detachment of the device is
implemented by unmapping the IRQ and IOMMU resources. Note that with
these changes, the device de-registration from the IOMMU driver should
only happen at the time when the DT overlay is removed from the Xen
device tree.

Signed-off-by: Henry Wang <xin.wang2@amd.com>
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
v4:
- Split the original patch, only do device detachment from domain.
---
 xen/common/dt-overlay.c     | 243 ++++++++++++++++++++++++++++--------
 xen/include/public/domctl.h |   3 +-
 2 files changed, 194 insertions(+), 52 deletions(-)

Comments

Julien Grall May 23, 2024, 10:42 p.m. UTC | #1 
Hi Henry,

On 23/05/2024 08:40, Henry Wang wrote:
> Similarly as the device attachment from DT overlay to domain, this
> commit implements the device detachment from domain. The DOMCTL
> XEN_DOMCTL_dt_overlay op is extended to have the operation
> XEN_DOMCTL_DT_OVERLAY_DETACH. The detachment of the device is
> implemented by unmapping the IRQ and IOMMU resources. Note that with
> these changes, the device de-registration from the IOMMU driver should
> only happen at the time when the DT overlay is removed from the Xen
> device tree.
> 
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> v4:
> - Split the original patch, only do device detachment from domain.
> ---
>   xen/common/dt-overlay.c     | 243 ++++++++++++++++++++++++++++--------
>   xen/include/public/domctl.h |   3 +-
>   2 files changed, 194 insertions(+), 52 deletions(-)
> 
> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
> index 1087f9b502..693b6e4777 100644
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -392,24 +392,100 @@ find_track_entry_from_tracker(const void *overlay_fdt,
>       return entry;
>   }
>   
> +static int remove_irq(unsigned long s, unsigned long e, void *data)
> +{
> +    struct domain *d = data;
> +    int rc = 0;
> +
> +    /*
> +     * IRQ should always have access unless there are duplication of
> +     * of irqs in device tree. There are few cases of xen device tree
> +     * where there are duplicate interrupts for the same node.
> +     */
> +    if (!irq_access_permitted(d, s))

Because of this check, it means that ...

> +        return 0;
> +    /*
> +     * TODO: We don't handle shared IRQs for now. So, it is assumed that
> +     * the IRQs was not shared with another domain.
> +     */
> +    rc = irq_deny_access(d, s);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "unable to revoke access for irq %ld\n", s);
> +        return rc;
> +    }
> +
> +    rc = release_guest_irq(d, s);

... release_guest_irq() fails on the next retry it will pass. I don't 
think this is what we want.

Instead, we probably want to re-order the call.

> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "unable to release irq %ld\n", s);
> +        return rc;
> +    }
> +
> +    return rc;
> +}
> +
> +static int remove_all_irqs(struct rangeset *irq_ranges, struct domain *d)
> +{
> +    return rangeset_report_ranges(irq_ranges, 0, ~0UL, remove_irq, d);
> +}
> +
> +static int remove_iomem(unsigned long s, unsigned long e, void *data)
> +{
> +    struct domain *d = data;
> +    int rc = 0;
> +    p2m_type_t t;
> +    mfn_t mfn;
> +
> +    mfn = p2m_lookup(d, _gfn(s), &t);

What are you trying to addres with this check? For instance, the fact 
that the first MFN is mapped, doesn't guarantee the rest is.

> +    if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )

I don't understand why we are checking for 0 here. In theory, it is 
valid MFN. Also, the second part wants to be INVALID_MFN.

> +        return -EINVAL;
> +
> +    rc = iomem_deny_access(d, s, e);

iomem_deny_access() works on MFN but here you pass an MFN. Are you 
assuming the GFN == MFN? How would that work for domains that are not 
direct mapped?

> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "Unable to remove %pd access to %#lx - %#lx\n",
> +               d, s, e);
> +        return rc;
> +    }
> +
> +    rc = unmap_mmio_regions(d, _gfn(s), e - s, _mfn(s));
> +    if ( rc )
> +        return rc;
> +
> +    return rc;
> +}
> +
> +static int remove_all_iomems(struct rangeset *iomem_ranges, struct domain *d)
> +{
> +    return rangeset_report_ranges(iomem_ranges, 0, ~0UL, remove_iomem, d);
> +}
> +
>   /* Check if node itself can be removed and remove node from IOMMU. */
> -static int remove_node_resources(struct dt_device_node *device_node)
> +static int remove_node_resources(struct dt_device_node *device_node,
> +                                 struct domain *d)
>   {
>       int rc = 0;
>       unsigned int len;
>       domid_t domid;
>   
> -    domid = dt_device_used_by(device_node);
> +    if ( !d )

I looked at the code, I am a bit unsure how "d" can be NULL. Do you have 
any pointer?

> +    {
> +        domid = dt_device_used_by(device_node);
>   
> -    dt_dprintk("Checking if node %s is used by any domain\n",
> -               device_node->full_name);
> +        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;
> +        /*
> +         * We also check if device is assigned to DOMID_IO as when a domain
> +         * is destroyed device is assigned to DOMID_IO.
> +         */
> +        if ( domid != DOMID_IO )
> +        {
> +            printk(XENLOG_ERR "Device %s is being assigned to %u. Device is assigned to %d\n",
> +                   device_node->full_name, DOMID_IO, domid);
> +            return -EINVAL;
> +        }
>       }
>   
>       /* Check if iommu property exists. */
> @@ -417,9 +493,12 @@ static int remove_node_resources(struct dt_device_node *device_node)
>       {
>           if ( dt_device_is_protected(device_node) )
>           {
> -            rc = iommu_remove_dt_device(device_node);
> -            if ( rc < 0 )
> -                return rc;
> +            if ( !list_empty(&device_node->domain_list) )
> +            {
> +                rc = iommu_deassign_dt_device(d, device_node);
> +                if ( rc < 0 )
> +                    return rc;
> +            }
>           }
>       }
>   
> @@ -428,7 +507,8 @@ static int remove_node_resources(struct dt_device_node *device_node)
>   
>   /* Remove all descendants from IOMMU. */
>   static int
> -remove_descendant_nodes_resources(const struct dt_device_node *device_node)
> +remove_descendant_nodes_resources(const struct dt_device_node *device_node,
> +                                  struct domain *d)
>   {
>       int rc = 0;
>       struct dt_device_node *child_node;
> @@ -438,12 +518,12 @@ remove_descendant_nodes_resources(const struct dt_device_node *device_node)
>       {
>           if ( child_node->child )
>           {
> -            rc = remove_descendant_nodes_resources(child_node);
> +            rc = remove_descendant_nodes_resources(child_node, d);
>               if ( rc )
>                   return rc;
>           }
>   
> -        rc = remove_node_resources(child_node);
> +        rc = remove_node_resources(child_node, d);
>           if ( rc )
>               return rc;
>       }
> @@ -456,8 +536,7 @@ 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;
> +    unsigned int j, len;
>   
>       for ( j = 0; j < tracker->num_nodes; j++ )
>       {
> @@ -467,18 +546,15 @@ static int remove_nodes(const struct overlay_track *tracker)
>   
>           write_lock(&dt_host_lock);
>   
> -        rc = remove_descendant_nodes_resources(overlay_node);
> -        if ( rc )
> +        /* Check if iommu property exists. */
> +        if ( dt_get_property(overlay_node, "iommus", &len) )

Why do we need to check for the property in the "iommus" rather than ...

>           {
> -            write_unlock(&dt_host_lock);
> -            return rc;
> -        }
> -
> -        rc = remove_node_resources(overlay_node);
> -        if ( rc )
> -        {
> -            write_unlock(&dt_host_lock);
> -            return rc;
> +            if ( dt_device_is_protected(overlay_node) )

... relying on dt_device_is_protected()?

> +            {
> +                rc = iommu_remove_dt_device(overlay_node);
> +                if ( rc < 0 )
> +                    return rc;
> +            }
>           }
>   
>           dt_dprintk("Removing node: %s\n", overlay_node->full_name);
> @@ -493,22 +569,6 @@ static int remove_nodes(const struct overlay_track *tracker)
>           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;
>   }
>   
> @@ -534,7 +594,6 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
>       {
>           rc = -EINVAL;
>           goto out;
> -

Valid but unrelated change?

>       }
>   
>       rc = remove_nodes(entry);
> @@ -552,9 +611,6 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
>   
>       xfree(entry->nodes_address);
>   
> -    rangeset_destroy(entry->irq_ranges);
> -    rangeset_destroy(entry->iomem_ranges);
> -
>       xfree(entry);
>   
>    out:
> @@ -840,6 +896,88 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
>       return rc;
>   }
>   
> +static long handle_detach_overlay_nodes(struct domain *d,
> +                                        const void *overlay_fdt,
> +                                        uint32_t overlay_fdt_size)
> +{
> +    int rc;
> +    unsigned int j;
> +    struct overlay_track *entry;
> +
> +    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> +    if ( rc )
> +        return rc;
> +
> +    spin_lock(&overlay_lock);
> +
> +    entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
> +    if ( entry == NULL )
> +    {
> +        rc = -EINVAL;
> +        goto out;
> +    }
> +
> +    for ( j = 0; j < entry->num_nodes; j++ )
> +    {
> +        struct dt_device_node *overlay_node;
> +
> +        overlay_node = (struct dt_device_node *)entry->nodes_address[j];

Can you remind me why nodes_address is storing unsigned long rather 
struct dt_device_node?

> +        if ( overlay_node == NULL )
> +        {
> +            rc = -EINVAL;
> +            goto out;
> +        }
> +
> +        write_lock(&dt_host_lock);
> +        rc = remove_descendant_nodes_resources(overlay_node, d);
> +        if ( rc )
> +        {
> +            write_unlock(&dt_host_lock);
> +            goto out;
> +        }
> +
> +        rc = remove_node_resources(overlay_node, d);
> +        if ( rc )
> +        {
> +            write_unlock(&dt_host_lock);
> +            goto out;
> +        }
> +        write_unlock(&dt_host_lock);
> +
> +        rc = remove_all_irqs(entry->irq_ranges, d);

I am really confused. This is called in the for loop but entry doesn't 
seem to change. So why do we need to call it for every iteration? Also...


> +        if ( rc )
> +            goto out;
> +
> +        rc = remove_all_iomems(entry->iomem_ranges, d);
> +        if ( rc )
> +            goto out;
> +    }
> +
> +    /* Remove IRQ access. */
> +    if ( entry->irq_ranges )
> +    {
> +        rc = rangeset_consume_ranges(entry->irq_ranges, irq_remove_cb, d);

... remove_all_irqs() will revert the permission. So why do we need to 
do it again?

> +        if ( rc )
> +            goto out;
> +    }
> +
> +    /* Remove mmio access. */
> +    if ( entry->iomem_ranges )
> +    {
> +        rc = rangeset_consume_ranges(entry->iomem_ranges, iomem_remove_cb, d);

Same question here.

> +        if ( rc )
> +            goto out;
> +    }
> +
> +    rangeset_destroy(entry->irq_ranges);
> +    rangeset_destroy(entry->iomem_ranges);
> +
> + out:
> +    spin_unlock(&overlay_lock);
> +
> +    return rc;
> +}
> +
>   static long handle_attach_overlay_nodes(struct domain *d,
>                                           const void *overlay_fdt,
>                                           uint32_t overlay_fdt_size)
> @@ -956,7 +1094,8 @@ long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
>       long ret;
>       void *overlay_fdt;
>   
> -    if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH )
> +    if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH &&
> +         op->overlay_op != XEN_DOMCTL_DT_OVERLAY_DETACH )
>           return -EOPNOTSUPP;
>   
>       if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
> @@ -979,7 +1118,9 @@ long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
>           return -EFAULT;
>       }
>   
> -    if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_ATTACH )
> +    if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_DETACH )
> +        ret = handle_detach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
> +    else
>           ret = handle_attach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
>   
>       xfree(overlay_fdt);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index ac3c2a7c4c..e91f743fa4 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1195,7 +1195,8 @@ struct xen_domctl_dt_overlay {
>       XEN_GUEST_HANDLE_64(const_void) overlay_fdt;  /* IN: overlay fdt. */
>       uint32_t overlay_fdt_size;              /* IN: Overlay dtb size. */
>   #define XEN_DOMCTL_DT_OVERLAY_ATTACH                1
> -    uint8_t overlay_op;                     /* IN: Attach. */
> +#define XEN_DOMCTL_DT_OVERLAY_DETACH                2
> +    uint8_t overlay_op;                     /* IN: Attach/Detach. */

Changing the comment for every operation we add is not really scalable 
:). If you want an explanation, can it be generic?

>       uint8_t pad[3];                         /* IN: Must be zero. */
>   };
>   #endif

Cheers,
diff mbox series

Patch

diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 1087f9b502..693b6e4777 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -392,24 +392,100 @@  find_track_entry_from_tracker(const void *overlay_fdt,
     return entry;
 }
 
+static int remove_irq(unsigned long s, unsigned long e, void *data)
+{
+    struct domain *d = data;
+    int rc = 0;
+
+    /*
+     * IRQ should always have access unless there are duplication of
+     * of irqs in device tree. There are few cases of xen device tree
+     * where there are duplicate interrupts for the same node.
+     */
+    if (!irq_access_permitted(d, s))
+        return 0;
+    /*
+     * TODO: We don't handle shared IRQs for now. So, it is assumed that
+     * the IRQs was not shared with another domain.
+     */
+    rc = irq_deny_access(d, s);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "unable to revoke access for irq %ld\n", s);
+        return rc;
+    }
+
+    rc = release_guest_irq(d, s);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "unable to release irq %ld\n", s);
+        return rc;
+    }
+
+    return rc;
+}
+
+static int remove_all_irqs(struct rangeset *irq_ranges, struct domain *d)
+{
+    return rangeset_report_ranges(irq_ranges, 0, ~0UL, remove_irq, d);
+}
+
+static int remove_iomem(unsigned long s, unsigned long e, void *data)
+{
+    struct domain *d = data;
+    int rc = 0;
+    p2m_type_t t;
+    mfn_t mfn;
+
+    mfn = p2m_lookup(d, _gfn(s), &t);
+    if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )
+        return -EINVAL;
+
+    rc = iomem_deny_access(d, s, e);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Unable to remove %pd access to %#lx - %#lx\n",
+               d, s, e);
+        return rc;
+    }
+
+    rc = unmap_mmio_regions(d, _gfn(s), e - s, _mfn(s));
+    if ( rc )
+        return rc;
+
+    return rc;
+}
+
+static int remove_all_iomems(struct rangeset *iomem_ranges, struct domain *d)
+{
+    return rangeset_report_ranges(iomem_ranges, 0, ~0UL, remove_iomem, d);
+}
+
 /* Check if node itself can be removed and remove node from IOMMU. */
-static int remove_node_resources(struct dt_device_node *device_node)
+static int remove_node_resources(struct dt_device_node *device_node,
+                                 struct domain *d)
 {
     int rc = 0;
     unsigned int len;
     domid_t domid;
 
-    domid = dt_device_used_by(device_node);
+    if ( !d )
+    {
+        domid = dt_device_used_by(device_node);
 
-    dt_dprintk("Checking if node %s is used by any domain\n",
-               device_node->full_name);
+        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;
+        /*
+         * We also check if device is assigned to DOMID_IO as when a domain
+         * is destroyed device is assigned to DOMID_IO.
+         */
+        if ( domid != DOMID_IO )
+        {
+            printk(XENLOG_ERR "Device %s is being assigned to %u. Device is assigned to %d\n",
+                   device_node->full_name, DOMID_IO, domid);
+            return -EINVAL;
+        }
     }
 
     /* Check if iommu property exists. */
@@ -417,9 +493,12 @@  static int remove_node_resources(struct dt_device_node *device_node)
     {
         if ( dt_device_is_protected(device_node) )
         {
-            rc = iommu_remove_dt_device(device_node);
-            if ( rc < 0 )
-                return rc;
+            if ( !list_empty(&device_node->domain_list) )
+            {
+                rc = iommu_deassign_dt_device(d, device_node);
+                if ( rc < 0 )
+                    return rc;
+            }
         }
     }
 
@@ -428,7 +507,8 @@  static int remove_node_resources(struct dt_device_node *device_node)
 
 /* Remove all descendants from IOMMU. */
 static int
-remove_descendant_nodes_resources(const struct dt_device_node *device_node)
+remove_descendant_nodes_resources(const struct dt_device_node *device_node,
+                                  struct domain *d)
 {
     int rc = 0;
     struct dt_device_node *child_node;
@@ -438,12 +518,12 @@  remove_descendant_nodes_resources(const struct dt_device_node *device_node)
     {
         if ( child_node->child )
         {
-            rc = remove_descendant_nodes_resources(child_node);
+            rc = remove_descendant_nodes_resources(child_node, d);
             if ( rc )
                 return rc;
         }
 
-        rc = remove_node_resources(child_node);
+        rc = remove_node_resources(child_node, d);
         if ( rc )
             return rc;
     }
@@ -456,8 +536,7 @@  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;
+    unsigned int j, len;
 
     for ( j = 0; j < tracker->num_nodes; j++ )
     {
@@ -467,18 +546,15 @@  static int remove_nodes(const struct overlay_track *tracker)
 
         write_lock(&dt_host_lock);
 
-        rc = remove_descendant_nodes_resources(overlay_node);
-        if ( rc )
+        /* Check if iommu property exists. */
+        if ( dt_get_property(overlay_node, "iommus", &len) )
         {
-            write_unlock(&dt_host_lock);
-            return rc;
-        }
-
-        rc = remove_node_resources(overlay_node);
-        if ( rc )
-        {
-            write_unlock(&dt_host_lock);
-            return rc;
+            if ( dt_device_is_protected(overlay_node) )
+            {
+                rc = iommu_remove_dt_device(overlay_node);
+                if ( rc < 0 )
+                    return rc;
+            }
         }
 
         dt_dprintk("Removing node: %s\n", overlay_node->full_name);
@@ -493,22 +569,6 @@  static int remove_nodes(const struct overlay_track *tracker)
         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;
 }
 
@@ -534,7 +594,6 @@  static long handle_remove_overlay_nodes(const void *overlay_fdt,
     {
         rc = -EINVAL;
         goto out;
-
     }
 
     rc = remove_nodes(entry);
@@ -552,9 +611,6 @@  static long handle_remove_overlay_nodes(const void *overlay_fdt,
 
     xfree(entry->nodes_address);
 
-    rangeset_destroy(entry->irq_ranges);
-    rangeset_destroy(entry->iomem_ranges);
-
     xfree(entry);
 
  out:
@@ -840,6 +896,88 @@  static long handle_add_overlay_nodes(void *overlay_fdt,
     return rc;
 }
 
+static long handle_detach_overlay_nodes(struct domain *d,
+                                        const void *overlay_fdt,
+                                        uint32_t overlay_fdt_size)
+{
+    int rc;
+    unsigned int j;
+    struct overlay_track *entry;
+
+    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+    if ( rc )
+        return rc;
+
+    spin_lock(&overlay_lock);
+
+    entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
+    if ( entry == NULL )
+    {
+        rc = -EINVAL;
+        goto out;
+    }
+
+    for ( j = 0; j < entry->num_nodes; j++ )
+    {
+        struct dt_device_node *overlay_node;
+
+        overlay_node = (struct dt_device_node *)entry->nodes_address[j];
+        if ( overlay_node == NULL )
+        {
+            rc = -EINVAL;
+            goto out;
+        }
+
+        write_lock(&dt_host_lock);
+        rc = remove_descendant_nodes_resources(overlay_node, d);
+        if ( rc )
+        {
+            write_unlock(&dt_host_lock);
+            goto out;
+        }
+
+        rc = remove_node_resources(overlay_node, d);
+        if ( rc )
+        {
+            write_unlock(&dt_host_lock);
+            goto out;
+        }
+        write_unlock(&dt_host_lock);
+
+        rc = remove_all_irqs(entry->irq_ranges, d);
+        if ( rc )
+            goto out;
+
+        rc = remove_all_iomems(entry->iomem_ranges, d);
+        if ( rc )
+            goto out;
+    }
+
+    /* Remove IRQ access. */
+    if ( entry->irq_ranges )
+    {
+        rc = rangeset_consume_ranges(entry->irq_ranges, irq_remove_cb, d);
+        if ( rc )
+            goto out;
+    }
+
+    /* Remove mmio access. */
+    if ( entry->iomem_ranges )
+    {
+        rc = rangeset_consume_ranges(entry->iomem_ranges, iomem_remove_cb, d);
+        if ( rc )
+            goto out;
+    }
+
+    rangeset_destroy(entry->irq_ranges);
+    rangeset_destroy(entry->iomem_ranges);
+
+ out:
+    spin_unlock(&overlay_lock);
+
+    return rc;
+}
+
 static long handle_attach_overlay_nodes(struct domain *d,
                                         const void *overlay_fdt,
                                         uint32_t overlay_fdt_size)
@@ -956,7 +1094,8 @@  long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
     long ret;
     void *overlay_fdt;
 
-    if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH )
+    if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH &&
+         op->overlay_op != XEN_DOMCTL_DT_OVERLAY_DETACH )
         return -EOPNOTSUPP;
 
     if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
@@ -979,7 +1118,9 @@  long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
         return -EFAULT;
     }
 
-    if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_ATTACH )
+    if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_DETACH )
+        ret = handle_detach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
+    else
         ret = handle_attach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
 
     xfree(overlay_fdt);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ac3c2a7c4c..e91f743fa4 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1195,7 +1195,8 @@  struct xen_domctl_dt_overlay {
     XEN_GUEST_HANDLE_64(const_void) overlay_fdt;  /* IN: overlay fdt. */
     uint32_t overlay_fdt_size;              /* IN: Overlay dtb size. */
 #define XEN_DOMCTL_DT_OVERLAY_ATTACH                1
-    uint8_t overlay_op;                     /* IN: Attach. */
+#define XEN_DOMCTL_DT_OVERLAY_DETACH                2
+    uint8_t overlay_op;                     /* IN: Attach/Detach. */
     uint8_t pad[3];                         /* IN: Must be zero. */
 };
 #endif