RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-06-08 Thread Shameerali Kolothum Thodi 


> -Original Message-
> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> Sent: Thursday, June 08, 2017 11:15 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyng...@arm.com; sudeep.ho...@arm.com; will.dea...@arm.com;
> robin.mur...@arm.com; hanjun@linaro.org; Gabriele Paoloni; John
> Garry; iommu@lists.linux-foundation.org; linux-arm-
> ker...@lists.infradead.org; linux-a...@vger.kernel.org; de...@acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Thu, Jun 08, 2017 at 09:09:28AM +, Shameerali Kolothum Thodi
> wrote:
> >
> >
> > > -Original Message-
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> > > Sent: Thursday, June 08, 2017 9:49 AM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyng...@arm.com; sudeep.ho...@arm.com;
> will.dea...@arm.com;
> > > robin.mur...@arm.com; hanjun@linaro.org; Gabriele Paoloni; John
> > > Garry; iommu@lists.linux-foundation.org; linux-arm-
> > > ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> de...@acpica.org;
> > > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Tue, Jun 06, 2017 at 03:01:36PM +, Shameerali Kolothum Thodi
> > > wrote:
> > >
> > > [...]
> > >
> > > > > > +   irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > > > +   if (irq_dom) {
> > > > > > +   int ret;
> > > > > > +   u32 rid;
> > > > > > +
> > > > > > +   rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > > > to_pci_dev(dev));
> > > > > > +   ret = iort_dev_find_its_base(dev, rid, 0, );
> > > > >
> > > > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > > > uses the same policy for getting the irq_domain (ie we want to
> > > > > reserve the ITS address space that is actually used by the device to
> > > > > send IRQs not a a different one) it is just a heads-up because I find
> this
> > > confusing.
> > > >
> > > > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > > > list.  I noted that iort_get_device_domain() uses index 0 while
> > > > retrieving the ITS identifier.  May be use the same approach here as
> > > > well? ie, remove the index from function call?
> > > >
> > > > I am not sure, how we can get the index info  though theoretically It
> > > > is possible for the ITS group node having multiple ITSs.
> > >
> > > Actually I think it would make sense to reserve ALL ITS regions a device
> may
> > > be mapped to instead of just index 0 (ie in your case it is equivalent); 
> > > this
> > > leaves us some leeway as to choose which ITS the device will be actually
> > > mapped to and this code does not have to care.
> >
> > Ok. That make sense. Just a quick one, is it ok to add another helper
> function in
> > iort code to retrieve the its->its_count then?
> 
> While at it, given that the pci API code to retrieve domain and rid falls
> back to IORT anyway, I would add the whole reservation to IORT (mind,
> it depends on IOMMU_API) as one function instead of fiddling about with
> indexes.

Ok. I will take a look at this. 

> Side note: why Hilisicon dts upstream (eg hip07.dtsi) report ITS size
> as 256K ? I was just checking whether the ITS reg map size is system
> dependent and I bumped into them, I suspect there may be some dts
> patching needed here.
> 

Thanks for catching this. I will check with the team and update.

Shameer

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-06-08 Thread Shameerali Kolothum Thodi 


> -Original Message-
> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> Sent: Wednesday, June 07, 2017 6:16 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyng...@arm.com; sudeep.ho...@arm.com; will.dea...@arm.com;
> robin.mur...@arm.com; hanjun@linaro.org; Gabriele Paoloni; John
> Garry; iommu@lists.linux-foundation.org; linux-arm-
> ker...@lists.infradead.org; linux-a...@vger.kernel.org; de...@acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +, Shameerali Kolothum Thodi
> wrote:
> > Hi Lorenzo,
> >
> > > -Original Message-
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> > > Sent: Tuesday, June 06, 2017 2:56 PM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyng...@arm.com; sudeep.ho...@arm.com;
> will.dea...@arm.com;
> > > robin.mur...@arm.com; hanjun@linaro.org; Gabriele Paoloni; John
> > > Garry; iommu@lists.linux-foundation.org; linux-arm-
> > > ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> de...@acpica.org;
> > > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > > The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> > > > platforms Hip06/Hip07 to support the SMMU mappings for MSI
> > > transactions.
> > > >
> > > > On these platforms GICv3 ITS translator is presented with the deviceID
> > > > by extending the MSI payload data to 64 bits to include the deviceID.
> > > > Hence, the PCIe controller on this platforms has to differentiate the
> > > > MSI payload against other DMA payload and has to modify the MSI
> > > payload.
> > > > This basically makes it difficult for this platforms to have a SMMU
> > > > translation for MSI.
> > > >
> > > > This patch implements a ACPI table based quirk to reserve the hw msi
> > > > regions in the smmu-v3 driver which means these address regions will
> > > > not be translated and will be excluded from iova allocations.
> > > >
> > > > The HW ITS address region associated with the dev is retrieved using a
> > > > new helper function added in the IORT code.
> > >
> > > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > > helper function in this patch but you actually aren't.
> >
> > Thanks for going through this patch series. I will remove this in next
> version.
> >
> > > > Signed-off-by: shameer <shameerali.kolothum.th...@huawei.com>
> > > > ---
> > > >  drivers/iommu/arm-smmu-v3.c | 49
> > > > ++---
> > > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-
> smmu-
> > > v3.c
> > > > index abe4b88..3767526 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > > > u32 features;
> > > >
> > > >  #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
> > > > +#define ARM_SMMU_OPT_RESV_HW_MSI   (1 << 1)
> > > > u32 options;
> > > >
> > > > struct arm_smmu_cmdqcmdq;
> > > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > > arm_smmu_device *smmu, u32 sid)
> > > >
> > > >  static struct iommu_ops arm_smmu_ops;
> > > >
> > > > +#ifdef CONFIG_ACPI
> > > > +static struct iommu_resv_region
> *arm_smmu_acpi_alloc_hw_msi(struct
> > > > +device *dev) {
> > > > +   struct iommu_resv_region *region;
> > > > +   struct  irq_domain *irq_dom;
> > > > +   int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > > +   u64 base;
> > >
> > > phys_addr_t
> >
> > Ok.
> >
> > > > +   irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +   if (irq_dom) {
> > > > +   int ret;
> > > > +

RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-06-08 Thread Shameerali Kolothum Thodi 


> -Original Message-
> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> Sent: Thursday, June 08, 2017 9:49 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyng...@arm.com; sudeep.ho...@arm.com; will.dea...@arm.com;
> robin.mur...@arm.com; hanjun@linaro.org; Gabriele Paoloni; John
> Garry; iommu@lists.linux-foundation.org; linux-arm-
> ker...@lists.infradead.org; linux-a...@vger.kernel.org; de...@acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +, Shameerali Kolothum Thodi
> wrote:
> 
> [...]
> 
> > > > +   irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +   if (irq_dom) {
> > > > +   int ret;
> > > > +   u32 rid;
> > > > +
> > > > +   rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > to_pci_dev(dev));
> > > > +   ret = iort_dev_find_its_base(dev, rid, 0, );
> > >
> > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > uses the same policy for getting the irq_domain (ie we want to
> > > reserve the ITS address space that is actually used by the device to
> > > send IRQs not a a different one) it is just a heads-up because I find this
> confusing.
> >
> > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > list.  I noted that iort_get_device_domain() uses index 0 while
> > retrieving the ITS identifier.  May be use the same approach here as
> > well? ie, remove the index from function call?
> >
> > I am not sure, how we can get the index info  though theoretically It
> > is possible for the ITS group node having multiple ITSs.
> 
> Actually I think it would make sense to reserve ALL ITS regions a device may
> be mapped to instead of just index 0 (ie in your case it is equivalent); this
> leaves us some leeway as to choose which ITS the device will be actually
> mapped to and this code does not have to care.

Ok. That make sense. Just a quick one, is it ok to add another helper function 
in
iort code to retrieve the its->its_count then? 

Thanks,
Shameer
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-06-08 Thread Lorenzo Pieralisi 
On Tue, Jun 06, 2017 at 03:01:36PM +, Shameerali Kolothum Thodi wrote:

[...]

> > > + irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > + if (irq_dom) {
> > > + int ret;
> > > + u32 rid;
> > > +
> > > + rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > + ret = iort_dev_find_its_base(dev, rid, 0, );
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the 
> > same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different 
> > one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier
> list.  I noted that iort_get_device_domain() uses index 0 while
> retrieving the ITS identifier.  May be use the same approach here as
> well? ie, remove the index from function call?
> 
> I am not sure, how we can get the index info  though theoretically It
> is possible for the ITS group node having multiple ITSs.

Actually I think it would make sense to reserve ALL ITS regions a device
may be mapped to instead of just index 0 (ie in your case it is
equivalent); this leaves us some leeway as to choose which ITS the
device will be actually mapped to and this code does not have to care.

Lorenzo

>  
> > > + if (!ret) {
> > > + dev_info(dev, "SMMUv3:HW MSI resv addr
> > 0x%pa\n", );
> > > + region = iommu_alloc_resv_region(base, SZ_128K,
> > > +  prot,
> > IOMMU_RESV_MSI);
> > > + return region;
> > > + }
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +#else
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > + return NULL;
> > > +}
> > > +#endif
> > > +
> > >  static int arm_smmu_add_device(struct device *dev)  {
> > >   int i, ret;
> > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > *dev, struct of_phandle_args *args)  static void
> > arm_smmu_get_resv_regions(struct device *dev,
> > > struct list_head *head)
> > >  {
> > > - struct iommu_resv_region *region;
> > > + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > + struct iommu_resv_region *region = NULL;
> > >   int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > + struct arm_smmu_device *smmu;
> > > +
> > > + smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > >
> > > - region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > MSI_IOVA_LENGTH,
> > > -  prot, IOMMU_RESV_SW_MSI);
> > > + if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > &&
> > > +   dev_is_pci(dev))
> > > + region = arm_smmu_acpi_alloc_hw_msi(dev);
> > 
> > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?
> 
> It is just that PCIe devices won't be functional on this platforms as the 
> endpoint will 
> be configured with ITS IOVA address. May be I should add some dev_warn() here.
> 
> Thanks,
> Shameer
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-06-07 Thread Lorenzo Pieralisi 
On Tue, Jun 06, 2017 at 03:01:36PM +, Shameerali Kolothum Thodi wrote:
> Hi Lorenzo,
> 
> > -Original Message-
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> > Sent: Tuesday, June 06, 2017 2:56 PM
> > To: Shameerali Kolothum Thodi
> > Cc: marc.zyng...@arm.com; sudeep.ho...@arm.com; will.dea...@arm.com;
> > robin.mur...@arm.com; hanjun@linaro.org; Gabriele Paoloni; John
> > Garry; iommu@lists.linux-foundation.org; linux-arm-
> > ker...@lists.infradead.org; linux-a...@vger.kernel.org; de...@acpica.org;
> > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> > erratum 161010801
> > 
> > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> > > platforms Hip06/Hip07 to support the SMMU mappings for MSI
> > transactions.
> > >
> > > On these platforms GICv3 ITS translator is presented with the deviceID
> > > by extending the MSI payload data to 64 bits to include the deviceID.
> > > Hence, the PCIe controller on this platforms has to differentiate the
> > > MSI payload against other DMA payload and has to modify the MSI
> > payload.
> > > This basically makes it difficult for this platforms to have a SMMU
> > > translation for MSI.
> > >
> > > This patch implements a ACPI table based quirk to reserve the hw msi
> > > regions in the smmu-v3 driver which means these address regions will
> > > not be translated and will be excluded from iova allocations.
> > >
> > > The HW ITS address region associated with the dev is retrieved using a
> > > new helper function added in the IORT code.
> > 
> > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > helper function in this patch but you actually aren't.
> 
> Thanks for going through this patch series. I will remove this in next 
> version.
> 
> > > Signed-off-by: shameer <shameerali.kolothum.th...@huawei.com>
> > > ---
> > >  drivers/iommu/arm-smmu-v3.c | 49
> > > ++---
> > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-
> > v3.c
> > > index abe4b88..3767526 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > >   u32 features;
> > >
> > >  #define ARM_SMMU_OPT_SKIP_PREFETCH   (1 << 0)
> > > +#define ARM_SMMU_OPT_RESV_HW_MSI (1 << 1)
> > >   u32 options;
> > >
> > >   struct arm_smmu_cmdqcmdq;
> > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > arm_smmu_device *smmu, u32 sid)
> > >
> > >  static struct iommu_ops arm_smmu_ops;
> > >
> > > +#ifdef CONFIG_ACPI
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > + struct iommu_resv_region *region;
> > > + struct  irq_domain *irq_dom;
> > > + int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > + u64 base;
> > 
> > phys_addr_t
> 
> Ok.
> 
> > > + irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > + if (irq_dom) {
> > > + int ret;
> > > + u32 rid;
> > > +
> > > + rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > + ret = iort_dev_find_its_base(dev, rid, 0, );
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the 
> > same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different 
> > one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier list.
> I noted that iort_get_device_domain() uses index 0 while retrieving the ITS 
> identifier.
> May be use the same approach here as well? ie, remove the index from function 
> call?
> 
> I am not sure, how we can get the index info  though theoretically It is 
> possible for
> the ITS group node having multiple ITSs.

Yes, it would be ideal to avoid the look-up through the ITS index and
just reuse the ITS node associated wi

RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-06-06 Thread Shameerali Kolothum Thodi 
Hi Lorenzo,

> -Original Message-
> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> Sent: Tuesday, June 06, 2017 2:56 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyng...@arm.com; sudeep.ho...@arm.com; will.dea...@arm.com;
> robin.mur...@arm.com; hanjun@linaro.org; Gabriele Paoloni; John
> Garry; iommu@lists.linux-foundation.org; linux-arm-
> ker...@lists.infradead.org; linux-a...@vger.kernel.org; de...@acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> > platforms Hip06/Hip07 to support the SMMU mappings for MSI
> transactions.
> >
> > On these platforms GICv3 ITS translator is presented with the deviceID
> > by extending the MSI payload data to 64 bits to include the deviceID.
> > Hence, the PCIe controller on this platforms has to differentiate the
> > MSI payload against other DMA payload and has to modify the MSI
> payload.
> > This basically makes it difficult for this platforms to have a SMMU
> > translation for MSI.
> >
> > This patch implements a ACPI table based quirk to reserve the hw msi
> > regions in the smmu-v3 driver which means these address regions will
> > not be translated and will be excluded from iova allocations.
> >
> > The HW ITS address region associated with the dev is retrieved using a
> > new helper function added in the IORT code.
> 
> Remove or rephrase last paragraph, it reads as if you are adding an IORT
> helper function in this patch but you actually aren't.

Thanks for going through this patch series. I will remove this in next version.

> > Signed-off-by: shameer <shameerali.kolothum.th...@huawei.com>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 49
> > ++---
> >  1 file changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-
> v3.c
> > index abe4b88..3767526 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > u32 features;
> >
> >  #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
> > +#define ARM_SMMU_OPT_RESV_HW_MSI   (1 << 1)
> > u32 options;
> >
> > struct arm_smmu_cmdqcmdq;
> > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > arm_smmu_device *smmu, u32 sid)
> >
> >  static struct iommu_ops arm_smmu_ops;
> >
> > +#ifdef CONFIG_ACPI
> > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > +device *dev) {
> > +   struct iommu_resv_region *region;
> > +   struct  irq_domain *irq_dom;
> > +   int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > +   u64 base;
> 
> phys_addr_t

Ok.

> > +   irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > +   if (irq_dom) {
> > +   int ret;
> > +   u32 rid;
> > +
> > +   rid = pci_msi_domain_get_msi_rid(irq_dom,
> to_pci_dev(dev));
> > +   ret = iort_dev_find_its_base(dev, rid, 0, );
> 
> Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> policy for getting the irq_domain (ie we want to reserve the ITS address
> space that is actually used by the device to send IRQs not a a different one) 
> it
> is just a heads-up because I find this confusing.

Ok. Just to make it clear, 0 is the index into the ITS identifier list.
I noted that iort_get_device_domain() uses index 0 while retrieving the ITS 
identifier.
May be use the same approach here as well? ie, remove the index from function 
call?

I am not sure, how we can get the index info  though theoretically It is 
possible for
the ITS group node having multiple ITSs.
 
> > +   if (!ret) {
> > +   dev_info(dev, "SMMUv3:HW MSI resv addr
> 0x%pa\n", );
> > +   region = iommu_alloc_resv_region(base, SZ_128K,
> > +prot,
> IOMMU_RESV_MSI);
> > +   return region;
> > +   }
> > +   }
> > +
> > +   return NULL;
> > +}
> > +#else
> > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > +device *dev) {
> > +   return NULL;
> > +}
> > +#endif
> > +
> >  static int arm_smmu_add_device(stru

Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-06-06 Thread Lorenzo Pieralisi 
On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
> 
> On these platforms GICv3 ITS translator is presented with the deviceID
> by extending the MSI payload data to 64 bits to include the deviceID.
> Hence, the PCIe controller on this platforms has to differentiate the
> MSI payload against other DMA payload and has to modify the MSI payload.
> This basically makes it difficult for this platforms to have a SMMU
> translation for MSI.
> 
> This patch implements a ACPI table based quirk to reserve the hw msi
> regions in the smmu-v3 driver which means these address regions will
> not be translated and will be excluded from iova allocations.
> 
> The HW ITS address region associated with the dev is retrieved
> using a new helper function added in the IORT code.

Remove or rephrase last paragraph, it reads as if you are adding an IORT
helper function in this patch but you actually aren't.

> Signed-off-by: shameer 
> ---
>  drivers/iommu/arm-smmu-v3.c | 49 
> ++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index abe4b88..3767526 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -597,6 +597,7 @@ struct arm_smmu_device {
>   u32 features;
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH   (1 << 0)
> +#define ARM_SMMU_OPT_RESV_HW_MSI (1 << 1)
>   u32 options;
>  
>   struct arm_smmu_cmdqcmdq;
> @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct 
> arm_smmu_device *smmu, u32 sid)
>  
>  static struct iommu_ops arm_smmu_ops;
>  
> +#ifdef CONFIG_ACPI
> +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device 
> *dev)
> +{
> + struct iommu_resv_region *region;
> + struct  irq_domain *irq_dom;
> + int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> + u64 base;

phys_addr_t

> + irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> + if (irq_dom) {
> + int ret;
> + u32 rid;
> +
> + rid = pci_msi_domain_get_msi_rid(irq_dom, to_pci_dev(dev));
> + ret = iort_dev_find_its_base(dev, rid, 0, );

Well, here we use ITS id 0 which is fine as long as code in IORT uses
the same policy for getting the irq_domain (ie we want to reserve the
ITS address space that is actually used by the device to send IRQs not a
a different one) it is just a heads-up because I find this confusing.

> + if (!ret) {
> + dev_info(dev, "SMMUv3:HW MSI resv addr 0x%pa\n", );
> + region = iommu_alloc_resv_region(base, SZ_128K,
> +  prot, IOMMU_RESV_MSI);
> + return region;
> + }
> + }
> +
> + return NULL;
> +}
> +#else
> +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device 
> *dev)
> +{
> + return NULL;
> +}
> +#endif
> +
>  static int arm_smmu_add_device(struct device *dev)
>  {
>   int i, ret;
> @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device *dev, 
> struct of_phandle_args *args)
>  static void arm_smmu_get_resv_regions(struct device *dev,
> struct list_head *head)
>  {
> - struct iommu_resv_region *region;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct iommu_resv_region *region = NULL;
>   int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> + struct arm_smmu_device *smmu;
> +
> + smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
>  
> - region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> -  prot, IOMMU_RESV_SW_MSI);
> + if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI) &&
> +   dev_is_pci(dev))
> + region = arm_smmu_acpi_alloc_hw_msi(dev);

Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?

Lorenzo

> + if (!region)
> + region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> +  prot, IOMMU_RESV_SW_MSI);
>   if (!region)
>   return;
>  
> @@ -2611,6 +2653,7 @@ static void parse_driver_acpi_options(struct 
> acpi_iort_smmu_v3 *iort_smmu,
>   switch (iort_smmu->model) {
>   case ACPI_IORT_SMMU_HISILICON_HI161X:
>   smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
> + smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
>   break;
>   default:
>   break;
> -- 
> 1.9.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

2017-05-31 Thread shameer 
The HiSilicon erratum 161010801 describes the limitation of HiSilicon
platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.

On these platforms GICv3 ITS translator is presented with the deviceID
by extending the MSI payload data to 64 bits to include the deviceID.
Hence, the PCIe controller on this platforms has to differentiate the
MSI payload against other DMA payload and has to modify the MSI payload.
This basically makes it difficult for this platforms to have a SMMU
translation for MSI.

This patch implements a ACPI table based quirk to reserve the hw msi
regions in the smmu-v3 driver which means these address regions will
not be translated and will be excluded from iova allocations.

The HW ITS address region associated with the dev is retrieved
using a new helper function added in the IORT code.

Signed-off-by: shameer 
---
 drivers/iommu/arm-smmu-v3.c | 49 ++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index abe4b88..3767526 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -597,6 +597,7 @@ struct arm_smmu_device {
u32 features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
+#define ARM_SMMU_OPT_RESV_HW_MSI   (1 << 1)
u32 options;
 
struct arm_smmu_cmdqcmdq;
@@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device 
*smmu, u32 sid)
 
 static struct iommu_ops arm_smmu_ops;
 
+#ifdef CONFIG_ACPI
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
+{
+   struct iommu_resv_region *region;
+   struct  irq_domain *irq_dom;
+   int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+   u64 base;
+
+   irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
+   if (irq_dom) {
+   int ret;
+   u32 rid;
+
+   rid = pci_msi_domain_get_msi_rid(irq_dom, to_pci_dev(dev));
+   ret = iort_dev_find_its_base(dev, rid, 0, );
+   if (!ret) {
+   dev_info(dev, "SMMUv3:HW MSI resv addr 0x%pa\n", );
+   region = iommu_alloc_resv_region(base, SZ_128K,
+prot, IOMMU_RESV_MSI);
+   return region;
+   }
+   }
+
+   return NULL;
+}
+#else
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
+{
+   return NULL;
+}
+#endif
+
 static int arm_smmu_add_device(struct device *dev)
 {
int i, ret;
@@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device *dev, struct 
of_phandle_args *args)
 static void arm_smmu_get_resv_regions(struct device *dev,
  struct list_head *head)
 {
-   struct iommu_resv_region *region;
+   struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+   struct iommu_resv_region *region = NULL;
int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+   struct arm_smmu_device *smmu;
+
+   smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
 
-   region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-prot, IOMMU_RESV_SW_MSI);
+   if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI) &&
+ dev_is_pci(dev))
+   region = arm_smmu_acpi_alloc_hw_msi(dev);
+
+   if (!region)
+   region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+prot, IOMMU_RESV_SW_MSI);
if (!region)
return;
 
@@ -2611,6 +2653,7 @@ static void parse_driver_acpi_options(struct 
acpi_iort_smmu_v3 *iort_smmu,
switch (iort_smmu->model) {
case ACPI_IORT_SMMU_HISILICON_HI161X:
smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
+   smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
break;
default:
break;
-- 
1.9.1


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu