Hi,
On 10/7/2026 8:23 AM, Tan Chi wrote:
IOTINVAL currently walks the IOATC hash table and invalidates matching
entries by setting iot->perm to IOMMU_NONE. The invalidated entries remain
in the hash table afterwards.
Under workloads with frequent DMA map/unmap activity, invalid entries can
accumulate while only a smaller number of entries remain valid. Subsequent
IOTINVAL operations continue to scan these entries. They are also included
in g_hash_table_size(), causing the IOATC to reach iot_limit earlier and
the entire cache to be flushed more frequently.
Remove matching entries during invalidation instead of leaving them in the
hash table with IOMMU_NONE.
Removing an entry also frees it, so a concurrent lookup must not retain a
pointer to an entry being removed. Accesses that modify the IOATC hash table
must also be serialized. Add iot_lock to protect IOATC lookup, update,
invalidation, and reset operations, keeping both the hash table state and
cached entry lifetime valid while they are accessed.
I'll ask you to please split this patch in two: one to add the iot_lock code,
another one for the IOATC inval changes.
One small comment below:
The measurements use a Linux 7.3-rc5 guest based on linux-iommu/next
commit cec2dc7663e7 with v6 of the "iommu/riscv: Enable MSI remapping,
IOMMU_DMA and VFIO" series applied. The QEMU behavior is not specific to
this guest kernel; this configuration makes it easier to exercise through
normal DMA map/unmap activity.
In a high-churn NVMe workload with frequent DMA map/unmap activity, runtime
drops from 168 seconds to 35 seconds, a 4.8x speedup.
A short fio workload with a small working set remains within measurement
noise, showing no measurable regression from the added locking. Across
three runs for each version of a 300-second fio workload with a 4 GiB
working set, median throughput improves from 68.6 MiB/s to 75.0 MiB/s, an
increase of 9.3%.
Signed-off-by: Tan Chi <[email protected]>
---
hw/riscv/riscv-iommu.c | 160 ++++++++++++++++++++---------------------
hw/riscv/riscv-iommu.h | 1 +
2 files changed, 79 insertions(+), 82 deletions(-)
diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index 323a041b4a..85121f6468 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -1582,107 +1582,96 @@ static guint riscv_iommu_iot_hash(gconstpointer v)
/* GV: 0 AV: 0 PSCV: 0 GVMA: 0 */
/* GV: 0 AV: 0 GVMA: 1 */
static
-void riscv_iommu_iot_inval_all(gpointer key, gpointer value, gpointer data)
+gboolean riscv_iommu_iot_inval_all(gpointer key, gpointer value, gpointer data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag;
}
/* GV: 0 AV: 0 PSCV: 1 GVMA: 0 */
static
-void riscv_iommu_iot_inval_pscid(gpointer key, gpointer value, gpointer data)
+gboolean riscv_iommu_iot_inval_pscid(gpointer key, gpointer value,
+ gpointer data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag &&
- iot->pscid == arg->pscid) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag &&
+ iot->pscid == arg->pscid;
}
/* GV: 0 AV: 1 PSCV: 0 GVMA: 0 */
static
-void riscv_iommu_iot_inval_iova(gpointer key, gpointer value, gpointer data)
+gboolean riscv_iommu_iot_inval_iova(gpointer key, gpointer value, gpointer
data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag &&
- iot->iova == arg->iova) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag &&
+ iot->iova == arg->iova;
}
/* GV: 0 AV: 1 PSCV: 1 GVMA: 0 */
-static void riscv_iommu_iot_inval_pscid_iova(gpointer key, gpointer value,
- gpointer data)
+static gboolean riscv_iommu_iot_inval_pscid_iova(gpointer key,
+ gpointer value,
+ gpointer data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag &&
- iot->pscid == arg->pscid &&
- iot->iova == arg->iova) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag &&
+ iot->pscid == arg->pscid &&
+ iot->iova == arg->iova;
}
/* GV: 1 AV: 0 PSCV: 0 GVMA: 0 */
/* GV: 1 AV: 0 GVMA: 1 */
-static
-void riscv_iommu_iot_inval_gscid(gpointer key, gpointer value, gpointer data)
+static gboolean riscv_iommu_iot_inval_gscid(gpointer key, gpointer value,
+ gpointer data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag &&
- iot->gscid == arg->gscid) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag &&
+ iot->gscid == arg->gscid;
}
/* GV: 1 AV: 0 PSCV: 1 GVMA: 0 */
-static void riscv_iommu_iot_inval_gscid_pscid(gpointer key, gpointer value,
- gpointer data)
+static gboolean riscv_iommu_iot_inval_gscid_pscid(gpointer key,
+ gpointer value,
+ gpointer data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag &&
- iot->gscid == arg->gscid &&
- iot->pscid == arg->pscid) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag &&
+ iot->gscid == arg->gscid &&
+ iot->pscid == arg->pscid;
}
/* GV: 1 AV: 1 PSCV: 0 GVMA: 0 */
/* GV: 1 AV: 1 GVMA: 1 */
-static void riscv_iommu_iot_inval_gscid_iova(gpointer key, gpointer value,
- gpointer data)
+static gboolean riscv_iommu_iot_inval_gscid_iova(gpointer key,
+ gpointer value,
+ gpointer data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag &&
- iot->gscid == arg->gscid &&
- iot->iova == arg->iova) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag &&
+ iot->gscid == arg->gscid &&
+ iot->iova == arg->iova;
}
/* GV: 1 AV: 1 PSCV: 1 GVMA: 0 */
-static void riscv_iommu_iot_inval_gscid_pscid_iova(gpointer key, gpointer
value,
- gpointer data)
+static gboolean riscv_iommu_iot_inval_gscid_pscid_iova(gpointer key,
+ gpointer value,
+ gpointer data)
{
RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
- if (iot->tag == arg->tag &&
- iot->gscid == arg->gscid &&
- iot->pscid == arg->pscid &&
- iot->iova == arg->iova) {
- iot->perm = IOMMU_NONE;
- }
+ return iot->tag == arg->tag &&
+ iot->gscid == arg->gscid &&
+ iot->pscid == arg->pscid &&
+ iot->iova == arg->iova;
}
-/* caller should keep ref-count for iot_cache object */
+/* caller must hold s->iot_lock */
static RISCVIOMMUEntry *riscv_iommu_iot_lookup(RISCVIOMMUContext *ctx,
GHashTable *iot_cache, hwaddr iova, RISCVIOMMUTransTag transtag)
{
@@ -1695,27 +1684,26 @@ static RISCVIOMMUEntry
*riscv_iommu_iot_lookup(RISCVIOMMUContext *ctx,
return g_hash_table_lookup(iot_cache, &key);
}
-/* caller should keep ref-count for iot_cache object */
-static void riscv_iommu_iot_update(RISCVIOMMUState *s,
- GHashTable *iot_cache, RISCVIOMMUEntry *iot)
+/* caller must hold s->iot_lock */
+static void riscv_iommu_iot_update(RISCVIOMMUState *s, RISCVIOMMUEntry *iot)
{
if (!s->iot_limit) {
return;
}
if (g_hash_table_size(s->iot_cache) >= s->iot_limit) {
- iot_cache = g_hash_table_new_full(riscv_iommu_iot_hash,
+ GHashTable *old_cache = s->iot_cache;
+ s->iot_cache = g_hash_table_new_full(riscv_iommu_iot_hash,
riscv_iommu_iot_equal,
g_free, NULL);
- g_hash_table_unref(qatomic_xchg(&s->iot_cache, iot_cache));
+ g_hash_table_unref(old_cache);
}
- g_hash_table_add(iot_cache, iot);
+ g_hash_table_add(s->iot_cache, iot);
}
-static void riscv_iommu_iot_inval(RISCVIOMMUState *s, GHFunc func,
+static void riscv_iommu_iot_inval(RISCVIOMMUState *s, GHRFunc func,
uint32_t gscid, uint32_t pscid, hwaddr iova, RISCVIOMMUTransTag transtag)
{
- GHashTable *iot_cache;
RISCVIOMMUEntry key = {
.tag = transtag,
.gscid = gscid,
@@ -1723,9 +1711,9 @@ static void riscv_iommu_iot_inval(RISCVIOMMUState *s,
GHFunc func,
.iova = PPN_DOWN(iova),
};
- iot_cache = g_hash_table_ref(s->iot_cache);
- g_hash_table_foreach(iot_cache, func, &key);
- g_hash_table_unref(iot_cache);
+ qemu_mutex_lock(&s->iot_lock);
+ g_hash_table_foreach_remove(s->iot_cache, func, &key);
+ qemu_mutex_unlock(&s->iot_lock);
}
static RISCVIOMMUTransTag riscv_iommu_get_transtag(RISCVIOMMUContext *ctx)
@@ -1750,12 +1738,10 @@ static int riscv_iommu_translate(RISCVIOMMUState *s,
RISCVIOMMUContext *ctx,
IOMMUAccessFlags perm;
bool enable_pid;
bool enable_pri;
- GHashTable *iot_cache;
int fault;
riscv_iommu_hpm_incr_ctr(s, ctx, RISCV_IOMMU_HPMEVENT_URQ);
- iot_cache = g_hash_table_ref(s->iot_cache);
enable_pri = (iotlb->perm == IOMMU_NONE) &&
(ctx->tc & RISCV_IOMMU_DC_TC_EN_PRI);
enable_pid = (ctx->tc & RISCV_IOMMU_DC_TC_PDTV);
@@ -1771,15 +1757,18 @@ static int riscv_iommu_translate(RISCVIOMMUState *s,
RISCVIOMMUContext *ctx,
}
}
- iot = riscv_iommu_iot_lookup(ctx, iot_cache, iotlb->iova, transtag);
+ qemu_mutex_lock(&s->iot_lock);
+ iot = riscv_iommu_iot_lookup(ctx, s->iot_cache, iotlb->iova, transtag);
perm = iot ? iot->perm : IOMMU_NONE;
if (perm != IOMMU_NONE) {
iotlb->translated_addr = PPN_PHYS(iot->phys);
iotlb->addr_mask = ~TARGET_PAGE_MASK;
iotlb->perm = perm;
fault = 0;
+ qemu_mutex_unlock(&s->iot_lock);
goto done;
}
+ qemu_mutex_unlock(&s->iot_lock);
Can't we do:
> + qemu_mutex_lock(&s->iot_lock);
> + iot = riscv_iommu_iot_lookup(ctx, s->iot_cache, iotlb->iova, transtag);
> + qemu_mutex_unlock(&s->iot_lock);
(...)
Instead of adding two unlocks? It doesn't seem to me that the lock is needed
to set iotlb->... before jumping to 'done'.
Thanks,
Daniel
riscv_iommu_hpm_incr_ctr(s, ctx, RISCV_IOMMU_HPMEVENT_TLB_MISS);
@@ -1805,12 +1794,12 @@ static int riscv_iommu_translate(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
iot->pscid = get_field(ctx->ta, RISCV_IOMMU_DC_TA_PSCID);
iot->perm = iotlb->perm;
iot->tag = transtag;
- riscv_iommu_iot_update(s, iot_cache, iot);
+ qemu_mutex_lock(&s->iot_lock);
+ riscv_iommu_iot_update(s, iot);
+ qemu_mutex_unlock(&s->iot_lock);
}
done:
- g_hash_table_unref(iot_cache);
-
if (enable_pri && fault) {
struct riscv_iommu_pq_record pr = {0};
if (enable_pid) {
@@ -1969,7 +1958,8 @@ static void riscv_iommu_process_cq_tail(RISCVIOMMUState
*s)
dma_addr_t addr;
uint32_t tail, head, ctrl;
uint64_t cmd_opcode;
- GHFunc func;
+ GHRFunc iot_func;
+ GHFunc ctx_func;
ctrl = riscv_iommu_reg_get32(s, RISCV_IOMMU_REG_CQCSR);
tail = riscv_iommu_reg_get32(s, RISCV_IOMMU_REG_CQT) & s->cq_mask;
@@ -2032,18 +2022,18 @@ static void riscv_iommu_process_cq_tail(RISCVIOMMUState
*s)
goto cmd_ill;
}
- func = riscv_iommu_iot_inval_all;
+ iot_func = riscv_iommu_iot_inval_all;
if (gv) {
- func = (av) ? riscv_iommu_iot_inval_gscid_iova :
+ iot_func = (av) ? riscv_iommu_iot_inval_gscid_iova :
riscv_iommu_iot_inval_gscid;
}
riscv_iommu_iot_inval(
- s, func, gscid, pscid, iova, RISCV_IOMMU_TRANS_TAG_VG);
+ s, iot_func, gscid, pscid, iova, RISCV_IOMMU_TRANS_TAG_VG);
riscv_iommu_iot_inval(
- s, func, gscid, pscid, iova, RISCV_IOMMU_TRANS_TAG_VN);
+ s, iot_func, gscid, pscid, iova, RISCV_IOMMU_TRANS_TAG_VN);
break;
}
@@ -2063,24 +2053,24 @@ static void riscv_iommu_process_cq_tail(RISCVIOMMUState *s)
if (gv) {
transtag = RISCV_IOMMU_TRANS_TAG_VN;
if (pscv) {
- func = (av) ? riscv_iommu_iot_inval_gscid_pscid_iova :
+ iot_func = (av) ? riscv_iommu_iot_inval_gscid_pscid_iova :
riscv_iommu_iot_inval_gscid_pscid;
} else {
- func = (av) ? riscv_iommu_iot_inval_gscid_iova :
+ iot_func = (av) ? riscv_iommu_iot_inval_gscid_iova :
riscv_iommu_iot_inval_gscid;
}
} else {
transtag = RISCV_IOMMU_TRANS_TAG_SS;
if (pscv) {
- func = (av) ? riscv_iommu_iot_inval_pscid_iova :
+ iot_func = (av) ? riscv_iommu_iot_inval_pscid_iova :
riscv_iommu_iot_inval_pscid;
} else {
- func = (av) ? riscv_iommu_iot_inval_iova :
+ iot_func = (av) ? riscv_iommu_iot_inval_iova :
riscv_iommu_iot_inval_all;
}
}
- riscv_iommu_iot_inval(s, func, gscid, pscid, iova, transtag);
+ riscv_iommu_iot_inval(s, iot_func, gscid, pscid, iova, transtag);
break;
}
@@ -2088,12 +2078,12 @@ static void riscv_iommu_process_cq_tail(RISCVIOMMUState *s)
RISCV_IOMMU_CMD_IODIR_OPCODE):
if (!(cmd.dword0 & RISCV_IOMMU_CMD_IODIR_DV)) {
/* invalidate all device context cache mappings */
- func = riscv_iommu_ctx_inval_all;
+ ctx_func = riscv_iommu_ctx_inval_all;
} else {
/* invalidate all device context matching DID */
- func = riscv_iommu_ctx_inval_devid;
+ ctx_func = riscv_iommu_ctx_inval_devid;
}
- riscv_iommu_ctx_inval(s, func,
+ riscv_iommu_ctx_inval(s, ctx_func,
get_field(cmd.dword0, RISCV_IOMMU_CMD_IODIR_DID), 0);
break;
@@ -2103,9 +2093,9 @@ static void riscv_iommu_process_cq_tail(RISCVIOMMUState *s)
/* illegal command arguments IODIR_PDT & DV == 0 */
goto cmd_ill;
} else {
- func = riscv_iommu_ctx_inval_devid_procid;
+ ctx_func = riscv_iommu_ctx_inval_devid_procid;
}
- riscv_iommu_ctx_inval(s, func,
+ riscv_iommu_ctx_inval(s, ctx_func,
get_field(cmd.dword0, RISCV_IOMMU_CMD_IODIR_DID),
get_field(cmd.dword0, RISCV_IOMMU_CMD_IODIR_PID));
break;
@@ -2671,6 +2661,7 @@ static void riscv_iommu_instance_init(Object *obj)
riscv_iommu_ctx_equal,
g_free, NULL);
+ qemu_mutex_init(&s->iot_lock);
s->iot_cache = g_hash_table_new_full(riscv_iommu_iot_hash,
riscv_iommu_iot_equal,
g_free, NULL);
@@ -2690,6 +2681,8 @@ static void riscv_iommu_instance_finalize(Object *obj)
g_hash_table_unref(s->ctx_cache);
g_hash_table_unref(s->iot_cache);
+
+ qemu_mutex_destroy(&s->iot_lock);
}
static void riscv_iommu_realize(DeviceState *dev, Error **errp)
@@ -2850,7 +2843,10 @@ void riscv_iommu_reset(RISCVIOMMUState *s)
riscv_iommu_reg_set32(s, RISCV_IOMMU_REG_IPSR, 0);
g_hash_table_remove_all(s->ctx_cache);
+
+ qemu_mutex_lock(&s->iot_lock);
g_hash_table_remove_all(s->iot_cache);
+ qemu_mutex_unlock(&s->iot_lock);
}
static const Property riscv_iommu_properties[] = {
diff --git a/hw/riscv/riscv-iommu.h b/hw/riscv/riscv-iommu.h
index da70e8bfa8..16d257a106 100644
--- a/hw/riscv/riscv-iommu.h
+++ b/hw/riscv/riscv-iommu.h
@@ -73,6 +73,7 @@ struct RISCVIOMMUState {
GHashTable *iot_cache; /* IO Translated Address Cache */
unsigned iot_limit; /* IO Translation Cache size limit */
+ QemuMutex iot_lock;
/* MMIO Hardware Interface */
MemoryRegion regs_mr;
