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

[Oleksandr <olekstysh@xxxxxxxxx>]

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?


> 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.


--
Regards,

Oleksandr Tyshchenko


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