| Message ID | 20170809100715.870516-4-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote: > The HiSilicon erratum 161010801 describes the limitation of HiSilicon > platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions. > > On these platforms GICv3 ITS translator is presented with the deviceID > by extending the MSI payload data to 64 bits to include the deviceID. > Hence, the PCIe controller on this platforms has to differentiate the > MSI payload against other DMA payload and has to modify the MSI payload. > This basically makes it difficult for this platforms to have a SMMU > translation for MSI. > > This patch implements a ACPI table based quirk to reserve the hw msi > regions in the smmu-v3 driver which means these address regions will > not be translated and will be excluded from iova allocations. > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) Please can you also add a devicetree binding with corresponding documentation to enable this workaround on non-ACPI based systems too? It should be straightforward if you update the arm_smmu_options table. Thanks, Will
Hi Will, > -----Original Message----- > From: Will Deacon [mailto:will.deacon@arm.com] > Sent: Thursday, August 10, 2017 6:27 PM > To: Shameerali Kolothum Thodi > Cc: lorenzo.pieralisi@arm.com; marc.zyngier@arm.com; > sudeep.holla@arm.com; robin.murphy@arm.com; hanjun.guo@linaro.org; > Gabriele Paoloni; John Garry; iommu@lists.linux-foundation.org; linux-arm- > kernel@lists.infradead.org; linux-acpi@vger.kernel.org; devel@acpica.org; > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo) > Subject: Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based > HiSilicon erratum 161010801 > > On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote: > > The HiSilicon erratum 161010801 describes the limitation of HiSilicon > > platforms Hip06/Hip07 to support the SMMU mappings for MSI > transactions. > > > > On these platforms GICv3 ITS translator is presented with the deviceID > > by extending the MSI payload data to 64 bits to include the deviceID. > > Hence, the PCIe controller on this platforms has to differentiate the > > MSI payload against other DMA payload and has to modify the MSI > payload. > > This basically makes it difficult for this platforms to have a SMMU > > translation for MSI. > > > > This patch implements a ACPI table based quirk to reserve the hw msi > > regions in the smmu-v3 driver which means these address regions will > > not be translated and will be excluded from iova allocations. > > > > Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@huawei.com> > > --- > > drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- > > 1 file changed, 22 insertions(+), 5 deletions(-) > > Please can you also add a devicetree binding with corresponding > documentation to enable this workaround on non-ACPI based systems too? > It should be straightforward if you update the arm_smmu_options table. As I mentioned before, devicetree was a lower priority and we would definitely submit patch to support that. Even if we update the arm_smmu_options table with DT binding, the generic function to retrieve the MSI address regions only works on ACPI/IORT case now. Moreover I am on holidays starting tomorrow, and really appreciate if this series can go through now. Please let me know. Thanks, Shameer
>>> Signed-off-by: Shameer Kolothum >> <shameerali.kolothum.thodi@huawei.com> >>> --- >>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- >>> 1 file changed, 22 insertions(+), 5 deletions(-) >> >> Please can you also add a devicetree binding with corresponding >> documentation to enable this workaround on non-ACPI based systems too? >> It should be straightforward if you update the arm_smmu_options table. > > As I mentioned before, devicetree was a lower priority and we would definitely > submit patch to support that. Even if we update the arm_smmu_options table > with DT binding, the generic function to retrieve the MSI address regions only > works on ACPI/IORT case now. > Hi Will, Can you confirm your stance on supporting this workaround for DT as well as ACPI? For us, we now only "officially" support ACPI FW, and DT support at this point is patchy/limited. To me, adding DT support is just more errata workaround code to maintain with little useful gain. Thanks very much, John > Moreover I am on holidays starting tomorrow, and really appreciate if this series > can go through now. > > Please let me know. > > Thanks, > Shameer > > . >
On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote: > >>>Signed-off-by: Shameer Kolothum > >><shameerali.kolothum.thodi@huawei.com> > >>>--- > >>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- > >>> 1 file changed, 22 insertions(+), 5 deletions(-) > >> > >>Please can you also add a devicetree binding with corresponding > >>documentation to enable this workaround on non-ACPI based systems too? > >>It should be straightforward if you update the arm_smmu_options table. > > > >As I mentioned before, devicetree was a lower priority and we would definitely > >submit patch to support that. Even if we update the arm_smmu_options table > >with DT binding, the generic function to retrieve the MSI address regions only > >works on ACPI/IORT case now. > > > > Hi Will, > > Can you confirm your stance on supporting this workaround for DT as well as > ACPI? > > For us, we now only "officially" support ACPI FW, and DT support at this > point is patchy/limited. To me, adding DT support is just more errata > workaround code to maintain with little useful gain. I basically don't like the idea of a driver that only works for one of ACPI or DT yet claims to support both. I'm less fussed about functionality differences (feature X is only available with firmware Y), but not working around a hardware erratum that we know about is just lazy. So I'd prefer that we handle this in both cases, or blacklist affected devices when booting with DT. Continuing as though there isn't an erratum is the worst thing we can do. Will
On 23/08/2017 14:24, Will Deacon wrote: > On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote: >>>>> Signed-off-by: Shameer Kolothum >>>> <shameerali.kolothum.thodi@huawei.com> >>>>> --- >>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- >>>>> 1 file changed, 22 insertions(+), 5 deletions(-) >>>> >>>> Please can you also add a devicetree binding with corresponding >>>> documentation to enable this workaround on non-ACPI based systems too? >>>> It should be straightforward if you update the arm_smmu_options table. >>> >>> As I mentioned before, devicetree was a lower priority and we would definitely >>> submit patch to support that. Even if we update the arm_smmu_options table >>> with DT binding, the generic function to retrieve the MSI address regions only >>> works on ACPI/IORT case now. >>> >> >> Hi Will, >> >> Can you confirm your stance on supporting this workaround for DT as well as >> ACPI? >> >> For us, we now only "officially" support ACPI FW, and DT support at this >> point is patchy/limited. To me, adding DT support is just more errata >> workaround code to maintain with little useful gain. > > I basically don't like the idea of a driver that only works for one of > ACPI or DT yet claims to support both. I'm less fussed about functionality > differences (feature X is only available with firmware Y), but not working > around a hardware erratum that we know about is just lazy. > > So I'd prefer that we handle this in both cases, or blacklist affected > devices when booting with DT. Continuing as though there isn't an erratum > is the worst thing we can do. OK, seems reasonable. We would consider blacklisting the device, where/how to do is the question. So the errata is in the GICv3 ITS/PCI host controller, and we just use the in-between SMMU (driver) to provide the workaround. So my initial impression is that the PCI host controller would have to be blacklisted IFF behind an IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it sound? Please also note that in our SoC we have other devices behind the same SMMU, like storage controller, which are not affected or related to the Errata. BTW, ignoring DT support, are you happy with this patchset? Regards, John > > Will > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > . >
On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote: > On 23/08/2017 14:24, Will Deacon wrote: > >On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote: > >>>>>Signed-off-by: Shameer Kolothum > >>>><shameerali.kolothum.thodi@huawei.com> > >>>>>--- > >>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- > >>>>>1 file changed, 22 insertions(+), 5 deletions(-) > >>>> > >>>>Please can you also add a devicetree binding with corresponding > >>>>documentation to enable this workaround on non-ACPI based systems too? > >>>>It should be straightforward if you update the arm_smmu_options table. > >>> > >>>As I mentioned before, devicetree was a lower priority and we would definitely > >>>submit patch to support that. Even if we update the arm_smmu_options table > >>>with DT binding, the generic function to retrieve the MSI address regions only > >>>works on ACPI/IORT case now. > >>> > >> > >>Hi Will, > >> > >>Can you confirm your stance on supporting this workaround for DT as well as > >>ACPI? > >> > >>For us, we now only "officially" support ACPI FW, and DT support at this > >>point is patchy/limited. To me, adding DT support is just more errata > >>workaround code to maintain with little useful gain. > > > >I basically don't like the idea of a driver that only works for one of > >ACPI or DT yet claims to support both. I'm less fussed about functionality > >differences (feature X is only available with firmware Y), but not working > >around a hardware erratum that we know about is just lazy. > > > >So I'd prefer that we handle this in both cases, or blacklist affected > >devices when booting with DT. Continuing as though there isn't an erratum > >is the worst thing we can do. > > OK, seems reasonable. > > We would consider blacklisting the device, where/how to do is the question. > > So the errata is in the GICv3 ITS/PCI host controller, and we just use the > in-between SMMU (driver) to provide the workaround. So my initial impression > is that the PCI host controller would have to be blacklisted IFF behind an > IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it > sound? If that avoids us running into the erratum, then fine by me. I'd obviously prefer we work-around it since we know how to, but that's up to you. > Please also note that in our SoC we have other devices behind the same SMMU, > like storage controller, which are not affected or related to the Errata. > > BTW, ignoring DT support, are you happy with this patchset? Yes, but I won't ack it without the above taken care of. Will
On 23/08/2017 17:43, Will Deacon wrote: > On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote: >> On 23/08/2017 14:24, Will Deacon wrote: >>> On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote: >>>>>>> Signed-off-by: Shameer Kolothum >>>>>> <shameerali.kolothum.thodi@huawei.com> >>>>>>> --- >>>>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- >>>>>>> 1 file changed, 22 insertions(+), 5 deletions(-) >>>>>> >>>>>> Please can you also add a devicetree binding with corresponding >>>>>> documentation to enable this workaround on non-ACPI based systems too? >>>>>> It should be straightforward if you update the arm_smmu_options table. >>>>> >>>>> As I mentioned before, devicetree was a lower priority and we would definitely >>>>> submit patch to support that. Even if we update the arm_smmu_options table >>>>> with DT binding, the generic function to retrieve the MSI address regions only >>>>> works on ACPI/IORT case now. >>>>> >>>> >>>> Hi Will, >>>> >>>> Can you confirm your stance on supporting this workaround for DT as well as >>>> ACPI? >>>> >>>> For us, we now only "officially" support ACPI FW, and DT support at this >>>> point is patchy/limited. To me, adding DT support is just more errata >>>> workaround code to maintain with little useful gain. >>> >>> I basically don't like the idea of a driver that only works for one of >>> ACPI or DT yet claims to support both. I'm less fussed about functionality >>> differences (feature X is only available with firmware Y), but not working >>> around a hardware erratum that we know about is just lazy. >>> >>> So I'd prefer that we handle this in both cases, or blacklist affected >>> devices when booting with DT. Continuing as though there isn't an erratum >>> is the worst thing we can do. >> >> OK, seems reasonable. >> >> We would consider blacklisting the device, where/how to do is the question. >> >> So the errata is in the GICv3 ITS/PCI host controller, and we just use the >> in-between SMMU (driver) to provide the workaround. So my initial impression >> is that the PCI host controller would have to be blacklisted IFF behind an >> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it >> sound? > > If that avoids us running into the erratum, then fine by me. I'd obviously > prefer we work-around it since we know how to, but that's up to you. I'm surpsised that you may want more errata workaround code to maintain. Anyway we'll check both approaches and show you how they look and go from there. > >> Please also note that in our SoC we have other devices behind the same SMMU, >> like storage controller, which are not affected or related to the Errata. >> >> BTW, ignoring DT support, are you happy with this patchset? > > Yes, but I won't ack it without the above taken care of. Fair enough. > > Will Thanks, John > > . >
On Wed, Aug 23, 2017 at 05:55:52PM +0100, John Garry wrote: > On 23/08/2017 17:43, Will Deacon wrote: > >On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote: > >>On 23/08/2017 14:24, Will Deacon wrote: > >>>On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote: > >>>>>>>Signed-off-by: Shameer Kolothum > >>>>>><shameerali.kolothum.thodi@huawei.com> > >>>>>>>--- > >>>>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- > >>>>>>>1 file changed, 22 insertions(+), 5 deletions(-) > >>>>>> > >>>>>>Please can you also add a devicetree binding with corresponding > >>>>>>documentation to enable this workaround on non-ACPI based systems too? > >>>>>>It should be straightforward if you update the arm_smmu_options table. > >>>>> > >>>>>As I mentioned before, devicetree was a lower priority and we would definitely > >>>>>submit patch to support that. Even if we update the arm_smmu_options table > >>>>>with DT binding, the generic function to retrieve the MSI address regions only > >>>>>works on ACPI/IORT case now. > >>>>> > >>>> > >>>>Hi Will, > >>>> > >>>>Can you confirm your stance on supporting this workaround for DT as well as > >>>>ACPI? > >>>> > >>>>For us, we now only "officially" support ACPI FW, and DT support at this > >>>>point is patchy/limited. To me, adding DT support is just more errata > >>>>workaround code to maintain with little useful gain. > >>> > >>>I basically don't like the idea of a driver that only works for one of > >>>ACPI or DT yet claims to support both. I'm less fussed about functionality > >>>differences (feature X is only available with firmware Y), but not working > >>>around a hardware erratum that we know about is just lazy. > >>> > >>>So I'd prefer that we handle this in both cases, or blacklist affected > >>>devices when booting with DT. Continuing as though there isn't an erratum > >>>is the worst thing we can do. > >> > >>OK, seems reasonable. > >> > >>We would consider blacklisting the device, where/how to do is the question. > >> > >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the > >>in-between SMMU (driver) to provide the workaround. So my initial impression > >>is that the PCI host controller would have to be blacklisted IFF behind an > >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it > >>sound? > > > >If that avoids us running into the erratum, then fine by me. I'd obviously > >prefer we work-around it since we know how to, but that's up to you. > > I'm surpsised that you may want more errata workaround code to maintain. > > Anyway we'll check both approaches and show you how they look and go from > there. Don't get me wrong, I don't dream about adding errata workarounds to the code, but our job as an operating system is to abstract the hardware from the user, which means dealing with its quirks whether we like it or not. Thanks, Will
On 24/08/2017 15:35, Will Deacon wrote: >>>> > >>OK, seems reasonable. >>>> > >> >>>> > >>We would consider blacklisting the device, where/how to do is the question. >>>> > >> >>>> > >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the >>>> > >>in-between SMMU (driver) to provide the workaround. So my initial impression >>>> > >>is that the PCI host controller would have to be blacklisted IFF behind an >>>> > >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it >>>> > >>sound? >>> > > >>> > >If that avoids us running into the erratum, then fine by me. I'd obviously >>> > >prefer we work-around it since we know how to, but that's up to you. >> > >> > I'm surpsised that you may want more errata workaround code to maintain. >> > >> > Anyway we'll check both approaches and show you how they look and go from >> > there. > Don't get me wrong, I don't dream about adding errata workarounds to the > code, but our job as an operating system is to abstract the hardware from > the user, which means dealing with its quirks whether we like it or not. > Fine, it's ok. For our next platform, hip08, we will provide no DT FW support, so this whole DT grey area should not be an issue. And hopefully no errata also. Much appreciated, John > Thanks, > > Will
On 10/08/2017 18:27, Will Deacon wrote: > On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote: >> The HiSilicon erratum 161010801 describes the limitation of HiSilicon >> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions. >> >> On these platforms GICv3 ITS translator is presented with the deviceID >> by extending the MSI payload data to 64 bits to include the deviceID. >> Hence, the PCIe controller on this platforms has to differentiate the >> MSI payload against other DMA payload and has to modify the MSI payload. >> This basically makes it difficult for this platforms to have a SMMU >> translation for MSI. >> >> This patch implements a ACPI table based quirk to reserve the hw msi >> regions in the smmu-v3 driver which means these address regions will >> not be translated and will be excluded from iova allocations. >> >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> >> --- >> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- >> 1 file changed, 22 insertions(+), 5 deletions(-) > > Please can you also add a devicetree binding with corresponding > documentation to enable this workaround on non-ACPI based systems too? It > should be straightforward if you update the arm_smmu_options table. > Hi Will, Lorenzo, Robin, I have created the patch to add DT support for this erratum. However, currently I have only added support for pci-based devices. I'm a bit stumped on how to add platform device support, or if we should also add support at all. And I would rather ask before sending the patches. The specific issue is that I know of no platform device with an ITS msi-parent which we would want reserve, i.e. do not translate msi. And, if there were, how to do it. The only platform devices I know off with msi ITS parents are SMMUv3 and mbigen. For mbigen, the msi are not translated. Actually, as I see under IORT solution, for mbigen we don't reserve the hw msi region as the SMMUv3 does not have an ID mapping. And we have no equivalent ID mapping property for DT SMMUv3, so cannot create an equivalent check. So here is how the DT iommu get reserved region function with only pci device support looks: /** * of_iommu_its_get_resv_regions - Reserved region driver helper * @dev: Device from iommu_get_resv_regions() * @list: Reserved region list from iommu_get_resv_regions() * * Returns: Number of reserved regions on success(0 if no associated ITS), * appropriate error value otherwise. */ int of_iommu_its_get_resv_regions(struct device *dev, struct list_head *head) { struct device_node *of_node = NULL; struct device *parent_dev; const __be32 *msi_map; int num_mappings, i, err, len, resv = 0; /* walk up to find the parent with a msi-map property */ for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) { if (!parent_dev->of_node) continue; /* * Current logic to reserve ITS regions for only PCI root * complex. */ msi_map = of_get_property(parent_dev->of_node, "msi-map", &len); if (msi_map) { of_node = parent_dev->of_node; break; } } if (!of_node) return 0; num_mappings = of_count_phandle_with_args(of_node, "msi-map", NULL) / 4; for (i = 0; i < num_mappings; i++) { int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; struct iommu_resv_region *region; struct device_node *msi_node; struct resource resource; u32 phandle; phandle = be32_to_cpup(msi_map + 1); msi_node = of_find_node_by_phandle(phandle); if (!msi_node) return -ENODEV; /* * There may be different types of msi-controller, so check * for ITS. */ if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) { of_node_put(msi_node); continue; } err = of_address_to_resource(msi_node, 0, &resource); of_node_put(msi_node); if (err) return err; region = iommu_alloc_resv_region(resource.start, SZ_128K, prot, IOMMU_RESV_MSI); if (region) { list_add_tail(®ion->list, head); resv++; } } return (resv == num_mappings) ? resv : -ENODEV; } Any feedback is appreciated. Thanks, John > Thanks, > > Will > > . >
On Fri, Sep 01, 2017 at 09:46:00AM +0100, John Garry wrote: > On 10/08/2017 18:27, Will Deacon wrote: > >On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote: > >>The HiSilicon erratum 161010801 describes the limitation of HiSilicon > >>platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions. > >> > >>On these platforms GICv3 ITS translator is presented with the deviceID > >>by extending the MSI payload data to 64 bits to include the deviceID. > >>Hence, the PCIe controller on this platforms has to differentiate the > >>MSI payload against other DMA payload and has to modify the MSI payload. > >>This basically makes it difficult for this platforms to have a SMMU > >>translation for MSI. > >> > >>This patch implements a ACPI table based quirk to reserve the hw msi > >>regions in the smmu-v3 driver which means these address regions will > >>not be translated and will be excluded from iova allocations. > >> > >>Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > >>--- > >> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- > >> 1 file changed, 22 insertions(+), 5 deletions(-) > > > >Please can you also add a devicetree binding with corresponding > >documentation to enable this workaround on non-ACPI based systems too? It > >should be straightforward if you update the arm_smmu_options table. > > > > Hi Will, Lorenzo, Robin, > > I have created the patch to add DT support for this erratum. > However, currently I have only added support for pci-based devices. > I'm a bit stumped on how to add platform device support, or if we > should also add support at all. And I would rather ask before > sending the patches. > > The specific issue is that I know of no platform device with an ITS > msi-parent which we would want reserve, i.e. do not translate msi. > And, if there were, how to do it. > > The only platform devices I know off with msi ITS parents are SMMUv3 > and mbigen. For mbigen, the msi are not translated. Actually, as I > see under IORT solution, for mbigen we don't reserve the hw msi > region as the SMMUv3 does not have an ID mapping. And we have no > equivalent ID mapping property for DT SMMUv3, so cannot create an > equivalent check. > > So here is how the DT iommu get reserved region function with only > pci device support looks: > > /** > * of_iommu_its_get_resv_regions - Reserved region driver helper > * @dev: Device from iommu_get_resv_regions() > * @list: Reserved region list from iommu_get_resv_regions() > * > * Returns: Number of reserved regions on success(0 if no associated ITS), > * appropriate error value otherwise. > */ > int of_iommu_its_get_resv_regions(struct device *dev, struct > list_head *head) > { > struct device_node *of_node = NULL; > struct device *parent_dev; > const __be32 *msi_map; > int num_mappings, i, err, len, resv = 0; > > /* walk up to find the parent with a msi-map property */ > for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) { > if (!parent_dev->of_node) > continue; > > /* > * Current logic to reserve ITS regions for only PCI root > * complex. > */ > msi_map = of_get_property(parent_dev->of_node, "msi-map", &len); > if (msi_map) { > of_node = parent_dev->of_node; > break; > } > } > > if (!of_node) > return 0; > > num_mappings = of_count_phandle_with_args(of_node, "msi-map", > NULL) / 4; > > for (i = 0; i < num_mappings; i++) { > int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; > struct iommu_resv_region *region; > struct device_node *msi_node; > struct resource resource; > u32 phandle; > > phandle = be32_to_cpup(msi_map + 1); > msi_node = of_find_node_by_phandle(phandle); > if (!msi_node) > return -ENODEV; > > /* > * There may be different types of msi-controller, so check > * for ITS. > */ > if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) { > of_node_put(msi_node); > continue; > } > > err = of_address_to_resource(msi_node, 0, &resource); > > of_node_put(msi_node); > if (err) > return err; > > region = iommu_alloc_resv_region(resource.start, SZ_128K, prot, > IOMMU_RESV_MSI); > if (region) { > list_add_tail(®ion->list, head); > resv++; > } > } > > return (resv == num_mappings) ? resv : -ENODEV; > } > > Any feedback is appreciated. Hi John, I appreciate it is not trivial to make the code generic, part of the snippet above can be shared between ACPI/IORT and OF, the only sticking point is probably the "compatible" string that has to be parameterized since having this code in generic IOMMU layer is a bit horrible to have it ITS specific (and it is a problem that was existing already in the original patch with the hardcoded ITS node type or function name). To sum it up the hook: - has to be called from SMMU driver in a firmware agnostic way - somehow it has to ascertain which interrupt controller "compatible" (which in IORT is a node type) to match against so the hook has to take an id of sorts I need to go back and have a look at the original patch to see how we can accommodate both OF/ACPI - certainly the region reservations code can and should be shared. Lorenzo
>> >> Hi Will, Lorenzo, Robin, >> >> I have created the patch to add DT support for this erratum. >> However, currently I have only added support for pci-based devices. >> I'm a bit stumped on how to add platform device support, or if we >> should also add support at all. And I would rather ask before >> sending the patches. >> >> The specific issue is that I know of no platform device with an ITS >> msi-parent which we would want reserve, i.e. do not translate msi. >> And, if there were, how to do it. >> >> The only platform devices I know off with msi ITS parents are SMMUv3 >> and mbigen. For mbigen, the msi are not translated. Actually, as I >> see under IORT solution, for mbigen we don't reserve the hw msi >> region as the SMMUv3 does not have an ID mapping. And we have no >> equivalent ID mapping property for DT SMMUv3, so cannot create an >> equivalent check. >> >> So here is how the DT iommu get reserved region function with only >> pci device support looks: >> >> /** >> * of_iommu_its_get_resv_regions - Reserved region driver helper >> * @dev: Device from iommu_get_resv_regions() >> * @list: Reserved region list from iommu_get_resv_regions() >> * >> * Returns: Number of reserved regions on success(0 if no associated ITS), >> * appropriate error value otherwise. >> */ >> int of_iommu_its_get_resv_regions(struct device *dev, struct >> list_head *head) >> { >> struct device_node *of_node = NULL; >> struct device *parent_dev; >> const __be32 *msi_map; >> int num_mappings, i, err, len, resv = 0; >> >> /* walk up to find the parent with a msi-map property */ >> for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) { >> if (!parent_dev->of_node) >> continue; >> >> /* >> * Current logic to reserve ITS regions for only PCI root >> * complex. >> */ >> msi_map = of_get_property(parent_dev->of_node, "msi-map", &len); >> if (msi_map) { >> of_node = parent_dev->of_node; >> break; >> } >> } >> >> if (!of_node) >> return 0; >> >> num_mappings = of_count_phandle_with_args(of_node, "msi-map", >> NULL) / 4; >> >> for (i = 0; i < num_mappings; i++) { >> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; >> struct iommu_resv_region *region; >> struct device_node *msi_node; >> struct resource resource; >> u32 phandle; >> >> phandle = be32_to_cpup(msi_map + 1); >> msi_node = of_find_node_by_phandle(phandle); >> if (!msi_node) >> return -ENODEV; >> >> /* >> * There may be different types of msi-controller, so check >> * for ITS. >> */ >> if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) { >> of_node_put(msi_node); >> continue; >> } >> >> err = of_address_to_resource(msi_node, 0, &resource); >> >> of_node_put(msi_node); >> if (err) >> return err; >> >> region = iommu_alloc_resv_region(resource.start, SZ_128K, prot, >> IOMMU_RESV_MSI); >> if (region) { >> list_add_tail(®ion->list, head); >> resv++; >> } >> } >> >> return (resv == num_mappings) ? resv : -ENODEV; >> } >> >> Any feedback is appreciated. > > Hi John, > > I appreciate it is not trivial to make the code generic, part of the > snippet above can be shared between ACPI/IORT and OF, the only sticking > point is probably the "compatible" string that has to be parameterized > since having this code in generic IOMMU layer is a bit horrible to > have it ITS specific (and it is a problem that was existing already > in the original patch with the hardcoded ITS node type or function > name). Hi Lorenzo, Agreed, checking the ITS compatible string in IOMMU code is not nice. However the function is just trying to replicate our ACPI version, which calls IORT code directly, and this is ITS specific. Anyway, we can make it generic. > > To sum it up the hook: > > - has to be called from SMMU driver in a firmware agnostic way ok > - somehow it has to ascertain which interrupt controller "compatible" > (which in IORT is a node type) to match against so the hook has to > take an id of sorts OK I will mention 2 more points after discussion on OF solution with Shameer: - for platform device, we can add suppport by checking for the devices msi-parent property - for both pci and platform device, we should check device rid for msi-controller also, like IORT solution BTW, even though merge window is open, we would like to send some solution to the lists this week, so any outstanding topics can potentially be discussed at LPC next week. Let me know if you're unhappy about this. All the best, John > > I need to go back and have a look at the original patch to see how > we can accommodate both OF/ACPI - certainly the region reservations > code can and should be shared. > > Lorenzo > > . >
On Tue, Sep 05, 2017 at 12:07:59PM +0100, John Garry wrote: > >> > >>Hi Will, Lorenzo, Robin, > >> > >>I have created the patch to add DT support for this erratum. > >>However, currently I have only added support for pci-based devices. > >>I'm a bit stumped on how to add platform device support, or if we > >>should also add support at all. And I would rather ask before > >>sending the patches. > >> > >>The specific issue is that I know of no platform device with an ITS > >>msi-parent which we would want reserve, i.e. do not translate msi. > >>And, if there were, how to do it. > >> > >>The only platform devices I know off with msi ITS parents are SMMUv3 > >>and mbigen. For mbigen, the msi are not translated. Actually, as I > >>see under IORT solution, for mbigen we don't reserve the hw msi > >>region as the SMMUv3 does not have an ID mapping. And we have no > >>equivalent ID mapping property for DT SMMUv3, so cannot create an > >>equivalent check. > >> > >>So here is how the DT iommu get reserved region function with only > >>pci device support looks: > >> > >>/** > >> * of_iommu_its_get_resv_regions - Reserved region driver helper > >> * @dev: Device from iommu_get_resv_regions() > >> * @list: Reserved region list from iommu_get_resv_regions() > >> * > >> * Returns: Number of reserved regions on success(0 if no associated ITS), > >> * appropriate error value otherwise. > >> */ > >>int of_iommu_its_get_resv_regions(struct device *dev, struct > >>list_head *head) > >>{ > >> struct device_node *of_node = NULL; > >> struct device *parent_dev; > >> const __be32 *msi_map; > >> int num_mappings, i, err, len, resv = 0; > >> > >> /* walk up to find the parent with a msi-map property */ > >> for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) { > >> if (!parent_dev->of_node) > >> continue; > >> > >> /* > >> * Current logic to reserve ITS regions for only PCI root > >> * complex. > >> */ > >> msi_map = of_get_property(parent_dev->of_node, "msi-map", &len); > >> if (msi_map) { > >> of_node = parent_dev->of_node; > >> break; > >> } > >> } > >> > >> if (!of_node) > >> return 0; > >> > >> num_mappings = of_count_phandle_with_args(of_node, "msi-map", > >>NULL) / 4; > >> > >> for (i = 0; i < num_mappings; i++) { > >> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; > >> struct iommu_resv_region *region; > >> struct device_node *msi_node; > >> struct resource resource; > >> u32 phandle; > >> > >> phandle = be32_to_cpup(msi_map + 1); > >> msi_node = of_find_node_by_phandle(phandle); > >> if (!msi_node) > >> return -ENODEV; > >> > >> /* > >> * There may be different types of msi-controller, so check > >> * for ITS. > >> */ > >> if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) { > >> of_node_put(msi_node); > >> continue; > >> } > >> > >> err = of_address_to_resource(msi_node, 0, &resource); > >> > >> of_node_put(msi_node); > >> if (err) > >> return err; > >> > >> region = iommu_alloc_resv_region(resource.start, SZ_128K, prot, > >> IOMMU_RESV_MSI); > >> if (region) { > >> list_add_tail(®ion->list, head); > >> resv++; > >> } > >> } > >> > >> return (resv == num_mappings) ? resv : -ENODEV; > >>} > >> > >>Any feedback is appreciated. > > > >Hi John, > > > >I appreciate it is not trivial to make the code generic, part of the > >snippet above can be shared between ACPI/IORT and OF, the only sticking > >point is probably the "compatible" string that has to be parameterized > >since having this code in generic IOMMU layer is a bit horrible to > >have it ITS specific (and it is a problem that was existing already > >in the original patch with the hardcoded ITS node type or function > >name). > > Hi Lorenzo, > > Agreed, checking the ITS compatible string in IOMMU code is not > nice. However the function is just trying to replicate our ACPI > version, which calls IORT code directly, and this is ITS specific. > Anyway, we can make it generic. > > > > >To sum it up the hook: > > > >- has to be called from SMMU driver in a firmware agnostic way > > ok > > >- somehow it has to ascertain which interrupt controller "compatible" > > (which in IORT is a node type) to match against so the hook has to > > take an id of sorts > > OK > > I will mention 2 more points after discussion on OF solution with Shameer: > - for platform device, we can add suppport by checking for the > devices msi-parent property > - for both pci and platform device, we should check device rid for > msi-controller also, like IORT solution > > BTW, even though merge window is open, we would like to send some > solution to the lists this week, so any outstanding topics can > potentially be discussed at LPC next week. Let me know if you're > unhappy about this. I am happy to find a way forward for this next week, posting patches this week would not help much with the merge window ongoing, I think it is better to try to find a way forward and post the resulting code after reaching an agreement, at -rc1 (or right after the IOMMU PR is merged). Thanks, Lorenzo
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 568c400..6f21dd7 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -608,6 +608,7 @@ struct arm_smmu_device { #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0) #define ARM_SMMU_OPT_PAGE0_REGS_ONLY (1 << 1) +#define ARM_SMMU_OPT_RESV_HW_MSI (1 << 2) u32 options; struct arm_smmu_cmdq cmdq; @@ -1934,14 +1935,29 @@ static void arm_smmu_get_resv_regions(struct device *dev, struct list_head *head) { struct iommu_resv_region *region; + struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv; + struct arm_smmu_device *smmu = master->smmu; int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; + int resv = 0; - region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, - prot, IOMMU_RESV_SW_MSI); - if (!region) - return; + if ((smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)) { - list_add_tail(®ion->list, head); + resv = iommu_dma_get_msi_resv_regions(dev, head); + + if (resv < 0) { + dev_warn(dev, "HW MSI region resv failed: %d\n", resv); + return; + } + } + + if (!resv) { + region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, + prot, IOMMU_RESV_SW_MSI); + if (!region) + return; + + list_add_tail(®ion->list, head); + } iommu_dma_get_resv_regions(dev, head); } @@ -2667,6 +2683,7 @@ static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu) break; case ACPI_IORT_SMMU_HISILICON_HI161X: smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH; + smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI; break; }
The HiSilicon erratum 161010801 describes the limitation of HiSilicon platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions. On these platforms GICv3 ITS translator is presented with the deviceID by extending the MSI payload data to 64 bits to include the deviceID. Hence, the PCIe controller on this platforms has to differentiate the MSI payload against other DMA payload and has to modify the MSI payload. This basically makes it difficult for this platforms to have a SMMU translation for MSI. This patch implements a ACPI table based quirk to reserve the hw msi regions in the smmu-v3 driver which means these address regions will not be translated and will be excluded from iova allocations. Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)