On 12.06.25 14:42, Oleksii Moisieiev wrote:
Hi Stefano,
I'm very sorry for a long silence. Please see my answers below:
On 22/05/2025 03:25, Stefano Stabellini wrote:
On Mon, 19 May 2025, Oleksii Moisieiev wrote:
From: Grygorii Strashko<grygorii_stras...@epam.com>
Add chained handling of assigned DT devices to support access-controller
functionality through SCI framework, so DT device assign request can be
passed to FW for processing and enabling VM access to requested device
(for example, device power management through FW interface like SCMI).
The SCI access-controller DT device processing is chained after IOMMU
processing and expected to be executed for any DT device regardless of its
protection by IOMMU (or if IOMMU is disabled).
This allows to pass not only IOMMU protected DT device through
xl.cfg:"dtdev" property for processing:
dtdev = [
"/soc/video@e6ef0000", <- IOMMU protected device
"/soc/i2c@e6508000", <- not IOMMU protected device
]
The change is done in two parts:
1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
not fail if DT device is not protected by IOMMU
2) add chained call to sci_do_domctl() in do_domctl()
Signed-off-by: Grygorii Strashko<grygorii_stras...@epam.com>
Signed-off-by: Oleksii Moisieiev<oleksii_moisie...@epam.com>
---
xen/arch/arm/firmware/sci.c | 37 +++++++++++++++++++++++++
xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
xen/common/domctl.c | 19 +++++++++++++
xen/drivers/passthrough/device_tree.c | 6 ++++
4 files changed, 76 insertions(+)
diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
index e1522e10e2..8efd541c4f 100644
--- a/xen/arch/arm/firmware/sci.c
+++ b/xen/arch/arm/firmware/sci.c
@@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct
dt_device_node *dev)
return 0;
}
+int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
+ XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+ struct dt_device_node *dev;
+ int ret = 0;
+
+ switch ( domctl->cmd )
+ {
+ case XEN_DOMCTL_assign_device:
+ ret = -EOPNOTSUPP;
Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
The -EOPNOTSUPP code is used because this is part of a chained call after
iommu_do_domctl, as stated in xen/common/domctl.c:859. The
XEN_DOMCTL_assign_device
call is expected to handle any DT device, regardless of whether the DT
device is
protected by an IOMMU or if the IOMMU is disabled.
The following cases are considered:
1. IOMMU Protected Device (Success)
If the device is protected by the IOMMU and iommu_do_domctl returns 0,
we continue
processing the DT device by calling sci_do_domctl.
2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
disabled,
we still proceed to call sci_do_domctl.
3. Error from iommu_do_domctl (Fail State)
If iommu_do_domctl returns any error, the system enters a fail state, and
sci_do_domctl is not called.
4. -EOPNOTSUPP from sci_do_domctl
If sci_do_domctl returns -EOPNOTSUPP, this indicates one of the following:
- The provided device is not a DT device.
- There is no cur_mediator available (indicating that the SCI subsystem
is enabled
in the configuration, but no mediator was provided).
- The current mediator does not support assign_dt_device (this is
expected to be changed;
see below for details).
In this case, -EOPNOTSUPP is returned but will be ignored, and the
original return value from iommu_do_domctl will be used as the final result.
5. Return Code from sci_do_domctl
If sci_do_domctl returns 0 (success) or an error code (failure),
the return value from iommu_do_domctl is overridden, and the result from
sci_do_domctl is returned.
Note: -EOPNOTSUPP from iommu_do_domctl will also be overridden since
step 2 was successfully completed (or failed).
+ if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
+ break;
this one
+ if ( !cur_mediator )
+ break;
this one
+ if ( !cur_mediator->assign_dt_device )
+ break;
and also this one? It seems more like an -EINVAL as the caller used a
wrong parameter?
I think you are right that this case should return -EINVAL because we
should fail if mediator
without implemented mandatory features was provided. Will be fixed.
+ ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
+ domctl->u.assign_device.u.dt.size, &dev);
+ if ( ret )
+ return ret;
+
+ ret = sci_assign_dt_device(d, dev);
+ if ( ret )
+ break;
+
+ break;
+ default:
+ /* do not fail here as call is chained with iommu handling */
It looks like this should be an error
+ break;
+ }
+
+ return ret;
+}
+
static int __init sci_init(void)
{
struct dt_device_node *np;
diff --git a/xen/arch/arm/include/asm/firmware/sci.h
b/xen/arch/arm/include/asm/firmware/sci.h
index 71fb54852e..b8d1bc8a62 100644
--- a/xen/arch/arm/include/asm/firmware/sci.h
+++ b/xen/arch/arm/include/asm/firmware/sci.h
@@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
* control" functionality.
*/
int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
+
+/*
+ * SCI domctl handler
+ *
+ * Only XEN_DOMCTL_assign_device is handled for now.
+ */
+int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
+ XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
#else
static inline bool sci_domain_is_enabled(struct domain *d)
@@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
return 0;
}
+static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
+ XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+ return 0;
+}
+
#endif /* CONFIG_ARM_SCI */
#endif /* __ASM_ARM_SCI_H */
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 05abb581a0..a74ee92067 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -27,6 +27,7 @@
#include <xen/vm_event.h>
#include <xen/monitor.h>
#include <asm/current.h>
+#include <asm/firmware/sci.h>
#include <asm/irq.h>
#include <asm/page.h>
#include <asm/p2m.h>
@@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
case XEN_DOMCTL_deassign_device:
case XEN_DOMCTL_get_device_group:
ret = iommu_do_domctl(op, d, u_domctl);
+
+ if ( !ret || ret == -EOPNOTSUPP )
It is better to invert the check:
if ( ret < 0 && ret != -EOPNOTSUPP )
return ret;
+
+ {
+ int ret1;
+ /*
+ * Add chained handling of assigned DT devices to support
+ * access-controller functionality through SCI framework, so
+ * DT device assign request can be passed to FW for processing and
+ * enabling VM access to requested device.
+ * The access-controller DT device processing is chained after
IOMMU
+ * processing and expected to be executed for any DT device
+ * regardless if DT device is protected by IOMMU or not (or IOMMU
+ * is disabled).
+ */
+ ret1 = sci_do_domctl(op, d, u_domctl);
+ if ( ret1 != -EOPNOTSUPP )
+ ret = ret1;
+ }
break;
case XEN_DOMCTL_get_paging_mempool_size:
diff --git a/xen/drivers/passthrough/device_tree.c
b/xen/drivers/passthrough/device_tree.c
index 075fb25a37..2624767e51 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
domain *d,
break;
}
+ if ( !dt_device_is_protected(dev) )
+ {
+ ret = 0;
+ break;
+ }
I am concerned about this: previously we would call
iommu_assign_dt_device and the same check at the beginning of
iommu_assign_dt_device would return -EINVAL. Now it is a success.
I am not sure this is appropriate. I wonder if instead we should:
- remove this chunk from the patch
- change the return error for !dt_device_is_protected at the top of
iommu_assign_dt_device from -EINVAL to -EOPNOTSUPP
- this would fall into the same ret != -EOPNOTSUPP check after
iommu_do_domctl
We need to be careful here :(
The reason for this change, in it's current form is to make this part of
the code to work the same way as other parts [1] and [2] where IOMMU is
configured (yep, and get what we need):
[1] xen/arch/arm/device.c handle_device()
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/device.c;h=5e1c1cc326ac04d30e7158905fead9b41c719fc0;hb=HEAD#l287
which is used to create hwdom(dom0) or apply overlays and called for every "own" node
(!"xen,passthrough").
The "own" node may not be IOMMU protected.
[2] xen/common/device-tree/dom0less-build.c handle_passthrough_prop()
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/device-tree/dom0less-build.c;h=3d503c697337a4cc6df35403b9a2595b40089dae;hb=HEAD#l224
which is used for "dom0less" devices pass-through (arm/ppc) and called for every node
with "xen,path" property,
but this path is intended to perform not only IOMMU, but also IRQs
configuration.
The DT node, pointed by "xen,path", may not be IOMMU protected.
In both above cases call pattern is:
res = iommu_add_dt_device(node);
if ( res < 0 )
return res;
if ( !dt_device_is_protected(node) )
return 0; //no failure
res = iommu_assign_dt_device(domain, node);
if ( res )
return res;
And only when XEN_DOMCTL_assign_device (iommu_do_dt_domctl()) is handled the
call pattern is
different and there is no check for dt_device_is_protected().
That's a good point. I think we should do the same for
> if ( !is_iommu_enabled(d) )
> return -EINVAL;
because in this case we should process sci as well. I will do the change
--
Best regards,
-grygorii
