[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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


> + * 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

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

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.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.