Re: [Xen-devel] [PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI devices

[Julien Grall <julien.grall@xxxxxxx>]

On 07/07/2017 07:06 AM, Wei Chen wrote:
> Hi Stefano,
>
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> > Sent: 2017年7月7日 5:05
> > To: Wei Chen <Wei.Chen@xxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; Julien Grall
> > <Julien.Grall@xxxxxxx>; Steve Capper <Steve.Capper@xxxxxxx>; Kaly Xin
> > <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> > Subject: Re: [PATCH 2/2] xen/arm: smmu: Parse generic iommu binding for PCI
> > devices
> >
> > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > The legacy smmu binding will be deprecated in favour of the generic
> > > "iommus" binding. So we need a new helper to parse generic iommu
> > > bindings. When the system detects the SMMU is using generic iommu
> > > binding, this helper will be called when this platform device is
> > > assiged to a guest.
> > >
> > > This patch is based on Linux of_iommu.c:
> > >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driver
> > s/iommu/of_iommu.c
> > > The commit id is:
> > > 2a0c57545a291f257cd231b1c4b18285b84608d8
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > ---
> > >  xen/drivers/passthrough/arm/smmu.c | 73
> > ++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 71 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > > index 25f2207..50ff997 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -2750,7 +2750,72 @@ static int arm_smmu_platform_iommu_init(struct device
> > *dev)
> > >      return 0;
> > >  }
> > >
> > > -static int arm_smmu_xen_add_device(u8 devfn, struct device*dev)
> > > +/*
> > > + * Currently, we haven't supported PCI device on ARM. So this is the
> >
> > "Currently, we don't support PCI devices on ARM."
> Ok.
>
> > > + * temporary function to get device node of pci bridge device for
> > > + * function verification only
> >
> > What do you mean by "for function verification only"?
> See comment below.
>
> > > + */
> > > +static struct device_node *pci_get_bridge_device_node(struct device *dev)
> > > +{
> > > +   struct device_node *dt_dev;
> > > +   const char *type_str;
> > > +
> > > +   dt_for_each_device_node(dt_host, dt_dev) {
> > > +           /* Return the first pci bridge device node simply */
> > > +           if (!dt_property_read_string(dt_dev, "device_type", &type_str) &&
> > > +                   !strcmp(type_str, "pci"))
> > > +                   return dt_dev;
> >
> > Not only this is very expensive, but also it doesn't seem to be even
> > checking that it found the right pci bridge before returning it. We
> > cannot just return the first one we find. If we need this function for
> > testing, then please separate it out to a different patch (that won't get
> > applied).
> I just used it to verify the translation of PCI RID on my board. The 
> original
> intention of placing this patch to this series is to do a reminder:
> After we support PCI devices on ARM, we should fix this function to return
> correct PCI bridge.
>
> But now, it seems the worry is needless, I will separate this patch from 
> this
> Series.
Well, I am a little bit concerned to merge a code that we don't know  
whether it will work when PCI will be fully supported in Xen and until  
then would just be dead code.
I can see quite a few problems in this code, you assume BDF == RID. This  
is not true on all the platform. You should at least call  
pci_for_each_dma_alias. The function to_pci will return NULL, how can  
you get through the segfault?
But looking at the code, I am not even sure why this code is in ARM SMMU  
given that likely you will have to do for the same for all the SMMUs.  
Looking at Linux, they are calling of_iommu_xlate on each PCI devices.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel