Re: [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxx>
> > >
> > > 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@xxxxxxxx>
> > >
> > > ---
> > > 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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel