Hi Peter,
On 7/6/20 3:26 PM, Peter Maydell wrote:
> On Thu, 2 Jul 2020 at 16:27, Eric Auger <eric.au...@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.au...@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
>
