Re: [PATCH 0/3] SMMUv3 CMD_SYNC optimisation

[Nate Watterson]

Hi Robin,

On 8/18/2017 1:33 PM, Robin Murphy wrote:

Hi all,

Waiting for the command queue to drain for CMD_SYNC completion is likely
a contention hotspot on high-core-count systems. If the SMMU is coherent
and supports MSIs, though, we can use this cool feature (as suggested by
the architecture, no less) to make syncs effectively non-blocking for
anyone other than the caller.

I don't have any hardware that supports MSIs, but this has at least
passed muster on the Fast Model with cache modelling enabled - I'm hoping
the Qualcomm machines have the appropriate configuration to actually test
how well it works in reality. If it is worthwhile, I do have most of a
plan for how we can do something similar in the non-MSI polling case (it's
mostly a problem of handling the queue-wrapping edge cases correctly).


I tested this on QDF2400 hardware which supports MSI as a CMD_SYNC
completion signal. As with Thunder's "performance optimization" series,
I evaluated the patches using FIO with 4 NVME drives connected to a
single SMMU. Here's how they compared:

FIO - 512k blocksize / io-depth 32 / 1 thread per drive
 Baseline 4.13-rc1 w/SMMU enabled: 25% of SMMU bypass performance
 Baseline + Thunder Patch 1  : 28%
 Baseline + CMD_SYNC Optimization: 36%
 Baseline + Thunder Patches 2-5  : 86%
 Baseline + Thunder Patches 1-5  : 100% [!!]

Seems like it would probably be worthwhile to implement this for the
non-MSI case also. Let me know if there are other workloads you're
particularly interested in, and I'll try to get those tested too.

-Nate


Robin.


Robin Murphy (3):
   iommu/arm-smmu-v3: Specialise CMD_SYNC handling
   iommu/arm-smmu-v3: Forget about cmdq-sync interrupt
   iommu/arm-smmu-v3: Utilise CMD_SYNC MSI feature

  drivers/iommu/arm-smmu-v3.c | 117 +---
  1 file changed, 77 insertions(+), 40 deletions(-)



--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 13/13] remoteproc: Use sychronized interface of the IOMMU-API

[Bjorn Andersson]

On Thu 17 Aug 05:56 PDT 2017, Joerg Roedel wrote:

> From: Joerg Roedel 
> 
> The map and unmap functions of the IOMMU-API changed their
> semantics: They do no longer guarantee that the hardware
> TLBs are synchronized with the page-table updates they made.
> 
> To make conversion easier, new synchronized functions have
> been introduced which give these guarantees again until the
> code is converted to use the new TLB-flush interface of the
> IOMMU-API, which allows certain optimizations.
> 
> But for now, just convert this code to use the synchronized
> functions so that it will behave as before.
> 
> Cc: Ohad Ben-Cohen 
> Cc: Bjorn Andersson 
> Cc: linux-remotep...@vger.kernel.org
> Signed-off-by: Joerg Roedel 

Acked-by: Bjorn Andersson 

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c 
> b/drivers/remoteproc/remoteproc_core.c
> index 564061d..16db19c 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -553,7 +553,8 @@ static int rproc_handle_devmem(struct rproc *rproc, 
> struct fw_rsc_devmem *rsc,
>   if (!mapping)
>   return -ENOMEM;
>  
> - ret = iommu_map(rproc->domain, rsc->da, rsc->pa, rsc->len, rsc->flags);
> + ret = iommu_map_sync(rproc->domain, rsc->da,
> +  rsc->pa, rsc->len, rsc->flags);
>   if (ret) {
>   dev_err(dev, "failed to map devmem: %d\n", ret);
>   goto out;
> @@ -661,8 +662,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>   goto dma_free;
>   }
>  
> - ret = iommu_map(rproc->domain, rsc->da, dma, rsc->len,
> - rsc->flags);
> + ret = iommu_map_sync(rproc->domain, rsc->da, dma, rsc->len,
> +  rsc->flags);
>   if (ret) {
>   dev_err(dev, "iommu_map failed: %d\n", ret);
>   goto free_mapping;
> @@ -823,7 +824,8 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>   list_for_each_entry_safe(entry, tmp, >mappings, node) {
>   size_t unmapped;
>  
> - unmapped = iommu_unmap(rproc->domain, entry->da, entry->len);
> + unmapped = iommu_unmap_sync(rproc->domain, entry->da,
> + entry->len);
>   if (unmapped != entry->len) {
>   /* nothing much to do besides complaining */
>   dev_err(dev, "failed to unmap %u/%zu\n", entry->len,
> -- 
> 2.7.4
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] iommu: Prevent VMD child devices from being remapping targets

[Bjorn Helgaas]

On Fri, Aug 18, 2017 at 11:04:33AM -0500, Bjorn Helgaas wrote:
> [+cc Robin]
> 
> This series looks fine to me as far as PCI is concerned, and I'd be
> happy to take it via my tree given an ack from David for this IOMMU
> piece.  Alternatively, you can add my
> 
> Acked-by: Bjorn Helgaas 
> 
> to the other patches if you want to take it via another tree.
> 
> Robin raised a question about basically this same patch the first time
> around.  Not sure whether there's still an objection there.

Ping, David, any thoughts on this patch?

> On Thu, Aug 17, 2017 at 12:10:14PM -0600, Jon Derrick wrote:
> > VMD child devices must use the VMD endpoint's ID as the requester.
> > Because of this, there needs to be a way to link the parent VMD
> > endpoint's iommu group and associated mappings to the VMD child devices
> > such that attaching and detaching child devices modify the endpoint's
> > mappings, while preventing early detaching on a singular device removal
> > or unbinding.
> > 
> > The reassignment of individual VMD child devices devices to VMs is
> > outside the scope of VMD, but may be implemented in the future. For now
> > it is best to prevent any such attempts.
> > 
> > This patch prevents VMD child devices from returning an IOMMU, which
> > prevents it from exposing an iommu_group sysfs directories and allowing
> > subsequent binding by userspace-access drivers such as VFIO.
> > 
> > Signed-off-by: Jon Derrick 
> > ---
> >  drivers/iommu/intel-iommu.c | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 687f18f..94353a6e 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -901,6 +901,11 @@ static struct intel_iommu *device_to_iommu(struct 
> > device *dev, u8 *bus, u8 *devf
> > struct pci_dev *pf_pdev;
> >  
> > pdev = to_pci_dev(dev);
> > +
> > +   /* VMD child devices currently cannot be handled individually */
> > +   if (is_vmd(pdev->bus))
> > +   return NULL;
> > +
> > /* VFs aren't listed in scope tables; we need to look up
> >  * the PF instead to find the IOMMU. */
> > pf_pdev = pci_physfn(pdev);
> > -- 
> > 2.9.4
> > 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 24/08/2017 15:35, Will Deacon wrote:

> >>OK, seems reasonable.
> >>
> >>We would consider blacklisting the device, where/how to do is the question.
> >>
> >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> >>in-between SMMU (driver) to provide the workaround. So my initial impression
> >>is that the PCI host controller would have to be blacklisted IFF behind an
> >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> >>sound?

> >
> >If that avoids us running into the erratum, then fine by me. I'd obviously
> >prefer we work-around it since we know how to, but that's up to you.

>
> I'm surpsised that you may want more errata workaround code to maintain.
>
> Anyway we'll check both approaches and show you how they look and go from
> there.

Don't get me wrong, I don't dream about adding errata workarounds to the
code, but our job as an operating system is to abstract the hardware from
the user, which means dealing with its quirks whether we like it or not.



Fine, it's ok.

For our next platform, hip08, we will provide no DT FW support, so this 
whole DT grey area should not be an issue.


And hopefully no errata also.

Much appreciated,
John


Thanks,

Will



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

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 23, 2017 at 05:55:52PM +0100, John Garry wrote:
> On 23/08/2017 17:43, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> >>On 23/08/2017 14:24, Will Deacon wrote:
> >>>On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>Signed-off-by: Shameer Kolothum
> >>
> >>>---
> >>>drivers/iommu/arm-smmu-v3.c | 27 ++-
> >>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >>Please can you also add a devicetree binding with corresponding
> >>documentation to enable this workaround on non-ACPI based systems too?
> >>It should be straightforward if you update the arm_smmu_options table.
> >
> >As I mentioned before, devicetree was a lower priority and we would 
> >definitely
> >submit patch to support that. Even if we update the arm_smmu_options 
> >table
> >with DT binding, the generic function to retrieve the MSI address 
> >regions only
> >works on ACPI/IORT case now.
> >
> 
> Hi Will,
> 
> Can you confirm your stance on supporting this workaround for DT as well 
> as
> ACPI?
> 
> For us, we now only "officially" support ACPI FW, and DT support at this
> point is patchy/limited. To me, adding DT support is just more errata
> workaround code to maintain with little useful gain.
> >>>
> >>>I basically don't like the idea of a driver that only works for one of
> >>>ACPI or DT yet claims to support both. I'm less fussed about functionality
> >>>differences (feature X is only available with firmware Y), but not working
> >>>around a hardware erratum that we know about is just lazy.
> >>>
> >>>So I'd prefer that we handle this in both cases, or blacklist affected
> >>>devices when booting with DT. Continuing as though there isn't an erratum
> >>>is the worst thing we can do.
> >>
> >>OK, seems reasonable.
> >>
> >>We would consider blacklisting the device, where/how to do is the question.
> >>
> >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> >>in-between SMMU (driver) to provide the workaround. So my initial impression
> >>is that the PCI host controller would have to be blacklisted IFF behind an
> >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> >>sound?
> >
> >If that avoids us running into the erratum, then fine by me. I'd obviously
> >prefer we work-around it since we know how to, but that's up to you.
> 
> I'm surpsised that you may want more errata workaround code to maintain.
> 
> Anyway we'll check both approaches and show you how they look and go from
> there.

Don't get me wrong, I don't dream about adding errata workarounds to the
code, but our job as an operating system is to abstract the hardware from
the user, which means dealing with its quirks whether we like it or not.

Thanks,

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

[PATCH v2] iommu/amd: Check if domain is NULL in get_domain() and return -EBUSY

[Baoquan He]

In get_domain(), 'domain' could be NULL before it's passed to dma_ops_domain()
to dereference. And the current code calling get_domain() can't deal with the
returned 'domain' well if its value is NULL.

So before dma_ops_domain() calling, check if 'domain' is NULL, If yes just 
return
ERR_PTR(-EBUSY) directly.

Reported-by: Dan Carpenter 
Fixes: df3f7a6e8e85 ('iommu/amd: Use is_attach_deferred call-back')
Signed-off-by: Baoquan He 
---
 drivers/iommu/amd_iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 16f1e6af00b0..2d7d04472555 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2262,6 +2262,9 @@ static struct protection_domain *get_domain(struct device 
*dev)
domain = to_pdomain(io_domain);
attach_device(dev, domain);
}
+   if (domain == NULL)
+   return ERR_PTR(-EBUSY);
+
if (!dma_ops_domain(domain))
return ERR_PTR(-EBUSY);
 
-- 
2.5.5

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

Re: [PATCH 4/4] iommu/pamu: Add support for generic iommu-device

[Joerg Roedel]

Hi Michael,

On Thu, Aug 24, 2017 at 12:04:13PM +1000, Michael Ellerman wrote:
> Joerg Roedel  writes:
> > Can you please try the attached patch?
> 
> Thanks, that works.

Great, thanks for testing it. I'll queue it on-top of the original
patch-set.

> 
> It boots happily, much later in boot I see these messages:
> 
> [2.085616] iommu: Adding device ffe301000.jr to group 9
> [2.091101] iommu: Adding device ffe302000.jr to group 10
> [2.096633] iommu: Adding device ffe303000.jr to group 11
> [2.102161] iommu: Adding device ffe304000.jr to group 12

Looks good.

> 
> In /sys I see:
> 
> root@p5020ds:~# ls -l /sys/class/iommu/iommu0
> lrwxrwxrwx 1 root root 0 Aug 24 12:01 /sys/class/iommu/iommu0 -> 
> ../../devices/virtual/iommu/iommu0
> 
> And:
> 
> root@p5020ds:~# ls -l /sys/devices/virtual/iommu/iommu0/devices/
> total 0
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 :00:00.0 -> 
> ../../../../platform/ffe20.pcie/pci:00/:00:00.0
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 2000:00:00.0 -> 
> ../../../../platform/ffe202000.pcie/pci2000:00/2000:00:00.0
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 2000:01:00.0 -> 
> ../../../../platform/ffe202000.pcie/pci2000:00/2000:00:00.0/2000:01:00.0
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 2000:01:00.1 -> 
> ../../../../platform/ffe202000.pcie/pci2000:00/2000:00:00.0/2000:01:00.1
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe100300.dma -> 
> ../../../../platform/ffe00.soc/ffe100300.dma
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe101300.dma -> 
> ../../../../platform/ffe00.soc/ffe101300.dma
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe114000.sdhc -> 
> ../../../../platform/ffe00.soc/ffe114000.sdhc
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe20.pcie -> 
> ../../../../platform/ffe20.pcie
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe202000.pcie -> 
> ../../../../platform/ffe202000.pcie
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe21.usb -> 
> ../../../../platform/ffe00.soc/ffe21.usb
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe211000.usb -> 
> ../../../../platform/ffe00.soc/ffe211000.usb
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe22.sata -> 
> ../../../../platform/ffe00.soc/ffe22.sata
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe221000.sata -> 
> ../../../../platform/ffe00.soc/ffe221000.sata
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe301000.jr -> 
> ../../../../platform/ffe00.soc/ffe30.crypto/ffe301000.jr
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe302000.jr -> 
> ../../../../platform/ffe00.soc/ffe30.crypto/ffe302000.jr
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe303000.jr -> 
> ../../../../platform/ffe00.soc/ffe30.crypto/ffe303000.jr
> lrwxrwxrwx 1 root root 0 Aug 24 12:02 ffe304000.jr -> 
> ../../../../platform/ffe00.soc/ffe30.crypto/ffe304000.jr
> 
> 
> Which seems like it's working?

Exactly, that is what I'd expect to see with the patches.


Thanks again and cheers,


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

Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it

[Baoquan He]

On 08/24/17 at 03:53pm, Dan Carpenter wrote:
> On Thu, Aug 24, 2017 at 08:47:33PM +0800, Baoquan He wrote:
> > On 08/24/17 at 03:32pm, Dan Carpenter wrote:
> > > Take a look at this code for example.  But all the places which call
> > > get_domain() are the same:
> > > 
> > > drivers/iommu/amd_iommu.c
> > >   2648  page = virt_to_page(virt_addr);
> > >   2649  size = PAGE_ALIGN(size);
> > >   2650  
> > >   2651  domain = get_domain(dev);
> > >  ^^
> > > imagined get_domain() returns NULL.
> > > 
> > >   2652  if (IS_ERR(domain))
> > >   2653  goto free_mem;
> > >   2654  
> > >   2655  dma_dom = to_dma_ops_domain(domain);
> > >   ^
> > > This will Oops.
> > 
> > I see, it's a problem. Thanks for telling!
> > 
> > How about below change? But I am not very sure which errno should be
> > picked, seems the latter one, EBUSY is better since it has passed the
> > check_device() checking.
> 
> Looks good to me.  You know better than I do which errno is best, so
> I'll leave that to you.

OK, thanks! Then let me post v2 with it.

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

Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it

[Dan Carpenter]

On Thu, Aug 24, 2017 at 08:47:33PM +0800, Baoquan He wrote:
> On 08/24/17 at 03:32pm, Dan Carpenter wrote:
> > Take a look at this code for example.  But all the places which call
> > get_domain() are the same:
> > 
> > drivers/iommu/amd_iommu.c
> >   2648  page = virt_to_page(virt_addr);
> >   2649  size = PAGE_ALIGN(size);
> >   2650  
> >   2651  domain = get_domain(dev);
> >  ^^
> > imagined get_domain() returns NULL.
> > 
> >   2652  if (IS_ERR(domain))
> >   2653  goto free_mem;
> >   2654  
> >   2655  dma_dom = to_dma_ops_domain(domain);
> >   ^
> > This will Oops.
> 
> I see, it's a problem. Thanks for telling!
> 
> How about below change? But I am not very sure which errno should be
> picked, seems the latter one, EBUSY is better since it has passed the
> check_device() checking.

Looks good to me.  You know better than I do which errno is best, so
I'll leave that to you.

regards,
dan carpenter

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

Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it

[Baoquan He]

On 08/24/17 at 03:32pm, Dan Carpenter wrote:
> Take a look at this code for example.  But all the places which call
> get_domain() are the same:
> 
> drivers/iommu/amd_iommu.c
>   2648  page = virt_to_page(virt_addr);
>   2649  size = PAGE_ALIGN(size);
>   2650  
>   2651  domain = get_domain(dev);
>  ^^
> imagined get_domain() returns NULL.
> 
>   2652  if (IS_ERR(domain))
>   2653  goto free_mem;
>   2654  
>   2655  dma_dom = to_dma_ops_domain(domain);
>   ^
> This will Oops.

I see, it's a problem. Thanks for telling!

How about below change? But I am not very sure which errno should be
picked, seems the latter one, EBUSY is better since it has passed the
check_device() checking.

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 16f1e6af00b0..2d7d04472555 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2262,6 +2262,9 @@ static struct protection_domain *get_domain(struct device 
*dev)
domain = to_pdomain(io_domain);
attach_device(dev, domain);
}
+   if (domain == NULL)
+   return ERR_PTR(-EBUSY);
+
if (!dma_ops_domain(domain))
return ERR_PTR(-EBUSY);
 
-- 
2.5.5

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

Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it

[Dan Carpenter]

Take a look at this code for example.  But all the places which call
get_domain() are the same:

drivers/iommu/amd_iommu.c
  2648  page = virt_to_page(virt_addr);
  2649  size = PAGE_ALIGN(size);
  2650  
  2651  domain = get_domain(dev);
 ^^
imagined get_domain() returns NULL.

  2652  if (IS_ERR(domain))
  2653  goto free_mem;
  2654  
  2655  dma_dom = to_dma_ops_domain(domain);
  ^
This will Oops.

  2656  

regards,
dan carpenter


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

Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it

[Baoquan He]

On 08/24/17 at 03:11pm, Dan Carpenter wrote:
> On Thu, Aug 24, 2017 at 07:56:47PM +0800, Baoquan He wrote:
> > In get_domain(), 'domain' could still be NULL before it's passed to
> > dma_ops_domain() to dereference. For safety, check if 'domain' is
> > NULL before passing to dma_ops_domain().
> > 
> > Reported-by: Dan Carpenter 
> > Signed-off-by: Baoquan He 
> > ---
> >  drivers/iommu/amd_iommu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index 16f1e6af00b0..2e2d5e6a13b3 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -2262,7 +2262,7 @@ static struct protection_domain *get_domain(struct 
> > device *dev)
> > domain = to_pdomain(io_domain);
> > attach_device(dev, domain);
> > }
> > -   if (!dma_ops_domain(domain))
> > +   if (domain && !dma_ops_domain(domain))
> > return ERR_PTR(-EBUSY);
> >  
> > return domain;
> 
> This still doesn't look right.  None of the callers can handle a NULL
> domain.

Here the NULL domain is on purpose. In kdump kernel if iommu is
pre-enabled, just stop attach the device to domain until the device
driver init. So here in driver init when call get_domain(), if found
get_dev_data(dev)->defer_attach is true, we just do the attachment of
device to domain.

Not sure if I got what you mean about 'callers'.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it

[Dan Carpenter]

On Thu, Aug 24, 2017 at 07:56:47PM +0800, Baoquan He wrote:
> In get_domain(), 'domain' could still be NULL before it's passed to
> dma_ops_domain() to dereference. For safety, check if 'domain' is
> NULL before passing to dma_ops_domain().
> 
> Reported-by: Dan Carpenter 
> Signed-off-by: Baoquan He 
> ---
>  drivers/iommu/amd_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 16f1e6af00b0..2e2d5e6a13b3 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2262,7 +2262,7 @@ static struct protection_domain *get_domain(struct 
> device *dev)
>   domain = to_pdomain(io_domain);
>   attach_device(dev, domain);
>   }
> - if (!dma_ops_domain(domain))
> + if (domain && !dma_ops_domain(domain))
>   return ERR_PTR(-EBUSY);
>  
>   return domain;

This still doesn't look right.  None of the callers can handle a NULL
domain.

regards,
dan carpenter

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

[PATCH] iommu/amd: Check if domain is NULL before dereference it

[Baoquan He]

In get_domain(), 'domain' could still be NULL before it's passed to
dma_ops_domain() to dereference. For safety, check if 'domain' is
NULL before passing to dma_ops_domain().

Reported-by: Dan Carpenter 
Signed-off-by: Baoquan He 
---
 drivers/iommu/amd_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 16f1e6af00b0..2e2d5e6a13b3 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2262,7 +2262,7 @@ static struct protection_domain *get_domain(struct device 
*dev)
domain = to_pdomain(io_domain);
attach_device(dev, domain);
}
-   if (!dma_ops_domain(domain))
+   if (domain && !dma_ops_domain(domain))
return ERR_PTR(-EBUSY);
 
return domain;
-- 
2.5.5

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

Re: [bug report] iommu/amd: Use is_attach_deferred call-back

[Baoquan He]

Hi Dan,

On 08/24/17 at 02:04pm, Dan Carpenter wrote:
> Hello Baoquan He,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch df3f7a6e8e85: "iommu/amd: Use is_attach_deferred call-back" 
> from Aug 9, 2017, leads to the following Smatch complaint:
> 
> drivers/iommu/amd_iommu.c:2265 get_domain()
>error: we previously assumed 'domain' could be null (see line 2259)
> 
> drivers/iommu/amd_iommu.c
>   2258domain = get_dev_data(dev)->domain;
>   2259if (domain == NULL && get_dev_data(dev)->defer_attach) {
> ^^
> The patch adds a new check for NULL.
> 
>   2260get_dev_data(dev)->defer_attach = false;
>   2261io_domain = iommu_get_domain_for_dev(dev);
>   2262domain = to_pdomain(io_domain);
>   2263attach_device(dev, domain);
>   2264}
>   2265if (!dma_ops_domain(domain))

Thanks for pointing it out, it's truly a code bug. We should check if
'domain' is NULL when pass it to dma_ops_domain() to dereference.

I would like to fix it with below code change, and will post a patch
soon.

-   if (!dma_ops_domain(domain))
+   if (domain && !dma_ops_domain(domain))
return ERR_PTR(-EBUSY);

Thanks
Baoquan

> ^^
> Existing unchecked dereference inside the function.
> 
>   2266return ERR_PTR(-EBUSY);
>   2267
> 
> regards,
> dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[bug report] iommu/amd: Use is_attach_deferred call-back

[Dan Carpenter]

Hello Baoquan He,

This is a semi-automatic email about new static checker warnings.

The patch df3f7a6e8e85: "iommu/amd: Use is_attach_deferred call-back" 
from Aug 9, 2017, leads to the following Smatch complaint:

drivers/iommu/amd_iommu.c:2265 get_domain()
 error: we previously assumed 'domain' could be null (see line 2259)

drivers/iommu/amd_iommu.c
  2258  domain = get_dev_data(dev)->domain;
  2259  if (domain == NULL && get_dev_data(dev)->defer_attach) {
^^
The patch adds a new check for NULL.

  2260  get_dev_data(dev)->defer_attach = false;
  2261  io_domain = iommu_get_domain_for_dev(dev);
  2262  domain = to_pdomain(io_domain);
  2263  attach_device(dev, domain);
  2264  }
  2265  if (!dma_ops_domain(domain))
^^
Existing unchecked dereference inside the function.

  2266  return ERR_PTR(-EBUSY);
  2267  

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

[PATCH v2 1/2] iommu/mediatek: Fix a build fail of m4u_type

[Yong Wu]

The commit ("iommu/mediatek: Enlarge the validate PA range
for 4GB mode") introduce the following build error:

   drivers/iommu/mtk_iommu.c: In function 'mtk_iommu_hw_init':
>> drivers/iommu/mtk_iommu.c:536:30: error: 'const struct mtk_iommu_data'
 has no member named 'm4u_type'; did you mean 'm4u_dom'?
 if (data->enable_4GB && data->m4u_type != M4U_MT8173) {

This patch fix it, use "m4u_plat" instead of "m4u_type".

Reported-by: kernel test robot 
Signed-off-by: Yong Wu 
---
1) In the v2 of mt2712 iommu support patch-set, We changed a variant
name from "m4u_type" to "m4u_plat", But I'm sorry that I forgot to
change it in the later patch.
2) This patch is based on [1].
[1]:https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023847.html
3) I cann't find the commit id of the patch ("iommu/mediatek: Enlarge
the validate PA range for 4GB mode") in the [2].
So I don't write the commit id for that patch in the commit message.
[2]https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
---
 drivers/iommu/mtk_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 4f233e1..bc00e40 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -533,7 +533,7 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data 
*data)
 
writel_relaxed(F_MMU_IVRP_PA_SET(data->protect_base, data->enable_4GB),
   data->base + REG_MMU_IVRP_PADDR);
-   if (data->enable_4GB && data->m4u_type != M4U_MT8173) {
+   if (data->enable_4GB && data->m4u_plat != M4U_MT8173) {
/*
 * If 4GB mode is enabled, the validate PA range is from
 * 0x1__ to 0x1__. here record bit[32:30].
-- 
1.9.1

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

[PATCH v2 2/2] iommu/mediatek: Fix a build warning of BIT(32) in ARM

[Yong Wu]

The commit ("iommu/mediatek: Enlarge the validate PA range
for 4GB mode") introduce the following build warning while ARCH=arm:

   drivers/iommu/mtk_iommu.c: In function 'mtk_iommu_iova_to_phys':
   include/linux/bitops.h:6:24: warning: left shift count >= width
of type [-Wshift-count-overflow]
#define BIT(nr)   (1UL << (nr))
   ^
>> drivers/iommu/mtk_iommu.c:407:9: note: in expansion of macro 'BIT'
  pa |= BIT(32);

  drivers/iommu/mtk_iommu.c: In function 'mtk_iommu_probe':
   include/linux/bitops.h:6:24: warning: left shift count >= width
of type [-Wshift-count-overflow]
#define BIT(nr)   (1UL << (nr))
   ^
   drivers/iommu/mtk_iommu.c:589:35: note: in expansion of macro 'BIT'
 data->enable_4GB = !!(max_pfn > (BIT(32) >> PAGE_SHIFT));

Use BIT_ULL instead of BIT.

Reported-by: kernel test robot 
Signed-off-by: Yong Wu 
---
 drivers/iommu/mtk_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index bc00e40..bd515be 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -404,7 +404,7 @@ static phys_addr_t mtk_iommu_iova_to_phys(struct 
iommu_domain *domain,
spin_unlock_irqrestore(>pgtlock, flags);
 
if (data->enable_4GB)
-   pa |= BIT(32);
+   pa |= BIT_ULL(32);
 
return pa;
 }
@@ -586,7 +586,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
 
/* Whether the current dram is over 4GB */
-   data->enable_4GB = !!(max_pfn > (BIT(32) >> PAGE_SHIFT));
+   data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
data->base = devm_ioremap_resource(dev, res);
-- 
1.9.1

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

Re: [RFC v2 2/4] iommu/arm-smmu-v3: Add tlbi_on_map option

[Auger Eric]

Hi Will,

On 23/08/2017 18:42, Will Deacon wrote:
> Hi Eric,
> 
> On Wed, Aug 23, 2017 at 02:36:53PM +0200, Auger Eric wrote:
>> On 23/08/2017 12:25, Will Deacon wrote:
>>> On Tue, Aug 22, 2017 at 10:09:15PM +0300, Michael S. Tsirkin wrote:
 On Fri, Aug 18, 2017 at 05:49:42AM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 17, 2017 at 05:34:25PM +0100, Will Deacon wrote:
>> On Fri, Aug 11, 2017 at 03:45:28PM +0200, Eric Auger wrote:
>>> When running a virtual SMMU on a guest we sometimes need to trap
>>> all changes to the translation structures. This is especially useful
>>> to integrate with VFIO. This patch adds a new option that forces
>>> the IO_PGTABLE_QUIRK_TLBI_ON_MAP to be applied on LPAE page tables.
>>>
>>> TLBI commands then can be trapped.
>>>
>>> Signed-off-by: Eric Auger 
>>>
>>> ---
>>> v1 -> v2:
>>> - rebase on v4.13-rc2
>>> ---
>>>  Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 4 
>>>  drivers/iommu/arm-smmu-v3.c | 5 +
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt 
>>> b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>> index c9abbf3..ebb85e9 100644
>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>> @@ -52,6 +52,10 @@ the PCIe specification.
>>>  
>>> devicetree/bindings/interrupt-controller/msi.txt
>>>for a description of the msi-parent property.
>>>  
>>> +- tlbi-on-map   : invalidate caches whenever there is an update of
>>> +  any remapping structure (updates to not-present 
>>> or
>>> +  present entries).
>>> +
>>
>> My position on this hasn't changed, so NAK for this patch. If you want to
>> emulate something outside of the SMMUv3 architecture, please do so, but
>> don't pretend that it's an SMMUv3.
>>
>> Will
>
> What if the emulated device does not list arm,smmu-v3, listing
> qemu,ssmu-v3 as compatible? Would that address the concern?

 Will, can you comment on this please? Are you open to reusing the code
 in drivers/iommu/arm-smmu-v3.c to support a paravirtual device that does
 not claim to be compatible with smmuv3 but does try to behave very close to
 it except it can cache non-present structures? Or would you rather
 the code to support this is forked to qemu-smmu-v3.c?
>>>
>>> I still don't understand why this is preferable to a PV IOMMU
>>> implementation. Not only is this proposing to issue TLB maintenance on
>>> map, but the maintenance command itself is entirely made up. Why not just
>>> have a map command? Anyway, I'm reluctant to add this hack to the driver 
>>> until:
>>>
>>>   1. There is a compelling reason to pursue this approach instead of a
>>>  PV approach (including performance measurements).
>>>
>>>   2. There is a specification for the QEMU fork of the ARM SMMUv3
>>>  architecture, including the semantics of the new command being proposed
>>>  and what exactly the TLB maintenance requirements are on map (for
>>>  example, what if I change an STE or a CD -- are they cached too?).
>> I am not sure I catch this last point. At the moment whenever the smmuv3
>> driver issues data structure invalidation commands (CMD_CFGI_*), those
>> are trapped and I replay the mappings on host side. I have not changed
>> anything on that side.
> 
> But STEs and CDs have very similar rules to TLBs: you don't need to issue
> invalidation if the data structure is transitioning from invalid to valid.
> If you're caching those in QEMU, how do you keep them up-to-date? I can
> also guarantee you that there will be additional data structures added
> in future versions of the architecture, so you'll need to consider how
> you want to operate when running on newer hardware.
OK I get your point now. I thought the driver was currently sending
CMD_CFGI_STE on each config change and I did not notice any issue yet
with the various use cases. So that should be part of the FW quirk to
guarantee that each time this happens, commands are sent for QEMU to trap.

This FW quirk was devised to do exactly the same as the Intel vtd
caching mode. Technically I guess Intel driver implements the exact same
hooks (but they did to comply with the spec, I agree).

Thanks

Eric
> 
>> I introduced a new map implementation defined command because the per
>> page CMD_TLBI_NH_VA IOVA invalidation command was not efficient/usable
>> with use cases such as DPDK on guest. I understood the spec provisions
>> for such implementation defined commands.
> 
> Whilst there is a space for IMP DEF commands, this doesn't generally mean
> that they can be repurposed by software. What if the 

Re: [PATCH v2 7/8] iommu/mediatek: Enlarge the validate PA range for 4GB mode

[kbuild test robot]

Hi Yong,

[auto build test WARNING on iommu/next]
[also build test WARNING on next-20170823]
[cannot apply to v4.13-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yong-Wu/MT2712-IOMMU-SUPPORT/20170824-074750
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:8,
from include/linux/preempt.h:10,
from include/linux/spinlock.h:50,
from include/linux/mmzone.h:7,
from include/linux/bootmem.h:7,
from drivers/iommu/mtk_iommu.c:14:
   drivers/iommu/mtk_iommu.c: In function 'mtk_iommu_iova_to_phys':
   include/linux/bitops.h:6:24: warning: left shift count >= width of type 
[-Wshift-count-overflow]
#define BIT(nr)   (1UL << (nr))
   ^
>> drivers/iommu/mtk_iommu.c:407:9: note: in expansion of macro 'BIT'
  pa |= BIT(32);
^~~
   drivers/iommu/mtk_iommu.c: In function 'mtk_iommu_hw_init':
   drivers/iommu/mtk_iommu.c:536:30: error: 'const struct mtk_iommu_data' has 
no member named 'm4u_type'; did you mean 'm4u_dom'?
 if (data->enable_4GB && data->m4u_type != M4U_MT8173) {
 ^~
   In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:8,
from include/linux/preempt.h:10,
from include/linux/spinlock.h:50,
from include/linux/mmzone.h:7,
from include/linux/bootmem.h:7,
from drivers/iommu/mtk_iommu.c:14:
   drivers/iommu/mtk_iommu.c: In function 'mtk_iommu_probe':
   include/linux/bitops.h:6:24: warning: left shift count >= width of type 
[-Wshift-count-overflow]
#define BIT(nr)   (1UL << (nr))
   ^
   drivers/iommu/mtk_iommu.c:589:35: note: in expansion of macro 'BIT'
 data->enable_4GB = !!(max_pfn > (BIT(32) >> PAGE_SHIFT));
  ^~~

vim +/BIT +407 drivers/iommu/mtk_iommu.c

   393  
   394  static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
   395dma_addr_t iova)
   396  {
   397  struct mtk_iommu_domain *dom = to_mtk_domain(domain);
   398  struct mtk_iommu_data *data = mtk_iommu_get_m4u_data();
   399  unsigned long flags;
   400  phys_addr_t pa;
   401  
   402  spin_lock_irqsave(>pgtlock, flags);
   403  pa = dom->iop->iova_to_phys(dom->iop, iova);
   404  spin_unlock_irqrestore(>pgtlock, flags);
   405  
   406  if (data->enable_4GB)
 > 407  pa |= BIT(32);
   408  
   409  return pa;
   410  }
   411  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu