Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/smmuv3: IOTLB emulation

[Auger Eric]

Hi Peter,

On 06/20/2018 06:07 PM, Peter Maydell wrote:
> On 12 June 2018 at 09:08, Eric Auger <address@hidden> wrote:
>> We emulate a TLB cache of size SMMU_IOTLB_MAX_SIZE=256.
>> It is implemented as a hash table whose key is a combination
>> of the 16b asid and 48b IOVA (Jenkins hash).
>>
>> Entries are invalidated on TLB invalidation commands, either
>> globally, or per asid, or per asid/iova.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v1 -> v2:
>> - add comment about Jenkins Hash
>> - remove init of iotlb_hits, misses
>>
>> v1:
>> - Add new trace point when smmu is bypassed
>> - s/iotlb_miss/iotlb_misses, s/iotlb_hit/iotlb_hits
>> - use SMMUIOTLBKey as a key
>>
>> Credit to Tomasz Nowicki who did the first implementation of
>> this IOTLB implementation, inspired of intel_iommu implementation.
>> ---
>>  hw/arm/smmu-common.c         | 60 +++++++++++++++++++++++++++
>>  hw/arm/smmuv3.c              | 98 
>> ++++++++++++++++++++++++++++++++++++++++++--
>>  hw/arm/trace-events          |  9 ++++
>>  include/hw/arm/smmu-common.h | 13 ++++++
>>  4 files changed, 176 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index 264e096..16cd33a 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -24,11 +24,43 @@
>>  #include "qom/cpu.h"
>>  #include "hw/qdev-properties.h"
>>  #include "qapi/error.h"
>> +#include "qemu/jhash.h"
>>
>>  #include "qemu/error-report.h"
>>  #include "hw/arm/smmu-common.h"
>>  #include "smmu-internal.h"
>>
>> +/* IOTLB Management */
>> +
>> +inline void smmu_iotlb_inv_all(SMMUState *s)
>> +{
>> +    trace_smmu_iotlb_inv_all();
>> +    g_hash_table_remove_all(s->iotlb);
>> +}
>> +
>> +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;
>> +
>> +    return iotlb_key->asid == asid;
>> +}
>> +
>> +inline void smmu_iotlb_inv_iova(SMMUState *s, uint16_t asid, dma_addr_t 
>> iova)
>> +{
>> +    SMMUIOTLBKey key = {.asid = asid, .iova = iova};
>> +
>> +    trace_smmu_iotlb_inv_iova(asid, iova);
>> +    g_hash_table_remove(s->iotlb, &key);
>> +}
>> +
>> +inline void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
>> +{
>> +    trace_smmu_iotlb_inv_asid(asid);
>> +    g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
>> +}
>> +
>>  /* VMSAv8-64 Translation */
>>
>>  /**
>> @@ -328,6 +360,31 @@ 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;
>> +
>> +    /* Henkins hash */
> 
> "Jenkins"
> 
>> +    a = b = c = JHASH_INITVAL + sizeof(*key);
> 
> Why do we add the sizeof(SMMUIOTLBKey) here ?
To be honest I did something similar to net/colo.c, connection_key_hash().

in
https://stackoverflow.com/questions/3279615/python-implementation-of-jenkins-hash

I can also find

def hashlittle2(data, initval = 0, initval2 = 0):
    length = lenpos = len(data)

    a = b = c = (0xdeadbeef + (length) + initval)
../..


> 
>> +    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)
>> +{
>> +    SMMUIOTLBKey *k1 = (SMMUIOTLBKey *)v1;
>> +    SMMUIOTLBKey *k2 = (SMMUIOTLBKey *)v2;
> 
> These should have a 'const', at which point I think you'll
> find that you don't need the explicit casts.
indeed

Thanks

Eric
> 
> Otherwise looks OK I think.
> 
> thanks
> -- PMM
>