Re: [PATCH 18/28] mm: enforce that vmap can't map pages executable

[Mark Rutland]

On Wed, Apr 08, 2020 at 01:59:16PM +0200, Christoph Hellwig wrote:
> To help enforcing the W^X protection don't allow remapping existing
> pages as executable.
> 
> Based on patch from Peter Zijlstra .
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  arch/x86/include/asm/pgtable_types.h | 6 ++
>  include/asm-generic/pgtable.h| 4 
>  mm/vmalloc.c | 2 +-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/pgtable_types.h 
> b/arch/x86/include/asm/pgtable_types.h
> index 947867f112ea..2e7c442cc618 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -282,6 +282,12 @@ typedef struct pgprot { pgprotval_t pgprot; } pgprot_t;
>  
>  typedef struct { pgdval_t pgd; } pgd_t;
>  
> +static inline pgprot_t pgprot_nx(pgprot_t prot)
> +{
> + return __pgprot(pgprot_val(prot) | _PAGE_NX);
> +}
> +#define pgprot_nx pgprot_nx
> +
>  #ifdef CONFIG_X86_PAE

I reckon for arm64 we can do similar in our :

#define pgprot_nx(pgprot_t prot) \
__pgprot_modify(prot, 0, PTE_PXN)

... matching the style of our existing pgprot_*() modifier helpers.

Mark.

>  
>  /*
> diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
> index 329b8c8ca703..8c5f9c29698b 100644
> --- a/include/asm-generic/pgtable.h
> +++ b/include/asm-generic/pgtable.h
> @@ -491,6 +491,10 @@ static inline int arch_unmap_one(struct mm_struct *mm,
>  #define flush_tlb_fix_spurious_fault(vma, address) flush_tlb_page(vma, 
> address)
>  #endif
>  
> +#ifndef pgprot_nx
> +#define pgprot_nx(prot)  (prot)
> +#endif
> +
>  #ifndef pgprot_noncached
>  #define pgprot_noncached(prot)   (prot)
>  #endif
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 7356b3f07bd8..334c75251ddb 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2390,7 +2390,7 @@ void *vmap(struct page **pages, unsigned int count,
>   if (!area)
>   return NULL;
>  
> - if (map_kernel_range((unsigned long)area->addr, size, prot,
> + if (map_kernel_range((unsigned long)area->addr, size, pgprot_nx(prot),
>   pages) < 0) {
>   vunmap(area->addr);
>   return NULL;
> -- 
> 2.25.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 26/28] arm64: use __vmalloc_node in arch_alloc_vmap_stack

[Mark Rutland]

On Wed, Apr 08, 2020 at 01:59:24PM +0200, Christoph Hellwig wrote:
> arch_alloc_vmap_stack can use a slightly higher level vmalloc function.
> 
> Signed-off-by: Christoph Hellwig 

Acked-by: Mark Rutland 

Mark.

> ---
>  arch/arm64/include/asm/vmap_stack.h | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/vmap_stack.h 
> b/arch/arm64/include/asm/vmap_stack.h
> index 0a12115d9638..0cc6636e3f15 100644
> --- a/arch/arm64/include/asm/vmap_stack.h
> +++ b/arch/arm64/include/asm/vmap_stack.h
> @@ -19,10 +19,8 @@ static inline unsigned long *arch_alloc_vmap_stack(size_t 
> stack_size, int node)
>  {
>   BUILD_BUG_ON(!IS_ENABLED(CONFIG_VMAP_STACK));
>  
> - return __vmalloc_node_range(stack_size, THREAD_ALIGN,
> - VMALLOC_START, VMALLOC_END,
> - THREADINFO_GFP, PAGE_KERNEL, 0, node,
> - __builtin_return_address(0));
> + return __vmalloc_node(stack_size, THREAD_ALIGN, THREADINFO_GFP, node,
> + __builtin_return_address(0));
>  }
>  
>  #endif /* __ASM_VMAP_STACK_H */
> -- 
> 2.25.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] swiotlb: Allow swiotlb to live at pre-defined address

[Mark Rutland]

On Thu, Mar 26, 2020 at 06:11:31PM +0100, Alexander Graf wrote:
> On 26.03.20 18:05, Christoph Hellwig wrote:
> > 
> > On Thu, Mar 26, 2020 at 05:29:22PM +0100, Alexander Graf wrote:
> > > The swiotlb is a very convenient fallback mechanism for bounce buffering 
> > > of
> > > DMAable data. It is usually used for the compatibility case where devices
> > > can only DMA to a "low region".
> > > 
> > > However, in some scenarios this "low region" may be bound even more
> > > heavily. For example, there are embedded system where only an SRAM region
> > > is shared between device and CPU. There are also heterogeneous computing
> > > scenarios where only a subset of RAM is cache coherent between the
> > > components of the system. There are partitioning hypervisors, where
> > > a "control VM" that implements device emulation has limited view into a
> > > partition's memory for DMA capabilities due to safety concerns.
> > > 
> > > This patch adds a command line driven mechanism to move all DMA memory 
> > > into
> > > a predefined shared memory region which may or may not be part of the
> > > physical address layout of the Operating System.
> > > 
> > > Ideally, the typical path to set this configuration would be through 
> > > Device
> > > Tree or ACPI, but neither of the two mechanisms is standardized yet. Also,
> > > in the x86 MicroVM use case, we have neither ACPI nor Device Tree, but
> > > instead configure the system purely through kernel command line options.
> > > 
> > > I'm sure other people will find the functionality useful going forward
> > > though and extend it to be triggered by DT/ACPI in the future.
> > 
> > I'm totally against hacking in a kernel parameter for this.  We'll need
> > a proper documented DT or ACPI way.
> 
> I'm with you on that sentiment, but in the environment I'm currently looking
> at, we have neither DT nor ACPI: The kernel gets purely configured via
> kernel command line. For other unenumerable artifacts on the system, such as
> virtio-mmio platform devices, that works well enough and also basically
> "hacks a kernel parameter" to specify the system layout.

On the arm64 front, you'd *have* to pass a DT to the kernel (as that's
where we get the command line from), and we *only* discover memory
from the DT or EFI memory map, so the arguments above aren't generally
applicable. You can enumerate virtio-mmio devices from DT, also.

Device-specific constraints on memory should really be described in a
per-device fashion in the FW tables so that the OS can decide how to
handle them. Just becuase one device can only access memory in a
specific 1MiB window doesn't mean all other should be forced to share
the same constraint. I think that's what Christoph was alluding to.

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

Re: [PATCH 6/6] arm64: document the choice of page attributes for pgprot_dmacoherent

[Mark Rutland]

On Fri, Aug 16, 2019 at 06:31:18PM +0100, Will Deacon wrote:
> Hi Christoph,
> 
> Thanks for spinning this into a patch.
> 
> On Fri, Aug 16, 2019 at 09:07:54AM +0200, Christoph Hellwig wrote:
> > Based on an email from Will Deacon.
> > 
> > Signed-off-by: Christoph Hellwig 
> > ---
> >  arch/arm64/include/asm/pgtable.h | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/pgtable.h 
> > b/arch/arm64/include/asm/pgtable.h
> > index 6700371227d1..6ff221d9a631 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -435,6 +435,14 @@ static inline pmd_t pmd_mkdevmap(pmd_t pmd)
> > __pgprot_modify(prot, PTE_ATTRINDX_MASK, PTE_ATTRINDX(MT_NORMAL_NC) | 
> > PTE_PXN | PTE_UXN)
> >  #define pgprot_device(prot) \
> > __pgprot_modify(prot, PTE_ATTRINDX_MASK, PTE_ATTRINDX(MT_DEVICE_nGnRE) 
> > | PTE_PXN | PTE_UXN)
> > +/*
> > + * DMA allocations for non-coherent devices use what the Arm architecture 
> > calls
> > + * "Normal non-cacheable" memory, which permits speculation, unaligned 
> > accesses
> > + * and merging of writes.  This is different from "Strongly Ordered" memory
> > + * which is intended for MMIO and thus forbids speculation, preserves 
> > access
> > + * size, requires strict alignment and also forces write responses to come 
> > from
> > + * the endpoint.
> > + */
> 
> Mind if I tweak the second sentence to be:
> 
>   This is different from "Device-nGnR[nE]" memory which is intended for MMIO
>   and thus forbids speculation, preserves access size, requires strict
>   alignment and can also force write responses to come from the endpoint.
> 
> ? It's a small change, but it better fits with the arm64 terminology
> ("strongly ordered" is no longer used in the architecture).
> 
> If you're happy with that, I can make the change and queue this patch
> for 5.4.

FWIW, with that wording:

Acked-by: Mark Rutland 

Mark.

Re: [RFC PATCH 1/2] dt-bindings: pci: Add drop mask property for MSI and IOMMU

[Mark Rutland]

On Fri, Jul 07, 2017 at 08:22:21AM -0700, Scott Branden wrote:
> On 17-07-07 07:55 AM, Robin Murphy wrote:
> >On 07/07/17 14:30, Mark Rutland wrote:
> >>On Fri, Jul 07, 2017 at 12:39:58PM +0530, Srinath Mannam wrote:
> >>>+Example (6)
> >>>+===
> >>>+
> >>>+/ {
> >>>+  #address-cells = <1>;
> >>>+  #size-cells = <1>;
> >>>+
> >>>+  msi: msi-controller@a {
> >>>+  reg = <0xa 0x1>;
> >>>+  compatible = "vendor,some-controller";
> >>>+  msi-controller;
> >>>+  #msi-cells = <1>;
> >>>+  };
> >>>+
> >>>+  pci: pci@f {
> >>>+  reg = <0xf 0x1>;
> >>>+  compatible = "vendor,pcie-root-complex";
> >>>+  device_type = "pci";
> >>>+
> >>>+  /*
> >>>+   * The sideband data provided to the  MSI controller is
> >>>+   * a 10bit data derived from the RID by dropping
> >>>+   * 4 MSBs of device number and 2 MSBs of function number.
> >>>+   */
> >>>+  msi-map = <0x0 _a 0x0 0x100>,
> >>>+  msi-map-drop-mask = <0xff09>
> >>>+  };
> >>>+};
> >>... likewise on all counts.
> >>
> >>Your mapping can be expressed today using a number of msi-map entries,
> >>which you can easily generate programmatically with a trivial perl
> >>script, without requiring a new binding or any new kernel code.
> >>
> >>Please do that instead.
> >
> >Indeed. The systems I'm aware of which need to express non-trivial RID
> >to SID mappings tend to have the bootloader probe PCI and dynamically
> >generate map entries per discovered RID, but even if you wanted to
> >statically generate the whole lot for the worst-case bus range that's
> >still only 512 entries, which is not unmanageable. Notably, it's also
> >what would have to be done (in equivalent) for IORT, although I assume
> >this is an embedded platform for which nobody cares about ACPI.
>
> Actually we will care about ACPI and need to add it (doesn't need to
> be in this patchet unless easy to do so...)

Similarly to what I said for the DT case, with IORT you can solve this
today by using multiple ID mapping entries in a node's ID mappings
array.

I don't imagine the sort of change you are proposing will sail into the
IORT spec.

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

Re: [RFC PATCH 2/2] pcie: sideband data by dropping RID bits

[Mark Rutland]

On Fri, Jul 07, 2017 at 12:39:59PM +0530, Srinath Mannam wrote:
> The MSI Device ID or Stream ID are passed as sideband
> data on the SOC bus to which PCI RC is connected.
> 
> If sideband data on SOC bus is less than 16bits then
> PCI RC will have to derive sideband data from RID by
> dropping selected bits.
> 
> This patch implements optional DT properties to generate
> smaller sideband data from RID which can be further
> mapped to MSI Device ID or Stream ID.
> 
> Sideband data generation from RID is done by dropping
> bits corresponding zero bits in {iommu/msi}-map-drop-mask.
> 
> Example: If drop-mask is 0xFF09 then sideband data is
> 8 bits bus number followed by 1 bit of device number and
> 1 bit function number. This means drop-mask=0xFF09 will
> convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).

As mentioned on the prior two patches, I don't think this is a good
idea, especially given this isn't a trivial masking operation.

It's already possible to express this mapping using a number of entries
in the the *-map properties, which you can generate programmatically
with a script.

Please do that instead.

Thanks,
Mark.

> 
> Signed-off-by: Anup Patel 
> Signed-off-by: Oza Pawandeep 
> Signed-off-by: Srinath Mannam 
> Reviewed-by: Ray Jui 
> Reviewed-by: Scott Branden 
> ---
>  drivers/iommu/of_iommu.c |  4 ++--
>  drivers/of/irq.c |  3 ++-
>  drivers/of/of_pci.c  | 48 
> +---
>  include/linux/of_pci.h   |  6 --
>  4 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 19779b8..f179724 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -169,8 +169,8 @@ static const struct iommu_ops
>*/
>   iommu_spec.np = NULL;
>   err = of_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
> -  "iommu-map-mask", _spec.np,
> -  iommu_spec.args);
> +  "iommu-map-mask", "iommu-map-drop-mask",
> +  _spec.np, iommu_spec.args);
>   if (err)
>   return err == -ENODEV ? NULL : ERR_PTR(err);
>  
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index d11437c..454f47a 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -606,7 +606,8 @@ static u32 __of_msi_map_rid(struct device *dev, struct 
> device_node **np,
>*/
>   for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
>   if (!of_pci_map_rid(parent_dev->of_node, rid_in, "msi-map",
> - "msi-map-mask", np, _out))
> + "msi-map-mask", "msi-map-drop-mask", np,
> + _out))
>   break;
>   return rid_out;
>  }
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index c9d4d3a..a914bcf 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -285,6 +285,35 @@ int of_pci_get_host_bridge_resources(struct device_node 
> *dev,
>  EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
>  #endif /* CONFIG_OF_ADDRESS */
>  
> +static inline u32 out_masked_rid(u32 rid, u32 drop_mask)
> +{
> + u32 id = 0;
> + u32 i = 0;
> +
> + /* RID's BUS, DEV, FUN values not inside the mask are invalid */
> + if (rid & ~drop_mask)
> + return -EINVAL;
> +
> + /*
> +  * RID value is translated to sideband data using drop_mask
> +  * by dropping bits corresponding zero bits in drop_mask.
> +  *
> +  * Example: If drop_mask is 0xFF09 then sideband data is
> +  * 8 bits bus number followed by 1 bit of device number and
> +  * 1 bit function number. This means drop_mask=0xFF09 will
> +  * convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).
> +  */
> + while (drop_mask) {
> + if (drop_mask & 0x1) {
> + id |= ((rid & 0x1) << i);
> + i++;
> + }
> + rid = rid >> 1;
> + drop_mask = drop_mask >> 1;
> + }
> +
> + return id;
> +}
>  /**
>   * of_pci_map_rid - Translate a requester ID through a downstream mapping.
>   * @np: root complex device node.
> @@ -304,11 +333,11 @@ EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
>   *
>   * Return: 0 on success or a standard error code on failure.
>   */
> -int of_pci_map_rid(struct device_node *np, u32 rid,
> -const char *map_name, const char *map_mask_name,
> +int of_pci_map_rid(struct device_node *np, u32 rid, const char *map_name,
> +const char *map_mask_name, const char *drop_mask_name,
>  struct device_node **target, u32 *id_out)
>  {
> - u32 map_mask, masked_rid;
> + u32 map_mask, masked_rid, drop_mask;
>   int map_len;
>   const 

Re: [RFC PATCH 1/2] dt-bindings: pci: Add drop mask property for MSI and IOMMU

[Mark Rutland]

On Fri, Jul 07, 2017 at 12:39:58PM +0530, Srinath Mannam wrote:
> This patch adds info about optional DT properties
> iommu-map-drop-mask and msi-map-drop-mask.
> 
> A drop mask represents the bits which will be
> removed/dropped by system from Requester ID before
> mapping it to msi ID or stream ID.
> 
> Signed-off-by: Anup Patel 
> Signed-off-by: Oza Pawandeep 
> Signed-off-by: Srinath Mannam 
> Reviewed-by: Ray Jui 
> Reviewed-by: Scott Branden 
> ---
>  .../devicetree/bindings/pci/pci-iommu.txt  | 31 
>  Documentation/devicetree/bindings/pci/pci-msi.txt  | 33 
> ++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt 
> b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> index 0def586..499cb27 100644
> --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> @@ -44,6 +44,9 @@ Optional properties
>  - iommu-map-mask: A mask to be applied to each Requester ID prior to being
>mapped to an IOMMU specifier per the iommu-map property.
>  
> +- iommu-map-drop-mask: A drop mask represents the bits which will be
> +  removed/dropped by system from Requester ID before mapping it to
> +  stream ID.

As mentioned in my reply to the cover letter, you're expecting this to
be handled as more than a mask, so this description is inadequate.

[...]

> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + iommu: iommu@a {
> + reg = <0xa 0x1>;
> + compatible = "vendor,some-iommu";
> + #iommu-cells = <1>;
> + };
> +
> + pci: pci@f {
> + reg = <0xf 0x1>;
> + compatible = "vendor,pcie-root-complex";
> + device_type = "pci";
> +
> + /*
> +  * The sideband data provided to the IOMMU is a 10bit
> +  * data derived from the RID by dropping 4 MSBs
> +  * of device number and 2 MSBs of function number.
> +  */
> + iommu-map = <0x0  0x0 0x1024>;
> + iommu-map-drop-mask = <0xff09>;
> + };
> +};

... as this this example.

Assuming this was truly a mask of bits to drop, you'd have:

RID ->  SID
0x  ->  0x00f6

... whereas from your cover letter it seems you want:

RID ->  SID
0x  ->  0x3f

... and I've just realsied you have non-coniguous masks, so this is even
worse.

> diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt 
> b/Documentation/devicetree/bindings/pci/pci-msi.txt
> index 9b3cc81..1de3f39 100644
> --- a/Documentation/devicetree/bindings/pci/pci-msi.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
> @@ -49,6 +49,10 @@ Optional properties
>  - msi-map-mask: A mask to be applied to each Requester ID prior to being 
> mapped
>to an msi-specifier per the msi-map property.
>  
> +- msi-map-drop-mask: A drop mask represents the bits which will be
> +  removed/dropped by system from Requester ID before mapping it to
> +  msi ID.
> +
>  - msi-parent: Describes the MSI parent of the root complex itself. Where
>the root complex and MSI controller do not pass sideband data with MSI
>writes, this property may be used to describe the MSI controller(s)
> @@ -218,3 +222,32 @@ Example (5)
> <0x _b 0x 0x1>;
>   };
>  };
> +
> +Example (6)
> +===
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + msi: msi-controller@a {
> + reg = <0xa 0x1>;
> + compatible = "vendor,some-controller";
> + msi-controller;
> + #msi-cells = <1>;
> + };
> +
> + pci: pci@f {
> + reg = <0xf 0x1>;
> + compatible = "vendor,pcie-root-complex";
> + device_type = "pci";
> +
> + /*
> +  * The sideband data provided to the  MSI controller is
> +  * a 10bit data derived from the RID by dropping
> +  * 4 MSBs of device number and 2 MSBs of function number.
> +  */
> + msi-map = <0x0 _a 0x0 0x100>,
> + msi-map-drop-mask = <0xff09>
> + };
> +};

... likewise on all counts.

Your mapping can be expressed today using a number of msi-map entries,
which you can easily generate programmatically with a trivial perl
script, without requiring a new binding or any new kernel code.

Please do that instead.

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

Re: [RFC PATCH 0/2] Add sideband data extraction

[Mark Rutland]

On Fri, Jul 07, 2017 at 12:39:57PM +0530, Srinath Mannam wrote:
> These patches implements optional DT properties to generate
> smaller sideband data from RID which can be further mapped
> to MSI Device ID or Stream ID
> 
> On some of the systems, sideband data is smaller than RID
> (16bits). For such system, sideband data has to be generated
> by dropping some of the RID bits
> 
> the process of sideband data extracted from RID can be expressed
> using optional DT property {iommu/msi}-map-drop-mask.
> 
> Example: If drop-mask is 0xFF09 then sideband data is
> 8 bits bus number followed by 1 bit of device number and
> 1 bit function number. This means drop-mask=0xFF09 will
> convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).

So IIUC, here's you're using this not only to mask bits out, but also to
determine a *shift* to apply to the value, implicitly provided by the
(contiguous) low bits of the mask.

That's really not obvious from the name.

Mark.

> 
> Srinath Mannam (2):
>   dt-bindings: pci: Add drop mask property for MSI and IOMMU
>   pcie: sideband data by dropping RID bits
> 
>  .../devicetree/bindings/pci/pci-iommu.txt  | 31 ++
>  Documentation/devicetree/bindings/pci/pci-msi.txt  | 33 +++
>  drivers/iommu/of_iommu.c   |  4 +-
>  drivers/of/irq.c   |  3 +-
>  drivers/of/of_pci.c| 48 
> --
>  include/linux/of_pci.h |  6 ++-
>  6 files changed, 117 insertions(+), 8 deletions(-)
> 
> -- 
> 2.7.4
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY

[Mark Rutland]

On Thu, May 04, 2017 at 06:05:33PM +0530, Geetha sowjanya wrote:
> From: Linu Cherian 
> 
> Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
> and PAGE0_REGS_ONLY option will be enabled as an errata workaround.
> 
> This option when turned on, replaces all page 1 offsets used for
> EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.
> 
> Signed-off-by: Linu Cherian 
> Signed-off-by: Geetha Sowjanya 
> ---
>  drivers/iommu/arm-smmu-v3.c | 44 
>  1 file changed, 32 insertions(+), 12 deletions(-)

>  static struct arm_smmu_option_prop arm_smmu_options[] = {

> + { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium-cn99xx,broken-page1-regspace"},

No patch in this series documented the new property.

Please update Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt,
with a description of the property. i.e. what it describes, and when it
should be set.

Please either make that a prepatory path, or merge it in with this one.

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

Re: [PATCH 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74

[Mark Rutland]

On Thu, Apr 27, 2017 at 05:16:23PM +0530, Geetha sowjanya wrote:
> + /*
> +  * Override the size, for Cavium CN99xx implementations
> +  * which doesn't support the page 1 SMMU register space.
> +  */
> + cpu_model = read_cpuid_id() & MIDR_CPU_MODEL_MASK;
> + if (cpu_model == MIDR_THUNDERX_99XX ||
> + cpu_model == MIDR_BRCM_VULCAN)
> + size = SZ_64K;

If you're trying to identify an SMMU erratum, identify the SMMU, not the
CPU it happens to be paired with this time.

There are ID registers in the SMMU you can use to do so.

NAK to using the CPU ID here.

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

Re: [PATCH v2] iommu/arm-smmu: Add global SMR masking property

[Mark Rutland]

On Fri, Mar 31, 2017 at 12:03:33PM +0100, Robin Murphy wrote:
> The current SMR masking support using a 2-cell iommu-specifier is
> primarily intended to handle individual masters with large and/or
> complex Stream ID assignments; it quickly gets a bit clunky in other SMR
> use-cases where we just want to consistently mask out the same part of
> every Stream ID (e.g. for MMU-500 configurations where the appended TBU
> number gets in the way unnecessarily). Let's add a new property to allow
> a single global mask value to better fit the latter situation.
> 
> Tested-by: Nipun Gupta <nipun.gu...@nxp.com>
> Signed-off-by: Robin Murphy <robin.mur...@arm.com>
> ---
> 
> v2: Rewrite the documentation to actually make sense, and add an
> explicit example per Mark's suggestion.
> 
>  .../devicetree/bindings/iommu/arm,smmu.txt | 27 
> ++
>  drivers/iommu/arm-smmu.c   |  4 +++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 6cdf32d037fc..b744b231766f 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -60,6 +60,16 @@ conditions.
>aliases of secure registers have to be used during
>SMMU configuration.
>  
> +- stream-match-mask : For SMMUs supporting stream matching and using
> +  #iommu-cells = <1>, specifies a fixed value to set in
> +  the SMRn.MASK field of every stream match register
> +  used, for cases where it is desirable to ignore some
> +  portion of every Stream ID (e.g. for certain MMU-500
> +  configurations given globally unique input IDs). This
> +  property is not valid for SMMUs using stream indexing,
> +  or using stream matching with #iommu-cells = <2>, and
> +  may be ignored if present in such cases.

This generally sounds fine, but I'd like it to be a little more explicit
about what this means for matching. Can we change the first sentence to:

For SMMUs supporting stream matching and using
#iommu-cells = <1>, specifies a mask of bits to
ignore when matching stream IDs (e.g. this may be
programmed into the SMRn.MASK field of every
stream match register used). 


Otherwise, this looks good to me; thanks for respinning this.

If you're happy to make the above change:

Acked-by: Mark Rutland <mark.rutl...@arm.com>

Thanks,
Mark.

> +
>  ** Deprecated properties:
>  
>  - mmu-masters (deprecated in favour of the generic "iommus" binding) :
> @@ -109,3 +119,20 @@ conditions.
>  master3 {
>  iommus = < 1 0x30>;
>  };
> +
> +
> +/* ARM MMU-500 with 10-bit stream ID input configuration */
> +smmu3: iommu {
> +compatible = "arm,mmu-500", "arm,smmu-v2";
> +...
> +#iommu-cells = <1>;
> +/* always ignore appended 5-bit TBU number */
> +stream-match-mask = 0x7c00;
> +};
> +
> +bus {
> +/* bus whose child devices emit one unique 10-bit stream
> +   ID each, but may master through multiple SMMU TBUs */
> +iommu-map = <0  0 0x400>;
> +...
> +};
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..e394d55146a6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1590,13 +1590,15 @@ static int arm_smmu_domain_set_attr(struct 
> iommu_domain *domain,
>  
>  static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args 
> *args)
>  {
> - u32 fwid = 0;
> + u32 mask, fwid = 0;
>  
>   if (args->args_count > 0)
>   fwid |= (u16)args->args[0];
>  
>   if (args->args_count > 1)
>   fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> + else if (!of_property_read_u32(args->np, "stream-match-mask", ))
> + fwid |= (u16)mask << SMR_MASK_SHIFT;
>  
>   return iommu_fwspec_add_ids(dev, , 1);
>  }
> -- 
> 2.11.0.dirty
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu: Add global SMR masking property

[Mark Rutland]

On Tue, Mar 07, 2017 at 06:26:28PM +, Robin Murphy wrote:
> The current SMR masking support using a 2-cell iommu-specifier is
> primarily intended to handle individual masters with large and/or
> complex Stream ID assignments; it quickly gets a bit clunky in other SMR
> use-cases where we just want to consistently mask out the same part of
> every Stream ID (e.g. for MMU-500 configurations where the appended TBU
> number gets in the way unnecessarily). Let's add a new property to allow
> a single global mask value to better fit the latter situation.
> 
> Tested-by: Nipun Gupta 
> Signed-off-by: Robin Murphy 
> ---
> 
> Time to rekindle the discussion about whether an architecture-level
> concept with a rather specific name needs a vendor prefix ;)
> 
> Robin.
> 
>  Documentation/devicetree/bindings/iommu/arm,smmu.txt | 10 ++
>  drivers/iommu/arm-smmu.c |  4 +++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 6cdf32d037fc..d66f355e174f 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -60,6 +60,16 @@ conditions.
>aliases of secure registers have to be used during
>SMMU configuration.
>  
> +- stream-match-mask : Specifies a fixed SMR mask value to combine with
> +  the Stream ID value from every iommu-specifier. This
> +  may be used instead of an "#iommu-cells" value of 2
> +  when there is no need for per-master SMR masks, but
> +  it is still desired to mask some portion of every
> +  Stream ID (e.g. for certain MMU-500 configurations
> +  given globally unique external IDs). This property is
> +  not valid for SMMUs using stream indexing, and may be
> +  ignored if stream matching is not supported.
> +

Generally, this sounds fine.

However, there is an ambiguity here (inherited from the existing
#iommu-cells = <2> case), in that it's not clear what is meant by "to
combine".

>From the example at the end of the file, this appears to be a mask of
bits to not consider for matching purposes?

Would you mind clarifying that in both cases?

This could also do with an example.

Thanks,
Mark.

>  ** Deprecated properties:
>  
>  - mmu-masters (deprecated in favour of the generic "iommus" binding) :
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..e394d55146a6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1590,13 +1590,15 @@ static int arm_smmu_domain_set_attr(struct 
> iommu_domain *domain,
>  
>  static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args 
> *args)
>  {
> - u32 fwid = 0;
> + u32 mask, fwid = 0;
>  
>   if (args->args_count > 0)
>   fwid |= (u16)args->args[0];
>  
>   if (args->args_count > 1)
>   fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> + else if (!of_property_read_u32(args->np, "stream-match-mask", ))
> + fwid |= (u16)mask << SMR_MASK_SHIFT;
>  
>   return iommu_fwspec_add_ids(dev, , 1);
>  }
> -- 
> 2.11.0.dirty
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 7/7] iommu/arm-smmu: add support for dynamic domains

[Mark Rutland]

On Tue, Mar 07, 2017 at 09:39:55AM -0700, Jordan Crouse wrote:
> Implement support for dynamic domain switching. This feature is
> only enabled when the qcom,dynamic device tree attribute for an smmu
> instance.
> 
> In order to use dynamic domains, a non-dynamic domain must first
> be created and attached.  The non-dynamic domain must remain
> attached while the device is in use.
> 
> The dynamic domain is cloned from the non-dynamic domain. Important
> configuration information is copied from the non-dynamic domain and
> the dynamic domain is automatically "attached" (though it doesn't
> program the hardware).
> 
> To switch domains dynamically the hardware must program the TTBR0 register
> with the value from the DOMAIN_ATTR_TTBR0 attribute for the dynamic domain.
> The upstream driver may also need to do other hardware specific register
> programming to properly synchronize the domain switch. It must ensure that
> all register state except for the TTBR0 register is restored
> at the end of the switch operation.

> + { ARM_SMMU_OPT_DYNAMIC, "qcom,dynamic" },

What *precisely* is the intended semantic of this property?

It's not clear to me what a dynamic domain is, there's no documentation
in this series for this property, and from a glance it sounds like a
pure SW detail rather than a hardware/system detail (i.e. it shouldn;t
be in the DT at all).

This needs documentation. In future, please also Cc the devicetree list
(devicet...@vger.kernel.org) when adding new properties.

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

Re: [PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Mark Rutland]

On Wed, Feb 08, 2017 at 07:15:37PM +0530, Sricharan wrote:
> >Clocks are not architectural, so it only makes sense to associate them
> >with an implementation-specific compatible string. There's also no
> 
> ok, it for this the QCOM specific implementation binding is tried(going to).
> 
> >guarantee that different microarchitectures have equivalent internal
> >clock domains - I'm not sure if "the SMMU's underlying bus access" is
> >meant to refer to accesses *by* the SMMU, i.e. page table walks,
> >accesses *through* the SMMU by upstream masters, or both
> 
> In the above QCOM case, it is actually both. Its the same path for both the
> page table walker and upstream masters.
> 
> >differences are rather significant. I'd also note that an MMU-500
> >configuration may have up to *33* clocks.
> >
> >Either way, the QCOM implementation deserves its own compatible if only
> >for the sake of the imp-def gaps in the architecture (e.g. FSR.SS
> >behaviour WRT to IRQs as touched upon in the other thread).
> >
> 
> Ok, slightly unclear, so you mean then *clocks* are not good enough reason
> to have a new compatible ?

I beleive Robin's point was even if the clocks didn't matter, there are
other reasons we should have the QCOM-specific compatible string.

So we should have one regardless.

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

Re: [PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Mark Rutland]

On Wed, Feb 08, 2017 at 04:23:17PM +0530, Sricharan wrote:
> >On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
> >> +- clock-names:Should be a pair of "smmu_iface_clk" and "smmu_bus_clk"
> >> +  required for smmu's register group access and interface
> >> +  clk for the smmu's underlying bus access.
> >> +
> >> +- clocks: Phandles for respective clocks described by clock-names.
> >
> >Which SMMU implementations are those clock-names valid for?
> >
> >The SMMU architecture specifications do not architect the clocks, which
> >are implemementation-specific.
> >
> >AFAICT, this doesn't match MMU-400 or MMU-500.
> 
> Ok, should be more specific. Infact QCOM has MMU-500 and also
> a smmu v2 implementation which is fully compatible with
> "arm,smmu-v2", with the clocks being controlled by the soc's
> clock controller. i was trying to define these clock bindings
> so that its works across socs.

I don't think we can do that, if we don't know precisely what those
clocks are used for.

i.e. we'd need a compatible string for the QCOM SMMUv2 variant, which
would imply the set of clocks.

> So there are one or more interface clocks which are required for the
> smmu's interface or the configuration access and one or more clocks
> required for smmu's downstream bus access. That was the reason i was
> trying to iterate over the list of clocks down below.  But agree that
> the bindings should define each of the clocks required separately.

As above, I don't think the code should do this. It should only touch
the clocks it knows about.

> So one way here is, define a separate compatible for QCOM's SMMU
> implementation and define all the clock bindings as a part of it
> and handle it in the same way in the driver.

That would be my preference.

> But just thinking if it would scale well for any other soc that is
> compatible with arm,smmu-v2 driver and wants to handle clocks in the
> future ?

I don't think we can have our cake and eat it here. Either we handle the
clock management for each variant, or we don't do it at all. We have no
idea what requirements a future variant might have w.r.t. the management
of clocks we don't know about yet.

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

Re: [PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Mark Rutland]

Hi,

On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
> +- clock-names:Should be a pair of "smmu_iface_clk" and "smmu_bus_clk"
> +  required for smmu's register group access and interface
> +  clk for the smmu's underlying bus access.
> +
> +- clocks: Phandles for respective clocks described by clock-names.

Which SMMU implementations are those clock-names valid for?

The SMMU architecture specifications do not architect the clocks, which
are implemementation-specific.

AFAICT, this doesn't match MMU-400 or MMU-500.

> +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu)
> +{
> + const char *cname;
> + struct property *prop;
> + int i = 0;
> + struct device *dev = smmu->dev;
> +
> + smmu->num_clocks =
> + of_property_count_strings(dev->of_node, "clock-names");
> +
> + if (smmu->num_clocks < 1)
> + return 0;
> +
> + smmu->clocks = devm_kzalloc(dev,
> + sizeof(*smmu->clocks) * smmu->num_clocks,
> + GFP_KERNEL);
> +
> + if (!smmu->clocks) {
> + dev_err(dev, "Failed to allocate memory for clocks\n");
> + return -ENODEV;
> + }
> +
> + of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
> + struct clk *c = devm_clk_get(dev, cname);
> +
> + if (IS_ERR(c)) {
> + dev_err(dev, "Couldn't get clock: %s", cname);
> + return -EPROBE_DEFER;
> + }
> +
> + smmu->clocks[i] = c;
> + ++i;
> + }
> +
> + return 0;
> +}

I am very much not a fan of grabbing hold of resources that don't
necessarily match the binding, and we likely don't understand the use
of.

Either we know the names, and can manage them, or we don't, and cannot.

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

Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling

[Mark Rutland]

On Thu, Jan 05, 2017 at 02:00:05PM +, Will Deacon wrote:
> On Thu, Jan 05, 2017 at 12:08:57PM +0000, Mark Rutland wrote:
> > On Thu, Jan 05, 2017 at 11:55:29AM +, Will Deacon wrote:
> > > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote:
> > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> > > > b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > > > index ef465b0..5f405a6 100644
> > > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > > > @@ -68,6 +68,9 @@ conditions.
> > > >aliases of secure registers have to be used during
> > > >SMMU configuration.
> > > >  
> > > > +- arm,smmu-enable-stall : Enable stall mode to stall memory 
> > > > transactions
> > > > +  and resume after fault is handled
> > 
> > The wording here seems to describe a policy rather than a property.
> > 
> > Can you elaborate on when/why this is required/preferred/valid?
> 
> It's not a policy, it's a hardware capability. There are some non-probeable
> reasons why stalling mode is unsafe or unusable:
> 
>   
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474530.html

Ok. My point was that the wording above is an imperative -- it tells
the kernel to enable stall mode, not if/why it is safe to do so (i.e. it
is a policy, not a property).

It sounds like that's just a wording issue. Something like
"arm,stalling-is-usable" (along with a descrition of when that
can/should be in the DT) would be vastly better.

> Some of these are specific to the SMMU implementation (e.g. whether or not
> the SS bit can remain set without reasserting the IRQ) and some are specific
> to the integration (e.g. whether or not stalling an endpoint can deadlock
> the SoC). The key point is that, without support from both the
> implementation and the integration, stalls are unusable.

Ok.

> > > >  static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> > > > @@ -824,6 +852,8 @@ static void arm_smmu_init_context_bank(struct 
> > > > arm_smmu_domain *smmu_domain,
> > > >  
> > > > /* SCTLR */
> > > > reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> > > > +   if (smmu->options & ARM_SMMU_OPT_ENABLE_STALL)
> > > > +   reg |= SCTLR_CFCFG;
> > > 
> > > I wonder if this should also be predicated on the compatible string, so
> > > that the "arm,smmu-enable-stall" property is ignored (with a warning) if
> > > the compatible string isn't specific enough to identify an implementation
> > > with the required SS behaviour? On the other hand, it feels pretty
> > > redundant and a single "stalling works" property is all we need.
> > 
> > Can you elaborate on what "stalling works" entails? Is that just the SS
> > bit behaviour? are there integration or endpoint-specific things that we
> > need to care about?
> 
> See above. The "stalling works" property (arm,smmu-enable-stall) would
> indicate that both the implementation *and* the integration are such
> that stalling is usable for demand paging. I suspect there are endpoints
> that can't deal with stalls (e.g. they might timeout and signal a RAS
> event), but in that case their respective device drivers should ensure
> that any DMA buffers are pinned and/or register a fault handler to
> request termination of the faulting transaction.

Ok. It would be good to elaborate on what "stalling is useable" means in
the property description. i.e. what specificallty the implementation and
integration need to ensure.

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

Re: RFC: extend iommu-map binding to support #iommu-cells > 1

[Mark Rutland]

On Fri, Dec 16, 2016 at 02:36:57AM +, Stuart Yoder wrote:
> For context, please see the thread:
> https://www.spinics.net/lists/arm-kernel/msg539066.html
> 
> The existing iommu-map binding did not account for the situation where
> #iommu-cells == 2, as permitted in the ARM SMMU binding.  The 2nd cell
> of the IOMMU specifier being the SMR mask.  The existing binding defines
> the mapping as:
>Any RID r in the interval [rid-base, rid-base + length) is associated with
>the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> 
> ...and that does not work if iommu-base is 2 cells, the second being the
> SMR mask.
> 
> While this can be worked around by always having length=1, it seems we
> can get this cleaned up by updating the binding definition for iommu-map.

To reiterate, I'm very much not keen on the pci-iommu binding having
knowledge of the decomposition of iommu-specifiers from other bindings.

As mentioned previously, there's an intended interpretation [1] that we
need to fix up the pci-iommu binding with. With that, I don't think it's
even necessary to bodge iommu-cells = <1> AFAICT.

I'm sorry that the patch I suggested never materialized; let me figure
out the wording now...

Thanks,
Mark.

[1] https://www.spinics.net/lists/arm-kernel/msg539357.html

> 
> See patch below.  Thoughts?
> 
> Thanks,
> Stuart
> 
> -
> 
> diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt 
> b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> index 56c8296..e81b461 100644
> --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> @@ -38,8 +38,20 @@ Optional properties
>The property is an arbitrary number of tuples of
>(rid-base,iommu,iommu-base,length).
> 
> -  Any RID r in the interval [rid-base, rid-base + length) is associated with
> -  the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> +  If the associated IOMMU has an #iommu-cells value of 1, any RID r in the
> +  interval [rid-base, rid-base + length) is associated with the listed IOMMU,
> +  with the iommu-specifier (r - rid-base + iommu-base).
> +
> +  ARM SMMU Note:
> +The ARM SMMU binding permits an #iommu-cells value of 2 and in this
> +case defines an IOMMU specifier to be: (stream-id,smr-mask)
> +
> +In an iommu-map this means the iommu-base consists of 2 cells:
> +(rid-base,iommu,[stream-id,smr-mask],length).
> +
> +In this case the RID to IOMMU specifier mapping is defined to be:
> +any RID r in the interval [rid-base, rid-base + length) is associated
> +with the listed IOMMU, with the iommu-specifier (r - rid-base + 
> stream-id).
> 
>  - iommu-map-mask: A mask to be applied to each Requester ID prior to being
>mapped to an iommu-specifier per the iommu-map property.
> 
> 
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Docs: dt: Be explicit and consistent in reference to IOMMU specifiers

[Mark Rutland]

Hi Stuart,

On Thu, Dec 15, 2016 at 06:16:13PM -0600, Stuart Yoder wrote:
> The generic IOMMU binding says that the meaning of an 'IOMMU specifier'
> is defined by the binding of a specific SMMU.  The ARM SMMU binding
> never explicitly uses the term 'specifier' at all.  Update implicit
> references to use the explicit term.
> 
> In the iommu-map binding change references to iommu-specifier to
> "IOMMU specifier" so we are 100% consistent everywhere with terminology
> and capitalization.

Elsewhere, we always use lower case "xxx-specifier" or "xxx specifier",
e.g. Documentation/devicetree/bindings/gpio/gpio.txt defines
"gpio-specifier", ePAPR defines "interrupt specifier".

Given we're morstly consistent on "iommu-specifier" today, could we
please jsut update the ARM SMMU binding to match that? If we're going to
fix the dash mismatch, that's a more general, cross-binding thing.

Thanks,
Mark.

> Signed-off-by: Stuart Yoder 
> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu.txt | 10 +-
>  Documentation/devicetree/bindings/pci/pci-iommu.txt  |  6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index e862d148..6cdf32d 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -36,15 +36,15 @@ conditions.
>combined interrupt, it must be listed multiple times.
>  
>  - #iommu-cells  : See Documentation/devicetree/bindings/iommu/iommu.txt
> -  for details. With a value of 1, each "iommus" entry
> +  for details. With a value of 1, each IOMMU specifier
>represents a distinct stream ID emitted by that device
>into the relevant SMMU.
>  
>SMMUs with stream matching support and complex masters
> -  may use a value of 2, where the second cell represents
> -  an SMR mask to combine with the ID in the first cell.
> -  Care must be taken to ensure the set of matched IDs
> -  does not result in conflicts.
> +  may use a value of 2, where the second cell of the
> +  IOMMU specifier represents an SMR mask to combine with
> +  the ID in the first cell.  Care must be taken to ensure
> +  the set of matched IDs does not result in conflicts.
>  
>  ** System MMU optional properties:
>  
> diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt 
> b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> index 56c8296..0def586 100644
> --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> @@ -32,17 +32,17 @@ PCI root complex
>  Optional properties
>  ---
>  
> -- iommu-map: Maps a Requester ID to an IOMMU and associated iommu-specifier
> +- iommu-map: Maps a Requester ID to an IOMMU and associated IOMMU specifier
>data.
>  
>The property is an arbitrary number of tuples of
>(rid-base,iommu,iommu-base,length).
>  
>Any RID r in the interval [rid-base, rid-base + length) is associated with
> -  the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> +  the listed IOMMU, with the IOMMU specifier (r - rid-base + iommu-base).
>  
>  - iommu-map-mask: A mask to be applied to each Requester ID prior to being
> -  mapped to an iommu-specifier per the iommu-map property.
> +  mapped to an IOMMU specifier per the iommu-map property.
>  
>  
>  Example (1)
> -- 
> 1.9.0
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu: Fix out-of-bounds dereference

[Mark Rutland]

On Mon, Nov 07, 2016 at 03:39:02PM +, Robin Murphy wrote:
> When we iterate a master's config entries, what we generally care
> about is the entry's stream map index, rather than the entry index
> itself, so it's nice to have the iterator automatically assign the
> former from the latter. Unfortunately, booting with KASAN reveals
> the oversight that using a simple comma operator results in the
> entry index being dereferenced before being checked for validity,
> so we always access one element past the end of the fwspec array.
> 
> Flip things around so that the check always happens before the index
> may be dereferenced.
> 
> Fixes: adfec2e709d2 ("iommu/arm-smmu: Convert to iommu_fwspec")
> Reported-by: Mark Rutland <mark.rutl...@arm.com>
> Signed-off-by: Robin Murphy <robin.mur...@arm.com>

This patch fixes the KASAN splats as I saw (example below).

With this patch applied, my dmesg is free of errors.

So feel free to add:

Tested-by: Mark Rutland <mark.rutl...@arm.com>

Thanks,
Mark.

==
BUG: KASAN: slab-out-of-bounds in arm_smmu_add_device+0x510/0x960 at addr 
800935c6e72c
Read of size 2 by task swapper/0/1
CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc3-5-g8cc374c #79
Hardware name: ARM Juno development board (r1) (DT)
Call trace:
[] dump_backtrace+0x0/0x278
[] show_stack+0x14/0x20
[] dump_stack+0xa4/0xc8
[] kasan_object_err+0x24/0x80
[] kasan_report_error+0x208/0x4d0
[] kasan_report+0x40/0x48
[] __asan_load2+0x84/0x98
[] arm_smmu_add_device+0x510/0x960
[] iommu_bus_notifier+0x120/0x160
[] notifier_call_chain+0x6c/0xb8
[] __blocking_notifier_call_chain+0x5c/0xa0
[] blocking_notifier_call_chain+0x14/0x20
[] device_add+0x5c8/0x840
[] amba_device_try_add+0x1cc/0x298
[] amba_device_add+0x20/0x148
[] of_platform_bus_create+0x34c/0x428
[] of_platform_populate+0x4c/0xb8
[] of_platform_default_populate_init+0x78/0x8c
[] do_one_initcall+0x90/0x1c8
[] kernel_init_freeable+0x280/0x324
[] kernel_init+0x10/0x110
[] ret_from_fork+0x10/0x50
Object at 800935c6e700, in cache kmalloc-128 size: 128
Allocated:
PID = 1
 [] save_stack_trace_tsk+0x0/0x180
 [] save_stack_trace+0x28/0x38
 [] kasan_kmalloc+0xdc/0x188
 [] __kmalloc+0x148/0x238
 [] arm_smmu_add_device+0x1c8/0x960
 [] iommu_bus_notifier+0x120/0x160
 [] notifier_call_chain+0x6c/0xb8
 [] __blocking_notifier_call_chain+0x5c/0xa0
 [] blocking_notifier_call_chain+0x14/0x20
 [] device_add+0x5c8/0x840
 [] amba_device_try_add+0x1cc/0x298
 [] amba_device_add+0x20/0x148
 [] of_platform_bus_create+0x34c/0x428
 [] of_platform_populate+0x4c/0xb8
 [] of_platform_default_populate_init+0x78/0x8c
 [] do_one_initcall+0x90/0x1c8
 [] kernel_init_freeable+0x280/0x324
 [] kernel_init+0x10/0x110
 [] ret_from_fork+0x10/0x50
Freed:
PID = 0
(stack is not available)
Memory state around the buggy address:
 800935c6e600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 800935c6e680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>800935c6e700: 00 00 00 00 00 04 fc fc fc fc fc fc fc fc fc fc
  ^
 800935c6e780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 800935c6e800: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: SMR masking and PCI

[Mark Rutland]

Hi, 

On Fri, Oct 28, 2016 at 05:16:37PM +0100, Robin Murphy wrote:
> On 27/10/16 18:10, Stuart Yoder wrote:
> > A question about how the SMR masking defined in the arm,smmu binding
> > relates to the PCI iommu-map.
> > 
> > The #iommu-cells property defines the number of cells an "IOMMU specifier"
> > takes and 2 is specified to be:
> > 
> >SMMUs with stream matching support and complex masters
> >may use a value of 2, where the second cell represents
> >an SMR mask to combine with the ID in the first cell.
> > 
> > An iommu-map entry is defined as:
> > 
> >(rid-base,iommu,iommu-base,length)
> > 
> > What seems to be currently missing in the iommu-map support is
> > the possibility the case where #iommu-cells=<2>.
> 
> Indeed. The bindings have so far rather implicitly assumed the case of
> #{msi,iommu}-cells = 1, and the code has followed suit.

The intention was that neither the iommu or msi bindings had such a
requirement, but evidently I did not specify the intended behaviour
thoroughly enough.

I had intended that the offset was added to the final cell of the
iommu-specifier (i.e. that the iommu-specifier was treated as a large
number).

You can handle this case by adding additional entries in the map table,
with a single entry length.

> > In this case iommu-base which is an IOMMU specifier should
> > occupy 2 cells.  For example on an ls2085a we would want:
> > 
> > iommu-map = <0x0   0x6 0x7 0x3ff 0x1
> >0x100 0x6 0x8 0x3ff 0x1>;
> > 
> > ...to mask our stream IDs to 10 bits.
> > 
> > This should work in theory and comply with the bindings, no?
> 
> In theory, but now consider:
> 
>   iommu-map = <0x0 0x6 0x7 0x3ff 0x2>;
> 
> faced with ID 1. The input base is 0, the output base is the 2-cell
> value 0x703ff, so the final ID value should be 0x70400, right?

That was the intended behaviour, yes.

> > (Also, I guess that msi-map is not affected by this since it
> > is not related to the IOMMU...but we do have common code
> > handling both maps.)
> 
> I'd say it's definitely affected, since #msi-cells can equally be more
> than 1, and encodes an equally opaque value.

Yes.

> It seems pretty reasonable to me that we could extend the binding to
> accommodate #cells > 1 iff length == 1. Mark?

I will try to come up with the wording to make the above explicit, for
both bindings.

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

Re: [v8, 1/7] Documentation: DT: update Freescale DCFG compatible

[Mark Rutland]

On Fri, Apr 22, 2016 at 02:27:38PM +0800, Yangbo Lu wrote:
> Update Freescale DCFG compatible with 'fsl,-dcfg' instead
> of 'fsl,ls1021a-dcfg' to include more chips.
> 
> Signed-off-by: Yangbo Lu 
> ---
> Changes for v8:
>   - Added this patch
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.txt 
> b/Documentation/devicetree/bindings/arm/fsl.txt
> index 752a685..1d5f512 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.txt
> +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> @@ -119,7 +119,7 @@ Freescale DCFG
>  configuration and status for the device. Such as setting the secondary
>  core start address and release the secondary core from holdoff and startup.
>Required properties:
> -  - compatible: should be "fsl,ls1021a-dcfg"
> +  - compatible: should be "fsl,-dcfg"

Please list specific values expected for , while jusy saying
 may be more generic, it makes it practically impossible to search
for the correct binding given a compatible string, and it's vague as to
exaclty what  should be.

Thanks,
Mark.



>- reg : should contain base address and length of DCFG memory-mapped 
> registers
>  
>  Example:
> -- 
> 2.1.0.27.g96db324
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

[Mark Rutland]

On Wed, Feb 24, 2016 at 03:54:48PM +, Chalamarla, Tirumalesh wrote:
> 
> On 2/24/16, 3:32 AM, "Mark Rutland" <mark.rutl...@arm.com> wrote:
> 
> >On Tue, Feb 23, 2016 at 03:50:21PM -0800, Tirumalesh Chalamarla wrote:
> >> in Summary,
> >> 
> >> if i change asid-base to cavium,asid-base and still use DT for
> >> supplying base value, is this a solution that will be accepted, 
> >
> >No. The property is _insufficient_, regardless of its name. This has
> >been pointed out more than once.
> >
> >A base alone does not tell you what set of IDs it is valid to use
> >without risking a clash. The OS is well within its rights to allocate
> >_any_ ID above that base. It is not a requirement of the hardware nor
> >the binding that the OS allocate a contiguous set of IDs starting at the
> >base.
> >
> >Consider:
> >
> >smmu_a {
> > whatver,*id-base = <128>;
> >};
> >
> >smmu_b {
> > whatever,*id-base = <64>;
> >};
> >
> >In both cases, the *IDs 129+ could be allocated on both SMMUs.
> 
> Does adding a check to see if base + number of contexts supported will not 
> overlap with each other
> Make it acceptable. 
> Or do you want the size be provided from DT also?

At this point I think Robin's suggestion of giving the ThunderX SMMU a
different compatible string and treating that as a separate case
entirely is the best thing to do.

Then the only requirement is that _all_ the ThunderX SMMUs with shared
TLBs are under the control of the OS, and it can allocate IDs as it
chooses, so long as it ensures that there are no conflicts across all
the SMMUs it is in control of.

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

Re: [PATCH V2] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

[Mark Rutland]

On Tue, Feb 23, 2016 at 03:50:21PM -0800, Tirumalesh Chalamarla wrote:
> in Summary,
> 
> if i change asid-base to cavium,asid-base and still use DT for
> supplying base value, is this a solution that will be accepted, 

No. The property is _insufficient_, regardless of its name. This has
been pointed out more than once.

A base alone does not tell you what set of IDs it is valid to use
without risking a clash. The OS is well within its rights to allocate
_any_ ID above that base. It is not a requirement of the hardware nor
the binding that the OS allocate a contiguous set of IDs starting at the
base.

Consider:

smmu_a {
whatver,*id-base = <128>;
};

smmu_b {
whatever,*id-base = <64>;
};

In both cases, the *IDs 129+ could be allocated on both SMMUs.

Mark.

> of course i will do range check to see we are not supplying 16bit VMID
> for 8 bit systems even though the property now indicates Cavium only.
> 
> Thanks,
> Tirumalesh.
> 
> On 02/23/2016 04:26 AM, Mark Rutland wrote:
> >On Thu, Feb 18, 2016 at 10:29:18AM -0800, tchalama...@caviumnetworks.com 
> >wrote:
> >>From: Tirumalesh Chalamarla <tchalama...@caviumnetworks.com>
> >>
> >>Due to Errata#27704 CN88xx SMMUv2,supports  only shared ASID and VMID
> >>namespaces; specifically within a given node SMMU0 and SMMU1 share,
> >>as does SMMU2 and SMMU3.
> >>
> >>This patch tries to address these issuee by supplying asid and vmid
> >>base from devicetree.
> >>
> >>changes from V1:
> >>- rebased on top of 16 bit VMID patch
> >>- removed redundent options from DT
> >>- insted of transform, DT now supplies starting ASID/VMID
> >>
> >>Signed-off-by: Akula Geethasowjanya 
> >><geethasowjanya.ak...@caviumnetworks.com>
> >>Signed-off-by: Tirumalesh Chalamarla <tchalama...@caviumnetworks.com>
> >>---
> >>  .../devicetree/bindings/iommu/arm,smmu.txt |  8 +
> >>  drivers/iommu/arm-smmu.c   | 37 
> >> +++---
> >>  2 files changed, 34 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> >>b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >>index bb7e569..80b8484 100644
> >>--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >>+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >>@@ -57,6 +57,14 @@ conditions.
> >>
> >>  - smmu-enable-vmid16 : Enable 16 bit VMID, if allowed.
> >>
> >>+- asid-base :  Buggy SMMUv2 implementations which doesn't satisfy the
> >>+   ASID namespace needs, use this field to specify starting
> >>+   ASID for the SMMU.
> >>+
> >>+- vmid-base :  Buggy SMMUv2 implementations which doesn't satisfy the 
> >>VMID
> >>+   namespace needs, use this field to specify starting VMID
> >>+   for the SMMU.
> >
> >As has been pointed out, these are not strictly properties of the
> >hardware, and are insufficient to aovid the issue in general (adding an
> >arbitrary base does not enforce IDs fall within a particular range).
> >
> >So NAK for *-base properties alone.
> >
> >>+   if (of_property_read_u32(dev->of_node, "#asid-base",
> >>+>asid_base)) {
> >>+   smmu->asid_base = 0;
> >>+   }
> >>+
> >>+   if (of_property_read_u32(dev->of_node, "#vmid-base",
> >>+>vmid_base)) {
> >>+   smmu->vmid_base = 1;
> >>+   }
> >
> >These do not match the documentation above.
> >
> >Mark.
> >
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] iommu/arm-smmu: Add stub of_xlate() operation in SMMUv1/SMMUv2 driver

[Mark Rutland]

On Mon, Feb 08, 2016 at 10:47:32AM +0530, Anup Patel wrote:
> To allow use of large memory (> 4Gb) with 32bit devices we need to use
> IOMMU based DMA mappings for such 32bit devices. The IOMMU dt-bindings
> allows us do this by specifying 'iommus' attribute in 32bit device DT
> node. Unfortunately, specifying 'iommus' attribute does not work with
> current SMMUv1/SMMUv2 driver because it requires of_xlate() operation
> to be implemented by the driver.
> 
> This patch adds a stub implementation of of_xlate() in SMMUv1/SMMUv2
> driver to allow usage of 'iommus' attribute in DT for 32bit devices.
> 
> Signed-off-by: Anup Patel 
> Reviewed-by: Ray Jui 
> Reviewed-by: Scott Branden 
> ---
>  drivers/iommu/arm-smmu.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 02cd67d..8e090d8 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1398,6 +1398,16 @@ static int arm_smmu_init_platform_device(struct device 
> *dev,
>   return 0;
>  }
>  
> +int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> + /*
> +  * Nothing to do here because SMMU is already aware of all
> +  * MMU masters and their stream IDs using mmu-master attibute
> +  * SMMU DT node.
> +  */
> + return 0;
> +}

NAK to this.

As previously mentioned by others [1], this is an abuse of the generic
iommu binding support code.

The SMMU binding currently does not define its implementation of the
generic IOMMU binding. This series did not define what an SMMU's
#iommu-cells would be nor what the contained values would represent.
Therefore it is not valid to use an SMMU node with an iommus property as
the SMMu doesn't follwo the generic IOMMU binding.

There is ongoing work to have generic iommu binding support for the
SMMU. In the absence of documentation for what this means for the
binding, I am worried that this hack harms that effort.

To use features of the generic IOMMU binding, we must properly implement
the generic IOMMU binding for the SMMU rather than bodging it onto the
side of the existing binding.

Thanks,
Mark.

> +
>  static int arm_smmu_add_device(struct device *dev)
>  {
>   struct iommu_group *group;
> @@ -1495,6 +1505,7 @@ static struct iommu_ops arm_smmu_ops = {
>   .unmap  = arm_smmu_unmap,
>   .map_sg = default_iommu_map_sg,
>   .iova_to_phys   = arm_smmu_iova_to_phys,
> + .of_xlate   = arm_smmu_of_xlate,
>   .add_device = arm_smmu_add_device,
>   .remove_device  = arm_smmu_remove_device,
>   .device_group   = arm_smmu_device_group,
> -- 
> 1.9.1
> 

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/402976.html
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] iommu/arm-smmu: Add stub of_xlate() operation in SMMUv1/SMMUv2 driver

[Mark Rutland]

On Mon, Feb 08, 2016 at 06:13:11PM +0530, Anup Patel wrote:
> On Mon, Feb 8, 2016 at 4:48 PM, Mark Rutland <mark.rutl...@arm.com> wrote:
> > On Mon, Feb 08, 2016 at 10:47:32AM +0530, Anup Patel wrote:
> >> To allow use of large memory (> 4Gb) with 32bit devices we need to use
> >> IOMMU based DMA mappings for such 32bit devices. The IOMMU dt-bindings
> >> allows us do this by specifying 'iommus' attribute in 32bit device DT
> >> node. Unfortunately, specifying 'iommus' attribute does not work with
> >> current SMMUv1/SMMUv2 driver because it requires of_xlate() operation
> >> to be implemented by the driver.
> >>
> >> This patch adds a stub implementation of of_xlate() in SMMUv1/SMMUv2
> >> driver to allow usage of 'iommus' attribute in DT for 32bit devices.
> >>
> >> Signed-off-by: Anup Patel <anup.pa...@broadcom.com>
> >> Reviewed-by: Ray Jui <r...@broadcom.com>
> >> Reviewed-by: Scott Branden <sbran...@broadcom.com>
> >> ---
> >>  drivers/iommu/arm-smmu.c | 11 +++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 02cd67d..8e090d8 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -1398,6 +1398,16 @@ static int arm_smmu_init_platform_device(struct 
> >> device *dev,
> >>   return 0;
> >>  }
> >>
> >> +int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> >> +{
> >> + /*
> >> +  * Nothing to do here because SMMU is already aware of all
> >> +  * MMU masters and their stream IDs using mmu-master attibute
> >> +  * SMMU DT node.
> >> +  */
> >> + return 0;
> >> +}
> >
> > NAK to this.
> 
> I had no intention in continuing this change if I knew some work
> on generic IOMMU binding was in-progress. In fact, I had asked
> about alternate options previously. (Refer,
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/403128.html)

Ok.

To be clear, I expect the generic binding alone to be used for the case
you described, regardless of who implements that or how long it takes to
appear.

> > As previously mentioned by others [1], this is an abuse of the generic
> > iommu binding support code.
> >
> > The SMMU binding currently does not define its implementation of the
> > generic IOMMU binding. This series did not define what an SMMU's
> > #iommu-cells would be nor what the contained values would represent.
> > Therefore it is not valid to use an SMMU node with an iommus property as
> > the SMMu doesn't follwo the generic IOMMU binding.
> >
> > There is ongoing work to have generic iommu binding support for the
> > SMMU. In the absence of documentation for what this means for the
> > binding, I am worried that this hack harms that effort.
> 
> Thanks for the info, I would like to try this on Broadcom SoCs.
> 
> Whats the ETA of patches for generic IOMMU binding for SMMU?

That I do not know.

Robin, what's the state of the generic IOMMU binding support?

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

Re: [PATCH] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

[Mark Rutland]

On Fri, Feb 05, 2016 at 08:02:09PM +, Mark Rutland wrote:
> On Fri, Feb 05, 2016 at 10:47:07AM -0800, tchalama...@caviumnetworks.com 
> wrote:
> > From: Tirumalesh Chalamarla <tchalama...@caviumnetworks.com>
> > 
> > An issue exists whereby the Linux kernel requires that ASIDs are a
> > unique namespace per SMMU.  
> 
> The SMMU architecture requires this, and Linux relies on hardware
> following the architecture.
> 
> Please describe the problem correctly. i.e. that the hardware
> erroneously shares TLBs between SMMU instances.
> 
> > That ASID-local choice conflicts with the
> > CN88xx SMMU, where only shared ASID namespaces are supported;
> > specifically within a given node SMMU0 and SMMU1 share, as does SMMU2
> > and SMMU3. CN88xx SMMU also requires global VMIDs.
> > 
> > This patch tries to address these issuee by supplying asid and vmid
> > transformations from devicetree.
> > 
> > Signed-off-by: Akula Geethasowjanya 
> > <geethasowjanya.ak...@caviumnetworks.com>
> > Signed-off-by: Tirumalesh Chalamarla <tchalama...@caviumnetworks.com>
> > ---
> >  .../devicetree/bindings/iommu/arm,smmu.txt | 16 ++
> >  drivers/iommu/arm-smmu.c   | 59 
> > ++
> >  2 files changed, 65 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> > b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > index 7180745..eef06d0 100644
> > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > @@ -55,6 +55,22 @@ conditions.
> >aliases of secure registers have to be used during
> >SMMU configuration.
> >  
> > +- thunderx,smmu-asid-transform : Enable proper handling for buggy
> > + implementations that require special transformations
> > + for smmu asid. if this property exists asid-transform
> > + property must be present.
> > +
> > +- thunderx,smmu-vmid-transform : Enable proper handling for buggy
> > + implementations that require special transformations
> > + for smmu vmid. if this property exists vmid-transform
> > + property must be present.
> > +
> > +- asid-transform   : Transform mask that needs to be applied to asid.
> > +   This property has to be specified as '/bits/ 8' value.
> > +
> > +- vmid-transform   : Transform mask that needs to be applied to vmid.
> > +   This property has to be specified as '/bits/ 8' value.
> 
> Don't bother with /bits/ 8. It saves no space in the DTB and only adds
> to confusion. Use a full cell, and of_property_read_u32, and validate
> that the value is in range.
> 
> Having two properties for each seems redundant. The presence of a mask
> should be sufficient to make it clear that the use of the mask is
> required.
> 
> These properties aren't sufficiently described. How is the mask
> "applied", and what is it applied to? What constraints does it impose
> on the OS's selection of ASIDs/VMIDs?
> 
> From the looks of the patch, it's ANDed into the value Linux has
> selected, so it sounds like an OS must avoid using any bits set in any
> mask on any SMMU instance. That sounds very difficult to ensure in
> general, and very fragile.

A much better approach would be to have something like:

valid-asid-range : two u32 cells describing the minimum and maximum
   ASIDs that may be used on this SMMU instance. The OS
   must allocate ASIDs in the interval [minimum,
   maximum].

That way the OS can prevent itself from allocating conflicting ASIDs,
which is not possible with a single base value as you have with your
asid-transform property.

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

Re: [PATCH] iommu/arm-smmu-v2: Workaround for ThunderX errata#27704

[Mark Rutland]

On Fri, Feb 05, 2016 at 10:47:07AM -0800, tchalama...@caviumnetworks.com wrote:
> From: Tirumalesh Chalamarla 
> 
> An issue exists whereby the Linux kernel requires that ASIDs are a
> unique namespace per SMMU.  

The SMMU architecture requires this, and Linux relies on hardware
following the architecture.

Please describe the problem correctly. i.e. that the hardware
erroneously shares TLBs between SMMU instances.

> That ASID-local choice conflicts with the
> CN88xx SMMU, where only shared ASID namespaces are supported;
> specifically within a given node SMMU0 and SMMU1 share, as does SMMU2
> and SMMU3. CN88xx SMMU also requires global VMIDs.
> 
> This patch tries to address these issuee by supplying asid and vmid
> transformations from devicetree.
> 
> Signed-off-by: Akula Geethasowjanya 
> Signed-off-by: Tirumalesh Chalamarla 
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt | 16 ++
>  drivers/iommu/arm-smmu.c   | 59 
> ++
>  2 files changed, 65 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 7180745..eef06d0 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -55,6 +55,22 @@ conditions.
>aliases of secure registers have to be used during
>SMMU configuration.
>  
> +- thunderx,smmu-asid-transform   : Enable proper handling for buggy
> +   implementations that require special transformations
> +   for smmu asid. if this property exists asid-transform
> +   property must be present.
> +
> +- thunderx,smmu-vmid-transform   : Enable proper handling for buggy
> +   implementations that require special transformations
> +   for smmu vmid. if this property exists vmid-transform
> +   property must be present.
> +
> +- asid-transform : Transform mask that needs to be applied to asid.
> + This property has to be specified as '/bits/ 8' value.
> +
> +- vmid-transform : Transform mask that needs to be applied to vmid.
> + This property has to be specified as '/bits/ 8' value.

Don't bother with /bits/ 8. It saves no space in the DTB and only adds
to confusion. Use a full cell, and of_property_read_u32, and validate
that the value is in range.

Having two properties for each seems redundant. The presence of a mask
should be sufficient to make it clear that the use of the mask is
required.

These properties aren't sufficiently described. How is the mask
"applied", and what is it applied to? What constraints does it impose
on the OS's selection of ASIDs/VMIDs?

>From the looks of the patch, it's ANDed into the value Linux has
selected, so it sounds like an OS must avoid using any bits set in any
mask on any SMMU instance. That sounds very difficult to ensure in
general, and very fragile.

Mark.

> +
>  Example:
>  
>  smmu {
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..8047834 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -300,8 +300,12 @@ struct arm_smmu_device {
>   u32 features;
>  
>  #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
> +#define ARM_SMMU_OPT_TRANSFORM_ASID  (1 << 1)
> +#define ARM_SMMU_OPT_TRANSFORM_VMID  (1 << 2)
>   u32 options;
>   enum arm_smmu_arch_version  version;
> + u8  asid_transform;
> + u8  vmid_transform;
>  
>   u32 num_context_banks;
>   u32 num_s2_context_banks;
> @@ -330,8 +334,25 @@ struct arm_smmu_cfg {
>  };
>  #define INVALID_IRPTNDX  0xff
>  
> -#define ARM_SMMU_CB_ASID(cfg)((cfg)->cbndx)
> -#define ARM_SMMU_CB_VMID(cfg)((cfg)->cbndx + 1)
> +/*
> + * ThunderX has a unique requirement because of ERRATA#27704
> + * An issue exists whereby the Linux kernel requires that ASIDs are a
> + * unique namespace per SMMU.  That ASID-local choice conflicts with the
> + * CN88xx SMMU, where only shared ASID namespaces are supported;
> + * specifically within a given node SMMU0 and SMMU1 share, as does SMMU2
> + * and SMMU3.
> + * CN88xx SMMU also requires global VMIDs.
> + */
> +
> +#define ARM_SMMU_CB_ASID(smmu, cfg)  \
> + (((smmu)->options & ARM_SMMU_OPT_TRANSFORM_ASID)\
> + ? ((cfg)->cbndx | (smmu)->asid_transform)   \
> + : ((cfg)->cbndx))
> +
> +#define ARM_SMMU_CB_VMID(smmu, cfg)  \
> + 

Re: [RFC PATCH 4/6] iommu/arm-smmu: Add support for IOMMU_DOMAIN_DMA in SMMUv1/SMMUv2 driver

[Mark Rutland]

On Thu, Jan 28, 2016 at 05:28:30PM +, Robin Murphy wrote:
> On 27/01/16 05:21, Anup Patel wrote:
> >To allow use of large memory (> 4Gb) with 32bit devices we need to use
> >some kind of iommu for such 32bit devices.
> >
> >This patch extends SMMUv1/SMMUv2 driver to support DMA domains which
> >in-turn will allows us to use iommu based DMA mappings for 32bit devices.
> >
> >Signed-off-by: Anup Patel 
> >Reviewed-by: Ray Jui 
> >Reviewed-by: Scott Branden 
> >---
> >  drivers/iommu/arm-smmu.c | 21 -
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >index 9bdf6b2..43424fe 100644
> >--- a/drivers/iommu/arm-smmu.c
> >+++ b/drivers/iommu/arm-smmu.c
> >@@ -29,6 +29,7 @@
> >  #define pr_fmt(fmt) "arm-smmu: " fmt
> >
> >  #include 
> >+#include 
> >  #include 
> >  #include 
> >  #include 
> >@@ -967,7 +968,7 @@ static struct iommu_domain 
> >*arm_smmu_domain_alloc(unsigned type)
> >  {
> > struct arm_smmu_domain *smmu_domain;
> >
> >-if (type != IOMMU_DOMAIN_UNMANAGED)
> >+if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
> > return NULL;
> > /*
> >  * Allocate the domain and initialise some of its data structures.
> >@@ -978,6 +979,12 @@ static struct iommu_domain 
> >*arm_smmu_domain_alloc(unsigned type)
> > if (!smmu_domain)
> > return NULL;
> >
> >+if (type == IOMMU_DOMAIN_DMA &&
> >+iommu_get_dma_cookie(_domain->domain)) {
> >+kfree(smmu_domain);
> >+return NULL;
> >+}
> >+
> > mutex_init(_domain->init_mutex);
> > spin_lock_init(_domain->pgtbl_lock);
> >
> >@@ -992,6 +999,7 @@ static void arm_smmu_domain_free(struct iommu_domain 
> >*domain)
> >  * Free the domain resources. We assume that all devices have
> >  * already been detached.
> >  */
> >+iommu_put_dma_cookie(domain);
> > arm_smmu_destroy_domain_context(domain);
> > kfree(smmu_domain);
> >  }
> >@@ -1361,6 +1369,16 @@ static int arm_smmu_init_platform_device(struct 
> >device *dev,
> > return 0;
> >  }
> >
> >+int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> >+{
> >+/*
> >+ * Nothing to do here because SMMU is already aware of all
> >+ * MMU masters and their stream IDs using mmu-master attibute
> >+ * SMMU DT node.
> >+ */
> 
> ...but on the same hand this will also never get called if there's
> no "iommus" property on the master. Maintaining support for existing
> users of the "mmu-masters" binding is one thing (namely the thing
> that's been slowing down my efforts to clean up the really hacky
> generic binding support I did all the DMA stuff with), but having
> _both_ bindings in a single DT is something I don't think anybody
> wants to see

Indeed. NAK to the mixed case.

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

Re: [RFC PATCH 6/6] iommu/arm-smmu: Update bindings document for smmu-inst-as-data DT option

[Mark Rutland]

On Wed, Jan 27, 2016 at 10:51:19AM +0530, Anup Patel wrote:
> This patch adds info about 'smmu-inst-as-data' DT option in ARM
> SMMUv1/SMMUv2 driver bindings document.
> 
> Signed-off-by: Anup Patel 
> Reviewed-by: Ray Jui 
> Reviewed-by: Vikram Prakash 
> Reviewed-by: Scott Branden 
> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu.txt | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 7180745..4c4d03e 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -49,6 +49,14 @@ conditions.
>NOTE: this only applies to the SMMU itself, not
>masters connected upstream of the SMMU.
>  
> +- smmu-inst-as-data : Treat privilege/unprivilege instruction fetch as
> +  data read for SMMUv2. The SMMU driver by default provides
> +  unprivilege read-write permission in page table entries.
> +  For SMMUv2, privilege instruction fetch from MMU masters
> +  will cause a context fault for unprivilege read-write
> +  pages. To allow both privilege and unprivilege instruction
> +  fetch, we have to forcefully treat it as data read.

What is this needed for? Which masters do instruction fetches through
the SMMU, and when?

Surely this should only need to aplly to a subset of transactions?

Mark.

> +
>  - calxeda,smmu-secure-config-access : Enable proper handling of buggy
>implementations that always use secure access to
>SMMU configuration registers. In this case non-secure
> -- 
> 1.9.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v5 0/3] vfio: platform: return device properties for a platform device

[Mark Rutland]

On Mon, Oct 05, 2015 at 12:32:00PM +0200, Christoffer Dall wrote:
> [cc'ing Mark R. and Shannon for their input on FDT and ACPI].
> 
> On Mon, Oct 05, 2015 at 11:07:35AM +0100, Peter Maydell wrote:
> > On 5 October 2015 at 10:37, Christoffer Dall
> >  wrote:
> > > On Fri, Oct 02, 2015 at 11:53:07PM +0100, Peter Maydell wrote:
> > >>  * I don't really want to build in a lot of infrastructure into
> > >>QEMU to either build the DTC compiler into it or else introduce
> > >>a runtime dependency on the dtc binary
> > >
> > > what level of complexity are we really talking about here?  Doesn't QEMU
> > > already link against libfdt and doesn't it support exactly what we're
> > > trying to do here?
> > 
> > We link against libfdt, but libfdt is for manipulating and creating
> > dt binary blobs. As I understand it what sysfs exposes to userspace
> > is not a dt binary blob (or fragment thereof) but a bit of dt source.
> > libfdt doesn't do parsing of source into blobs; that is done by the
> > dtc compiler, which QEMU doesn't currently build and which is set
> > up to build only a separate command line binary anyway, not to be
> > a utility library for compiling dt sources.
> > 
> > Do correct me if I'm wrong about the sysfs interface -- if it is
> > just exposing dt blobs that would be easier to deal with.
> > 
> 
> I thought there was also /sys/firmware/fdt being the unflattened one
> accessible by libfdt, but dtc -I dtb seems to be unhappy parsin this on
> my Mustang.
> 
> Mark, can you refresh my memory about this?

/sys/firmware/fdt is the original, unadulterated DTB passed to the
kernel, while /sys/firmware/device-tree is the unflattened form which
may have had overlays (or perhaps other fixups) applied.

For runtime stuff I'd generally expect /sys/firmware/device-tree to be
preferable (e.g. if we want to expose some device added by an overlay).
We added /sys/firmware/fdt for kexec, as memory reservations are in the
DTB header, which doesn't show up in the unflattened tree.

> > > I also don't think this is the job of VFIO, assuming there is some
> > > better place to do this.  I initially thought exposing device properties
> > > in some canonical format from sysfs independently from whether the
> > > system was booted with ACPI or DT was the right thing to do, but the
> > > counter argument to this was essentially that the guest kernel needs the
> > > same description as the host kernel and therefore we really have to find
> > > a way to pass the HW description bits on to the guest.
> > 
> > So we end up with even more code to pass through ACPI table
> > fragments as well :-(
> > 
> 
> Probaby, yes.  But I think it's even worse, because there's no guarantee
> you can get at the original firmware data, because it's all from the
> DSDT so invoking the initial 'probe' method of the device entry could
> theoretically overwrite itself...  (If I remember the details
> correctly).

I don't think you can realistically expose host AML to the guest. Even
if it doesn't overwrite itself, it could perform oneshot configuration
that breaks if executed repeatedly, try to access otehr bits of ACPI
which may or may not be present, call to secure firmware, etc.

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

Re: [PATCH 2/3] Docs: dt: Add PCI MSI map bindings

[Mark Rutland]

> > +PCI root complex
> > +
> > +
> > +Optional properties
> > +---
> > +
> > +- msi-map: Maps a Requester ID to an MSI controller and associated
> > +  msi-specifier data. The property is an arbitrary number of tuples of
> > +  (rid-base,msi-controller,msi-base,length), where:
> > +
> > +  * rid-base is a single cell describing the first RID matched by the 
> > entry.
> > +
> > +  * msi-controller is a single phandle to an MSI controller
> > +
> > +  * msi-base is an msi-specifier describing the msi-specifier produced for 
> > the
> > +first RID matched by the entry.
> > +
> > +  * length is a single cell describing how many consecutive RIDs are 
> > matched
> > +following the rid-base.
> > +
> > +  Any RID r in the interval [rid-base, rid-base + length) is associated 
> > with
> > +  the listed msi-controller, with the msi-specifier (r - rid-base + 
> > msi-base).
> > +
> > +- msi-map-mask: A mask to be applied to each Requester ID prior to being 
> > mapped
> > +  to an msi-specifier per the msi-map property.
> > +
> > +- msi-parent: Describes the MSI parent of the root complex itself. Where
> > +  the root complex and MSI controller do not pass sideband data with MSI
> > +  writes, this property may be used to describe the MSI controller(s)
> > +  used by PCI devices under the root complex, if defined as such in the
> > +  binding for the root complex.
> 
> Right, this is where I'd expect some details about #msi-cells. Is it
> meant to be ignored? The lack of symmetry between the PCI and non-PCI
> use cases feels a bit inelegant (not to mention that it precludes having
> an unified parser for both cases).

I would expect that the msi-parent's #msi-cells could be non-zero.

As I see it, there are three possible interpretations (and the choice
between these would be specific to a PCI root complex):

(1) Devices under the root complex generate MSIs via the MSI parent,
using common sideband data described in the msi cells (and therefore
cannot be distingushed from each other).

(2) The root complex itself generates MSIs via the MSI parent, with any
sideband data described in the msi cells.

(3) Both the root complex and devices underneath it generate MSIs via
the MSI parent, using common sideband data described in the msi
cells (and therefore cannot be distinguished from each other).

Of these, (1) is the common case today, though (2) would be more in
keeping with how this style of property usually works.

I'm not sure how to capture that.

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

Re: [PATCH 2/3] Docs: dt: Add PCI MSI map bindings

[Mark Rutland]

On Fri, Sep 04, 2015 at 11:33:35PM +0100, David Daney wrote:
> Hi Mark,

Hi David,

> I now have a prototype implementation for irq-gic-v3-its.c that is using 
> this binding on Cavium's ThunderX platform.
> 
> Q: Have you guys had any more thoughts on this that might require 
> changing the binding?

Having discussed this with Stuart and others at Linux Plumbers, I think
that the binding is sufficient, and unlikely to change greatly unless
there is a strong objection (Stuart, please correct me if I am wrong).

Per Marc's comments there are probably some edge cases and/or wording
details to sort out, but I think the common/simple case is sorted. I'll
send a v2 once those have been settled.

> If not, I will be sending out my patches for your consideration.

Please do!

> Thanks,
> David Daney
> 
> On 07/27/2015 01:16 AM, Marc Zyngier wrote:
> > On 23/07/15 17:52, Mark Rutland wrote:
> >> Currently msi-parent is used by a few bindings to describe the
> >> relationship between a PCI root complex and a single MSI controller, but
> >> this property does not have a generic binding document.
> >>
> >> Additionally, msi-parent is insufficient to describe more complex
> >> relationships between MSI controllers and devices under a root complex,
> >> where devices may be able to target multiple MSI controllers, or where
> >> MSI controllers use (non-probeable) sideband information to distinguish
> >> devices.
> >>
> >> This patch adds a generic binding for mapping PCI devices to MSI
> >> controllers. This document covers msi-parent, and a new msi-map property
> >> (specific to PCI*) which may be used to map devices (identified by their
> >> Requester ID) to sideband data for each MSI controller that they may
> >> target.
> >>
> >> Signed-off-by: Mark Rutland <mark.rutl...@arm.com>
> 
> Acked-by: David Daney <david.da...@cavium.com>

Thanks!

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

Re: [PATCH 1/3] Docs: dt: add generic MSI bindings

[Mark Rutland]

On Wed, Aug 05, 2015 at 05:51:20PM +0100, Mark Rutland wrote:
 Rob,
 
 Do you have any objections to this, or are you happy to take this patch?
 
 There's a user of this binding (the GICv3 ITS) queued for v4.3 already in
 the tip tree, so either we either need to be ok with this binding or we
 need to rework that before v4.3.

Sorry to ping, but are you happy to take this? Marc's replied and
provided his ack.

I'm happy to resend if you prefer?

Thanks,
Mark.

 Marc, can I take it from your use of the binding that you are happy to
 provide your ack?
 
 Thanks,
 Mark.
 
 On Thu, Jul 23, 2015 at 05:52:43PM +0100, Mark Rutland wrote:
  Currently msi-parent is used in a couple of drivers despite being fairly
  underspecified. This patch adds a generic binding for MSIs (including
  the existing msi-parent property) enabling the description of platform
  devices capable of using MSIs.
  
  While MSIs are primarily distinguished by doorbell and payload, some MSI
  controllers (e.g. the GICv3 ITS) also use side-band information
  accompanying the write to identify the master which originated the MSI,
  to allow for sandboxing. This sideband information is non-probeable and
  needs to be described in the DT. Other MSI controllers may have
  additional configuration details which need to be described per-master.
  
  This patch adds a generic msi-parent binding document, extending the
  de-facto standard with a new (optional) #msi-cells which can be used to
  express any per-master configuration and/or sideband data. This is
  sufficient to describe non-hotpluggable devices.
  
  For busses where sideband data may be derived from some bus-specific
  master ID scheme, other properties will be required to describe the
  mapping.
  
  Signed-off-by: Mark Rutland mark.rutl...@arm.com
  ---
   .../bindings/interrupt-controller/msi.txt  | 135 
  +
   1 file changed, 135 insertions(+)
   create mode 100644 
  Documentation/devicetree/bindings/interrupt-controller/msi.txt
  
  diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi.txt 
  b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
  new file mode 100644
  index 000..c60c034
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
  @@ -0,0 +1,135 @@
  +This document describes the generic device tree binding for MSI 
  controllers and
  +their master(s).
  +
  +Message Signaled Interrupts (MSIs) are a class of interrupts generated by a
  +write to an MMIO address.
  +
  +MSIs were originally specified by PCI (and are used with PCIe), but may 
  also be
  +used with other busses, and hence a mechanism is required to relate 
  devices on
  +those busses to the MSI controllers which they are capable of using,
  +potentially including additional information.
  +
  +MSIs are distinguished by some combination of:
  +
  +- The doorbell (the MMIO address written to).
  +  
  +  Devices may be configured by software to write to arbitrary doorbells 
  which
  +  they can address. An MSI controller may feature a number of doorbells.
  +
  +- The payload (the value written to the doorbell).
  +  
  +  Devices may be configured to write an arbitrary payload chosen by 
  software.
  +  MSI controllers may have restrictions on permitted payloads.
  +
  +- Sideband information accompanying the write.
  +  
  +  Typically this is neither configurable nor probeable, and depends on the 
  path
  +  taken through the memory system (i.e. it is a property of the 
  combination of
  +  MSI controller and device rather than a property of either in isolation).
  +
  +
  +MSI controllers:
  +
  +
  +An MSI controller signals interrupts to a CPU when a write is made to an 
  MMIO
  +address by some master. An MSI controller may feature a number of 
  doorbells.
  +
  +Required properties:
  +
  +
  +- msi-controller: Identifies the node as an MSI controller.
  +
  +Optional properties:
  +
  +
  +- #msi-cells: The number of cells in an msi-specifier, required if not 
  zero.
  +
  +  Typically this will encode information related to sideband data, and will
  +  not encode doorbells or payloads as these can be configured dynamically.
  +
  +  The meaning of the msi-specifier is defined by the device tree binding of
  +  the specific MSI controller. 
  +
  +
  +MSI clients
  +===
  +
  +MSI clients are devices which generate MSIs. For each MSI they wish to
  +generate, the doorbell and payload may be configured, though sideband
  +information may not be configurable.
  +
  +Required properties:
  +
  +
  +- msi-parent: A list of phandle + msi-specifier pairs, one for each MSI
  +  controller which the device is capable of using.
  +
  +  This property is unordered, and MSIs may be allocated from any 
  combination of
  +  MSI controllers listed in the msi-parent property.
  +
  +  If a device has restrictions

Re: [PATCH 2/3] Docs: dt: Add PCI MSI map bindings

[Mark Rutland]

[...]

  +PCI root complex
  +
  +
  +Optional properties
  +---
  +
  +- msi-map: Maps a Requester ID to an MSI controller and associated
  +  msi-specifier data. The property is an arbitrary number of tuples of
  +  (rid-base,msi-controller,msi-base,length), where:
  +
  +  * rid-base is a single cell describing the first RID matched by the 
  entry.
  +
  +  * msi-controller is a single phandle to an MSI controller
  +
  +  * msi-base is an msi-specifier describing the msi-specifier produced for 
  the
  +first RID matched by the entry.
  +
  +  * length is a single cell describing how many consecutive RIDs are 
  matched
  +following the rid-base.
  +
  +  Any RID r in the interval [rid-base, rid-base + length) is associated 
  with
  +  the listed msi-controller, with the msi-specifier (r - rid-base + 
  msi-base).
  +
  +- msi-map-mask: A mask to be applied to each Requester ID prior to being 
  mapped
  +  to an msi-specifier per the msi-map property.
 
 Can we extend the msi-map-mask definition to say:  A mask value of 0x0 is 
 valid
 and indicates that no RIDs are _currently_ mapped to any msi-specifier.

That would break a valid case of the mask being all zeroes.

Consider the case that all RIDs get mapped to a single msi-specifier;
the obvious way to write that is:

msi-map-mask = 0x;
msi-map = 0x msi (msi-specifier) 1;

In this case all RIDS are always mapped to the single msi-specifier.

 We have an SoC with a programmable hardware table in the PCI controller that 
 maps
 requester ID to stream ID, so the overall msi-map (and iommu-map) definition 
 fit
 into that scheme.  But, we would like to be able make the RID-stream-ID 
 mapping
 decision _lazily_, in Linux, based on actual usage of PCI devices.

Dynamically programming the mapping is at odds to this binding. I don't
see how that can fit.

Why can the RID-SID mapping not be statically configured prior to
entering the OS?

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

Re: [PATCH 1/3] Docs: dt: add generic MSI bindings

[Mark Rutland]

Rob,

Do you have any objections to this, or are you happy to take this patch?

There's a user of this binding (the GICv3 ITS) queued for v4.3 already in
the tip tree, so either we either need to be ok with this binding or we
need to rework that before v4.3.

Marc, can I take it from your use of the binding that you are happy to
provide your ack?

Thanks,
Mark.

On Thu, Jul 23, 2015 at 05:52:43PM +0100, Mark Rutland wrote:
 Currently msi-parent is used in a couple of drivers despite being fairly
 underspecified. This patch adds a generic binding for MSIs (including
 the existing msi-parent property) enabling the description of platform
 devices capable of using MSIs.
 
 While MSIs are primarily distinguished by doorbell and payload, some MSI
 controllers (e.g. the GICv3 ITS) also use side-band information
 accompanying the write to identify the master which originated the MSI,
 to allow for sandboxing. This sideband information is non-probeable and
 needs to be described in the DT. Other MSI controllers may have
 additional configuration details which need to be described per-master.
 
 This patch adds a generic msi-parent binding document, extending the
 de-facto standard with a new (optional) #msi-cells which can be used to
 express any per-master configuration and/or sideband data. This is
 sufficient to describe non-hotpluggable devices.
 
 For busses where sideband data may be derived from some bus-specific
 master ID scheme, other properties will be required to describe the
 mapping.
 
 Signed-off-by: Mark Rutland mark.rutl...@arm.com
 ---
  .../bindings/interrupt-controller/msi.txt  | 135 
 +
  1 file changed, 135 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/interrupt-controller/msi.txt
 
 diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi.txt 
 b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
 new file mode 100644
 index 000..c60c034
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
 @@ -0,0 +1,135 @@
 +This document describes the generic device tree binding for MSI controllers 
 and
 +their master(s).
 +
 +Message Signaled Interrupts (MSIs) are a class of interrupts generated by a
 +write to an MMIO address.
 +
 +MSIs were originally specified by PCI (and are used with PCIe), but may also 
 be
 +used with other busses, and hence a mechanism is required to relate devices 
 on
 +those busses to the MSI controllers which they are capable of using,
 +potentially including additional information.
 +
 +MSIs are distinguished by some combination of:
 +
 +- The doorbell (the MMIO address written to).
 +  
 +  Devices may be configured by software to write to arbitrary doorbells which
 +  they can address. An MSI controller may feature a number of doorbells.
 +
 +- The payload (the value written to the doorbell).
 +  
 +  Devices may be configured to write an arbitrary payload chosen by software.
 +  MSI controllers may have restrictions on permitted payloads.
 +
 +- Sideband information accompanying the write.
 +  
 +  Typically this is neither configurable nor probeable, and depends on the 
 path
 +  taken through the memory system (i.e. it is a property of the combination 
 of
 +  MSI controller and device rather than a property of either in isolation).
 +
 +
 +MSI controllers:
 +
 +
 +An MSI controller signals interrupts to a CPU when a write is made to an MMIO
 +address by some master. An MSI controller may feature a number of doorbells.
 +
 +Required properties:
 +
 +
 +- msi-controller: Identifies the node as an MSI controller.
 +
 +Optional properties:
 +
 +
 +- #msi-cells: The number of cells in an msi-specifier, required if not zero.
 +
 +  Typically this will encode information related to sideband data, and will
 +  not encode doorbells or payloads as these can be configured dynamically.
 +
 +  The meaning of the msi-specifier is defined by the device tree binding of
 +  the specific MSI controller. 
 +
 +
 +MSI clients
 +===
 +
 +MSI clients are devices which generate MSIs. For each MSI they wish to
 +generate, the doorbell and payload may be configured, though sideband
 +information may not be configurable.
 +
 +Required properties:
 +
 +
 +- msi-parent: A list of phandle + msi-specifier pairs, one for each MSI
 +  controller which the device is capable of using.
 +
 +  This property is unordered, and MSIs may be allocated from any combination 
 of
 +  MSI controllers listed in the msi-parent property.
 +
 +  If a device has restrictions on the allocation of MSIs, these restrictions
 +  must be described with additional properties.
 +
 +  When #msi-cells is non-zero, busses with an msi-parent will require
 +  additional properties to describe the relationship between devices on the 
 bus
 +  and the set of MSIs they can potentially generate.
 +
 +
 +Example

Re: [PATCH 2/3] Docs: dt: Add PCI MSI map bindings

[Mark Rutland]

  +Example (5)
  +===
  +
  +/ {
  +   #address-cells = 1;
  +   #size-cells = 1;
  +
  +   msi_a: msi-controller@a {
  +   reg = 0xa 0x1;
  +   compatible = vendor,some-controller;
  +   msi-controller;
  +   #msi-cells = 1;
  +   };
  +
  +   msi_b: msi-controller@b {
  +   reg = 0xb 0x1;
  +   compatible = vendor,some-controller;
  +   msi-controller;
  +   #msi-cells = 1;
  +   };
  +
  +   msi_c: msi-controller@c {
  +   reg = 0xc 0x1;
  +   compatible = vendor,some-controller;
  +   msi-controller;
  +   #msi-cells = 1;
  +   };
  +
  +   pci: pci@c {
  +   reg = 0xf 0x1;
  +   compatible = vendor,pcie-root-complex;
  +   device_type = pci;
  +
  +   /*
  +* The sideband data provided to MSI controller a is the
  +* RID, but the high bit of the bus number is negated.
  +* The sideband data provided to MSI controller b is the
  +* RID, identity-mapped.
  +* MSI controller c is not addressable.
  +*/
  +   msi-map = 0x msi_a 0x8000 0x08000,
  + 0x8000 msi_a 0x 0x08000,
  + 0x msi_b 0x 0x1;
  +   };
 
 they can be identical right? like
   0x8000 msi_a 0x 0x08000,
   0x8000 msi_b 0x 0x08000;

In general that would be valid, yes.

In this case two entries are required for MSI controller a because the
high bit passed to it is negated. This does not occur for MSI controller
b, so it only requires a single entry to describe the transformation.

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

Re: [PATCH 1/3] Docs: dt: add generic MSI bindings

[Mark Rutland]

On Mon, Jul 27, 2015 at 09:02:46AM +0100, Marc Zyngier wrote:
 Hi Mark,

Hi,

 On 23/07/15 17:52, Mark Rutland wrote:
  Currently msi-parent is used in a couple of drivers despite being fairly
  underspecified. This patch adds a generic binding for MSIs (including
  the existing msi-parent property) enabling the description of platform
  devices capable of using MSIs.
  
  While MSIs are primarily distinguished by doorbell and payload, some MSI
  controllers (e.g. the GICv3 ITS) also use side-band information
  accompanying the write to identify the master which originated the MSI,
  to allow for sandboxing. This sideband information is non-probeable and
  needs to be described in the DT. Other MSI controllers may have
  additional configuration details which need to be described per-master.
  
  This patch adds a generic msi-parent binding document, extending the
  de-facto standard with a new (optional) #msi-cells which can be used to
  express any per-master configuration and/or sideband data. This is
  sufficient to describe non-hotpluggable devices.
  
  For busses where sideband data may be derived from some bus-specific
  master ID scheme, other properties will be required to describe the
  mapping.
  
  Signed-off-by: Mark Rutland mark.rutl...@arm.com
  ---
   .../bindings/interrupt-controller/msi.txt  | 135 
  +
   1 file changed, 135 insertions(+)
   create mode 100644 
  Documentation/devicetree/bindings/interrupt-controller/msi.txt
  
  diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi.txt 
  b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
  new file mode 100644
  index 000..c60c034
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
  @@ -0,0 +1,135 @@
  +This document describes the generic device tree binding for MSI 
  controllers and
  +their master(s).
  +
  +Message Signaled Interrupts (MSIs) are a class of interrupts generated by a
  +write to an MMIO address.
  +
  +MSIs were originally specified by PCI (and are used with PCIe), but may 
  also be
  +used with other busses, and hence a mechanism is required to relate 
  devices on
  +those busses to the MSI controllers which they are capable of using,
  +potentially including additional information.
  +
  +MSIs are distinguished by some combination of:
  +
  +- The doorbell (the MMIO address written to).
  +  
  +  Devices may be configured by software to write to arbitrary doorbells 
  which
  +  they can address. An MSI controller may feature a number of doorbells.
  +
  +- The payload (the value written to the doorbell).
  +  
  +  Devices may be configured to write an arbitrary payload chosen by 
  software.
  +  MSI controllers may have restrictions on permitted payloads.
  +
  +- Sideband information accompanying the write.
  +  
  +  Typically this is neither configurable nor probeable, and depends on the 
  path
  +  taken through the memory system (i.e. it is a property of the 
  combination of
  +  MSI controller and device rather than a property of either in isolation).
  +
  +
  +MSI controllers:
  +
  +
  +An MSI controller signals interrupts to a CPU when a write is made to an 
  MMIO
  +address by some master. An MSI controller may feature a number of 
  doorbells.
  +
  +Required properties:
  +
  +
  +- msi-controller: Identifies the node as an MSI controller.
  +
  +Optional properties:
  +
  +
  +- #msi-cells: The number of cells in an msi-specifier, required if not 
  zero.
  +
  +  Typically this will encode information related to sideband data, and will
  +  not encode doorbells or payloads as these can be configured dynamically.
  +
  +  The meaning of the msi-specifier is defined by the device tree binding of
  +  the specific MSI controller. 
  +
  +
  +MSI clients
  +===
  +
  +MSI clients are devices which generate MSIs. For each MSI they wish to
  +generate, the doorbell and payload may be configured, though sideband
  +information may not be configurable.
  +
  +Required properties:
  +
  +
  +- msi-parent: A list of phandle + msi-specifier pairs, one for each MSI
  +  controller which the device is capable of using.
  +
  +  This property is unordered, and MSIs may be allocated from any 
  combination of
  +  MSI controllers listed in the msi-parent property.
  +
  +  If a device has restrictions on the allocation of MSIs, these 
  restrictions
  +  must be described with additional properties.
  +
  +  When #msi-cells is non-zero, busses with an msi-parent will require
  +  additional properties to describe the relationship between devices on 
  the bus
  +  and the set of MSIs they can potentially generate.
  +
  +
  +Example
  +===
  +
  +/ {
  +   #address-cells = 1;
  +   #size-cells = 1;
  +
  +   msi_a: msi-controller@a {
  +   reg = 0xa 0xf00;
  +   compatible = vendor-a,some-controller

Re: [PATCH 3/3] Docs: dt: add PCI IOMMU map bindings

[Mark Rutland]

Hi,

 This looks sane, and lets me describe the thing I have on my desk, so 
 I'm happy. I have a couple of general thoughts below, but I don't intend 
 that they should stand in the way of this proposal as-is.

Good to hear that this doesn't fall apart at the sight of a real system!

 On 23/07/15 17:52, Mark Rutland wrote:
  The existing IOMMU bindings are able to specify the relationship between
  masters and IOMMUs, but they are insufficient for describing the general
  case of hotpluggable busses such as PCI where the set of masters is not
  known until runtime, and the relationship between masters and IOMMUs is
  a property of the integration of the system.
 
  This patch adds a generic binding for mapping PCI devices to IOMMUs,
  using a new iommu-map property (specific to PCI*) which may be used to
  map devices (identified by their Requester ID) to sideband data for the
  IOMMU which they master through.
 
  Signed-off-by: Mark Rutland mark.rutl...@arm.com
  ---
.../devicetree/bindings/pci/pci-iommu.txt  | 171 
  +
1 file changed, 171 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt
 
  diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt 
  b/Documentation/devicetree/bindings/pci/pci-iommu.txt
  new file mode 100644
  index 000..56c8296
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
  @@ -0,0 +1,171 @@
  +This document describes the generic device tree binding for describing the
  +relationship between PCI(e) devices and IOMMU(s).
  +
  +Each PCI(e) device under a root complex is uniquely identified by its 
  Requester
  +ID (AKA RID). A Requester ID is a triplet of a Bus number, Device number, 
  and
  +Function number.
  +
  +For the purpose of this document, when treated as a numeric value, a RID is
  +formatted such that:
  +
  +* Bits [15:8] are the Bus number.
  +* Bits [7:3] are the Device number.
  +* Bits [2:0] are the Function number.
  +* Any other bits required for padding must be zero.
  +
  +IOMMUs may distinguish PCI devices through sideband data derived from the
  +Requester ID. While a given PCI device can only master through one IOMMU, a
  +root complex may split masters across a set of IOMMUs (e.g. with one IOMMU 
  per
  +bus).
  +
  +The generic 'iommus' property is insufficient to describe this 
  relationship,
  +and a mechanism is required to map from a PCI device to its IOMMU and 
  sideband
  +data.
  +
  +For generic IOMMU bindings, see
  +Documentation/devicetree/bindings/iommu/iommu.txt.
  +
  +
  +PCI root complex
  +
  +
  +Optional properties
  +---
  +
  +- iommu-map: Maps a Requester ID to an IOMMU and associated iommu-specifier
  +  data.
  +
  +  The property is an arbitrary number of tuples of
  +  (rid-base,iommu,iommu-base,length).
  +
  +  Any RID r in the interval [rid-base, rid-base + length) is associated 
  with
  +  the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
 
 Can we take as a guarantee that the system cannot present any ID at a 
 given IOMMU that is not represented in an appropriate output range (in 
 the sense that we may do things that could blow up horribly if spurious 
 IDs appeared)?

I would expect that for the root complex, the iommu-map should cover all
possible RIDs (and hence we would know their output IDs). In the case
that a possible RID was not translated by the property, I would hope
that we could either detect this at parse time, or prevent probing of
the device when it appeared.

The root complex could share IOMMUs with other masters, so in general
iommu-map alone would not be sufficient to detect all possible IDs.

 Furthermore, would representing one-to-many mappings by having multiple 
 matches for a given RID be legal? In the general case it's certainly 
 feasible for the IOMMU to see different IDs for e.g. reads vs. writes, 
 where the system munges extra bus lines into the sideband signals - 
 whether anyone would actually integrate a PCI host controller that way 
 is another matter, so I don't think it's something worth really worrying 
 about without a definite need.

I'd expect no-one would implement separate read and write IDs, given
that the IOMMU should distinguish reads and writes anyway.

Generally I don't think multiple matches for the same RID make sense.

  +- iommu-map-mask: A mask to be applied to each Requester ID prior to being
  +  mapped to an iommu-specifier per the iommu-map property.
 
 Am I right to assume a mask of 0 would be a valid way to represent 
 everything (and if so, should rid-base and length just be ignored, or 
 mandated to be 0 and 1 respectively)? It looks a bit off at first 
 glance, but it does neatly address a genuine use-case.

I think that should be valid, yes.

  +
  +
  +Example (1)
  +===
  +
  +/ {
  +   #address-cells = 1;
  +   #size-cells = 1;
  +
  +   iommu: iommu@a {
  +   reg = 0xa

[PATCH 0/3] Generic PCI MSI + IOMMU topology bindings

[Mark Rutland]

Hi all,

Currently we have no generic/standard mechanisms for describing the
relationship between PCI root complexes and other components which may be
required to make them usable, specifically IOMMUs and MSI controllers.

There is an existing binding for IOMMUs, and there is a de-facto standard for
referring to MSI controllers, but the generic portion of these can only
describe a relationship with a root complex as opposed to a device under a root
complex. This falls apart where IOMMUs and MSI controllers may distinguish
individual devices based on non-probeable information.

This series adds a generic glue bindings for describing the relationship
between root complexes, IOMMUs, and MSI controllers. The existing de-facto
binding for MSI controllers is formalised, along with a (backwards compatible)
extension necessary for describing contemporary MSI controllers which make use
of (non-probeable) sideband data.

Thanks,
Mark.

Mark Rutland (3):
  Docs: dt: add generic MSI bindings
  Docs: dt: Add PCI MSI map bindings
  Docs: dt: add PCI IOMMU map bindings

 .../bindings/interrupt-controller/msi.txt  | 135 +
 .../devicetree/bindings/pci/pci-iommu.txt  | 171 
 Documentation/devicetree/bindings/pci/pci-msi.txt  | 220 +
 3 files changed, 526 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/interrupt-controller/msi.txt
 create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt
 create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt

-- 
1.9.1

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

[PATCH 2/3] Docs: dt: Add PCI MSI map bindings

[Mark Rutland]

Currently msi-parent is used by a few bindings to describe the
relationship between a PCI root complex and a single MSI controller, but
this property does not have a generic binding document.

Additionally, msi-parent is insufficient to describe more complex
relationships between MSI controllers and devices under a root complex,
where devices may be able to target multiple MSI controllers, or where
MSI controllers use (non-probeable) sideband information to distinguish
devices.

This patch adds a generic binding for mapping PCI devices to MSI
controllers. This document covers msi-parent, and a new msi-map property
(specific to PCI*) which may be used to map devices (identified by their
Requester ID) to sideband data for each MSI controller that they may
target.

Signed-off-by: Mark Rutland mark.rutl...@arm.com
---
 Documentation/devicetree/bindings/pci/pci-msi.txt | 220 ++
 1 file changed, 220 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt 
b/Documentation/devicetree/bindings/pci/pci-msi.txt
new file mode 100644
index 000..9b3cc81
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
@@ -0,0 +1,220 @@
+This document describes the generic device tree binding for describing the
+relationship between PCI devices and MSI controllers.
+
+Each PCI device under a root complex is uniquely identified by its Requester ID
+(AKA RID). A Requester ID is a triplet of a Bus number, Device number, and
+Function number.
+
+For the purpose of this document, when treated as a numeric value, a RID is
+formatted such that:
+
+* Bits [15:8] are the Bus number.
+* Bits [7:3] are the Device number.
+* Bits [2:0] are the Function number.
+* Any other bits required for padding must be zero.
+
+MSIs may be distinguished in part through the use of sideband data accompanying
+writes. In the case of PCI devices, this sideband data may be derived from the
+Requester ID. A mechanism is required to associate a device with both the MSI
+controllers it can address, and the sideband data that will be associated with
+its writes to those controllers.
+
+For generic MSI bindings, see
+Documentation/devicetree/bindings/interrupt-controller/msi.txt.
+
+
+PCI root complex
+
+
+Optional properties
+---
+
+- msi-map: Maps a Requester ID to an MSI controller and associated
+  msi-specifier data. The property is an arbitrary number of tuples of
+  (rid-base,msi-controller,msi-base,length), where:
+
+  * rid-base is a single cell describing the first RID matched by the entry.
+
+  * msi-controller is a single phandle to an MSI controller
+
+  * msi-base is an msi-specifier describing the msi-specifier produced for the
+first RID matched by the entry.
+
+  * length is a single cell describing how many consecutive RIDs are matched
+following the rid-base.
+
+  Any RID r in the interval [rid-base, rid-base + length) is associated with
+  the listed msi-controller, with the msi-specifier (r - rid-base + msi-base).
+
+- msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
+  to an msi-specifier per the msi-map property.
+
+- msi-parent: Describes the MSI parent of the root complex itself. Where
+  the root complex and MSI controller do not pass sideband data with MSI
+  writes, this property may be used to describe the MSI controller(s)
+  used by PCI devices under the root complex, if defined as such in the
+  binding for the root complex.
+
+
+Example (1)
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   msi: msi-controller@a {
+   reg = 0xa 0x1;
+   compatible = vendor,some-controller;
+   msi-controller;
+   #msi-cells = 1;
+   };
+
+   pci: pci@f {
+   reg = 0xf 0x1;
+   compatible = vendor,pcie-root-complex;
+   device_type = pci;
+
+   /*
+* The sideband data provided to the MSI controller is
+* the RID, identity-mapped.
+*/
+   msi-map = 0x0 msi_a 0x0 0x1,
+   };
+};
+
+
+Example (2)
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   msi: msi-controller@a {
+   reg = 0xa 0x1;
+   compatible = vendor,some-controller;
+   msi-controller;
+   #msi-cells = 1;
+   };
+
+   pci: pci@f {
+   reg = 0xf 0x1;
+   compatible = vendor,pcie-root-complex;
+   device_type = pci;
+
+   /*
+* The sideband data provided to the MSI controller is
+* the RID, masked to only the device and function bits.
+*/
+   msi-map = 0x0 msi_a 0x0 0x100,
+   msi-map-mask = 0xff
+   };
+};
+
+
+Example (3)
+===
+
+/ {
+   #address-cells = 1;
+   #size

[PATCH 1/3] Docs: dt: add generic MSI bindings

[Mark Rutland]

Currently msi-parent is used in a couple of drivers despite being fairly
underspecified. This patch adds a generic binding for MSIs (including
the existing msi-parent property) enabling the description of platform
devices capable of using MSIs.

While MSIs are primarily distinguished by doorbell and payload, some MSI
controllers (e.g. the GICv3 ITS) also use side-band information
accompanying the write to identify the master which originated the MSI,
to allow for sandboxing. This sideband information is non-probeable and
needs to be described in the DT. Other MSI controllers may have
additional configuration details which need to be described per-master.

This patch adds a generic msi-parent binding document, extending the
de-facto standard with a new (optional) #msi-cells which can be used to
express any per-master configuration and/or sideband data. This is
sufficient to describe non-hotpluggable devices.

For busses where sideband data may be derived from some bus-specific
master ID scheme, other properties will be required to describe the
mapping.

Signed-off-by: Mark Rutland mark.rutl...@arm.com
---
 .../bindings/interrupt-controller/msi.txt  | 135 +
 1 file changed, 135 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/interrupt-controller/msi.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi.txt 
b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
new file mode 100644
index 000..c60c034
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
@@ -0,0 +1,135 @@
+This document describes the generic device tree binding for MSI controllers and
+their master(s).
+
+Message Signaled Interrupts (MSIs) are a class of interrupts generated by a
+write to an MMIO address.
+
+MSIs were originally specified by PCI (and are used with PCIe), but may also be
+used with other busses, and hence a mechanism is required to relate devices on
+those busses to the MSI controllers which they are capable of using,
+potentially including additional information.
+
+MSIs are distinguished by some combination of:
+
+- The doorbell (the MMIO address written to).
+  
+  Devices may be configured by software to write to arbitrary doorbells which
+  they can address. An MSI controller may feature a number of doorbells.
+
+- The payload (the value written to the doorbell).
+  
+  Devices may be configured to write an arbitrary payload chosen by software.
+  MSI controllers may have restrictions on permitted payloads.
+
+- Sideband information accompanying the write.
+  
+  Typically this is neither configurable nor probeable, and depends on the path
+  taken through the memory system (i.e. it is a property of the combination of
+  MSI controller and device rather than a property of either in isolation).
+
+
+MSI controllers:
+
+
+An MSI controller signals interrupts to a CPU when a write is made to an MMIO
+address by some master. An MSI controller may feature a number of doorbells.
+
+Required properties:
+
+
+- msi-controller: Identifies the node as an MSI controller.
+
+Optional properties:
+
+
+- #msi-cells: The number of cells in an msi-specifier, required if not zero.
+
+  Typically this will encode information related to sideband data, and will
+  not encode doorbells or payloads as these can be configured dynamically.
+
+  The meaning of the msi-specifier is defined by the device tree binding of
+  the specific MSI controller. 
+
+
+MSI clients
+===
+
+MSI clients are devices which generate MSIs. For each MSI they wish to
+generate, the doorbell and payload may be configured, though sideband
+information may not be configurable.
+
+Required properties:
+
+
+- msi-parent: A list of phandle + msi-specifier pairs, one for each MSI
+  controller which the device is capable of using.
+
+  This property is unordered, and MSIs may be allocated from any combination of
+  MSI controllers listed in the msi-parent property.
+
+  If a device has restrictions on the allocation of MSIs, these restrictions
+  must be described with additional properties.
+
+  When #msi-cells is non-zero, busses with an msi-parent will require
+  additional properties to describe the relationship between devices on the bus
+  and the set of MSIs they can potentially generate.
+
+
+Example
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   msi_a: msi-controller@a {
+   reg = 0xa 0xf00;
+   compatible = vendor-a,some-controller;
+   msi-controller;
+   /* No sideband data, so #msi-cells omitted */
+   };
+
+   msi_b: msi-controller@b {
+   reg = 0xb 0xf00;
+   compatible = vendor-b,another-controller;
+   msi-controller;
+   /* Each device has some unique ID */
+   #msi-cells = 1;
+   };
+
+   msi_c: msi

[PATCH 3/3] Docs: dt: add PCI IOMMU map bindings

[Mark Rutland]

The existing IOMMU bindings are able to specify the relationship between
masters and IOMMUs, but they are insufficient for describing the general
case of hotpluggable busses such as PCI where the set of masters is not
known until runtime, and the relationship between masters and IOMMUs is
a property of the integration of the system.

This patch adds a generic binding for mapping PCI devices to IOMMUs,
using a new iommu-map property (specific to PCI*) which may be used to
map devices (identified by their Requester ID) to sideband data for the
IOMMU which they master through.

Signed-off-by: Mark Rutland mark.rutl...@arm.com
---
 .../devicetree/bindings/pci/pci-iommu.txt  | 171 +
 1 file changed, 171 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt 
b/Documentation/devicetree/bindings/pci/pci-iommu.txt
new file mode 100644
index 000..56c8296
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
@@ -0,0 +1,171 @@
+This document describes the generic device tree binding for describing the
+relationship between PCI(e) devices and IOMMU(s).
+
+Each PCI(e) device under a root complex is uniquely identified by its Requester
+ID (AKA RID). A Requester ID is a triplet of a Bus number, Device number, and
+Function number.
+
+For the purpose of this document, when treated as a numeric value, a RID is
+formatted such that:
+
+* Bits [15:8] are the Bus number.
+* Bits [7:3] are the Device number.
+* Bits [2:0] are the Function number.
+* Any other bits required for padding must be zero.
+
+IOMMUs may distinguish PCI devices through sideband data derived from the
+Requester ID. While a given PCI device can only master through one IOMMU, a
+root complex may split masters across a set of IOMMUs (e.g. with one IOMMU per
+bus).
+
+The generic 'iommus' property is insufficient to describe this relationship,
+and a mechanism is required to map from a PCI device to its IOMMU and sideband
+data.
+
+For generic IOMMU bindings, see
+Documentation/devicetree/bindings/iommu/iommu.txt.
+
+
+PCI root complex
+
+
+Optional properties
+---
+
+- iommu-map: Maps a Requester ID to an IOMMU and associated iommu-specifier
+  data.
+
+  The property is an arbitrary number of tuples of
+  (rid-base,iommu,iommu-base,length).
+
+  Any RID r in the interval [rid-base, rid-base + length) is associated with
+  the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
+
+- iommu-map-mask: A mask to be applied to each Requester ID prior to being
+  mapped to an iommu-specifier per the iommu-map property.
+
+
+Example (1)
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   iommu: iommu@a {
+   reg = 0xa 0x1;
+   compatible = vendor,some-iommu;
+   #iommu-cells = 1;
+   };
+
+   pci: pci@f {
+   reg = 0xf 0x1;
+   compatible = vendor,pcie-root-complex;
+   device_type = pci;
+
+   /*
+* The sideband data provided to the IOMMU is the RID,
+* identity-mapped.
+*/
+   iommu-map = 0x0 iommu 0x0 0x1;
+   };
+};
+
+
+Example (2)
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   iommu: iommu@a {
+   reg = 0xa 0x1;
+   compatible = vendor,some-iommu;
+   #iommu-cells = 1;
+   };
+
+   pci: pci@f {
+   reg = 0xf 0x1;
+   compatible = vendor,pcie-root-complex;
+   device_type = pci;
+
+   /*
+* The sideband data provided to the IOMMU is the RID with the
+* function bits masked out.
+*/
+   iommu-map = 0x0 iommu 0x0 0x1;
+   iommu-map-mask = 0xfff8;
+   };
+};
+
+
+Example (3)
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   iommu: iommu@a {
+   reg = 0xa 0x1;
+   compatible = vendor,some-iommu;
+   #iommu-cells = 1;
+   };
+
+   pci: pci@f {
+   reg = 0xf 0x1;
+   compatible = vendor,pcie-root-complex;
+   device_type = pci;
+
+   /*
+* The sideband data provided to the IOMMU is the RID,
+* but the high bits of the bus number are flipped.
+*/
+   iommu-map = 0x iommu 0x8000 0x8000,
+   0x8000 iommu 0x 0x8000;
+   };
+};
+
+
+Example (4)
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   iommu_a: iommu@a {
+   reg = 0xa 0x1;
+   compatible = vendor,some-iommu;
+   #iommu-cells = 1;
+   };
+
+   iommu_b: iommu@b {
+   reg = 0xb 0x1;
+   compatible = vendor,some

Re: Master-aware devices and sideband ID data

[Mark Rutland]

On Tue, Jun 09, 2015 at 11:17:54AM +0100, Mark Rutland wrote:
 On Fri, Jun 05, 2015 at 10:05:34AM +0100, Will Deacon wrote:
  On Thu, Jun 04, 2015 at 11:19:30PM +0100, Chalamarla, Tirumalesh wrote:
On Jun 1, 2015, at 3:22 AM, Mark Rutland mark.rutl...@arm.com wrote:
It's possible to specify that the paths exist. I expect that software
would select which to use at runtime.

   My worry is how to define any priorities/preferences between masters. 
   in general the proposal looks reasonable.
  
  I agree that the proposal looks reasonable (in terms of the ability to
  describe the sort of topologies that we will face) but I still don't
  understand what I need to do in e.g. my IOMMU driver to support this
  binding whilst continuing to support the existing iommus binding, which
  is relied upon to configure dma-mapping.
  
  Mark: how do you see this co-existing/merging with the current bindings?
 
 As I mentioned in my initial mail, it's not clear to me how this can be
 reconciled with the current bindings. Everything I've been able to come
 up with so far at best ends up describing the same thing repeatedly.
 
 I'll see what I can come up with. Any sugestions are welcome!

I can't see a way of keeping the ID transformations explicit with the
existing bindings, but I think we can simply fold these down into
properties in the master nodes, given we expect each ID to be derived
from some initial master ID anyway.

So, to cater for the ITS we would need to pass master IDs along with the
MSI parent information, which we could do by extending msi-parent or by
introducing a new msis property which behaves similarly to the iommus
property, describing the MSI controllers the device can address (via any
IOMMUs), along with any controller-specific identification data.

Which means we'd have DT fragments like the following for an arbitrary
platform device:

its0: its {
...
msi-controller;
#msi-cells = 1; // DeviceId
};

its1: its {
...
msi-controller;
#msi-cells = 1; // DeviceId
};

smmu: smmu {
...
iommu-cells = 1; // StreamId
};

device {
...
iommus = its 0;
/* Can use either ITS, but has a different ID at each */
msis = its0 0x0, its1 0x400;
};

That doesn't allow you to describe a device with multiple mater ports
where each master port might want to generate MSIs, but I'm not sure if
that's a real case.

For PCIe root complexes, we'd need to describe the BDF - iommu-cells
and BDF - msi-cells translations separately with new properties on the
node for the root complex itself.

Is there anything obviously broken with the above approach?

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

Re: Master-aware devices and sideband ID data

[Mark Rutland]

On Fri, Jun 05, 2015 at 10:05:34AM +0100, Will Deacon wrote:
 On Thu, Jun 04, 2015 at 11:19:30PM +0100, Chalamarla, Tirumalesh wrote:
   On Jun 1, 2015, at 3:22 AM, Mark Rutland mark.rutl...@arm.com wrote:
   It's possible to specify that the paths exist. I expect that software
   would select which to use at runtime.
   
  My worry is how to define any priorities/preferences between masters. 
  in general the proposal looks reasonable.
 
 I agree that the proposal looks reasonable (in terms of the ability to
 describe the sort of topologies that we will face) but I still don't
 understand what I need to do in e.g. my IOMMU driver to support this
 binding whilst continuing to support the existing iommus binding, which
 is relied upon to configure dma-mapping.
 
 Mark: how do you see this co-existing/merging with the current bindings?

As I mentioned in my initial mail, it's not clear to me how this can be
reconciled with the current bindings. Everything I've been able to come
up with so far at best ends up describing the same thing repeatedly.

I'll see what I can come up with. Any sugestions are welcome!

 I don't think it's practical to throw away what we have and move over to
 something totally different all in one go, but there clearly *is* benefit
 in your proposal over the existing scheme.

I can see that's probably not practical. :(

Do we know what we're going to do w.r.t. IORT? That's going to require
the kernel to be able to handle a similar description to this proposal.

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

Re: Master-aware devices and sideband ID data

[Mark Rutland]

On Fri, May 29, 2015 at 06:46:07PM +0100, Chalamarla, Tirumalesh wrote:
 
  On May 27, 2015, at 10:39 AM, Mark Rutland mark.rutl...@arm.com wrote:
  
  On Tue, May 26, 2015 at 11:20:59PM +0100, Chalamarla, Tirumalesh wrote:
  This is some thing we also like to see in ITS and SMMU drivers. 
  On Mar 24, 2015, at 8:50 AM, Mark Rutland mark.rutl...@arm.com wrote:
  
  Hi all,
  
  For some devices, identification of particular masters is critical to
  their operation (e.g. IOMMUs, MSI controllers). The identity of masters
  is determined from sideband signals on the bus, and it may or may not be
  possible to probe and/or configure this information. Worse still, these
  information may be rewritten by intermediate buses, so the
  information for a given master may differ at different points in the bus
  hierarchy.
  
  We currently describe this master information for devices attached to
  IOMMUs, with a master ID being encoded in the iommu-cells. However this
  only covers the ID between the master and its IOMMU(s), and we don't
  currently have a mechanism to describe the master IDs as they are seen
  by devices beyond the IOMMU(s), or in the absence of any IOMMU(s).
  
  The following are example of master IDs:
  * PCIe Requester IDs (RIDs) (bus:device.function AKA BDF)
  * SMMU Stream IDs (SIDs)
  * GICv3 ITS Device IDs (DIDs)
  
  In the case of a system with PCIe, SMMU and GICv3, the master IDs are
  rewritten in a chain of RID=SID=DID, as in the following diagram:
  
  +-+
  | PCIe master |
  +-+
||
|| Requester ID (RID)
|| Probeable from RC.
\/
  +---+
  | PCIe Root Complex |
  +---+
||
|| SMMU Stream ID (SID)
|| Derived from RID, static non-probeable mapping.
\/
  +--+
  | SMMU (IOMMU) |
  +--+
||
|| ITS Device ID (DID)
|| Derived from SID, static non-probeable mapping.
\/
  ++
  | GICv3 ITS (MSI controller) |
  ++
  
  In simpler cases, you might have a simpler set of master ID
  translations, e.g. just a DID:
  
  +-+
  | Platform device |
  +-+
||
|| ITS Device ID (DID)
|| Hard-wired on the bus.
\/
  ++
  | GICv3 ITS (MSI controller) |
  ++
  
  Ignoring current bindings for the moment, I can see how we can describe
  this with a generic master id-binding, with master-id-map along the
  lines of interrupt-map, with a tuple format like:
  child-id-base child-id-length parent parent-id-base
  
  For the PCIe example, this would look something like the following (with
  properties omitted for brevity):
  
  PCI: pci@af00 {
...

/* Requester ID of PCIe endpoints, implicit at runtime */
master-id-cells = 1;
  
/* RIDS idmapped to SIDS @ SMMU */
master-id-map = 0 0x1 SMMU 0;
  }
  
  SMMU: iommu@bf00 {
...
  
/* SID, derived from RID */
master-id-cells = 1;
  
/* 
 * For some reason the high bit of the ID was negated.
 */
master-id-map = 0x 0x8000 ITS 0x0 0x8000,
0x8000 0x8000 ITS 0x0 0x;
  
  };
  
  ITS: its@cf00 {
...
  
/* DID, derived from SID */
master-id-cells = 2;
  
/* 
 * Master traffic not propagated beyond this point, so no
 * master-id-ranges
 */
  };
  
  I think it is nice to have max IDs supported by masters. so that drivers 
  can check and enforce.  
  
  The set of IDs that we expect to transform (i.e. those masters use)
  would be implicit in the first master-id-map from a master. In the PCI
  example above, no master is expected to generate an ID outside of the
  range 0-0x1 inclusive (and that's all the SMMU would see).
  
  For devices which are not hotpluggable, the nodes for those devices
  would describe the specific set of IDs they use.
  
  Generally, between a master and a slave there might not be one
  contiguous set of valid IDs as these may be rewritten along the way (as
  happens at the SMMU between the PCI root complex and the ITS in the
  example above).
  
  Which drivers do you think need this information? What exactly are they
  trying to check and enforce?
  
 
 I think, i miss understood by the names, so it is not base it is some thing 
 like
 
 x = f(x) = y   (and f(x) is bitwise | )
 
 ok now i see it. 

It's subtraction and addition rather than bitwise ORing.

Consider a SID 0x8040 entering the SMMU. This falls in the range
described by the SMMU's second master-id-map entry (I've added brackets
to separate each entry:
 
(0x8000) (0x8000) (ITS) (0x0 0x)

As 0x8040 falls in the range described by the first two elements, we kow
we can translate the ID to calculate the ID at the ITS. To do so, we
subtract the child-id-base (0x8000), and add the parent-id-base (0x0
0x). That give us 0x0 0x0040 as our ITS device ID.

  For the simpler case

Re: Master-aware devices and sideband ID data

[Mark Rutland]

On Tue, May 26, 2015 at 11:20:59PM +0100, Chalamarla, Tirumalesh wrote:
 This is some thing we also like to see in ITS and SMMU drivers. 
  On Mar 24, 2015, at 8:50 AM, Mark Rutland mark.rutl...@arm.com wrote:
  
  Hi all,
  
  For some devices, identification of particular masters is critical to
  their operation (e.g. IOMMUs, MSI controllers). The identity of masters
  is determined from sideband signals on the bus, and it may or may not be
  possible to probe and/or configure this information. Worse still, these
  information may be rewritten by intermediate buses, so the
  information for a given master may differ at different points in the bus
  hierarchy.
  
  We currently describe this master information for devices attached to
  IOMMUs, with a master ID being encoded in the iommu-cells. However this
  only covers the ID between the master and its IOMMU(s), and we don't
  currently have a mechanism to describe the master IDs as they are seen
  by devices beyond the IOMMU(s), or in the absence of any IOMMU(s).
  
  The following are example of master IDs:
  * PCIe Requester IDs (RIDs) (bus:device.function AKA BDF)
  * SMMU Stream IDs (SIDs)
  * GICv3 ITS Device IDs (DIDs)
  
  In the case of a system with PCIe, SMMU and GICv3, the master IDs are
  rewritten in a chain of RID=SID=DID, as in the following diagram:
  
  +-+
  | PCIe master |
  +-+
 ||
 || Requester ID (RID)
 || Probeable from RC.
 \/
  +---+
  | PCIe Root Complex |
  +---+
 ||
 || SMMU Stream ID (SID)
 || Derived from RID, static non-probeable mapping.
 \/
  +--+
  | SMMU (IOMMU) |
  +--+
 ||
 || ITS Device ID (DID)
 || Derived from SID, static non-probeable mapping.
 \/
  ++
  | GICv3 ITS (MSI controller) |
  ++
  
  In simpler cases, you might have a simpler set of master ID
  translations, e.g. just a DID:
  
  +-+
  | Platform device |
  +-+
 ||
 || ITS Device ID (DID)
 || Hard-wired on the bus.
 \/
  ++
  | GICv3 ITS (MSI controller) |
  ++
  
  Ignoring current bindings for the moment, I can see how we can describe
  this with a generic master id-binding, with master-id-map along the
  lines of interrupt-map, with a tuple format like:
  child-id-base child-id-length parent parent-id-base
  
  For the PCIe example, this would look something like the following (with
  properties omitted for brevity):
  
  PCI: pci@af00 {
  ...
  
  /* Requester ID of PCIe endpoints, implicit at runtime */
  master-id-cells = 1;
  
  /* RIDS idmapped to SIDS @ SMMU */
  master-id-map = 0 0x1 SMMU 0;
  }
  
  SMMU: iommu@bf00 {
  ...
  
  /* SID, derived from RID */
  master-id-cells = 1;
  
  /* 
   * For some reason the high bit of the ID was negated.
   */
  master-id-map = 0x 0x8000 ITS 0x0 0x8000,
  0x8000 0x8000 ITS 0x0 0x;
  
  };
  
  ITS: its@cf00 {
  ...
  
  /* DID, derived from SID */
  master-id-cells = 2;
  
  /* 
   * Master traffic not propagated beyond this point, so no
   * master-id-ranges
   */
  };
 
 I think it is nice to have max IDs supported by masters. so that drivers can 
 check and enforce.  

The set of IDs that we expect to transform (i.e. those masters use)
would be implicit in the first master-id-map from a master. In the PCI
example above, no master is expected to generate an ID outside of the
range 0-0x1 inclusive (and that's all the SMMU would see).

For devices which are not hotpluggable, the nodes for those devices
would describe the specific set of IDs they use.

Generally, between a master and a slave there might not be one
contiguous set of valid IDs as these may be rewritten along the way (as
happens at the SMMU between the PCI root complex and the ITS in the
example above).

Which drivers do you think need this information? What exactly are they
trying to check and enforce?

  For the simpler case, this would look something like:
  
  DEV: device@af00 {
  master-id-cells = 1;
  master-ids = 0xf, 0xb;
  master-id-map = 0 0xf ITS 0 0;
  };
  
  ITS: its@cf00 {
  ...
  
  /* DID */
  master-id-cells = 2;
  };
  
 Is this is not depending heavily on discover order, how do drivers
 know which device to get which ID. is it implicitly assumed in
 discovery order? 

I'm not sure I follow the question. Could you elaborate?

 what happens to hot pluggable devices.  

It would be necessary to be able to discover the ID assigned to the
device by the hotpluggable bus. For example, this could depend on which
slot the device is plugged into.

If you can't discover the ID associated with a hotpluggable device from
the bus it is plugged into I can't see how hotplug could work.

From

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Mark Rutland]

On Tue, Apr 14, 2015 at 10:07:54AM +0100, Yong Wu wrote:
 Hi Mark,
 Thanks very much for review. 
 About the clock name should be the PoV of _this_ device. Could you
 help check below?
 
 On Fri, 2015-03-06 at 11:13 +, Mark Rutland wrote:
  On Fri, Mar 06, 2015 at 10:48:18AM +, yong...@mediatek.com wrote:
   From: Yong Wu yong...@mediatek.com
   
   This patch add smi binding document.
  
  Please move binding documents to the start of the series. It makes
  things far easier to review.
  
   
   Signed-off-by: Yong Wu yong...@mediatek.com
   +++ b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
   @@ -0,0 +1,17 @@
   +SMI hardware block diagram please help check 
   bindings/iommu/mediatek,iommu.txt
   +
   +Required properties:
   +- compatible : must be mediatek,mediatek,mt8173-smi-larb
  
  Double vendor prefix?
  
  What does larb mean? It would be nice for the intorductory paragraph
  in this file to explain.
  
   +- reg : the register of each local arbiter
   +- clocks : the clocks of each local arbiter
   +- clock-name: larb_sub*(3 clockes at most)
  
  The names required _must_ be specified here, or clock-names is
  pointless.
  
  The clock names should be from the PoV of _this_ device (i.e. they
  should be the names of the inputs) not from the PoV of the provider
  (i.e. they should not be the names of the outputs from the provider).
  
  Mark.
  
  After we check with our SMI Designer. Every SMI local arbiter need
 two clocks, which is called  APB clocks and SMI clock.
  APB clock : Advanced Peripheral Bus Clock. It is the clock for
 setting the register of local arbiter.
  SMI clock : Smart Multimedia Interface Clock, It is the clock for
 transfering the data and command. 
 
  And all the local arbiters need the smi common clock, so we
 separate it. 
 
  Then I prepare to design the smi the dtsi like this: 
 
   smi_common:smi@14022000 {
  compatible = “mediate, mt8173-smi”;
  reg = 0 0x14022000 0 0x1000;
  clocks = mmsys MM_SMI_COMMON;
  clocks-names = “smi_common”;
   };
 
   larb0: larb@14021000 {
  compatible = “mediate, mt8173-smi-larb”;
  reg = 0 0x14021000 0 0x1000;
  smi = smi_common;
  clocks = mmsys MM_SMI_LARB0, 
   mmsys MM_SMI_LARB0;
  clocks-names = “apb_clk”, “smi_clk”;
  };
 
  larb1: larb@1601 {
  compatible = “mediate, mt8173-smi-larb”;
  reg = 0 0x1601 0 0x1000;
  smi = smi_common;
  clocks = vdecsys VDEC_CKEN, 
mmsys VDEC_LARB_CKEN;
  clocks-names = “apb_clk”, “smi_clk”;
   };
   … 
  In some local arbiter, the source clock of the APB clock and the
 SMI clock may be the same, like larb0. so the two clocks are the same.
 And they may be different in other local arbiteres, like larb1.  
 
  If it is designed like this, is it ok?

That looks pretty good; the clocks and names on the larb nodes seem
sensible.

The naming of the smi_common clock on the smi_common node looks a bit
odd though. Is that really what the clock input is called?

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

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Mark Rutland]

 smi_common:smi@14022000 {
compatible = “mediate, mt8173-smi”;
reg = 0 0x14022000 0 0x1000;
clocks = mmsys MM_SMI_COMMON;
clocks-names = “smi_common”;
 };
   
 larb0: larb@14021000 {
compatible = “mediate, mt8173-smi-larb”;
reg = 0 0x14021000 0 0x1000;
smi = smi_common;
clocks = mmsys MM_SMI_LARB0, 
 mmsys MM_SMI_LARB0;
clocks-names = “apb_clk”, “smi_clk”;
};
   
larb1: larb@1601 {
compatible = “mediate, mt8173-smi-larb”;
reg = 0 0x1601 0 0x1000;
smi = smi_common;
clocks = vdecsys VDEC_CKEN, 
  mmsys VDEC_LARB_CKEN;
clocks-names = “apb_clk”, “smi_clk”;
 };
 … 
In some local arbiter, the source clock of the APB clock and the
   SMI clock may be the same, like larb0. so the two clocks are the same.
   And they may be different in other local arbiteres, like larb1.  
   
If it is designed like this, is it ok?
  
  That looks pretty good; the clocks and names on the larb nodes seem
  sensible.
  
  The naming of the smi_common clock on the smi_common node looks a bit
  odd though. Is that really what the clock input is called?
  
  Mark.
  After check with DE, the smi_common clock also have its APB clock
 and the smi clock(they have the same clock source).
 And I prepare to delete _clk in all the clock-names.
 So it may be like this:
  smi_common:smi@14022000 {
  compatible = “mediate, mt8173-smi”;
 reg = 0 0x14022000 0 0x1000;
   clocks = mmsys MM_SMI_COMMON, 
  mmsys MM_SMI_COMMON;
   clocks-names = “apk”,smi;
 };
  How about this?

That looks fine to me. I assume apk should be apb in the last
example.

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

Master-aware devices and sideband ID data

[Mark Rutland]

Hi all,

For some devices, identification of particular masters is critical to
their operation (e.g. IOMMUs, MSI controllers). The identity of masters
is determined from sideband signals on the bus, and it may or may not be
possible to probe and/or configure this information. Worse still, these
information may be rewritten by intermediate buses, so the
information for a given master may differ at different points in the bus
hierarchy.

We currently describe this master information for devices attached to
IOMMUs, with a master ID being encoded in the iommu-cells. However this
only covers the ID between the master and its IOMMU(s), and we don't
currently have a mechanism to describe the master IDs as they are seen
by devices beyond the IOMMU(s), or in the absence of any IOMMU(s).

The following are example of master IDs:
* PCIe Requester IDs (RIDs) (bus:device.function AKA BDF)
* SMMU Stream IDs (SIDs)
* GICv3 ITS Device IDs (DIDs)

In the case of a system with PCIe, SMMU and GICv3, the master IDs are
rewritten in a chain of RID=SID=DID, as in the following diagram:

+-+
| PCIe master |
+-+
||
|| Requester ID (RID)
|| Probeable from RC.
\/
+---+
| PCIe Root Complex |
+---+
||
|| SMMU Stream ID (SID)
|| Derived from RID, static non-probeable mapping.
\/
+--+
| SMMU (IOMMU) |
+--+
||
|| ITS Device ID (DID)
|| Derived from SID, static non-probeable mapping.
\/
++
| GICv3 ITS (MSI controller) |
++

In simpler cases, you might have a simpler set of master ID
translations, e.g. just a DID:

+-+
| Platform device |
+-+
||
|| ITS Device ID (DID)
|| Hard-wired on the bus.
\/
++
| GICv3 ITS (MSI controller) |
++

Ignoring current bindings for the moment, I can see how we can describe
this with a generic master id-binding, with master-id-map along the
lines of interrupt-map, with a tuple format like:
child-id-base child-id-length parent parent-id-base

For the PCIe example, this would look something like the following (with
properties omitted for brevity):

PCI: pci@af00 {
...

/* Requester ID of PCIe endpoints, implicit at runtime */
master-id-cells = 1;

/* RIDS idmapped to SIDS @ SMMU */
master-id-map = 0 0x1 SMMU 0;
}

SMMU: iommu@bf00 {
...

/* SID, derived from RID */
master-id-cells = 1;

/* 
 * For some reason the high bit of the ID was negated.
 */
master-id-map = 0x 0x8000 ITS 0x0 0x8000,
0x8000 0x8000 ITS 0x0 0x;

};

ITS: its@cf00 {
...

/* DID, derived from SID */
master-id-cells = 2;

/* 
 * Master traffic not propagated beyond this point, so no
 * master-id-ranges
 */
};

For the simpler case, this would look something like:

DEV: device@af00 {
master-id-cells = 1;
master-ids = 0xf, 0xb;
master-id-map = 0 0xf ITS 0 0;
};

ITS: its@cf00 {
...

/* DID */
master-id-cells = 2;
};


However, this conflicts/overlaps with existing bindings (at least iommus
and msi-parent), and I'm not sure how to reconcile them. Am I missing a
neat way of describing this that works with the existing bindings?

It's also not clear to me if it's sufficient to have a generic
master-ids property (with the relevant type being implicit from
context), or if each type of ID needs each type of ID (RID, SID, DID,
etc) needs its own.

Which other devices out there which require side-band master
information, and what do they require?

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

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Mark Rutland]

On Fri, Mar 06, 2015 at 10:48:18AM +, yong...@mediatek.com wrote:
 From: Yong Wu yong...@mediatek.com
 
 This patch add smi binding document.

Please move binding documents to the start of the series. It makes
things far easier to review.

 
 Signed-off-by: Yong Wu yong...@mediatek.com
 ---
  .../devicetree/bindings/soc/mediatek/mediatek,smi.txt   | 17 
 +
  1 file changed, 17 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
 
 diff --git a/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt 
 b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
 new file mode 100644
 index 000..0fc9d1a
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
 @@ -0,0 +1,17 @@
 +SMI hardware block diagram please help check 
 bindings/iommu/mediatek,iommu.txt
 +
 +Required properties:
 +- compatible : must be mediatek,mediatek,mt8173-smi-larb

Double vendor prefix?

What does larb mean? It would be nice for the intorductory paragraph
in this file to explain.

 +- reg : the register of each local arbiter
 +- clocks : the clocks of each local arbiter
 +- clock-name: larb_sub*(3 clockes at most)

The names required _must_ be specified here, or clock-names is
pointless.

The clock names should be from the PoV of _this_ device (i.e. they
should be the names of the inputs) not from the PoV of the provider
(i.e. they should not be the names of the outputs from the provider).

Mark.

 +
 +Example:
 + larb1:larb@1601 {
 + compatible = mediatek,mt8173-smi-larb;
 + reg = 0 0x1601 0 0x1000;
 + clocks = mmsys MM_SMI_COMMON,
 + vdecsys VDEC_CKEN,
 +vdecsys VDEC_LARB_CKEN;
 + clock-names = larb_sub0, larb_sub1, larb_sub2;
 + };
 -- 
 1.8.1.1.dirty
 
 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/5] dt-bindings: iommu: Add binding for mediatek IOMMU

[Mark Rutland]

On Fri, Mar 06, 2015 at 10:48:19AM +, yong...@mediatek.com wrote:
 From: Yong Wu yong...@mediatek.com
 
 This patch add mediatek iommu dts binding document.
 
 Signed-off-by: Yong Wu yong...@mediatek.com
 ---
  .../devicetree/bindings/iommu/mediatek,iommu.txt   | 41 
 ++
  1 file changed, 41 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
 
 diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt 
 b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
 new file mode 100644
 index 000..db01c92
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
 @@ -0,0 +1,41 @@
 +/**/
 +/*Mediatek IOMMU Hardware block diagram   */
 +/**/
 +  EMI (External Memory Interface)
 +   |
 +  m4u (Multimedia Memory Management Unit)
 +   |
 +  smi (Smart Multimedia Interface)
 +   |
 ++---+---
 +|   |
 +|   |
 +vdec larb   disp larb  ... SoCs have different local 
 arbiter(larb).
 +|   |
 +|   |
 +   ++++-+-+
 +   |||| | |...
 +   |||| | |...
 +   |||| | |...
 +  MC   PP   VLD  OVL0 RDMA0 WDMA0  ... There are different ports in each 
 larb.
 +
 +Required properties:
 +- compatible : must be mediatek,mt8173-iommu

s/iommu/m4u/ -- the name should match what the hardware is called.

 +- reg : m4u register base

... and size

 +- interrupts : must contain the interrupts from each internal translation 
 unit

How many internal translation units are there?

How should the interrupts be ordered?

How do these relate to the larbs?

 +- clocks : must contain one entry for each clock-name
 +- clock-name: m4u clock

This does not match the example.

s/clock-name/clock-names/

Please format this like a list, with names quoted, e.g.

- clock-names: must contain:
  * m4u - The functional clock for the m4u

 +- larb : must contain the larbes of current platform

s/larbes/local arbiters/

How should these be ordered? Surely there's a relationship with
registers/interrupts/etc in this unit?

 +- iommu-cells : must be 1. Specifies the client PortID as defined in
 +dt-binding/iommu/mt**-iommu-port.h

Give the absolute filename.

The include file should be added in this patch (it's part of the
binding). The whole patch should be moved earlier in the series.

Thanks,
Mark.

 +
 +Example:
 + iommu: mmsys_iommu@10205000 {
 + compatible = mediatek,mt8173-iommu;
 + reg = 0 0x10205000 0 0x1000;
 + interrupts = GIC_SPI 139 IRQ_TYPE_LEVEL_LOW;
 + clocks = infrasys INFRA_M4U;
 + clock-names = infra_m4u;
 + larb = larb0 larb1 larb2 larb3 larb4 larb5;
 + #iommu-cells = 1;
 + };
 \ No newline at end of file
 -- 
 1.8.1.1.dirty
 
 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4] devicetree: Add generic IOMMU device tree bindings

[Mark Rutland]

[...]

   +Examples:
   +=
   +
   +Single-master IOMMU:
   +
   +
   + iommu {
   + #iommu-cells = 0;
   + };
   +
   + master {
   + iommus = /iommu;
  
  Nit: this should be iommus = {/iommu}, or it's not valid dts syntax.
 
 Done.

Cheers. I take it that was done for the other occurrences too?

 
   + };
   +
   +Multiple-master IOMMU with fixed associations:
   +--
   +
   + /* multiple-master IOMMU */
   + iommu {
   + /*
   +  * Masters are statically associated with this IOMMU and
   +  * address translation is always enabled.
   +  */
   + #iommu-cells = 0;
  
  I don't follow why translation being always enabled is relevant to the
  example; that would seem to be independent from the binding.
  
  Surely the key point is that with no way to distinguish devices, they
  presumably share the same translations?
 
 Both aspects are important I think. For #iommu-cells = 0 there is no
 way for the IOMMU driver to know how to enable translation for a given
 device. So it must be either always on or always off.

Sure. But always on or off is not the same as always enabled, which
was what confused me.

 I guess one could say that this is implicit if all masters share the
 same translations. And I guess translations don't always have to be on
 or off technically. Let me try to rephrase this:
 
   /*
* Masters are statically associated with this IOMMU and share
* the same address translations because the IOMMU does not
* have sufficient information to distinguish between masters.
*
* Consequently address translation is always on or off for
* all masters at any given point in time.
*/
 
 Does that sound better?

That addresses my concern, so yes.

Given these are minor and everyone wants this in now, I'm happy for
these to go through in a fixup patch later.

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

Re: [PATCH v4] devicetree: Add generic IOMMU device tree bindings

[Mark Rutland]

Hi Thierry,

This looks sane to me.

I just have a few questions below which are hopefully simple/stupid.

On Fri, Jul 04, 2014 at 04:29:17PM +0100, Thierry Reding wrote:
 From: Thierry Reding tred...@nvidia.com
 
 This commit introduces a generic device tree binding for IOMMU devices.
 Only a very minimal subset is described here, but it is enough to cover
 the requirements of both the Exynos System MMU and Tegra SMMU as
 discussed here:
 
 https://lkml.org/lkml/2014/4/27/346
 
 Signed-off-by: Thierry Reding tred...@nvidia.com
 ---
 Changes in v4:
 - clarify that disabling an IOMMU DT node may not disable translation
 - be more explicit that examples are only examples
 - add multi-ID master example
 
 Changes in v3:
 - use #iommu-cells instead of #address-cells/#size-cells
 - drop optional iommu-names property
 
 Changes in v2:
 - add notes about dma-ranges property (drop note from commit message)
 - document priorities of iommus property vs. dma-ranges property
 - drop #iommu-cells in favour of #address-cells and #size-cells
 - remove multiple-master device example
 
  Documentation/devicetree/bindings/iommu/iommu.txt | 172 
 ++
  1 file changed, 172 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/iommu/iommu.txt
 
 diff --git a/Documentation/devicetree/bindings/iommu/iommu.txt 
 b/Documentation/devicetree/bindings/iommu/iommu.txt
 new file mode 100644
 index ..464a81eaaf61
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/iommu/iommu.txt
 @@ -0,0 +1,172 @@
 +This document describes the generic device tree binding for IOMMUs and their
 +master(s).
 +
 +
 +IOMMU device node:
 +==
 +
 +An IOMMU can provide the following services:
 +
 +* Remap address space to allow devices to access physical memory ranges that
 +  they otherwise wouldn't be capable of accessing.
 +
 +  Example: 32-bit DMA to 64-bit physical addresses
 +
 +* Implement scatter-gather at page level granularity so that the device does
 +  not have to.
 +
 +* Provide system protection against rogue DMA by forcing all accesses to go
 +  through the IOMMU and faulting when encountering accesses to unmapped
 +  address regions.
 +
 +* Provide address space isolation between multiple contexts.
 +
 +  Example: Virtualization
 +
 +Device nodes compatible with this binding represent hardware with some of the
 +above capabilities.
 +
 +IOMMUs can be single-master or multiple-master. Single-master IOMMU devices
 +typically have a fixed association to the master device, whereas multiple-
 +master IOMMU devices can translate accesses from more than one master.
 +
 +The device tree node of the IOMMU device's parent bus must contain a valid
 +dma-ranges property that describes how the physical address space of the
 +IOMMU maps to memory. An empty dma-ranges property means that there is a
 +1:1 mapping from IOMMU to memory.
 +
 +Required properties:
 +
 +- #iommu-cells: The number of cells in an IOMMU specifier needed to encode an
 +  address.
 +
 +The meaning of the IOMMU specifier is defined by the device tree binding of
 +the specific IOMMU. Below are a few examples of typical use-cases:
 +
 +- #iommu-cells = 0: Single master IOMMU devices are not configurable and
 +  therefore no additional information needs to be encoded in the specifier.
 +  This may also apply to multiple master IOMMU devices that do not allow the
 +  association of masters to be configured. Note that an IOMMU can by design
 +  be multi-master yet only expose a single master in a given configuration.
 +  In such cases the number of cells will usually be 1 as in the next case.
 +- #iommu-cells = 1: Multiple master IOMMU devices may need to be configured
 +  in order to enable translation for a given master. In such cases the single
 +  address cell corresponds to the master device's ID. In some cases more than
 +  one cell can be required to represent a single master ID.
 +- #iommu-cells = 4: Some IOMMU devices allow the DMA window for masters to
 +  be configured. The first cell of the address in this may contain the master
 +  device's ID for example, while the second cell could contain the start of
 +  the DMA window for the given device. The length of the DMA window is given
 +  by the third and fourth cells.
 +
 +Note that these are merely examples and real-world use-cases may use 
 different
 +definitions to represent their individual needs. Always refer to the specific
 +IOMMU binding for the exact meaning of the cells that make up the specifier.
 +
 +
 +IOMMU master node:
 +==
 +
 +Devices that access memory through an IOMMU are called masters. A device can
 +have multiple master interfaces (to one or more IOMMU devices).
 +
 +Required properties:
 +
 +- iommus: A list of phandle and IOMMU specifier pairs that describe the IOMMU
 +  master interfaces of the device. One entry in the list describes one master
 +  interface of the device.
 +
 

Re: [PATCH v4] devicetree: Add generic IOMMU device tree bindings

[Mark Rutland]

[...]

  +Multiple-master IOMMU:
  +--
  +
  + iommu {
  + /* the specifier represents the ID of the master */
  + #iommu-cells = 1;
  + };
  +
  + master@1 {
  + /* device has master ID 42 in the IOMMU */
  + iommus = /iommu 42;
  + };
  +
  + master@2 {
  + /* device has master IDs 23 and 24 in the IOMMU */
  + iommus = /iommu 23, /iommu 24;
  + };
 
  In future I suspect master will need to be able to identify which master
  IDs correspond to which of their master ports (where each port might
  have an arbitrary number of master IDs).
 
  While we don't need that for the first run, it would be nice to have
  that looked into so master bindings don't come up with arbitrarily
  different ways of doing that.
 
 iommu-names would be the logical extension to handle that, just like
 we do with other resources, right?

Possibly. If the master has multiple IDs assigned to transactions from a
single master port then it depends on how the master wants to group
those for the sake of the binding. If it's per-port then you'd need the
same name multiple times:

iommus = iommu 0, iommu 4, iommu 17, iommu 25;
iommu-names = video, video, dram, dram;

This is really specific to a given master, so we can table that until
the first master appears which needs to distinguish between IDs.

  +
  +Multiple-master IOMMU with configurable DMA window:
  +---
  +
  + / {
  + #address-cells = 1;
  + #size-cells = 1;
  +
  + iommu {
  + /* master ID, address and length of DMA window */
  + #iommu-cells = 4;
  + };
  +
  + master {
  + /* master ID 42, 4 GiB DMA window starting at 0 */
  + iommus = /iommu  42  0  0x1 0x0;
 
  Is this that window is from the POV of the master, i.e. the master can
  address 0x0 to 0x when generating transactions, and these get
  translated somehow?
 
  Or is this the physical addresses to allocate to the master?
 
 It needs to be clarified in the documentation, but as far as I know it
 is the DMA address space that is used.

Ok. So that's pre-translation, from the POV of the master?

If we don't have that knowledge about the master already (e.g. based on
the compatible string), surely we always need that information in a
given iommu-specifier format? Otherwise certain iommus won't be able to
handle masters with limited addressing only due to limitations of their
binding.

 It is somewhat confusing to have size-cells = 1 and then use 2 cells
 for size in the iommu property. It's legal and expected, but having
 size-cells in the example adds a little confusion.
 
 Either way, I'm OK with fixing the above with an incremental patch,
 assuming there is no disagreements on what's said above.

I like the general idea.

Given my concerns are to do with implementation details I'm happy to
have this go through and fix it up as the first implementations of the
binding take shape.

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

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get nvidia,swgroups from DT

[Mark Rutland]

On Fri, Oct 18, 2013 at 11:26:51AM +0100, Hiroshi Doyu wrote:
 This provides the info about which H/W Accelerators are supported on
 Tegra SoC. This info is passed from DT. This is necessary to have the
 unified SMMU driver among Tegra SoCs. Instead of using platform data,
 DT passes nvidia,swgroups now. DT is mandatory in Tegra.
 
 Signed-off-by: Hiroshi Doyu hd...@nvidia.com
 ---
  .../bindings/iommu/nvidia,tegra30-smmu.txt |  6 +
  drivers/iommu/tegra-smmu.c | 31 
 ++
  2 files changed, 20 insertions(+), 17 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt 
 b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
 index 89fb543..6a844b3 100644
 --- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
 +++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
 @@ -8,6 +8,11 @@ Required properties:
  - nvidia,#asids : # of ASIDs
  - dma-window : IOVA start address and length.
  - nvidia,ahb : phandle to the ahb bus connected to SMMU.
 +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA).
 +  Each bit represents one swgroup. The assignments may be found in header
 +  file dt-bindings/memory/tegra-swgroup.h. Its max is 64. 2 cells
 +  are required. This unique ID info can be used to calculate
 +  MC_SMMU_SWGROUP name_ASID_0 offset and HOTRESET bit.

I'd prefer that you pointed out the type of the property explicitly at the
beginning of the description. It's difficult to spot it in the middle, and
while the current description defines that the property is two cells, it
doesn't point out that they are expected to form a (big endian) u64 (rather
than an array of two u32s).

While we don't yet have a formal set of rules for consistent binding
documentation, defining the explicitly is one of the requirements I'd like to
push for in new bindings. Placing this at the start of the description makes it
far easier to skim a binding document and perform basic sanity checks of the
binding itself, dts using the binding, and kernel code parsing it.

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

Re: [PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Mark Rutland]

On Thu, Oct 31, 2013 at 05:11:26PM +, Hiroshi Doyu wrote:
 Hiroshi Doyu hd...@nvidia.com wrote @ Thu, 31 Oct 2013 19:05:24 +0200 (EET):
 
  Hi Mark,
  
  Mark Rutland mark.rutl...@arm.com wrote @ Thu, 31 Oct 2013 17:51:27 +0100:
  
   On Fri, Oct 18, 2013 at 11:26:46AM +0100, Hiroshi Doyu wrote:
Fix IOMMU register address.

Signed-off-by: Hiroshi Doyu hd...@nvidia.com
---
 arch/arm/boot/dts/tegra114.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi 
b/arch/arm/boot/dts/tegra114.dtsi
index 2905145..8d42787 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -318,9 +318,9 @@
 
iommu {
compatible = nvidia,tegra114-smmu, 
nvidia,tegra30-smmu;
-   reg = 0x7000f010 0x02c
-  0x7000f1f0 0x010
-  0x7000f228 0x074;
+   reg = 0x70019010 0x02c
+  0x700191f0 0x010
+  0x70019228 0x074;
   
   Minor cleanup/consistency request: as all these lines are changing anyway,
   would you mind bracketing each entry individually:
   
 reg = 0x70019010 0x02c,
   0x700191f0 0x010,
   0x70019228 0x074;
  
  No, at all. Attached the updated [PATCHv3' 05/19].
 
 Oops, here's the right one, [PATCHv3'' 05/19].

:)

Acked-by: Mark Rutland mark.rutl...@arm.com

Cheers,
Mark.

 From 156a6ad059f902590094570697cb33e5b892d835 Mon Sep 17 00:00:00 2001
 From: Hiroshi Doyu hd...@nvidia.com
 Date: Mon, 17 Jun 2013 15:37:35 +0300
 Subject: [PATCHv3'' 1/1] ARM: dt: tegra114: iommu: Fix IOMMU register address
 
 Fix IOMMU register address.
 
 Signed-off-by: Hiroshi Doyu hd...@nvidia.com
 ---
  arch/arm/boot/dts/tegra114.dtsi | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
 index 2905145..e629042 100644
 --- a/arch/arm/boot/dts/tegra114.dtsi
 +++ b/arch/arm/boot/dts/tegra114.dtsi
 @@ -318,9 +318,9 @@
  
   iommu {
   compatible = nvidia,tegra114-smmu, nvidia,tegra30-smmu;
 - reg = 0x7000f010 0x02c
 -0x7000f1f0 0x010
 -0x7000f228 0x074;
 + reg = 0x70019010 0x02c,
 +   0x700191f0 0x010,
 +   0x70019228 0x074;
   nvidia,#asids = 4;
   dma-window = 0 0x4000;
   nvidia,swgroups = 0x18659fe;
 -- 
 1.8.1.5
 

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

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get nvidia,memory-clients from DT

[Mark Rutland]

On Thu, Oct 31, 2013 at 08:18:08AM +, Hiroshi Doyu wrote:
 Stephen Warren swar...@wwwdotorg.org wrote @ Wed, 30 Oct 2013 23:44:04 
 +0100:
 
   + host1x {
   + compatible = nvidia,tegra30-host1x, simple-bus;
   + nvidia,memory-clients = smmu TEGRA_SWGROUP_HC;
   + 
   + gr3d {
   + compatible = nvidia,tegra30-gr3d;
   + nvidia,memory-clients = smmu TEGRA_SWGROUP_NV
   +TEGRA_SWGROUP_NV2;
  
  Why one cell for the host1x property, and two cells for the gr3d
  property; shouldn't they be the same length?
 
 That can vary. Even a single device node in DT can belong to multiple
 software groups.

Same with ARM SMMU.

 
   diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
  
   +static u64 smmu_of_get_memory_client(struct device *dev)
  
   + np = of_parse_phandle(dev-of_node, propname, 0);
   + if (np != smmu_handle-dev-of_node)
   + return ~0;
   +
   + prop = of_get_property(dev-of_node, propname, bytes);
   + if (!prop || !bytes)
   + return ~0;
  
  of_parse_phandle_with_fixed_args() might be a good fit here, or perhaps
  require a property #smmu-cells in the SMMU node, thus allowing the
  non-fixed-length of_parse_phandle_with_args() to be used here.
 
 I once considered of_parse_phandle_with_args() but I couldn't use that
 since #smmu-cells needed to be fixed in smmu node, not in client
 node. In this case, the number of argument(number of swgroup IDs)
 varies per client device.

For the ARM SMMU binding, each device has a #stream-id-cells property
describing how many IDs it has, and then the SMMU node has a phandle+args
linkage to each of the devices attached to it, describing their stream IDs.
While this does have some limitations (a device can't be plugged into multiple
SMMUs with a different number of IDs), it seems generally sane.

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

Re: [PATCH 2/3] Initial skeleton of VFIO support for Device Tree based devices

[Mark Rutland]

   +static const struct of_device_id vfio_dt_match[] = {
   + � � /* In the future, we can implement a better mechanism to bind
  the
   + � � �* module to any device. For now add the compatible property to
  the
   + � � �* dtb of the devices we want to use. � */
   + � � {
   + � � � � � � .compatible = vfio-dt,
   + � � },
   + � � {},
   +};
 
  This definitely doesn't belong in the dt. It's purely a Linux
  abstraction and does not represent a piece of hardware or common
  interface.
 
  We need to think of a better mechanism for binding the module to these
  devices now.
 
I already make this remark in the cover letter; thanks for confirming it.
�
Antonios

Sorry, I found the cover letter a little unclear in that regard.

Thanks for the clarification :)

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