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

[Auger Eric]

Hi Peter,

On 6/26/20 3:53 PM, Auger Eric wrote:
> 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

Does that make more sense overall? I will respin shortly.

Thanks

Eric
> 
> Thanks
> 
> Eric
>>
>> thanks
>> -- PMM
>>
> 
>