| Message ID | 1570548304-12020-1-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.13,v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom | expand |
Hi, On 10/8/19 4:25 PM, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > We don't passthrough IOMMU device to hwdom 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 It is not entirely clear that documentation is from Linux. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > --- > Changes V1 [1] -> V2: > - Only skip IOMMU specific properties of the master device if we > skip the corresponding IOMMU device > - Use "hwdom" over "Dom0" > > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00104.html > --- > xen/arch/arm/domain_build.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 6d6dd52..a7321b8 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 hwdom (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 hwdom. > + * Use "iommu_node" as an indicator of the master device which properties > + * should be skipped. > + */ > + iommu_node = dt_parse_phandle(node, "iommus", 0); The code is slightly confusing to read. I don't think we should care of invalid DT here, so let's only consider valid one. For valid DT, any non-NULL return should point to an IOMMU. The comment above suggests that all IOMMU will be skipped. However, the check below (device_get_class(iommu_node)) will not return DEVICE_IOMMU when there are not have a driver for the IOMMU. So this needs to be clarified in the commit message. > + if ( iommu_node ) > + { > + if ( device_get_class(iommu_node) != DEVICE_IOMMU ) > + iommu_node = NULL; > + } Could we gather the two conditions in one if? > + > 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 hwdom */ > + 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,
On 10.10.19 18:18, Julien Grall wrote: > Hi, Hi Julien > > On 10/8/19 4:25 PM, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> We don't passthrough IOMMU device to hwdom 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 > > It is not entirely clear that documentation is from Linux. Will clarify. > >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> --- >> Changes V1 [1] -> V2: >> - Only skip IOMMU specific properties of the master device if we >> skip the corresponding IOMMU device >> - Use "hwdom" over "Dom0" >> >> [1] >> https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00104.html >> --- >> xen/arch/arm/domain_build.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 6d6dd52..a7321b8 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 hwdom (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 hwdom. >> + * Use "iommu_node" as an indicator of the master device which >> properties >> + * should be skipped. >> + */ >> + iommu_node = dt_parse_phandle(node, "iommus", 0); > > The code is slightly confusing to read. I don't think we should care > of invalid DT here, so let's only consider valid one. Do you mean "the comment" is confusing to read? > > For valid DT, any non-NULL return should point to an IOMMU. The > comment above suggests that all IOMMU will be skipped. However, the > check below (device_get_class(iommu_node)) will not return > DEVICE_IOMMU when there are not have a driver for the IOMMU. > > So this needs to be clarified in the commit message. Will do. > >> + if ( iommu_node ) >> + { >> + if ( device_get_class(iommu_node) != DEVICE_IOMMU ) >> + iommu_node = NULL; >> + } > > Could we gather the two conditions in one if? Yes.
Hi, On 10/10/19 4:27 PM, Oleksandr wrote: > > On 10.10.19 18:18, Julien Grall wrote: >> Hi, > > Hi Julien > > >> >> On 10/8/19 4:25 PM, Oleksandr Tyshchenko wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> We don't passthrough IOMMU device to hwdom 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 >> >> It is not entirely clear that documentation is from Linux. > > Will clarify. > > >> >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> --- >>> Changes V1 [1] -> V2: >>> - Only skip IOMMU specific properties of the master device if we >>> skip the corresponding IOMMU device >>> - Use "hwdom" over "Dom0" >>> >>> [1] >>> https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00104.html >>> >>> --- >>> xen/arch/arm/domain_build.c | 29 +++++++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 6d6dd52..a7321b8 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 hwdom (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 hwdom. >>> + * Use "iommu_node" as an indicator of the master device which >>> properties >>> + * should be skipped. >>> + */ >>> + iommu_node = dt_parse_phandle(node, "iommus", 0); >> >> The code is slightly confusing to read. I don't think we should care >> of invalid DT here, so let's only consider valid one. > > Do you mean "the comment" is confusing to read? The code is confusing because "iommus" should always point to a IOMMU node, but then you check whether this is an IOMMU. So it is not clear if this is done for sanity check (or for a different reason). Cheers,
On 10.10.19 18:32, Julien Grall wrote: > Hi, Hi > >>>> [1] >>>> https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00104.html >>>> >>>> --- >>>> xen/arch/arm/domain_build.c | 29 +++++++++++++++++++++++++++++ >>>> 1 file changed, 29 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index 6d6dd52..a7321b8 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 hwdom >>>> (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 hwdom. >>>> + * Use "iommu_node" as an indicator of the master device which >>>> properties >>>> + * should be skipped. >>>> + */ >>>> + iommu_node = dt_parse_phandle(node, "iommus", 0); >>> >>> The code is slightly confusing to read. I don't think we should care >>> of invalid DT here, so let's only consider valid one. >> >> Do you mean "the comment" is confusing to read? > > The code is confusing because "iommus" should always point to a IOMMU > node, but then you check whether this is an IOMMU. So it is not clear > if this is done for sanity check (or for a different reason). Got it. Will clarify a reason.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 6d6dd52..a7321b8 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 hwdom (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 hwdom. + * 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 hwdom */ + 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 )