* [PATCH 0/2] Dangling fixes for ARM iommu
@ 2019-01-21 17:04 Andrii Anisov
2019-01-21 17:04 ` [PATCH 1/2] iommu/arm: Misc fixes for arch specific part Andrii Anisov
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Andrii Anisov @ 2019-01-21 17:04 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Andrii Anisov
From: Andrii Anisov <andrii_anisov@epam.com>
Dear All,
With moving our working setup to 4.12-rc2 (IPMMU series especially),I've
realized that we have a couple of patches from IPMMU series [1] which are
fixes, got RB/AB, but not reached mainline yet. I really hope those RB/AB
would not be treated as stale, and patches are OK to go into 4.12 release.
Oleksandr Tyshchenko (2):
iommu/arm: Misc fixes for arch specific part
RB from Julien is here [2]
xen/arm: domain_build: Don't expose IOMMU specific properties to the
guest
AB from Julien is here [3]
[1] https://lists.xen.org/archives/html/xen-devel/2017-07/msg02545.html
[2] https://patchwork.kernel.org/patch/9862567/
[3] https://patchwork.kernel.org/patch/9862565/
xen/arch/arm/domain_build.c | 10 ++++++++++
xen/drivers/passthrough/arm/iommu.c | 7 +++++--
2 files changed, 15 insertions(+), 2 deletions(-)
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] iommu/arm: Misc fixes for arch specific part 2019-01-21 17:04 [PATCH 0/2] Dangling fixes for ARM iommu Andrii Anisov @ 2019-01-21 17:04 ` Andrii Anisov 2019-01-21 17:04 ` [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest Andrii Anisov 2019-01-21 17:29 ` [PATCH 0/2] Dangling fixes for ARM iommu Julien Grall 2 siblings, 0 replies; 18+ messages in thread From: Andrii Anisov @ 2019-01-21 17:04 UTC (permalink / raw) To: xen-devel; +Cc: Oleksandr Tyshchenko, Julien Grall, Stefano Stabellini From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> 1. Add missing return in case if IOMMU ops have been already set. 2. Add check for shared IOMMU before returning an error. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Reviewed-by: Julien Grall <julien.grall@arm.com> --- xen/drivers/passthrough/arm/iommu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c index 325997b..cbf9b82 100644 --- a/xen/drivers/passthrough/arm/iommu.c +++ b/xen/drivers/passthrough/arm/iommu.c @@ -32,7 +32,10 @@ void __init iommu_set_ops(const struct iommu_ops *ops) BUG_ON(ops == NULL); if ( iommu_ops && iommu_ops != ops ) + { printk("WARNING: Cannot set IOMMU ops, already set to a different value\n"); + return; + } iommu_ops = ops; } @@ -70,8 +73,8 @@ void arch_iommu_domain_destroy(struct domain *d) int arch_iommu_populate_page_table(struct domain *d) { - /* The IOMMU shares the p2m with the CPU */ - return -ENOSYS; + /* Return an error if the IOMMU shares the p2m with the CPU */ + return iommu_use_hap_pt(d) ? -ENOSYS : 0; } void __hwdom_init arch_iommu_hwdom_init(struct domain *d) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-01-21 17:04 [PATCH 0/2] Dangling fixes for ARM iommu Andrii Anisov 2019-01-21 17:04 ` [PATCH 1/2] iommu/arm: Misc fixes for arch specific part Andrii Anisov @ 2019-01-21 17:04 ` Andrii Anisov 2019-10-01 15:04 ` [Xen-devel] " Julien Grall 2019-01-21 17:29 ` [PATCH 0/2] Dangling fixes for ARM iommu Julien Grall 2 siblings, 1 reply; 18+ messages in thread From: Andrii Anisov @ 2019-01-21 17:04 UTC (permalink / raw) To: xen-devel; +Cc: Oleksandr Tyshchenko, Julien Grall, Stefano Stabellini From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> We don't passthrough IOMMU device to DOM0 even if it is not used by Xen. Therefore exposing the properties that describe relationship between master devices and IOMMUs does not make any sense. According to the: 1. Documentation/devicetree/bindings/iommu/iommu.txt 2. Documentation/devicetree/bindings/pci/pci-iommu.txt Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Acked-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/domain_build.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d2c63a8..15a08d6 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -540,6 +540,16 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, continue; } + /* Don't expose IOMMU specific properties to the guest */ + if ( dt_property_name_is_equal(prop, "iommus") ) + continue; + + if ( dt_property_name_is_equal(prop, "iommu-map") ) + continue; + + if ( dt_property_name_is_equal(prop, "iommu-map-mask") ) + continue; + res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); if ( res ) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-01-21 17:04 ` [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest Andrii Anisov @ 2019-10-01 15:04 ` Julien Grall 2019-10-01 15:25 ` Oleksandr 0 siblings, 1 reply; 18+ messages in thread From: Julien Grall @ 2019-10-01 15:04 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Volodymyr Babchuk Hi, I am reviving the thread. I think we need a patch similar to this one for Xen 4.13. This is because generic are now used by Xen so they should be hidden from the hardware domain. Andrii, Oleksandr, can one of you look at it? Cheers, On 21/01/2019 17:04, Andrii Anisov wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > We don't passthrough IOMMU device to DOM0 even if it is not used by > Xen. Therefore exposing the properties that describe relationship > between master devices and IOMMUs does not make any sense. > > According to the: > 1. Documentation/devicetree/bindings/iommu/iommu.txt > 2. Documentation/devicetree/bindings/pci/pci-iommu.txt > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Acked-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/domain_build.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index d2c63a8..15a08d6 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -540,6 +540,16 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, > continue; > } > > + /* Don't expose IOMMU specific properties to the guest */ > + if ( dt_property_name_is_equal(prop, "iommus") ) > + continue; > + > + if ( dt_property_name_is_equal(prop, "iommu-map") ) > + continue; > + > + if ( dt_property_name_is_equal(prop, "iommu-map-mask") ) > + continue; > + > res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); > > if ( res ) > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-10-01 15:04 ` [Xen-devel] " Julien Grall @ 2019-10-01 15:25 ` Oleksandr 2019-10-01 15:36 ` Julien Grall 0 siblings, 1 reply; 18+ messages in thread From: Oleksandr @ 2019-10-01 15:25 UTC (permalink / raw) To: Julien Grall, Andrii Anisov, xen-devel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Volodymyr Babchuk On 01.10.19 18:04, Julien Grall wrote: > Hi, Hi Julien > > I am reviving the thread. I think we need a patch similar to this one > for Xen 4.13. This is because generic are now used by Xen so they > should be hidden from the hardware domain. > > Andrii, Oleksandr, can one of you look at it? I will be able to look at it probably at the end of the week if there is no urgency. > > Cheers, > > On 21/01/2019 17:04, Andrii Anisov wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> We don't passthrough IOMMU device to DOM0 even if it is not used by >> Xen. Therefore exposing the properties that describe relationship >> between master devices and IOMMUs does not make any sense. >> >> According to the: >> 1. Documentation/devicetree/bindings/iommu/iommu.txt >> 2. Documentation/devicetree/bindings/pci/pci-iommu.txt >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> Acked-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/domain_build.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index d2c63a8..15a08d6 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -540,6 +540,16 @@ static int __init write_properties(struct domain >> *d, struct kernel_info *kinfo, >> continue; >> } >> + /* Don't expose IOMMU specific properties to the guest */ >> + if ( dt_property_name_is_equal(prop, "iommus") ) >> + continue; >> + >> + if ( dt_property_name_is_equal(prop, "iommu-map") ) >> + continue; >> + >> + if ( dt_property_name_is_equal(prop, "iommu-map-mask") ) >> + continue; >> + >> res = fdt_property(kinfo->fdt, prop->name, prop_data, >> prop_len); >> if ( res ) >> > Julien, are you happy to see this patch as is, or do you have some comments regarding it? -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-10-01 15:25 ` Oleksandr @ 2019-10-01 15:36 ` Julien Grall 2019-10-01 16:07 ` Oleksandr 0 siblings, 1 reply; 18+ messages in thread From: Julien Grall @ 2019-10-01 15:36 UTC (permalink / raw) To: Oleksandr, Andrii Anisov, xen-devel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Volodymyr Babchuk Hi Oleksandr, On 01/10/2019 16:25, Oleksandr wrote: > > On 01.10.19 18:04, Julien Grall wrote: >> Hi, > > Hi Julien > > >> >> I am reviving the thread. I think we need a patch similar to this one for Xen >> 4.13. This is because generic are now used by Xen so they should be hidden >> from the hardware domain. >> >> Andrii, Oleksandr, can one of you look at it? > > I will be able to look at it probably at the end of the week if there is no > urgency. That's fine, I think we can make a case to add it in Xen 4.13. > > >> >> Cheers, >> >> On 21/01/2019 17:04, Andrii Anisov wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> We don't passthrough IOMMU device to DOM0 even if it is not used by >>> Xen. Therefore exposing the properties that describe relationship >>> between master devices and IOMMUs does not make any sense. >>> >>> According to the: >>> 1. Documentation/devicetree/bindings/iommu/iommu.txt >>> 2. Documentation/devicetree/bindings/pci/pci-iommu.txt >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> Acked-by: Julien Grall <julien.grall@arm.com> >>> --- >>> xen/arch/arm/domain_build.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index d2c63a8..15a08d6 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -540,6 +540,16 @@ static int __init write_properties(struct domain *d, >>> struct kernel_info *kinfo, >>> continue; >>> } >>> + /* Don't expose IOMMU specific properties to the guest */ >>> + if ( dt_property_name_is_equal(prop, "iommus") ) >>> + continue; >>> + >>> + if ( dt_property_name_is_equal(prop, "iommu-map") ) >>> + continue; >>> + >>> + if ( dt_property_name_is_equal(prop, "iommu-map-mask") ) >>> + continue; >>> + >>> res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); >>> if ( res ) >>> >> > Julien, are you happy to see this patch as is, or do you have some comments > regarding it? I have some comments on the cover letter for this patch. Please see [1]. Thank you for having a look at the patch. Cheers, [1] <ed087980-a2b9-2fd4-7e84-446142e8176b@arm.com> -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-10-01 15:36 ` Julien Grall @ 2019-10-01 16:07 ` Oleksandr 2019-10-01 19:07 ` Julien Grall 0 siblings, 1 reply; 18+ messages in thread From: Oleksandr @ 2019-10-01 16:07 UTC (permalink / raw) To: Julien Grall, Andrii Anisov, xen-devel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Volodymyr Babchuk On 01.10.19 18:36, Julien Grall wrote: > Hi Oleksandr, Hi Julien > > On 01/10/2019 16:25, Oleksandr wrote: >> >> On 01.10.19 18:04, Julien Grall wrote: >>> Hi, >> >> Hi Julien >> >> >>> >>> I am reviving the thread. I think we need a patch similar to this >>> one for Xen 4.13. This is because generic are now used by Xen so >>> they should be hidden from the hardware domain. >>> >>> Andrii, Oleksandr, can one of you look at it? >> >> I will be able to look at it probably at the end of the week if there >> is no urgency. > > That's fine, I think we can make a case to add it in Xen 4.13. > >> >> >>> >>> Cheers, >>> >>> On 21/01/2019 17:04, Andrii Anisov wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> We don't passthrough IOMMU device to DOM0 even if it is not used by >>>> Xen. Therefore exposing the properties that describe relationship >>>> between master devices and IOMMUs does not make any sense. >>>> >>>> According to the: >>>> 1. Documentation/devicetree/bindings/iommu/iommu.txt >>>> 2. Documentation/devicetree/bindings/pci/pci-iommu.txt >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> Acked-by: Julien Grall <julien.grall@arm.com> >>>> --- >>>> xen/arch/arm/domain_build.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index d2c63a8..15a08d6 100644 >>>> --- a/xen/arch/arm/domain_build.c >>>> +++ b/xen/arch/arm/domain_build.c >>>> @@ -540,6 +540,16 @@ static int __init write_properties(struct >>>> domain *d, struct kernel_info *kinfo, >>>> continue; >>>> } >>>> + /* Don't expose IOMMU specific properties to the guest */ >>>> + if ( dt_property_name_is_equal(prop, "iommus") ) >>>> + continue; >>>> + >>>> + if ( dt_property_name_is_equal(prop, "iommu-map") ) >>>> + continue; >>>> + >>>> + if ( dt_property_name_is_equal(prop, "iommu-map-mask") ) >>>> + continue; >>>> + >>>> res = fdt_property(kinfo->fdt, prop->name, prop_data, >>>> prop_len); >>>> if ( res ) >>>> >>> >> Julien, are you happy to see this patch as is, or do you have some >> comments regarding it? > > I have some comments on the cover letter for this patch. Please see [1]. > > Thank you for having a look at the patch. > > Cheers, > > [1] <ed087980-a2b9-2fd4-7e84-446142e8176b@arm.com> Looking briefly, I found two main points regarding that patch. This is how I understand them (please, correct me if I am wrong): 1. The IOMMU can be accessible by Dom0 (for example, if we pass "iommu=disabled" to Xen command line or it is enabled, but there is not suitable driver in Xen found). There is no need to remove properties if Dom0 is already touching the IOMMU. 2. Generic IOMMU DT bindings is not used in Xen so far. There is no need to remove properties. As I understand, both points are not actual anymore and nothing to modify in that patch, correct? Because: 1. Giving the IOMMU to Dom0 is a bad idea. 2. Already supported. -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-10-01 16:07 ` Oleksandr @ 2019-10-01 19:07 ` Julien Grall 2019-10-03 12:18 ` Oleksandr 0 siblings, 1 reply; 18+ messages in thread From: Julien Grall @ 2019-10-01 19:07 UTC (permalink / raw) To: Oleksandr, Andrii Anisov, xen-devel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Volodymyr Babchuk Hi Oleksandr, On 10/1/19 5:07 PM, Oleksandr wrote: > > On 01.10.19 18:36, Julien Grall wrote: >> On 01/10/2019 16:25, Oleksandr wrote: >>> >>> On 01.10.19 18:04, Julien Grall wrote: > > 1. Giving the IOMMU to Dom0 is a bad idea. Please to try expand your thoughts in the same e-mail when you say "this is a bad idea". But, this is clearly what happen in current Xen setup if the driver is not enabled. What I want to avoid is exposing an half complete bindings to the guest (you don't know how it will behave). So we either remove all the properties and node related to the IOMMUs or nothing. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-10-01 19:07 ` Julien Grall @ 2019-10-03 12:18 ` Oleksandr 2019-10-07 21:02 ` Julien Grall 0 siblings, 1 reply; 18+ messages in thread From: Oleksandr @ 2019-10-03 12:18 UTC (permalink / raw) To: Julien Grall, xen-devel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Volodymyr Babchuk, Andrii Anisov On 01.10.19 22:07, Julien Grall wrote: > Hi Oleksandr, Hi Julien > > On 10/1/19 5:07 PM, Oleksandr wrote: >> >> On 01.10.19 18:36, Julien Grall wrote: >>> On 01/10/2019 16:25, Oleksandr wrote: >>>> >>>> On 01.10.19 18:04, Julien Grall wrote: >> > 1. Giving the IOMMU to Dom0 is a bad idea. > > Please to try expand your thoughts in the same e-mail when you say > "this is a bad idea". Well, this was a conclusion I had got from the discussion [1]. Sorry for not being clear here. > > But, this is clearly what happen in current Xen setup if the driver is > not enabled. What I want to avoid is exposing an half complete > bindings to the guest (you don't know how it will behave). > > So we either remove all the properties and node related to the IOMMUs > or nothing. I think, I got it. Our target is *not* to add a way for Dom0 to use IOMMU, but to be consistent in removing IOMMU node/master device properties. Now, we remove the IOMMU node if Xen identifies it (the IOMMU driver is present in Xen), so looks like we have to remove master device properties only if this master device is behind the IOMMU which node is removed. This, I hope, will avoid exposing an half complete bindings to guest. Am I right? [1] https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00858.html ---------- If you happy with that logic I will craft a proper patch. diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 67021d9..6d45e55 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -480,10 +480,26 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, const struct dt_property *prop, *status = NULL; int res = 0; int had_dom0_bootargs = 0; + struct dt_device_node *iommu_node; if ( kinfo->cmdline && kinfo->cmdline[0] ) bootargs = &kinfo->cmdline[0]; + /* + * If we skip the IOMMU device when creating DT for Dom0 (even if + * the IOMMU device is not used by Xen), we should also skip the IOMMU + * specific properties of the master device behind it in order to avoid + * exposing an half complete IOMMU bindings to Dom0. + * Use "iommu_node" as an indicator of the master device which properties + * should be skipped. + */ + iommu_node = dt_parse_phandle(node, "iommus", 0); + if ( iommu_node ) + { + if ( device_get_class(iommu_node) != DEVICE_IOMMU ) + iommu_node = NULL; + } + dt_for_each_property_node (node, prop) { const void *prop_data = prop->value; @@ -540,6 +556,19 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, continue; } + if ( iommu_node ) + { + /* Don't expose IOMMU specific properties to Dom0 */ + if ( dt_property_name_is_equal(prop, "iommus") ) + continue; + + if ( dt_property_name_is_equal(prop, "iommu-map") ) + continue; + + if ( dt_property_name_is_equal(prop, "iommu-map-mask") ) + continue; + } + res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); if ( res ) -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-10-03 12:18 ` Oleksandr @ 2019-10-07 21:02 ` Julien Grall 2019-10-08 15:34 ` Oleksandr 0 siblings, 1 reply; 18+ messages in thread From: Julien Grall @ 2019-10-07 21:02 UTC (permalink / raw) To: Oleksandr, xen-devel Cc: Oleksandr Tyshchenko, nd, Volodymyr Babchuk, Stefano Stabellini, Andrii Anisov Hi, On 03/10/2019 13:18, Oleksandr wrote: > > On 01.10.19 22:07, Julien Grall wrote: >> On 10/1/19 5:07 PM, Oleksandr wrote: >>> >>> On 01.10.19 18:36, Julien Grall wrote: >>>> On 01/10/2019 16:25, Oleksandr wrote: >>>>> >>>>> On 01.10.19 18:04, Julien Grall wrote: >>> > 1. Giving the IOMMU to Dom0 is a bad idea. >> >> Please to try expand your thoughts in the same e-mail when you say >> "this is a bad idea". > > Well, this was a conclusion I had got from the discussion [1]. Sorry for > not being clear here. > > >> >> But, this is clearly what happen in current Xen setup if the driver is >> not enabled. What I want to avoid is exposing an half complete >> bindings to the guest (you don't know how it will behave). >> >> So we either remove all the properties and node related to the IOMMUs >> or nothing. > I think, I got it. Our target is *not* to add a way for Dom0 to use > IOMMU, but to be consistent in removing IOMMU node/master device > properties. Now, we remove the IOMMU node if Xen identifies it (the > IOMMU driver is present in Xen), > so looks like we have to remove master device properties only if this > master device is behind the IOMMU which node is removed. This, I hope, > will avoid exposing an half complete bindings to guest. Am I right? > > > [1] > https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00858.html > > > ---------- > > If you happy with that logic I will craft a proper patch. The logic looks alright to me. One comment below, I will look at the rest once this is formally sent. > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 67021d9..6d45e55 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -480,10 +480,26 @@ static int __init write_properties(struct domain > *d, struct kernel_info *kinfo, > const struct dt_property *prop, *status = NULL; > int res = 0; > int had_dom0_bootargs = 0; > + struct dt_device_node *iommu_node; > > if ( kinfo->cmdline && kinfo->cmdline[0] ) > bootargs = &kinfo->cmdline[0]; > > + /* > + * If we skip the IOMMU device when creating DT for Dom0 (even if I would prefer if we use "hwdom" over "Dom0". They are both the same on Arm, but this may change in the future (we may actually drop Dom0 ;)). > + * the IOMMU device is not used by Xen), we should also skip the IOMMU > + * specific properties of the master device behind it in order to > avoid > + * exposing an half complete IOMMU bindings to Dom0. > + * Use "iommu_node" as an indicator of the master device which > properties > + * should be skipped. > + */ > + iommu_node = dt_parse_phandle(node, "iommus", 0); > + if ( iommu_node ) > + { > + if ( device_get_class(iommu_node) != DEVICE_IOMMU ) > + iommu_node = NULL; > + } > + > dt_for_each_property_node (node, prop) > { > const void *prop_data = prop->value; > @@ -540,6 +556,19 @@ static int __init write_properties(struct domain > *d, struct kernel_info *kinfo, > continue; > } > > + if ( iommu_node ) > + { > + /* Don't expose IOMMU specific properties to Dom0 */ > + if ( dt_property_name_is_equal(prop, "iommus") ) > + continue; > + > + if ( dt_property_name_is_equal(prop, "iommu-map") ) > + continue; > + > + if ( dt_property_name_is_equal(prop, "iommu-map-mask") ) > + continue; > + } > + > res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len); > > if ( res ) > > Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest 2019-10-07 21:02 ` Julien Grall @ 2019-10-08 15:34 ` Oleksandr 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr @ 2019-10-08 15:34 UTC (permalink / raw) To: Julien Grall, xen-devel Cc: Oleksandr Tyshchenko, nd, Volodymyr Babchuk, Stefano Stabellini, Andrii Anisov On 08.10.19 00:02, Julien Grall wrote: > Hi, Hi Julien >>> But, this is clearly what happen in current Xen setup if the driver is >>> not enabled. What I want to avoid is exposing an half complete >>> bindings to the guest (you don't know how it will behave). >>> >>> So we either remove all the properties and node related to the IOMMUs >>> or nothing. >> I think, I got it. Our target is *not* to add a way for Dom0 to use >> IOMMU, but to be consistent in removing IOMMU node/master device >> properties. Now, we remove the IOMMU node if Xen identifies it (the >> IOMMU driver is present in Xen), >> so looks like we have to remove master device properties only if this >> master device is behind the IOMMU which node is removed. This, I hope, >> will avoid exposing an half complete bindings to guest. Am I right? >> >> >> [1] >> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00858.html >> >> >> ---------- >> >> If you happy with that logic I will craft a proper patch. > The logic looks alright to me. One comment below, I will look at the > rest once this is formally sent. ok > >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 67021d9..6d45e55 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -480,10 +480,26 @@ static int __init write_properties(struct domain >> *d, struct kernel_info *kinfo, >> const struct dt_property *prop, *status = NULL; >> int res = 0; >> int had_dom0_bootargs = 0; >> + struct dt_device_node *iommu_node; >> >> if ( kinfo->cmdline && kinfo->cmdline[0] ) >> bootargs = &kinfo->cmdline[0]; >> >> + /* >> + * If we skip the IOMMU device when creating DT for Dom0 (even if > I would prefer if we use "hwdom" over "Dom0". They are both the same on > Arm, but this may change in the future (we may actually drop Dom0 ;)). Already sent v2, please see: https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00673.html -- Regards, Oleksandr Tyshchenko _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Dangling fixes for ARM iommu 2019-01-21 17:04 [PATCH 0/2] Dangling fixes for ARM iommu Andrii Anisov 2019-01-21 17:04 ` [PATCH 1/2] iommu/arm: Misc fixes for arch specific part Andrii Anisov 2019-01-21 17:04 ` [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest Andrii Anisov @ 2019-01-21 17:29 ` Julien Grall 2019-01-22 13:48 ` Andrii Anisov 2 siblings, 1 reply; 18+ messages in thread From: Julien Grall @ 2019-01-21 17:29 UTC (permalink / raw) To: Andrii Anisov, xen-devel; +Cc: Juergen Gross, Stefano Stabellini, Andrii Anisov (+ Juergen) On 21/01/2019 17:04, Andrii Anisov wrote: > From: Andrii Anisov <andrii_anisov@epam.com> > > Dear All, Hello, All patches candidate for Xen 4.12 should have the release manager CCed and explain the pros/cons to have those patches for this release. It is also useful if you add for-4.12 (or similar) in the [...] so we can prioritize it. > > With moving our working setup to 4.12-rc2 (IPMMU series especially),I've > realized that we have a couple of patches from IPMMU series [1] which are > fixes, got RB/AB, but not reached mainline yet. I really hope those RB/AB > would not be treated as stale, and patches are OK to go into 4.12 release. Firstly, the RB tag means the reviewer reviewed the patch in depth with the code base at that time. So I don't think the RBs should have been carried from a patch series sent 2 years ago. Indeed code base evolved, for instance iommu_use_hap_pt has a slight different behavior now. So please stripped my RB. The AB is arguable as it is quite old. Secondly, it often happens that some patches have the required acked-by and reviewed-by but are not merged because they makes little sense without the rest of the series. In that case, the current code base does not support the case where the P2M is not shared with the IOMMU and does not support new IOMMU bindings in the current setup. So I am not convinced they should be included in Xen 4.12 or even without the rest of the series. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Dangling fixes for ARM iommu 2019-01-21 17:29 ` [PATCH 0/2] Dangling fixes for ARM iommu Julien Grall @ 2019-01-22 13:48 ` Andrii Anisov 2019-01-22 14:31 ` Julien Grall 0 siblings, 1 reply; 18+ messages in thread From: Andrii Anisov @ 2019-01-22 13:48 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Juergen Gross, Stefano Stabellini, Andrii Anisov Hello Julien, On 21.01.19 19:29, Julien Grall wrote: > (+ Juergen) > All patches candidate for Xen 4.12 should have the release manager CCed and explain the pros/cons to have those patches for this release. It is also useful if you add for-4.12 (or similar) in the [...] so we can prioritize it. I've got the point. Thank you for adding Juergen. > Indeed code base evolved, for instance iommu_use_hap_pt has a slight different behavior now. Actually you properly pointed the problem. I'm not sure how it happened, because I did test build and run for those patches yesterday. But now, progressing with our stuff porting, I realized that the patch 1 is missing dependency on `xen/sched.h` required by `iommu_use_hap_pt`. > So please stripped my RB OK. > The AB is arguable as it is quite old. I could drop it for v2. > Secondly, it often happens that some patches have the required acked-by and reviewed-by but are not merged because they makes little sense without the rest of the series. Yes, sure, but fixes are still relevant and are quite autonomous. The only relation to the series is that they were discovered during its implementation. > In that case, the current code base does not support the case where the P2M is not shared with the IOMMU and does not support new IOMMU bindings in the current setup. It is the intention of the rest of the series. > So I am not convinced they should be included in Xen 4.12 or even without the rest of the series. IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should be OK for 4.12. -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Dangling fixes for ARM iommu 2019-01-22 13:48 ` Andrii Anisov @ 2019-01-22 14:31 ` Julien Grall 2019-01-23 17:53 ` Andrii Anisov 0 siblings, 1 reply; 18+ messages in thread From: Julien Grall @ 2019-01-22 14:31 UTC (permalink / raw) To: Andrii Anisov, xen-devel; +Cc: Juergen Gross, Stefano Stabellini, Andrii Anisov On 1/22/19 1:48 PM, Andrii Anisov wrote: > Hello Julien, Hi, > On 21.01.19 19:29, Julien Grall wrote: >> (+ Juergen) >> All patches candidate for Xen 4.12 should have the release manager >> CCed and explain the pros/cons to have those patches for this release. >> It is also useful if you add for-4.12 (or similar) in the [...] so we >> can prioritize it. > > I've got the point. Thank you for adding Juergen. > >> Indeed code base evolved, for instance iommu_use_hap_pt has a slight >> different behavior now. > > Actually you properly pointed the problem. I'm not sure how it happened, > because I did test build and run for those patches yesterday. But now, > progressing with our stuff porting, I realized that the patch 1 is > missing dependency on `xen/sched.h` required by `iommu_use_hap_pt`. > >> So please stripped my RB > OK. > >> The AB is arguable as it is quite old. > I could drop it for v2. Yes please. But see below. > >> Secondly, it often happens that some patches have the required >> acked-by and reviewed-by but are not merged because they makes little >> sense without the rest of the series. > > Yes, sure, but fixes are still relevant and are quite autonomous. The > only relation to the series is that they were discovered during its > implementation. A fix would be if you found a bug in a configuration that we are meant to support. > >> In that case, the current code base does not support the case where >> the P2M is not shared with the IOMMU and does not support new IOMMU >> bindings in the current setup. > > It is the intention of the rest of the series. The rest of series is not ready and not targeting 4.12. So why should we get them merged? > >> So I am not convinced they should be included in Xen 4.12 or even >> without the rest of the series. > IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should > be OK for 4.12. The first hunk of patch 1 aside, we never supported the new IOMMU bindings nor unsharing the P2M. So what do you actually fix? Supporting unshared P2M will require more work given because the code relies on mfn_to_gmfn that is not implemented on Arm. So accepting this patch standalone would be misleading as the rest of the series is not going to be merged in Xen 4.12. Similarly, we don't support new IOMMU bindings. The patch #2 alone is going to add more trouble as now Dom0 would not be able to use the IOMMU if it were not hidden. So I still question the usefulness of those 2 patches outside of the series. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Dangling fixes for ARM iommu 2019-01-22 14:31 ` Julien Grall @ 2019-01-23 17:53 ` Andrii Anisov 2019-01-23 18:14 ` Julien Grall 0 siblings, 1 reply; 18+ messages in thread From: Andrii Anisov @ 2019-01-23 17:53 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Juergen Gross, Stefano Stabellini, Andrii Anisov Hello Julien, On 22.01.19 16:31, Julien Grall wrote: >> IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should be OK for 4.12. > > The first hunk of patch 1 aside, we never supported the new IOMMU bindings nor unsharing the P2M. So what do you actually fix? > > Supporting unshared P2M will require more work given because the code relies on mfn_to_gmfn that is not implemented on Arm. So accepting this patch standalone would be misleading as the rest of the series is not going to be merged in Xen 4.12. I'm not saying I want something from non-shared p2m push into 4.12. I would strip the first patch to its first chunk. > Similarly, we don't support new IOMMU bindings. The patch #2 alone is going to add more trouble as now Dom0 would not be able to use the IOMMU if it were not hidden. Are you sure we should pass IOMMU devices or related info to Dom0 if they are not supported by hypervisor? The commit message states "We don't passthrough IOMMU device to DOM0 even if it is not used by Xen.", and you agreed that some time ago. Could you please clarify why you have changed your mind? -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Dangling fixes for ARM iommu 2019-01-23 17:53 ` Andrii Anisov @ 2019-01-23 18:14 ` Julien Grall 2019-01-23 18:29 ` Andrii Anisov 0 siblings, 1 reply; 18+ messages in thread From: Julien Grall @ 2019-01-23 18:14 UTC (permalink / raw) To: Andrii Anisov, xen-devel; +Cc: Juergen Gross, Stefano Stabellini, Andrii Anisov On 23/01/2019 17:53, Andrii Anisov wrote: > Hello Julien, Hi, > On 22.01.19 16:31, Julien Grall wrote: >>> IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should be OK >>> for 4.12. >> >> The first hunk of patch 1 aside, we never supported the new IOMMU bindings nor >> unsharing the P2M. So what do you actually fix? >> >> Supporting unshared P2M will require more work given because the code relies >> on mfn_to_gmfn that is not implemented on Arm. So accepting this patch >> standalone would be misleading as the rest of the series is not going to be >> merged in Xen 4.12. > I'm not saying I want something from non-shared p2m push into 4.12. I know and this is not what I implied in my previous e-mail. I pointed out that the rest of the series is not planned for Xen 4.12, hence it does not make sense to get that patch merged. > I would strip the first patch to its first chunk. I can queue the first hunk for Xen 4.13. > >> Similarly, we don't support new IOMMU bindings. The patch #2 alone is going to >> add more trouble as now Dom0 would not be able to use the IOMMU if it were not >> hidden. > Are you sure we should pass IOMMU devices or related info to Dom0 if they are > not supported by hypervisor? > The commit message states "We don't passthrough IOMMU device to DOM0 even if it > is not used by Xen.", and you agreed that some time ago. Could you please > clarify why you have changed your mind? This statement is only correct if the IOMMU has been actively blacklisted by Xen. In the case of the SMMU driver, this is done by arm_smmu_dt_init(). The function is only called when the IOMMU has been enabled. So if you pass "iommu=disabled" to Xen commandline, the SMMU will be accessible by Dom0. When there are no drivers for the IOMMU used, then you end up to assign the IOMMU to Dom0. This means that Dom0 can use them. It does not make sense to remove the properties "iommus", "iommu-map", "iommu-map-mask" if Dom0 is already touching the IOMMU. It is either everything or nothing. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Dangling fixes for ARM iommu 2019-01-23 18:14 ` Julien Grall @ 2019-01-23 18:29 ` Andrii Anisov 2019-01-23 18:34 ` Julien Grall 0 siblings, 1 reply; 18+ messages in thread From: Andrii Anisov @ 2019-01-23 18:29 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Juergen Gross, Stefano Stabellini, Andrii Anisov On 23.01.19 20:14, Julien Grall wrote: > I can queue the first hunk for Xen 4.13. For 4.13? Well I hope we will manage to upstream the whole series until then. > This statement is only correct if the IOMMU has been actively blacklisted by Xen. In the case of the SMMU driver, this is done by arm_smmu_dt_init(). The function is only called when the IOMMU has been enabled. > > So if you pass "iommu=disabled" to Xen commandline, the SMMU will be accessible by Dom0. > > When there are no drivers for the IOMMU used, then you end up to assign the IOMMU to Dom0. This means that Dom0 can use them. > > It does not make sense to remove the properties "iommus", "iommu-map", "iommu-map-mask" if Dom0 is already touching the IOMMU. It is either everything or nothing. So that patch should be totally rewritten. At least to support iommu=disabled. -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Dangling fixes for ARM iommu 2019-01-23 18:29 ` Andrii Anisov @ 2019-01-23 18:34 ` Julien Grall 0 siblings, 0 replies; 18+ messages in thread From: Julien Grall @ 2019-01-23 18:34 UTC (permalink / raw) To: Andrii Anisov, xen-devel; +Cc: Juergen Gross, Stefano Stabellini, Andrii Anisov On 23/01/2019 18:29, Andrii Anisov wrote: > > On 23.01.19 20:14, Julien Grall wrote: >> I can queue the first hunk for Xen 4.13. > For 4.13? Well I hope we will manage to upstream the whole series until then. If you plan to resend it separately, I can apply to the next branch regardless the rest of the series. >> It does not make sense to remove the properties "iommus", "iommu-map", >> "iommu-map-mask" if Dom0 is already touching the IOMMU. It is either >> everything or nothing. > So that patch should be totally rewritten. At least to support iommu=disabled. And the case where IOMMU is enabled but no IOMMU driver is present in Xen. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-10-08 15:34 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-21 17:04 [PATCH 0/2] Dangling fixes for ARM iommu Andrii Anisov 2019-01-21 17:04 ` [PATCH 1/2] iommu/arm: Misc fixes for arch specific part Andrii Anisov 2019-01-21 17:04 ` [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest Andrii Anisov 2019-10-01 15:04 ` [Xen-devel] " Julien Grall 2019-10-01 15:25 ` Oleksandr 2019-10-01 15:36 ` Julien Grall 2019-10-01 16:07 ` Oleksandr 2019-10-01 19:07 ` Julien Grall 2019-10-03 12:18 ` Oleksandr 2019-10-07 21:02 ` Julien Grall 2019-10-08 15:34 ` Oleksandr 2019-01-21 17:29 ` [PATCH 0/2] Dangling fixes for ARM iommu Julien Grall 2019-01-22 13:48 ` Andrii Anisov 2019-01-22 14:31 ` Julien Grall 2019-01-23 17:53 ` Andrii Anisov 2019-01-23 18:14 ` Julien Grall 2019-01-23 18:29 ` Andrii Anisov 2019-01-23 18:34 ` Julien Grall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).