| Message ID | 20240516100330.1433265-4-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remaining patches for dynamic node programming using overlay dtbo | expand |
Hi Henry, On 16/05/2024 11:03, Henry Wang wrote: > There are some use cases in which the dom0less domUs need to have > the XEN_DOMCTL_CDF_iommu set at the domain construction time. For > example, the dynamic dtbo feature allows the domain to be assigned > a device that is behind the IOMMU at runtime. For these use cases, > we need to have a way to specify the domain will need the IOMMU > mapping at domain construction time. > > Introduce a "passthrough" DT property for Dom0less DomUs following > the same entry as the xl.cfg. Currently only provide two options, > i.e. "enable" and "disable". Set the XEN_DOMCTL_CDF_iommu at domain > construction time based on the property. > > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > v2: > - New patch to replace the original patch in v1: > "[PATCH 03/15] xen/arm: Always enable IOMMU" > --- > docs/misc/arm/device-tree/booting.txt | 13 +++++++++++++ > xen/arch/arm/dom0less-build.c | 7 +++++-- > 2 files changed, 18 insertions(+), 2 deletions(-) > > 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? > + > 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? Also, shouldn't we throw a panic if passthrough = "enabled" but the IOMMU is enabled? > d_cfg.flags |= XEN_DOMCTL_CDF_iommu; > > if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) ) Cheers,
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, >
Hi Julien, On 5/20/2024 8:41 AM, Henry Wang wrote: > 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. Oh I think I get your points, you meant the XEN_DOMCTL_CDF_iommu will still be set if the passthrough dt property is "disabled", but user provides a partial device tree. Yes you are correct. I will update the doc to explain a bit more details as below. Thanks for pointing it out. - "enabled" IOMMU mappings are enabled for the domain. Note that this option is the default if the user provides the device partial passthrough device tree 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 and the user does not provide the device partial device tree for the domain. Kind regards, Henry
Hi Julien, On 5/20/2024 8:41 AM, Henry Wang wrote: > 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. Oh I think I get your points, you meant the XEN_DOMCTL_CDF_iommu will still be set if the passthrough dt property is "disabled", but user provides a partial device tree. Yes you are correct. I will update the doc to explain a bit more details as below. Thanks for pointing it out. - "enabled" IOMMU mappings are enabled for the domain. Note that this option is the default if the user provides the device partial passthrough device tree 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 and the user does not provide the device partial device tree for the domain. Kind regards, Henry
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. + 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")) ) d_cfg.flags |= XEN_DOMCTL_CDF_iommu; if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
There are some use cases in which the dom0less domUs need to have the XEN_DOMCTL_CDF_iommu set at the domain construction time. For example, the dynamic dtbo feature allows the domain to be assigned a device that is behind the IOMMU at runtime. For these use cases, we need to have a way to specify the domain will need the IOMMU mapping at domain construction time. Introduce a "passthrough" DT property for Dom0less DomUs following the same entry as the xl.cfg. Currently only provide two options, i.e. "enable" and "disable". Set the XEN_DOMCTL_CDF_iommu at domain construction time based on the property. Signed-off-by: Henry Wang <xin.wang2@amd.com> --- v2: - New patch to replace the original patch in v1: "[PATCH 03/15] xen/arm: Always enable IOMMU" --- docs/misc/arm/device-tree/booting.txt | 13 +++++++++++++ xen/arch/arm/dom0less-build.c | 7 +++++-- 2 files changed, 18 insertions(+), 2 deletions(-)