### 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