Hi Julien,
Thanks for spending time on the review!
On 5/19/2024 6:17 PM, Julien Grall wrote:
Hi Henry,
On 16/05/2024 11:03, Henry Wang wrote:
diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
index bbd955e9c2..61f9082553 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -260,6 +260,19 @@ with the following properties:
value specified by Xen command line parameter
gnttab_max_maptrack_frames
(or its default value if unspecified, i.e. 1024) is used.
+- passthrough
+
+ A string property specifying whether IOMMU mappings are enabled
for the
+ domain and hence whether it will be enabled for passthrough
hardware.
+ Possible property values are:
+
+ - "enabled"
+ IOMMU mappings are enabled for the domain.
+
+ - "disabled"
+ IOMMU mappings are disabled for the domain and so hardware may
not be
+ passed through. This option is the default if this property is
missing.
Looking at the code below, it seems like the default will depend on
whether the partial device-tree is present. Did I misunderstand?
I am not sure if I understand the "partial device tree" in above comment
correctly. The "passthrough" property is supposed to be placed in the
dom0less domU domain node exactly the same way as the other dom0less
domU properties (such as "direct-map" etc.). This way we can control the
XEN_DOMCTL_CDF_iommu is set or not for each dom0less domU separately.
+
Under the "xen,domain" compatible node, one or more sub-nodes are
present
for the DomU kernel and ramdisk.
diff --git a/xen/arch/arm/dom0less-build.c
b/xen/arch/arm/dom0less-build.c
index 74f053c242..1396a102e1 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -848,6 +848,7 @@ static int __init construct_domU(struct domain *d,
void __init create_domUs(void)
{
struct dt_device_node *node;
+ const char *dom0less_iommu;
const struct dt_device_node *cpupool_node,
*chosen =
dt_find_node_by_path("/chosen");
@@ -895,8 +896,10 @@ void __init create_domUs(void)
panic("Missing property 'cpus' for domain %s\n",
dt_node_name(node));
- if ( dt_find_compatible_node(node, NULL,
"multiboot,device-tree") &&
- iommu_enabled )
+ if ( iommu_enabled &&
+ ((!dt_property_read_string(node, "passthrough",
&dom0less_iommu) &&
+ !strcmp(dom0less_iommu, "enabled")) ||
+ dt_find_compatible_node(node, NULL,
"multiboot,device-tree")) )
This condition is getting a little bit harder to read. Can we cache
the "passthrough" flag separately?
Yes sure. Will do this in v3.
Also, shouldn't we throw a panic if passthrough = "enabled" but the
IOMMU is enabled?
I take the above "enabled" should be "disabled"? Actually we already
have several checks to do that: Firstly, the above if condition checks
the "iommu_enabled", so if IOMMU is disabled, the XEN_DOMCTL_CDF_iommu
is never set. Also, in later on domain config sanitising process, i.e.
domain_create() -> sanitise_domain_config(), there is also a check and
panic to check if XEN_DOMCTL_CDF_iommu is somehow set but IOMMU is
disabled. So I think these are sufficient for us. Did I understand your
comment correctly?
Kind regards,
Henry
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
if ( !dt_property_read_u32(node, "nr_spis",
&d_cfg.arch.nr_spis) )
Cheers,
