Re: [PATCH RESEND 6/9] hw/arm/smmu-common: Manage IOTLB block entries

[Auger Eric]

Hi Peter,

On 6/25/20 5:30 PM, Peter Maydell wrote:
> On Thu, 11 Jun 2020 at 17:16, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> At the moment each entry in the IOTLB corresponds to a page sized
>> mapping (4K, 16K or 64K), even if the page belongs to a mapped
>> block. In case of block mapping this unefficiently consume IOTLB
>> entries.
>>
>> Change the value of the entry so that it reflects the actual
>> mapping it belongs to (block or page start address and size).
>>
>> Also the level/tg of the entry is encoded in the key. In subsequent
>> patches we will enable range invalidation. This latter is able
>> to provide the level/tg of the entry.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> 
>> -uint64_t smmu_get_iotlb_key(uint16_t asid, uint64_t iova)
>> +uint64_t smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
>> +                            uint8_t tg, uint8_t level)
>>  {
>> -    return iova >> 12 | (uint64_t)(asid) << SMMU_IOTLB_ASID_SHIFT;
>> +    return iova >> 12 | (uint64_t)(asid) << SMMU_IOTLB_ASID_SHIFT |
>> +           (uint64_t)(level) << SMMU_IOTLB_LEVEL_SHIFT |
>> +           (uint64_t)(tg) << SMMU_IOTLB_TG_SHIFT;
>>  }
> 
>>  SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
>> -                                 hwaddr iova)
>> +                                SMMUTransTableInfo *tt, hwaddr iova)
>>  {
>> -    uint64_t key = smmu_get_iotlb_key(cfg->asid, iova);
>> -    SMMUTLBEntry *entry = g_hash_table_lookup(bs->iotlb, &key);
>> +    uint8_t tg = (tt->granule_sz - 10) / 2;
>> +    int level = tt->starting_level;
>> +    SMMUTLBEntry *entry = NULL;
>> +
>> +    while (level <= 3) {
>> +        uint64_t subpage_size = 1ULL << level_shift(level, tt->granule_sz);
>> +        uint64_t mask = subpage_size - 1;
>> +        uint64_t key;
>> +
>> +        key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level);
>> +        entry = g_hash_table_lookup(bs->iotlb, &key);
>> +        if (entry) {
>> +            break;
>> +        }
>> +        level++;
> 
> Rather than looping around doing multiple hash table lookups like
> this, why not just avoid including the tg and level in the
> key equality test?
> 
> If I understand the range-based-invalidation feature correctly,
> the only time we care about the TG/LVL is if we're processing
> an invalidate-range command that specifies them. But in that
> case there should never be multiple entries in the bs->iotlb
> with the same iova, so we can just check whether the entry
> matches the requested TG/LVL once we've pulled it out of the
> hash table. (Or we could architecturally validly just blow
> it away regardless of requested TG/LVL -- they are only hints,
> not required-to-match.)

This change could have been done independently on the RIL feature. As we
now put block entries in the IOTLB , when we look for an iova
translation, the IOVA can be mapped using different block sizes or using
page entries. So we start looking at blocks of the bigger size (entry
level) downto the page, for instance 4TB/512MB/64KB. We cannot know
which block and size the address belongs to. I do not know if we can
make any hypothesis on whether the driver is forbidden to invalidate an
address that is not the starting address of an initial mapping.

Not a justification but an info, this is implemented the same way on x86
(except they don't have variable TG), see vtd_lookup_iotlb in
hw/i386/intel_iommu.c

Thanks

Eric
> 
> thanks
> -- PMM
>