diff mbox series

[RESEND,3/9] hw/arm/smmu: Simplify the IOTLB key format

Message ID 20200611161500.23580-4-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show
Series SMMUv3.2 Range-based TLB Invalidation Support | expand

Commit Message

Eric Auger June 11, 2020, 4:14 p.m. UTC 
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>
---
 hw/arm/smmu-internal.h       |  4 +++
 include/hw/arm/smmu-common.h |  6 +---
 hw/arm/smmu-common.c         | 53 ++++++++++++++----------------------
 3 files changed, 26 insertions(+), 37 deletions(-)

Comments

Peter Maydell June 25, 2020, 3:03 p.m. UTC | #1 
On Thu, 11 Jun 2020 at 17:15, 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/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 1dceec5cb1..7b9d2f0eb7 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -88,11 +88,6 @@ typedef struct SMMUPciBus {
>      SMMUDevice   *pbdev[]; /* Parent array is sparse, so dynamically alloc */
>  } SMMUPciBus;
>
> -typedef struct SMMUIOTLBKey {
> -    uint64_t iova;
> -    uint16_t asid;
> -} SMMUIOTLBKey;

I think we should keep the SMMUIOTLBKey type to abstract out what
the key type is under the hood, so it would now be
 typedef uint64_t SMMUIOTLBKey;

(and then the code that works with SMMUIOTLBKeys should never
directly look at it as a uint64_t. If you wanted you could
put the abstraction layer into place with the existing
SMMUIOTLBKey type and then change the type in a second patch.)

> +uint64_t smmu_get_iotlb_key(uint16_t asid, uint64_t iova);

This should return SMMUIOTLBKey rather than uint64_t,
or pass in the pointer, like:
   smmu_get_iotlb_key(SMMUIOTLBKey *key, uint16_t asid, uint64_t iova);

thanks
-- PMM
Eric Auger June 26, 2020, 1:53 p.m. UTC | #2 
Hi Peter,

On 6/25/20 5:03 PM, Peter Maydell wrote:
> On Thu, 11 Jun 2020 at 17:15, 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/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>> index 1dceec5cb1..7b9d2f0eb7 100644
>> --- a/include/hw/arm/smmu-common.h
>> +++ b/include/hw/arm/smmu-common.h
>> @@ -88,11 +88,6 @@ typedef struct SMMUPciBus {
>>      SMMUDevice   *pbdev[]; /* Parent array is sparse, so dynamically alloc */
>>  } SMMUPciBus;
>>
>> -typedef struct SMMUIOTLBKey {
>> -    uint64_t iova;
>> -    uint16_t asid;
>> -} SMMUIOTLBKey;
> 
> I think we should keep the SMMUIOTLBKey type to abstract out what
> the key type is under the hood, so it would now be
>  typedef uint64_t SMMUIOTLBKey;

OK
> 
> (and then the code that works with SMMUIOTLBKeys should never
> directly look at it as a uint64_t. If you wanted you could
> put the abstraction layer into place with the existing
> SMMUIOTLBKey type and then change the type in a second patch.)

done this way

> 
>> +uint64_t smmu_get_iotlb_key(uint16_t asid, uint64_t iova);
> 
> This should return SMMUIOTLBKey rather than uint64_t,
> or pass in the pointer, like:
>    smmu_get_iotlb_key(SMMUIOTLBKey *key, uint16_t asid, uint64_t iova);
sure

Thanks

Eric
> 
> thanks
> -- PMM
>
diff mbox series

Patch

diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index 7794d6d394..2ecb6f1dc6 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -96,4 +96,8 @@  uint64_t iova_level_offset(uint64_t iova, int inputsize,
             MAKE_64BIT_MASK(0, gsz - 3);
 }
 
+#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 1dceec5cb1..7b9d2f0eb7 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -88,11 +88,6 @@  typedef struct SMMUPciBus {
     SMMUDevice   *pbdev[]; /* Parent array is sparse, so dynamically alloc */
 } SMMUPciBus;
 
-typedef struct SMMUIOTLBKey {
-    uint64_t iova;
-    uint16_t asid;
-} SMMUIOTLBKey;
-
 typedef struct SMMUState {
     /* <private> */
     SysBusDevice  dev;
@@ -155,6 +150,7 @@  IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
 
 IOMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, hwaddr iova);
 void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, IOMMUTLBEntry *entry);
+uint64_t smmu_get_iotlb_key(uint16_t asid, uint64_t iova);
 void smmu_iotlb_inv_all(SMMUState *s);
 void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
 void smmu_iotlb_inv_iova(SMMUState *s, uint16_t asid, dma_addr_t iova);
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 8409de052d..3101043540 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -32,10 +32,25 @@ 
 
 /* IOTLB Management */
 
+static guint smmu_iotlb_key_hash(gconstpointer v)
+{
+    return (guint)*(const uint64_t *)v;
+}
+
+static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
+{
+    return *((const uint64_t *)v1) == *((const uint64_t *)v2);
+}
+
+uint64_t smmu_get_iotlb_key(uint16_t asid, uint64_t iova)
+{
+    return iova >> 12 | (uint64_t)(asid) << SMMU_IOTLB_ASID_SHIFT;
+}
+
 IOMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
                                  hwaddr iova)
 {
-    SMMUIOTLBKey key = {.asid = cfg->asid, .iova = iova};
+    uint64_t key = smmu_get_iotlb_key(cfg->asid, iova);
     IOMMUTLBEntry *entry = g_hash_table_lookup(bs->iotlb, &key);
 
     if (entry) {
@@ -56,14 +71,13 @@  IOMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
 
 void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, IOMMUTLBEntry *entry)
 {
-    SMMUIOTLBKey *key = g_new0(SMMUIOTLBKey, 1);
+    uint64_t *key = g_new0(uint64_t, 1);
 
     if (g_hash_table_size(bs->iotlb) >= SMMU_IOTLB_MAX_SIZE) {
         smmu_iotlb_inv_all(bs);
     }
 
-    key->asid = cfg->asid;
-    key->iova = entry->iova;
+    *key = smmu_get_iotlb_key(cfg->asid, entry->iova);
     trace_smmu_iotlb_insert(cfg->asid, entry->iova);
     g_hash_table_insert(bs->iotlb, key, entry);
 }
@@ -78,14 +92,14 @@  static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
                                          gpointer user_data)
 {
     uint16_t asid = *(uint16_t *)user_data;
-    SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
+    uint64_t *iotlb_key = (uint64_t *)key;
 
-    return iotlb_key->asid == asid;
+    return SMMU_IOTLB_ASID(*iotlb_key) == asid;
 }
 
 inline void smmu_iotlb_inv_iova(SMMUState *s, uint16_t asid, dma_addr_t iova)
 {
-    SMMUIOTLBKey key = {.asid = asid, .iova = iova};
+    uint64_t key = smmu_get_iotlb_key(asid, iova);
 
     trace_smmu_iotlb_inv_iova(asid, iova);
     g_hash_table_remove(s->iotlb, &key);
@@ -382,31 +396,6 @@  IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
     return NULL;
 }
 
-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;
-}
-
-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);
-}
-
 /* Unmap the whole notifier's range */
 static void smmu_unmap_notifier_range(IOMMUNotifier *n)
 {