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>

---

v1 -> v2:
- split the inital patch into 2 patches and keep SMMUIOTLBKey type
---
 hw/arm/smmu-internal.h       |  5 ++++-
 include/hw/arm/smmu-common.h |  5 +----
 hw/arm/smmu-common.c         | 23 +++--------------------
 3 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index 3104f768cd..2ecb6f1dc6 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -96,5 +96,8 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
             MAKE_64BIT_MASK(0, gsz - 3);
 }
 
-#define SMMU_IOTLB_ASID(key) ((key).asid)
+#define SMMU_IOTLB_ASID_SHIFT  40
+
+#define SMMU_IOTLB_ASID(key) (((key) >> SMMU_IOTLB_ASID_SHIFT) & 0xFFFF)
+#define SMMU_IOTLB_IOVA(key) (((key) & MAKE_64BIT_MASK(0, 40)) << 12)
 #endif
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 5f9f3535d2..455d7855b2 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -88,10 +88,7 @@ typedef struct SMMUPciBus {
     SMMUDevice   *pbdev[]; /* Parent array is sparse, so dynamically alloc */
 } SMMUPciBus;
 
-typedef struct SMMUIOTLBKey {
-    uint64_t iova;
-    uint16_t asid;
-} SMMUIOTLBKey;
+ typedef uint64_t SMMUIOTLBKey;
 
 typedef struct SMMUState {
     /* <private> */
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;
 }
 
 static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
 {
-    const SMMUIOTLBKey *k1 = v1;
-    const SMMUIOTLBKey *k2 = v2;
-
-    return (k1->asid == k2->asid) && (k1->iova == k2->iova);
+    return *((const uint64_t *)v1) == *((const uint64_t *)v2);
 }
 
 SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova)
 {
-    SMMUIOTLBKey key = {.asid = asid, .iova = iova};
-
-    return key;
+    return iova >> 12 | (uint64_t)(asid) << SMMU_IOTLB_ASID_SHIFT;
 }
 
 IOMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
-- 
2.21.3

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.

thanks
-- PMM

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
>