Re: [PATCH v2 4/9] hw/arm/smmu: Simplify the IOTLB key format

[Auger Eric]

Hi Peter,

On 7/6/20 3:26 PM, Peter Maydell wrote:
> On Thu, 2 Jul 2020 at 16:27, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> Instead of using a Jenkins hash function to generate
>> the key let's just use a 64 bit unsigned integer that
>> contains the asid and the 40 upper bits of the iova.
>> A maximum of 52-bit IOVA is supported. This change in the
>> key format also prepares for the addition of new fields
>> in subsequent patches (granule and level).
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index 7dc8541e8b..5e85e30bdf 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -34,34 +34,17 @@
>>
>>  static guint smmu_iotlb_key_hash(gconstpointer v)
>>  {
>> -    SMMUIOTLBKey *key = (SMMUIOTLBKey *)v;
>> -    uint32_t a, b, c;
>> -
>> -    /* Jenkins hash */
>> -    a = b = c = JHASH_INITVAL + sizeof(*key);
>> -    a += key->asid;
>> -    b += extract64(key->iova, 0, 32);
>> -    c += extract64(key->iova, 32, 32);
>> -
>> -    __jhash_mix(a, b, c);
>> -    __jhash_final(a, b, c);
>> -
>> -    return c;
>> +    return (guint)*(const uint64_t *)v;
>>  }
> 
> So the hash value is now going to be the lower 32
> bits of the key, which is to say bits [40,12] of the IOVA,
> and won't include the ASID at all. Isn't that going to
> result in more hash collisions than would be ideal?
> 
> I was going to suggest using the glib builtin g_int64_hash()
> instead, but looking at the source that seems to be the
> identical implementation to this one. I guess that's
> intended for cases where an integer key is really a
> random integer, not one where it's got internal structure
> of different bit fields within it being for different
> purposes.

That's something I did not notice. Would you recommend to keep the
Jenkins hash function then? Note the intel iommu also use the hash
function which only covers the gfn.

Thanks

Eric

> 
> thanks
> -- PMM
>