Re: [PATCH v2 4/9] hw/arm/smmu: Simplify the IOTLB key format
Hi Peter,
On 7/6/20 3:26 PM, Peter Maydell wrote:
> On Thu, 2 Jul 2020 at 16:27, Eric Auger 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
>
>> 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
>
Re: [PATCH v2 4/9] hw/arm/smmu: Simplify the IOTLB key format
On Thu, 2 Jul 2020 at 16:27, Eric Auger 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
> 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
[PATCH v2 4/9] hw/arm/smmu: Simplify the IOTLB key format
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
---
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) & 0x)
+#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 {
/* */
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
