* [Qemu-devel] [PATCH v2 1/4] hw/arm/smmuv3: Fix translate error handling
2018-06-12 8:08 [Qemu-devel] [PATCH v2 0/4] ARM SMMUv3: IOTLB Emulation and VHOST Support Eric Auger
@ 2018-06-12 8:08 ` Eric Auger
2018-06-20 15:36 ` Peter Maydell
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data Eric Auger
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Eric Auger @ 2018-06-12 8:08 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
jia.he, hejianet
From: Jia He <hejianet@gmail.com>
In case the STE's config is "Bypass" we currently don't set the
IOMMUTLBEntry perm flags and the access does not succeed. Also
if the config is 0b0xx (Aborted/Reserved), decode_ste and
smmuv3_decode_config currently returns -EINVAL and we don't enter
the expected code path: we record an event whereas we should not.
This patch fixes those bugs and simplifies the error handling.
decode_ste and smmuv3_decode_config now return 0 if aborted or
bypassed config was found. Only bad config info produces negative
error values. In smmuv3_translate we more clearly differentiate
errors, bypass/smmu disabled, aborted and success cases. Also
trace points are differentiated.
Fixes: 9bde7f0674fe ("hw/arm/smmuv3: Implement translate callback")
Reported-by: jia.he@hxt-semitech.com
Signed-off-by: jia.he@hxt-semitech.com
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v1 -> v2:
- set addr_mask in case of bypass and disabled. Mandated for
vhost use case since a411c84b561baa94b28165c52f21c33517ee8f59
"exec: extract address_space_translate_iommu, fix page_mask
corner case"
---
hw/arm/smmuv3-internal.h | 12 +++++-
hw/arm/smmuv3.c | 96 +++++++++++++++++++++++++++++++++---------------
hw/arm/trace-events | 7 ++--
3 files changed, 80 insertions(+), 35 deletions(-)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index a9d714b..bab25d6 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -23,6 +23,14 @@
#include "hw/arm/smmu-common.h"
+typedef enum SMMUTranslationStatus {
+ SMMU_TRANS_DISABLE,
+ SMMU_TRANS_ABORT,
+ SMMU_TRANS_BYPASS,
+ SMMU_TRANS_ERROR,
+ SMMU_TRANS_SUCCESS,
+} SMMUTranslationStatus;
+
/* MMIO Registers */
REG32(IDR0, 0x0)
@@ -315,7 +323,7 @@ enum { /* Command completion notification */
/* Events */
typedef enum SMMUEventType {
- SMMU_EVT_OK = 0x00,
+ SMMU_EVT_NONE = 0x00,
SMMU_EVT_F_UUT ,
SMMU_EVT_C_BAD_STREAMID ,
SMMU_EVT_F_STE_FETCH ,
@@ -337,7 +345,7 @@ typedef enum SMMUEventType {
} SMMUEventType;
static const char *event_stringify[] = {
- [SMMU_EVT_OK] = "SMMU_EVT_OK",
+ [SMMU_EVT_NONE] = "no recorded event",
[SMMU_EVT_F_UUT] = "SMMU_EVT_F_UUT",
[SMMU_EVT_C_BAD_STREAMID] = "SMMU_EVT_C_BAD_STREAMID",
[SMMU_EVT_F_STE_FETCH] = "SMMU_EVT_F_STE_FETCH",
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index b3026de..4e7833b 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -23,6 +23,7 @@
#include "hw/qdev-core.h"
#include "hw/pci/pci.h"
#include "exec/address-spaces.h"
+#include "cpu.h"
#include "trace.h"
#include "qemu/log.h"
#include "qemu/error-report.h"
@@ -154,7 +155,7 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
EVT_SET_SID(&evt, info->sid);
switch (info->type) {
- case SMMU_EVT_OK:
+ case SMMU_EVT_NONE:
return;
case SMMU_EVT_F_UUT:
EVT_SET_SSID(&evt, info->u.f_uut.ssid);
@@ -312,12 +313,11 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
return 0;
}
-/* Returns <0 if the caller has no need to continue the translation */
+/* Returns < 0 in case of invalid STE, 0 otherwise */
static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
STE *ste, SMMUEventInfo *event)
{
uint32_t config;
- int ret = -EINVAL;
if (!STE_VALID(ste)) {
goto bad_ste;
@@ -326,13 +326,13 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
config = STE_CONFIG(ste);
if (STE_CFG_ABORT(config)) {
- cfg->aborted = true; /* abort but don't record any event */
- return ret;
+ cfg->aborted = true;
+ return 0;
}
if (STE_CFG_BYPASS(config)) {
cfg->bypassed = true;
- return ret;
+ return 0;
}
if (STE_CFG_S2_ENABLED(config)) {
@@ -509,7 +509,7 @@ bad_cd:
* the different configuration decoding steps
* @event: must be zero'ed by the caller
*
- * return < 0 if the translation needs to be aborted (@event is filled
+ * return < 0 in case of config decoding error (@event is filled
* accordingly). Return 0 otherwise.
*/
static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
@@ -518,19 +518,26 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
uint32_t sid = smmu_get_sid(sdev);
SMMUv3State *s = sdev->smmu;
- int ret = -EINVAL;
+ int ret;
STE ste;
CD cd;
- if (smmu_find_ste(s, sid, &ste, event)) {
+ ret = smmu_find_ste(s, sid, &ste, event);
+ if (ret) {
return ret;
}
- if (decode_ste(s, cfg, &ste, event)) {
+ ret = decode_ste(s, cfg, &ste, event);
+ if (ret) {
return ret;
}
- if (smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event)) {
+ if (cfg->aborted || cfg->bypassed) {
+ return 0;
+ }
+
+ ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event);
+ if (ret) {
return ret;
}
@@ -543,8 +550,9 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
SMMUv3State *s = sdev->smmu;
uint32_t sid = smmu_get_sid(sdev);
- SMMUEventInfo event = {.type = SMMU_EVT_OK, .sid = sid};
+ SMMUEventInfo event = {.type = SMMU_EVT_NONE, .sid = sid};
SMMUPTWEventInfo ptw_info = {};
+ SMMUTranslationStatus status;
SMMUTransCfg cfg = {};
IOMMUTLBEntry entry = {
.target_as = &address_space_memory,
@@ -553,23 +561,28 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
.addr_mask = ~(hwaddr)0,
.perm = IOMMU_NONE,
};
- int ret = 0;
if (!smmu_enabled(s)) {
- goto out;
+ status = SMMU_TRANS_DISABLE;
+ goto epilogue;
}
- ret = smmuv3_decode_config(mr, &cfg, &event);
- if (ret) {
- goto out;
+ if (smmuv3_decode_config(mr, &cfg, &event)) {
+ status = SMMU_TRANS_ERROR;
+ goto epilogue;
}
if (cfg.aborted) {
- goto out;
+ status = SMMU_TRANS_ABORT;
+ goto epilogue;
}
- ret = smmu_ptw(&cfg, addr, flag, &entry, &ptw_info);
- if (ret) {
+ if (cfg.bypassed) {
+ status = SMMU_TRANS_BYPASS;
+ goto epilogue;
+ }
+
+ 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;
@@ -609,18 +622,41 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
default:
g_assert_not_reached();
}
+ status = SMMU_TRANS_ERROR;
+ } else {
+ status = SMMU_TRANS_SUCCESS;
}
-out:
- if (ret) {
- qemu_log_mask(LOG_GUEST_ERROR,
- "%s translation failed for iova=0x%"PRIx64"(%d)\n",
- mr->parent_obj.name, addr, ret);
- entry.perm = IOMMU_NONE;
- smmuv3_record_event(s, &event);
- } else if (!cfg.aborted) {
+
+epilogue:
+ switch (status) {
+ case SMMU_TRANS_SUCCESS:
entry.perm = flag;
- trace_smmuv3_translate(mr->parent_obj.name, sid, addr,
- entry.translated_addr, entry.perm);
+ trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
+ entry.translated_addr, entry.perm);
+ break;
+ case SMMU_TRANS_DISABLE:
+ entry.perm = flag;
+ entry.addr_mask = ~TARGET_PAGE_MASK;
+ trace_smmuv3_translate_disable(mr->parent_obj.name, sid, addr,
+ entry.perm);
+ break;
+ case SMMU_TRANS_BYPASS:
+ entry.perm = flag;
+ entry.addr_mask = ~TARGET_PAGE_MASK;
+ trace_smmuv3_translate_bypass(mr->parent_obj.name, sid, addr,
+ entry.perm);
+ break;
+ case SMMU_TRANS_ABORT:
+ /* no event is recorded on abort */
+ trace_smmuv3_translate_abort(mr->parent_obj.name, sid, addr,
+ entry.perm);
+ break;
+ case SMMU_TRANS_ERROR:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s translation failed for iova=0x%"PRIx64"(%s)\n",
+ mr->parent_obj.name, addr, smmu_event_string(event.type));
+ smmuv3_record_event(s, &event);
+ break;
}
return entry;
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 2d92727..0ab66bb 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -33,9 +33,10 @@ smmuv3_record_event(const char *type, uint32_t sid) "%s sid=%d"
smmuv3_find_ste(uint16_t sid, uint32_t features, uint16_t sid_split) "SID:0x%x features:0x%x, sid_split:0x%x"
smmuv3_find_ste_2lvl(uint64_t strtab_base, uint64_t l1ptr, int l1_ste_offset, uint64_t l2ptr, int l2_ste_offset, int max_l2_ste) "strtab_base:0x%"PRIx64" l1ptr:0x%"PRIx64" l1_off:0x%x, l2ptr:0x%"PRIx64" l2_off:0x%x max_l2_ste:%d"
smmuv3_get_ste(uint64_t addr) "STE addr: 0x%"PRIx64
-smmuv3_translate_bypass(const char *n, uint16_t sid, uint64_t addr, bool is_write) "%s sid=%d bypass iova:0x%"PRIx64" is_write=%d"
-smmuv3_translate_in(uint16_t sid, int pci_bus_num, uint64_t strtab_base) "SID:0x%x bus:%d strtab_base:0x%"PRIx64
+smmuv3_translate_disable(const char *n, uint16_t sid, uint64_t addr, bool is_write) "%s sid=%d bypass (smmu disabled) iova:0x%"PRIx64" is_write=%d"
+smmuv3_translate_bypass(const char *n, uint16_t sid, uint64_t addr, bool is_write) "%s sid=%d STE bypass iova:0x%"PRIx64" is_write=%d"
+smmuv3_translate_abort(const char *n, uint16_t sid, uint64_t addr, bool is_write) "%s sid=%d abort on iova:0x%"PRIx64" is_write=%d"
+smmuv3_translate_success(const char *n, uint16_t sid, uint64_t iova, uint64_t translated, int perm) "%s sid=%d iova=0x%"PRIx64" translated=0x%"PRIx64" perm=0x%x"
smmuv3_get_cd(uint64_t addr) "CD addr: 0x%"PRIx64
-smmuv3_translate(const char *n, uint16_t sid, uint64_t iova, uint64_t translated, int perm) "%s sid=%d iova=0x%"PRIx64" translated=0x%"PRIx64" perm=0x%x"
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"
--
2.5.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] hw/arm/smmuv3: Fix translate error handling
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 1/4] hw/arm/smmuv3: Fix translate error handling Eric Auger
@ 2018-06-20 15:36 ` Peter Maydell
0 siblings, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2018-06-20 15:36 UTC (permalink / raw)
To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm, jia.he, Jia He
On 12 June 2018 at 09:08, Eric Auger <eric.auger@redhat.com> wrote:
> From: Jia He <hejianet@gmail.com>
>
> In case the STE's config is "Bypass" we currently don't set the
> IOMMUTLBEntry perm flags and the access does not succeed. Also
> if the config is 0b0xx (Aborted/Reserved), decode_ste and
> smmuv3_decode_config currently returns -EINVAL and we don't enter
> the expected code path: we record an event whereas we should not.
>
> This patch fixes those bugs and simplifies the error handling.
> decode_ste and smmuv3_decode_config now return 0 if aborted or
> bypassed config was found. Only bad config info produces negative
> error values. In smmuv3_translate we more clearly differentiate
> errors, bypass/smmu disabled, aborted and success cases. Also
> trace points are differentiated.
>
> Fixes: 9bde7f0674fe ("hw/arm/smmuv3: Implement translate callback")
> Reported-by: jia.he@hxt-semitech.com
> Signed-off-by: jia.he@hxt-semitech.com
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data
2018-06-12 8:08 [Qemu-devel] [PATCH v2 0/4] ARM SMMUv3: IOTLB Emulation and VHOST Support Eric Auger
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 1/4] hw/arm/smmuv3: Fix translate error handling Eric Auger
@ 2018-06-12 8:08 ` Eric Auger
2018-06-20 15:56 ` Peter Maydell
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 3/4] hw/arm/smmuv3: IOTLB emulation Eric Auger
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 4/4] hw/arm/smmuv3: Add notifications on invalidation Eric Auger
3 siblings, 1 reply; 13+ messages in thread
From: Eric Auger @ 2018-06-12 8:08 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
jia.he, hejianet
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.auger@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;
+ 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;
+ 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 */
+ {
+ 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"
+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)"
+smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid %d"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index c41eb5c3..7ce95ca 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -75,6 +75,8 @@ typedef struct SMMUDevice {
int devfn;
IOMMUMemoryRegion iommu;
AddressSpace as;
+ uint32_t cfg_cache_hits;
+ uint32_t cfg_cache_misses;
} SMMUDevice;
typedef struct SMMUNotifierNode {
@@ -142,4 +144,7 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
*/
SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova);
+/* Return the iommu mr associated to @sid, or NULL if none */
+IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
+
#endif /* HW_ARM_SMMU_COMMON */
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index 23f7036..36b2f45 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -59,6 +59,7 @@ typedef struct SMMUv3State {
SMMUQueue eventq, cmdq;
qemu_irq irq[4];
+ QemuMutex mutex;
} SMMUv3State;
typedef enum {
--
2.5.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data Eric Auger
@ 2018-06-20 15:56 ` Peter Maydell
2018-06-20 16:10 ` Peter Maydell
2018-06-21 7:51 ` Auger Eric
0 siblings, 2 replies; 13+ messages in thread
From: Peter Maydell @ 2018-06-20 15:56 UTC (permalink / raw)
To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm, jia.he, Jia He
On 12 June 2018 at 09:08, Eric Auger <eric.auger@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.auger@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 "++".
> + 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?
> + {
> + 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 ?
> +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.
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data
2018-06-20 15:56 ` Peter Maydell
@ 2018-06-20 16:10 ` Peter Maydell
2018-06-21 7:54 ` Auger Eric
2018-06-21 7:51 ` Auger Eric
1 sibling, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2018-06-20 16:10 UTC (permalink / raw)
To: Eric Auger
Cc: Eric Auger, QEMU Developers, qemu-arm, jia.he, Jia He,
Stefan Hajnoczi
On 20 June 2018 at 16:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 June 2018 at 09:08, Eric Auger <eric.auger@redhat.com> wrote:
>> +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.
I asked Stefan on IRC and the systemtap backend doesn't support
float or double, so we need to use an integer type here.
Lazy approach is to just round the % to an integer; if you think
the extra precision is useful then you can have %d.%d and
calculate the 10th-of-a-percent digit or something.
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data
2018-06-20 16:10 ` Peter Maydell
@ 2018-06-21 7:54 ` Auger Eric
0 siblings, 0 replies; 13+ messages in thread
From: Auger Eric @ 2018-06-21 7:54 UTC (permalink / raw)
To: Peter Maydell
Cc: Eric Auger, QEMU Developers, qemu-arm, jia.he, Jia He,
Stefan Hajnoczi
Hi Peter,
On 06/20/2018 06:10 PM, Peter Maydell wrote:
> On 20 June 2018 at 16:56, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 June 2018 at 09:08, Eric Auger <eric.auger@redhat.com> wrote:
>>> +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.
>
> I asked Stefan on IRC and the systemtap backend doesn't support
> float or double, so we need to use an integer type here.
> Lazy approach is to just round the % to an integer; if you think
> the extra precision is useful then you can have %d.%d and
> calculate the 10th-of-a-percent digit or something.
Oh OK thanks.
I will adopt the lazy approach then ;-)
Thanks
Eric
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data
2018-06-20 15:56 ` Peter Maydell
2018-06-20 16:10 ` Peter Maydell
@ 2018-06-21 7:51 ` Auger Eric
1 sibling, 0 replies; 13+ messages in thread
From: Auger Eric @ 2018-06-21 7:51 UTC (permalink / raw)
To: Peter Maydell; +Cc: Eric Auger, QEMU Developers, qemu-arm, jia.he, Jia He
Hi Peter,
On 06/20/2018 05:56 PM, Peter Maydell wrote:
> On 12 June 2018 at 09:08, Eric Auger <eric.auger@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.auger@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.maydell@linaro.org>
Thanks!
Eric
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] hw/arm/smmuv3: IOTLB emulation
2018-06-12 8:08 [Qemu-devel] [PATCH v2 0/4] ARM SMMUv3: IOTLB Emulation and VHOST Support Eric Auger
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 1/4] hw/arm/smmuv3: Fix translate error handling Eric Auger
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 2/4] hw/arm/smmuv3: Cache/invalidate config data Eric Auger
@ 2018-06-12 8:08 ` Eric Auger
2018-06-20 16:07 ` Peter Maydell
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 4/4] hw/arm/smmuv3: Add notifications on invalidation Eric Auger
3 siblings, 1 reply; 13+ messages in thread
From: Eric Auger @ 2018-06-12 8:08 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
jia.he, hejianet
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 <eric.auger@redhat.com>
---
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 */
+ 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)
+{
+ SMMUIOTLBKey *k1 = (SMMUIOTLBKey *)v1;
+ SMMUIOTLBKey *k2 = (SMMUIOTLBKey *)v2;
+
+ return (k1->asid == k2->asid) && (k1->iova == k2->iova);
+}
+
static void smmu_base_realize(DeviceState *dev, Error **errp)
{
SMMUState *s = ARM_SMMU(dev);
@@ -340,6 +397,8 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
return;
}
s->configs = g_hash_table_new_full(NULL, NULL, NULL, g_free);
+ s->iotlb = g_hash_table_new_full(smmu_iotlb_key_hash, smmu_iotlb_key_equal,
+ g_free, g_free);
s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
if (s->primary_bus) {
@@ -354,6 +413,7 @@ static void smmu_base_reset(DeviceState *dev)
SMMUState *s = ARM_SMMU(dev);
g_hash_table_remove_all(s->configs);
+ g_hash_table_remove_all(s->iotlb);
}
static Property smmu_dev_properties[] = {
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 4c41f51..cc21f4c 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -605,6 +605,10 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
SMMUEventInfo event = {.type = SMMU_EVT_NONE, .sid = sid};
SMMUPTWEventInfo ptw_info = {};
SMMUTranslationStatus status;
+ SMMUState *bs = ARM_SMMU(s);
+ uint64_t page_mask, aligned_addr;
+ IOMMUTLBEntry *cached_entry = NULL;
+ SMMUTransTableInfo *tt;
SMMUTransCfg *cfg = NULL;
IOMMUTLBEntry entry = {
.target_as = &address_space_memory,
@@ -613,6 +617,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
.addr_mask = ~(hwaddr)0,
.perm = IOMMU_NONE,
};
+ SMMUIOTLBKey key, *new_key;
qemu_mutex_lock(&s->mutex);
@@ -637,7 +642,57 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
goto epilogue;
}
- if (smmu_ptw(cfg, addr, flag, &entry, &ptw_info)) {
+ tt = select_tt(cfg, addr);
+ if (!tt) {
+ if (event.record_trans_faults) {
+ event.type = SMMU_EVT_F_TRANSLATION;
+ event.u.f_translation.addr = addr;
+ event.u.f_translation.rnw = flag & 0x1;
+ }
+ status = SMMU_TRANS_ERROR;
+ goto epilogue;
+ }
+
+ page_mask = (1ULL << (tt->granule_sz)) - 1;
+ aligned_addr = addr & ~page_mask;
+
+ key.asid = cfg->asid;
+ key.iova = aligned_addr;
+
+ cached_entry = g_hash_table_lookup(bs->iotlb, &key);
+ if (cached_entry) {
+ cfg->iotlb_hits += 1;
+ trace_smmu_iotlb_cache_hit(cfg->asid, aligned_addr,
+ cfg->iotlb_hits, cfg->iotlb_misses,
+ 100 * cfg->iotlb_hits /
+ (cfg->iotlb_hits + cfg->iotlb_misses));
+ if ((flag & IOMMU_WO) && !(cached_entry->perm & IOMMU_WO)) {
+ status = SMMU_TRANS_ERROR;
+ if (event.record_trans_faults) {
+ event.type = SMMU_EVT_F_PERMISSION;
+ event.u.f_permission.addr = addr;
+ event.u.f_permission.rnw = flag & 0x1;
+ }
+ } else {
+ status = SMMU_TRANS_SUCCESS;
+ }
+ goto epilogue;
+ }
+
+ cfg->iotlb_misses += 1;
+ trace_smmu_iotlb_cache_miss(cfg->asid, addr & ~page_mask,
+ cfg->iotlb_hits, cfg->iotlb_misses,
+ 100 * cfg->iotlb_hits /
+ (cfg->iotlb_hits + cfg->iotlb_misses));
+
+ if (g_hash_table_size(bs->iotlb) >= SMMU_IOTLB_MAX_SIZE) {
+ smmu_iotlb_inv_all(bs);
+ }
+
+ cached_entry = g_new0(IOMMUTLBEntry, 1);
+
+ if (smmu_ptw(cfg, aligned_addr, flag, cached_entry, &ptw_info)) {
+ g_free(cached_entry);
switch (ptw_info.type) {
case SMMU_PTW_ERR_WALK_EABT:
event.type = SMMU_EVT_F_WALK_EABT;
@@ -679,6 +734,10 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
}
status = SMMU_TRANS_ERROR;
} else {
+ new_key = g_new0(SMMUIOTLBKey, 1);
+ new_key->asid = cfg->asid;
+ new_key->iova = aligned_addr;
+ g_hash_table_insert(bs->iotlb, new_key, cached_entry);
status = SMMU_TRANS_SUCCESS;
}
@@ -687,6 +746,9 @@ epilogue:
switch (status) {
case SMMU_TRANS_SUCCESS:
entry.perm = flag;
+ entry.translated_addr = cached_entry->translated_addr +
+ (addr & page_mask);
+ entry.addr_mask = cached_entry->addr_mask;
trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
entry.translated_addr, entry.perm);
break;
@@ -832,10 +894,39 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
smmuv3_flush_config(sdev);
break;
}
- case SMMU_CMD_TLBI_NH_ALL:
case SMMU_CMD_TLBI_NH_ASID:
- case SMMU_CMD_TLBI_NH_VA:
+ {
+ uint16_t asid = CMD_ASID(&cmd);
+
+ trace_smmuv3_cmdq_tlbi_nh_asid(asid);
+ smmu_iotlb_inv_asid(bs, asid);
+ break;
+ }
+ case SMMU_CMD_TLBI_NH_ALL:
+ case SMMU_CMD_TLBI_NSNH_ALL:
+ trace_smmuv3_cmdq_tlbi_nh();
+ smmu_iotlb_inv_all(bs);
+ break;
case SMMU_CMD_TLBI_NH_VAA:
+ {
+ dma_addr_t addr = CMD_ADDR(&cmd);
+ uint16_t vmid = CMD_VMID(&cmd);
+
+ trace_smmuv3_cmdq_tlbi_nh_vaa(vmid, addr);
+ smmu_iotlb_inv_all(bs);
+ break;
+ }
+ case SMMU_CMD_TLBI_NH_VA:
+ {
+ uint16_t asid = CMD_ASID(&cmd);
+ uint16_t vmid = CMD_VMID(&cmd);
+ dma_addr_t addr = CMD_ADDR(&cmd);
+ bool leaf = CMD_LEAF(&cmd);
+
+ trace_smmuv3_cmdq_tlbi_nh_va(vmid, asid, addr, leaf);
+ smmu_iotlb_inv_iova(bs, asid, addr);
+ break;
+ }
case SMMU_CMD_TLBI_EL3_ALL:
case SMMU_CMD_TLBI_EL3_VA:
case SMMU_CMD_TLBI_EL2_ALL:
@@ -844,7 +935,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
case SMMU_CMD_TLBI_EL2_VAA:
case SMMU_CMD_TLBI_S12_VMALL:
case SMMU_CMD_TLBI_S2_IPA:
- case SMMU_CMD_TLBI_NSNH_ALL:
case SMMU_CMD_ATC_INV:
case SMMU_CMD_PRI_RESP:
case SMMU_CMD_RESUME:
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 5bddfeb..8ad900c 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -12,6 +12,11 @@ smmu_ptw_invalid_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr,
smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t address) "stage=%d level=%d iova=0x%"PRIx64" base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" page address = 0x%"PRIx64
smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t iova, uint64_t gpa, int bsize_mb) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" iova=0x%"PRIx64" block address = 0x%"PRIx64" block size = %d MiB"
smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
+smmu_iotlb_cache_hit(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, float p) "IOTLB cache HIT asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%.1f"
+smmu_iotlb_cache_miss(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, float p) "IOTLB cache MISS asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%.1f"
+smmu_iotlb_inv_all(void) "IOTLB invalidate all"
+smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
+smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
#hw/arm/smmuv3.c
smmuv3_read_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
@@ -42,6 +47,10 @@ 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_tlbi_nh_va(int vmid, int asid, uint64_t addr, bool leaf) " |_ vmid =%d asid =%d addr=0x%"PRIx64" leaf=%d"
+smmuv3_cmdq_tlbi_nh_vaa(int vmid, uint64_t addr) " |_ vmid =%d addr=0x%"PRIx64
+smmuv3_cmdq_tlbi_nh(void) ""
+smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
smmuv3_cmdq_cfgi_cd(uint32_t sid) " |_ streamid = %d"
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)"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 7ce95ca..d173806 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -67,6 +67,8 @@ typedef struct SMMUTransCfg {
uint8_t tbi; /* Top Byte Ignore */
uint16_t asid;
SMMUTransTableInfo tt[2];
+ uint32_t iotlb_hits; /* counts IOTLB hits for this asid */
+ uint32_t iotlb_misses; /* counts IOTLB misses for this asid */
} SMMUTransCfg;
typedef struct SMMUDevice {
@@ -89,6 +91,11 @@ typedef struct SMMUPciBus {
SMMUDevice *pbdev[0]; /* Parent array is sparse, so dynamically alloc */
} SMMUPciBus;
+typedef struct SMMUIOTLBKey {
+ uint64_t iova;
+ uint16_t asid;
+} SMMUIOTLBKey;
+
typedef struct SMMUState {
/* <private> */
SysBusDevice dev;
@@ -147,4 +154,10 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova);
/* Return the iommu mr associated to @sid, or NULL if none */
IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
+#define SMMU_IOTLB_MAX_SIZE 256
+
+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);
+
#endif /* HW_ARM_SMMU_COMMON */
--
2.5.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/smmuv3: IOTLB emulation
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 3/4] hw/arm/smmuv3: IOTLB emulation Eric Auger
@ 2018-06-20 16:07 ` Peter Maydell
2018-06-21 7:49 ` Auger Eric
0 siblings, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2018-06-20 16:07 UTC (permalink / raw)
To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm, jia.he, Jia He
On 12 June 2018 at 09:08, Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>
> ---
>
> 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 ?
> + 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.
Otherwise looks OK I think.
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/smmuv3: IOTLB emulation
2018-06-20 16:07 ` Peter Maydell
@ 2018-06-21 7:49 ` Auger Eric
0 siblings, 0 replies; 13+ messages in thread
From: Auger Eric @ 2018-06-21 7:49 UTC (permalink / raw)
To: Peter Maydell; +Cc: Eric Auger, QEMU Developers, qemu-arm, jia.he, Jia He
Hi Peter,
On 06/20/2018 06:07 PM, Peter Maydell wrote:
> On 12 June 2018 at 09:08, Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>
>> ---
>>
>> 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] hw/arm/smmuv3: Add notifications on invalidation
2018-06-12 8:08 [Qemu-devel] [PATCH v2 0/4] ARM SMMUv3: IOTLB Emulation and VHOST Support Eric Auger
` (2 preceding siblings ...)
2018-06-12 8:08 ` [Qemu-devel] [PATCH v2 3/4] hw/arm/smmuv3: IOTLB emulation Eric Auger
@ 2018-06-12 8:08 ` Eric Auger
2018-06-20 16:15 ` Peter Maydell
3 siblings, 1 reply; 13+ messages in thread
From: Eric Auger @ 2018-06-12 8:08 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
jia.he, hejianet
On TLB invalidation commands, let's call registered
IOMMU notifiers. Those can only be UNMAP notifiers.
SMMUv3 does not support notification on MAP (VFIO).
This patch allows vhost use case where IOTLB API is notified
on each guest IOTLB invalidation.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
hw/arm/smmu-common.c | 34 +++++++++++++++
hw/arm/smmuv3.c | 99 +++++++++++++++++++++++++++++++++++++++++++-
hw/arm/trace-events | 5 +++
include/hw/arm/smmu-common.h | 6 +++
4 files changed, 142 insertions(+), 2 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 16cd33a..5929275 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -385,6 +385,40 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
return (k1->asid == k2->asid) && (k1->iova == k2->iova);
}
+/* Unmap the whole notifier's range */
+static void smmu_unmap_notifier_range(IOMMUNotifier *n)
+{
+ IOMMUTLBEntry entry;
+
+ entry.target_as = &address_space_memory;
+ entry.iova = n->start;
+ entry.perm = IOMMU_NONE;
+ entry.addr_mask = n->end - n->start;
+
+ memory_region_notify_one(n, &entry);
+}
+
+/* Unmap all notifiers attached to @mr */
+inline void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr)
+{
+ IOMMUNotifier *n;
+
+ trace_smmu_inv_notifiers_mr(mr->parent_obj.name);
+ IOMMU_NOTIFIER_FOREACH(n, mr) {
+ smmu_unmap_notifier_range(n);
+ }
+}
+
+/* Unmap all notifiers of all mr's */
+void smmu_inv_notifiers_all(SMMUState *s)
+{
+ SMMUNotifierNode *node;
+
+ QLIST_FOREACH(node, &s->notifiers_list, next) {
+ smmu_inv_notifiers_mr(&node->sdev->iommu);
+ }
+}
+
static void smmu_base_realize(DeviceState *dev, Error **errp)
{
SMMUState *s = ARM_SMMU(dev);
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index cc21f4c..6afc07e 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -780,6 +780,68 @@ epilogue:
return entry;
}
+/**
+ * smmuv3_notify_iova - call the notifier @n for a given
+ * @asid and @iova tuple.
+ *
+ * @mr: IOMMU mr region handle
+ * @n: notifier to be called
+ * @asid: address space ID or negative value if we don't care
+ * @iova: iova
+ */
+static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
+ IOMMUNotifier *n,
+ int asid,
+ dma_addr_t iova)
+{
+ SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
+ SMMUEventInfo event = {};
+ SMMUTransTableInfo *tt;
+ SMMUTransCfg *cfg;
+ IOMMUTLBEntry entry;
+
+ cfg = smmuv3_get_config(sdev, &event);
+ if (!cfg) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s error decoding the configuration for iommu mr=%s\n",
+ __func__, mr->parent_obj.name);
+ return;
+ }
+
+ if (asid >= 0 && cfg->asid != asid) {
+ return;
+ }
+
+ tt = select_tt(cfg, iova);
+ if (!tt) {
+ return;
+ }
+
+ entry.target_as = &address_space_memory;
+ entry.iova = iova;
+ entry.addr_mask = (1 << tt->granule_sz) - 1;
+ entry.perm = IOMMU_NONE;
+
+ memory_region_notify_one(n, &entry);
+}
+
+/* invalidate an asid/iova tuple in all mr's */
+static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova)
+{
+ SMMUNotifierNode *node;
+
+ QLIST_FOREACH(node, &s->notifiers_list, next) {
+ IOMMUMemoryRegion *mr = &node->sdev->iommu;
+ IOMMUNotifier *n;
+
+ trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, iova);
+
+ IOMMU_NOTIFIER_FOREACH(n, mr) {
+ smmuv3_notify_iova(mr, n, asid, iova);
+ }
+ }
+}
+
static int smmuv3_cmdq_consume(SMMUv3State *s)
{
SMMUState *bs = ARM_SMMU(s);
@@ -899,12 +961,14 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
uint16_t asid = CMD_ASID(&cmd);
trace_smmuv3_cmdq_tlbi_nh_asid(asid);
+ smmu_inv_notifiers_all(&s->smmu_state);
smmu_iotlb_inv_asid(bs, asid);
break;
}
case SMMU_CMD_TLBI_NH_ALL:
case SMMU_CMD_TLBI_NSNH_ALL:
trace_smmuv3_cmdq_tlbi_nh();
+ smmu_inv_notifiers_all(&s->smmu_state);
smmu_iotlb_inv_all(bs);
break;
case SMMU_CMD_TLBI_NH_VAA:
@@ -913,6 +977,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
uint16_t vmid = CMD_VMID(&cmd);
trace_smmuv3_cmdq_tlbi_nh_vaa(vmid, addr);
+ smmuv3_inv_notifiers_iova(bs, -1, addr);
smmu_iotlb_inv_all(bs);
break;
}
@@ -924,6 +989,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
bool leaf = CMD_LEAF(&cmd);
trace_smmuv3_cmdq_tlbi_nh_va(vmid, asid, addr, leaf);
+ smmuv3_inv_notifiers_iova(bs, asid, addr);
smmu_iotlb_inv_iova(bs, asid, addr);
break;
}
@@ -1402,9 +1468,38 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
IOMMUNotifierFlag old,
IOMMUNotifierFlag new)
{
+ SMMUDevice *sdev = container_of(iommu, SMMUDevice, iommu);
+ SMMUv3State *s3 = sdev->smmu;
+ SMMUState *s = &(s3->smmu_state);
+ SMMUNotifierNode *node = NULL;
+ SMMUNotifierNode *next_node = NULL;
+
+ if (new == IOMMU_NOTIFIER_MAP) {
+ int bus_num = pci_bus_num(sdev->bus);
+ PCIDevice *pcidev = pci_find_device(sdev->bus, bus_num, sdev->devfn);
+
+ warn_report("SMMUv3 does not support notification on MAP: "
+ "device %s will not function properly", pcidev->name);
+ }
+
if (old == IOMMU_NOTIFIER_NONE) {
- warn_report("SMMUV3 does not support vhost/vfio integration yet: "
- "devices of those types will not function properly");
+ trace_smmuv3_notify_flag_add(iommu->parent_obj.name);
+ node = g_malloc0(sizeof(*node));
+ node->sdev = sdev;
+ QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
+ return;
+ }
+
+ /* update notifier node with new flags */
+ QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
+ if (node->sdev == sdev) {
+ if (new == IOMMU_NOTIFIER_NONE) {
+ trace_smmuv3_notify_flag_del(iommu->parent_obj.name);
+ QLIST_REMOVE(node, next);
+ g_free(node);
+ }
+ return;
+ }
}
}
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 8ad900c..11f7bbf 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -17,6 +17,7 @@ smmu_iotlb_cache_miss(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss,
smmu_iotlb_inv_all(void) "IOTLB invalidate all"
smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
+smmu_inv_notifiers_mr(const char *name) "iommu mr=%s"
#hw/arm/smmuv3.c
smmuv3_read_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
@@ -55,3 +56,7 @@ smmuv3_cmdq_cfgi_cd(uint32_t sid) " |_ streamid = %d"
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)"
smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid %d"
+smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
+smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
+smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint64_t iova) "iommu mr=%s asid=%d iova=0x%"PRIx64
+
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index d173806..50e2912 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -160,4 +160,10 @@ 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);
+/* Unmap the range of all the notifiers registered to any IOMMU mr */
+void smmu_inv_notifiers_all(SMMUState *s);
+
+/* Unmap the range of all the notifiers registered to @mr */
+void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr);
+
#endif /* HW_ARM_SMMU_COMMON */
--
2.5.5
^ permalink raw reply related [flat|nested] 13+ messages in thread