Re: [PATCH 37/37] vfio: Add support for Shared Virtual Addressing

[Sinan Kaya]

On 2/12/2018 1:33 PM, Jean-Philippe Brucker wrote:
> Add two new ioctl for VFIO containers. VFIO_IOMMU_BIND_PROCESS creates a
> bond between a container and a process address space, identified by a
> device-specific ID named PASID. This allows the device to target DMA
> transactions at the process virtual addresses without a need for mapping
> and unmapping buffers explicitly in the IOMMU. The process page tables are
> shared with the IOMMU, and mechanisms such as PCI ATS/PRI are used to
> handle faults. VFIO_IOMMU_UNBIND_PROCESS removes a bond created with
> VFIO_IOMMU_BIND_PROCESS.
> 
> Signed-off-by: Jean-Philippe Brucker 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 399 
> 
>  include/uapi/linux/vfio.h   |  76 
>  2 files changed, 475 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index e30e29ae4819..cac066f0026b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -60,6 +61,7 @@ MODULE_PARM_DESC(disable_hugepages,
>  
>  struct vfio_iommu {
>   struct list_headdomain_list;
> + struct list_headmm_list;
>   struct vfio_domain  *external_domain; /* domain for external user */
>   struct mutexlock;
>   struct rb_root  dma_list;
> @@ -90,6 +92,15 @@ struct vfio_dma {
>  struct vfio_group {
>   struct iommu_group  *iommu_group;
>   struct list_headnext;
> + boolsva_enabled;
> +};
> +
> +struct vfio_mm {
> +#define VFIO_PASID_INVALID   (-1)
> + spinlock_t  lock;
> + int pasid;
> + struct mm_struct*mm;
> + struct list_headnext;
>  };
>  
>  /*
> @@ -1117,6 +1128,157 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
>   return 0;
>  }
>  
> +static int vfio_iommu_mm_exit(struct device *dev, int pasid, void *data)
> +{
> + struct vfio_mm *vfio_mm = data;
> +
> + /*
> +  * The mm_exit callback cannot block, so we can't take the iommu mutex
> +  * and remove this vfio_mm from the list. Hopefully the SVA code will
> +  * relax its locking requirement in the future.
> +  *
> +  * We mostly care about attach_group, which will attempt to replay all
> +  * binds in this container. Ensure that it doesn't touch this defunct mm
> +  * struct, by clearing the pointer. The structure will be freed when the
> +  * group is removed from the container.
> +  */
> + spin_lock(_mm->lock);
> + vfio_mm->mm = NULL;
> + spin_unlock(_mm->lock);
> +
> + return 0;
> +}
> +
> +static int vfio_iommu_sva_init(struct device *dev, void *data)
> +{

data is not getting used.

> +
> + int ret;
> +
> + ret = iommu_sva_device_init(dev, IOMMU_SVA_FEAT_PASID |
> + IOMMU_SVA_FEAT_IOPF, 0);
> + if (ret)
> + return ret;
> +
> + return iommu_register_mm_exit_handler(dev, vfio_iommu_mm_exit);
> +}
> +
> +static int vfio_iommu_sva_shutdown(struct device *dev, void *data)
> +{
> + iommu_sva_device_shutdown(dev);
> + iommu_unregister_mm_exit_handler(dev);
> +
> + return 0;
> +}
> +
> +static int vfio_iommu_bind_group(struct vfio_iommu *iommu,
> +  struct vfio_group *group,
> +  struct vfio_mm *vfio_mm)
> +{
> + int ret;
> + int pasid;
> +
> + if (!group->sva_enabled) {
> + ret = iommu_group_for_each_dev(group->iommu_group, NULL,
> +vfio_iommu_sva_init);
> + if (ret)
> + return ret;
> +
> + group->sva_enabled = true;
> + }
> +
> + ret = iommu_sva_bind_group(group->iommu_group, vfio_mm->mm, ,
> +IOMMU_SVA_FEAT_PASID | IOMMU_SVA_FEAT_IOPF,
> +vfio_mm);
> + if (ret)
> + return ret;

don't you need to clean up the work done by vfio_iommu_sva_init() here.

> +
> + if (WARN_ON(vfio_mm->pasid != VFIO_PASID_INVALID && pasid !=
> + vfio_mm->pasid))
> + return -EFAULT;
> +
> + vfio_mm->pasid = pasid;
> +
> + return 0;
> +}
> +
> +static void vfio_iommu_unbind_group(struct vfio_group *group,
> + struct vfio_mm *vfio_mm)
> +{
> + iommu_sva_unbind_group(group->iommu_group, vfio_mm->pasid);
> +}
> +
> +static void vfio_iommu_unbind(struct vfio_iommu *iommu,
> +   struct vfio_mm *vfio_mm)
> +{
> + struct vfio_group *group;
> + struct vfio_domain *domain;
> +
> + list_for_each_entry(domain, >domain_list, next)
> + list_for_each_entry(group, >group_list, next)
> + vfio_iommu_unbind_group(group, 

Re: [PATCH 16/37] iommu: Add generic PASID table library

[Jacob Pan]

On Mon, 12 Feb 2018 18:33:31 +
Jean-Philippe Brucker  wrote:

> Add a small API within the IOMMU subsystem to handle different
> formats of PASID tables. It uses the same principle as io-pgtable:
> 
> * The IOMMU driver registers a PASID table with some invalidation
>   callbacks.
> * The pasid-table lib allocates a set of tables of the right format,
> and returns an iommu_pasid_table_ops structure.
> * The IOMMU driver allocates entries and writes them using the
> provided ops.
> * The pasid-table lib calls the IOMMU driver back for invalidation
> when necessary.
> * The IOMMU driver unregisters the ops which frees the tables when
>   finished.
> 
> An example user will be Arm SMMU in a subsequent patch.
> 
> Signed-off-by: Jean-Philippe Brucker 
> ---
>  drivers/iommu/Kconfig   |   8 +++
>  drivers/iommu/Makefile  |   1 +
>  drivers/iommu/iommu-pasid.c |  53 +
>  drivers/iommu/iommu-pasid.h | 142
>  4 files changed, 204
> insertions(+) create mode 100644 drivers/iommu/iommu-pasid.c
>  create mode 100644 drivers/iommu/iommu-pasid.h
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index e751bb9958ba..8add90ba9b75 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -60,6 +60,14 @@ config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
>  
>  endmenu
>  
> +menu "Generic PASID table support"
> +
> +# Selected by the actual PASID table implementations
> +config IOMMU_PASID_TABLE
> + bool
> +
> +endmenu
> +
>  config IOMMU_IOVA
>   tristate
>  
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index f4324e29035e..338e59c93131 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_IOMMU_FAULT) += io-pgfault.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
> +obj-$(CONFIG_IOMMU_PASID_TABLE) += iommu-pasid.o
>  obj-$(CONFIG_IOMMU_IOVA) += iova.o
>  obj-$(CONFIG_OF_IOMMU)   += of_iommu.o
>  obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
> diff --git a/drivers/iommu/iommu-pasid.c b/drivers/iommu/iommu-pasid.c
> new file mode 100644
> index ..6b21d369d514
> --- /dev/null
> +++ b/drivers/iommu/iommu-pasid.c
> @@ -0,0 +1,53 @@
> +/*
> + * PASID table management for the IOMMU
> + *
> + * Copyright (C) 2018 ARM Ltd.
> + * Author: Jean-Philippe Brucker 
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include 
> +
> +#include "iommu-pasid.h"
> +
> +static const struct iommu_pasid_init_fns *
> +pasid_table_init_fns[PASID_TABLE_NUM_FMTS] = {
> +};
> +
> +struct iommu_pasid_table_ops *
> +iommu_alloc_pasid_ops(enum iommu_pasid_table_fmt fmt,
> +   struct iommu_pasid_table_cfg *cfg, void
> *cookie) +{
I guess you don't need to pass in cookie here.
> + struct iommu_pasid_table *table;
> + const struct iommu_pasid_init_fns *fns;
> +
> + if (fmt >= PASID_TABLE_NUM_FMTS)
> + return NULL;
> +
> + fns = pasid_table_init_fns[fmt];
> + if (!fns)
> + return NULL;
> +
> + table = fns->alloc(cfg, cookie);
> + if (!table)
> + return NULL;
> +
> + table->fmt = fmt;
> + table->cookie = cookie;
> + table->cfg = *cfg;
> +
the ops is already IOMMU model specific, why do you need to pass cfg
back?
> + return >ops;
If there is no common code that uses these ops, I don't see the benefit
of having these APIs. Or the plan is to consolidate even further such
that referene to pasid table can be attached at per iommu_domain etc,
but that would be model specific choice.

Jacob 
> +}
> +
> +void iommu_free_pasid_ops(struct iommu_pasid_table_ops *ops)
> +{
> + struct iommu_pasid_table *table;
> +
> + if (!ops)
> + return;
> +
> + table = container_of(ops, struct iommu_pasid_table, ops);
> + iommu_pasid_flush_all(table);
> + pasid_table_init_fns[table->fmt]->free(table);
> +}
> diff --git a/drivers/iommu/iommu-pasid.h b/drivers/iommu/iommu-pasid.h
> new file mode 100644
> index ..40a27d35c1e0
> --- /dev/null
> +++ b/drivers/iommu/iommu-pasid.h
> @@ -0,0 +1,142 @@
> +/*
> + * PASID table management for the IOMMU
> + *
> + * Copyright (C) 2017 ARM Ltd.
> + * Author: Jean-Philippe Brucker 
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +#ifndef __IOMMU_PASID_H
> +#define __IOMMU_PASID_H
> +
> +#include 
> +#include "io-pgtable.h"
> +
> +struct mm_struct;
> +
> +enum iommu_pasid_table_fmt {
> + PASID_TABLE_NUM_FMTS,
> +};
> +
> +/**
> + * iommu_pasid_entry - Entry of a PASID table
> + *
> + * @token:   architecture-specific data needed to uniquely
> identify the
> + *   entry. Most notably used for TLB invalidation
> + */
> +struct iommu_pasid_entry {
> + u64 

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[Robin Murphy]

On 23/02/18 10:24, JeffyChen wrote:

Hi guys,

On 02/01/2018 07:19 PM, JeffyChen wrote:



diff --git
a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
index 2098f7732264..33dd853359fa 100644
--- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
@@ -14,6 +14,13 @@ Required properties:
  "single-master" device, and needs no
additional information
  to associate with its master device.  See:

Documentation/devicetree/bindings/iommu/iommu.txt
+Optional properties:
+- clocks : A list of master clocks requires for the IOMMU to be
accessible
+   by the host CPU. The number of clocks depends on the
master
+   block and might as well be zero. See [1] for generic 
clock

+   bindings description.


Hardware blocks don't have a variable number of clock connections.


I think you underestimate the imagination of hardware designers. :)


Learned long ago to never do that. If there are 2 ways to do
something, they will find a 3rd way.


For Rockchip IOMMU, there is a set of clocks, which all need to be
enabled for IOMMU register access to succeed. The clocks are not
directly fed to the IOMMU, but they are needed for the various buses
and intermediate blocks on the way to the IOMMU to work.


The binding should describe the clock connections, not what clocks a
driver needs (currently). It sounds like a lack of managing bus clocks
to me.

In any case, the binding must be written so it can be verified. If you
can have any number of clocks with any names, there's no point in
documenting.


the rockchip IOMMU is part of the master block in hardware, so it needs
to control the master's power domain and some of the master's clocks
when access it's registers.

and the number of clocks needed here, might be different between each
IOMMUs(according to which master block it belongs), it's a little like
our power domain:
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935 




i'm not sure how to describe this correctly, is it ok use something like
"the same as it's master block"?


would it make sense to add a property to specify the master who owns the 
iommu, and we can get all clocks(only some of those clocks are actually 
needed) from it in the of_xlate()? and we can also reuse the clock-names 
of that master to build clk_bulk_data and log errors in clk_bulk_get.


I'm inclined to agree with Rob here - if we're to add anything to the 
binding, it should only be whatever clock inputs are defined for the 
IOMMU IP block itself. If Linux doesn't properly handle the interconnect 
clock hierarchy external to a particular integration, that's a separate 
issue and it's not the binding's problem.


I actually quite like the hack of "borrowing" the clocks from 
dev->of_node in of_xlate() - you shouldn't need any DT changes for that, 
because you already know that each IOMMU instance only has the one 
master device anyway.


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

Re: [PATCH 1/4] iommu: Add virtio-iommu driver

[Michael S. Tsirkin]

On Thu, Feb 22, 2018 at 11:04:30AM +, Jean-Philippe Brucker wrote:
> On 21/02/18 20:12, kbuild test robot wrote:
> [...]
> > config: arm64-allmodconfig (attached as .config)
> [...]
> >aarch64-linux-gnu-ld: arch/arm64/kernel/head.o: relocation 
> > R_AARCH64_ABS32 against `_kernel_offset_le_lo32' can not be used when 
> > making a shared object
> >arch/arm64/kernel/head.o: In function `kimage_vaddr':
> >(.idmap.text+0x0): dangerous relocation: unsupported relocation
> 
> Is this related?
> 
> >arch/arm64/kernel/head.o: In function `__primary_switch':
> >(.idmap.text+0x340): dangerous relocation: unsupported relocation
> >(.idmap.text+0x348): dangerous relocation: unsupported relocation
> >drivers/iommu/virtio-iommu.o: In function `viommu_probe':
> >virtio-iommu.c:(.text+0xbdc): undefined reference to 
> > `virtio_check_driver_offered_feature'
> >virtio-iommu.c:(.text+0xcfc): undefined reference to 
> > `virtio_check_driver_offered_feature'
> >virtio-iommu.c:(.text+0xe10): undefined reference to 
> > `virtio_check_driver_offered_feature'
> >drivers/iommu/virtio-iommu.o: In function `viommu_send_reqs_sync':
> >virtio-iommu.c:(.text+0x130c): undefined reference to `virtqueue_add_sgs'
> >virtio-iommu.c:(.text+0x1398): undefined reference to `virtqueue_kick'
> >virtio-iommu.c:(.text+0x14d4): undefined reference to `virtqueue_get_buf'
> >drivers/iommu/virtio-iommu.o: In function `virtio_iommu_drv_init':
> >virtio-iommu.c:(.init.text+0x1c): undefined reference to 
> > `register_virtio_driver'
> >drivers/iommu/virtio-iommu.o: In function `virtio_iommu_drv_exit':
> >>> virtio-iommu.c:(.exit.text+0x14): undefined reference to 
> >>> `unregister_virtio_driver'
> 
> Right. At the moment CONFIG_VIRTIO_IOMMU is a bool instead of tristate,
> because the IOMMU subsystem isn't entirely ready to have IOMMU drivers
> built as modules. In addition to exporting symbols it would also needs to
> hold off probing endpoints behind the IOMMU until the IOMMU driver is
> loaded. At the moment (I think) it gives up once userspace is reached (see
> of_iommu_driver_present).
> 
> The above report is due to VIRTIO=m and VIRTIO_IOMMU=y. To solve it we could:
> 
> a) Allow VIRTIO_IOMMU to be built as module by exporting a dozen IOMMU
> symbols. It would be a lie. The driver wouldn't be usable because of the
> probe issue discussed above, but it would build.
> 
> b) Actually make any IOMMU driver work as module. Whilst it would
> certainly be a nice feature, it's a bigger problem and I don't think it
> has its place in this series.
> 
> c) Make VIRTIO_IOMMU depend on VIRTIO_MMIO=y instead of VIRTIO_MMIO, which
> I think is the sanest for now (and does work), even though distro kernels
> probably all have VIRTIO=m.
> 
> I prefer doing c) now and experiment with b) once I got some time.
> 
> Thanks,
> Jean

It kind of means it's a toy for now though. So fine as long
as it's out of tree.

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

Re: [PATCH v2 4/4] iommu/arm-smmu-v3: Support 52-bit virtual address

[Robin Murphy]

On 26/02/18 18:05, Will Deacon wrote:

On Thu, Dec 14, 2017 at 04:58:53PM +, Robin Murphy wrote:

Stage 1 input addresses are effectively 64-bit in SMMUv3 anyway, so
really all that's involved is letting io-pgtable know the appropriate
upper bound for T0SZ.

Tested-by: Nate Watterson 
Signed-off-by: Robin Murphy 
---

v2: No change

  drivers/iommu/arm-smmu-v3.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index c9c4e6132e27..7059a0805bd1 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -102,6 +102,7 @@
  #define IDR5_OAS_44_BIT   (4 << IDR5_OAS_SHIFT)
  #define IDR5_OAS_48_BIT   (5 << IDR5_OAS_SHIFT)
  #define IDR5_OAS_52_BIT   (6 << IDR5_OAS_SHIFT)
+#define IDR5_VAX   (1 << 10)


I think this is actually a two-bit field, so we should check the whole
thing.


Yes, I either overlooked that or took a cheeky shortcut; will fix.

  
  #define ARM_SMMU_CR0			0x20

  #define CR0_CMDQEN(1 << 3)
@@ -604,6 +605,7 @@ struct arm_smmu_device {
  #define ARM_SMMU_FEAT_STALLS  (1 << 11)
  #define ARM_SMMU_FEAT_HYP (1 << 12)
  #define ARM_SMMU_FEAT_STALL_FORCE (1 << 13)
+#define ARM_SMMU_FEAT_VAX  (1 << 14)
u32 features;
  
  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)

@@ -1656,6 +1658,8 @@ static int arm_smmu_domain_finalise(struct iommu_domain 
*domain)
switch (smmu_domain->stage) {
case ARM_SMMU_DOMAIN_S1:
ias = VA_BITS;
+   if (VA_BITS > 48 && !(smmu->features & ARM_SMMU_FEAT_VAX))


nit, but I'd rather the if condition was checking ias instead of VA_BITS.


+   ias = 48;
oas = smmu->ias;


Should we also be sanitising the oas here if the SMMU doesn't support 64k
pages? It looks like the ias/oas sanitisation isn't cleanly split between
the pgtable code and the SMMU driver.


Right, based on my previous argument it would be cleaner to set 48/52 
here based on the SMMU features, then constrain to VA_BITS in 
io-pgtable; I think I was more focused on minimising the diff initially.


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

Re: [PATCH v2 2/4] iommu/io-pgtable-arm: Support 52-bit physical address

[Robin Murphy]

On 26/02/18 18:05, Will Deacon wrote:

On Thu, Dec 14, 2017 at 04:58:51PM +, Robin Murphy wrote:

Bring io-pgtable-arm in line with the ARMv8.2-LPA feature allowing
52-bit physical addresses when using the 64KB translation granule.
This will be supported by SMMUv3.1.

Tested-by: Nate Watterson 
Signed-off-by: Robin Murphy 
---

v2: Fix TCR_PS/TCR_IPS copy-paste error

  drivers/iommu/io-pgtable-arm.c | 65 ++
  1 file changed, 47 insertions(+), 18 deletions(-)


[...]


@@ -203,6 +199,25 @@ struct arm_lpae_io_pgtable {
  
  typedef u64 arm_lpae_iopte;
  
+static arm_lpae_iopte paddr_to_iopte(phys_addr_t paddr,

+struct arm_lpae_io_pgtable *data)
+{
+   arm_lpae_iopte pte = paddr;
+
+   /* Of the bits which overlap, either 51:48 or 15:12 are always RES0 */
+   return (pte | (pte >> 36)) & ARM_LPAE_PTE_ADDR_MASK;
+}


I don't particularly like relying on properties of the paddr for correct
construction of the pte here. The existing macro doesn't have this
limitation. I suspect it's all fine at the moment because we only use TTBR0,
but I'd rather not bake that in if we can avoid it.


What's the relevance of TTBR0 to physical addresses? :/

Note that by this point paddr has been validated against cfg->oas by 
arm_lpae_map(), and validated to be granule-aligned by iommu_map(), so 
it really can't be wrong under reasonable conditions.



+static phys_addr_t iopte_to_paddr(arm_lpae_iopte pte,
+ struct arm_lpae_io_pgtable *data)
+{
+   phys_addr_t paddr = pte & ARM_LPAE_PTE_ADDR_MASK;
+   phys_addr_t paddr_hi = paddr & (ARM_LPAE_GRANULE(data) - 1);
+
+   /* paddr_hi spans nothing for 4K granule, and only RES0 bits for 16K */
+   return (paddr ^ paddr_hi) | (paddr_hi << 36);


Why do we need xor here?


Because "(paddr ^ paddr_hi)" is more concise than "(paddr & 
~(phys_addr_t)(ARM_LPAE_GRANULE(data) - 1)" or variants thereof. It's 
potentially a teeny bit more efficient too, I think, but it's mostly 
about the readability.



  static bool selftest_running = false;
  
  static dma_addr_t __arm_lpae_dma_addr(void *pages)

@@ -287,7 +302,7 @@ static void __arm_lpae_init_pte(struct arm_lpae_io_pgtable 
*data,
pte |= ARM_LPAE_PTE_TYPE_BLOCK;
  
  	pte |= ARM_LPAE_PTE_AF | ARM_LPAE_PTE_SH_IS;

-   pte |= pfn_to_iopte(paddr >> data->pg_shift, data);
+   pte |= paddr_to_iopte(paddr, data);
  
  	__arm_lpae_set_pte(ptep, pte, >iop.cfg);

  }
@@ -528,7 +543,7 @@ static int arm_lpae_split_blk_unmap(struct 
arm_lpae_io_pgtable *data,
if (size == split_sz)
unmap_idx = ARM_LPAE_LVL_IDX(iova, lvl, data);
  
-	blk_paddr = iopte_to_pfn(blk_pte, data) << data->pg_shift;

+   blk_paddr = iopte_to_paddr(blk_pte, data);
pte = iopte_prot(blk_pte);
  
  	for (i = 0; i < tablesz / sizeof(pte); i++, blk_paddr += split_sz) {

@@ -652,12 +667,13 @@ static phys_addr_t arm_lpae_iova_to_phys(struct 
io_pgtable_ops *ops,
  
  found_translation:

iova &= (ARM_LPAE_BLOCK_SIZE(lvl, data) - 1);
-   return ((phys_addr_t)iopte_to_pfn(pte,data) << data->pg_shift) | iova;
+   return iopte_to_paddr(pte, data) | iova;
  }
  
  static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)

  {
-   unsigned long granule;
+   unsigned long granule, page_sizes;
+   unsigned int max_addr_bits = 48;
  
  	/*

 * We need to restrict the supported page sizes to match the
@@ -677,17 +693,24 @@ static void arm_lpae_restrict_pgsizes(struct 
io_pgtable_cfg *cfg)
  
  	switch (granule) {

case SZ_4K:
-   cfg->pgsize_bitmap &= (SZ_4K | SZ_2M | SZ_1G);
+   page_sizes = (SZ_4K | SZ_2M | SZ_1G);
break;
case SZ_16K:
-   cfg->pgsize_bitmap &= (SZ_16K | SZ_32M);
+   page_sizes = (SZ_16K | SZ_32M);
break;
case SZ_64K:
-   cfg->pgsize_bitmap &= (SZ_64K | SZ_512M);
+   max_addr_bits = 52;
+   page_sizes = (SZ_64K | SZ_512M);
+   if (cfg->oas > 48)
+   page_sizes |= 1ULL << 42; /* 4TB */
break;
default:
-   cfg->pgsize_bitmap = 0;
+   page_sizes = 0;
}
+
+   cfg->pgsize_bitmap &= page_sizes;
+   cfg->ias = min(cfg->ias, max_addr_bits);
+   cfg->oas = min(cfg->oas, max_addr_bits);


I don't think we should be writing to the ias/oas fields here, at least
now without auditing the drivers and updating the comments about the
io-pgtable API. For example, the SMMUv3 driver uses its own ias local
variable to initialise the domain geometry, and won't pick up any changes
made here.


As you've discovered, the driver thing is indeed true. More generally, 
though, we've always adjusted cfg->pgsize_bitmap here, so I don't see 
why other fields should be treated 

Re: [PATCH v2 1/4] iommu/arm-smmu-v3: Clean up address masking

[Robin Murphy]

Hi Will,

On 26/02/18 18:04, Will Deacon wrote:

Hi Robin,

On Thu, Dec 14, 2017 at 04:58:50PM +, Robin Murphy wrote:

Before trying to add the SMMUv3.1 support for 52-bit addresses, make
things bearable by cleaning up the various address mask definitions to
use GENMASK_ULL() consistently. The fact that doing so reveals (and
fixes) a latent off-by-one in Q_BASE_ADDR_MASK only goes to show what a
jolly good idea it is...

Tested-by: Nate Watterson 
Signed-off-by: Robin Murphy 
---

v2: Clean up one more now-unnecessary linewrap

  drivers/iommu/arm-smmu-v3.c | 53 ++---
  1 file changed, 21 insertions(+), 32 deletions(-)


Whilst I agree that using GENMASK is better, this patch does mean that the
driver is (more) inconsistent with its _MASK terminology in that you can't
generally tell whether a definition that ends in _MASK is shifted or not,
and this isn't even consistent for fields within the same register.


The apparently slightly-less-than-obvious internal consistency is that 
every mask used for an *address field* is now in-place, while other 
types of field are still handled as inconsistently as they were before. 
It should also be the case that every x_MASK without a corresponding 
x_SHIFT defined next to it is unshifted.


Either way it's certainly no *worse* than the current situation where 
address masks sometimes have a nonzero shift, sometimes have zero bits 
at the bottom and a shift of 0, and sometimes have no shift defined at all.


Thinking about it some more, the address masks should only ever be 
needed when *extracting* an address from a register/structure word, or 
validating them in the context of an address *before* inserting into a 
field - if we can't trust input to be correct then just silently masking 
off bits probably isn't the best idea either way - so IMHO there is 
plenty of contextual disambiguation too.



Should we be using GENMASK/BIT for all fields instead and removing all of
the _SHIFT definitions?


I'm all aboard using BIT() consistently for single-bit boolean fields, 
but for multi-bit fields in general we do have to keep an explicit shift 
defined *somewhere* in order to make sensible use of the value, i.e. either:


val = (reg >> 22) & 0x1f;
reg = (val & 0x1f) << 22;

or:
val = (reg & 0x07c0) >> 22;
reg = (val << 22) & 0x07c0;

[ but ideally not this mess we currently have in some places:

val = (reg & 0x1f << 22) >> 22;
]

Again, I'd gladly clean everything up to at least be self-consistent 
(and line up more with how we did things in SMMUv2) if you think it's 
worthwhile. Although I guess that means I'd get the job of fixing up 
future stable backport conflicts too ;)


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

Re: [PATCH 0/8] R-Car M3-N: Enable EtherAVB device node

[jacopo mondi]

Hi Geert,

On Mon, Feb 26, 2018 at 07:28:47PM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Feb 26, 2018 at 6:57 PM, Jacopo Mondi  
> wrote:
> >   as discussed with you Sergei and Geert, in order to enable EtherAVB for
> > R8A77965 we first wanted to make the phy-mode "rgmii-txid" a board specific
> > property for all other SoCs.
>
> Thanks for your series!
>
> > This series adds  the phy-mode property to salvator-common.dtsi and reset 
> > the
> > one for R8A77965/R8A7796/R8A77995 to "rgmii".
>
> I forgot that r8a7795.dtsi and r8a7796.dtsi are used for the ULCB boards, too.
> So to avoid regressions, you need to make a similar change to ulcb.dtsi like 
> you
> made for salvator-common.dtsi.
>
> In addition, r8a77995.dtsi is only used for the Draak board.
> So you have to update r8a7795-draak.dtsi first, too.

Of course! I forgot about ULCB (and had a patch for Draak I lost while
rebasing :/ )
>
> > Then, in order to enable EtherAVB for R-Car M3-N, iommu nodes had to be 
> > added
> > as they are referenced by the "avb" device node (CC the iommu list for the
> > series for that reason).
>
> I don't think we need the iommu properties and nodes at this early stage.
> Ethernet works fine without them. Simon, do you agree?

I'll wait for Simon reply and then resend, possibly without iommu and
ULCB and Draak patches!

Thanks
   j

>
> P.S. scripts/dtc/dtx_diff is a great tool to compare DTBs before and after 
> your
>   changes. It would have revealed the changes for ULCB and Draak.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Question on IOMMU indentity mapping for intel-iommu

[David Woodhouse]

On Mon, 2018-02-26 at 15:01 -0800, Alexander Duyck wrote:
> I am interested in adding a new memory mapping option that
> establishes
> one identity-mapped region for all DMA_TO_DEVICE mappings and creates
> a new dynamic mapping for any DMA_FROM_DEVICE and DMA_BIDIRECTIONAL
> mappings. My thought is it should allow for a compromise between
> security and performance (in the case of networking) in that many of
> the server NICs drivers these days are running with mostly pinned or
> resused paged for Rx. By using an identity mapping for the Tx packets
> we should be able to significantly cut down on the IOMMU overhead for
> the device. The other advantage if this works is that we could use
> this to possibly do something like dirty page tracking in the case of
> a emulated version of the IOMMU.
> 
> I was originally thinking I could get away with just reusing the
> identity mapping code but it looks like that would end up merging
> everything into one domain if I am understanding correctly. Do I have
> that right?
> 
> Would I be correct in assuming that I will need to have a separate
> domain per device, each domain containing the 1 TO_DEVICE identity
> mapped region, and then whatever other mappings are needed to handle
> the FROM and BIDIRECTIONAL mappings?

In the normal model where we explicitly map every RX and TX buffer, you
have a domain device anyway; that's not a new requirement for your
model.

It sounds like an interesting idea; I agree that it's a reasonable
compromise between security and performance. The device can *read* all
of memory, but it can't write anywhere that isn't explicitly mapped.

In addition, we're mapping buffers for RX some time in advance of them
being needed (replenishing the tail of the RX ring), where the latency
of the map operation hopefully shouldn't be quite so much of an issue.
While packets for TX can go straight to the device with no latency.
Overall, I think it might work really well.

You don't want the existing identity mapping code; that will give you a
RW mapping which you don't want — you really do want read-only or this
whole exercise is pointless, right? And you're right, it would have put
the domain into the single identity domain.

You could probably start by mocking this up with the IOMMU API. Create
a domain with the 1:1 read-only mapping of all memory, add your device
to it, and then do your writeable mappings on top (at IOVAs higher than
the top of physical memory). That's probably a quick way to assess
performance and prove the concept (although you don't get deferred
unmap of RX packets that way, which might mess things up a bit).

When we expose this through the DMA API, I'd quite like this *not* to
be Intel-specific. It could reasonably live in a higher layer and be
usable with all kinds of IOMMU implementations.

smime.p7s
Description: S/MIME cryptographic signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/8] R-Car M3-N: Enable EtherAVB device node

[Simon Horman]

On Tue, Feb 27, 2018 at 09:28:38AM +0100, jacopo mondi wrote:
> Hi Geert,
> 
> On Mon, Feb 26, 2018 at 07:28:47PM +0100, Geert Uytterhoeven wrote:
> > Hi Jacopo,
> >
> > On Mon, Feb 26, 2018 at 6:57 PM, Jacopo Mondi  
> > wrote:
> > >   as discussed with you Sergei and Geert, in order to enable EtherAVB for
> > > R8A77965 we first wanted to make the phy-mode "rgmii-txid" a board 
> > > specific
> > > property for all other SoCs.
> >
> > Thanks for your series!
> >
> > > This series adds  the phy-mode property to salvator-common.dtsi and reset 
> > > the
> > > one for R8A77965/R8A7796/R8A77995 to "rgmii".
> >
> > I forgot that r8a7795.dtsi and r8a7796.dtsi are used for the ULCB boards, 
> > too.
> > So to avoid regressions, you need to make a similar change to ulcb.dtsi 
> > like you
> > made for salvator-common.dtsi.
> >
> > In addition, r8a77995.dtsi is only used for the Draak board.
> > So you have to update r8a7795-draak.dtsi first, too.
> 
> Of course! I forgot about ULCB (and had a patch for Draak I lost while
> rebasing :/ )
> >
> > > Then, in order to enable EtherAVB for R-Car M3-N, iommu nodes had to be 
> > > added
> > > as they are referenced by the "avb" device node (CC the iommu list for the
> > > series for that reason).
> >
> > I don't think we need the iommu properties and nodes at this early stage.
> > Ethernet works fine without them. Simon, do you agree?
> 
> I'll wait for Simon reply and then resend, possibly without iommu and
> ULCB and Draak patches!

I don't feel strongly about this but I don't think iommu is a strict
dependency of enabling Ethernet and I think its good not to include extra
dependencies - if it was me I'd try to get Ethernet accepted then follow-up
on iommu.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu