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

[Auger Eric]

Hi Peter,

On 6/30/20 6:29 PM, Auger Eric wrote:
> Hi Peter,
> 
> On 6/30/20 5:50 PM, Peter Maydell wrote:
>> On Fri, 26 Jun 2020 at 14:53, Auger Eric <eric.auger@redhat.com> wrote:
>>> On 6/25/20 5:30 PM, Peter Maydell wrote:
>>>> 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.
>>
>> Yes, but we wouldn't need to care which TG and LVL the
>> address belongs to if we didn't put them into
>> the key, would we? I'm probably missing something here, but
>> just because the hardware might want to use the hints in
>> the invalidation-command about TG and LVL doesn't inherently
>> mean QEMU is most efficient if it cares about the hints.
> 
> OK I think I understand your point now. It is not necessary to put
> TG/LVL in the key as log as they are in the entry. I will look at this
> implementation ...

Looking further at the implementation, if we don't encode the LVL in the
key (but just encode the block addr), on invalidation, we are not able
to remove the associated entry in one shot, using g_hash_table_remove().
We are obliged to use g_hash_table_foreach_remove and sort out the
entries according to the invalidation parameters.

Putting the TG and LVL in the key allows to do this in one short if
num_pages == 1. See [8/9] hw/arm/smmuv3: Get prepared for range
invalidation, smmu_iotlb_inv_iova() implementation.

So my understanding is it is obviously feasible to get rid of TG/LVL in
the key but may be less optimal when range invalidation gets used by the
guest (most invalidations having num_pages == 1)

Thanks

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