Re: [PATCH 2/4] iommu: add ARM LPAE page table allocator

[Will Deacon]

On Tue, Dec 02, 2014 at 11:47:36AM +, Laurent Pinchart wrote:
> On Tuesday 02 December 2014 09:41:56 Will Deacon wrote:
> > On Mon, Dec 01, 2014 at 08:21:58PM +, Laurent Pinchart wrote:
> > > On Monday 01 December 2014 17:23:15 Will Deacon wrote:
> > > > On Sun, Nov 30, 2014 at 11:29:46PM +, Laurent Pinchart wrote:
> > > > > On Thursday 27 November 2014 11:51:16 Will Deacon wrote:
> > > > > > + /* Looking good; allocate a pgd */
> > > > > > + data->pgd = alloc_pages_exact(1UL << data->pg_shift,
> > > > > > +   GFP_KERNEL | __GFP_ZERO);
> > > > > 
> > > > > data->pg_shift is computed as __ffs(cfg->pgsize_bitmap). 1UL <<
> > > > > data->pg_shift will thus be equal to the smallest page size supported
> > > > > by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on
> > > > > the IOMMU configuration. However, if I'm not mistaken the top-level
> > > > > directory needs to store one entry per largest supported page size.
> > > > > That's 4, 128 or 8 entries depending on the configuration. You're thus
> > > > > over-allocating.
> > > > 
> > > > Yeah, I'll take a closer look at this. The size of the pgd really
> > > > depends on the TxSZ configuration, which in turn depends on the ias and
> > > > the page size. There are also alignment constraints to bear in mind, but
> > > > I'll see what I can do (as it stands, over-allocating will always work).
> > > 
> > > Beside wasting memory, the code also doesn't reflect the requirements. It
> > > works by chance, meaning it could break later.
> > 
> > It won't break, as the maximum size *is* bounded by a page for stage-1
> > and we already handle stage-2 concatenation correctly.
> 
> What I mean is that there's no correlation between the required size and the 
> allocated size in the current code. It happens to work, but if the driver 
> gets 
> extended later to support more IOMMU configurations subtle bugs may crop up.
> 
> > > That's why I'd like to see this
> > > being fixed. Can't the size be computed with something like
> > > 
> > >   size = (1 << (ias - data->levels * data->pg_shift))
> > >   
> > >* sizeof(arm_lpae_iopte);
> > > 
> > > (please add a proper detailed comment to explain the computation, as the
> > > meaning is not straightforward)
> > 
> > That's actually the easy part. The harder part is getting the correct
> > alignment, which means managing by own kmem_cache on a per-ops basis. That
> > feels like overkill to me and we also need to make sure that we don't screw
> > up the case of concatenated pgds at stage-2. On top of that, since each
> > cache would be per-ops, I'm not even sure we'd save anything (the slab
> > allocators all operate on pages afaict).
> > 
> > If I use alloc_page_exact, we'll still have some wasteage, but it would
> > be less for the case where the CPU page size is smaller than the SMMU page
> > size. Do you think that's worth the extra complexity? We allocate full pages
> > at all levels after the pgd, so the wasteage is relatively small.
> > 
> > An alternative would be preinitialising some caches for `likely' pgd sizes,
> > but that's also horrible, especially if the kernel decides that it doesn't
> > need a bunch of the configurations at runtime.
> 
> How about just computing the right size, align it to a page size, and using 
> alloc_page_exact ? The waste is small, so it doesn't justify anything more 
> complex than that.

Ok, I'll have a go at that.

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

Re: [PATCH 2/4] iommu: add ARM LPAE page table allocator

[Will Deacon]

On Fri, Dec 05, 2014 at 10:55:11AM +, Varun Sethi wrote:
> Hi Will,

Hi Varun,

Thanks for the review!

> +static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
> +unsigned long iova, phys_addr_t paddr,
> +arm_lpae_iopte prot, int lvl,
> +arm_lpae_iopte *ptep)
> +{
> +   arm_lpae_iopte pte = prot;
> +
> +   /* We require an unmap first */
> +   if (iopte_leaf(*ptep, lvl))
> +   return -EEXIST;
> [varun] Instead of returning an error, how about displaying a warning and
> replacing the entry?

I'd be ok with displaying a warning, but I don't think we should just
continue. It indicates a misuse of the IOMMU API and probably a missing
TLBI.

> +static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long 
> iova,
> + phys_addr_t paddr, size_t size, arm_lpae_iopte prot,
> + int lvl, arm_lpae_iopte *ptep)
> +{
> +   arm_lpae_iopte *cptep, pte;
> +   void *cookie = data->iop.cookie;
> +   size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
> +
> +   /* Find our entry at the current level */
> +   ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
> +
> +   /* If we can install a leaf entry at this level, then do so */
> +   if (size == block_size && (size & data->iop.cfg.pgsize_bitmap))
> +   return arm_lpae_init_pte(data, iova, paddr, prot, lvl, ptep);
> +
> +   /* We can't allocate tables at the final level */
> +   if (WARN_ON(lvl >= ARM_LPAE_MAX_LEVELS - 1))
> +   return -EINVAL;
> 
> [varun] A warning message would be helpful.

Sure, I can add one.

> +static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
> +  int prot)
> +{
> +   arm_lpae_iopte pte;
> +
> +   if (data->iop.fmt == ARM_LPAE_S1) {
> +   pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
> +
> +   if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
> +   pte |= ARM_LPAE_PTE_AP_RDONLY;
> +
> +   if (prot & IOMMU_CACHE)
> +   pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
> +   << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> +   } else {
> +   pte = ARM_LPAE_PTE_HAP_FAULT;
> +   if (prot & IOMMU_READ)
> +   pte |= ARM_LPAE_PTE_HAP_READ;
> +   if (prot & IOMMU_WRITE)
> +   pte |= ARM_LPAE_PTE_HAP_WRITE;
> +   if (prot & IOMMU_CACHE)
> +   pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
> +   else
> +   pte |= ARM_LPAE_PTE_MEMATTR_NC;
> +   }
> +
> +   if (prot & IOMMU_NOEXEC)
> +   pte |= ARM_LPAE_PTE_XN;
> +
> +   return pte;
> +}
> [[varun]] Do you plan to add a flag to indicate device memory? We had a
> discussion about this on the patch submitted by me. May be you can include
> that as a part of this patch.

That needs to go in as a separate patch. I think you should continue to push
that separately!

> +static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
> +   unsigned long iova, size_t size,
> +   arm_lpae_iopte prot, int lvl,
> +   arm_lpae_iopte *ptep, size_t blk_size) {
> +   unsigned long blk_start, blk_end;
> +   phys_addr_t blk_paddr;
> +   arm_lpae_iopte table = 0;
> +   void *cookie = data->iop.cookie;
> +   struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> +
> +   blk_start = iova & ~(blk_size - 1);
> +   blk_end = blk_start + blk_size;
> +   blk_paddr = iopte_to_pfn(*ptep, data) << data->pg_shift;
> +
> +   for (; blk_start < blk_end; blk_start += size, blk_paddr += size) {
> +   arm_lpae_iopte *tablep;
> +
> +   /* Unmap! */
> +   if (blk_start == iova)
> +   continue;
> +
> +   /* __arm_lpae_map expects a pointer to the start of the table 
> */
> +   tablep = &table - ARM_LPAE_LVL_IDX(blk_start, lvl, data);
> 
> 
> [[varun]] Not clear what's happening here. May be I am missing something,
> but where is the table allocated?

It is allocated in __arm_lpae_map, because the pte will be 0. The
subtraction above is to avoid us having to allocate a whole level, just for
a single invalid pte.
> 
> +   if (__arm_lpae_map(data, blk_start, blk_paddr, size, prot, 
> lvl,
> +  tablep) < 0) {
> 
> 
> [[varun]] Again not clear how are we unmapping the range. Index at the
> current level should point to a page table (with contiguous block
> mappings). Unmap would applied to the mappings at the next level. Unmap
> can happen anywhere in the contiguous range. It seems that you are just
> creating a subset of the block mapping.

We will be unmapping a s

Re: [GIT PULL] Automatic DMA configuration for OF-based IOMMU masters

[Arnd Bergmann]

On Friday 05 December 2014 16:35:06 Will Deacon wrote:
> Hi Olof,
> 
> Please consider pulling the following series for 3.19. I appreciate that
> this is late in the day, but the patches have been around for a while and
> have collected all the Acks that they need. Marek has already been finding
> the series useful with the Exynos IOMMU, so it seems a pity to hold the
> patches up any longer.
> 

I've pulled it into a separate next/iommu-config branch in case we have
to take it out again for some reason. The autobuilder has not found any
regressions, so that's a good sign.

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

[GIT PULL] Automatic DMA configuration for OF-based IOMMU masters

[Will Deacon]

Hi Olof,

Please consider pulling the following series for 3.19. I appreciate that
this is late in the day, but the patches have been around for a while and
have collected all the Acks that they need. Marek has already been finding
the series useful with the Exynos IOMMU, so it seems a pity to hold the
patches up any longer.

Cheers,

Will

--->8

The following changes since commit 5d01410fe4d92081f349b013a2e7a95429e4f2c9:

  Linux 3.18-rc6 (2014-11-23 15:25:20 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git 
tags/of-iommu-configure

for you to fetch changes up to a42a7a1fb5f1f9004b023594609dc22da02fc08b:

  iommu: store DT-probed IOMMU data privately (2014-12-05 14:35:52 +)


This series adds automatic IOMMU and DMA-mapping configuration for
OF-based DMA masters described using the generic IOMMU devicetree
bindings. Although there is plenty of future work around splitting up
iommu_ops, adding default IOMMU domains and sorting out automatic IOMMU
group creation for the platform_bus, this is already useful enough for
people to port over their IOMMU drivers and start using the new probing
infrastructure (indeed, Marek has patches queued for the Exynos IOMMU).


Marek Szyprowski (1):
  iommu: fix initialization without 'add_device' callback

Robin Murphy (1):
  iommu: store DT-probed IOMMU data privately

Will Deacon (7):
  iommu: provide early initialisation hook for IOMMU drivers
  dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
  iommu: add new iommu_ops callback for adding an OF device
  iommu: provide helper function to configure an IOMMU for an of master
  dma-mapping: detect and configure IOMMU in of_dma_configure
  arm: call iommu_init before of_platform_populate
  arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

 arch/arm/include/asm/dma-mapping.h | 12 ++---
 arch/arm/kernel/setup.c|  2 +
 arch/arm/mm/dma-mapping.c  | 83 ---
 drivers/iommu/Kconfig  |  2 +-
 drivers/iommu/iommu.c  |  2 +-
 drivers/iommu/of_iommu.c   | 89 ++
 drivers/of/platform.c  | 50 +++--
 include/asm-generic/vmlinux.lds.h  |  2 +
 include/linux/dma-mapping.h| 13 +++---
 include/linux/iommu.h  |  8 
 include/linux/of_iommu.h   | 23 ++
 11 files changed, 242 insertions(+), 44 deletions(-)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Marek Szyprowski]

Hello,

On 2014-12-05 14:18, Thierry Reding wrote:

On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote:

On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy  wrote:

Hi Will,

On 05/12/14 12:10, Will Deacon wrote:
[...]

Do you expect drivers to modify that *priv pointer after the ops
structure is registered? I'd be very surprised if that was the use
case. It's fine for the driver to register a non-const version, but
once it is registered, the infrastructure can treat it as const from
then on.


Possibly not - certainly my current port of the ARM SMMU which makes use
of *priv is only ever reading it - although we did also wave around
reasons for mutable ops like dynamically changing the pgsize_bitmap and
possibly even swizzling individual ops for runtime reconfiguration. On
consideration though, I'd agree that things like that are mad enough to
stay well within individual drivers if they did ever happen, and
certainly shouldn't apply to this bit of the infrastructure at any rate.


I certainly need to update the pgsize_bitmap at runtime because I don't
know the supported page sizes until I've both (a) probed the hardware
and (b) allocated page tables for a domain. We've already discussed
moving the pgsize_bitmap out of the ops, but moving it somewhere where
it remains const doesn't really help.


We can safely cast the call to get_ops in the SMMU driver though, since
we'll know that we put a mutable per-instance ops in there in the first
place. At least that way drivers that aren't taking advantage and just pass
their static const ops around shouldn't provoke warnings. I deliberately
didn't touch anything beyond get_ops as that would be too disruptive.


Can I just take the patch that Grant acked, in the interest of getting
something merged? As you say, there's plenty of planned changes in this
area anyway. I plan to send Olof a pull request this afternoon.


Grant, Thierry? Personally I'm not fussed either way - the sooner something
goes in, the sooner I can carry on working at replacing it :D

I've already acked it. Why are we still talking about it?  :-D

Am I missing something? Why is there a need to rush things? Are there
actually drivers that depend on this that will be merged during the 3.19
merge window? It seems like that'd be cutting it really close given
where we are in the release cycle.

If that's not the case, why even bother getting this hack into 3.19 if
nobody uses it and we're going to change it in 3.20 anyway?


There are Exynos SYSMMU patches ready & waiting for this gets merged...

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Thierry Reding]

On Fri, Dec 05, 2014 at 01:21:31PM +, Grant Likely wrote:
> On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding  
> wrote:
> > On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote:
> >> On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy  wrote:
> >> > Hi Will,
> >> >
> >> > On 05/12/14 12:10, Will Deacon wrote:
> >> > [...]
> >> 
> >>  Do you expect drivers to modify that *priv pointer after the ops
> >>  structure is registered? I'd be very surprised if that was the use
> >>  case. It's fine for the driver to register a non-const version, but
> >>  once it is registered, the infrastructure can treat it as const from
> >>  then on.
> >> >>>
> >> >>>
> >> >>> Possibly not - certainly my current port of the ARM SMMU which makes 
> >> >>> use
> >> >>> of *priv is only ever reading it - although we did also wave around
> >> >>> reasons for mutable ops like dynamically changing the pgsize_bitmap and
> >> >>> possibly even swizzling individual ops for runtime reconfiguration. On
> >> >>> consideration though, I'd agree that things like that are mad enough to
> >> >>> stay well within individual drivers if they did ever happen, and
> >> >>> certainly shouldn't apply to this bit of the infrastructure at any 
> >> >>> rate.
> >> >>
> >> >>
> >> >> I certainly need to update the pgsize_bitmap at runtime because I don't
> >> >> know the supported page sizes until I've both (a) probed the hardware
> >> >> and (b) allocated page tables for a domain. We've already discussed
> >> >> moving the pgsize_bitmap out of the ops, but moving it somewhere where
> >> >> it remains const doesn't really help.
> >> >
> >> >
> >> > We can safely cast the call to get_ops in the SMMU driver though, since
> >> > we'll know that we put a mutable per-instance ops in there in the first
> >> > place. At least that way drivers that aren't taking advantage and just 
> >> > pass
> >> > their static const ops around shouldn't provoke warnings. I deliberately
> >> > didn't touch anything beyond get_ops as that would be too disruptive.
> >> >
> >> >> Can I just take the patch that Grant acked, in the interest of getting
> >> >> something merged? As you say, there's plenty of planned changes in this
> >> >> area anyway. I plan to send Olof a pull request this afternoon.
> >> >
> >> >
> >> > Grant, Thierry? Personally I'm not fussed either way - the sooner 
> >> > something
> >> > goes in, the sooner I can carry on working at replacing it :D
> >>
> >> I've already acked it. Why are we still talking about it?  :-D
> >
> > Am I missing something? Why is there a need to rush things? Are there
> > actually drivers that depend on this that will be merged during the 3.19
> > merge window? It seems like that'd be cutting it really close given
> > where we are in the release cycle.
> >
> > If that's not the case, why even bother getting this hack into 3.19 if
> > nobody uses it and we're going to change it in 3.20 anyway?
> 
> I also acked the non-hack version, the patch that doesn't try to make
> everything const. I assumed that was the one that we are talking about
> merging.

Actually not making everything const would be a hack. Drivers already
mark their struct iommu_ops as const.

But I'm more referring to the series as a whole. It seems like there are
various issues that still need to be ironed out, and there's committment
to do that before 3.20, so unless there are drivers that need any of the
unfinished patches for 3.19 I don't see why we should be merging them in
the first place.

If getting them into 3.19 is merely to resolve dependencies then it's
not going to work well anyway. Since this is all going to change in 3.20
anyway we'd likely have new dependencies that need to be handled, so
might just as well do it properly at that time.

Thierry


pgp7c9IuvJxq5.pgp
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Grant Likely]

On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding  wrote:
> On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote:
>> On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy  wrote:
>> > Hi Will,
>> >
>> > On 05/12/14 12:10, Will Deacon wrote:
>> > [...]
>> 
>>  Do you expect drivers to modify that *priv pointer after the ops
>>  structure is registered? I'd be very surprised if that was the use
>>  case. It's fine for the driver to register a non-const version, but
>>  once it is registered, the infrastructure can treat it as const from
>>  then on.
>> >>>
>> >>>
>> >>> Possibly not - certainly my current port of the ARM SMMU which makes use
>> >>> of *priv is only ever reading it - although we did also wave around
>> >>> reasons for mutable ops like dynamically changing the pgsize_bitmap and
>> >>> possibly even swizzling individual ops for runtime reconfiguration. On
>> >>> consideration though, I'd agree that things like that are mad enough to
>> >>> stay well within individual drivers if they did ever happen, and
>> >>> certainly shouldn't apply to this bit of the infrastructure at any rate.
>> >>
>> >>
>> >> I certainly need to update the pgsize_bitmap at runtime because I don't
>> >> know the supported page sizes until I've both (a) probed the hardware
>> >> and (b) allocated page tables for a domain. We've already discussed
>> >> moving the pgsize_bitmap out of the ops, but moving it somewhere where
>> >> it remains const doesn't really help.
>> >
>> >
>> > We can safely cast the call to get_ops in the SMMU driver though, since
>> > we'll know that we put a mutable per-instance ops in there in the first
>> > place. At least that way drivers that aren't taking advantage and just pass
>> > their static const ops around shouldn't provoke warnings. I deliberately
>> > didn't touch anything beyond get_ops as that would be too disruptive.
>> >
>> >> Can I just take the patch that Grant acked, in the interest of getting
>> >> something merged? As you say, there's plenty of planned changes in this
>> >> area anyway. I plan to send Olof a pull request this afternoon.
>> >
>> >
>> > Grant, Thierry? Personally I'm not fussed either way - the sooner something
>> > goes in, the sooner I can carry on working at replacing it :D
>>
>> I've already acked it. Why are we still talking about it?  :-D
>
> Am I missing something? Why is there a need to rush things? Are there
> actually drivers that depend on this that will be merged during the 3.19
> merge window? It seems like that'd be cutting it really close given
> where we are in the release cycle.
>
> If that's not the case, why even bother getting this hack into 3.19 if
> nobody uses it and we're going to change it in 3.20 anyway?

I also acked the non-hack version, the patch that doesn't try to make
everything const. I assumed that was the one that we are talking about
merging.

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

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Thierry Reding]

On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote:
> On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy  wrote:
> > Hi Will,
> >
> > On 05/12/14 12:10, Will Deacon wrote:
> > [...]
> 
>  Do you expect drivers to modify that *priv pointer after the ops
>  structure is registered? I'd be very surprised if that was the use
>  case. It's fine for the driver to register a non-const version, but
>  once it is registered, the infrastructure can treat it as const from
>  then on.
> >>>
> >>>
> >>> Possibly not - certainly my current port of the ARM SMMU which makes use
> >>> of *priv is only ever reading it - although we did also wave around
> >>> reasons for mutable ops like dynamically changing the pgsize_bitmap and
> >>> possibly even swizzling individual ops for runtime reconfiguration. On
> >>> consideration though, I'd agree that things like that are mad enough to
> >>> stay well within individual drivers if they did ever happen, and
> >>> certainly shouldn't apply to this bit of the infrastructure at any rate.
> >>
> >>
> >> I certainly need to update the pgsize_bitmap at runtime because I don't
> >> know the supported page sizes until I've both (a) probed the hardware
> >> and (b) allocated page tables for a domain. We've already discussed
> >> moving the pgsize_bitmap out of the ops, but moving it somewhere where
> >> it remains const doesn't really help.
> >
> >
> > We can safely cast the call to get_ops in the SMMU driver though, since
> > we'll know that we put a mutable per-instance ops in there in the first
> > place. At least that way drivers that aren't taking advantage and just pass
> > their static const ops around shouldn't provoke warnings. I deliberately
> > didn't touch anything beyond get_ops as that would be too disruptive.
> >
> >> Can I just take the patch that Grant acked, in the interest of getting
> >> something merged? As you say, there's plenty of planned changes in this
> >> area anyway. I plan to send Olof a pull request this afternoon.
> >
> >
> > Grant, Thierry? Personally I'm not fussed either way - the sooner something
> > goes in, the sooner I can carry on working at replacing it :D
> 
> I've already acked it. Why are we still talking about it?  :-D

Am I missing something? Why is there a need to rush things? Are there
actually drivers that depend on this that will be merged during the 3.19
merge window? It seems like that'd be cutting it really close given
where we are in the release cycle.

If that's not the case, why even bother getting this hack into 3.19 if
nobody uses it and we're going to change it in 3.20 anyway?

Thierry


pgpP_DLSxlZYQ.pgp
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Grant Likely]

On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy  wrote:
> Hi Will,
>
> On 05/12/14 12:10, Will Deacon wrote:
> [...]

 Do you expect drivers to modify that *priv pointer after the ops
 structure is registered? I'd be very surprised if that was the use
 case. It's fine for the driver to register a non-const version, but
 once it is registered, the infrastructure can treat it as const from
 then on.
>>>
>>>
>>> Possibly not - certainly my current port of the ARM SMMU which makes use
>>> of *priv is only ever reading it - although we did also wave around
>>> reasons for mutable ops like dynamically changing the pgsize_bitmap and
>>> possibly even swizzling individual ops for runtime reconfiguration. On
>>> consideration though, I'd agree that things like that are mad enough to
>>> stay well within individual drivers if they did ever happen, and
>>> certainly shouldn't apply to this bit of the infrastructure at any rate.
>>
>>
>> I certainly need to update the pgsize_bitmap at runtime because I don't
>> know the supported page sizes until I've both (a) probed the hardware
>> and (b) allocated page tables for a domain. We've already discussed
>> moving the pgsize_bitmap out of the ops, but moving it somewhere where
>> it remains const doesn't really help.
>
>
> We can safely cast the call to get_ops in the SMMU driver though, since
> we'll know that we put a mutable per-instance ops in there in the first
> place. At least that way drivers that aren't taking advantage and just pass
> their static const ops around shouldn't provoke warnings. I deliberately
> didn't touch anything beyond get_ops as that would be too disruptive.
>
>> Can I just take the patch that Grant acked, in the interest of getting
>> something merged? As you say, there's plenty of planned changes in this
>> area anyway. I plan to send Olof a pull request this afternoon.
>
>
> Grant, Thierry? Personally I'm not fussed either way - the sooner something
> goes in, the sooner I can carry on working at replacing it :D

I've already acked it. Why are we still talking about it?  :-D

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

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Robin Murphy]

Hi Will,

On 05/12/14 12:10, Will Deacon wrote:
[...]

Do you expect drivers to modify that *priv pointer after the ops
structure is registered? I'd be very surprised if that was the use
case. It's fine for the driver to register a non-const version, but
once it is registered, the infrastructure can treat it as const from
then on.


Possibly not - certainly my current port of the ARM SMMU which makes use
of *priv is only ever reading it - although we did also wave around
reasons for mutable ops like dynamically changing the pgsize_bitmap and
possibly even swizzling individual ops for runtime reconfiguration. On
consideration though, I'd agree that things like that are mad enough to
stay well within individual drivers if they did ever happen, and
certainly shouldn't apply to this bit of the infrastructure at any rate.


I certainly need to update the pgsize_bitmap at runtime because I don't
know the supported page sizes until I've both (a) probed the hardware
and (b) allocated page tables for a domain. We've already discussed
moving the pgsize_bitmap out of the ops, but moving it somewhere where
it remains const doesn't really help.


We can safely cast the call to get_ops in the SMMU driver though, since 
we'll know that we put a mutable per-instance ops in there in the first 
place. At least that way drivers that aren't taking advantage and just 
pass their static const ops around shouldn't provoke warnings. I 
deliberately didn't touch anything beyond get_ops as that would be too 
disruptive.



Can I just take the patch that Grant acked, in the interest of getting
something merged? As you say, there's plenty of planned changes in this
area anyway. I plan to send Olof a pull request this afternoon.


Grant, Thierry? Personally I'm not fussed either way - the sooner 
something goes in, the sooner I can carry on working at replacing it :D


Robin.

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

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Arnd Bergmann]

On Friday 05 December 2014 12:10:37 Will Deacon wrote:
> On Thu, Dec 04, 2014 at 07:42:28PM +, Robin Murphy wrote:
> > On 04/12/14 17:58, Grant Likely wrote:
> > [...]
> >  +struct of_iommu_node {
> >  +   struct list_head list;
> >  +   struct device_node *np;
> >  +   struct iommu_ops *ops;
> > >>>
> > >>>
> > >>> Why can't this be const? Why would anyone ever need to modify it? Also
> > >>> drivers do define their iommu_ops structures const these days, so you'll
> > >>> either still have to cast away at some point or the compiler will
> > >>> complain once you start calling this from drivers.
> > >>>
> > >>
> > >> ...whereas if we make everything const then the drivers that _do_ want to
> > >> use the private data introduced by this series and thus pass mutable
> > >> per-instance iommu_ops will be the ones having to cast. We have no users
> > >> either way until this series is merged, and the need to stash the
> > >> per-instance data somewhere other than np->data is in the way of getting 
> > >> it
> > >> merged - this is just a quick hack to address that. I think we've already
> > >> agreed that mutable per-instance iommu_ops holding private data aren't
> > >> particularly pleasant and will (hopefully) go away in the next 
> > >> iteration[1],
> > >> at which point this all changes anyway.
> > >
> > > Do you expect drivers to modify that *priv pointer after the ops
> > > structure is registered? I'd be very surprised if that was the use
> > > case. It's fine for the driver to register a non-const version, but
> > > once it is registered, the infrastructure can treat it as const from
> > > then on.
> > 
> > Possibly not - certainly my current port of the ARM SMMU which makes use 
> > of *priv is only ever reading it - although we did also wave around 
> > reasons for mutable ops like dynamically changing the pgsize_bitmap and 
> > possibly even swizzling individual ops for runtime reconfiguration. On 
> > consideration though, I'd agree that things like that are mad enough to 
> > stay well within individual drivers if they did ever happen, and 
> > certainly shouldn't apply to this bit of the infrastructure at any rate.
> 
> I certainly need to update the pgsize_bitmap at runtime because I don't
> know the supported page sizes until I've both (a) probed the hardware
> and (b) allocated page tables for a domain. We've already discussed
> moving the pgsize_bitmap out of the ops, but moving it somewhere where
> it remains const doesn't really help.
> 
> Can I just take the patch that Grant acked, in the interest of getting
> something merged? As you say, there's plenty of planned changes in this
> area anyway. I plan to send Olof a pull request this afternoon.
> 

I think that would be ok. The fix later should be to move the
private data pointer into of_iommu_node, but I think that will
require a larger set of changes, so let's defer that.

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

Re: [PATCH v6 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

On Fri, Dec 05, 2014 at 07:12:24AM +, Olof Johansson wrote:
> On Mon, Dec 1, 2014 at 8:57 AM, Will Deacon  wrote:
> > Hello again,
> >
> > This is version 6 of the patches previously posted here:
> >
> >   RFCv1: 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
> >   RFCv2: 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
> >   RFCv3: 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
> >   RFCv4: 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/302711.html
> >  v5: 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/307213.html
> >
> > The only change since v5 is the addition of acks from various maintainers.
> >
> > Now that the ARM bits have rmk's ack and the IOMMU bits have joro's ack,
> > I think this is good for merging via the arm-soc tree.
> >
> > Please let me know if a pull request would be preferable.
> 
> 
> Hi,
> 
> Yes, please collect the acks from the discussion in the last day or so
> and send a pull request (and include Robin's patch, obviously).

Sure, I'm just working out which of Robin's patches to take (if I don't hear
otherwise, it will be the one with Grant's ack on it).

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

Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Will Deacon]

On Thu, Dec 04, 2014 at 07:42:28PM +, Robin Murphy wrote:
> On 04/12/14 17:58, Grant Likely wrote:
> [...]
>  +struct of_iommu_node {
>  +   struct list_head list;
>  +   struct device_node *np;
>  +   struct iommu_ops *ops;
> >>>
> >>>
> >>> Why can't this be const? Why would anyone ever need to modify it? Also
> >>> drivers do define their iommu_ops structures const these days, so you'll
> >>> either still have to cast away at some point or the compiler will
> >>> complain once you start calling this from drivers.
> >>>
> >>
> >> ...whereas if we make everything const then the drivers that _do_ want to
> >> use the private data introduced by this series and thus pass mutable
> >> per-instance iommu_ops will be the ones having to cast. We have no users
> >> either way until this series is merged, and the need to stash the
> >> per-instance data somewhere other than np->data is in the way of getting it
> >> merged - this is just a quick hack to address that. I think we've already
> >> agreed that mutable per-instance iommu_ops holding private data aren't
> >> particularly pleasant and will (hopefully) go away in the next 
> >> iteration[1],
> >> at which point this all changes anyway.
> >
> > Do you expect drivers to modify that *priv pointer after the ops
> > structure is registered? I'd be very surprised if that was the use
> > case. It's fine for the driver to register a non-const version, but
> > once it is registered, the infrastructure can treat it as const from
> > then on.
> 
> Possibly not - certainly my current port of the ARM SMMU which makes use 
> of *priv is only ever reading it - although we did also wave around 
> reasons for mutable ops like dynamically changing the pgsize_bitmap and 
> possibly even swizzling individual ops for runtime reconfiguration. On 
> consideration though, I'd agree that things like that are mad enough to 
> stay well within individual drivers if they did ever happen, and 
> certainly shouldn't apply to this bit of the infrastructure at any rate.

I certainly need to update the pgsize_bitmap at runtime because I don't
know the supported page sizes until I've both (a) probed the hardware
and (b) allocated page tables for a domain. We've already discussed
moving the pgsize_bitmap out of the ops, but moving it somewhere where
it remains const doesn't really help.

Can I just take the patch that Grant acked, in the interest of getting
something merged? As you say, there's plenty of planned changes in this
area anyway. I plan to send Olof a pull request this afternoon.

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

RE: [PATCH 2/4] iommu: add ARM LPAE page table allocator

[Varun Sethi]

Hi Will,
Please find my comments inline. Search for "varun"

-Original Message-
From: Will Deacon [mailto:will.dea...@arm.com] 
Sent: Thursday, November 27, 2014 5:21 PM
To: linux-arm-ker...@lists.infradead.org; iommu@lists.linux-foundation.org
Cc: prem.malla...@broadcom.com; robin.mur...@arm.com; lau...@codeaurora.org; 
mitch...@codeaurora.org; laurent.pinch...@ideasonboard.com; j...@8bytes.org; 
Sethi Varun-B16395; m.szyprow...@samsung.com; Will Deacon
Subject: [PATCH 2/4] iommu: add ARM LPAE page table allocator

A number of IOMMUs found in ARM SoCs can walk architecture-compatible page 
tables.

This patch adds a generic allocator for Stage-1 and Stage-2 v7/v8 
long-descriptor page tables. 4k, 16k and 64k pages are supported, with up to 
4-levels of walk to cover a 48-bit address space.

Signed-off-by: Will Deacon 
---
 MAINTAINERS|   1 +
 drivers/iommu/Kconfig  |   9 +
 drivers/iommu/Makefile |   1 +
 drivers/iommu/io-pgtable-arm.c | 735 +
 drivers/iommu/io-pgtable.c |   7 +
 drivers/iommu/io-pgtable.h |  12 +
 6 files changed, 765 insertions(+)
 create mode 100644 drivers/iommu/io-pgtable-arm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0ff630de8a6d..d3ca31b7c960 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1562,6 +1562,7 @@ M:Will Deacon 
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 S: Maintained
 F: drivers/iommu/arm-smmu.c
+F: drivers/iommu/io-pgtable-arm.c
 
 ARM64 PORT (AARCH64 ARCHITECTURE)
 M: Catalin Marinas 
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 
0f10554e7114..e1742a0146f8 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -19,6 +19,15 @@ menu "Generic IOMMU Pagetable Support"
 config IOMMU_IO_PGTABLE
bool
 
+config IOMMU_IO_PGTABLE_LPAE
+   bool "ARMv7/v8 Long Descriptor Format"
+   select IOMMU_IO_PGTABLE
+   help
+ Enable support for the ARM long descriptor pagetable format.
+ This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page
+ sizes at both stage-1 and stage-2, as well as address spaces
+ up to 48-bits in size.
+
 endmenu
 
 config OF_IOMMU
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 
aff244c78181..269cdd82b672 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
+obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
 obj-$(CONFIG_OF_IOMMU) += of_iommu.o
 obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o diff --git 
a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c new file mode 
100644 index ..9dbaa2e48424
--- /dev/null
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -0,0 +1,735 @@
+/*
+ * CPU-agnostic ARM page table allocator.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon   */
+
+#define pr_fmt(fmt)"arm-lpae io-pgtable: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "io-pgtable.h"
+
+#define ARM_LPAE_MAX_ADDR_BITS 48
+#define ARM_LPAE_S2_MAX_CONCAT_PAGES   16
+#define ARM_LPAE_MAX_LEVELS4
+
+/* Struct accessors */
+#define io_pgtable_to_data(x)  \
+   container_of((x), struct arm_lpae_io_pgtable, iop)
+
+#define io_pgtable_ops_to_pgtable(x)   \
+   container_of((x), struct io_pgtable, ops)
+
+#define io_pgtable_ops_to_data(x)  \
+   io_pgtable_to_data(io_pgtable_ops_to_pgtable(x))
+
+/*
+ * For consistency with the architecture, we always consider
+ * ARM_LPAE_MAX_LEVELS levels, with the walk starting at level n >=0  
+*/
+#define ARM_LPAE_START_LVL(d)  (ARM_LPAE_MAX_LEVELS - (d)->levels)
+
+/*
+ * Calculate the right shift amount to get to the portion describing 
+level l
+ * in a virtual address mapped by the pagetable in d.
+ */
+#define ARM_LPAE_LVL_SHIFT(l,d)
\
+   d)->levels - ((l) - ARM_LPAE_START_LVL(d) + 1)) \
+ * (d)->bits_per_level) + (d)->pg_shift)
+
+/*
+ * Calculate the ind

Re: [PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem

[Marek Szyprowski]

Hello,

On 2014-12-02 10:59, Sjoerd Simons wrote:

Hey Marek, Inki,

On Wed, 2014-11-19 at 12:15 +0100, Marek Szyprowski wrote:

Hello Everyone,

This is another attempt to finally make Exynos SYSMMU driver fully
integrated with DMA-mapping subsystem. The main change from previous
version is a rebase onto latest "automatic DMA configuration for IOMMU
masters" patches from Will Deacon.

Do you happen to know if anyone is working on iommu/dma-mapping patches
for Exynos 5 based on this patchset?


I hope to add Exynos5 SYSMMU patches to the next iteration of my patchset,
but I doubt it will get into v3.19-rc1.


For some background to that question, We (re-)discovered yesterday that
the out-of-tree exynos-reference kernel iommu patches are required to
get HDMI out working on exynos 5 boards. The current situation in
mainline is rather broken, HDMI output without CONFIG_DRM_EXYNOS_IOMMU
results in just displaying stripes[0]. While turning on
CONFIG_DRM_EXYNOS_IOMMU causes a kernel oops at boot


We have observed similar issues with Exynos4 based boards, when LCD0 power
domain was turned off and only TV power domain has been powered on. Please
check the power domain configuration. Maybe in case of Exynos5 the same 
issue

is caused by the interaction between DISP1 and GSCL domains.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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