Re: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group

2020-03-02 Thread Lu Baolu 

Hi Joerg,

On 2020/3/2 23:08, Joerg Roedel wrote:

Hello Sai, Baolu,

On Sun, Feb 16, 2020 at 01:57:26PM -0800, Sai Praneeth Prakhya wrote:

Hence it will be helpful if there is some way to change the default
domain of a B:D.F dynamically. Since, linux iommu subsystem prefers to
deal at iommu_group level instead of B:D.F level, it might be helpful
if there is some way to change the default domain of a *iommu_group*
dynamically. Hence, add such support.


The question is how this plays together with the per-device private
domains in the Intel VT-d driver. I recently debugged an issue there and
I think there are more. The overall code for this seems to be pretty
fragile, so I had the idea to make the private default domains more
general.

IOMMU default domains don't necessarily need to stick to the iommu-group
granularity, because the default domain is used by in-kernel drivers
only, and the kernel trusts itself.

So my idea was to make the private-domain concept of the VT-d driver
more generic and move it to the iommu core code. With that we can
configure real per-device default domain types and don't have the race
condition with driver probing when changing the default domain of
multiple devices. We have to limit the ability to change default domain
types to devices with no PCI aliases, but that should not be a problem
for the intended use-case.

What do you think?



Theoretically speaking, per-device default domain is impractical. PCI
aliased devices (PCI bridge and all devices beneath it, VMD devices and
various devices quirked with pci_add_dma_alias()) must use the same
domain. It's likely that we have to introduce something like a sub-group
with all PCI aliased devices staying in it. Current private-domain
implementation in the vt-d driver was introduced for compatible purpose
and I wanted to abandon it from the first day. :-)

On Intel platforms, there are only rare devices which require a specific
default domain: some graphic devices (identity), a specific model of
AZALIA (identity) and external devices connected through thunderbolt
(dma). They are not supposed to belong to a same group. Hence, if we
are able to configure per-group default domain type, we don't need to
keep private domain anymore.

Probably, we are able to configure per-group default domain type with
below two interfaces.

- (ops->)dev_def_domain_type: Return the required default domain type
  for a device. It returns
  - IOMMU_DOMAIN_DMA (device must use a DMA domain), unlikely
  - IOMMU_DOMAIN_IDENTITY (device must use an Identity domain), unlikely
  - 0 (both are okay), likely

- iommu_group_change_def_domain: Change the default domain of a group
  Works only when all devices have no driver bond.

[Sai's patch set has already included these two interfaces.]

In iommu_probe_device(),

dev_def_type = ops->dev_def_domain_type(dev)
if (dev_def_type && dev_def_type != group->default_domain->type) {
ret = iommu_group_change_def_domain(...)
if (ret)
return -EINVAL;
}

This should work during boot since iommu_probe_device() always happens
before device driver binding. We need to further consider the hot-plug
cases.

- Hardware initiated device hotplug
We should always use DMA domain for devices connected through an
external port to avoid DMA attacking from malicious devices. And
such devices shouldn't share a group with internal (trusted) devices.
Hence, I can't see any problems here.

- Software initiated device hotplug
The default domain type won't change before and after device hotplug
so there's no problem as well.

This is what I have for the private domain in vt-d driver. Just for
discussion.

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

[PATCH AUTOSEL 5.4 39/58] iommu/amd: Disable IOMMU on Stoney Ridge systems

2020-03-02 Thread Sasha Levin 
From: Kai-Heng Feng 

[ Upstream commit 3dfee47b215e49788cfc80e474820ea2e948c031 ]

Serious screen flickering when Stoney Ridge outputs to a 4K monitor.

Use identity-mapping and PCI ATS doesn't help this issue.

According to Alex Deucher, IOMMU isn't enabled on Windows, so let's do
the same here to avoid screen flickering on 4K monitor.

Cc: Alex Deucher 
Bug: https://gitlab.freedesktop.org/drm/amd/issues/961
Signed-off-by: Kai-Heng Feng 
Acked-by: Alex Deucher 
Signed-off-by: Joerg Roedel 
Signed-off-by: Sasha Levin 
---
 drivers/iommu/amd_iommu_init.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index d7cbca8bf2cd4..b5ae9f7c0510b 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2533,6 +2533,7 @@ static int __init early_amd_iommu_init(void)
struct acpi_table_header *ivrs_base;
acpi_status status;
int i, remap_cache_sz, ret = 0;
+   u32 pci_id;
 
if (!amd_iommu_detected)
return -ENODEV;
@@ -2620,6 +2621,16 @@ static int __init early_amd_iommu_init(void)
if (ret)
goto out;
 
+   /* Disable IOMMU if there's Stoney Ridge graphics */
+   for (i = 0; i < 32; i++) {
+   pci_id = read_pci_config(0, i, 0, 0);
+   if ((pci_id & 0x) == 0x1002 && (pci_id >> 16) == 0x98e4) {
+   pr_info("Disable IOMMU on Stoney Ridge\n");
+   amd_iommu_disabled = true;
+   break;
+   }
+   }
+
/* Disable any previously enabled IOMMUs */
if (!is_kdump_kernel() || amd_iommu_disabled)
disable_iommus();
@@ -2728,7 +2739,7 @@ static int __init state_next(void)
ret = early_amd_iommu_init();
init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
if (init_state == IOMMU_ACPI_FINISHED && amd_iommu_disabled) {
-   pr_info("AMD IOMMU disabled on kernel command-line\n");
+   pr_info("AMD IOMMU disabled\n");
init_state = IOMMU_CMDLINE_DISABLED;
ret = -EINVAL;
}
-- 
2.20.1

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

[PATCH AUTOSEL 5.5 44/66] iommu/amd: Disable IOMMU on Stoney Ridge systems

2020-03-02 Thread Sasha Levin 
From: Kai-Heng Feng 

[ Upstream commit 3dfee47b215e49788cfc80e474820ea2e948c031 ]

Serious screen flickering when Stoney Ridge outputs to a 4K monitor.

Use identity-mapping and PCI ATS doesn't help this issue.

According to Alex Deucher, IOMMU isn't enabled on Windows, so let's do
the same here to avoid screen flickering on 4K monitor.

Cc: Alex Deucher 
Bug: https://gitlab.freedesktop.org/drm/amd/issues/961
Signed-off-by: Kai-Heng Feng 
Acked-by: Alex Deucher 
Signed-off-by: Joerg Roedel 
Signed-off-by: Sasha Levin 
---
 drivers/iommu/amd_iommu_init.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index d7cbca8bf2cd4..b5ae9f7c0510b 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2533,6 +2533,7 @@ static int __init early_amd_iommu_init(void)
struct acpi_table_header *ivrs_base;
acpi_status status;
int i, remap_cache_sz, ret = 0;
+   u32 pci_id;
 
if (!amd_iommu_detected)
return -ENODEV;
@@ -2620,6 +2621,16 @@ static int __init early_amd_iommu_init(void)
if (ret)
goto out;
 
+   /* Disable IOMMU if there's Stoney Ridge graphics */
+   for (i = 0; i < 32; i++) {
+   pci_id = read_pci_config(0, i, 0, 0);
+   if ((pci_id & 0x) == 0x1002 && (pci_id >> 16) == 0x98e4) {
+   pr_info("Disable IOMMU on Stoney Ridge\n");
+   amd_iommu_disabled = true;
+   break;
+   }
+   }
+
/* Disable any previously enabled IOMMUs */
if (!is_kdump_kernel() || amd_iommu_disabled)
disable_iommus();
@@ -2728,7 +2739,7 @@ static int __init state_next(void)
ret = early_amd_iommu_init();
init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
if (init_state == IOMMU_ACPI_FINISHED && amd_iommu_disabled) {
-   pr_info("AMD IOMMU disabled on kernel command-line\n");
+   pr_info("AMD IOMMU disabled\n");
init_state = IOMMU_CMDLINE_DISABLED;
ret = -EINVAL;
}
-- 
2.20.1

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

Re: [PATCH] iommu/io-pgtable-arm: Fix IOVA validation for 32-bit

2020-03-02 Thread Stephan Gerhold 
Hi Robin,

On Fri, Feb 28, 2020 at 02:18:55PM +, Robin Murphy wrote:
> Since we ony support the TTB1 quirk for AArch64 contexts, and
> consequently only for 64-bit builds, the sign-extension aspect of the
> "are all bits above IAS consistent?" check should implicitly only apply
> to 64-bit IOVAs. Change the type of the cast to ensure that 32-bit longs
> don't inadvertently get sign-extended, and thus considered invalid, if
> they happen to be above 2GB in the TTB0 region.
> 
> Reported-by: Stephan Gerhold 
> Signed-off-by: Robin Murphy 
> 

Thanks for the patch!

Just wanted to report that this patch does indeed fix the problem
I had with qcom-venus on ARM32.

It's probably too late now, but FWIW:
Tested-by: Stephan Gerhold 

> ---
> 
> Logically there may also have been a UBSAN "shift greater than size of
> type" warning too, but arch/arm doesn't support UBSAN_SANITIZE_ALL,
> and that's now my only easy "spin up a 32-bit VM" option to hand :)
> 
>  drivers/iommu/io-pgtable-arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 983b08477e64..04fbd4bf0ff9 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -468,7 +468,7 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, 
> unsigned long iova,
>   arm_lpae_iopte *ptep = data->pgd;
>   int ret, lvl = data->start_level;
>   arm_lpae_iopte prot;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = (s64)iova >> cfg->ias;
>  
>   /* If no access, then nothing to do */
>   if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> @@ -645,7 +645,7 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, 
> unsigned long iova,
>   struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>   struct io_pgtable_cfg *cfg = >iop.cfg;
>   arm_lpae_iopte *ptep = data->pgd;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = (s64)iova >> cfg->ias;
>  
>   if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
>   return 0;
> -- 
> 2.23.0.dirty
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] dma: Fix max PFN arithmetic overflow on 32 bit systems

2020-03-02 Thread Alexander Dahl 
For ARCH=x86 (32 bit) when you set CONFIG_IOMMU_INTEL since c5a5dc4cbbf4
("iommu/vt-d: Don't switch off swiotlb if bounce page is used") there's
a dependency on CONFIG_SWIOTLB, which was not necessarily active before.

The init code for swiotlb in 'pci_swiotlb_detect_4gb()' compares
something against MAX_DMA32_PFN to decide if it should be active.
However that define suffers from an arithmetic overflow since
1b7e03ef7570 ("x86, NUMA: Enable emulation on 32bit too") when it was
first made visible to x86_32.

The effect is at boot time 64 MiB (default size) were allocated for
bounce buffers now, which is a noticeable amount of memory on small
systems. We noticed this effect on the fli4l Linux distribution when
migrating from kernel v4.19 (LTS) to v5.4 (LTS) on boards like pcengines
ALIX 2D3 with 256 MiB memory for example:

  Linux version 5.4.22 (buildroot@buildroot) (gcc version 7.3.0 (Buildroot 
2018.02.8)) #1 SMP Mon Nov 26 23:40:00 CET 2018
  …
  Memory: 183484K/261756K available (4594K kernel code, 393K rwdata, 1660K 
rodata, 536K init, 456K bss , 78272K reserved, 0K cma-reserved, 0K highmem)
  …
  PCI-DMA: Using software bounce buffering for IO (SWIOTLB)
  software IO TLB: mapped [mem 0x0bb78000-0x0fb78000] (64MB)

The initial analysis and the suggested fix was done by user 'sourcejedi'
at stackoverflow and explicitly marked as GPLv2 for inclusion in the
Linux kernel:

  https://unix.stackexchange.com/a/520525/50007

Fixes: https://web.nettworks.org/bugs/browse/FFL-2560
Fixes: https://unix.stackexchange.com/q/520065/50007
Suggested-by: Alan Jenkins 
Signed-off-by: Alexander Dahl 
---
We tested this in qemu and on real hardware with fli4l on top of v5.4,
v5.5, and v5.6-rc kernels, but only as far as the reserved memory goes.
The patch itself is based on v5.6-rc3 (IIRC).

A quick grep over the kernel code showed me this define MAX_DMA32_PFN is
used in other places as well. I would appreciate feedback on this,
because I can not oversee all side effects this might have?!

Thanks again to Alan who proposed the fix, and for his permission to
send it upstream.

Greets
Alex
---
 arch/x86/include/asm/dma.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/dma.h b/arch/x86/include/asm/dma.h
index 00f7cf45e699..e25514eca8d6 100644
--- a/arch/x86/include/asm/dma.h
+++ b/arch/x86/include/asm/dma.h
@@ -74,7 +74,7 @@
 #define MAX_DMA_PFN   ((16UL * 1024 * 1024) >> PAGE_SHIFT)
 
 /* 4GB broken PCI/AGP hardware bus master zone */
-#define MAX_DMA32_PFN ((4UL * 1024 * 1024 * 1024) >> PAGE_SHIFT)
+#define MAX_DMA32_PFN (4UL * ((1024 * 1024 * 1024) >> PAGE_SHIFT))
 
 #ifdef CONFIG_X86_32
 /* The maximum address that we can perform a DMA transfer to on this platform 
*/
-- 
2.20.1

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

Re: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space

2020-03-02 Thread Joerg Roedel 
On Fri, Feb 28, 2020 at 06:25:36PM +0100, Jean-Philippe Brucker wrote:
> This solution isn't elegant nor foolproof, but is the best we can do at
> the moment and works with existing virtio-iommu implementations. It also
> enables an IOMMU for lightweight hypervisors that do not rely on
> firmware methods for booting.

I appreciate the enablement on x86, but putting the conmfiguration into
mmio-space isn't really something I want to see upstream. What is the
problem with defining an ACPI table instead? This would also make things
work on AARCH64 UEFI machines.

Regards,

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

Re: [PATCH] iommu/io-pgtable-arm: Fix IOVA validation for 32-bit

2020-03-02 Thread Will Deacon 
On Mon, Mar 02, 2020 at 05:11:23PM +0100, Joerg Roedel wrote:
> On Mon, Mar 02, 2020 at 11:53:01AM +, Will Deacon wrote:
> > On Fri, Feb 28, 2020 at 02:18:55PM +, Robin Murphy wrote:
> > > Since we ony support the TTB1 quirk for AArch64 contexts, and
> > > consequently only for 64-bit builds, the sign-extension aspect of the
> > > "are all bits above IAS consistent?" check should implicitly only apply
> > > to 64-bit IOVAs. Change the type of the cast to ensure that 32-bit longs
> > > don't inadvertently get sign-extended, and thus considered invalid, if
> > > they happen to be above 2GB in the TTB0 region.
> > > 
> > > Reported-by: Stephan Gerhold 
> > > Signed-off-by: Robin Murphy 
> > > 
> > > ---
> > > 
> > > Logically there may also have been a UBSAN "shift greater than size of
> > > type" warning too, but arch/arm doesn't support UBSAN_SANITIZE_ALL,
> > > and that's now my only easy "spin up a 32-bit VM" option to hand :)
> > > 
> > >  drivers/iommu/io-pgtable-arm.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Acked-by: Will Deacon 
> > 
> > Joerg -- pleae can you take this as a fix for 5.6?
> 
> Done, do you also have a fixes-tag for me?

Fixes: db6903010aa5 ("iommu/io-pgtable-arm: Prepare for TTBR1 usage")

Although it doesn't need to go to -stable, since this was only introduced
during the recent merge window.

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

Re: [PATCH] iommu/io-pgtable-arm: Fix IOVA validation for 32-bit

2020-03-02 Thread Joerg Roedel 
On Mon, Mar 02, 2020 at 11:53:01AM +, Will Deacon wrote:
> On Fri, Feb 28, 2020 at 02:18:55PM +, Robin Murphy wrote:
> > Since we ony support the TTB1 quirk for AArch64 contexts, and
> > consequently only for 64-bit builds, the sign-extension aspect of the
> > "are all bits above IAS consistent?" check should implicitly only apply
> > to 64-bit IOVAs. Change the type of the cast to ensure that 32-bit longs
> > don't inadvertently get sign-extended, and thus considered invalid, if
> > they happen to be above 2GB in the TTB0 region.
> > 
> > Reported-by: Stephan Gerhold 
> > Signed-off-by: Robin Murphy 
> > 
> > ---
> > 
> > Logically there may also have been a UBSAN "shift greater than size of
> > type" warning too, but arch/arm doesn't support UBSAN_SANITIZE_ALL,
> > and that's now my only easy "spin up a 32-bit VM" option to hand :)
> > 
> >  drivers/iommu/io-pgtable-arm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Acked-by: Will Deacon 
> 
> Joerg -- pleae can you take this as a fix for 5.6?

Done, do you also have a fixes-tag for me?

Regards,

Joerg

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

Re: [PATCH v3] iommu/vt-d: Fix a bug in intel_iommu_iova_to_phys() for huge page

2020-03-02 Thread Joerg Roedel 
On Wed, Feb 26, 2020 at 12:30:06PM -0800, Yonghyun Hwang wrote:
> intel_iommu_iova_to_phys() has a bug when it translates an IOVA for a huge
> page onto its corresponding physical address. This commit fixes the bug by
> accomodating the level of page entry for the IOVA and adds IOVA's lower
> address to the physical address.
> 
> Cc: 
> Acked-by: Lu Baolu 
> Reviewed-by: Moritz Fischer 
> Signed-off-by: Yonghyun Hwang 

Applied with Fixes tag:

Fixes: 3871794642579 ("VT-d: Changes to support KVM")
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: provide in-place uncached remapping for dma-direct v2

2020-03-02 Thread Robin Murphy 

On 24/02/2020 7:44 pm, Christoph Hellwig wrote:

Hi all,

this series provides support for remapping places uncached in-place in
the generic dma-direct code, and moves openrisc over from its own
in-place remapping scheme.  The arm64 folks also had interest in such
a scheme to avoid problems with speculating into cache aliases.

Also all architectures that always use small page mappings for the
kernel and have non-coherent DMA should look into enabling this
scheme, as it is much more efficient than the vmap remapping.

Changes since v1:
  - share the arch hook for inline remap and uncached segment support



For the whole series:

Reviewed-by: Robin Murphy 

I think we might ultimately want to fiddle around a bit more in 
dma_direct_alloc_pages() to give ARCH_HAS_DMA_SET_UNCACHED clear 
precedence over DMA_DIRECT_REMAP if they have to coexist, but let's land 
these patches first as a solid foundation.


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

Re: [PATCH] iommu: silence iommu group prints

2020-03-02 Thread Joerg Roedel 
On Thu, Feb 27, 2020 at 11:57:52AM +, Russell King wrote:
> On the LX2160A, there are lots (about 160) of IOMMU messages produced
> during boot; this is excessive.  Reduce the severity of these messages
> to debug level.

No, these messages are a very useful tool when it comes to debugging
IOMMU issues on other platforms, and the user only has to provide dmesg
output. When your system has 160 devices, so be it.

Regards,

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

Re: [PATCH v2 2/4] iommu: Add Allwinner H6 IOMMU driver

2020-03-02 Thread Joerg Roedel 
Hi Maxime,

On Thu, Feb 20, 2020 at 07:15:14PM +0100, Maxime Ripard wrote:
> +struct sun50i_iommu_domain {
> + struct iommu_domain domain;
> +
> + /* Number of devices attached to the domain */
> + refcount_t refcnt;
> +
> + /* Lock to modify the Directory Table */
> + spinlock_t dt_lock;

I suggest you make page-table updates lock-less. Otherwise this lock
will become a bottle-neck when using the IOMMU through DMA-API.

> +
> +static int sun50i_iommu_map(struct iommu_domain *domain, unsigned long iova,
> + phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> +{
> + struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
> + struct sun50i_iommu *iommu = sun50i_domain->iommu;
> + u32 pte_index;
> + u32 *page_table, *pte_addr;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(_domain->dt_lock, flags);
> + page_table = sun50i_dte_get_page_table(sun50i_domain, iova, gfp);
> + if (IS_ERR(page_table)) {
> + ret = PTR_ERR(page_table);
> + goto out;
> + }
> +
> + pte_index = sun50i_iova_get_pte_index(iova);
> + pte_addr = _table[pte_index];
> + if (sun50i_pte_is_page_valid(*pte_addr)) {

You can use unlikely() here.

> + phys_addr_t page_phys = sun50i_pte_get_page_address(*pte_addr);
> + dev_err(iommu->dev,
> + "iova %pad already mapped to %pa cannot remap to %pa 
> prot: %#x\n",
> + , _phys, , prot);
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + *pte_addr = sun50i_mk_pte(paddr, prot);
> + sun50i_table_flush(sun50i_domain, pte_addr, 1);

This maps only one page, right? But the function needs to map up to
'size' as given in the parameter list.

> +
> + spin_lock_irqsave(>iommu_lock, flags);
> + sun50i_iommu_tlb_invalidate(iommu, iova);
> + spin_unlock_irqrestore(>iommu_lock, flags);

Why is there a need to flush the TLB here? The IOMMU-API provides
call-backs so that the user of the API can decide when it wants
to flush the IO/TLB. Such flushes are usually expensive and doing them
on every map and unmap will cost significant performance.

> +static size_t sun50i_iommu_unmap(struct iommu_domain *domain, unsigned long 
> iova,
> +  size_t size, struct iommu_iotlb_gather *gather)
> +{
> + struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
> + struct sun50i_iommu *iommu = sun50i_domain->iommu;
> + unsigned long flags;
> + phys_addr_t pt_phys;
> + dma_addr_t pte_dma;
> + u32 *pte_addr;
> + u32 dte;
> +
> + spin_lock_irqsave(_domain->dt_lock, flags);
> +
> + dte = sun50i_domain->dt[sun50i_iova_get_dte_index(iova)];
> + if (!sun50i_dte_is_pt_valid(dte)) {
> + spin_unlock_irqrestore(_domain->dt_lock, flags);
> + return 0;
> + }
> +
> + pt_phys = sun50i_dte_get_pt_address(dte);
> + pte_addr = (u32 *)phys_to_virt(pt_phys) + 
> sun50i_iova_get_pte_index(iova);
> + pte_dma = pt_phys + sun50i_iova_get_pte_index(iova) * PT_ENTRY_SIZE;
> +
> + if (!sun50i_pte_is_page_valid(*pte_addr)) {
> + spin_unlock_irqrestore(_domain->dt_lock, flags);
> + return 0;
> + }
> +
> + memset(pte_addr, 0, sizeof(*pte_addr));
> + sun50i_table_flush(sun50i_domain, pte_addr, 1);
> +
> + spin_lock(>iommu_lock);
> + sun50i_iommu_tlb_invalidate(iommu, iova);
> + sun50i_iommu_ptw_invalidate(iommu, iova);
> + spin_unlock(>iommu_lock);

Same objections as in the map function. This only unmaps one page, and
is the IO/TLB flush really needed here?

> +static struct iommu_domain *sun50i_iommu_domain_alloc(unsigned type)
> +{
> + struct sun50i_iommu_domain *sun50i_domain;
> +
> + if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
> + return NULL;

I think you should at least also support identity domains here. The
iommu-core code might allocate those for default domains.


Regards,

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

RE: [PATCH] iommu: silence iommu group prints

2020-03-02 Thread Laurentiu Tudor 
Hi Robin,

> -Original Message-
> From: Robin Murphy 
> Sent: Friday, February 28, 2020 8:32 PM
> 
> [ +Laurentiu ]
> 
> Hi Russell,
> 
> Thanks for sharing a log, now I properly understand what's up... further
> comments at the end (for context).
> 
> On 28/02/2020 10:06 am, Russell King - ARM Linux admin wrote:
> > On Fri, Feb 28, 2020 at 09:33:40AM +, John Garry wrote:
> >> On 28/02/2020 02:16, Lu Baolu wrote:
> >>> Hi,
> >>>
> >>> On 2020/2/27 19:57, Russell King wrote:
>  On the LX2160A, there are lots (about 160) of IOMMU messages produced
>  during boot; this is excessive.  Reduce the severity of these
> messages
>  to debug level.
> 
>  Signed-off-by: Russell King 
>  ---
>     drivers/iommu/iommu.c | 4 ++--
>     1 file changed, 2 insertions(+), 2 deletions(-)
> 
>  diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>  index 3ead597e1c57..304281ec623b 100644
>  --- a/drivers/iommu/iommu.c
>  +++ b/drivers/iommu/iommu.c
>  @@ -741,7 +741,7 @@ int iommu_group_add_device(struct iommu_group
>  *group, struct device *dev)
>     trace_add_device_to_group(group->id, dev);
>  -    dev_info(dev, "Adding to iommu group %d\n", group->id);
>  +    dev_dbg(dev, "Adding to iommu group %d\n", group->id);
> >>>
> >>> I'm not strongly against this. But to me this message seems to be a
> good
> >>> indicator that a device was probed successfully by the iommu subsystem.
> >>> Keeping it in the default kernel message always helps to the kernel
> >>> debugging.
> >>>
> >>
> >> I would tend to agree.
> >

[snip]

> >
> > # dmesg |grep 'Adding to iommu' | wc -l
> > 164
> > # dmesg |grep -v 'Adding to iommu' | wc -l
> > 551
> >
> > So, 23% of the kernel messages on this platform are "Adding to iommu",
> > which is excessive.
> 
> Indeed, however I would note that on most platforms bringing up a
> network interface involves hot-adding 0 devices, so hot-adding 19
> devices as full-blown DMA masters is arguably the root of "excessive"
> already. Per the concern I initially raised, each of those messages
> represents a whole bunch of internal allocation and bookkeeping going
> on, which if it isn't necessary would be far better avoided altogether,
> than simply done more quietly.
> 
> Laurentiu, I guess at the moment the nature of the of_dma_configure()
> integration means we end up treating all DPAA2 objects identically, but
> do you think we have scope to be a bit cleverer in that regard?
> Presumably not every type of object that shows up on the fsl_mc bus is
> really an independent DMA master, so if we could skip doing the full
> DMA/IOMMU/MSI setup for the ones that don't need it, it would work out
> nicer all round. In fact your .dma_configure proposal (which I'll try to
> take a proper look at next week) couldn't have come at a better time for
> that argument :)
 
Thanks! That's a very good point - I'll check on which devices we actually use 
dma apis and filter the rest out. Will keep in mind for the next spin of the 
patches.

---
Best Regards, Laurentiu

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

Re: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group

2020-03-02 Thread Joerg Roedel 
Hello Sai, Baolu,

On Sun, Feb 16, 2020 at 01:57:26PM -0800, Sai Praneeth Prakhya wrote:
> Hence it will be helpful if there is some way to change the default
> domain of a B:D.F dynamically. Since, linux iommu subsystem prefers to
> deal at iommu_group level instead of B:D.F level, it might be helpful
> if there is some way to change the default domain of a *iommu_group*
> dynamically. Hence, add such support.

The question is how this plays together with the per-device private
domains in the Intel VT-d driver. I recently debugged an issue there and
I think there are more. The overall code for this seems to be pretty
fragile, so I had the idea to make the private default domains more
general.

IOMMU default domains don't necessarily need to stick to the iommu-group
granularity, because the default domain is used by in-kernel drivers
only, and the kernel trusts itself.

So my idea was to make the private-domain concept of the VT-d driver
more generic and move it to the iommu core code. With that we can
configure real per-device default domain types and don't have the race
condition with driver probing when changing the default domain of
multiple devices. We have to limit the ability to change default domain
types to devices with no PCI aliases, but that should not be a problem
for the intended use-case.

What do you think?

Regards,

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

Re: [PATCH] iommu/io-pgtable-arm: Fix IOVA validation for 32-bit

2020-03-02 Thread Will Deacon 
On Fri, Feb 28, 2020 at 02:18:55PM +, Robin Murphy wrote:
> Since we ony support the TTB1 quirk for AArch64 contexts, and
> consequently only for 64-bit builds, the sign-extension aspect of the
> "are all bits above IAS consistent?" check should implicitly only apply
> to 64-bit IOVAs. Change the type of the cast to ensure that 32-bit longs
> don't inadvertently get sign-extended, and thus considered invalid, if
> they happen to be above 2GB in the TTB0 region.
> 
> Reported-by: Stephan Gerhold 
> Signed-off-by: Robin Murphy 
> 
> ---
> 
> Logically there may also have been a UBSAN "shift greater than size of
> type" warning too, but arch/arm doesn't support UBSAN_SANITIZE_ALL,
> and that's now my only easy "spin up a 32-bit VM" option to hand :)
> 
>  drivers/iommu/io-pgtable-arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Will Deacon 

Joerg -- pleae can you take this as a fix for 5.6?

Thanks,

Will

> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 983b08477e64..04fbd4bf0ff9 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -468,7 +468,7 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, 
> unsigned long iova,
>   arm_lpae_iopte *ptep = data->pgd;
>   int ret, lvl = data->start_level;
>   arm_lpae_iopte prot;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = (s64)iova >> cfg->ias;
>  
>   /* If no access, then nothing to do */
>   if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> @@ -645,7 +645,7 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, 
> unsigned long iova,
>   struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>   struct io_pgtable_cfg *cfg = >iop.cfg;
>   arm_lpae_iopte *ptep = data->pgd;
> - long iaext = (long)iova >> cfg->ias;
> + long iaext = (s64)iova >> cfg->ias;
>  
>   if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
>   return 0;
> -- 
> 2.23.0.dirty
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/3] iommu/mediatek: add pdata member for legacy ivrp paddr

2020-03-02 Thread Fabien Parent 
Add a new platform data member in order to select which IVRP_PADDR
format is used by an SoC.

Signed-off-by: Fabien Parent 
---

v2: new patch

---
 drivers/iommu/mtk_iommu.c | 3 ++-
 drivers/iommu/mtk_iommu.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 95945f467c03..78cb14ab7dd0 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -569,7 +569,7 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data 
*data)
F_INT_PRETETCH_TRANSATION_FIFO_FAULT;
writel_relaxed(regval, data->base + REG_MMU_INT_MAIN_CONTROL);
 
-   if (data->plat_data->m4u_plat == M4U_MT8173)
+   if (data->plat_data->has_legacy_ivrp_paddr)
regval = (data->protect_base >> 1) | (data->enable_4GB << 31);
else
regval = lower_32_bits(data->protect_base) |
@@ -786,6 +786,7 @@ static const struct mtk_iommu_plat_data mt8173_data = {
.m4u_plat = M4U_MT8173,
.has_4gb_mode = true,
.has_bclk = true,
+   .has_legacy_ivrp_paddr = true;
.reset_axi= true,
.larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
 };
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index ea949a324e33..4696ba027a71 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -42,6 +42,7 @@ struct mtk_iommu_plat_data {
boolhas_bclk;
boolhas_vld_pa_rng;
boolreset_axi;
+   boolhas_legacy_ivrp_paddr;
unsigned char   larbid_remap[MTK_LARB_NR_MAX];
 };
 
-- 
2.25.0

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

[PATCH v2 3/3] iommu/mediatek: add support for MT8167

2020-03-02 Thread Fabien Parent 
Add support for the IOMMU on MT8167

Signed-off-by: Fabien Parent 
---

V2:
* removed if based on m4u_plat, and using instead the new
has_legacy_ivrp_paddr member that was introduced in patch 2.

---
 drivers/iommu/mtk_iommu.c | 9 +
 drivers/iommu/mtk_iommu.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 78cb14ab7dd0..25b7ad1647ba 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -782,6 +782,14 @@ static const struct mtk_iommu_plat_data mt2712_data = {
.larbid_remap = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
 };
 
+static const struct mtk_iommu_plat_data mt8167_data = {
+   .m4u_plat = M4U_MT8167,
+   .has_4gb_mode = true,
+   .has_legacy_ivrp_paddr = true;
+   .reset_axi= true,
+   .larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
+};
+
 static const struct mtk_iommu_plat_data mt8173_data = {
.m4u_plat = M4U_MT8173,
.has_4gb_mode = true,
@@ -799,6 +807,7 @@ static const struct mtk_iommu_plat_data mt8183_data = {
 
 static const struct of_device_id mtk_iommu_of_ids[] = {
{ .compatible = "mediatek,mt2712-m4u", .data = _data},
+   { .compatible = "mediatek,mt8167-m4u", .data = _data},
{ .compatible = "mediatek,mt8173-m4u", .data = _data},
{ .compatible = "mediatek,mt8183-m4u", .data = _data},
{}
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index 4696ba027a71..72f874ec9e9c 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -30,6 +30,7 @@ struct mtk_iommu_suspend_reg {
 enum mtk_iommu_plat {
M4U_MT2701,
M4U_MT2712,
+   M4U_MT8167,
M4U_MT8173,
M4U_MT8183,
 };
-- 
2.25.0

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

[PATCH v2 1/3] dt-bindings: iommu: Add binding for MediaTek MT8167 IOMMU

2020-03-02 Thread Fabien Parent 
This commit adds IOMMU binding documentation for the MT8167 SoC.

Signed-off-by: Fabien Parent 
Acked-by: Rob Herring 
---

V2: no change

---
 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt 
b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
index ce59a505f5a4..eee9116cf9bb 100644
--- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
@@ -60,6 +60,7 @@ Required properties:
"mediatek,mt2712-m4u" for mt2712 which uses generation two m4u HW.
"mediatek,mt7623-m4u", "mediatek,mt2701-m4u" for mt7623 which uses
 generation one m4u HW.
+   "mediatek,mt8167-m4u" for mt8167 which uses generation two m4u HW.
"mediatek,mt8173-m4u" for mt8173 which uses generation two m4u HW.
"mediatek,mt8183-m4u" for mt8183 which uses generation two m4u HW.
 - reg : m4u register base and size.
-- 
2.25.0

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