Re: [PATCH v4 7/9] xen/arm: Support device detachment from domains
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
Signed-off-by: Vikram Garhwal
---
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;
+}
}
[PATCH v4 7/9] xen/arm: Support device detachment from domains
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
Signed-off-by: Vikram Garhwal
---
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))
+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_nod
