On 2018/8/23 1:52, Robin Murphy wrote:
> On 15/08/18 02:28, Zhen Lei wrote:
>> To support the non-strict mode, now we only tlbi and sync for the strict
>> mode. But for the non-leaf case, always follow strict mode.
>>
>> Signed-off-by: Zhen Lei <thunder.leiz...@huawei.com>
>> ---
>>   drivers/iommu/io-pgtable-arm.c | 20 ++++++++++++++------
>>   drivers/iommu/io-pgtable.h     |  3 +++
>>   2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
>> index 010a254..20d3e98 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -538,6 +538,7 @@ static size_t arm_lpae_split_blk_unmap(struct 
>> arm_lpae_io_pgtable *data,
>>       phys_addr_t blk_paddr;
>>       size_t tablesz = ARM_LPAE_GRANULE(data);
>>       size_t split_sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
>> +    size_t unmapped = size;
>>       int i, unmap_idx = -1;
>>
>>       if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
>> @@ -575,11 +576,16 @@ static size_t arm_lpae_split_blk_unmap(struct 
>> arm_lpae_io_pgtable *data,
>>           tablep = iopte_deref(pte, data);
>>       }
>>
>> -    if (unmap_idx < 0)
> 
> [ side note: the more I see this test the more it looks slightly wrong, but 
> that's a separate issue. I'll have to sit down and really remember what's 
> going on here... ]
> 
>> -        return __arm_lpae_unmap(data, iova, size, lvl, tablep);
>> +    if (unmap_idx < 0) {
>> +        unmapped = __arm_lpae_unmap(data, iova, size, lvl, tablep);
>> +        if (!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT))
>> +            return unmapped;
>> +    }
> 
> I don't quite get this change - we should only be recursing back into 
> __arm_lpae_unmap() here if nothing's actually been unmapped at this point - 
> the block entry is simply replaced by a full next-level table and we're going 
> to come back and split another block at that next level, or we raced against 
> someone else splitting the same block and that's their table instead. Since 
> that's reverting back to a "regular" unmap, I don't see where the need to 
> introduce an additional flush to that path comes from (relative to the 
> existing behaviour, at least).

The old block mapping maybe cached in TLBs, it should be invalidated completely 
before the new next-level mapping to be used. Just ensure that.

In fact, I think the code of arm_lpae_split_blk_unmap may has some mistakes. 
For example:
        if (size == split_sz)
                unmap_idx = ARM_LPAE_LVL_IDX(iova, lvl, data);

It means that "the size" can only be the block/page size of the next-level. 
Suppose current level is 2M block, but we may unmap 12K, and
the above "if" will limit us only be able to unmap 4K.

Furthermore, the situation "if (unmap_idx < 0)" should not appear.

Maybe my analysis is wrong, I will try to test it.


> 
>>       io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
> 
> This is the flush which corresponds to whatever page split_blk_unmap() 
> actually unmapped itself (and also covers any recursively-split 
> intermediate-level entries)...
> 
>> -    return size;
>> +    io_pgtable_tlb_sync(&data->iop);
> 
> ...which does want this sync, but only technically for non-strict mode, since 
> it's otherwise covered by the sync in iommu_unmap().

Because split_blk_unmap() is rarely to be called, it has little impact on the 
overall performance,
so I ommitted the if statement of non-strict, I will add it back.

> 
> I'm not *against* tightening up the TLB maintenance here in general, but if 
> so that should be a separately-reasoned patch, not snuck in with other 
> changes.

OK

> 
> Robin.
> 
>> +
>> +    return unmapped;
>>   }
>>
>>   static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>> @@ -609,7 +615,7 @@ static size_t __arm_lpae_unmap(struct 
>> arm_lpae_io_pgtable *data,
>>               io_pgtable_tlb_sync(iop);
>>               ptep = iopte_deref(pte, data);
>>               __arm_lpae_free_pgtable(data, lvl + 1, ptep);
>> -        } else {
>> +        } else if (!(iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT)) {
>>               io_pgtable_tlb_add_flush(iop, iova, size, size, true);
>>           }
>>
>> @@ -771,7 +777,8 @@ static void arm_lpae_restrict_pgsizes(struct 
>> io_pgtable_cfg *cfg)
>>       u64 reg;
>>       struct arm_lpae_io_pgtable *data;
>>
>> -    if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA))
>> +    if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA |
>> +            IO_PGTABLE_QUIRK_NON_STRICT))
>>           return NULL;
>>
>>       data = arm_lpae_alloc_pgtable(cfg);
>> @@ -863,7 +870,8 @@ static void arm_lpae_restrict_pgsizes(struct 
>> io_pgtable_cfg *cfg)
>>       struct arm_lpae_io_pgtable *data;
>>
>>       /* The NS quirk doesn't apply at stage 2 */
>> -    if (cfg->quirks & ~IO_PGTABLE_QUIRK_NO_DMA)
>> +    if (cfg->quirks & ~(IO_PGTABLE_QUIRK_NO_DMA |
>> +        IO_PGTABLE_QUIRK_NON_STRICT))
>>           return NULL;
>>
>>       data = arm_lpae_alloc_pgtable(cfg);
>> diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
>> index 2df7909..beb14a3 100644
>> --- a/drivers/iommu/io-pgtable.h
>> +++ b/drivers/iommu/io-pgtable.h
>> @@ -71,12 +71,15 @@ struct io_pgtable_cfg {
>>        *    be accessed by a fully cache-coherent IOMMU or CPU (e.g. for a
>>        *    software-emulated IOMMU), such that pagetable updates need not
>>        *    be treated as explicit DMA data.
>> +     * IO_PGTABLE_QUIRK_NON_STRICT: Put off TLBs invalidation and release
>> +     *    memory first.
>>        */
>>       #define IO_PGTABLE_QUIRK_ARM_NS        BIT(0)
>>       #define IO_PGTABLE_QUIRK_NO_PERMS    BIT(1)
>>       #define IO_PGTABLE_QUIRK_TLBI_ON_MAP    BIT(2)
>>       #define IO_PGTABLE_QUIRK_ARM_MTK_4GB    BIT(3)
>>       #define IO_PGTABLE_QUIRK_NO_DMA        BIT(4)
>> +    #define IO_PGTABLE_QUIRK_NON_STRICT    BIT(5)
>>       unsigned long            quirks;
>>       unsigned long            pgsize_bitmap;
>>       unsigned int            ias;
>> -- 
>> 1.8.3
>>
>>
> 
> .
> 

-- 
Thanks!
BestRegards

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

Reply via email to