Re: [RFC] switch m68k to use the generic remapping DMA allocator

2019-06-24 Thread Christoph Hellwig
On Mon, Jun 24, 2019 at 09:16:30AM +0200, Geert Uytterhoeven wrote:
> Doing
> 
> -   select DMA_DIRECT_REMAP if MMU && !COLDFIRE
> +   select DMA_DIRECT_REMAP if MMU && !COLDFIRE && !SUN3
> 
> in arch/m68k/Kconfig fixes the build.
> 
> Alternatively, you could use:
> 
> -   select DMA_DIRECT_REMAP if MMU && !COLDFIRE
> +   select DMA_DIRECT_REMAP if HAS_DMA && MMU && !COLDFIRE

The latter might be a little cleaner.


Re: [RFC] switch m68k to use the generic remapping DMA allocator

2019-06-24 Thread Christoph Hellwig
On Mon, Jun 17, 2019 at 08:53:55PM +0200, Geert Uytterhoeven wrote:
> Hi Christoph,
> 
> On Fri, Jun 14, 2019 at 12:21 PM Christoph Hellwig  wrote:
> > can you take a look at the (untested) patches below?  They convert m68k
> > to use the generic remapping DMA allocator, which is also used by
> > arm64 and csky.
> 
> Thanks. But what does this buy us?

A common dma mapping code base with everyone, including supporting
DMA allocations from atomic context, which the documentation and
API assume are there, but which don't work on m68k.

> bloat-o-meter says:
> 
> add/remove: 75/0 grow/shrink: 11/6 up/down: 4122/-82 (4040)

What do these values stand for?  The code should grow a little as
we now need to include the the pool allocator for the above API
fix.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 1/7] arm-nommu: remove the partial DMA_ATTR_NON_CONSISTENT support

2019-06-24 Thread Christoph Hellwig
On Mon, Jun 24, 2019 at 03:23:08PM +0100, Vladimir Murzin wrote:
> On 6/14/19 3:44 PM, Christoph Hellwig wrote:
> > The arm-nommu DMA code supports DMA_ATTR_NON_CONSISTENT allocations, but
> > does not provide a cache_sync operation.  This means any user of it
> > will never be able to actually transfer cache ownership and thus cause
> > coherency bugs.
> 
> By the way, Documentation/DMA-attributes.txt doesn't specify cache_sync() as
> requirement for DMA_ATTR_NON_CONSISTENT it only states that it is 
> responsibility
> of the driver to have all the correct and necessary sync points.

True.  dma_cache_sync has always been a rather odd interface, as it
doesn't specify in what direction we need to sync and doesn't
participate in our ownership protocol.  So my mid-term plan is to kill
it off and replace it with the existing dma_sync_* helpers.  This
series is the first step towards that.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 5/7] dma-direct: handle DMA_ATTR_NON_CONSISTENT in common code

2019-06-24 Thread Christoph Hellwig
On Sun, Jun 16, 2019 at 06:08:40PM +0800, Hillf Danton wrote:
> Literally, any cpu (call it cpuW) other than pcx12 and pcx1 will no longer do
> dma alloc for any device with this patch applied.

Yes.  And that is not a chance from the previous code, where only
pcx1 and pcx12 could do coherent allocations,

> On the other hand, 
> !dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_NON_CONSISTENT) will ask
> any cpu to do dma alloc, regardless of pcx1. This patch works imo unless cpuW
> plays games only with devices that are dma coherent. I doubt it is true.

I can't parse these two sentences.  But to explains the bits mentioned
here - dev_is_dma_coherent will return if a device is coherently
attached vs the cpu.  This will never be true for the parisc direct
mapping.  DMA_ATTR_NON_CONSISTENT asks for a non-coherent mapping that
needs to be explicitly synced.  This support now is in the dma-direct
core code, and this is what the parisc specific devices used on the
non-pcxl systems use, as they do not support dma coherency at all.
(the story slightly changes when using an iommu, but that is irrelevant
here)


Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

2019-06-24 Thread Christoph Hellwig
On Fri, Jun 14, 2019 at 09:46:48AM +0200, Christoph Hellwig wrote:
> If I don't hear anything back in the next days I will just merge
> these patches, please comment.

Applied to the dma-mapping for-next tree now.


Re: [PATCH 1/2] nios2: use the generic uncached segment support in dma-direct

2019-06-24 Thread Christoph Hellwig
On Tue, Jun 25, 2019 at 01:29:40PM +0800, Ley Foon Tan wrote:
> On Mon, Jun 3, 2019 at 2:54 PM Christoph Hellwig  wrote:
> >
> > Stop providing our own arch alloc/free hooks and just expose the segment
> > offset and use the generic dma-direct allocator.
> >
> > Signed-off-by: Christoph Hellwig 
> 
> Acked-by: Ley Foon Tan 

Thanks,

applied to dma-mapping for-next.


Re: [RFC] switch nds32 to use the generic remapping DMA allocator

2019-06-24 Thread Christoph Hellwig
Thanks,

applied to dma-mapping for-next.


Re: [PATCH 1/2] nios2: use the generic uncached segment support in dma-direct

2019-06-24 Thread Ley Foon Tan
On Mon, Jun 3, 2019 at 2:54 PM Christoph Hellwig  wrote:
>
> Stop providing our own arch alloc/free hooks and just expose the segment
> offset and use the generic dma-direct allocator.
>
> Signed-off-by: Christoph Hellwig 

Acked-by: Ley Foon Tan 

> ---
>  arch/nios2/Kconfig|  1 +
>  arch/nios2/include/asm/page.h |  6 --
>  arch/nios2/mm/dma-mapping.c   | 34 +++---
>  3 files changed, 16 insertions(+), 25 deletions(-)
>
> diff --git a/arch/nios2/Kconfig b/arch/nios2/Kconfig
> index 26a9c760a98b..44b5da37e8bd 100644
> --- a/arch/nios2/Kconfig
> +++ b/arch/nios2/Kconfig
> @@ -4,6 +4,7 @@ config NIOS2
> select ARCH_32BIT_OFF_T
> select ARCH_HAS_SYNC_DMA_FOR_CPU
> select ARCH_HAS_SYNC_DMA_FOR_DEVICE
> +   select ARCH_HAS_UNCACHED_SEGMENT
> select ARCH_NO_SWAP
> select TIMER_OF
> select GENERIC_ATOMIC64
> diff --git a/arch/nios2/include/asm/page.h b/arch/nios2/include/asm/page.h
> index f1fbdc47bdaf..79fcac61f6ef 100644
> --- a/arch/nios2/include/asm/page.h
> +++ b/arch/nios2/include/asm/page.h
> @@ -101,12 +101,6 @@ static inline bool pfn_valid(unsigned long pfn)
>  # define VM_DATA_DEFAULT_FLAGS (VM_READ | VM_WRITE | \
>  VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
>
> -# define UNCAC_ADDR(addr)  \
> -   ((void *)((unsigned)(addr) | CONFIG_NIOS2_IO_REGION_BASE))
> -# define CAC_ADDR(addr)\
> -   ((void *)(((unsigned)(addr) & ~CONFIG_NIOS2_IO_REGION_BASE) |   \
> -   CONFIG_NIOS2_KERNEL_REGION_BASE))
> -
>  #include 
>
>  #include 
> diff --git a/arch/nios2/mm/dma-mapping.c b/arch/nios2/mm/dma-mapping.c
> index 4af9e5b5ba1c..9cb238664584 100644
> --- a/arch/nios2/mm/dma-mapping.c
> +++ b/arch/nios2/mm/dma-mapping.c
> @@ -60,32 +60,28 @@ void arch_sync_dma_for_cpu(struct device *dev, 
> phys_addr_t paddr,
> }
>  }
>
> -void *arch_dma_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
> -   gfp_t gfp, unsigned long attrs)
> +void arch_dma_prep_coherent(struct page *page, size_t size)
>  {
> -   void *ret;
> +   unsigned long start = (unsigned long)page_address(page);
>
> -   /* optimized page clearing */
> -   gfp |= __GFP_ZERO;
> +   flush_dcache_range(start, start + size);
> +}
>
> -   if (dev == NULL || (dev->coherent_dma_mask < 0x))
> -   gfp |= GFP_DMA;
> +void *uncached_kernel_address(void *ptr)
> +{
> +   unsigned long addr = (unsigned long)ptr;
>
> -   ret = (void *) __get_free_pages(gfp, get_order(size));
> -   if (ret != NULL) {
> -   *dma_handle = virt_to_phys(ret);
> -   flush_dcache_range((unsigned long) ret,
> -   (unsigned long) ret + size);
> -   ret = UNCAC_ADDR(ret);
> -   }
> +   addr |= CONFIG_NIOS2_IO_REGION_BASE;
>
> -   return ret;
> +   return (void *)ptr;
>  }
>
> -void arch_dma_free(struct device *dev, size_t size, void *vaddr,
> -   dma_addr_t dma_handle, unsigned long attrs)
> +void *cached_kernel_address(void *ptr)
>  {
> -   unsigned long addr = (unsigned long) CAC_ADDR((unsigned long) vaddr);
> +   unsigned long addr = (unsigned long)ptr;
> +
> +   addr &= ~CONFIG_NIOS2_IO_REGION_BASE;
> +   addr |= CONFIG_NIOS2_KERNEL_REGION_BASE;
>
> -   free_pages(addr, get_order(size));
> +   return (void *)ptr;
>  }
> --
> 2.20.1
>


Re: [PATCH v4 19/22] iommu/vt-d: Clean up for SVM device list

2019-06-24 Thread Jacob Pan
On Tue, 18 Jun 2019 17:42:37 +0100
Jonathan Cameron  wrote:

> On Sun, 9 Jun 2019 06:44:19 -0700
> Jacob Pan  wrote:
> 
> > Use combined macro for_each_svm_dev() to simplify SVM device
> > iteration.
> > 
> > Suggested-by: Andy Shevchenko 
> > Signed-off-by: Jacob Pan 
> > Reviewed-by: Eric Auger 
> > ---
> >  drivers/iommu/intel-svm.c | 79
> > +++ 1 file changed, 39
> > insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index 9cbcc1f..66d98e1 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -225,6 +225,9 @@ static const struct mmu_notifier_ops
> > intel_mmuops = { 
> >  static DEFINE_MUTEX(pasid_mutex);
> >  static LIST_HEAD(global_svm_list);
> > +#define for_each_svm_dev() \
> > +   list_for_each_entry(sdev, &svm->devs, list) \
> > +   if (dev == sdev->dev)   \  
> 
> Could we make this macro less opaque and have it take the svm and dev
> as arguments?
> 
sounds good, it makes the code more readable.


Re: [PATCH v4 17/22] iommu/vt-d: Avoid duplicated code for PASID setup

2019-06-24 Thread Jacob Pan
On Tue, 18 Jun 2019 17:03:35 +0100
Jonathan Cameron  wrote:

> On Sun, 9 Jun 2019 06:44:17 -0700
> Jacob Pan  wrote:
> 
> > After each setup for PASID entry, related translation caches must
> > be flushed. We can combine duplicated code into one function which
> > is less error prone.
> > 
> > Signed-off-by: Jacob Pan   
> Formatting nitpick below ;)
> 
> Otherwise it's cut and paste
> > ---
> >  drivers/iommu/intel-pasid.c | 48
> > + 1 file changed, 18
> > insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-pasid.c
> > b/drivers/iommu/intel-pasid.c index 1e25539..1ff2ecc 100644
> > --- a/drivers/iommu/intel-pasid.c
> > +++ b/drivers/iommu/intel-pasid.c
> > @@ -522,6 +522,21 @@ void intel_pasid_tear_down_entry(struct
> > intel_iommu *iommu, devtlb_invalidation_with_pasid(iommu, dev,
> > pasid); }
> >  
> > +static inline void pasid_flush_caches(struct intel_iommu *iommu,
> > +   struct pasid_entry *pte,
> > +   int pasid, u16 did)
> > +{
> > +   if (!ecap_coherent(iommu->ecap))
> > +   clflush_cache_range(pte, sizeof(*pte));
> > +
> > +   if (cap_caching_mode(iommu->cap)) {
> > +   pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > +   iotlb_invalidation_with_pasid(iommu, did, pasid);
> > +   } else
> > +   iommu_flush_write_buffer(iommu);  
> 
> I have some vague recollection kernel style says use brackets around
> single lines if other blocks in an if / else stack have multiple
> lines..
> 
> I checked, this case is specifically called out
> 
> https://www.kernel.org/doc/html/v5.1/process/coding-style.html
> > +  
> This blank line doesn't add anything either ;)
indeed. i will add the braces and remove the blank line.

Thanks for looking it up.
> > +}
> > +
> >  /*
> >   * Set up the scalable mode pasid table entry for first only
> >   * translation type.
> > @@ -567,16 +582,7 @@ int intel_pasid_setup_first_level(struct
> > intel_iommu *iommu, /* Setup Present and PASID Granular Transfer
> > Type: */ pasid_set_translation_type(pte, 1);
> > pasid_set_present(pte);
> > -
> > -   if (!ecap_coherent(iommu->ecap))
> > -   clflush_cache_range(pte, sizeof(*pte));
> > -
> > -   if (cap_caching_mode(iommu->cap)) {
> > -   pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > -   iotlb_invalidation_with_pasid(iommu, did, pasid);
> > -   } else {
> > -   iommu_flush_write_buffer(iommu);
> > -   }
> > +   pasid_flush_caches(iommu, pte, pasid, did);
> >  
> > return 0;
> >  }
> > @@ -640,16 +646,7 @@ int intel_pasid_setup_second_level(struct
> > intel_iommu *iommu, */
> > pasid_set_sre(pte);
> > pasid_set_present(pte);
> > -
> > -   if (!ecap_coherent(iommu->ecap))
> > -   clflush_cache_range(pte, sizeof(*pte));
> > -
> > -   if (cap_caching_mode(iommu->cap)) {
> > -   pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > -   iotlb_invalidation_with_pasid(iommu, did, pasid);
> > -   } else {
> > -   iommu_flush_write_buffer(iommu);
> > -   }
> > +   pasid_flush_caches(iommu, pte, pasid, did);
> >  
> > return 0;
> >  }
> > @@ -683,16 +680,7 @@ int intel_pasid_setup_pass_through(struct
> > intel_iommu *iommu, */
> > pasid_set_sre(pte);
> > pasid_set_present(pte);
> > -
> > -   if (!ecap_coherent(iommu->ecap))
> > -   clflush_cache_range(pte, sizeof(*pte));
> > -
> > -   if (cap_caching_mode(iommu->cap)) {
> > -   pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > -   iotlb_invalidation_with_pasid(iommu, did, pasid);
> > -   } else {
> > -   iommu_flush_write_buffer(iommu);
> > -   }
> > +   pasid_flush_caches(iommu, pte, pasid, did);
> >  
> > return 0;
> >  }  
> 
> 

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


Re: [PATCH v4 20/22] iommu/vt-d: Add bind guest PASID support

2019-06-24 Thread Jacob Pan
On Tue, 18 Jun 2019 17:44:49 +0100
Jonathan Cameron  wrote:

> On Sun, 9 Jun 2019 06:44:20 -0700
> Jacob Pan  wrote:
> 
> > When supporting guest SVA with emulated IOMMU, the guest PASID
> > table is shadowed in VMM. Updates to guest vIOMMU PASID table
> > will result in PASID cache flush which will be passed down to
> > the host as bind guest PASID calls.
> > 
> > For the SL page tables, it will be harvested from device's
> > default domain (request w/o PASID), or aux domain in case of
> > mediated device.
> > 
> > .-.  .---.
> > |   vIOMMU|  | Guest process CR3, FL only|
> > | |  '---'
> > ./
> > | PASID Entry |--- PASID cache flush -
> > '-'   |
> > | |   V
> > | |CR3 in GPA
> > '-'
> > Guest
> > --| Shadow |--|
> >   vv  v
> > Host
> > .-.  .--.
> > |   pIOMMU|  | Bind FL for GVA-GPA  |
> > | |  '--'
> > ./  |
> > | PASID Entry | V (Nested xlate)
> > '\.--.
> > | |   |SL for GPA-HPA, default domain|
> > | |   '--'
> > '-'
> > Where:
> >  - FL = First level/stage one page tables
> >  - SL = Second level/stage two page tables
> > 
> > Signed-off-by: Jacob Pan 
> > Signed-off-by: Liu, Yi L   
> 
> 
> A few trivial bits inline.  As far as I can tell looks good but I'm
> not that familiar with the hardware.
> 
Thank you so much for the review.

> Jonathan
> 
> > ---
> >  drivers/iommu/intel-iommu.c |   4 +
> >  drivers/iommu/intel-svm.c   | 187
> > 
> > include/linux/intel-iommu.h |  13 ++- include/linux/intel-svm.h
> > |  17  4 files changed, 219 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index 7cfa0eb..3b4d712 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -5782,6 +5782,10 @@ const struct iommu_ops intel_iommu_ops = {
> > .dev_enable_feat= intel_iommu_dev_enable_feat,
> > .dev_disable_feat   = intel_iommu_dev_disable_feat,
> > .pgsize_bitmap  = INTEL_IOMMU_PGSIZES,
> > +#ifdef CONFIG_INTEL_IOMMU_SVM
> > +   .sva_bind_gpasid= intel_svm_bind_gpasid,
> > +   .sva_unbind_gpasid  = intel_svm_unbind_gpasid,
> > +#endif
> >  };
> >  
> >  static void quirk_iommu_g4x_gfx(struct pci_dev *dev)
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index 66d98e1..f06a82f 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -229,6 +229,193 @@ static LIST_HEAD(global_svm_list);
> > list_for_each_entry(sdev, &svm->devs, list) \
> > if (dev == sdev->dev)   \
> >  
> > +int intel_svm_bind_gpasid(struct iommu_domain *domain,
> > +   struct device *dev,
> > +   struct gpasid_bind_data *data)
> > +{
> > +   struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
> > +   struct intel_svm_dev *sdev;
> > +   struct intel_svm *svm = NULL;  
> I think this is set in all the paths that use it..
> 
indeed. will remove.
> > +   struct dmar_domain *ddomain;
> > +   int ret = 0;
> > +
> > +   if (WARN_ON(!iommu) || !data)
> > +   return -EINVAL;
> > +
> > +   if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
> > +   data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
> > +   return -EINVAL;
> > +
> > +   if (dev_is_pci(dev)) {
> > +   /* VT-d supports devices with full 20 bit PASIDs
> > only */
> > +   if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX)
> > +   return -EINVAL;
> > +   }
> > +
> > +   /*
> > +* We only check host PASID range, we have no knowledge to
> > check
> > +* guest PASID range nor do we use the guest PASID.
> > +*/
> > +   if (data->hpasid <= 0 || data->hpasid >= PASID_MAX)
> > +   return -EINVAL;
> > +
> > +   ddomain = to_dmar_domain(domain);
> > +   /* REVISIT:
> > +* Sanity check adddress width and paging mode support
> > +* width matching in two dimensions:
> > +* 1. paging mode CPU <= IOMMU
> > +* 2. address width Guest <= Host.
> > +*/
> > +   mutex_lock(&pasid_mutex);
> > +   svm = ioasid_find(NULL, data->hpasid, NULL);
> > +   if (IS_ERR(svm)) {
> > +   ret = PTR_ERR(svm);
> > +   goto out;
> > +   }
> > +   if (svm) {
> > +   /*
> > +* If we found svm for the PASID, there must be at
> > +* least one device bond, otherwise svm should be
> > freed.
> > +*/
> > +   BUG_ON(list_empty(&svm->devs));
> 

Re: [PATCH v4 10/22] iommu: Fix compile error without IOMMU_API

2019-06-24 Thread Jacob Pan
On Tue, 18 Jun 2019 15:10:31 +0100
Jonathan Cameron  wrote:

> On Sun, 9 Jun 2019 06:44:10 -0700
> Jacob Pan  wrote:
> 
> > struct page_response_msg needs to be defined outside
> > CONFIG_IOMMU_API.  
> 
> What was the error? 
> 
struct page_response_msg can be undefined if under CONFIG_IOMMU_API.
This was resolved after Jean move it to uapi. Sorry, I could have made
it more clear, not a fix for mainline kernel bug just a patch for
previous patch in Jean's API tree.
> If this is a fix for an earlier patch in this series role it in there
> (or put it before it). If more general we should add a fixes tag.
> 
> Jonathan
>  [...]  
> 
> 

[Jacob Pan]


Re: [PATCH v4 11/22] iommu: Introduce guest PASID bind function

2019-06-24 Thread Jacob Pan
On Tue, 18 Jun 2019 16:36:33 +0100
Jean-Philippe Brucker  wrote:

> On 09/06/2019 14:44, Jacob Pan wrote:
> > Guest shared virtual address (SVA) may require host to shadow guest
> > PASID tables. Guest PASID can also be allocated from the host via
> > enlightened interfaces. In this case, guest needs to bind the guest
> > mm, i.e. cr3 in guest physical address to the actual PASID table in
> > the host IOMMU. Nesting will be turned on such that guest virtual
> > address can go through a two level translation:
> > - 1st level translates GVA to GPA
> > - 2nd level translates GPA to HPA
> > This patch introduces APIs to bind guest PASID data to the assigned
> > device entry in the physical IOMMU. See the diagram below for usage
> > explaination.  
> 
> explanation
> 
will fix, thanks
> > 
> > .-.  .---.
> > |   vIOMMU|  | Guest process mm, FL only |
> > | |  '---'
> > ./
> > | PASID Entry |--- PASID cache flush -
> > '-'   |
> > | |   V
> > | |  GP
> > '-'
> > Guest
> > --| Shadow |--- GP->HP* -
> >   vv  |
> > Host  v
> > .-.  .--.
> > |   pIOMMU|  | Bind FL for GVA-GPA  |
> > | |  '--'
> > ./  |
> > | PASID Entry | V (Nested xlate)
> > '\.-.
> > | |   |Set SL to GPA-HPA|
> > | |   '-'
> > '-'
> > 
> > Where:
> >  - FL = First level/stage one page tables
> >  - SL = Second level/stage two page tables
> >  - GP = Guest PASID
> >  - HP = Host PASID
> > * Conversion needed if non-identity GP-HP mapping option is chosen.
> > 
> > Signed-off-by: Jacob Pan 
> > Signed-off-by: Liu Yi L 
> > ---
> >  drivers/iommu/iommu.c  | 20 
> >  include/linux/iommu.h  | 21 +
> >  include/uapi/linux/iommu.h | 58
> > ++ 3 files changed, 99
> > insertions(+)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 1758b57..d0416f60 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1648,6 +1648,26 @@ int iommu_cache_invalidate(struct
> > iommu_domain *domain, struct device *dev, }
> >  EXPORT_SYMBOL_GPL(iommu_cache_invalidate);
> >  
> > +int iommu_sva_bind_gpasid(struct iommu_domain *domain,
> > +   struct device *dev, struct
> > gpasid_bind_data *data)  
> 
> I'm curious about the VFIO side of this. Is the ioctl on the device or
> on the container fd? For bind_pasid_table, it's on the container and
> we only pass the iommu_domain to the IOMMU driver, not the device
> (since devices in a domain share the same PASID table).
> 
VFIO side of gpasid bind is on the container fd (Yi can confirm :)).
We have per device PASID table regardless of domain sharing. It can
provide more protection within the guest.
Second level page tables are harvested from domain for nested
translation.
> > +{
> > +   if (unlikely(!domain->ops->sva_bind_gpasid))
> > +   return -ENODEV;
> > +
> > +   return domain->ops->sva_bind_gpasid(domain, dev, data);
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_sva_bind_gpasid);
> > +
> > +int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct
> > device *dev,
> > +   ioasid_t pasid)
> > +{
> > +   if (unlikely(!domain->ops->sva_unbind_gpasid))
> > +   return -ENODEV;
> > +
> > +   return domain->ops->sva_unbind_gpasid(dev, pasid);
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
> > +
> >  static void __iommu_detach_device(struct iommu_domain *domain,
> >   struct device *dev)
> >  {
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 8d766a8..560c8c8 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -25,6 +25,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  
> >  #define IOMMU_READ (1 << 0)
> > @@ -267,6 +268,8 @@ struct page_response_msg {
> >   * @detach_pasid_table: detach the pasid table
> >   * @cache_invalidate: invalidate translation caches
> >   * @pgsize_bitmap: bitmap of all possible supported page sizes
> > + * @sva_bind_gpasid: bind guest pasid and mm
> > + * @sva_unbind_gpasid: unbind guest pasid and mm
> >   */
> >  struct iommu_ops {
> > bool (*capable)(enum iommu_cap);
> > @@ -332,6 +335,10 @@ struct iommu_ops {
> > int (*page_response)(struct device *dev, struct
> > page_response_msg *msg); int (*cache_invalidate)(struct
> > iommu_domain *domain, struct device *dev, struct
> > iommu_cache_invalidate_info *inv_info);
> > +   int (*sva_bind_gpasid)(stru

Re: [PATCH v4 15/22] iommu/vt-d: Replace Intel specific PASID allocator with IOASID

2019-06-24 Thread Jacob Pan
On Tue, 18 Jun 2019 16:57:09 +0100
Jonathan Cameron  wrote:

> On Sun, 9 Jun 2019 06:44:15 -0700
> Jacob Pan  wrote:
> 
> > Make use of generic IOASID code to manage PASID allocation,
> > free, and lookup. Replace Intel specific code.
> > 
> > Signed-off-by: Jacob Pan   
> Hi Jacob,
> 
> One question inline.
> 
Hi Jonathan,

Sorry, I got distracted and missed your review. My answer
below.

Thanks!

Jacob

> Jonathan
> 
> > ---
> >  drivers/iommu/intel-iommu.c | 11 +--
> >  drivers/iommu/intel-pasid.c | 36
> >  drivers/iommu/intel-svm.c   |
> > 37 + 3 files changed, 26
> > insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index 5b84994..39b63fe 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -5167,7 +5167,7 @@ static void auxiliary_unlink_device(struct
> > dmar_domain *domain, domain->auxd_refcnt--;
> >  
> > if (!domain->auxd_refcnt && domain->default_pasid > 0)
> > -   intel_pasid_free_id(domain->default_pasid);
> > +   ioasid_free(domain->default_pasid);
> >  }
> >  
> >  static int aux_domain_add_dev(struct dmar_domain *domain,
> > @@ -5185,10 +5185,9 @@ static int aux_domain_add_dev(struct
> > dmar_domain *domain, if (domain->default_pasid <= 0) {
> > int pasid;
> >  
> > -   pasid = intel_pasid_alloc_id(domain, PASID_MIN,
> > -
> > pci_max_pasids(to_pci_dev(dev)),
> > -GFP_KERNEL);
> > -   if (pasid <= 0) {
> > +   pasid = ioasid_alloc(NULL, PASID_MIN,
> > pci_max_pasids(to_pci_dev(dev)) - 1,
> > +   domain);  
> 
> Is there any point in passing the domain in as the private pointer
> here? I can't immediately see anywhere it is read back?
> 
Good point, I agree. There is no need for passing the domain. aux domain
and its defaul_pasid has 1:1 mapping.

> It is also rather confusing as the same driver stashes two different
> types of data in the same xarray.

On VT-d, there should be only one type of data for all PASID users that
needs private data. Whether it is native SVA or guest SVA. Only an
internal flag will tell them apart.


[PATCH] irq_remapping/vt-d: cleanup unused variable

2019-06-24 Thread Jacob Pan
Linux IRQ number virq is not used in IRTE allocation. Remove it.

Signed-off-by: Jacob Pan 
---
 drivers/iommu/intel_irq_remapping.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel_irq_remapping.c 
b/drivers/iommu/intel_irq_remapping.c
index 4160aa9..4786ca0 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -101,7 +101,7 @@ static void init_ir_status(struct intel_iommu *iommu)
iommu->flags |= VTD_FLAG_IRQ_REMAP_PRE_ENABLED;
 }
 
-static int alloc_irte(struct intel_iommu *iommu, int irq,
+static int alloc_irte(struct intel_iommu *iommu,
  struct irq_2_iommu *irq_iommu, u16 count)
 {
struct ir_table *table = iommu->ir_table;
@@ -1374,7 +1374,7 @@ static int intel_irq_remapping_alloc(struct irq_domain 
*domain,
goto out_free_parent;
 
down_read(&dmar_global_lock);
-   index = alloc_irte(iommu, virq, &data->irq_2_iommu, nr_irqs);
+   index = alloc_irte(iommu, &data->irq_2_iommu, nr_irqs);
up_read(&dmar_global_lock);
if (index < 0) {
pr_warn("Failed to allocate IRTE\n");
-- 
2.7.4



Re: [PATCH v3 3/4] iommu/arm-smmu: Add support to handle Qcom's wait-for-safe logic

2019-06-24 Thread Will Deacon
[+Krishna]

Hi Vivek,

On Mon, Jun 24, 2019 at 03:58:32PM +0530, Vivek Gautam wrote:
> On Tue, Jun 18, 2019 at 11:22 PM Will Deacon  wrote:
> > On Fri, Jun 14, 2019 at 02:48:07PM +0530, Vivek Gautam wrote:
> > > On 6/14/2019 9:35 AM, Bjorn Andersson wrote:
> > > > On Wed 12 Jun 00:15 PDT 2019, Vivek Gautam wrote:
> > > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > > > index 0ad086da399c..3c3ad43eda97 100644
> > > > > --- a/drivers/iommu/arm-smmu.c
> > > > > +++ b/drivers/iommu/arm-smmu.c
> > > > > @@ -39,6 +39,7 @@
> > > > >   #include 
> > > > >   #include 
> > > > >   #include 
> > > > > +#include 
> > > > >   #include 
> > > > >   #include 
> > > > > @@ -177,6 +178,7 @@ struct arm_smmu_device {
> > > > >   u32 features;
> > > > >   #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
> > > > > +#define ARM_SMMU_OPT_QCOM_FW_IMPL_SAFE_ERRATA (1 << 1)
> > > > >   u32 options;
> > > > >   enum arm_smmu_arch_version  version;
> > > > >   enum arm_smmu_implementationmodel;
> > > > > @@ -262,6 +264,7 @@ static bool using_legacy_binding, 
> > > > > using_generic_binding;
> > > > >   static struct arm_smmu_option_prop arm_smmu_options[] = {
> > > > >   { ARM_SMMU_OPT_SECURE_CFG_ACCESS, 
> > > > > "calxeda,smmu-secure-config-access" },
> > > > > + { ARM_SMMU_OPT_QCOM_FW_IMPL_SAFE_ERRATA, 
> > > > > "qcom,smmu-500-fw-impl-safe-errata" },
> > > > This should be added to the DT binding as well.
> > >
> > > Ah right. I missed that. Will add this and respin unless Robin and Will 
> > > have
> > > concerns with this change.
> >
> > My only concern really is whether it's safe for us to turn this off. It's
> > clear that somebody went to a lot of effort to add this extra goodness to
> > the IP, but your benchmarks suggest they never actually tried it out after
> > they finished building it.
> >
> > Is there some downside I'm not seeing from disabling this stuff?
> 
> This wait-for-safe is a TLB invalidation enhancement to help display
> and camera devices.
> The SMMU hardware throttles the invalidations so that clients such as
> display and camera can indicate when to start the invalidation.
> So the SMMU essentially reduces the rate at which invalidations are
> serviced from its queue. This also throttles the invalidations from
> other masters too.
> 
> On sdm845, the software is expected to serialize the invalidation
> command loading into SMMU invalidation FIFO using hardware locks
> (downstream code [2]), and is also expected to throttle non-real time
> clients while waiting for SAFE==1 (downstream code[2]). We don't do
> any of these yet, and as per my understanding as this wait-for-safe is
> enabled by the bootloader in a one time config, this logic reduces
> performance of devices such as usb and ufs.
> 
> There's isn't any downside from disabling this logic until we have all
> the pieces together from downstream in upstream kernels, and until we
> have sdm845 devices that are running with full display/gfx stack
> running. That's when we plan to revisit this and enable all the pieces
> to get display and USB/UFS working with their optimum performance.

Generally, I'd agree that approaching this incrementally makes sense, but
in this case you're adding new device-tree properties
("qcom,smmu-500-fw-impl-safe-errata") in order to do so, which seems
questionable if they're only going to be used in the short-term and will
be obsolete once Linux knows how to drive the device properly.

Instead, I think this needs to be part of a separate file that is maintained
by you, which follows on from the work that Krishna is doing for nvidia
built on top of Robin's prototype patches:

http://linux-arm.org/git?p=linux-rm.git;a=shortlog;h=refs/heads/iommu/smmu-impl

Once we have that, you can key this behaviour off the compatible string
rather than having to add quirk properties to reflect the transient needs of
Linux.

Krishna -- how have you been getting on with the branch above?

Will


Re: [PATCH v4 08/22] iommu: Introduce attach/detach_pasid_table API

2019-06-24 Thread Jean-Philippe Brucker
On 24/06/2019 16:06, Auger Eric wrote:
> Hi,
> 
> On 6/18/19 5:41 PM, Jonathan Cameron wrote:
>> On Sun, 9 Jun 2019 06:44:08 -0700
>> Jacob Pan  wrote:
>>
>>> In virtualization use case, when a guest is assigned
>>> a PCI host device, protected by a virtual IOMMU on the guest,
>>> the physical IOMMU must be programmed to be consistent with
>>> the guest mappings. If the physical IOMMU supports two
>>> translation stages it makes sense to program guest mappings
>>> onto the first stage/level (ARM/Intel terminology) while the host
>>> owns the stage/level 2.
>>>
>>> In that case, it is mandated to trap on guest configuration
>>> settings and pass those to the physical iommu driver.
>>>
>>> This patch adds a new API to the iommu subsystem that allows
>>> to set/unset the pasid table information.
>>>
>>> A generic iommu_pasid_table_config struct is introduced in
>>> a new iommu.h uapi header. This is going to be used by the VFIO
>>> user API.
>>
>> Another case where strictly speaking stuff is introduced that this series
>> doesn't use.  I don't know what the plans are to merge the various
>> related series though so this might make sense in general. Right now
>> it just bloats this series a bit..
> 
> I am now the only user of this API in
> [PATCH v8 00/29] SMMUv3 Nested Stage Setup
> (https://patchwork.kernel.org/cover/10961733/).
> 
> This can live in this series or Jean-Philippe do you intend to keep on
> maintaining it in your api branch?

No, I think it makes sense to keep it within your series

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


Re: [PATCH v4 08/22] iommu: Introduce attach/detach_pasid_table API

2019-06-24 Thread Auger Eric
Hi,

On 6/18/19 5:41 PM, Jonathan Cameron wrote:
> On Sun, 9 Jun 2019 06:44:08 -0700
> Jacob Pan  wrote:
> 
>> In virtualization use case, when a guest is assigned
>> a PCI host device, protected by a virtual IOMMU on the guest,
>> the physical IOMMU must be programmed to be consistent with
>> the guest mappings. If the physical IOMMU supports two
>> translation stages it makes sense to program guest mappings
>> onto the first stage/level (ARM/Intel terminology) while the host
>> owns the stage/level 2.
>>
>> In that case, it is mandated to trap on guest configuration
>> settings and pass those to the physical iommu driver.
>>
>> This patch adds a new API to the iommu subsystem that allows
>> to set/unset the pasid table information.
>>
>> A generic iommu_pasid_table_config struct is introduced in
>> a new iommu.h uapi header. This is going to be used by the VFIO
>> user API.
> 
> Another case where strictly speaking stuff is introduced that this series
> doesn't use.  I don't know what the plans are to merge the various
> related series though so this might make sense in general. Right now
> it just bloats this series a bit..

I am now the only user of this API in
[PATCH v8 00/29] SMMUv3 Nested Stage Setup
(https://patchwork.kernel.org/cover/10961733/).

This can live in this series or Jean-Philippe do you intend to keep on
maintaining it in your api branch?

Thanks

Eric
>>
>> Signed-off-by: Jean-Philippe Brucker 
>> Signed-off-by: Liu, Yi L 
>> Signed-off-by: Ashok Raj 
>> Signed-off-by: Jacob Pan 
>> Signed-off-by: Eric Auger 
>> Reviewed-by: Jean-Philippe Brucker 
>> ---
>>  drivers/iommu/iommu.c  | 19 +
>>  include/linux/iommu.h  | 18 
>>  include/uapi/linux/iommu.h | 52 
>> ++
>>  3 files changed, 89 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 166adb8..4496ccd 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1619,6 +1619,25 @@ int iommu_page_response(struct device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(iommu_page_response);
>>  
>> +int iommu_attach_pasid_table(struct iommu_domain *domain,
>> + struct iommu_pasid_table_config *cfg)
>> +{
>> +if (unlikely(!domain->ops->attach_pasid_table))
>> +return -ENODEV;
>> +
>> +return domain->ops->attach_pasid_table(domain, cfg);
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_attach_pasid_table);
>> +
>> +void iommu_detach_pasid_table(struct iommu_domain *domain)
>> +{
>> +if (unlikely(!domain->ops->detach_pasid_table))
>> +return;
>> +
>> +domain->ops->detach_pasid_table(domain);
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_detach_pasid_table);
>> +
>>  static void __iommu_detach_device(struct iommu_domain *domain,
>>struct device *dev)
>>  {
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 950347b..d3edb10 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -264,6 +264,8 @@ struct page_response_msg {
>>   * @sva_unbind: Unbind process address space from device
>>   * @sva_get_pasid: Get PASID associated to a SVA handle
>>   * @page_response: handle page request response
>> + * @attach_pasid_table: attach a pasid table
>> + * @detach_pasid_table: detach the pasid table
>>   * @pgsize_bitmap: bitmap of all possible supported page sizes
>>   */
>>  struct iommu_ops {
>> @@ -323,6 +325,9 @@ struct iommu_ops {
>>void *drvdata);
>>  void (*sva_unbind)(struct iommu_sva *handle);
>>  int (*sva_get_pasid)(struct iommu_sva *handle);
>> +int (*attach_pasid_table)(struct iommu_domain *domain,
>> +  struct iommu_pasid_table_config *cfg);
>> +void (*detach_pasid_table)(struct iommu_domain *domain);
>>  
>>  int (*page_response)(struct device *dev, struct page_response_msg *msg);
>>  
>> @@ -434,6 +439,9 @@ extern int iommu_attach_device(struct iommu_domain 
>> *domain,
>> struct device *dev);
>>  extern void iommu_detach_device(struct iommu_domain *domain,
>>  struct device *dev);
>> +extern int iommu_attach_pasid_table(struct iommu_domain *domain,
>> +struct iommu_pasid_table_config *cfg);
>> +extern void iommu_detach_pasid_table(struct iommu_domain *domain);
>>  extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
>>  extern struct iommu_domain *iommu_get_dma_domain(struct device *dev);
>>  extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
>> @@ -947,6 +955,13 @@ iommu_aux_get_pasid(struct iommu_domain *domain, struct 
>> device *dev)
>>  return -ENODEV;
>>  }
>>  
>> +static inline
>> +int iommu_attach_pasid_table(struct iommu_domain *domain,
>> + struct iommu_pasid_table_config *cfg)
>> +{
>> +return -ENODEV;
>> +}
>> +
>>  static inline struct iommu_sva *
>>  iommu

Re: [PATCH 1/7] arm-nommu: remove the partial DMA_ATTR_NON_CONSISTENT support

2019-06-24 Thread Vladimir Murzin
On 6/14/19 3:44 PM, Christoph Hellwig wrote:
> The arm-nommu DMA code supports DMA_ATTR_NON_CONSISTENT allocations, but
> does not provide a cache_sync operation.  This means any user of it
> will never be able to actually transfer cache ownership and thus cause
> coherency bugs.

By the way, Documentation/DMA-attributes.txt doesn't specify cache_sync() as
requirement for DMA_ATTR_NON_CONSISTENT it only states that it is responsibility
of the driver to have all the correct and necessary sync points.

> 
> Signed-off-by: Christoph Hellwig 
> ---
>  arch/arm/mm/dma-mapping-nommu.c | 24 +++-
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
> index f304b10e23a4..bc003df45546 100644
> --- a/arch/arm/mm/dma-mapping-nommu.c
> +++ b/arch/arm/mm/dma-mapping-nommu.c
> @@ -39,18 +39,7 @@ static void *arm_nommu_dma_alloc(struct device *dev, 
> size_t size,
>unsigned long attrs)
>  
>  {
> - void *ret;
> -
> - /*
> -  * Try generic allocator first if we are advertised that
> -  * consistency is not required.
> -  */
> -
> - if (attrs & DMA_ATTR_NON_CONSISTENT)
> - return dma_direct_alloc_pages(dev, size, dma_handle, gfp,
> - attrs);
> -
> - ret = dma_alloc_from_global_coherent(size, dma_handle);
> + void *ret = dma_alloc_from_global_coherent(size, dma_handle);
>  
>   /*
>* dma_alloc_from_global_coherent() may fail because:
> @@ -70,16 +59,9 @@ static void arm_nommu_dma_free(struct device *dev, size_t 
> size,
>  void *cpu_addr, dma_addr_t dma_addr,
>  unsigned long attrs)
>  {
> - if (attrs & DMA_ATTR_NON_CONSISTENT) {
> - dma_direct_free_pages(dev, size, cpu_addr, dma_addr, attrs);
> - } else {
> - int ret = dma_release_from_global_coherent(get_order(size),
> -cpu_addr);
> -
> - WARN_ON_ONCE(ret == 0);
> - }
> + int ret = dma_release_from_global_coherent(get_order(size), cpu_addr);
>  
> - return;
> + WARN_ON_ONCE(ret == 0);
>  }
>  
>  static int arm_nommu_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> 

FWIW:

Reviewed-by: Vladimir Murzin 

Cheers
Vladimir


Re: [PATCH] iommu: io-pgtable: Support non-coherent page tables

2019-06-24 Thread Will Deacon
Hi again, Bjorn,

On Tue, Jun 18, 2019 at 06:39:33PM +0100, Will Deacon wrote:
> On Wed, May 15, 2019 at 04:32:34PM -0700, Bjorn Andersson wrote:
> > Describe the memory related to page table walks as non-cachable for iommu
> > instances that are not DMA coherent.
> > 
> > Signed-off-by: Bjorn Andersson 
> > ---
> >  drivers/iommu/io-pgtable-arm.c | 12 +---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index 4e21efbc4459..68ff22ffd2cb 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -803,9 +803,15 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg 
> > *cfg, void *cookie)
> > return NULL;
> >  
> > /* TCR */
> > -   reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
> > - (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) |
> > - (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
> > +   if (cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA) {
> > +   reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
> > + (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) |
> > + (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
> > +   } else {
> > +   reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
> 
> Nit: this should be outer-shareable (ARM_LPAE_TCR_SH_OS).
> 
> > + (ARM_LPAE_TCR_RGN_NC << ARM_LPAE_TCR_IRGN0_SHIFT) |
> > + (ARM_LPAE_TCR_RGN_NC << ARM_LPAE_TCR_ORGN0_SHIFT);
> > +   }
> 
> Should we also be doing something similar for the short-descriptor code
> in io-pgtable-arm-v7s.c? Looks like you just need to use ARM_V7S_RGN_NC
> instead of ARM_V7S_RGN_WBWA when initialising ttbr0 for non-coherent
> SMMUs.

Do you plan to respin this? I'll need it this week if you're shooting for
5.3.

Thanks,

Will


Re: [PATCH v3 3/4] iommu/arm-smmu: Add support to handle Qcom's wait-for-safe logic

2019-06-24 Thread Vivek Gautam
Hi Will,

On Tue, Jun 18, 2019 at 11:22 PM Will Deacon  wrote:
>
> Hi Vivek,
>
> On Fri, Jun 14, 2019 at 02:48:07PM +0530, Vivek Gautam wrote:
> > On 6/14/2019 9:35 AM, Bjorn Andersson wrote:
> > > On Wed 12 Jun 00:15 PDT 2019, Vivek Gautam wrote:
> > >
> > > > Qcom's implementation of arm,mmu-500 adds a WAIT-FOR-SAFE logic
> > > > to address under-performance issues in real-time clients, such as
> > > > Display, and Camera.
> > > > On receiving an invalidation requests, the SMMU forwards SAFE request
> > > > to these clients and waits for SAFE ack signal from real-time clients.
> > > > The SAFE signal from such clients is used to qualify the start of
> > > > invalidation.
> > > > This logic is controlled by chicken bits, one for each - MDP (display),
> > > > IFE0, and IFE1 (camera), that can be accessed only from secure software
> > > > on sdm845.
> > > >
> > > > This configuration, however, degrades the performance of non-real time
> > > > clients, such as USB, and UFS etc. This happens because, with 
> > > > wait-for-safe
> > > > logic enabled the hardware tries to throttle non-real time clients while
> > > > waiting for SAFE ack signals from real-time clients.
> > > >
> > > > On MTP sdm845 devices, with wait-for-safe logic enabled at the boot time
> > > > by the bootloaders we see degraded performance of USB and UFS when 
> > > > kernel
> > > > enables the smmu stage-1 translations for these clients.
> > > > Turn off this wait-for-safe logic from the kernel gets us back the perf
> > > > of USB and UFS devices until we re-visit this when we start seeing perf
> > > > issues on display/camera on upstream supported SDM845 platforms.
>
> Re-visit what exactly, and how?
>
> > > > Now, different bootloaders with their access control policies allow this
> > > > register access differently through secure monitor calls -
> > > > 1) With one we can issue io-read/write secure monitor call (qcom-scm)
> > > > to update the register, while,
> > > > 2) With other, such as one on MTP sdm845 we should use the specific
> > > > qcom-scm command to send request to do the complete register
> > > > configuration.
> > > > Adding a separate device tree flag for arm-smmu to identify which
> > > > firmware configuration of the two mentioned above we use.
> > > > Not adding code change to allow type-(1) bootloaders to toggle the
> > > > safe using io-read/write qcom-scm call.
> > > >
> > > > This change is inspired by the downstream change from Patrick Daly
> > > > to address performance issues with display and camera by handling
> > > > this wait-for-safe within separte io-pagetable ops to do TLB
> > > > maintenance. So a big thanks to him for the change.
> > > >
> > > > Without this change the UFS reads are pretty slow:
> > > > $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=10 conv=sync
> > > > 10+0 records in
> > > > 10+0 records out
> > > > 10485760 bytes (10.0MB) copied, 22.394903 seconds, 457.2KB/s
> > > > real0m 22.39s
> > > > user0m 0.00s
> > > > sys 0m 0.01s
> > > >
> > > > With this change they are back to rock!
> > > > $ time dd if=/dev/sda of=/dev/zero bs=1048576 count=300 conv=sync
> > > > 300+0 records in
> > > > 300+0 records out
> > > > 314572800 bytes (300.0MB) copied, 1.030541 seconds, 291.1MB/s
> > > > real0m 1.03s
> > > > user0m 0.00s
> > > > sys 0m 0.54s
> > > >
> > > > Signed-off-by: Vivek Gautam 
> > > > ---
> > > >   drivers/iommu/arm-smmu.c | 16 
> > > >   1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > > index 0ad086da399c..3c3ad43eda97 100644
> > > > --- a/drivers/iommu/arm-smmu.c
> > > > +++ b/drivers/iommu/arm-smmu.c
> > > > @@ -39,6 +39,7 @@
> > > >   #include 
> > > >   #include 
> > > >   #include 
> > > > +#include 
> > > >   #include 
> > > >   #include 
> > > > @@ -177,6 +178,7 @@ struct arm_smmu_device {
> > > >   u32 features;
> > > >   #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
> > > > +#define ARM_SMMU_OPT_QCOM_FW_IMPL_SAFE_ERRATA (1 << 1)
> > > >   u32 options;
> > > >   enum arm_smmu_arch_version  version;
> > > >   enum arm_smmu_implementationmodel;
> > > > @@ -262,6 +264,7 @@ static bool using_legacy_binding, 
> > > > using_generic_binding;
> > > >   static struct arm_smmu_option_prop arm_smmu_options[] = {
> > > >   { ARM_SMMU_OPT_SECURE_CFG_ACCESS, 
> > > > "calxeda,smmu-secure-config-access" },
> > > > + { ARM_SMMU_OPT_QCOM_FW_IMPL_SAFE_ERRATA, 
> > > > "qcom,smmu-500-fw-impl-safe-errata" },
> > > This should be added to the DT binding as well.
> >
> > Ah right. I missed that. Will add this and respin unless Robin and Will have
> > concerns with this change.
>
> My only concern really is whether it's safe for us to turn this off. It's
> clear that somebody went to a lot of effort to add this extra goodness to
> the IP, but your benchmarks suggest they

Re: [RFC] switch m68k to use the generic remapping DMA allocator

2019-06-24 Thread Geert Uytterhoeven
Hi Christoph,

On Fri, Jun 14, 2019 at 12:21 PM Christoph Hellwig  wrote:
> can you take a look at the (untested) patches below?  They convert m68k
> to use the generic remapping DMA allocator, which is also used by
> arm64 and csky.

When building for Sun-3:

kernel/dma/remap.o: In function `arch_dma_alloc':
remap.c:(.text+0x316): undefined reference to `__dma_direct_alloc_pages'
remap.c:(.text+0x32a): undefined reference to `arch_dma_prep_coherent'
remap.c:(.text+0x34e): undefined reference to `arch_dma_mmap_pgprot'
remap.c:(.text+0x378): undefined reference to `__dma_direct_free_pages'
remap.c:(.text+0x3f4): undefined reference to `arch_dma_prep_coherent'
remap.c:(.text+0x40a): undefined reference to `__dma_direct_alloc_pages'
kernel/dma/remap.o: In function `arch_dma_free':
remap.c:(.text+0x446): undefined reference to `__dma_direct_free_pages'
remap.c:(.text+0x4a8): undefined reference to `__dma_direct_free_pages'
kernel/dma/remap.o: In function `dma_atomic_pool_init':
remap.c:(.init.text+0x66): undefined reference to `arch_dma_prep_coherent'

Doing

-   select DMA_DIRECT_REMAP if MMU && !COLDFIRE
+   select DMA_DIRECT_REMAP if MMU && !COLDFIRE && !SUN3

in arch/m68k/Kconfig fixes the build.

Alternatively, you could use:

-   select DMA_DIRECT_REMAP if MMU && !COLDFIRE
+   select DMA_DIRECT_REMAP if HAS_DMA && MMU && !COLDFIRE

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds