[v5,0/5] add non-strict mode support for arm-smmu-v3

Message

Leizhen (ThunderTown) Aug. 15, 2018, 1:28 a.m. UTC

v4 -> v5:
1. change the type of global variable and struct member named "non_strict" from
   "int" to "bool".
2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
   in v4.
3. change boot option "arm_iommu" to "iommu.non_strict".
4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
   non-leaf unmaps still need to be synchronous.

Thanks for Robin's review comments.

v3 -> v4:
1. Add a new member "non_strict" in struct iommu_domain to mark whether
   that domain use non-strict mode or not. This can help us to remove the
   capability which was added in prior version.
2. Add a new quirk IO_PGTABLE_QUIRK_NON_STRICT, so that we can get "strict
   mode" in io-pgtable-arm.c according to data->iop.cfg.quirks.
3. rename the new boot option to "arm_iommu".

v2 -> v3:
Add a bootup option "iommu_strict_mode" to make the manager can choose which
mode to be used. The first 5 patches have not changed.
+	iommu_strict_mode=	[arm-smmu-v3]
+		0 - strict mode (default)
+		1 - non-strict mode

v1 -> v2:
Use the lowest bit of the io_pgtable_ops.unmap's iova parameter to pass the strict mode:
0, IOMMU_STRICT;
1, IOMMU_NON_STRICT;
Treat 0 as IOMMU_STRICT, so that the unmap operation can compatible with
other IOMMUs which still use strict mode. In other words, this patch series
will not impact other IOMMU drivers. I tried add a new quirk IO_PGTABLE_QUIRK_NON_STRICT
in io_pgtable_cfg.quirks, but it can not pass the strict mode of the domain from SMMUv3
driver to io-pgtable module. 

Add a new member domain_non_strict in struct iommu_dma_cookie, this member will only be
initialized when the related domain and IOMMU driver support non-strict mode.

v1:
In common, a IOMMU unmap operation follow the below steps:
1. remove the mapping in page table of the specified iova range
2. execute tlbi command to invalid the mapping which is cached in TLB
3. wait for the above tlbi operation to be finished
4. free the IOVA resource
5. free the physical memory resource

This maybe a problem when unmap is very frequently, the combination of tlbi
and wait operation will consume a lot of time. A feasible method is put off
tlbi and iova-free operation, when accumulating to a certain number or
reaching a specified time, execute only one tlbi_all command to clean up
TLB, then free the backup IOVAs. Mark as non-strict mode.

But it must be noted that, although the mapping has already been removed in
the page table, it maybe still exist in TLB. And the freed physical memory
may also be reused for others. So a attacker can persistent access to memory
based on the just freed IOVA, to obtain sensible data or corrupt memory. So
the VFIO should always choose the strict mode.

Some may consider put off physical memory free also, that will still follow
strict mode. But for the map_sg cases, the memory allocation is not controlled
by IOMMU APIs, so it is not enforceable.

Fortunately, Intel and AMD have already applied the non-strict mode, and put
queue_iova() operation into the common file dma-iommu.c., and my work is based
on it. The difference is that arm-smmu-v3 driver will call IOMMU common APIs to
unmap, but Intel and AMD IOMMU drivers are not.

Below is the performance data of strict vs non-strict for NVMe device:
Randomly Read  IOPS: 146K(strict) vs 573K(non-strict)
Randomly Write IOPS: 143K(strict) vs 513K(non-strict)

Zhen Lei (5):
  iommu/arm-smmu-v3: fix the implementation of flush_iotlb_all hook
  iommu/dma: add support for non-strict mode
  iommu/io-pgtable-arm: add support for non-strict mode
  iommu/arm-smmu-v3: add support for non-strict mode
  iommu/arm-smmu-v3: add bootup option "iommu.non_strict"

 Documentation/admin-guide/kernel-parameters.txt | 13 +++++++++
 drivers/iommu/arm-smmu-v3.c                     | 35 ++++++++++++++++++++++++-
 drivers/iommu/dma-iommu.c                       | 29 +++++++++++++++++++-
 drivers/iommu/io-pgtable-arm.c                  | 20 +++++++++-----
 drivers/iommu/io-pgtable.h                      |  3 +++
 drivers/iommu/iommu.c                           |  1 +
 include/linux/iommu.h                           |  1 +
 7 files changed, 94 insertions(+), 8 deletions(-)

Comments

Will Deacon Sept. 12, 2018, 4:57 p.m. UTC | #1

Hi all,

On Wed, Aug 15, 2018 at 09:28:25AM +0800, Zhen Lei wrote:
> v4 -> v5:
> 1. change the type of global variable and struct member named "non_strict" from
>    "int" to "bool".
> 2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
>    in v4.
> 3. change boot option "arm_iommu" to "iommu.non_strict".
> 4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
>    non-leaf unmaps still need to be synchronous.
> 
> Thanks for Robin's review comments.

Since this is 90% of the way there now, I suggest Robin picks up what's here
and incorporates his remaining review comments directly (especially since it
sounded like Zhen Lei hasn't got much free time lately). With that, I can
queue this lot via my smmu branch, which already has some stuff queued
for SMMUv3 and io-pgtable.

Please shout if you have any objections, but I'm keen for this not to
languish on the lists given how close it is!

Will

Robin Murphy Sept. 12, 2018, 5:12 p.m. UTC | #2

On 12/09/18 17:57, Will Deacon wrote:
> Hi all,
> 
> On Wed, Aug 15, 2018 at 09:28:25AM +0800, Zhen Lei wrote:
>> v4 -> v5:
>> 1. change the type of global variable and struct member named "non_strict" from
>>     "int" to "bool".
>> 2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
>>     in v4.
>> 3. change boot option "arm_iommu" to "iommu.non_strict".
>> 4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
>>     non-leaf unmaps still need to be synchronous.
>>
>> Thanks for Robin's review comments.
> 
> Since this is 90% of the way there now, I suggest Robin picks up what's here
> and incorporates his remaining review comments directly (especially since it
> sounded like Zhen Lei hasn't got much free time lately). With that, I can
> queue this lot via my smmu branch, which already has some stuff queued
> for SMMUv3 and io-pgtable.
> 
> Please shout if you have any objections, but I'm keen for this not to
> languish on the lists given how close it is!

Sure, having got this far I'd like to see it get done too. I'll make 
some time and give it a go.

Robin.

Leizhen (ThunderTown) Sept. 13, 2018, 2:02 a.m. UTC | #3

On 2018/9/13 1:12, Robin Murphy wrote:
> On 12/09/18 17:57, Will Deacon wrote:
>> Hi all,
>>
>> On Wed, Aug 15, 2018 at 09:28:25AM +0800, Zhen Lei wrote:
>>> v4 -> v5:
>>> 1. change the type of global variable and struct member named "non_strict" from
>>>     "int" to "bool".
>>> 2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
>>>     in v4.
>>> 3. change boot option "arm_iommu" to "iommu.non_strict".
>>> 4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
>>>     non-leaf unmaps still need to be synchronous.
>>>
>>> Thanks for Robin's review comments.
>>
>> Since this is 90% of the way there now, I suggest Robin picks up what's here
>> and incorporates his remaining review comments directly (especially since it
>> sounded like Zhen Lei hasn't got much free time lately). With that, I can
>> queue this lot via my smmu branch, which already has some stuff queued
>> for SMMUv3 and io-pgtable.
>>
>> Please shout if you have any objections, but I'm keen for this not to
>> languish on the lists given how close it is!
> 
> Sure, having got this far I'd like to see it get done too. I'll make some time and give it a go.

Thank you very much.

I've been too busy lately, at least I still have no time this week, I also think it's been too long.

So sorry and thanks again.

> 
> Robin.
> 
> .
>