Hi Peter, On 06/20/2018 05:56 PM, Peter Maydell wrote: > On 12 June 2018 at 09:08, Eric Auger <eric.au...@redhat.com> wrote: >> Let's cache config data to avoid fetching and parsing STE/CD >> structures on each translation. We invalidate them on data structure >> invalidation commands. >> >> We put in place a per-smmu mutex to protect the config cache. This >> will be useful too to protect the IOTLB cache. The caches can be >> accessed without BQL, ie. in IO dataplane. The same kind of mutex was >> put in place in the intel viommu. >> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> >> --- >> >> v1 -> v2: >> - restore mutex >> >> v1: >> - only insert the new config if decode_cfg succeeds >> - use smmu_get_sid for trace_* and store hits/misses in the SMMUDevice >> - s/smmuv3_put_config/smmuv3_flush_config >> - document smmuv3_get_config >> - removing the mutex as BQL does the job >> --- >> hw/arm/smmu-common.c | 24 +++++++- >> hw/arm/smmuv3.c | 135 >> +++++++++++++++++++++++++++++++++++++++++-- >> hw/arm/trace-events | 6 ++ >> include/hw/arm/smmu-common.h | 5 ++ >> include/hw/arm/smmuv3.h | 1 + >> 5 files changed, 164 insertions(+), 7 deletions(-) >> >> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c >> index 01c7be8..264e096 100644 >> --- a/hw/arm/smmu-common.c >> +++ b/hw/arm/smmu-common.c >> @@ -310,6 +310,24 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void >> *opaque, int devfn) >> return &sdev->as; >> } >> >> +IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) >> +{ >> + uint8_t bus_n, devfn; >> + SMMUPciBus *smmu_bus; >> + SMMUDevice *smmu; >> + >> + bus_n = PCI_BUS_NUM(sid); >> + smmu_bus = smmu_find_smmu_pcibus(s, bus_n); >> + if (smmu_bus) { >> + devfn = sid & 0x7; >> + smmu = smmu_bus->pbdev[devfn]; >> + if (smmu) { >> + return &smmu->iommu; >> + } >> + } >> + return NULL; >> +} >> + >> static void smmu_base_realize(DeviceState *dev, Error **errp) >> { >> SMMUState *s = ARM_SMMU(dev); >> @@ -321,7 +339,7 @@ static void smmu_base_realize(DeviceState *dev, Error >> **errp) >> error_propagate(errp, local_err); >> return; >> } >> - >> + s->configs = g_hash_table_new_full(NULL, NULL, NULL, g_free); >> s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL); >> >> if (s->primary_bus) { >> @@ -333,7 +351,9 @@ static void smmu_base_realize(DeviceState *dev, Error >> **errp) >> >> static void smmu_base_reset(DeviceState *dev) >> { >> - /* will be filled later on */ >> + SMMUState *s = ARM_SMMU(dev); >> + >> + g_hash_table_remove_all(s->configs); >> } >> >> static Property smmu_dev_properties[] = { >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c >> index 4e7833b..4c41f51 100644 >> --- a/hw/arm/smmuv3.c >> +++ b/hw/arm/smmuv3.c >> @@ -544,6 +544,58 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, >> SMMUTransCfg *cfg, >> return decode_cd(cfg, &cd, event); >> } >> >> +/** >> + * smmuv3_get_config - Look up for a cached copy of configuration data for >> + * @sdev and on cache miss performs a configuration structure decoding from >> + * guest RAM. >> + * >> + * @sdev: SMMUDevice handle >> + * @event: output event info >> + * >> + * The configuration cache contains data resulting from both STE and CD >> + * decoding under the form of an SMMUTransCfg struct. The hash table is >> indexed >> + * by the SMMUDevice handle. >> + */ >> +static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo >> *event) >> +{ >> + SMMUv3State *s = sdev->smmu; >> + SMMUState *bc = &s->smmu_state; >> + SMMUTransCfg *cfg; >> + >> + cfg = g_hash_table_lookup(bc->configs, sdev); >> + if (cfg) { >> + sdev->cfg_cache_hits += 1; > > Increments are more usually written "++". sure > >> + trace_smmuv3_config_cache_hit(smmu_get_sid(sdev), >> + sdev->cfg_cache_hits, sdev->cfg_cache_misses, >> + 100 * sdev->cfg_cache_hits / >> + (sdev->cfg_cache_hits + >> sdev->cfg_cache_misses)); >> + } else { >> + sdev->cfg_cache_misses += 1; > > Ditto. > >> + trace_smmuv3_config_cache_miss(smmu_get_sid(sdev), >> + sdev->cfg_cache_hits, sdev->cfg_cache_misses, >> + 100 * sdev->cfg_cache_hits / >> + (sdev->cfg_cache_hits + >> sdev->cfg_cache_misses)); >> + cfg = g_new0(SMMUTransCfg, 1); >> + >> + if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) { >> + g_hash_table_insert(bc->configs, sdev, cfg); >> + } else { >> + g_free(cfg); >> + cfg = NULL; >> + } >> + } >> + return cfg; >> +} >> + >> +static void smmuv3_flush_config(SMMUDevice *sdev) >> +{ >> + SMMUv3State *s = sdev->smmu; >> + SMMUState *bc = &s->smmu_state; >> + >> + trace_smmuv3_config_cache_inv(smmu_get_sid(sdev)); >> + g_hash_table_remove(bc->configs, sdev); >> +} >> + >> static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, >> IOMMUAccessFlags flag) >> { >> @@ -553,7 +605,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion >> *mr, hwaddr addr, >> SMMUEventInfo event = {.type = SMMU_EVT_NONE, .sid = sid}; >> SMMUPTWEventInfo ptw_info = {}; >> SMMUTranslationStatus status; >> - SMMUTransCfg cfg = {}; >> + SMMUTransCfg *cfg = NULL; >> IOMMUTLBEntry entry = { >> .target_as = &address_space_memory, >> .iova = addr, >> @@ -562,27 +614,30 @@ static IOMMUTLBEntry >> smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, >> .perm = IOMMU_NONE, >> }; >> >> + qemu_mutex_lock(&s->mutex); >> + >> if (!smmu_enabled(s)) { >> status = SMMU_TRANS_DISABLE; >> goto epilogue; >> } >> >> - if (smmuv3_decode_config(mr, &cfg, &event)) { >> + cfg = smmuv3_get_config(sdev, &event); >> + if (!cfg) { >> status = SMMU_TRANS_ERROR; >> goto epilogue; >> } >> >> - if (cfg.aborted) { >> + if (cfg->aborted) { >> status = SMMU_TRANS_ABORT; >> goto epilogue; >> } >> >> - if (cfg.bypassed) { >> + if (cfg->bypassed) { >> status = SMMU_TRANS_BYPASS; >> goto epilogue; >> } >> >> - if (smmu_ptw(&cfg, addr, flag, &entry, &ptw_info)) { >> + if (smmu_ptw(cfg, addr, flag, &entry, &ptw_info)) { >> switch (ptw_info.type) { >> case SMMU_PTW_ERR_WALK_EABT: >> event.type = SMMU_EVT_F_WALK_EABT; >> @@ -628,6 +683,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion >> *mr, hwaddr addr, >> } >> >> epilogue: >> + qemu_mutex_unlock(&s->mutex); >> switch (status) { >> case SMMU_TRANS_SUCCESS: >> entry.perm = flag; >> @@ -664,6 +720,7 @@ epilogue: >> >> static int smmuv3_cmdq_consume(SMMUv3State *s) >> { >> + SMMUState *bs = ARM_SMMU(s); >> SMMUCmdError cmd_error = SMMU_CERROR_NONE; >> SMMUQueue *q = &s->cmdq; >> SMMUCommandType type = 0; >> @@ -698,6 +755,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s) >> >> trace_smmuv3_cmdq_opcode(smmu_cmd_string(type)); >> >> + qemu_mutex_lock(&s->mutex); >> switch (type) { >> case SMMU_CMD_SYNC: >> if (CMD_SYNC_CS(&cmd) & CMD_SYNC_SIG_IRQ) { >> @@ -706,10 +764,74 @@ static int smmuv3_cmdq_consume(SMMUv3State *s) >> break; >> case SMMU_CMD_PREFETCH_CONFIG: >> case SMMU_CMD_PREFETCH_ADDR: >> + break; >> case SMMU_CMD_CFGI_STE: >> + { >> + uint32_t sid = CMD_SID(&cmd); >> + IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid); >> + SMMUDevice *sdev; >> + >> + if (CMD_SSEC(&cmd)) { >> + cmd_error = SMMU_CERROR_ILL; >> + break; >> + } >> + >> + if (!mr) { >> + break; >> + } >> + >> + trace_smmuv3_cmdq_cfgi_ste(sid); >> + sdev = container_of(mr, SMMUDevice, iommu); >> + smmuv3_flush_config(sdev); >> + >> + break; >> + } >> case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL */ > > Comment says these two commands are the same, but their implementation > is different? SMMU_CMD_CFGI_ALL is SMMU_CMD_CFGI_STE_RANGE with hardcoded streamid=0 and range=31 so their handling is identical. > >> + { >> + uint32_t start = CMD_SID(&cmd), end, i; >> + uint8_t range = CMD_STE_RANGE(&cmd); >> + >> + if (CMD_SSEC(&cmd)) { >> + cmd_error = SMMU_CERROR_ILL; >> + break; >> + } >> + >> + end = start + (1 << (range + 1)) - 1; >> + trace_smmuv3_cmdq_cfgi_ste_range(start, end); >> + >> + for (i = start; i <= end; i++) { >> + IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, i); >> + SMMUDevice *sdev; >> + >> + if (!mr) { >> + continue; >> + } >> + sdev = container_of(mr, SMMUDevice, iommu); >> + smmuv3_flush_config(sdev); >> + } >> + break; >> + } >> case SMMU_CMD_CFGI_CD: >> case SMMU_CMD_CFGI_CD_ALL: >> + { >> + uint32_t sid = CMD_SID(&cmd); >> + IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid); >> + SMMUDevice *sdev; >> + >> + if (CMD_SSEC(&cmd)) { >> + cmd_error = SMMU_CERROR_ILL; >> + break; >> + } >> + >> + if (!mr) { >> + break; >> + } >> + >> + trace_smmuv3_cmdq_cfgi_cd(sid); >> + sdev = container_of(mr, SMMUDevice, iommu); >> + smmuv3_flush_config(sdev); >> + break; >> + } >> case SMMU_CMD_TLBI_NH_ALL: >> case SMMU_CMD_TLBI_NH_ASID: >> case SMMU_CMD_TLBI_NH_VA: >> @@ -735,6 +857,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s) >> "Illegal command type: %d\n", CMD_TYPE(&cmd)); >> break; >> } >> + qemu_mutex_unlock(&s->mutex); >> if (cmd_error) { >> break; >> } >> @@ -1114,6 +1237,8 @@ static void smmu_realize(DeviceState *d, Error **errp) >> return; >> } >> >> + qemu_mutex_init(&s->mutex); >> + >> memory_region_init_io(&sys->iomem, OBJECT(s), >> &smmu_mem_ops, sys, TYPE_ARM_SMMUV3, 0x20000); >> >> diff --git a/hw/arm/trace-events b/hw/arm/trace-events >> index 0ab66bb..5bddfeb 100644 >> --- a/hw/arm/trace-events >> +++ b/hw/arm/trace-events >> @@ -40,3 +40,9 @@ smmuv3_translate_success(const char *n, uint16_t sid, >> uint64_t iova, uint64_t tr >> smmuv3_get_cd(uint64_t addr) "CD addr: 0x%"PRIx64 >> smmuv3_decode_cd(uint32_t oas) "oas=%d" >> smmuv3_decode_cd_tt(int i, uint32_t tsz, uint64_t ttb, uint32_t granule_sz) >> "TT[%d]:tsz:%d ttb:0x%"PRIx64" granule_sz:%d" >> +smmuv3_cmdq_cfgi_ste(int streamid) " |_ streamid =%d" >> +smmuv3_cmdq_cfgi_ste_range(int start, int end) " |_ start=0x%d - >> end=0x%d" >> +smmuv3_cmdq_cfgi_cd(uint32_t sid) " |_ streamid = %d" > > What are the underscores in the trace strings for ? I used to output those trace points together with smmuv3_cmdq_opcode. But I can remove this formatting now. > >> +smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, float >> perc) "Config cache HIT for sid %d (hits=%d, misses=%d, hit rate=%.1f)" >> +smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, >> float perc) "Config cache MISS for sid %d (hits=%d, misses=%d, hit >> rate=%.1f)" > > Does our tracing infrastructure really support floats? There > is no other use of it in the tree. There is one instance of > 'double', though, so maybe we do. > > In general I think we should prefer 'double' over 'float' anyway, > though. OK switching to double > > Otherwise > Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> Thanks!
Eric > > thanks > -- PMM >