Re: [PATCH v5 5/7] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains

2024-05-24 Thread Stefano Stabellini
On Fri, 24 May 2024, Julien Grall wrote:
> Hi Stefano,
> 
> On 24/05/2024 03:18, Stefano Stabellini wrote:
> > From: Henry Wang 
> > 
> > In order to support the dynamic dtbo device assignment to a running
> > VM, the add/remove of the DT overlay and the attach/detach of the
> > device from the DT overlay should happen separately. Therefore,
> > repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
> > overlay to Xen device tree, instead of assigning the device to the
> > hardware domain at the same time. It is OK to change the sysctl behavior
> > as this feature is experimental so changing sysctl behavior and breaking
> > compatibility is OK.
> > 
> > Add the XEN_DOMCTL_dt_overlay with operations
> > XEN_DOMCTL_DT_OVERLAY_ATTACH to do the device assignment to the domain.
> > 
> > The hypervisor firstly checks the DT overlay passed from the toolstack
> > is valid. Then the device nodes are retrieved from the overlay tracker
> > based on the DT overlay. The attach of the device is implemented by
> > mapping the IRQ and IOMMU resources. All devices in the overlay are
> > assigned to a single domain.
> > 
> > Also take the opportunity to make one coding style fix in sysctl.h.
> > 
> > xen,reg is to be used to handle non-1:1 mappings but it is currently
> > unsupported.
> 
> This means that we would still try to use 1:1 mappings for non-directmap
> domain. Given that the overlay is a blob, I am a bit concerned that the user
> may not notice any clash and it would be difficult to debug.
> 
> Therefore, I would like xen,reg to be mandatory when using non directmapped
> domain. For now, the best approach would be to prevent device assignment if
> !is_domain_direct_mapped().

That's fine, I'll make the change


> > +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 )
> > +return -EOPNOTSUPP;
> > +
> > +if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
> 
> Please add #define DT_OVERLAY_MAX_SIZE KB(500) and use it here and the other
> place.

OK



Re: [PATCH v5 5/7] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains

2024-05-24 Thread Julien Grall

Hi Stefano,

On 24/05/2024 03:18, Stefano Stabellini wrote:

From: Henry Wang 

In order to support the dynamic dtbo device assignment to a running
VM, the add/remove of the DT overlay and the attach/detach of the
device from the DT overlay should happen separately. Therefore,
repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
overlay to Xen device tree, instead of assigning the device to the
hardware domain at the same time. It is OK to change the sysctl behavior
as this feature is experimental so changing sysctl behavior and breaking
compatibility is OK.

Add the XEN_DOMCTL_dt_overlay with operations
XEN_DOMCTL_DT_OVERLAY_ATTACH to do the device assignment to the domain.

The hypervisor firstly checks the DT overlay passed from the toolstack
is valid. Then the device nodes are retrieved from the overlay tracker
based on the DT overlay. The attach of the device is implemented by
mapping the IRQ and IOMMU resources. All devices in the overlay are
assigned to a single domain.

Also take the opportunity to make one coding style fix in sysctl.h.

xen,reg is to be used to handle non-1:1 mappings but it is currently
unsupported.


This means that we would still try to use 1:1 mappings for non-directmap 
domain. Given that the overlay is a blob, I am a bit concerned that the 
user may not notice any clash and it would be difficult to debug.


Therefore, I would like xen,reg to be mandatory when using non 
directmapped domain. For now, the best approach would be to prevent 
device assignment if !is_domain_direct_mapped().



+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 )
+return -EOPNOTSUPP;
+
+if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )


Please add #define DT_OVERLAY_MAX_SIZE KB(500) and use it here and the 
other place.


Cheers,

--
Julien Grall



[PATCH v5 5/7] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains

2024-05-23 Thread Stefano Stabellini
From: Henry Wang 

In order to support the dynamic dtbo device assignment to a running
VM, the add/remove of the DT overlay and the attach/detach of the
device from the DT overlay should happen separately. Therefore,
repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
overlay to Xen device tree, instead of assigning the device to the
hardware domain at the same time. It is OK to change the sysctl behavior
as this feature is experimental so changing sysctl behavior and breaking
compatibility is OK.

Add the XEN_DOMCTL_dt_overlay with operations
XEN_DOMCTL_DT_OVERLAY_ATTACH to do the device assignment to the domain.

The hypervisor firstly checks the DT overlay passed from the toolstack
is valid. Then the device nodes are retrieved from the overlay tracker
based on the DT overlay. The attach of the device is implemented by
mapping the IRQ and IOMMU resources. All devices in the overlay are
assigned to a single domain.

Also take the opportunity to make one coding style fix in sysctl.h.

xen,reg is to be used to handle non-1:1 mappings but it is currently
unsupported.

Signed-off-by: Henry Wang 
Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
---
 xen/arch/arm/domctl.c|   3 +
 xen/common/dt-overlay.c  | 207 ++-
 xen/include/public/domctl.h  |  16 ++-
 xen/include/public/sysctl.h  |  11 +-
 xen/include/xen/dt-overlay.h |   8 ++
 5 files changed, 186 insertions(+), 59 deletions(-)

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index ad56efb0f5..12a12ee781 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2012, Citrix Systems
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -176,6 +177,8 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
domain *d,
 
 return rc;
 }
+case XEN_DOMCTL_dt_overlay:
+return dt_overlay_domctl(d, >u.dt_overlay);
 default:
 return subarch_do_domctl(domctl, d, u_domctl);
 }
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 9cece79067..c2b03865a7 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -356,6 +356,42 @@ static int overlay_get_nodes_info(const void *fdto, char 
**nodes_full_path)
 return 0;
 }
 
+/* This function should be called with the overlay_lock taken */
+static struct overlay_track *
+find_track_entry_from_tracker(const void *overlay_fdt,
+  uint32_t overlay_fdt_size)
+{
+struct overlay_track *entry, *temp;
+bool found_entry = false;
+
+ASSERT(spin_is_locked(_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, _tracker, entry )
+{
+if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
+{
+found_entry = true;
+break;
+}
+}
+
+if ( !found_entry )
+{
+printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
+   " Operation is supported only for prior added dtbo.\n");
+return NULL;
+}
+
+return entry;
+}
+
 /* Check if node itself can be removed and remove node from IOMMU. */
 static int remove_node_resources(struct dt_device_node *device_node)
 {
@@ -485,8 +521,7 @@ 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;
+struct overlay_track *entry;
 
 rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
 if ( rc )
@@ -494,29 +529,10 @@ static long handle_remove_overlay_nodes(const void 
*overlay_fdt,
 
 spin_lock(_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, _tracker, entry )
-{
-if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
-{
-track = entry;
-found_entry = true;
-break;
-}
-}
-
-if ( !found_entry )
+entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
+if ( entry == NULL )
 {
 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;
 
 }
@@