Re: [PATCH v3 03/14] iommu/mediatek: Add device_link between the consumer and the larb devices

2020-03-05 Thread Yong Wu
On Thu, 2020-03-05 at 13:14 +0800, Nicolas Boichat wrote:
> On Tue, Sep 3, 2019 at 5:38 PM Yong Wu  wrote:
> >
> > MediaTek IOMMU don't have its power-domain. all the consumer connect
> > with smi-larb, then connect with smi-common.
> >
> > M4U
> >  |
> > smi-common
> >  |
> >   -
> >   | |...
> >   | |
> > larb1 larb2
> >   | |
> > vdec   venc
> >
> > When the consumer works, it should enable the smi-larb's power which
> > also need enable the smi-common's power firstly.
> >
> > Thus, First of all, use the device link connect the consumer and the
> > smi-larbs. then add device link between the smi-larb and smi-common.
> >
> > This patch adds device_link between the consumer and the larbs.
> >
> > When device_link_add, I add the flag DL_FLAG_STATELESS to avoid calling
> > pm_runtime_xx to keep the original status of clocks. It can avoid two
> > issues:
> > 1) Display HW show fastlogo abnormally reported in [1]. At the beggining,
> > all the clocks are enabled before entering kernel, but the clocks for
> > display HW(always in larb0) will be gated after clk_enable and clk_disable
> > called from device_link_add(->pm_runtime_resume) and rpm_idle. The clock
> > operation happened before display driver probe. At that time, the display
> > HW will be abnormal.
> >
> > 2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
> > pm_runtime_xx to avoid the deadlock.
> >
> > Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
> > device_link_removed should be added explicitly.
> >
> > [1] http://lists.infradead.org/pipermail/linux-mediatek/2019-July/
> > 021500.html
> > [2] https://lore.kernel.org/patchwork/patch/1086569/
> >
> > Suggested-by: Tomasz Figa 
> > Signed-off-by: Yong Wu 
> > ---
> >  drivers/iommu/mtk_iommu.c| 17 +
> >  drivers/iommu/mtk_iommu_v1.c | 18 +-
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index b138b94..2511b3c 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -450,6 +450,9 @@ static int mtk_iommu_add_device(struct device *dev)
> > struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > struct mtk_iommu_data *data;
> > struct iommu_group *group;
> > +   struct device_link *link;
> > +   struct device *larbdev;
> > +   unsigned int larbid;
> >
> > if (!fwspec || fwspec->ops != _iommu_ops)
> > return -ENODEV; /* Not a iommu client device */
> > @@ -461,6 +464,14 @@ static int mtk_iommu_add_device(struct device *dev)
> > if (IS_ERR(group))
> > return PTR_ERR(group);
> >
> > +   /* Link the consumer device with the smi-larb device(supplier) */
> > +   larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> 
> I'll mirror the comment I made on gerrit
> (https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1361013):
> Maybe I'm missing something here, but for example, on MT8173,
> vcodec_enc: vcodec@18002000 needs to use both larb3 and larb5, isn't
> the code below just adding a link for larb3?

Yes. It only add larb3 here.

> 
> Do we need to iterate over all fwspecs->ids to figure out which larbs
> we need to add links to each of them?

We have checked this venc issue. Currently I have requested our venc guy
to seperate larb3-venc and larb5-venc in the driver[1] since they are
independent HW actually. I will put it into this series when I send next
version.

If there is some reasonable driver which have two larbs in it, then the
iterating is really necessary, But I don't see it right now. Only using
fwspec->ids[0] is enough for now.

[1]
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1958322

> 
> > +   larbdev = data->larb_imu[larbid].dev;
> > +   link = device_link_add(dev, larbdev,
> > +  DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
> > +   if (!link)
> > +   dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
> > +
> > iommu_group_put(group);
> > return 0;
> >  }
> > @@ -469,6 +480,8 @@ static void mtk_iommu_remove_device(struct device *dev)
> >  {
> > struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > struct mtk_iommu_data *data;
> > +   struct device *larbdev;
> > +   unsigned int larbid;
> >
> > if (!fwspec || fwspec->ops != _iommu_ops)
> > return;
> > @@ -476,6 +489,10 @@ static void mtk_iommu_remove_device(struct device *dev)
> > data = fwspec->iommu_priv;
> > iommu_device_unlink(>iommu, dev);
> >
> > +   larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> > +   larbdev = data->larb_imu[larbid].dev;
> > +   device_link_remove(dev, larbdev);
> > +
> > iommu_group_remove_device(dev);
> > iommu_fwspec_free(dev);
> >  }
> > diff --git 

Re: [PATCH v2] uacce: unmap remaining mmapping from user space

2020-03-05 Thread zhangfei



On 2020/3/6 上午9:51, Herbert Xu wrote:

On Wed, Feb 26, 2020 at 03:12:06PM +0800, Zhangfei Gao wrote:

When uacce parent device module is removed, user app may
still keep the mmaped area, which can be accessed unsafely.
When rmmod, Parent device driver will call uacce_remove,
which unmap all remaining mapping from user space for safety.
VM_FAULT_SIGBUS is also reported to user space accordingly.

Suggested-by: Dave Jiang 
Signed-off-by: Zhangfei Gao 
---
  v2: Unmap before put_queue, where memory is freed, commented from Zaibo.

  drivers/misc/uacce/uacce.c | 16 
  include/linux/uacce.h  |  2 ++
  2 files changed, 18 insertions(+)

Patch applied.  Thanks.

Thanks Herbert for the help.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] uacce: unmap remaining mmapping from user space

2020-03-05 Thread Herbert Xu
On Wed, Feb 26, 2020 at 03:12:06PM +0800, Zhangfei Gao wrote:
> When uacce parent device module is removed, user app may
> still keep the mmaped area, which can be accessed unsafely.
> When rmmod, Parent device driver will call uacce_remove,
> which unmap all remaining mapping from user space for safety.
> VM_FAULT_SIGBUS is also reported to user space accordingly.
> 
> Suggested-by: Dave Jiang 
> Signed-off-by: Zhangfei Gao 
> ---
>  v2: Unmap before put_queue, where memory is freed, commented from Zaibo.
> 
>  drivers/misc/uacce/uacce.c | 16 
>  include/linux/uacce.h  |  2 ++
>  2 files changed, 18 insertions(+)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2] MAINTAINERS: add maintainers for uacce

2020-03-05 Thread Herbert Xu
On Wed, Feb 26, 2020 at 09:28:28AM +0800, Zhangfei Gao wrote:
> Add Zhangfei Gao and Zhou Wang as maintainers for uacce
> 
> Signed-off-by: Zhangfei Gao 
> Signed-off-by: Zhou Wang 
> ---
> Add list, suggested by Dave
> 
> MAINTAINERS | 12 
>  1 file changed, 12 insertions(+)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Possible bugs in iommu_map_sg()/iommu_map_dma_sg()

2020-03-05 Thread Nicolin Chen
Hi all,

I recently ran a 4GB+ allocation test case with my downstream
older-version kernel, and found two possible bugs. I then saw
the mainline code, yet don't find them getting fixed.

However, I am not 100% sure that they are real practical bugs
because I later figured out that my use case was not entirely
correct. So I'd like to get some advice first, before sending
any patch.


First problem is accumulating the pad_len in iommu_map_dma_sg.
My use case was to map a size of 4GB+ sg while the device did
not set its segmentation boundary -- leaving it to the default
32-bit mask.

00 of 14: s_length9, s->length9, iova_len 0
01 of 14: s_length   10, s->length   10, iova_len 9
02 of 14: s_length   10, s->length   10, iova_len 19
03 of 14: s_length   20, s->length   20, iova_len 29
04 of 14: s_length   20, s->length   20, iova_len 49
05 of 14: s_length 39c0, s->length 39c0, iova_len 69
06 of 14: s_length   40, s->length   40, iova_len 3a29
07 of 14: s_length   40, s->length   40, iova_len 3a69
08 of 14: s_length   40, s->length   40, iova_len 3aa9
09 of 14: s_length   40, s->length   40, iova_len 3ae9
10 of 14: s_length   40, s->length   40, iova_len 3b29
11 of 14: s_length   40, s->length   40, iova_len 3b69
12 of 14: s_length f000, s->length f000, iova_len 3ba9
12 of 14: prev->length 40 + pad_len c457 = c497
13 of 14: s_length 1df41000, s->length 1df41000, iova_len 1f000
13 of 14: prev->length f000 + pad_len 1000 = 1

So, the problem here is adding the last pad_len 0x1000 to the
prev->length 0xf000, and writing the result back:
880 if (pad_len && pad_len < s_length - 1) {
881 prev->length += pad_len;

This 0x1 overflows that "unsigned int" prev->length.


Second problem is in the iommu_map_sg function. When it maps
iova to phys for each list, it uses previously padded length
instead of the actual s->dma_length, which means it possibly
maps some of the iova space to a physical address space that
is out of the allocated region. For a large value of pad_len
0xc457, it might end up map iova to somewhere invalid:
iova [0xc3b69, 0xd] ==>
  pa [0x00026280, 0x00032717]
  // size 0xc497, dma_size 0x40

This 0x32717 is out of actual physical address space for
my platform. And even for the small 0x1000 pad_len, it still
maps out of the allocated region.


For my use case, I made it work by setting the segmentation
boundary to a larger size, which shouldn't be wrong because
I need a contiguous iova space with no paddings in-between.

Yet, since the code is designed to take care of a "mask size
< IOVA size" case, I feel that we probably need to fix these
two issues.

For problem 1, should we change the type of length to size_t?

For problem 2, should we map each iova<=>pa using dma_length?

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


[PATCH] iommu/vt-d: fix RCU-list bugs in intel_iommu_init

2020-03-05 Thread Qian Cai
There are several places traverse RCU-list without holding any lock in
intel_iommu_init(). Fix them by acquiring dmar_global_lock.

 WARNING: suspicious RCU usage
 -
 drivers/iommu/intel-iommu.c:5216 RCU-list traversed in non-reader section!!

 other info that might help us debug this:

 rcu_scheduler_active = 2, debug_locks = 1
 no locks held by swapper/0/1.

 Call Trace:
  dump_stack+0xa0/0xea
  lockdep_rcu_suspicious+0x102/0x10b
  intel_iommu_init+0x947/0xb13
  pci_iommu_init+0x26/0x62
  do_one_initcall+0xfe/0x500
  kernel_init_freeable+0x45a/0x4f8
  kernel_init+0x11/0x139
  ret_from_fork+0x3a/0x50
 DMAR: Intel(R) Virtualization Technology for Directed I/O

Fixes: d8190dc63886 ("iommu/vt-d: Enable DMA remapping after rmrr mapped")
Signed-off-by: Qian Cai 
---
 drivers/iommu/intel-iommu.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6fa6de2b6ad5..bc138ceb07bc 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5193,6 +5193,7 @@ int __init intel_iommu_init(void)
 
init_iommu_pm_ops();
 
+   down_read(_global_lock);
for_each_active_iommu(iommu, drhd) {
iommu_device_sysfs_add(>iommu, NULL,
   intel_iommu_groups,
@@ -5200,6 +5201,7 @@ int __init intel_iommu_init(void)
iommu_device_set_ops(>iommu, _iommu_ops);
iommu_device_register(>iommu);
}
+   up_read(_global_lock);
 
bus_set_iommu(_bus_type, _iommu_ops);
if (si_domain && !hw_pass_through)
@@ -5210,7 +5212,6 @@ int __init intel_iommu_init(void)
down_read(_global_lock);
if (probe_acpi_namespace_devices())
pr_warn("ACPI name space devices didn't probe correctly\n");
-   up_read(_global_lock);
 
/* Finally, we enable the DMA remapping hardware. */
for_each_iommu(iommu, drhd) {
@@ -5219,6 +5220,8 @@ int __init intel_iommu_init(void)
 
iommu_disable_protect_mem_regions(iommu);
}
+   up_read(_global_lock);
+
pr_info("Intel(R) Virtualization Technology for Directed I/O\n");
 
intel_iommu_enabled = 1;
-- 
1.8.3.1

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


[PATCH -next] iommu/dmar: silence RCU-list debugging warnings

2020-03-05 Thread Qian Cai
Similar to the commit 02d715b4a818 ("iommu/vt-d: Fix RCU list debugging
warnings"), there are several other places that call
list_for_each_entry_rcu() outside of an RCU read side critical section
but with dmar_global_lock held. Silence those false positives as well.

 drivers/iommu/intel-iommu.c:4288 RCU-list traversed in non-reader section!!
 1 lock held by swapper/0/1:
  #0: 935892c8 (dmar_global_lock){+.+.}, at: 
intel_iommu_init+0x1ad/0xb97

 drivers/iommu/dmar.c:366 RCU-list traversed in non-reader section!!
 1 lock held by swapper/0/1:
  #0: 935892c8 (dmar_global_lock){+.+.}, at: 
intel_iommu_init+0x125/0xb97

 drivers/iommu/intel-iommu.c:5057 RCU-list traversed in non-reader section!!
 1 lock held by swapper/0/1:
  #0: a71892c8 (dmar_global_lock){}, at: 
intel_iommu_init+0x61a/0xb13

Signed-off-by: Qian Cai 
---
 drivers/iommu/dmar.c | 3 ++-
 include/linux/dmar.h | 6 --
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 071bb42bbbc5..7b16c4db40b4 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -363,7 +363,8 @@ static int dmar_pci_bus_notifier(struct notifier_block *nb,
 {
struct dmar_drhd_unit *dmaru;
 
-   list_for_each_entry_rcu(dmaru, _drhd_units, list)
+   list_for_each_entry_rcu(dmaru, _drhd_units, list,
+   dmar_rcu_check())
if (dmaru->segment == drhd->segment &&
dmaru->reg_base_addr == drhd->address)
return dmaru;
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index 712be8bc6a7c..d7bf029df737 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -74,11 +74,13 @@ struct dmar_pci_notify_info {
dmar_rcu_check())
 
 #define for_each_active_drhd_unit(drhd)
\
-   list_for_each_entry_rcu(drhd, _drhd_units, list)   \
+   list_for_each_entry_rcu(drhd, _drhd_units, list,   \
+   dmar_rcu_check())   \
if (drhd->ignored) {} else
 
 #define for_each_active_iommu(i, drhd) \
-   list_for_each_entry_rcu(drhd, _drhd_units, list)   \
+   list_for_each_entry_rcu(drhd, _drhd_units, list,   \
+   dmar_rcu_check())   \
if (i=drhd->iommu, drhd->ignored) {} else
 
 #define for_each_iommu(i, drhd)
\
-- 
1.8.3.1

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


Re: [PATCH v4 01/26] mm/mmu_notifiers: pass private data down to alloc_notifier()

2020-03-05 Thread Christoph Hellwig
On Mon, Feb 24, 2020 at 07:23:36PM +0100, Jean-Philippe Brucker wrote:
> -static struct mmu_notifier *gru_alloc_notifier(struct mm_struct *mm)
> +static struct mmu_notifier *gru_alloc_notifier(struct mm_struct *mm, void 
> *privdata)

Pleae don't introduce any > 80 char lines.  Not here, and not anywhere
else in this patch or the series.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [rfc 6/6] dma-remap: double the default DMA coherent pool size

2020-03-05 Thread Christoph Hellwig
On Sun, Mar 01, 2020 at 04:05:27PM -0800, David Rientjes wrote:
> When AMD memory encryption is enabled, some devices may used more than
> 256KB/sec from the atomic pools.  Double the default size to make the
> original size and expansion more appropriate.
> 
> This provides a slight optimization on initial expansion and is deemed
> appropriate for all configs with CONFIG_DMA_REMAP enabled because of the
> increased reliance on the atomic pools.
> 
> Alternatively, this could be done only when CONFIG_AMD_MEM_ENCRYPT is
> enabled.

Can we just scale the initial size based on the system memory size?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [rfc 5/6] dma-direct: atomic allocations must come from unencrypted pools

2020-03-05 Thread Christoph Hellwig
On Sun, Mar 01, 2020 at 04:05:23PM -0800, David Rientjes wrote:
> When AMD memory encryption is enabled, all non-blocking DMA allocations
> must originate from the atomic pools depending on the device and the gfp
> mask of the allocation.
> 
> Keep all memory in these pools unencrypted.
> 
> Signed-off-by: David Rientjes 
> ---
>  arch/x86/Kconfig| 1 +
>  kernel/dma/direct.c | 9 -
>  kernel/dma/remap.c  | 2 ++
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1523,6 +1523,7 @@ config X86_CPA_STATISTICS
>  config AMD_MEM_ENCRYPT
>   bool "AMD Secure Memory Encryption (SME) support"
>   depends on X86_64 && CPU_SUP_AMD
> + select DMA_DIRECT_REMAP

I think we need to split the pool from remapping so that we don't drag
in the remap code for x86.

>   if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
> - dma_alloc_need_uncached(dev, attrs) &&

We still need a check here for either uncached or memory encryption.

> @@ -141,6 +142,7 @@ static int atomic_pool_expand(struct gen_pool *pool, 
> size_t pool_size,
>   if (!addr)
>   goto free_page;
>  
> + set_memory_decrypted((unsigned long)page_to_virt(page), nr_pages);

This probably warrants a comment.

Also I think the infrastructure changes should be split from the x86
wire up.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [rfc 3/6] dma-remap: wire up the atomic pools depending on gfp mask

2020-03-05 Thread Christoph Hellwig
On Sun, Mar 01, 2020 at 04:05:16PM -0800, David Rientjes wrote:
> 
> When allocating non-blockable memory, determine the optimal gfp mask of
> the device and use the appropriate atomic pool.
> 
> The coherent DMA mask will remain the same between allocation and free
> and, thus, memory will be freed to the same atomic pool it was allocated
> from.

I think this should go into the previous patch, as it is rather pointless
without the changes in this one.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [rfc 2/6] dma-remap: add additional atomic pools to map to gfp mask

2020-03-05 Thread Christoph Hellwig
On Sun, Mar 01, 2020 at 04:05:13PM -0800, David Rientjes wrote:
> The single atomic pool is allocated from the lowest zone possible since
> it is guaranteed to be applicable for any DMA allocation.
> 
> Devices may allocate through the DMA API but not have a strict reliance
> on GFP_DMA memory.  Since the atomic pool will be used for all
> non-blockable allocations, returning all memory from ZONE_DMA may
> unnecessarily deplete the zone.
> 
> Provision for multiple atomic pools that will map to the optimal gfp
> mask of the device.  These will be wired up in a subsequent patch.
> 
> Signed-off-by: David Rientjes 
> ---
>  kernel/dma/remap.c | 75 +++---
>  1 file changed, 45 insertions(+), 30 deletions(-)
> 
> diff --git a/kernel/dma/remap.c b/kernel/dma/remap.c
> --- a/kernel/dma/remap.c
> +++ b/kernel/dma/remap.c
> @@ -100,6 +100,8 @@ void dma_common_free_remap(void *cpu_addr, size_t size)
>  
>  #ifdef CONFIG_DMA_DIRECT_REMAP
>  static struct gen_pool *atomic_pool __ro_after_init;
> +static struct gen_pool *atomic_pool_dma32 __ro_after_init;
> +static struct gen_pool *atomic_pool_normal __ro_after_init;

Maybe rename atomic_pool as well as it really kinda looks like the
default at the moment?

>  
>  #define DEFAULT_DMA_COHERENT_POOL_SIZE  SZ_256K
>  static size_t atomic_pool_size __initdata = DEFAULT_DMA_COHERENT_POOL_SIZE;
> @@ -111,66 +113,79 @@ static int __init early_coherent_pool(char *p)
>  }
>  early_param("coherent_pool", early_coherent_pool);
>  
> -static gfp_t dma_atomic_pool_gfp(void)
> +static int __init __dma_atomic_pool_init(struct gen_pool **pool,
> +  size_t pool_size, gfp_t gfp)
>  {

Can this just return the pool and return NULL (or an ERR_PTR) on
failure?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


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

2020-03-05 Thread Michael S. Tsirkin
On Wed, Mar 04, 2020 at 10:54:49PM +0100, Joerg Roedel wrote:
> On Wed, Mar 04, 2020 at 01:37:44PM -0800, Jacob Pan (Jun) wrote:
> > + Mike and Erik who work closely on UEFI and ACPICA.
> > 
> > Copy paste Erik's initial response below on how to get a new table,
> > seems to confirm with the process you stated above.
> > 
> > "Fairly easy. You reserve a 4-letter symbol by sending a message
> > requesting to reserve the signature to Mike or the ASWG mailing list or
> > i...@acpi.info
> 
> Great! I think this is going to be the path forward here.
> 
> Regards,
> 
>   Joerg

I don't, I don't see why we should bother with ACPI. This is a PV device
anyway, we can just make it self-describing. The need for extra firmware
on some platforms is a bug not a feature. No point in emulating that.

> > 
> > There is also another option. You can have ASWG own this new table so
> > that not one entity or company owns the new table."

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


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

2020-03-05 Thread Michael S. Tsirkin
On Wed, Mar 04, 2020 at 10:50:02PM +0100, Joerg Roedel wrote:
> On Wed, Mar 04, 2020 at 02:34:33PM -0500, Michael S. Tsirkin wrote:
> > All these extra levels of indirection is one of the reasons
> > hypervisors such as kata try to avoid ACPI.
> 
> Platforms that don't use ACPI need another hardware detection mechanism,
> which can also be supported. But the first step here is to enable the
> general case, and for x86 platforms this means ACPI.
> 
> Regards,
> 
>   Joerg

Frankly since a portable way to do it is needed anyway I don't see why
we also need ACPI.

-- 
MST

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


Re: [iommu:arm/omap 4/4] drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:20: error: implicit declaration of function 'vmap'; did you mean 'bmap'?

2020-03-05 Thread Krzysztof Kozlowski
On Thu, 5 Mar 2020 at 02:00, kbuild test robot  wrote:
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git 
> arm/omap
> head:   e93a1695d7fb551376b1c1220a267d032b6ad159
> commit: e93a1695d7fb551376b1c1220a267d032b6ad159 [4/4] iommu: Enable compile 
> testing for some of drivers
> config: sparc-allyesconfig (attached as .config)
> compiler: sparc64-linux-gcc (GCC) 7.5.0
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout e93a1695d7fb551376b1c1220a267d032b6ad159
> # save the attached .config to linux build tree
> GCC_VERSION=7.5.0 make.cross ARCH=sparc
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot 
>
> All errors (new ones prefixed by >>):
>
>drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function 
> 'rockchip_gem_alloc_iommu':
> >> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:20: error: implicit 
> >> declaration of function 'vmap'; did you mean 'bmap'? 
> >> [-Werror=implicit-function-declaration]
>   rk_obj->kvaddr = vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
>^~~~
>bmap
> >> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:59: error: 'VM_MAP' 
> >> undeclared (first use in this function); did you mean 'VM_MPX'?
>   rk_obj->kvaddr = vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
>   ^~
>   VM_MPX
>drivers/gpu/drm/rockchip/rockchip_drm_gem.c:134:59: note: each undeclared 
> identifier is reported only once for each function it appears in
>drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function 
> 'rockchip_gem_free_iommu':
> >> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:190:2: error: implicit 
> >> declaration of function 'vunmap'; did you mean 'iounmap'? 
> >> [-Werror=implicit-function-declaration]
>  vunmap(rk_obj->kvaddr);
>  ^~
>  iounmap
>drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function 
> 'rockchip_gem_prime_vmap':
>drivers/gpu/drm/rockchip/rockchip_drm_gem.c:547:49: error: 'VM_MAP' 
> undeclared (first use in this function); did you mean 'VM_MPX'?
>   return vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
> ^~
> VM_MPX
>cc1: some warnings being treated as errors
>
> vim +134 drivers/gpu/drm/rockchip/rockchip_drm_gem.c

Hi,

+Cc Heiko,

This is already fixed in drm-misc here:
https://patchwork.freedesktop.org/patch/347106/

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


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

2020-03-05 Thread Tian, Kevin
> From: Jean-Philippe Brucker
> Sent: Saturday, February 29, 2020 1:26 AM
> 
> Platforms without device-tree do not currently have a method for
> describing the vIOMMU topology. Provide a topology description embedded
> into the virtio device.
> 
> Use PCI FIXUP to probe the config space early, because we need to
> discover the topology before any DMA configuration takes place, and the
> virtio driver may be loaded much later. Since we discover the topology
> description when probing the PCI hierarchy, the virtual IOMMU cannot
> manage other platform devices discovered earlier.
> 
> This solution isn't elegant nor foolproof, but is the best we can do at

can you elaborate "isn't elegant nor foolproof" part? is there any other 
limitation (beside pci fixup) along the route, when comparing it to 
the ACPI-approach?

> the moment and works with existing virtio-iommu implementations. It also
> enables an IOMMU for lightweight hypervisors that do not rely on
> firmware methods for booting.
> 
> Signed-off-by: Eric Auger 
> Signed-off-by: Jean-Philippe Brucker 
> ---
>  MAINTAINERS   |   2 +
>  drivers/iommu/Kconfig |  10 +
>  drivers/iommu/Makefile|   1 +
>  drivers/iommu/virtio-iommu-topology.c | 343
> ++
>  drivers/iommu/virtio-iommu.c  |   3 +
>  include/linux/virt_iommu.h|  19 ++
>  include/uapi/linux/virtio_iommu.h |  26 ++
>  7 files changed, 404 insertions(+)
>  create mode 100644 drivers/iommu/virtio-iommu-topology.c
>  create mode 100644 include/linux/virt_iommu.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fcd79fc38928..65a03ce53096 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17781,6 +17781,8 @@ M:Jean-Philippe Brucker  phili...@linaro.org>
>  L:   virtualizat...@lists.linux-foundation.org
>  S:   Maintained
>  F:   drivers/iommu/virtio-iommu.c
> +F:   drivers/iommu/virtio-iommu-topology.c
> +F:   include/linux/virt_iommu.h
>  F:   include/uapi/linux/virtio_iommu.h
> 
>  VIRTUAL BOX GUEST DEVICE DRIVER
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index c5df570ef84a..f8cb45d84bb0 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -516,4 +516,14 @@ config VIRTIO_IOMMU
> 
> Say Y here if you intend to run this kernel as a guest.
> 
> +config VIRTIO_IOMMU_TOPOLOGY
> + bool "Topology properties for the virtio-iommu"
> + depends on VIRTIO_IOMMU
> + default y
> + help
> +   Enable early probing of the virtio-iommu device, to detect the
> +   built-in topology description.
> +
> +   Say Y here if you intend to run this kernel as a guest.
> +
>  endif # IOMMU_SUPPORT
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 9f33fdb3bb05..5da24280d08c 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
>  obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
>  obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
>  obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> +obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY) += virtio-iommu-topology.o
> diff --git a/drivers/iommu/virtio-iommu-topology.c b/drivers/iommu/virtio-
> iommu-topology.c
> new file mode 100644
> index ..2188624ef216
> --- /dev/null
> +++ b/drivers/iommu/virtio-iommu-topology.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct viommu_cap_config {
> + u8 bar;
> + u32 length; /* structure size */
> + u32 offset; /* structure offset within the bar */
> +};
> +
> +union viommu_topo_cfg {
> + __le16  type;
> + struct virtio_iommu_topo_pci_range  pci;
> + struct virtio_iommu_topo_endpoint   ep;
> +};
> +
> +struct viommu_spec {
> + struct device   *dev; /* transport device */
> + struct fwnode_handle*fwnode;
> + struct iommu_ops*ops;
> + struct list_headlist;
> + size_t  num_items;
> + /* The config array of length num_items follows */
> + union viommu_topo_cfg   cfg[];
> +};
> +
> +static LIST_HEAD(viommus);
> +static DEFINE_MUTEX(viommus_lock);
> +
> +#define VPCI_FIELD(field) offsetof(struct virtio_pci_cap, field)
> +
> +static inline int viommu_pci_find_capability(struct pci_dev *dev, u8 
> cfg_type,
> +  struct viommu_cap_config *cap)
> +{
> + int pos;
> + u8 bar;
> +
> + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> +  pos > 0;
> +  pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> + u8 type;
> +
> + pci_read_config_byte(dev, pos + VPCI_FIELD(cfg_type),
> );
> + if (type