[{"id":1779599,"web_url":"http://patchwork.ozlabs.org/comment/1779599/","msgid":"<5FC3163CFD30C246ABAA99954A238FA83841C405@FRAEML521-MBX.china.huawei.com>","list_archive_url":null,"date":"2017-10-04T10:03:39","subject":"RE: [PATCH v8 0/5] iommu/smmu-v3: Workaround for hisilicon\n\t161010801 erratum(reserve HW MSI)","submitter":{"id":70507,"url":"http://patchwork.ozlabs.org/api/people/70507/","name":"Shameerali Kolothum Thodi","email":"shameerali.kolothum.thodi@huawei.com"},"content":"Hi Will/Lorenzo/Marc,\n\nAny feedback on this series, please? Really appreciate if you can take a look\nand let us know. \n\nThanks,\nShameer\n\n> -----Original Message-----\n> From: Shameerali Kolothum Thodi\n> Sent: Wednesday, September 27, 2017 2:33 PM\n> To: lorenzo.pieralisi@arm.com; marc.zyngier@arm.com;\n> sudeep.holla@arm.com; will.deacon@arm.com; robin.murphy@arm.com;\n> joro@8bytes.org; mark.rutland@arm.com; robh@kernel.org\n> Cc: Gabriele Paoloni ; John Garry\n> ; iommu@lists.linux-foundation.org; linux-arm-\n> kernel@lists.infradead.org; linux-acpi@vger.kernel.org;\n> devicetree@vger.kernel.org; devel@acpica.org; Linuxarm\n> ; Wangzhou (B) ;\n> Guohanjun (Hanjun Guo) ; Shameerali Kolothum\n> Thodi \n> Subject: [PATCH v8 0/5] iommu/smmu-v3: Workaround for hisilicon\n> 161010801 erratum(reserve HW MSI)\n> \n> On certain HiSilicon platforms (hip06/hip07) the GIC ITS and PCIe RC\n> deviates from the standard implementation and this breaks PCIe MSI\n> functionality when SMMU is enabled.\n> \n> The HiSilicon erratum 161010801 describes this limitation of certain\n> HiSilicon platforms to support the SMMU mappings for MSI transactions.\n> On these platforms GICv3 ITS translator is presented with the deviceID\n> by extending the MSI payload data to 64 bits to include the deviceID.\n> Hence, the PCIe controller on this platforms has to differentiate the MSI\n> payload against other DMA payload and has to modify the MSI payload.\n> This basically makes it difficult for this platforms to have a SMMU\n> translation for MSI.\n> \n> This patch implements an ACPI and DT based quirk to reserve the hw msi\n> regions in the smmu-v3 driver which means these address regions will\n> not be translated and will be excluded from iova allocations.\n> \n> To implement this quirk, the following changes are incorporated:\n> 1. Added a generic helper function to IORT code to retrieve the\n> associated ITS base address from a device IORT node.\n> 2. Added a generic helper function to of iommu code to retrieve the\n> associated msi controller base address from for a PCI RC\n> msi-mapping and also platform device msi-parent.\n> 3. Added quirk to SMMUv3 to retrieve the HW ITS address and replace\n> the default SW MSI reserve address based on the IORT SMMU model\n> or DT bindings.\n> \n> Changelog:\n> \n> v7 --> v8\n> Addressed comments from Rob and Lorenzo:\n> -Modified to use DT compatible string for errata.\n> -Changed logic to retrieve the msi-parent for DT case.\n> \n> v6 --> v7\n> Addressed request from Will to add DT support for the erratum:\n> - added bt binding\n> - add of_iommu_msi_get_resv_regions()\n> New arm64 silicon errata entry\n> Rename iort_iommu_{its->msi}_get_resv_regions\n> \n> v5 --> v6\n> Addressed comments from Robin and Lorenzo:\n> -No change to patch#1 .\n> -Reverted v5 patch#2 as this might break the platforms where this quirk\n> is not applicable. Provided a generic function in iommu code and added\n> back the quirk implementation in SMMU v3 driver(patch#3)\n> \n> v4 --> v5\n> Addressed comments from Robin and Lorenzo:\n> -Added a comment to make it clear that, for now, only straightforward\n> HW topologies are handled while reserving ITS regions(patch #1).\n> \n> v3 --> v4\n> Rebased on 4.13-rc1.\n> Addressed comments from Robin, Will and Lorenzo:\n> -As suggested by Robin, moved the ITS msi reservation into\n> iommu_dma_get_resv_regions().\n> -Added its_count != resv region failure case(patch #1).\n> \n> v2 --> v3\n> Addressed comments from Lorenzo and Robin:\n> -Removed dev_is_pci() check in smmuV3 driver.\n> -Don't treat device not having an ITS mapping as an error in\n> iort helper function.\n> \n> v1 --> v2\n> -patch 2/2: Invoke iort helper fn based on fwnode type(acpi).\n> \n> RFCv2 -->PATCH\n> -Incorporated Lorenzo's review comments.\n> \n> RFC v1 --> RFC v2\n> Based on Robin's review comments,\n> -Removed the generic erratum framework.\n> -Using IORT/MADT tables to retrieve the ITS base addr instead of vendor\n> specific CSRT table.\n> \n> John Garry (2):\n> Doc: iommu/arm-smmu-v3: Add workaround for HiSilicon erratum\n> 161010801\n> iommu/of: Add msi address regions reservation helper\n> \n> Shameer Kolothum (3):\n> ACPI/IORT: Add msi address regions reservation helper\n> iommu/dma: Add a helper function to reserve HW MSI address regions for\n> IOMMU drivers\n> iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801\n> \n> Documentation/arm64/silicon-errata.txt | 1 +\n> .../devicetree/bindings/iommu/arm,smmu-v3.txt | 9 +-\n> drivers/acpi/arm64/iort.c | 96 +++++++++++++++++++++-\n> drivers/iommu/arm-smmu-v3.c | 41 +++++++--\n> drivers/iommu/dma-iommu.c | 19 +++++\n> drivers/iommu/of_iommu.c | 95 +++++++++++++++++++++\n> drivers/irqchip/irq-gic-v3-its.c | 3 +-\n> include/linux/acpi_iort.h | 7 +-\n> include/linux/dma-iommu.h | 7 ++\n> include/linux/of_iommu.h | 10 +++\n> 10 files changed, 276 insertions(+), 12 deletions(-)\n> \n> --\n> 1.9.1\n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y6WjC4TfRz9t2W\n\tfor ;\n\tWed, 4 Oct 2017 21:04:47 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751296AbdJDKEq convert rfc822-to-8bit (ORCPT\n\t);\n\tWed, 4 Oct 2017 06:04:46 -0400","from lhrrgout.huawei.com ([194.213.3.17]:37142 \"EHLO\n\tlhrrgout.huawei.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751272AbdJDKEo (ORCPT\n\t); Wed, 4 Oct 2017 06:04:44 -0400","from 172.18.7.190 (EHLO lhreml701-cah.china.huawei.com)\n\t([172.18.7.190])\n\tby lhrrg01-dlp.huawei.com (MOS 4.3.7-GA FastPath queued)\n\twith ESMTP id DWV07584; Wed, 04 Oct 2017 10:03:54 +0000 (GMT)","from FRAEML704-CAH.china.huawei.com (10.206.14.35) by\n\tlhreml701-cah.china.huawei.com (10.201.108.42) with Microsoft SMTP\n\tServer (TLS) id 14.3.301.0; Wed, 4 Oct 2017 11:03:46 +0100","from FRAEML521-MBX.china.huawei.com ([169.254.1.161]) by\n\tFRAEML704-CAH.china.huawei.com ([10.206.14.35]) with mapi id\n\t14.03.0301.000; Wed, 4 Oct 2017 12:03:39 +0200"],"From":"Shameerali Kolothum Thodi ","To":"\"lorenzo.pieralisi@arm.com\" ,\n\t\"marc.zyngier@arm.com\" ,\n\t\"sudeep.holla@arm.com\" ,\n\t\"will.deacon@arm.com\" ,\n\t\"robin.murphy@arm.com\" ,\n\t\"joro@8bytes.org\" ,\n\t\"mark.rutland@arm.com\" ,\n\t\"robh@kernel.org\" ","CC":"Gabriele Paoloni ,\n\tJohn Garry ,\n\t\"iommu@lists.linux-foundation.org\" ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-acpi@vger.kernel.org\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"devel@acpica.org\" , Linuxarm ,\n\t\"Wangzhou (B)\" ,\n\t\"Guohanjun (Hanjun Guo)\" ","Subject":"RE: [PATCH v8 0/5] iommu/smmu-v3: Workaround for hisilicon\n\t161010801 erratum(reserve HW MSI)","Thread-Topic":"[PATCH v8 0/5] iommu/smmu-v3: Workaround for hisilicon\n\t161010801 erratum(reserve HW MSI)","Thread-Index":"AQHTN5Vus0vofXRwg0eovrwp4TeguqLTgBxw","Date":"Wed, 4 Oct 2017 10:03:39 +0000","Message-ID":"<5FC3163CFD30C246ABAA99954A238FA83841C405@FRAEML521-MBX.china.huawei.com>","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>","In-Reply-To":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>","Accept-Language":"en-GB, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.203.177.212]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-CFilter-Loop":"Reflected","X-Mirapoint-Virus-RAPID-Raw":"score=unknown(0),\n\trefid=str=0001.0A090206.59D4B20E.00A1, ss=1, re=0.000, recu=0.000,\n\treip=0.000, \n\tcl=1, cld=1, fgs=0, ip=169.254.1.161,\n\tso=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32","X-Mirapoint-Loop-Id":"1c21f63cb83ff8ce99d7d0f9125ddc0d","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1779712,"web_url":"http://patchwork.ozlabs.org/comment/1779712/","msgid":"","list_archive_url":null,"date":"2017-10-04T11:22:04","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":7353,"url":"http://patchwork.ozlabs.org/api/people/7353/","name":"Marc Zyngier","email":"marc.zyngier@arm.com"},"content":"On 27/09/17 14:32, Shameer Kolothum wrote:\n> From: John Garry \n> \n> On some platforms msi-controller address regions have to be excluded\n> from normal IOVA allocation in that they are detected and decoded in\n> a HW specific way by system components and so they cannot be considered\n> normal IOVA address space.\n> \n> Add a helper function that retrieves msi address regions through device\n> tree msi mapping, so that these regions will not be translated by IOMMU\n> and will be excluded from IOVA allocations.\n> \n> Signed-off-by: John Garry \n> [Shameer: Modified msi-parent retrieval logic]\n> Signed-off-by: Shameer Kolothum \n> ---\n> drivers/iommu/of_iommu.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++\n> include/linux/of_iommu.h | 10 +++++\n> 2 files changed, 105 insertions(+)\n> \n> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n> index e60e3db..ffd7fb7 100644\n> --- a/drivers/iommu/of_iommu.c\n> +++ b/drivers/iommu/of_iommu.c\n> @@ -21,6 +21,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n> \treturn ops->of_xlate(dev, iommu_spec);\n> }\n> \n> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n> +{\n> +\tu32 *rid = data;\n> +\n> +\t*rid = alias;\n> +\treturn 0;\n> +}\n> +\n> struct of_pci_iommu_alias_info {\n> \tstruct device *dev;\n> \tstruct device_node *np;\n> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)\n> \treturn info->np == pdev->bus->dev.of_node;\n> }\n> \n> +static int of_iommu_alloc_resv_region(struct device_node *np,\n> +\t\t\t\t struct list_head *head)\n> +{\n> +\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n> +\tstruct iommu_resv_region *region;\n> +\tstruct resource res;\n> +\tint err;\n> +\n> +\terr = of_address_to_resource(np, 0, &res);\n\nWhat is the rational for registering the first region only? Surely you\ncan have more than one region in an MSI controller (yes, your particular\ncase is the ITS which has only one, but we're trying to do something\ngeneric here).\n\nAnother issue I have with this code is that it inserts all of the ITS\nMMIO in the RESV_MSI range. As long as we don't generate any page tables\nfor this, we're fine. But if we ever change this, we'll end-up with the\nITS programming interface being exposed to a device, which wouldn't be\nacceptable.\n\nWhy can't we have some generic property instead, that would describe the\nactual ranges that cannot be translated? That way, no random insertion\nof a random range, and relatively future proof.\n\nI'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel\nlike a much nicer and simpler solution to this problem.\n\nThoughts?\n\n\tM.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y6YQZ1h3mz9t2h\n\tfor ;\n\tWed, 4 Oct 2017 22:22:14 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751665AbdJDLWM (ORCPT\n\t);\n\tWed, 4 Oct 2017 07:22:12 -0400","from foss.arm.com ([217.140.101.70]:60020 \"EHLO foss.arm.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751557AbdJDLWM (ORCPT );\n\tWed, 4 Oct 2017 07:22:12 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AA9D61435;\n\tWed, 4 Oct 2017 04:22:11 -0700 (PDT)","from [10.1.206.41] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t183123F53D; Wed, 4 Oct 2017 04:22:05 -0700 (PDT)"],"Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","To":"Shameer Kolothum ,\n\tlorenzo.pieralisi@arm.com, sudeep.holla@arm.com,\n\twill.deacon@arm.com, robin.murphy@arm.com, joro@8bytes.org,\n\tmark.rutland@arm.com, robh@kernel.org","Cc":"gabriele.paoloni@huawei.com, john.garry@huawei.com,\n\tiommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>","From":"Marc Zyngier ","Organization":"ARM Ltd","Message-ID":"","Date":"Wed, 4 Oct 2017 12:22:04 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1779827,"web_url":"http://patchwork.ozlabs.org/comment/1779827/","msgid":"<20171004135007.GA12327@red-moon>","list_archive_url":null,"date":"2017-10-04T13:50:07","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":5388,"url":"http://patchwork.ozlabs.org/api/people/5388/","name":"Lorenzo Pieralisi","email":"Lorenzo.Pieralisi@arm.com"},"content":"On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n> On 27/09/17 14:32, Shameer Kolothum wrote:\n> > From: John Garry \n> > \n> > On some platforms msi-controller address regions have to be excluded\n> > from normal IOVA allocation in that they are detected and decoded in\n> > a HW specific way by system components and so they cannot be considered\n> > normal IOVA address space.\n> > \n> > Add a helper function that retrieves msi address regions through device\n> > tree msi mapping, so that these regions will not be translated by IOMMU\n> > and will be excluded from IOVA allocations.\n> > \n> > Signed-off-by: John Garry \n> > [Shameer: Modified msi-parent retrieval logic]\n> > Signed-off-by: Shameer Kolothum \n> > ---\n> > drivers/iommu/of_iommu.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++\n> > include/linux/of_iommu.h | 10 +++++\n> > 2 files changed, 105 insertions(+)\n> > \n> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n> > index e60e3db..ffd7fb7 100644\n> > --- a/drivers/iommu/of_iommu.c\n> > +++ b/drivers/iommu/of_iommu.c\n> > @@ -21,6 +21,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n> > \treturn ops->of_xlate(dev, iommu_spec);\n> > }\n> > \n> > +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n> > +{\n> > +\tu32 *rid = data;\n> > +\n> > +\t*rid = alias;\n> > +\treturn 0;\n> > +}\n> > +\n> > struct of_pci_iommu_alias_info {\n> > \tstruct device *dev;\n> > \tstruct device_node *np;\n> > @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)\n> > \treturn info->np == pdev->bus->dev.of_node;\n> > }\n> > \n> > +static int of_iommu_alloc_resv_region(struct device_node *np,\n> > +\t\t\t\t struct list_head *head)\n> > +{\n> > +\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n> > +\tstruct iommu_resv_region *region;\n> > +\tstruct resource res;\n> > +\tint err;\n> > +\n> > +\terr = of_address_to_resource(np, 0, &res);\n> \n> What is the rational for registering the first region only? Surely you\n> can have more than one region in an MSI controller (yes, your particular\n> case is the ITS which has only one, but we're trying to do something\n> generic here).\n> \n> Another issue I have with this code is that it inserts all of the ITS\n> MMIO in the RESV_MSI range. As long as we don't generate any page tables\n> for this, we're fine. But if we ever change this, we'll end-up with the\n> ITS programming interface being exposed to a device, which wouldn't be\n> acceptable.\n> \n> Why can't we have some generic property instead, that would describe the\n> actual ranges that cannot be translated? That way, no random insertion\n> of a random range, and relatively future proof.\n\nIORT code has the same issue (ie it reserves all ITS regions) and I do\nnot know where a property can be added to describe those ranges (IORT\nspecs ? I'd rather not) in ACPI other than the IORT tables entries\nthemselves.\n\n> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel\n> like a much nicer and simpler solution to this problem.\n\nIt could be but if we throw ACPI into the picture we have to knock\ntogether Hisilicon specific namespace bindings to handle this and\nquickly.\n\nLorenzo\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y6cfJ37XPz9t2l\n\tfor ;\n\tThu, 5 Oct 2017 00:47:36 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752084AbdJDNre (ORCPT\n\t);\n\tWed, 4 Oct 2017 09:47:34 -0400","from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:33584 \"EHLO\n\tfoss.arm.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751997AbdJDNre (ORCPT );\n\tWed, 4 Oct 2017 09:47:34 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 97DB21435;\n\tWed, 4 Oct 2017 06:47:33 -0700 (PDT)","from red-moon (red-moon.cambridge.arm.com [10.1.206.55])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t8CF9B3F483; Wed, 4 Oct 2017 06:47:30 -0700 (PDT)"],"Date":"Wed, 4 Oct 2017 14:50:07 +0100","From":"Lorenzo Pieralisi ","To":"Marc Zyngier ","Cc":"Shameer Kolothum ,\n\tsudeep.holla@arm.com, will.deacon@arm.com, robin.murphy@arm.com,\n\tjoro@8bytes.org, mark.rutland@arm.com, robh@kernel.org,\n\tgabriele.paoloni@huawei.com, john.garry@huawei.com,\n\tiommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","Message-ID":"<20171004135007.GA12327@red-moon>","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1779867,"web_url":"http://patchwork.ozlabs.org/comment/1779867/","msgid":"<001729e8-7b34-0bb4-8af6-8af100661906@arm.com>","list_archive_url":null,"date":"2017-10-04T14:17:40","subject":"Re: [PATCH v8 2/5] ACPI/IORT: Add msi address regions reservation\n\thelper","submitter":{"id":7353,"url":"http://patchwork.ozlabs.org/api/people/7353/","name":"Marc Zyngier","email":"marc.zyngier@arm.com"},"content":"On 27/09/17 14:32, Shameer Kolothum wrote:\n> On some platforms msi parent address regions have to be excluded from\n> normal IOVA allocation in that they are detected and decoded in a HW\n> specific way by system components and so they cannot be considered normal\n> IOVA address space.\n> \n> Add a helper function that retrieves ITS address regions - the msi\n> parent - through IORT device <-> ITS mappings and reserves it so that\n> these regions will not be translated by IOMMU and will be excluded from\n> IOVA allocations.\n> \n> Signed-off-by: Shameer Kolothum \n> [lorenzo.pieralisi@arm.com: updated commit log/added comments]\n> Signed-off-by: Lorenzo Pieralisi \n> ---\n> drivers/acpi/arm64/iort.c | 96 ++++++++++++++++++++++++++++++++++++++--\n> drivers/irqchip/irq-gic-v3-its.c | 3 +-\n> include/linux/acpi_iort.h | 7 ++-\n> 3 files changed, 101 insertions(+), 5 deletions(-)\n> \n> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c\n> index 9565d57..14efa9d 100644\n> --- a/drivers/acpi/arm64/iort.c\n> +++ b/drivers/acpi/arm64/iort.c\n> @@ -39,6 +39,7 @@\n> struct iort_its_msi_chip {\n> \tstruct list_head\tlist;\n> \tstruct fwnode_handle\t*fw_node;\n> +\tphys_addr_t\t\tbase_addr;\n> \tu32\t\t\ttranslation_id;\n> };\n> \n> @@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)\n> static DEFINE_SPINLOCK(iort_msi_chip_lock);\n> \n> /**\n> - * iort_register_domain_token() - register domain token and related ITS ID\n> - * to the list from where we can get it back later on.\n> + * iort_register_domain_token() - register domain token along with related\n> + * ITS ID and base address to the list from where we can get it back later on.\n> * @trans_id: ITS ID.\n> + * @base: ITS base address.\n> * @fw_node: Domain token.\n> *\n> * Returns: 0 on success, -ENOMEM if no memory when allocating list element\n> */\n> -int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)\n> +int iort_register_domain_token(int trans_id, phys_addr_t base,\n> +\t\t\t struct fwnode_handle *fw_node)\n> {\n> \tstruct iort_its_msi_chip *its_msi_chip;\n> \n> @@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)\n> \n> \tits_msi_chip->fw_node = fw_node;\n> \tits_msi_chip->translation_id = trans_id;\n> +\tits_msi_chip->base_addr = base;\n> \n> \tspin_lock(&iort_msi_chip_lock);\n> \tlist_add(&its_msi_chip->list, &iort_msi_chip_list);\n> @@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)\n> \treturn -ENODEV;\n> }\n> \n> +static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)\n> +{\n> +\tstruct iort_its_msi_chip *its_msi_chip;\n> +\tbool match = false;\n> +\n> +\tspin_lock(&iort_msi_chip_lock);\n> +\tlist_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {\n> +\t\tif (its_msi_chip->translation_id == its_id) {\n> +\t\t\t*base = its_msi_chip->base_addr;\n> +\t\t\tmatch = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\tspin_unlock(&iort_msi_chip_lock);\n> +\n> +\treturn match ? 0 : -ENODEV;\n> +}\n> +\n> /**\n> * iort_dev_find_its_id() - Find the ITS identifier for a device\n> * @dev: The device.\n> @@ -639,6 +661,72 @@ int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)\n> \n> \treturn err;\n> }\n> +\n> +/**\n> + * iort_iommu_msi_get_resv_regions - Reserved region driver helper\n> + * @dev: Device from iommu_get_resv_regions()\n> + * @head: Reserved region list from iommu_get_resv_regions()\n> + *\n> + * Returns: Number of reserved regions on success (0 if no associated msi\n> + * regions), appropriate error value otherwise. The ITS regions\n> + * associated with the device are the msi reserved regions.\n> + */\n> +int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)\n> +{\n> +\tstruct acpi_iort_its_group *its;\n> +\tstruct acpi_iort_node *node, *its_node = NULL;\n> +\tint i, resv = 0;\n> +\n> +\tnode = iort_find_dev_node(dev);\n> +\tif (!node)\n> +\t\treturn -ENODEV;\n> +\n> +\t/*\n> +\t * Current logic to reserve ITS regions relies on HW topologies\n> +\t * where a given PCI or named component maps its IDs to only one\n> +\t * ITS group; if a PCI or named component can map its IDs to\n> +\t * different ITS groups through IORT mappings this function has\n> +\t * to be reworked to ensure we reserve regions for all ITS groups\n> +\t * a given PCI or named component may map IDs to.\n> +\t */\n> +\tif (dev_is_pci(dev)) {\n> +\t\tu32 rid;\n> +\n> +\t\tpci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, &rid);\n> +\t\tits_node = iort_node_map_id(node, rid, NULL, IORT_MSI_TYPE);\n> +\t} else {\n> +\t\tfor (i = 0; i < node->mapping_count; i++) {\n> +\t\t\tits_node = iort_node_map_platform_id(node, NULL,\n> +\t\t\t\t\t\t\t IORT_MSI_TYPE, i);\n> +\t\t\tif (its_node)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!its_node)\n> +\t\treturn 0;\n> +\n> +\t/* Move to ITS specific data */\n> +\tits = (struct acpi_iort_its_group *)its_node->node_data;\n> +\n> +\tfor (i = 0; i < its->its_count; i++) {\n> +\t\tphys_addr_t base;\n> +\n> +\t\tif (!iort_find_its_base(its->identifiers[i], &base)) {\n> +\t\t\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n> +\t\t\tstruct iommu_resv_region *region;\n> +\n> +\t\t\tregion = iommu_alloc_resv_region(base, SZ_128K, prot,\n> +\t\t\t\t\t\t\t IOMMU_RESV_MSI);\n\nSame as the OF part: I strongly object to reserving the whole 128kB\nrange. What we really care about is the second 64kB page, and that is\nwhat should get reserved.\n\nThanks,\n\n\tM.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y6dKD6Kg9z9sP1\n\tfor ;\n\tThu, 5 Oct 2017 01:17:52 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752407AbdJDORu (ORCPT\n\t);\n\tWed, 4 Oct 2017 10:17:50 -0400","from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:33990 \"EHLO\n\tfoss.arm.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752366AbdJDORs (ORCPT );\n\tWed, 4 Oct 2017 10:17:48 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 27D6D1435;\n\tWed, 4 Oct 2017 07:17:48 -0700 (PDT)","from [10.1.206.41] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t5F2073F483; Wed, 4 Oct 2017 07:17:42 -0700 (PDT)"],"Subject":"Re: [PATCH v8 2/5] ACPI/IORT: Add msi address regions reservation\n\thelper","To":"Shameer Kolothum ,\n\tlorenzo.pieralisi@arm.com, sudeep.holla@arm.com,\n\twill.deacon@arm.com, robin.murphy@arm.com, joro@8bytes.org,\n\tmark.rutland@arm.com, robh@kernel.org","Cc":"gabriele.paoloni@huawei.com, john.garry@huawei.com,\n\tiommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-3-shameerali.kolothum.thodi@huawei.com>","From":"Marc Zyngier ","Organization":"ARM Ltd","Message-ID":"<001729e8-7b34-0bb4-8af6-8af100661906@arm.com>","Date":"Wed, 4 Oct 2017 15:17:40 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170927133241.21036-3-shameerali.kolothum.thodi@huawei.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1779996,"web_url":"http://patchwork.ozlabs.org/comment/1779996/","msgid":"<5FC3163CFD30C246ABAA99954A238FA83841C809@FRAEML521-MBX.china.huawei.com>","list_archive_url":null,"date":"2017-10-04T17:07:15","subject":"RE: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":70507,"url":"http://patchwork.ozlabs.org/api/people/70507/","name":"Shameerali Kolothum Thodi","email":"shameerali.kolothum.thodi@huawei.com"},"content":"> -----Original Message-----\r\n> From: Marc Zyngier [mailto:marc.zyngier@arm.com]\r\n> Sent: Wednesday, October 04, 2017 12:22 PM\r\n> To: Shameerali Kolothum Thodi ;\r\n> lorenzo.pieralisi@arm.com; sudeep.holla@arm.com; will.deacon@arm.com;\r\n> robin.murphy@arm.com; joro@8bytes.org; mark.rutland@arm.com;\r\n> robh@kernel.org\r\n> Cc: Gabriele Paoloni ; John Garry\r\n> ; iommu@lists.linux-foundation.org; linux-arm-\r\n> kernel@lists.infradead.org; linux-acpi@vger.kernel.org;\r\n> devicetree@vger.kernel.org; devel@acpica.org; Linuxarm\r\n> ; Wangzhou (B) ;\r\n> Guohanjun (Hanjun Guo) \r\n> Subject: Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\r\n> helper\r\n> \r\n> On 27/09/17 14:32, Shameer Kolothum wrote:\r\n> > From: John Garry \r\n> >\r\n> > On some platforms msi-controller address regions have to be excluded\r\n> > from normal IOVA allocation in that they are detected and decoded in a\r\n> > HW specific way by system components and so they cannot be considered\r\n> > normal IOVA address space.\r\n> >\r\n> > Add a helper function that retrieves msi address regions through\r\n> > device tree msi mapping, so that these regions will not be translated\r\n> > by IOMMU and will be excluded from IOVA allocations.\r\n> >\r\n> > Signed-off-by: John Garry \r\n> > [Shameer: Modified msi-parent retrieval logic]\r\n> > Signed-off-by: Shameer Kolothum\r\n> \r\n> > ---\r\n> > drivers/iommu/of_iommu.c | 95\r\n> > ++++++++++++++++++++++++++++++++++++++++++++++++\r\n> > include/linux/of_iommu.h | 10 +++++\r\n> > 2 files changed, 105 insertions(+)\r\n> >\r\n> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\r\n> index\r\n> > e60e3db..ffd7fb7 100644\r\n> > --- a/drivers/iommu/of_iommu.c\r\n> > +++ b/drivers/iommu/of_iommu.c\r\n> > @@ -21,6 +21,7 @@\r\n> > #include \r\n> > #include \r\n> > #include \r\n> > +#include \r\n> > #include \r\n> > #include \r\n> > #include \r\n> > @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\r\n> > \treturn ops->of_xlate(dev, iommu_spec); }\r\n> >\r\n> > +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\r\n> > +{\r\n> > +\tu32 *rid = data;\r\n> > +\r\n> > +\t*rid = alias;\r\n> > +\treturn 0;\r\n> > +}\r\n> > +\r\n> > struct of_pci_iommu_alias_info {\r\n> > \tstruct device *dev;\r\n> > \tstruct device_node *np;\r\n> > @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev,\r\n> u16 alias, void *data)\r\n> > \treturn info->np == pdev->bus->dev.of_node; }\r\n> >\r\n> > +static int of_iommu_alloc_resv_region(struct device_node *np,\r\n> > +\t\t\t\t struct list_head *head)\r\n> > +{\r\n> > +\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\r\n> > +\tstruct iommu_resv_region *region;\r\n> > +\tstruct resource res;\r\n> > +\tint err;\r\n> > +\r\n> > +\terr = of_address_to_resource(np, 0, &res);\r\n> \r\n> What is the rational for registering the first region only? Surely you can have\r\n> more than one region in an MSI controller (yes, your particular case is the ITS\r\n> which has only one, but we're trying to do something generic here).\r\n\r\nOk. \r\n\r\n> Another issue I have with this code is that it inserts all of the ITS MMIO in the\r\n> RESV_MSI range. As long as we don't generate any page tables for this, we're\r\n> fine. But if we ever change this, we'll end-up with the ITS programming\r\n> interface being exposed to a device, which wouldn't be acceptable.\r\n\r\nI understand the concern of reserving the whole of ITS MMIO region. Sorry, \r\nbut just being curious, how this will be exposed to a device ? You mean a device\r\ncan be configured to access the ITS MMIO region and it will fail because there is\r\nno SMMU mapping for that?\r\n \r\n> Why can't we have some generic property instead, that would describe the\r\n> actual ranges that cannot be translated? That way, no random insertion of a\r\n> random range, and relatively future proof.\r\n> \r\n> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel like\r\n> a much nicer and simpler solution to this problem.\r\n\r\nI am not sure that will be seen as legitimizing the untranslated regions or not.\r\nWe had this discussion to include the regions specified in IORT spec and the\r\nanswer was that, it will in a way legitimize this and encourage future\r\nimplementations.\r\n\r\nHow about making the _msi_get_resv_regions() function be very specific to GIC\r\nITS like _its_msi_get_resv_regions() ? Is that something we can consider?\r\n(In fact we had a checking for \"arm, gic-v3-its\" in a previous version of this series).\r\n\r\nThanks,\r\nShameer","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y6j5q0PDzz9t1G\n\tfor ;\n\tThu, 5 Oct 2017 04:08:15 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751477AbdJDRIN (ORCPT\n\t);\n\tWed, 4 Oct 2017 13:08:13 -0400","from lhrrgout.huawei.com ([194.213.3.17]:37211 \"EHLO\n\tlhrrgout.huawei.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751240AbdJDRIJ (ORCPT\n\t); Wed, 4 Oct 2017 13:08:09 -0400","from 172.18.7.190 (EHLO lhreml704-cah.china.huawei.com)\n\t([172.18.7.190])\n\tby lhrrg02-dlp.huawei.com (MOS 4.3.7-GA FastPath queued)\n\twith ESMTP id DPX24069; Wed, 04 Oct 2017 17:07:26 +0000 (GMT)","from FRAEML701-CAH.china.huawei.com (10.206.14.32) by\n\tlhreml704-cah.china.huawei.com (10.201.108.45) with Microsoft SMTP\n\tServer (TLS) id 14.3.301.0; Wed, 4 Oct 2017 18:07:22 +0100","from FRAEML521-MBX.china.huawei.com ([169.254.1.161]) by\n\tFRAEML701-CAH.china.huawei.com ([10.206.14.32]) with mapi id\n\t14.03.0301.000; Wed, 4 Oct 2017 19:07:16 +0200"],"From":"Shameerali Kolothum Thodi ","To":"Marc Zyngier ,\n\t\"lorenzo.pieralisi@arm.com\" ,\n\t\"sudeep.holla@arm.com\" ,\n\t\"will.deacon@arm.com\" ,\n\t\"robin.murphy@arm.com\" ,\n\t\"joro@8bytes.org\" ,\n\t\"mark.rutland@arm.com\" ,\n\t\"robh@kernel.org\" ","CC":"Gabriele Paoloni ,\n\tJohn Garry ,\n\t\"iommu@lists.linux-foundation.org\" ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-acpi@vger.kernel.org\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"devel@acpica.org\" , Linuxarm ,\n\t\"Wangzhou (B)\" ,\n\t\"Guohanjun (Hanjun Guo)\" ","Subject":"RE: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","Thread-Topic":"[PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","Thread-Index":"AQHTN5V3OXrIH2YcaU+WYXgS347Gg6LTdVAAgAB8fVA=","Date":"Wed, 4 Oct 2017 17:07:15 +0000","Message-ID":"<5FC3163CFD30C246ABAA99954A238FA83841C809@FRAEML521-MBX.china.huawei.com>","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t","In-Reply-To":"","Accept-Language":"en-GB, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.202.227.237]","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-CFilter-Loop":"Reflected","X-Mirapoint-Virus-RAPID-Raw":"score=unknown(0),\n\trefid=str=0001.0A090206.59D51551.005A, ss=1, re=0.000, recu=0.000,\n\treip=0.000, \n\tcl=1, cld=1, fgs=0, ip=169.254.1.161,\n\tso=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32","X-Mirapoint-Loop-Id":"09bf6db45bad6cb9b30092810f64a2f4","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1780472,"web_url":"http://patchwork.ozlabs.org/comment/1780472/","msgid":"<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>","list_archive_url":null,"date":"2017-10-05T11:07:26","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":65641,"url":"http://patchwork.ozlabs.org/api/people/65641/","name":"Robin Murphy","email":"robin.murphy@arm.com"},"content":"On 04/10/17 14:50, Lorenzo Pieralisi wrote:\n> On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n>> On 27/09/17 14:32, Shameer Kolothum wrote:\n>>> From: John Garry \n>>>\n>>> On some platforms msi-controller address regions have to be excluded\n>>> from normal IOVA allocation in that they are detected and decoded in\n>>> a HW specific way by system components and so they cannot be considered\n>>> normal IOVA address space.\n>>>\n>>> Add a helper function that retrieves msi address regions through device\n>>> tree msi mapping, so that these regions will not be translated by IOMMU\n>>> and will be excluded from IOVA allocations.\n>>>\n>>> Signed-off-by: John Garry \n>>> [Shameer: Modified msi-parent retrieval logic]\n>>> Signed-off-by: Shameer Kolothum \n>>> ---\n>>> drivers/iommu/of_iommu.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++\n>>> include/linux/of_iommu.h | 10 +++++\n>>> 2 files changed, 105 insertions(+)\n>>>\n>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n>>> index e60e3db..ffd7fb7 100644\n>>> --- a/drivers/iommu/of_iommu.c\n>>> +++ b/drivers/iommu/of_iommu.c\n>>> @@ -21,6 +21,7 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n>>> \treturn ops->of_xlate(dev, iommu_spec);\n>>> }\n>>> \n>>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n>>> +{\n>>> +\tu32 *rid = data;\n>>> +\n>>> +\t*rid = alias;\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> struct of_pci_iommu_alias_info {\n>>> \tstruct device *dev;\n>>> \tstruct device_node *np;\n>>> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)\n>>> \treturn info->np == pdev->bus->dev.of_node;\n>>> }\n>>> \n>>> +static int of_iommu_alloc_resv_region(struct device_node *np,\n>>> +\t\t\t\t struct list_head *head)\n>>> +{\n>>> +\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n>>> +\tstruct iommu_resv_region *region;\n>>> +\tstruct resource res;\n>>> +\tint err;\n>>> +\n>>> +\terr = of_address_to_resource(np, 0, &res);\n>>\n>> What is the rational for registering the first region only? Surely you\n>> can have more than one region in an MSI controller (yes, your particular\n>> case is the ITS which has only one, but we're trying to do something\n>> generic here).\n>>\n>> Another issue I have with this code is that it inserts all of the ITS\n>> MMIO in the RESV_MSI range. As long as we don't generate any page tables\n>> for this, we're fine. But if we ever change this, we'll end-up with the\n>> ITS programming interface being exposed to a device, which wouldn't be\n>> acceptable.\n>>\n>> Why can't we have some generic property instead, that would describe the\n>> actual ranges that cannot be translated? That way, no random insertion\n>> of a random range, and relatively future proof.\n\nIndeed. Furthermore, IORT has the advantage of being limited to a few\ndistinct device types and SBSA-compliant system topologies, so the\nITS-chasing is reasonable there (modulo only reserving GITS_TRANSLATER).\nThe scope of DT covers more embedded things as well like PCI host\ncontrollers with internal MSI doorbells, and potentially even\ndirect-mapped memory regions for things like bootloader framebuffers to\nprevent display glitches - a generic address/size/flags description of a\nregion could work for just about everything.\n\n> IORT code has the same issue (ie it reserves all ITS regions) and I do\n> not know where a property can be added to describe those ranges (IORT\n> specs ? I'd rather not) in ACPI other than the IORT tables entries\n> themselves.\n> \n>> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel\n>> like a much nicer and simpler solution to this problem.\n> \n> It could be but if we throw ACPI into the picture we have to knock\n> together Hisilicon specific namespace bindings to handle this and\n> quickly.\n\nAs above I'm happy with the ITS-specific solution for ACPI given the\nlimits of IORT. What I had in mind for DT was something tied in with the\ngeneric IOMMU bindings. Something like this is probably easiest to\nhandle, but does rather spread the information around:\n\n\n pci {\n \t...\n \tiommu-map = <0 0 &iommu1 0x10000>;\n \tiommu-reserved-ranges = <0x12340000 0x1000 IOMMU_MSI>,\n \t\t\t\t<0x34560000 0x1000 IOMMU_MSI>;\n };\n\n display {\n \t...\n \tiommus = <&iommu1 0x20000>;\n \t/* simplefb region */\n \tiommu-reserved-ranges = <0x80080000 0x80000 IOMMU_DIRECT>,\n };\n\n\nAlternatively, something inspired by reserved-memory might perhaps be\nconceptually neater, at the risk of being more complicated:\n\n\n iommu1: iommu@acbd0000 {\n \t...\n \t#iommu-cells = <1>;\n\n \tiommu-reserved-ranges {\n \t\t#address-cells = <1>;\n \t\t#size-cells = <1>;\n\n\t\tits0_resv: msi@12340000 {\n \t\t\tcompatible = \"iommu-msi-region\";\n \t\t\treg = <0x12340000 0x1000>;\n \t\t};\n\n\t\tits1_resv: msi@34560000 {\n \t\t\tcompatible = \"iommu-msi-region\";\n \t\t\treg = <0x34560000 0x1000>;\n \t\t};\n\n\t\tfb_resv: direct@12340000 {\n \t\t\tcompatible = \"iommu-direct-region\";\n \t\t\treg = <0x80080000 0x80000>;\n \t\t};\n \t};\n };\n\n\nDT folks - any opinions?\n\nRobin.\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y793C0mxkz9t1G\n\tfor ;\n\tThu, 5 Oct 2017 22:07:35 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751705AbdJELHd (ORCPT\n\t);\n\tThu, 5 Oct 2017 07:07:33 -0400","from foss.arm.com ([217.140.101.70]:43408 \"EHLO foss.arm.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751536AbdJELHc (ORCPT );\n\tThu, 5 Oct 2017 07:07:32 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2C11E80D;\n\tThu, 5 Oct 2017 04:07:32 -0700 (PDT)","from [10.1.210.88] (e110467-lin.cambridge.arm.com [10.1.210.88])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t2FA6B3F483; Thu, 5 Oct 2017 04:07:29 -0700 (PDT)"],"Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","To":"Lorenzo Pieralisi ,\n\tMarc Zyngier ","Cc":"Shameer Kolothum ,\n\tsudeep.holla@arm.com, will.deacon@arm.com, joro@8bytes.org,\n\tmark.rutland@arm.com, robh@kernel.org, gabriele.paoloni@huawei.com,\n\tjohn.garry@huawei.com, iommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t\n\t<20171004135007.GA12327@red-moon>","From":"Robin Murphy ","Message-ID":"<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>","Date":"Thu, 5 Oct 2017 12:07:26 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20171004135007.GA12327@red-moon>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1780536,"web_url":"http://patchwork.ozlabs.org/comment/1780536/","msgid":"<20171005115719.GC11088@arm.com>","list_archive_url":null,"date":"2017-10-05T11:57:19","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":7916,"url":"http://patchwork.ozlabs.org/api/people/7916/","name":"Will Deacon","email":"will.deacon@arm.com"},"content":"On Thu, Oct 05, 2017 at 12:07:26PM +0100, Robin Murphy wrote:\n> On 04/10/17 14:50, Lorenzo Pieralisi wrote:\n> > On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n> >> On 27/09/17 14:32, Shameer Kolothum wrote:\n> >>> From: John Garry \n> >>>\n> >>> On some platforms msi-controller address regions have to be excluded\n> >>> from normal IOVA allocation in that they are detected and decoded in\n> >>> a HW specific way by system components and so they cannot be considered\n> >>> normal IOVA address space.\n> >>>\n> >>> Add a helper function that retrieves msi address regions through device\n> >>> tree msi mapping, so that these regions will not be translated by IOMMU\n> >>> and will be excluded from IOVA allocations.\n> >>>\n> >>> Signed-off-by: John Garry \n> >>> [Shameer: Modified msi-parent retrieval logic]\n> >>> Signed-off-by: Shameer Kolothum \n> >>> ---\n> >>> drivers/iommu/of_iommu.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++\n> >>> include/linux/of_iommu.h | 10 +++++\n> >>> 2 files changed, 105 insertions(+)\n> >>>\n> >>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n> >>> index e60e3db..ffd7fb7 100644\n> >>> --- a/drivers/iommu/of_iommu.c\n> >>> +++ b/drivers/iommu/of_iommu.c\n> >>> @@ -21,6 +21,7 @@\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n> >>> \treturn ops->of_xlate(dev, iommu_spec);\n> >>> }\n> >>> \n> >>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n> >>> +{\n> >>> +\tu32 *rid = data;\n> >>> +\n> >>> +\t*rid = alias;\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> struct of_pci_iommu_alias_info {\n> >>> \tstruct device *dev;\n> >>> \tstruct device_node *np;\n> >>> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)\n> >>> \treturn info->np == pdev->bus->dev.of_node;\n> >>> }\n> >>> \n> >>> +static int of_iommu_alloc_resv_region(struct device_node *np,\n> >>> +\t\t\t\t struct list_head *head)\n> >>> +{\n> >>> +\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n> >>> +\tstruct iommu_resv_region *region;\n> >>> +\tstruct resource res;\n> >>> +\tint err;\n> >>> +\n> >>> +\terr = of_address_to_resource(np, 0, &res);\n> >>\n> >> What is the rational for registering the first region only? Surely you\n> >> can have more than one region in an MSI controller (yes, your particular\n> >> case is the ITS which has only one, but we're trying to do something\n> >> generic here).\n> >>\n> >> Another issue I have with this code is that it inserts all of the ITS\n> >> MMIO in the RESV_MSI range. As long as we don't generate any page tables\n> >> for this, we're fine. But if we ever change this, we'll end-up with the\n> >> ITS programming interface being exposed to a device, which wouldn't be\n> >> acceptable.\n> >>\n> >> Why can't we have some generic property instead, that would describe the\n> >> actual ranges that cannot be translated? That way, no random insertion\n> >> of a random range, and relatively future proof.\n> \n> Indeed. Furthermore, IORT has the advantage of being limited to a few\n> distinct device types and SBSA-compliant system topologies, so the\n> ITS-chasing is reasonable there (modulo only reserving GITS_TRANSLATER).\n> The scope of DT covers more embedded things as well like PCI host\n> controllers with internal MSI doorbells, and potentially even\n> direct-mapped memory regions for things like bootloader framebuffers to\n> prevent display glitches - a generic address/size/flags description of a\n> region could work for just about everything.\n> \n> > IORT code has the same issue (ie it reserves all ITS regions) and I do\n> > not know where a property can be added to describe those ranges (IORT\n> > specs ? I'd rather not) in ACPI other than the IORT tables entries\n> > themselves.\n> > \n> >> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel\n> >> like a much nicer and simpler solution to this problem.\n> > \n> > It could be but if we throw ACPI into the picture we have to knock\n> > together Hisilicon specific namespace bindings to handle this and\n> > quickly.\n> \n> As above I'm happy with the ITS-specific solution for ACPI given the\n> limits of IORT. What I had in mind for DT was something tied in with the\n> generic IOMMU bindings. Something like this is probably easiest to\n> handle, but does rather spread the information around:\n> \n> \n> pci {\n> \t...\n> \tiommu-map = <0 0 &iommu1 0x10000>;\n> \tiommu-reserved-ranges = <0x12340000 0x1000 IOMMU_MSI>,\n> \t\t\t\t<0x34560000 0x1000 IOMMU_MSI>;\n> };\n> \n> display {\n> \t...\n> \tiommus = <&iommu1 0x20000>;\n> \t/* simplefb region */\n> \tiommu-reserved-ranges = <0x80080000 0x80000 IOMMU_DIRECT>,\n> };\n> \n> \n> Alternatively, something inspired by reserved-memory might perhaps be\n> conceptually neater, at the risk of being more complicated:\n> \n> \n> iommu1: iommu@acbd0000 {\n> \t...\n> \t#iommu-cells = <1>;\n> \n> \tiommu-reserved-ranges {\n> \t\t#address-cells = <1>;\n> \t\t#size-cells = <1>;\n> \n> \t\tits0_resv: msi@12340000 {\n> \t\t\tcompatible = \"iommu-msi-region\";\n> \t\t\treg = <0x12340000 0x1000>;\n> \t\t};\n> \n> \t\tits1_resv: msi@34560000 {\n> \t\t\tcompatible = \"iommu-msi-region\";\n> \t\t\treg = <0x34560000 0x1000>;\n> \t\t};\n> \n> \t\tfb_resv: direct@12340000 {\n> \t\t\tcompatible = \"iommu-direct-region\";\n> \t\t\treg = <0x80080000 0x80000>;\n> \t\t};\n> \t};\n> };\n\nI like the locality of this, but is it definitely flexible enough? Do we\nneed to deal with systems where the reserved regions are specific to the\nmaster (i.e. TBU) as opposed to the entire SMMU block?\n\nWill\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7B8c72Xkz9t2x\n\tfor ;\n\tThu, 5 Oct 2017 22:57:20 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751330AbdJEL5T (ORCPT\n\t);\n\tThu, 5 Oct 2017 07:57:19 -0400","from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:44200 \"EHLO\n\tfoss.arm.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751323AbdJEL5S (ORCPT );\n\tThu, 5 Oct 2017 07:57:18 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 87EFE1529;\n\tThu, 5 Oct 2017 04:57:18 -0700 (PDT)","from edgewater-inn.cambridge.arm.com\n\t(usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id\n\t57DDC3F483; Thu, 5 Oct 2017 04:57:18 -0700 (PDT)","by edgewater-inn.cambridge.arm.com (Postfix, from userid 1000)\n\tid A0B601AE2DD7; Thu, 5 Oct 2017 12:57:19 +0100 (BST)"],"Date":"Thu, 5 Oct 2017 12:57:19 +0100","From":"Will Deacon ","To":"Robin Murphy ","Cc":"Lorenzo Pieralisi ,\n\tMarc Zyngier ,\n\tShameer Kolothum ,\n\tsudeep.holla@arm.com, joro@8bytes.org, mark.rutland@arm.com,\n\trobh@kernel.org, gabriele.paoloni@huawei.com,\n\tjohn.garry@huawei.com, iommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","Message-ID":"<20171005115719.GC11088@arm.com>","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t\n\t<20171004135007.GA12327@red-moon>\n\t<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1780571,"web_url":"http://patchwork.ozlabs.org/comment/1780571/","msgid":"","list_archive_url":null,"date":"2017-10-05T12:37:06","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":67406,"url":"http://patchwork.ozlabs.org/api/people/67406/","name":"John Garry","email":"john.garry@huawei.com"},"content":"On 05/10/2017 12:07, Robin Murphy wrote:\n> On 04/10/17 14:50, Lorenzo Pieralisi wrote:\n>> On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n>>> On 27/09/17 14:32, Shameer Kolothum wrote:\n>>>> From: John Garry \n>>>>\n>>>> On some platforms msi-controller address regions have to be excluded\n>>>> from normal IOVA allocation in that they are detected and decoded in\n>>>> a HW specific way by system components and so they cannot be considered\n>>>> normal IOVA address space.\n>>>>\n>>>> Add a helper function that retrieves msi address regions through device\n>>>> tree msi mapping, so that these regions will not be translated by IOMMU\n>>>> and will be excluded from IOVA allocations.\n>>>>\n>>>> Signed-off-by: John Garry \n>>>> [Shameer: Modified msi-parent retrieval logic]\n>>>> Signed-off-by: Shameer Kolothum \n>>>> ---\n>>>> drivers/iommu/of_iommu.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++\n>>>> include/linux/of_iommu.h | 10 +++++\n>>>> 2 files changed, 105 insertions(+)\n>>>>\n>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n>>>> index e60e3db..ffd7fb7 100644\n>>>> --- a/drivers/iommu/of_iommu.c\n>>>> +++ b/drivers/iommu/of_iommu.c\n>>>> @@ -21,6 +21,7 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>> #include \n>>>> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n>>>> \treturn ops->of_xlate(dev, iommu_spec);\n>>>> }\n>>>>\n>>>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n>>>> +{\n>>>> +\tu32 *rid = data;\n>>>> +\n>>>> +\t*rid = alias;\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> struct of_pci_iommu_alias_info {\n>>>> \tstruct device *dev;\n>>>> \tstruct device_node *np;\n>>>> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)\n>>>> \treturn info->np == pdev->bus->dev.of_node;\n>>>> }\n>>>>\n>>>> +static int of_iommu_alloc_resv_region(struct device_node *np,\n>>>> +\t\t\t\t struct list_head *head)\n>>>> +{\n>>>> +\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n>>>> +\tstruct iommu_resv_region *region;\n>>>> +\tstruct resource res;\n>>>> +\tint err;\n>>>> +\n>>>> +\terr = of_address_to_resource(np, 0, &res);\n>>>\n>>> What is the rational for registering the first region only? Surely you\n>>> can have more than one region in an MSI controller (yes, your particular\n>>> case is the ITS which has only one, but we're trying to do something\n>>> generic here).\n>>>\n>>> Another issue I have with this code is that it inserts all of the ITS\n>>> MMIO in the RESV_MSI range. As long as we don't generate any page tables\n>>> for this, we're fine. But if we ever change this, we'll end-up with the\n>>> ITS programming interface being exposed to a device, which wouldn't be\n>>> acceptable.\n>>>\n>>> Why can't we have some generic property instead, that would describe the\n>>> actual ranges that cannot be translated? That way, no random insertion\n>>> of a random range, and relatively future proof.\n>\n> Indeed. Furthermore, IORT has the advantage of being limited to a few\n> distinct device types and SBSA-compliant system topologies, so the\n> ITS-chasing is reasonable there (modulo only reserving GITS_TRANSLATER).\n> The scope of DT covers more embedded things as well like PCI host\n> controllers with internal MSI doorbells, and potentially even\n> direct-mapped memory regions for things like bootloader framebuffers to\n> prevent display glitches - a generic address/size/flags description of a\n> region could work for just about everything.\n>\n>> IORT code has the same issue (ie it reserves all ITS regions) and I do\n>> not know where a property can be added to describe those ranges (IORT\n>> specs ? I'd rather not) in ACPI other than the IORT tables entries\n>> themselves.\n>>\n>>> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel\n>>> like a much nicer and simpler solution to this problem.\n>>\n>> It could be but if we throw ACPI into the picture we have to knock\n>> together Hisilicon specific namespace bindings to handle this and\n>> quickly.\n>\n> As above I'm happy with the ITS-specific solution for ACPI given the\n> limits of IORT. What I had in mind for DT was something tied in with the\n> generic IOMMU bindings. Something like this is probably easiest to\n> handle, but does rather spread the information around:\n>\n>\n> pci {\n> \t...\n> \tiommu-map = <0 0 &iommu1 0x10000>;\n> \tiommu-reserved-ranges = <0x12340000 0x1000 IOMMU_MSI>,\n> \t\t\t\t<0x34560000 0x1000 IOMMU_MSI>;\n> };\n>\n> display {\n> \t...\n> \tiommus = <&iommu1 0x20000>;\n> \t/* simplefb region */\n> \tiommu-reserved-ranges = <0x80080000 0x80000 IOMMU_DIRECT>,\n> };\n>\n>\n> Alternatively, something inspired by reserved-memory might perhaps be\n> conceptually neater, at the risk of being more complicated:\n>\n>\n> iommu1: iommu@acbd0000 {\n> \t...\n> \t#iommu-cells = <1>;\n>\n> \tiommu-reserved-ranges {\n> \t\t#address-cells = <1>;\n> \t\t#size-cells = <1>;\n>\n> \t\tits0_resv: msi@12340000 {\n> \t\t\tcompatible = \"iommu-msi-region\";\n> \t\t\treg = <0x12340000 0x1000>;\n> \t\t};\n>\n> \t\tits1_resv: msi@34560000 {\n> \t\t\tcompatible = \"iommu-msi-region\";\n> \t\t\treg = <0x34560000 0x1000>;\n> \t\t};\n>\n> \t\tfb_resv: direct@12340000 {\n> \t\t\tcompatible = \"iommu-direct-region\";\n> \t\t\treg = <0x80080000 0x80000>;\n> \t\t};\n> \t};\n> };\n>\n>\n\nIf we did this, wouldn't it be easier to create dangerous reserved \nregions, like our ITS region which Marc was concerned by? It's not so \nhard to get dts changes into the kernel.\n\nJohn\n\n> DT folks - any opinions?\n>\n> Robin.\n>\n> .\n>\n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7C370KLMz9s7B\n\tfor ;\n\tThu, 5 Oct 2017 23:37:38 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751318AbdJEMhh (ORCPT\n\t);\n\tThu, 5 Oct 2017 08:37:37 -0400","from szxga05-in.huawei.com ([45.249.212.191]:7517 \"EHLO\n\tszxga05-in.huawei.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751295AbdJEMhg (ORCPT\n\t); Thu, 5 Oct 2017 08:37:36 -0400","from 172.30.72.59 (EHLO DGGEMS403-HUB.china.huawei.com)\n\t([172.30.72.59])\n\tby dggrg05-dlp.huawei.com (MOS 4.4.6-GA FastPath queued)\n\twith ESMTP id DIO96500; Thu, 05 Oct 2017 20:37:24 +0800 (CST)","from [127.0.0.1] (10.202.227.238) by DGGEMS403-HUB.china.huawei.com\n\t(10.3.19.203) with Microsoft SMTP Server id 14.3.301.0;\n\tThu, 5 Oct 2017 20:37:16 +0800"],"Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","To":"Robin Murphy ,\n\tLorenzo Pieralisi ,\n\tMarc Zyngier ","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t\n\t<20171004135007.GA12327@red-moon>\n\t<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>","CC":"Shameer Kolothum ,\n\t, , ,\n\t, ,\n\t, ,\n\t,\n\t, ,\n\t, ,\n\t, ","From":"John Garry ","Message-ID":"","Date":"Thu, 5 Oct 2017 13:37:06 +0100","User-Agent":"Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101\n\tThunderbird/45.3.0","MIME-Version":"1.0","In-Reply-To":"<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>","Content-Type":"text/plain; charset=\"utf-8\"; format=flowed","Content-Transfer-Encoding":"7bit","X-Originating-IP":"[10.202.227.238]","X-CFilter-Loop":"Reflected","X-Mirapoint-Virus-RAPID-Raw":"score=unknown(0),\n\trefid=str=0001.0A020201.59D62785.004B, ss=1, re=0.000, recu=0.000,\n\treip=0.000, cl=1, cld=1, fgs=0, ip=0.0.0.0,\n\tso=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32","X-Mirapoint-Loop-Id":"c174926b03a3b8d63dda84c97352c621","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1780580,"web_url":"http://patchwork.ozlabs.org/comment/1780580/","msgid":"","list_archive_url":null,"date":"2017-10-05T12:44:49","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":65641,"url":"http://patchwork.ozlabs.org/api/people/65641/","name":"Robin Murphy","email":"robin.murphy@arm.com"},"content":"On 05/10/17 13:37, John Garry wrote:\n> On 05/10/2017 12:07, Robin Murphy wrote:\n>> On 04/10/17 14:50, Lorenzo Pieralisi wrote:\n>>> On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n>>>> On 27/09/17 14:32, Shameer Kolothum wrote:\n>>>>> From: John Garry \n>>>>>\n>>>>> On some platforms msi-controller address regions have to be excluded\n>>>>> from normal IOVA allocation in that they are detected and decoded in\n>>>>> a HW specific way by system components and so they cannot be\n>>>>> considered\n>>>>> normal IOVA address space.\n>>>>>\n>>>>> Add a helper function that retrieves msi address regions through\n>>>>> device\n>>>>> tree msi mapping, so that these regions will not be translated by\n>>>>> IOMMU\n>>>>> and will be excluded from IOVA allocations.\n>>>>>\n>>>>> Signed-off-by: John Garry \n>>>>> [Shameer: Modified msi-parent retrieval logic]\n>>>>> Signed-off-by: Shameer Kolothum \n>>>>> ---\n>>>>>  drivers/iommu/of_iommu.c | 95\n>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++\n>>>>>  include/linux/of_iommu.h | 10 +++++\n>>>>>  2 files changed, 105 insertions(+)\n>>>>>\n>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n>>>>> index e60e3db..ffd7fb7 100644\n>>>>> --- a/drivers/iommu/of_iommu.c\n>>>>> +++ b/drivers/iommu/of_iommu.c\n>>>>> @@ -21,6 +21,7 @@\n>>>>>  #include \n>>>>>  #include \n>>>>>  #include \n>>>>> +#include \n>>>>>  #include \n>>>>>  #include \n>>>>>  #include \n>>>>> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n>>>>>      return ops->of_xlate(dev, iommu_spec);\n>>>>>  }\n>>>>>\n>>>>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n>>>>> +{\n>>>>> +    u32 *rid = data;\n>>>>> +\n>>>>> +    *rid = alias;\n>>>>> +    return 0;\n>>>>> +}\n>>>>> +\n>>>>>  struct of_pci_iommu_alias_info {\n>>>>>      struct device *dev;\n>>>>>      struct device_node *np;\n>>>>> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev\n>>>>> *pdev, u16 alias, void *data)\n>>>>>      return info->np == pdev->bus->dev.of_node;\n>>>>>  }\n>>>>>\n>>>>> +static int of_iommu_alloc_resv_region(struct device_node *np,\n>>>>> +                      struct list_head *head)\n>>>>> +{\n>>>>> +    int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n>>>>> +    struct iommu_resv_region *region;\n>>>>> +    struct resource res;\n>>>>> +    int err;\n>>>>> +\n>>>>> +    err = of_address_to_resource(np, 0, &res);\n>>>>\n>>>> What is the rational for registering the first region only? Surely you\n>>>> can have more than one region in an MSI controller (yes, your\n>>>> particular\n>>>> case is the ITS which has only one, but we're trying to do something\n>>>> generic here).\n>>>>\n>>>> Another issue I have with this code is that it inserts all of the ITS\n>>>> MMIO in the RESV_MSI range. As long as we don't generate any page\n>>>> tables\n>>>> for this, we're fine. But if we ever change this, we'll end-up with the\n>>>> ITS programming interface being exposed to a device, which wouldn't be\n>>>> acceptable.\n>>>>\n>>>> Why can't we have some generic property instead, that would describe\n>>>> the\n>>>> actual ranges that cannot be translated? That way, no random insertion\n>>>> of a random range, and relatively future proof.\n>>\n>> Indeed. Furthermore, IORT has the advantage of being limited to a few\n>> distinct device types and SBSA-compliant system topologies, so the\n>> ITS-chasing is reasonable there (modulo only reserving GITS_TRANSLATER).\n>> The scope of DT covers more embedded things as well like PCI host\n>> controllers with internal MSI doorbells, and potentially even\n>> direct-mapped memory regions for things like bootloader framebuffers to\n>> prevent display glitches - a generic address/size/flags description of a\n>> region could work for just about everything.\n>>\n>>> IORT code has the same issue (ie it reserves all ITS regions) and I do\n>>> not know where a property can be added to describe those ranges (IORT\n>>> specs ? I'd rather not) in ACPI other than the IORT tables entries\n>>> themselves.\n>>>\n>>>> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd\n>>>> feel\n>>>> like a much nicer and simpler solution to this problem.\n>>>\n>>> It could be but if we throw ACPI into the picture we have to knock\n>>> together Hisilicon specific namespace bindings to handle this and\n>>> quickly.\n>>\n>> As above I'm happy with the ITS-specific solution for ACPI given the\n>> limits of IORT. What I had in mind for DT was something tied in with the\n>> generic IOMMU bindings. Something like this is probably easiest to\n>> handle, but does rather spread the information around:\n>>\n>>\n>>   pci {\n>>       ...\n>>       iommu-map = <0 0 &iommu1 0x10000>;\n>>       iommu-reserved-ranges = <0x12340000 0x1000 IOMMU_MSI>,\n>>                   <0x34560000 0x1000 IOMMU_MSI>;\n>>   };\n>>\n>>   display {\n>>       ...\n>>       iommus = <&iommu1 0x20000>;\n>>       /* simplefb region */\n>>       iommu-reserved-ranges = <0x80080000 0x80000 IOMMU_DIRECT>,\n>>   };\n>>\n>>\n>> Alternatively, something inspired by reserved-memory might perhaps be\n>> conceptually neater, at the risk of being more complicated:\n>>\n>>\n>>   iommu1: iommu@acbd0000 {\n>>       ...\n>>       #iommu-cells = <1>;\n>>\n>>       iommu-reserved-ranges {\n>>           #address-cells = <1>;\n>>           #size-cells = <1>;\n>>\n>>         its0_resv: msi@12340000 {\n>>               compatible = \"iommu-msi-region\";\n>>               reg = <0x12340000 0x1000>;\n>>           };\n>>\n>>         its1_resv: msi@34560000 {\n>>               compatible = \"iommu-msi-region\";\n>>               reg = <0x34560000 0x1000>;\n>>           };\n>>\n>>         fb_resv: direct@12340000 {\n>>               compatible = \"iommu-direct-region\";\n>>               reg = <0x80080000 0x80000>;\n>>           };\n>>       };\n>>   };\n>>\n>>\n> \n> If we did this, wouldn't it be easier to create dangerous reserved\n> regions, like our ITS region which Marc was concerned by? It's not so\n> hard to get dts changes into the kernel.\n\nWell, yeah. It's also equally easy to add some peripheral register\nregion to the /memory node and watch hilarity ensue. The solution to\nboth is \"don't put bogus crap in your DT\".\n\nThere's a big difference between wilfully misdescribing your platform\nrequirements vs. having the kernel automagically infer something but\nleave a hole open in the process.\n\nRobin.\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7CCr4v05z9t2m\n\tfor ;\n\tThu, 5 Oct 2017 23:45:12 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751450AbdJEMo5 (ORCPT\n\t);\n\tThu, 5 Oct 2017 08:44:57 -0400","from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:44870 \"EHLO\n\tfoss.arm.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751446AbdJEMoz (ORCPT );\n\tThu, 5 Oct 2017 08:44:55 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5783A1529;\n\tThu, 5 Oct 2017 05:44:54 -0700 (PDT)","from [10.1.210.88] (e110467-lin.cambridge.arm.com [10.1.210.88])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t0B9C93F483; Thu, 5 Oct 2017 05:44:50 -0700 (PDT)"],"Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","To":"John Garry ,\n\tLorenzo Pieralisi ,\n\tMarc Zyngier ","Cc":"Shameer Kolothum ,\n\tsudeep.holla@arm.com, will.deacon@arm.com, joro@8bytes.org,\n\tmark.rutland@arm.com, robh@kernel.org, gabriele.paoloni@huawei.com,\n\tiommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t\n\t<20171004135007.GA12327@red-moon>\n\t<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>\n\t","From":"Robin Murphy ","Message-ID":"","Date":"Thu, 5 Oct 2017 13:44:49 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1780593,"web_url":"http://patchwork.ozlabs.org/comment/1780593/","msgid":"","list_archive_url":null,"date":"2017-10-05T12:55:10","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":65641,"url":"http://patchwork.ozlabs.org/api/people/65641/","name":"Robin Murphy","email":"robin.murphy@arm.com"},"content":"On 05/10/17 12:57, Will Deacon wrote:\n> On Thu, Oct 05, 2017 at 12:07:26PM +0100, Robin Murphy wrote:\n>> On 04/10/17 14:50, Lorenzo Pieralisi wrote:\n>>> On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n>>>> On 27/09/17 14:32, Shameer Kolothum wrote:\n>>>>> From: John Garry \n>>>>>\n>>>>> On some platforms msi-controller address regions have to be excluded\n>>>>> from normal IOVA allocation in that they are detected and decoded in\n>>>>> a HW specific way by system components and so they cannot be considered\n>>>>> normal IOVA address space.\n>>>>>\n>>>>> Add a helper function that retrieves msi address regions through device\n>>>>> tree msi mapping, so that these regions will not be translated by IOMMU\n>>>>> and will be excluded from IOVA allocations.\n>>>>>\n>>>>> Signed-off-by: John Garry \n>>>>> [Shameer: Modified msi-parent retrieval logic]\n>>>>> Signed-off-by: Shameer Kolothum \n>>>>> ---\n>>>>> drivers/iommu/of_iommu.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++\n>>>>> include/linux/of_iommu.h | 10 +++++\n>>>>> 2 files changed, 105 insertions(+)\n>>>>>\n>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n>>>>> index e60e3db..ffd7fb7 100644\n>>>>> --- a/drivers/iommu/of_iommu.c\n>>>>> +++ b/drivers/iommu/of_iommu.c\n>>>>> @@ -21,6 +21,7 @@\n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> +#include \n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n>>>>> \treturn ops->of_xlate(dev, iommu_spec);\n>>>>> }\n>>>>> \n>>>>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n>>>>> +{\n>>>>> +\tu32 *rid = data;\n>>>>> +\n>>>>> +\t*rid = alias;\n>>>>> +\treturn 0;\n>>>>> +}\n>>>>> +\n>>>>> struct of_pci_iommu_alias_info {\n>>>>> \tstruct device *dev;\n>>>>> \tstruct device_node *np;\n>>>>> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)\n>>>>> \treturn info->np == pdev->bus->dev.of_node;\n>>>>> }\n>>>>> \n>>>>> +static int of_iommu_alloc_resv_region(struct device_node *np,\n>>>>> +\t\t\t\t struct list_head *head)\n>>>>> +{\n>>>>> +\tint prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n>>>>> +\tstruct iommu_resv_region *region;\n>>>>> +\tstruct resource res;\n>>>>> +\tint err;\n>>>>> +\n>>>>> +\terr = of_address_to_resource(np, 0, &res);\n>>>>\n>>>> What is the rational for registering the first region only? Surely you\n>>>> can have more than one region in an MSI controller (yes, your particular\n>>>> case is the ITS which has only one, but we're trying to do something\n>>>> generic here).\n>>>>\n>>>> Another issue I have with this code is that it inserts all of the ITS\n>>>> MMIO in the RESV_MSI range. As long as we don't generate any page tables\n>>>> for this, we're fine. But if we ever change this, we'll end-up with the\n>>>> ITS programming interface being exposed to a device, which wouldn't be\n>>>> acceptable.\n>>>>\n>>>> Why can't we have some generic property instead, that would describe the\n>>>> actual ranges that cannot be translated? That way, no random insertion\n>>>> of a random range, and relatively future proof.\n>>\n>> Indeed. Furthermore, IORT has the advantage of being limited to a few\n>> distinct device types and SBSA-compliant system topologies, so the\n>> ITS-chasing is reasonable there (modulo only reserving GITS_TRANSLATER).\n>> The scope of DT covers more embedded things as well like PCI host\n>> controllers with internal MSI doorbells, and potentially even\n>> direct-mapped memory regions for things like bootloader framebuffers to\n>> prevent display glitches - a generic address/size/flags description of a\n>> region could work for just about everything.\n>>\n>>> IORT code has the same issue (ie it reserves all ITS regions) and I do\n>>> not know where a property can be added to describe those ranges (IORT\n>>> specs ? I'd rather not) in ACPI other than the IORT tables entries\n>>> themselves.\n>>>\n>>>> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd feel\n>>>> like a much nicer and simpler solution to this problem.\n>>>\n>>> It could be but if we throw ACPI into the picture we have to knock\n>>> together Hisilicon specific namespace bindings to handle this and\n>>> quickly.\n>>\n>> As above I'm happy with the ITS-specific solution for ACPI given the\n>> limits of IORT. What I had in mind for DT was something tied in with the\n>> generic IOMMU bindings. Something like this is probably easiest to\n>> handle, but does rather spread the information around:\n>>\n>>\n>> pci {\n>> \t...\n>> \tiommu-map = <0 0 &iommu1 0x10000>;\n>> \tiommu-reserved-ranges = <0x12340000 0x1000 IOMMU_MSI>,\n>> \t\t\t\t<0x34560000 0x1000 IOMMU_MSI>;\n>> };\n>>\n>> display {\n>> \t...\n>> \tiommus = <&iommu1 0x20000>;\n>> \t/* simplefb region */\n>> \tiommu-reserved-ranges = <0x80080000 0x80000 IOMMU_DIRECT>,\n>> };\n>>\n>>\n>> Alternatively, something inspired by reserved-memory might perhaps be\n>> conceptually neater, at the risk of being more complicated:\n>>\n>>\n>> iommu1: iommu@acbd0000 {\n>> \t...\n>> \t#iommu-cells = <1>;\n>>\n>> \tiommu-reserved-ranges {\n>> \t\t#address-cells = <1>;\n>> \t\t#size-cells = <1>;\n>>\n>> \t\tits0_resv: msi@12340000 {\n>> \t\t\tcompatible = \"iommu-msi-region\";\n>> \t\t\treg = <0x12340000 0x1000>;\n>> \t\t};\n>>\n>> \t\tits1_resv: msi@34560000 {\n>> \t\t\tcompatible = \"iommu-msi-region\";\n>> \t\t\treg = <0x34560000 0x1000>;\n>> \t\t};\n>>\n>> \t\tfb_resv: direct@12340000 {\n>> \t\t\tcompatible = \"iommu-direct-region\";\n>> \t\t\treg = <0x80080000 0x80000>;\n>> \t\t};\n>> \t};\n>> };\n> \n> I like the locality of this, but is it definitely flexible enough? Do we\n> need to deal with systems where the reserved regions are specific to the\n> master (i.e. TBU) as opposed to the entire SMMU block?\n\nThat would certainly be true for most direct-mapping cases, where the\nreservation is tied to the specific stream ID(s) of one master, let\nalone a TBU. I guess we'd have to make a phandle reference from the\ndevice node(s) to the region node mandatory, such that software need\nonly make the actual reservation/mapping upon encountering a device that\nactually needs it.\n\nRobin.\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7CRX0pxvz9t2m\n\tfor ;\n\tThu, 5 Oct 2017 23:55:20 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751396AbdJEMzR (ORCPT\n\t);\n\tThu, 5 Oct 2017 08:55:17 -0400","from foss.arm.com ([217.140.101.70]:45084 \"EHLO foss.arm.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751317AbdJEMzP (ORCPT );\n\tThu, 5 Oct 2017 08:55:15 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6C0441682;\n\tThu, 5 Oct 2017 05:55:15 -0700 (PDT)","from [10.1.210.88] (e110467-lin.cambridge.arm.com [10.1.210.88])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t558453F483; Thu, 5 Oct 2017 05:55:12 -0700 (PDT)"],"Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","To":"Will Deacon ","Cc":"Lorenzo Pieralisi ,\n\tMarc Zyngier ,\n\tShameer Kolothum ,\n\tsudeep.holla@arm.com, joro@8bytes.org, mark.rutland@arm.com,\n\trobh@kernel.org, gabriele.paoloni@huawei.com,\n\tjohn.garry@huawei.com, iommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t\n\t<20171004135007.GA12327@red-moon>\n\t<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>\n\t<20171005115719.GC11088@arm.com>","From":"Robin Murphy ","Message-ID":"","Date":"Thu, 5 Oct 2017 13:55:10 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20171005115719.GC11088@arm.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1780603,"web_url":"http://patchwork.ozlabs.org/comment/1780603/","msgid":"<82272c18-717d-bbaf-b8c3-61785af496bd@huawei.com>","list_archive_url":null,"date":"2017-10-05T13:08:45","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":67406,"url":"http://patchwork.ozlabs.org/api/people/67406/","name":"John Garry","email":"john.garry@huawei.com"},"content":"On 05/10/2017 13:44, Robin Murphy wrote:\n> On 05/10/17 13:37, John Garry wrote:\n>> On 05/10/2017 12:07, Robin Murphy wrote:\n>>> On 04/10/17 14:50, Lorenzo Pieralisi wrote:\n>>>> On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n>>>>> On 27/09/17 14:32, Shameer Kolothum wrote:\n>>>>>> From: John Garry \n>>>>>>\n>>>>>> On some platforms msi-controller address regions have to be excluded\n>>>>>> from normal IOVA allocation in that they are detected and decoded in\n>>>>>> a HW specific way by system components and so they cannot be\n>>>>>> considered\n>>>>>> normal IOVA address space.\n>>>>>>\n>>>>>> Add a helper function that retrieves msi address regions through\n>>>>>> device\n>>>>>> tree msi mapping, so that these regions will not be translated by\n>>>>>> IOMMU\n>>>>>> and will be excluded from IOVA allocations.\n>>>>>>\n>>>>>> Signed-off-by: John Garry \n>>>>>> [Shameer: Modified msi-parent retrieval logic]\n>>>>>> Signed-off-by: Shameer Kolothum \n>>>>>> ---\n>>>>>> drivers/iommu/of_iommu.c | 95\n>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++\n>>>>>> include/linux/of_iommu.h | 10 +++++\n>>>>>> 2 files changed, 105 insertions(+)\n>>>>>>\n>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n>>>>>> index e60e3db..ffd7fb7 100644\n>>>>>> --- a/drivers/iommu/of_iommu.c\n>>>>>> +++ b/drivers/iommu/of_iommu.c\n>>>>>> @@ -21,6 +21,7 @@\n>>>>>> #include \n>>>>>> #include \n>>>>>> #include \n>>>>>> +#include \n>>>>>> #include \n>>>>>> #include \n>>>>>> #include \n>>>>>> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n>>>>>> return ops->of_xlate(dev, iommu_spec);\n>>>>>> }\n>>>>>>\n>>>>>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)\n>>>>>> +{\n>>>>>> + u32 *rid = data;\n>>>>>> +\n>>>>>> + *rid = alias;\n>>>>>> + return 0;\n>>>>>> +}\n>>>>>> +\n>>>>>> struct of_pci_iommu_alias_info {\n>>>>>> struct device *dev;\n>>>>>> struct device_node *np;\n>>>>>> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev\n>>>>>> *pdev, u16 alias, void *data)\n>>>>>> return info->np == pdev->bus->dev.of_node;\n>>>>>> }\n>>>>>>\n>>>>>> +static int of_iommu_alloc_resv_region(struct device_node *np,\n>>>>>> + struct list_head *head)\n>>>>>> +{\n>>>>>> + int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n>>>>>> + struct iommu_resv_region *region;\n>>>>>> + struct resource res;\n>>>>>> + int err;\n>>>>>> +\n>>>>>> + err = of_address_to_resource(np, 0, &res);\n>>>>>\n>>>>> What is the rational for registering the first region only? Surely you\n>>>>> can have more than one region in an MSI controller (yes, your\n>>>>> particular\n>>>>> case is the ITS which has only one, but we're trying to do something\n>>>>> generic here).\n>>>>>\n>>>>> Another issue I have with this code is that it inserts all of the ITS\n>>>>> MMIO in the RESV_MSI range. As long as we don't generate any page\n>>>>> tables\n>>>>> for this, we're fine. But if we ever change this, we'll end-up with the\n>>>>> ITS programming interface being exposed to a device, which wouldn't be\n>>>>> acceptable.\n>>>>>\n>>>>> Why can't we have some generic property instead, that would describe\n>>>>> the\n>>>>> actual ranges that cannot be translated? That way, no random insertion\n>>>>> of a random range, and relatively future proof.\n>>>\n>>> Indeed. Furthermore, IORT has the advantage of being limited to a few\n>>> distinct device types and SBSA-compliant system topologies, so the\n>>> ITS-chasing is reasonable there (modulo only reserving GITS_TRANSLATER).\n>>> The scope of DT covers more embedded things as well like PCI host\n>>> controllers with internal MSI doorbells, and potentially even\n>>> direct-mapped memory regions for things like bootloader framebuffers to\n>>> prevent display glitches - a generic address/size/flags description of a\n>>> region could work for just about everything.\n>>>\n>>>> IORT code has the same issue (ie it reserves all ITS regions) and I do\n>>>> not know where a property can be added to describe those ranges (IORT\n>>>> specs ? I'd rather not) in ACPI other than the IORT tables entries\n>>>> themselves.\n>>>>\n>>>>> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd\n>>>>> feel\n>>>>> like a much nicer and simpler solution to this problem.\n>>>>\n>>>> It could be but if we throw ACPI into the picture we have to knock\n>>>> together Hisilicon specific namespace bindings to handle this and\n>>>> quickly.\n>>>\n>>> As above I'm happy with the ITS-specific solution for ACPI given the\n>>> limits of IORT. What I had in mind for DT was something tied in with the\n>>> generic IOMMU bindings. Something like this is probably easiest to\n>>> handle, but does rather spread the information around:\n>>>\n>>>\n>>> pci {\n>>> ...\n>>> iommu-map = <0 0 &iommu1 0x10000>;\n>>> iommu-reserved-ranges = <0x12340000 0x1000 IOMMU_MSI>,\n>>> <0x34560000 0x1000 IOMMU_MSI>;\n>>> };\n>>>\n>>> display {\n>>> ...\n>>> iommus = <&iommu1 0x20000>;\n>>> /* simplefb region */\n>>> iommu-reserved-ranges = <0x80080000 0x80000 IOMMU_DIRECT>,\n>>> };\n>>>\n>>>\n>>> Alternatively, something inspired by reserved-memory might perhaps be\n>>> conceptually neater, at the risk of being more complicated:\n>>>\n>>>\n>>> iommu1: iommu@acbd0000 {\n>>> ...\n>>> #iommu-cells = <1>;\n>>>\n>>> iommu-reserved-ranges {\n>>> #address-cells = <1>;\n>>> #size-cells = <1>;\n>>>\n>>> its0_resv: msi@12340000 {\n>>> compatible = \"iommu-msi-region\";\n>>> reg = <0x12340000 0x1000>;\n>>> };\n>>>\n>>> its1_resv: msi@34560000 {\n>>> compatible = \"iommu-msi-region\";\n>>> reg = <0x34560000 0x1000>;\n>>> };\n>>>\n>>> fb_resv: direct@12340000 {\n>>> compatible = \"iommu-direct-region\";\n>>> reg = <0x80080000 0x80000>;\n>>> };\n>>> };\n>>> };\n>>>\n>>>\n>>\n>> If we did this, wouldn't it be easier to create dangerous reserved\n>> regions, like our ITS region which Marc was concerned by? It's not so\n>> hard to get dts changes into the kernel.\n>\n> Well, yeah. It's also equally easy to add some peripheral register\n> region to the /memory node and watch hilarity ensue. The solution to\n> both is \"don't put bogus crap in your DT\".\n>\n\nSure, but people make mistakes and often a lot more subtle than your \nexample.\n\nOnly one person spotted the problem with our approach to the ITS \nproblem. I'm pretty confident it would not have been spotted if it was \nsubmitted as a dts change.\n\nMuch appreciated,\nJohn\n\n> There's a big difference between wilfully misdescribing your platform\n> requirements vs. having the kernel automatically infer something but\n> leave a hole open in the process.\n>\n> Robin.\n>\n> .\n>\n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7Clk5fDfz9sDB\n\tfor ;\n\tFri, 6 Oct 2017 00:09:22 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751319AbdJENJV (ORCPT\n\t);\n\tThu, 5 Oct 2017 09:09:21 -0400","from szxga04-in.huawei.com ([45.249.212.190]:7494 \"EHLO\n\tszxga04-in.huawei.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751280AbdJENJU (ORCPT\n\t); Thu, 5 Oct 2017 09:09:20 -0400","from 172.30.72.60 (EHLO DGGEMS408-HUB.china.huawei.com)\n\t([172.30.72.60])\n\tby dggrg04-dlp.huawei.com (MOS 4.4.6-GA FastPath queued)\n\twith ESMTP id DIM98723; Thu, 05 Oct 2017 21:09:06 +0800 (CST)","from [127.0.0.1] (10.202.227.238) by DGGEMS408-HUB.china.huawei.com\n\t(10.3.19.208) with Microsoft SMTP Server id 14.3.301.0;\n\tThu, 5 Oct 2017 21:08:55 +0800"],"Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","To":"Robin Murphy ,\n\tLorenzo Pieralisi ,\n\tMarc Zyngier ","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t\n\t<20171004135007.GA12327@red-moon>\n\t<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>\n\t\n\t","CC":"Shameer Kolothum ,\n\t, , ,\n\t, ,\n\t, ,\n\t,\n\t, ,\n\t, ,\n\t, ","From":"John Garry ","Message-ID":"<82272c18-717d-bbaf-b8c3-61785af496bd@huawei.com>","Date":"Thu, 5 Oct 2017 14:08:45 +0100","User-Agent":"Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101\n\tThunderbird/45.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=\"utf-8\"; format=flowed","Content-Transfer-Encoding":"7bit","X-Originating-IP":"[10.202.227.238]","X-CFilter-Loop":"Reflected","X-Mirapoint-Virus-RAPID-Raw":"score=unknown(0),\n\trefid=str=0001.0A090202.59D62EF2.015B, ss=1, re=0.000, recu=0.000,\n\treip=0.000, cl=1, cld=1, fgs=0, ip=0.0.0.0,\n\tso=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32","X-Mirapoint-Loop-Id":"b8ee2335347b3905525619f615b08cc7","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1780647,"web_url":"http://patchwork.ozlabs.org/comment/1780647/","msgid":"<6f8e27fc-3f16-7f66-5443-2be64ce9e464@arm.com>","list_archive_url":null,"date":"2017-10-05T14:05:41","subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","submitter":{"id":65641,"url":"http://patchwork.ozlabs.org/api/people/65641/","name":"Robin Murphy","email":"robin.murphy@arm.com"},"content":"On 05/10/17 14:08, John Garry wrote:\n> On 05/10/2017 13:44, Robin Murphy wrote:\n>> On 05/10/17 13:37, John Garry wrote:\n>>> On 05/10/2017 12:07, Robin Murphy wrote:\n>>>> On 04/10/17 14:50, Lorenzo Pieralisi wrote:\n>>>>> On Wed, Oct 04, 2017 at 12:22:04PM +0100, Marc Zyngier wrote:\n>>>>>> On 27/09/17 14:32, Shameer Kolothum wrote:\n>>>>>>> From: John Garry \n>>>>>>>\n>>>>>>> On some platforms msi-controller address regions have to be excluded\n>>>>>>> from normal IOVA allocation in that they are detected and decoded in\n>>>>>>> a HW specific way by system components and so they cannot be\n>>>>>>> considered\n>>>>>>> normal IOVA address space.\n>>>>>>>\n>>>>>>> Add a helper function that retrieves msi address regions through\n>>>>>>> device\n>>>>>>> tree msi mapping, so that these regions will not be translated by\n>>>>>>> IOMMU\n>>>>>>> and will be excluded from IOVA allocations.\n>>>>>>>\n>>>>>>> Signed-off-by: John Garry \n>>>>>>> [Shameer: Modified msi-parent retrieval logic]\n>>>>>>> Signed-off-by: Shameer Kolothum\n>>>>>>> \n>>>>>>> ---\n>>>>>>>  drivers/iommu/of_iommu.c | 95\n>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++\n>>>>>>>  include/linux/of_iommu.h | 10 +++++\n>>>>>>>  2 files changed, 105 insertions(+)\n>>>>>>>\n>>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c\n>>>>>>> index e60e3db..ffd7fb7 100644\n>>>>>>> --- a/drivers/iommu/of_iommu.c\n>>>>>>> +++ b/drivers/iommu/of_iommu.c\n>>>>>>> @@ -21,6 +21,7 @@\n>>>>>>>  #include \n>>>>>>>  #include \n>>>>>>>  #include \n>>>>>>> +#include \n>>>>>>>  #include \n>>>>>>>  #include \n>>>>>>>  #include \n>>>>>>> @@ -138,6 +139,14 @@ static int of_iommu_xlate(struct device *dev,\n>>>>>>>      return ops->of_xlate(dev, iommu_spec);\n>>>>>>>  }\n>>>>>>>\n>>>>>>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void\n>>>>>>> *data)\n>>>>>>> +{\n>>>>>>> +    u32 *rid = data;\n>>>>>>> +\n>>>>>>> +    *rid = alias;\n>>>>>>> +    return 0;\n>>>>>>> +}\n>>>>>>> +\n>>>>>>>  struct of_pci_iommu_alias_info {\n>>>>>>>      struct device *dev;\n>>>>>>>      struct device_node *np;\n>>>>>>> @@ -163,6 +172,73 @@ static int of_pci_iommu_init(struct pci_dev\n>>>>>>> *pdev, u16 alias, void *data)\n>>>>>>>      return info->np == pdev->bus->dev.of_node;\n>>>>>>>  }\n>>>>>>>\n>>>>>>> +static int of_iommu_alloc_resv_region(struct device_node *np,\n>>>>>>> +                      struct list_head *head)\n>>>>>>> +{\n>>>>>>> +    int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;\n>>>>>>> +    struct iommu_resv_region *region;\n>>>>>>> +    struct resource res;\n>>>>>>> +    int err;\n>>>>>>> +\n>>>>>>> +    err = of_address_to_resource(np, 0, &res);\n>>>>>>\n>>>>>> What is the rational for registering the first region only? Surely\n>>>>>> you\n>>>>>> can have more than one region in an MSI controller (yes, your\n>>>>>> particular\n>>>>>> case is the ITS which has only one, but we're trying to do something\n>>>>>> generic here).\n>>>>>>\n>>>>>> Another issue I have with this code is that it inserts all of the ITS\n>>>>>> MMIO in the RESV_MSI range. As long as we don't generate any page\n>>>>>> tables\n>>>>>> for this, we're fine. But if we ever change this, we'll end-up\n>>>>>> with the\n>>>>>> ITS programming interface being exposed to a device, which\n>>>>>> wouldn't be\n>>>>>> acceptable.\n>>>>>>\n>>>>>> Why can't we have some generic property instead, that would describe\n>>>>>> the\n>>>>>> actual ranges that cannot be translated? That way, no random\n>>>>>> insertion\n>>>>>> of a random range, and relatively future proof.\n>>>>\n>>>> Indeed. Furthermore, IORT has the advantage of being limited to a few\n>>>> distinct device types and SBSA-compliant system topologies, so the\n>>>> ITS-chasing is reasonable there (modulo only reserving\n>>>> GITS_TRANSLATER).\n>>>> The scope of DT covers more embedded things as well like PCI host\n>>>> controllers with internal MSI doorbells, and potentially even\n>>>> direct-mapped memory regions for things like bootloader framebuffers to\n>>>> prevent display glitches - a generic address/size/flags description\n>>>> of a\n>>>> region could work for just about everything.\n>>>>\n>>>>> IORT code has the same issue (ie it reserves all ITS regions) and I do\n>>>>> not know where a property can be added to describe those ranges (IORT\n>>>>> specs ? I'd rather not) in ACPI other than the IORT tables entries\n>>>>> themselves.\n>>>>>\n>>>>>> I'm not sure where to declare it (PCIe node? IOMMU node?), but it'd\n>>>>>> feel\n>>>>>> like a much nicer and simpler solution to this problem.\n>>>>>\n>>>>> It could be but if we throw ACPI into the picture we have to knock\n>>>>> together Hisilicon specific namespace bindings to handle this and\n>>>>> quickly.\n>>>>\n>>>> As above I'm happy with the ITS-specific solution for ACPI given the\n>>>> limits of IORT. What I had in mind for DT was something tied in with\n>>>> the\n>>>> generic IOMMU bindings. Something like this is probably easiest to\n>>>> handle, but does rather spread the information around:\n>>>>\n>>>>\n>>>>   pci {\n>>>>       ...\n>>>>       iommu-map = <0 0 &iommu1 0x10000>;\n>>>>       iommu-reserved-ranges = <0x12340000 0x1000 IOMMU_MSI>,\n>>>>                   <0x34560000 0x1000 IOMMU_MSI>;\n>>>>   };\n>>>>\n>>>>   display {\n>>>>       ...\n>>>>       iommus = <&iommu1 0x20000>;\n>>>>       /* simplefb region */\n>>>>       iommu-reserved-ranges = <0x80080000 0x80000 IOMMU_DIRECT>,\n>>>>   };\n>>>>\n>>>>\n>>>> Alternatively, something inspired by reserved-memory might perhaps be\n>>>> conceptually neater, at the risk of being more complicated:\n>>>>\n>>>>\n>>>>   iommu1: iommu@acbd0000 {\n>>>>       ...\n>>>>       #iommu-cells = <1>;\n>>>>\n>>>>       iommu-reserved-ranges {\n>>>>           #address-cells = <1>;\n>>>>           #size-cells = <1>;\n>>>>\n>>>>         its0_resv: msi@12340000 {\n>>>>               compatible = \"iommu-msi-region\";\n>>>>               reg = <0x12340000 0x1000>;\n>>>>           };\n>>>>\n>>>>         its1_resv: msi@34560000 {\n>>>>               compatible = \"iommu-msi-region\";\n>>>>               reg = <0x34560000 0x1000>;\n>>>>           };\n>>>>\n>>>>         fb_resv: direct@12340000 {\n>>>>               compatible = \"iommu-direct-region\";\n>>>>               reg = <0x80080000 0x80000>;\n>>>>           };\n>>>>       };\n>>>>   };\n>>>>\n>>>>\n>>>\n>>> If we did this, wouldn't it be easier to create dangerous reserved\n>>> regions, like our ITS region which Marc was concerned by? It's not so\n>>> hard to get dts changes into the kernel.\n>>\n>> Well, yeah. It's also equally easy to add some peripheral register\n>> region to the /memory node and watch hilarity ensue. The solution to\n>> both is \"don't put bogus crap in your DT\".\n>>\n> \n> Sure, but people make mistakes and often a lot more subtle than your\n> example.\n\nI wanted to write a response to that but since it was too easy to hit\nthe wrong keys on my keyboard, I didn't.\n\nI don't see a proposal for some alternate method of describing\nrestricted IOVA regions which scales beyond HiSilicon Hip07 and would be\nentirely immune to bugs, so I'm really not sure what your argument is\nhere :/\n\nRobin.\n\n> Only one person spotted the problem with our approach to the ITS\n> problem. I'm pretty confident it would not have been spotted if it was\n> submitted as a dts change.\n> \n> Much appreciated,\n> John\n> \n>> There's a big difference between wilfully misdescribing your platform\n>> requirements vs. having the kernel automatically infer something but\n>> leave a hole open in the process.\n>>\n>> Robin.\n>>\n>> .\n>>\n> \n> \n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7F0t1628z9t2h\n\tfor ;\n\tFri, 6 Oct 2017 01:05:50 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751286AbdJEOFs (ORCPT\n\t);\n\tThu, 5 Oct 2017 10:05:48 -0400","from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:46492 \"EHLO\n\tfoss.arm.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751094AbdJEOFr (ORCPT );\n\tThu, 5 Oct 2017 10:05:47 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CA63C1529;\n\tThu, 5 Oct 2017 07:05:46 -0700 (PDT)","from [10.1.210.88] (e110467-lin.cambridge.arm.com [10.1.210.88])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t7189E3F58C; Thu, 5 Oct 2017 07:05:43 -0700 (PDT)"],"Subject":"Re: [PATCH v8 3/5] iommu/of: Add msi address regions reservation\n\thelper","To":"John Garry ,\n\tLorenzo Pieralisi ,\n\tMarc Zyngier ","Cc":"Shameer Kolothum ,\n\tsudeep.holla@arm.com, will.deacon@arm.com, joro@8bytes.org,\n\tmark.rutland@arm.com, robh@kernel.org, gabriele.paoloni@huawei.com,\n\tiommu@lists.linux-foundation.org,\n\tlinux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,\n\tdevicetree@vger.kernel.org, devel@acpica.org, linuxarm@huawei.com,\n\twangzhou1@hisilicon.com, guohanjun@huawei.com","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-4-shameerali.kolothum.thodi@huawei.com>\n\t\n\t<20171004135007.GA12327@red-moon>\n\t<5c80f292-dc5d-a1c6-d7a0-df2a84cc77d1@arm.com>\n\t\n\t\n\t<82272c18-717d-bbaf-b8c3-61785af496bd@huawei.com>","From":"Robin Murphy ","Message-ID":"<6f8e27fc-3f16-7f66-5443-2be64ce9e464@arm.com>","Date":"Thu, 5 Oct 2017 15:05:41 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<82272c18-717d-bbaf-b8c3-61785af496bd@huawei.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1781404,"web_url":"http://patchwork.ozlabs.org/comment/1781404/","msgid":"<5FC3163CFD30C246ABAA99954A238FA83841DB56@FRAEML521-MBX.china.huawei.com>","list_archive_url":null,"date":"2017-10-06T10:17:17","subject":"RE: [PATCH v8 2/5] ACPI/IORT: Add msi address regions reservation\n\thelper","submitter":{"id":70507,"url":"http://patchwork.ozlabs.org/api/people/70507/","name":"Shameerali Kolothum Thodi","email":"shameerali.kolothum.thodi@huawei.com"},"content":"> -----Original Message-----\r\n> From: Marc Zyngier [mailto:marc.zyngier@arm.com]\r\n> Sent: Wednesday, October 04, 2017 3:18 PM\r\n> To: Shameerali Kolothum Thodi ;\r\n> lorenzo.pieralisi@arm.com; sudeep.holla@arm.com; will.deacon@arm.com;\r\n> robin.murphy@arm.com; joro@8bytes.org; mark.rutland@arm.com;\r\n> robh@kernel.org\r\n> Cc: Gabriele Paoloni ; John Garry\r\n> ; iommu@lists.linux-foundation.org; linux-arm-\r\n> kernel@lists.infradead.org; linux-acpi@vger.kernel.org;\r\n> devicetree@vger.kernel.org; devel@acpica.org; Linuxarm\r\n> ; Wangzhou (B) ;\r\n> Guohanjun (Hanjun Guo) \r\n> Subject: Re: [PATCH v8 2/5] ACPI/IORT: Add msi address regions reservation\r\n> helper\r\n> \r\n> On 27/09/17 14:32, Shameer Kolothum wrote:\r\n> > On some platforms msi parent address regions have to be excluded from\r\n> > normal IOVA allocation in that they are detected and decoded in a HW\r\n> > specific way by system components and so they cannot be considered\r\n> > normal IOVA address space.\r\n> >\r\n> > Add a helper function that retrieves ITS address regions - the msi\r\n> > parent - through IORT device <-> ITS mappings and reserves it so that\r\n> > these regions will not be translated by IOMMU and will be excluded\r\n> > from IOVA allocations.\r\n> >\r\n> > Signed-off-by: Shameer Kolothum\r\n> \r\n> > [lorenzo.pieralisi@arm.com: updated commit log/added comments]\r\n> > Signed-off-by: Lorenzo Pieralisi \r\n> > ---\r\n> > drivers/acpi/arm64/iort.c | 96\r\n> ++++++++++++++++++++++++++++++++++++++--\r\n> > drivers/irqchip/irq-gic-v3-its.c | 3 +-\r\n> > include/linux/acpi_iort.h | 7 ++-\r\n> > 3 files changed, 101 insertions(+), 5 deletions(-)\r\n> >\r\n> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c\r\n> > index 9565d57..14efa9d 100644\r\n> > --- a/drivers/acpi/arm64/iort.c\r\n> > +++ b/drivers/acpi/arm64/iort.c\r\n> > @@ -39,6 +39,7 @@\r\n> > struct iort_its_msi_chip {\r\n> > \tstruct list_head\tlist;\r\n> > \tstruct fwnode_handle\t*fw_node;\r\n> > +\tphys_addr_t\t\tbase_addr;\r\n> > \tu32\t\t\ttranslation_id;\r\n> > };\r\n> >\r\n> > @@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)\r\n> > static DEFINE_SPINLOCK(iort_msi_chip_lock);\r\n> >\r\n> > /**\r\n> > - * iort_register_domain_token() - register domain token and related\r\n> > ITS ID\r\n> > - * to the list from where we can get it back later on.\r\n> > + * iort_register_domain_token() - register domain token along with\r\n> > + related\r\n> > + * ITS ID and base address to the list from where we can get it back later\r\n> on.\r\n> > * @trans_id: ITS ID.\r\n> > + * @base: ITS base address.\r\n> > * @fw_node: Domain token.\r\n> > *\r\n> > * Returns: 0 on success, -ENOMEM if no memory when allocating list\r\n> element\r\n> > */\r\n> > -int iort_register_domain_token(int trans_id, struct fwnode_handle\r\n> > *fw_node)\r\n> > +int iort_register_domain_token(int trans_id, phys_addr_t base,\r\n> > +\t\t\t struct fwnode_handle *fw_node)\r\n> > {\r\n> > \tstruct iort_its_msi_chip *its_msi_chip;\r\n> >\r\n> > @@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id,\r\n> > struct fwnode_handle *fw_node)\r\n> >\r\n> > \tits_msi_chip->fw_node = fw_node;\r\n> > \tits_msi_chip->translation_id = trans_id;\r\n> > +\tits_msi_chip->base_addr = base;\r\n> >\r\n> > \tspin_lock(&iort_msi_chip_lock);\r\n> > \tlist_add(&its_msi_chip->list, &iort_msi_chip_list); @@ -481,6\r\n> > +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)\r\n> > \treturn -ENODEV;\r\n> > }\r\n> >\r\n> > +static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t\r\n> > +*base) {\r\n> > +\tstruct iort_its_msi_chip *its_msi_chip;\r\n> > +\tbool match = false;\r\n> > +\r\n> > +\tspin_lock(&iort_msi_chip_lock);\r\n> > +\tlist_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {\r\n> > +\t\tif (its_msi_chip->translation_id == its_id) {\r\n> > +\t\t\t*base = its_msi_chip->base_addr;\r\n> > +\t\t\tmatch = true;\r\n> > +\t\t\tbreak;\r\n> > +\t\t}\r\n> > +\t}\r\n> > +\tspin_unlock(&iort_msi_chip_lock);\r\n> > +\r\n> > +\treturn match ? 0 : -ENODEV;\r\n> > +}\r\n> > +\r\n> > /**\r\n> > * iort_dev_find_its_id() - Find the ITS identifier for a device\r\n> > * @dev: The device.\r\n> > @@ -639,6 +661,72 @@ int iort_add_device_replay(const struct\r\n> iommu_ops\r\n> > *ops, struct device *dev)\r\n> >\r\n> > \treturn err;\r\n> > }\r\n> > +\r\n> > +/**\r\n> > + * iort_iommu_msi_get_resv_regions - Reserved region driver helper\r\n> > + * @dev: Device from iommu_get_resv_regions()\r\n> > + * @head: Reserved region list from iommu_get_resv_regions()\r\n> > + *\r\n> > + * Returns: Number of reserved regions on success (0 if no associated msi\r\n> > + * regions), appropriate error value otherwise. The ITS regions\r\n> > + * associated with the device are the msi reserved regions.\r\n> > + */\r\n> > +int iort_iommu_msi_get_resv_regions(struct device *dev, struct\r\n> > +list_head *head) {\r\n> > +\tstruct acpi_iort_its_group *its;\r\n> > +\tstruct acpi_iort_node *node, *its_node = NULL;\r\n> > +\tint i, resv = 0;\r\n> > +\r\n> > +\tnode = iort_find_dev_node(dev);\r\n> > +\tif (!node)\r\n> > +\t\treturn -ENODEV;\r\n> > +\r\n> > +\t/*\r\n> > +\t * Current logic to reserve ITS regions relies on HW topologies\r\n> > +\t * where a given PCI or named component maps its IDs to only one\r\n> > +\t * ITS group; if a PCI or named component can map its IDs to\r\n> > +\t * different ITS groups through IORT mappings this function has\r\n> > +\t * to be reworked to ensure we reserve regions for all ITS groups\r\n> > +\t * a given PCI or named component may map IDs to.\r\n> > +\t */\r\n> > +\tif (dev_is_pci(dev)) {\r\n> > +\t\tu32 rid;\r\n> > +\r\n> > +\t\tpci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,\r\n> &rid);\r\n> > +\t\tits_node = iort_node_map_id(node, rid, NULL,\r\n> IORT_MSI_TYPE);\r\n> > +\t} else {\r\n> > +\t\tfor (i = 0; i < node->mapping_count; i++) {\r\n> > +\t\t\tits_node = iort_node_map_platform_id(node, NULL,\r\n> > +\t\t\t\t\t\t\t IORT_MSI_TYPE, i);\r\n> > +\t\t\tif (its_node)\r\n> > +\t\t\t\tbreak;\r\n> > +\t\t}\r\n> > +\t}\r\n> > +\r\n> > +\tif (!its_node)\r\n> > +\t\treturn 0;\r\n> > +\r\n> > +\t/* Move to ITS specific data */\r\n> > +\tits = (struct acpi_iort_its_group *)its_node->node_data;\r\n> > +\r\n> > +\tfor (i = 0; i < its->its_count; i++) {\r\n> > +\t\tphys_addr_t base;\r\n> > +\r\n> > +\t\tif (!iort_find_its_base(its->identifiers[i], &base)) {\r\n> > +\t\t\tint prot = IOMMU_WRITE | IOMMU_NOEXEC |\r\n> IOMMU_MMIO;\r\n> > +\t\t\tstruct iommu_resv_region *region;\r\n> > +\r\n> > +\t\t\tregion = iommu_alloc_resv_region(base, SZ_128K,\r\n> prot,\r\n> > +\t\t\t\t\t\t\t IOMMU_RESV_MSI);\r\n> \r\n> Same as the OF part: I strongly object to reserving the whole 128kB range.\r\n> What we really care about is the second 64kB page, and that is what should\r\n> get reserved.\r\n\r\nThanks Marc. I will make the changes in next revision.\r\n\r\nAlso as we are still discussing about the DT approach for this, I am thinking\r\nof sending out the v9 with the above fix and blacklisting the HiSilicon PCIe\r\ncontrollers on DT based hip06/hip07 systems when SMMUv3 is enabled.\r\n\r\nHi Will,\r\n\r\nHope that will address your concerns with respect to only having ACPI quirk\r\nfor this. \r\n\r\nThanks,\r\nShameer","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y7lxK6b0mz9t4R\n\tfor ;\n\tFri, 6 Oct 2017 21:19:33 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751774AbdJFKTc (ORCPT\n\t);\n\tFri, 6 Oct 2017 06:19:32 -0400","from lhrrgout.huawei.com ([194.213.3.17]:37277 \"EHLO\n\tlhrrgout.huawei.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750915AbdJFKTb (ORCPT\n\t); Fri, 6 Oct 2017 06:19:31 -0400","from 172.18.7.190 (EHLO LHREML713-CAH.china.huawei.com)\n\t([172.18.7.190])\n\tby lhrrg01-dlp.huawei.com (MOS 4.3.7-GA FastPath queued)\n\twith ESMTP id DWY80627; Fri, 06 Oct 2017 10:17:32 +0000 (GMT)","from FRAEML701-CAH.china.huawei.com (10.206.14.32) by\n\tLHREML713-CAH.china.huawei.com (10.201.108.36) with Microsoft SMTP\n\tServer (TLS) id 14.3.301.0; Fri, 6 Oct 2017 11:17:28 +0100","from FRAEML521-MBX.china.huawei.com ([169.254.1.161]) by\n\tFRAEML701-CAH.china.huawei.com ([10.206.14.32]) with mapi id\n\t14.03.0301.000; Fri, 6 Oct 2017 12:17:17 +0200"],"From":"Shameerali Kolothum Thodi ","To":"Marc Zyngier ,\n\t\"lorenzo.pieralisi@arm.com\" ,\n\t\"sudeep.holla@arm.com\" ,\n\t\"will.deacon@arm.com\" ,\n\t\"robin.murphy@arm.com\" ,\n\t\"joro@8bytes.org\" ,\n\t\"mark.rutland@arm.com\" ,\n\t\"robh@kernel.org\" ","CC":"Gabriele Paoloni ,\n\tJohn Garry ,\n\t\"iommu@lists.linux-foundation.org\" ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-acpi@vger.kernel.org\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"devel@acpica.org\" , Linuxarm ,\n\t\"Wangzhou (B)\" ,\n\t\"Guohanjun (Hanjun Guo)\" ","Subject":"RE: [PATCH v8 2/5] ACPI/IORT: Add msi address regions reservation\n\thelper","Thread-Topic":"[PATCH v8 2/5] ACPI/IORT: Add msi address regions reservation\n\thelper","Thread-Index":"AQHTN5Vy5QEmsxwVdU2bqzIuBNKwQqLTpmAAgAL/AiA=","Date":"Fri, 6 Oct 2017 10:17:17 +0000","Message-ID":"<5FC3163CFD30C246ABAA99954A238FA83841DB56@FRAEML521-MBX.china.huawei.com>","References":"<20170927133241.21036-1-shameerali.kolothum.thodi@huawei.com>\n\t<20170927133241.21036-3-shameerali.kolothum.thodi@huawei.com>\n\t<001729e8-7b34-0bb4-8af6-8af100661906@arm.com>","In-Reply-To":"<001729e8-7b34-0bb4-8af6-8af100661906@arm.com>","Accept-Language":"en-GB, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.202.227.237]","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-CFilter-Loop":"Reflected","X-Mirapoint-Virus-RAPID-Raw":"score=unknown(0),\n\trefid=str=0001.0A0C0208.59D7583E.00F3, ss=1, re=0.000, recu=0.000,\n\treip=0.000, \n\tcl=1, cld=1, fgs=0, ip=169.254.1.161,\n\tso=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32","X-Mirapoint-Loop-Id":"2abc8361fd43b2863f2ff8335abeb72d","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]