* [RFC PATCH v2 00/13] SMMUv3 nested translation support
@ 2024-04-08 14:08 Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 01/13] hw/arm/smmu: Use enum for SMMU stage Mostafa Saleh
` (13 more replies)
0 siblings, 14 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
Currently, QEMU supports emulating either stage-1 or stage-2 SMMUs
but not nested instances.
This patch series adds support for nested translation in SMMUv3,
this is controlled by property “arm-smmuv3.stage=nested”, and
advertised to guests as (IDR0.S1P == 1 && IDR0.S2P == 2)
Main changes(architecture):
============================
1) CDs are considered IPA and translated with stage-2.
2) TTBx and tables for stage-1 are considered IPA and translated
with stage-2.
3) Translate the IPA address with stage-2.
TLBs:
======
TLBs are the most tricky part.
1) General design
Unified(Combined) design is used, where entries with ASID=-1 are
IPAs(cached from stage-2 config)
TLBs are also modified to cache 2 permissions, a new permission added
"parent_perm."
For non-nested configuration, perm == parent_perm and nothing
changes. This is used to know which stage to use in case there is
a permission fault from a TLB entry.
2) Caching in TLB
Stage-1 and stage-2 are inserted in the TLB as is.
For nested translation, both entries are combined into one TLB
entry. The size (level and granule) are chosen from the smallest entries.
That means that a stage-1 translation can be cached with sage-2
granule in key, this is take into account lookup.
3) TLB Lookup
TLB lookup already uses ASID in key, so it can distinguish between
stage-1 and stage-2.
And as mentioned above, the granule for stage-1 can be different,
If stage-1 lookup failed, we try again with the stage-2 granule.
4) TLB invalidation
- Address invalidation is split, for IOVA(CMD_TLBI_NH_VA
/CMD_TLBI_NH_VAA) and IPA(CMD_TLBI_S2_IPA) based on ASID value
- CMD_TLBI_NH_ASID/CMD_TLBI_NH_ALL: Consider VMID if stage-2 is
supported, and invalidate stage-1 only by VMIDs
As far as I understand, this is compliant with the ARM architecture:
- ARM ARM DDI 0487J.a: RLGSCG, RTVTYQ, RGNJPZ
- ARM IHI 0070F.b: 16.2 Caching
An alternative approach would be to instantiate 2 TLBs, one per each
stage. I haven’t investigated that.
Others
=======
- Advertise SMMUv3.2-S2FWB, it is NOP for QEMU as it doesn’t support
attributes.
- OAS: A typical setup with nesting is to share CPU stage-2 with the
SMMU, and according to the user manual, SMMU OAS must match the
system physical address.
This was discussed before in
https://lore.kernel.org/all/20230226220650.1480786-11-smostafa@google.com/
The implementation here, follows the discussion, where migration is
added and oas is set up from the board (virt). However, the OAS is
chosen based on the CPU PARANGE as there is no fixed one.
- For nested configuration, IOVA notifier only notifies for stage-1
invalidations (as far as I understand this is the intended
behaviour as it notifies for IOVA)
- Stop ignoring VMID for stage-1 if stage-2 is also supported.
Future improvements:
=====================
1) One small improvement, that I don’t think it’s worth the extra
complexity, is in case of Stage-1 TLB miss for nested translation,
we can do stage-1 walk and lookup for stage-2 TLBs, instead of
doing the full walk.
2) Patch 0006 (hw/arm/smmuv3: Translate CD and TT using stage-2 table)
introduces a macro to use functions that rely on cfg for stage-2,
I don’t like it. However, I didn’t find a simple way around it,
either we change many functions to have a separate stage argument,
or add another arg in config, which is probably more code.
Testing
========
1) IOMMUFD + VFIO
Kernel: https://lore.kernel.org/all/cover.1683688960.git.nicolinc@nvidia.com/
VMM: https://qemu-devel.nongnu.narkive.com/o815DqpI/rfc-v5-0-8-arm-smmuv3-emulation-support
By assigning “virtio-net-pci,netdev=net0,disable-legacy=on,iommu_platform=on,ats=on”,
to a guest VM (on top of QEMU guest) with VIFO and IOMMUFD.
2) Work in progress prototype I am hacking on for nesting on KVM
(this is nowhere near complete, and misses many stuff but it
doesn't require VMs/VFIO) also with virtio-net-pci and git
cloning a bunch of stuff and also observing traces.
https://android-kvm.googlesource.com/linux/+log/refs/heads/smostafa/android15-6.6-smmu-nesting-wip
I also modified the Linux driver to test with mixed granules/levels.
hw/arm/smmuv3: Split smmuv3_translate() better viewed with --color-moved
Changes in v2:
v1: https://lore.kernel.org/qemu-devel/20240325101442.1306300-1-smostafa@google.com/
- Collected Eric Rbs
- Rework TLB to rely on VMID/ASID instead of an extra key.
- Fixed TLB issue with large stage-1 reported by Julian.
- Cap the OAS to 48 bits as PTW doesn’t support 52 bits.
- Fix ASID/VMID representation in some contexts as 16 bits while
they can be -1
- Increase visibility in trace points
Mostafa Saleh (13):
hw/arm/smmu: Use enum for SMMU stage
hw/arm/smmu: Split smmuv3_translate()
hw/arm/smmu: Consolidate ASID and VMID types
hw/arm/smmuv3: Translate CD and TT using stage-2 table
hw/arm/smmu-common: Support nested translation
hw/arm/smmu: Support nesting in smmuv3_range_inval()
hw/arm/smmu: Support nesting in the rest of commands
hw/arm/smmuv3: Support nested SMMUs in smmuv3_notify_iova()
hw/arm/smmuv3: Support and advertise nesting
hw/arm/smmuv3: Advertise S2FWB
hw/arm/smmu: Refactor SMMU OAS
hw/arm/smmuv3: Add property for OAS
hw/arm/virt: Set SMMU OAS based on CPU PARANGE
hw/arm/smmu-common.c | 306 +++++++++++++++++++++----
hw/arm/smmuv3-internal.h | 16 +-
hw/arm/smmuv3.c | 418 ++++++++++++++++++++++-------------
hw/arm/trace-events | 18 +-
hw/arm/virt.c | 14 +-
include/hw/arm/smmu-common.h | 57 ++++-
include/hw/arm/smmuv3.h | 1 +
target/arm/cpu.h | 2 +
target/arm/cpu64.c | 5 +
9 files changed, 608 insertions(+), 229 deletions(-)
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH v2 01/13] hw/arm/smmu: Use enum for SMMU stage
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 02/13] hw/arm/smmu: Split smmuv3_translate() Mostafa Saleh
` (12 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
Currently, translation stage is represented as an int, where 1 is stage-1 and
2 is stage-2, when nested is added, 3 would be confusing to represent nesting,
so we use an enum instead.
While keeping the same values, this is useful for:
- Doing tricks with bit masks, where BIT(0) is stage-1 and BIT(1) is
stage-2 and both is nested.
- Tracing, as stage is printed as int.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
hw/arm/smmu-common.c | 14 +++++++-------
hw/arm/smmuv3.c | 15 ++++++++-------
include/hw/arm/smmu-common.h | 11 +++++++++--
3 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 4caedb4998..3a7c350aca 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -304,7 +304,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
{
dma_addr_t baseaddr, indexmask;
- int stage = cfg->stage;
+ SMMUStage stage = cfg->stage;
SMMUTransTableInfo *tt = select_tt(cfg, iova);
uint8_t level, granule_sz, inputsize, stride;
@@ -392,7 +392,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
info->type = SMMU_PTW_ERR_TRANSLATION;
error:
- info->stage = 1;
+ info->stage = SMMU_STAGE_1;
tlbe->entry.perm = IOMMU_NONE;
return -EINVAL;
}
@@ -415,7 +415,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
dma_addr_t ipa, IOMMUAccessFlags perm,
SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
{
- const int stage = 2;
+ const SMMUStage stage = SMMU_STAGE_2;
int granule_sz = cfg->s2cfg.granule_sz;
/* ARM DDI0487I.a: Table D8-7. */
int inputsize = 64 - cfg->s2cfg.tsz;
@@ -513,7 +513,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
info->type = SMMU_PTW_ERR_TRANSLATION;
error:
- info->stage = 2;
+ info->stage = SMMU_STAGE_2;
tlbe->entry.perm = IOMMU_NONE;
return -EINVAL;
}
@@ -532,9 +532,9 @@ error:
int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
{
- if (cfg->stage == 1) {
+ if (cfg->stage == SMMU_STAGE_1) {
return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
- } else if (cfg->stage == 2) {
+ } else if (cfg->stage == SMMU_STAGE_2) {
/*
* If bypassing stage 1(or unimplemented), the input address is passed
* directly to stage 2 as IPA. If the input address of a transaction
@@ -543,7 +543,7 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
*/
if (iova >= (1ULL << cfg->oas)) {
info->type = SMMU_PTW_ERR_ADDR_SIZE;
- info->stage = 1;
+ info->stage = SMMU_STAGE_1;
tlbe->entry.perm = IOMMU_NONE;
return -EINVAL;
}
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 9eb56a70f3..50e5a72d54 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -34,7 +34,8 @@
#include "smmuv3-internal.h"
#include "smmu-internal.h"
-#define PTW_RECORD_FAULT(cfg) (((cfg)->stage == 1) ? (cfg)->record_faults : \
+#define PTW_RECORD_FAULT(cfg) (((cfg)->stage == SMMU_STAGE_1) ? \
+ (cfg)->record_faults : \
(cfg)->s2cfg.record_faults)
/**
@@ -402,7 +403,7 @@ static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
{
- cfg->stage = 2;
+ cfg->stage = SMMU_STAGE_2;
if (STE_S2AA64(ste) == 0x0) {
qemu_log_mask(LOG_UNIMP,
@@ -678,7 +679,7 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
/* we support only those at the moment */
cfg->aa64 = true;
- cfg->stage = 1;
+ cfg->stage = SMMU_STAGE_1;
cfg->oas = oas2bits(CD_IPS(cd));
cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
@@ -762,7 +763,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
return ret;
}
- if (cfg->aborted || cfg->bypassed || (cfg->stage == 2)) {
+ if (cfg->aborted || cfg->bypassed || (cfg->stage == SMMU_STAGE_2)) {
return 0;
}
@@ -882,7 +883,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
goto epilogue;
}
- if (cfg->stage == 1) {
+ if (cfg->stage == SMMU_STAGE_1) {
/* Select stage1 translation table. */
tt = select_tt(cfg, addr);
if (!tt) {
@@ -919,7 +920,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
* nesting is not supported. So it is sufficient to check the
* translation stage to know the TLB stage for now.
*/
- event.u.f_walk_eabt.s2 = (cfg->stage == 2);
+ event.u.f_walk_eabt.s2 = (cfg->stage == SMMU_STAGE_2);
if (PTW_RECORD_FAULT(cfg)) {
event.type = SMMU_EVT_F_PERMISSION;
event.u.f_permission.addr = addr;
@@ -935,7 +936,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
if (smmu_ptw(cfg, aligned_addr, flag, cached_entry, &ptw_info)) {
/* All faults from PTW has S2 field. */
- event.u.f_walk_eabt.s2 = (ptw_info.stage == 2);
+ event.u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2);
g_free(cached_entry);
switch (ptw_info.type) {
case SMMU_PTW_ERR_WALK_EABT:
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 5ec2e6c1a4..b3c881f0ee 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -49,8 +49,15 @@ typedef enum {
SMMU_PTW_ERR_PERMISSION, /* Permission fault */
} SMMUPTWEventType;
+/* SMMU Stage */
+typedef enum {
+ SMMU_STAGE_1 = 1,
+ SMMU_STAGE_2,
+ SMMU_NESTED,
+} SMMUStage;
+
typedef struct SMMUPTWEventInfo {
- int stage;
+ SMMUStage stage;
SMMUPTWEventType type;
dma_addr_t addr; /* fetched address that induced an abort, if any */
} SMMUPTWEventInfo;
@@ -88,7 +95,7 @@ typedef struct SMMUS2Cfg {
*/
typedef struct SMMUTransCfg {
/* Shared fields between stage-1 and stage-2. */
- int stage; /* translation stage */
+ SMMUStage stage; /* translation stage */
bool disabled; /* smmu is disabled */
bool bypassed; /* translation is bypassed */
bool aborted; /* translation is aborted */
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 02/13] hw/arm/smmu: Split smmuv3_translate()
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 01/13] hw/arm/smmu: Use enum for SMMU stage Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 03/13] hw/arm/smmu: Consolidate ASID and VMID types Mostafa Saleh
` (11 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
smmuv3_translate() does everything from STE/CD parsing to TLB lookup
and PTW.
Soon, when nesting is supported, stage-1 data (tt, CD) needs to be
translated using stage-2.
Split smmuv3_translate() to 3 functions:
- smmu_translate(): in smmu-common.c, which does the TLB lookup, PTW,
TLB insertion, all the functions are already there, this just puts
them together.
This also simplifies the code as it consolidates event generation
in case of TLB lookup permission failure or in TT selection.
- smmuv3_do_translate(): in smmuv3.c, Calls smmu_translate() and does
the event population in case of errors.
- smmuv3_translate(), now calls smmuv3_do_translate() for
translation while the rest is the same.
Also, add stage in trace_smmuv3_translate_success()
Signed-off-by: Mostafa Saleh <smostafa@google.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
hw/arm/smmu-common.c | 59 ++++++++++++
hw/arm/smmuv3.c | 175 +++++++++++++----------------------
hw/arm/trace-events | 2 +-
include/hw/arm/smmu-common.h | 8 ++
4 files changed, 133 insertions(+), 111 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 3a7c350aca..20630eb670 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -554,6 +554,65 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
g_assert_not_reached();
}
+SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
+ IOMMUAccessFlags flag, SMMUPTWEventInfo *info)
+{
+ uint64_t page_mask, aligned_addr;
+ SMMUTLBEntry *cached_entry = NULL;
+ SMMUTransTableInfo *tt;
+ int status;
+
+ /*
+ * Combined attributes used for TLB lookup, as only one stage is supported,
+ * it will hold attributes based on the enabled stage.
+ */
+ SMMUTransTableInfo tt_combined;
+
+ if (cfg->stage == SMMU_STAGE_1) {
+ /* Select stage1 translation table. */
+ tt = select_tt(cfg, addr);
+ if (!tt) {
+ info->type = SMMU_PTW_ERR_TRANSLATION;
+ info->stage = SMMU_STAGE_1;
+ return NULL;
+ }
+ tt_combined.granule_sz = tt->granule_sz;
+ tt_combined.tsz = tt->tsz;
+
+ } else {
+ /* Stage2. */
+ tt_combined.granule_sz = cfg->s2cfg.granule_sz;
+ tt_combined.tsz = cfg->s2cfg.tsz;
+ }
+
+ /*
+ * TLB lookup looks for granule and input size for a translation stage,
+ * as only one stage is supported right now, choose the right values
+ * from the configuration.
+ */
+ page_mask = (1ULL << tt_combined.granule_sz) - 1;
+ aligned_addr = addr & ~page_mask;
+
+ cached_entry = smmu_iotlb_lookup(bs, cfg, &tt_combined, aligned_addr);
+ if (cached_entry) {
+ if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
+ info->type = SMMU_PTW_ERR_PERMISSION;
+ info->stage = cfg->stage;
+ return NULL;
+ }
+ return cached_entry;
+ }
+
+ cached_entry = g_new0(SMMUTLBEntry, 1);
+ status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info);
+ if (status) {
+ g_free(cached_entry);
+ return NULL;
+ }
+ smmu_iotlb_insert(bs, cfg, cached_entry);
+ return cached_entry;
+}
+
/**
* The bus number is used for lookup when SID based invalidation occurs.
* In that case we lazily populate the SMMUPciBus array from the bus hash
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 50e5a72d54..f081ff0cc4 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -827,6 +827,67 @@ static void smmuv3_flush_config(SMMUDevice *sdev)
g_hash_table_remove(bc->configs, sdev);
}
+/* Do translation with TLB lookup. */
+static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
+ SMMUTransCfg *cfg,
+ SMMUEventInfo *event,
+ IOMMUAccessFlags flag,
+ SMMUTLBEntry **out_entry)
+{
+ SMMUPTWEventInfo ptw_info = {};
+ SMMUState *bs = ARM_SMMU(s);
+ SMMUTLBEntry *cached_entry = NULL;
+
+ cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info);
+ if (!cached_entry) {
+ /* All faults from PTW has S2 field. */
+ event->u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2);
+ switch (ptw_info.type) {
+ case SMMU_PTW_ERR_WALK_EABT:
+ event->type = SMMU_EVT_F_WALK_EABT;
+ event->u.f_walk_eabt.addr = addr;
+ event->u.f_walk_eabt.rnw = flag & 0x1;
+ event->u.f_walk_eabt.class = 0x1;
+ event->u.f_walk_eabt.addr2 = ptw_info.addr;
+ break;
+ case SMMU_PTW_ERR_TRANSLATION:
+ if (PTW_RECORD_FAULT(cfg)) {
+ event->type = SMMU_EVT_F_TRANSLATION;
+ event->u.f_translation.addr = addr;
+ event->u.f_translation.rnw = flag & 0x1;
+ }
+ break;
+ case SMMU_PTW_ERR_ADDR_SIZE:
+ if (PTW_RECORD_FAULT(cfg)) {
+ event->type = SMMU_EVT_F_ADDR_SIZE;
+ event->u.f_addr_size.addr = addr;
+ event->u.f_addr_size.rnw = flag & 0x1;
+ }
+ break;
+ case SMMU_PTW_ERR_ACCESS:
+ if (PTW_RECORD_FAULT(cfg)) {
+ event->type = SMMU_EVT_F_ACCESS;
+ event->u.f_access.addr = addr;
+ event->u.f_access.rnw = flag & 0x1;
+ }
+ break;
+ case SMMU_PTW_ERR_PERMISSION:
+ if (PTW_RECORD_FAULT(cfg)) {
+ event->type = SMMU_EVT_F_PERMISSION;
+ event->u.f_permission.addr = addr;
+ event->u.f_permission.rnw = flag & 0x1;
+ }
+ break;
+ default:
+ g_assert_not_reached();
+ }
+ return SMMU_TRANS_ERROR;
+ }
+ *out_entry = cached_entry;
+ return SMMU_TRANS_SUCCESS;
+}
+
+/* Entry point to SMMU, does everything. */
static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
IOMMUAccessFlags flag, int iommu_idx)
{
@@ -836,12 +897,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
SMMUEventInfo event = {.type = SMMU_EVT_NONE,
.sid = sid,
.inval_ste_allowed = false};
- SMMUPTWEventInfo ptw_info = {};
SMMUTranslationStatus status;
- SMMUState *bs = ARM_SMMU(s);
- uint64_t page_mask, aligned_addr;
- SMMUTLBEntry *cached_entry = NULL;
- SMMUTransTableInfo *tt;
SMMUTransCfg *cfg = NULL;
IOMMUTLBEntry entry = {
.target_as = &address_space_memory,
@@ -850,11 +906,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
.addr_mask = ~(hwaddr)0,
.perm = IOMMU_NONE,
};
- /*
- * Combined attributes used for TLB lookup, as only one stage is supported,
- * it will hold attributes based on the enabled stage.
- */
- SMMUTransTableInfo tt_combined;
+ SMMUTLBEntry *cached_entry = NULL;
qemu_mutex_lock(&s->mutex);
@@ -883,105 +935,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
goto epilogue;
}
- if (cfg->stage == SMMU_STAGE_1) {
- /* Select stage1 translation table. */
- tt = select_tt(cfg, addr);
- if (!tt) {
- if (cfg->record_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;
- }
- tt_combined.granule_sz = tt->granule_sz;
- tt_combined.tsz = tt->tsz;
-
- } else {
- /* Stage2. */
- tt_combined.granule_sz = cfg->s2cfg.granule_sz;
- tt_combined.tsz = cfg->s2cfg.tsz;
- }
- /*
- * TLB lookup looks for granule and input size for a translation stage,
- * as only one stage is supported right now, choose the right values
- * from the configuration.
- */
- page_mask = (1ULL << tt_combined.granule_sz) - 1;
- aligned_addr = addr & ~page_mask;
-
- cached_entry = smmu_iotlb_lookup(bs, cfg, &tt_combined, aligned_addr);
- if (cached_entry) {
- if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
- status = SMMU_TRANS_ERROR;
- /*
- * We know that the TLB only contains either stage-1 or stage-2 as
- * nesting is not supported. So it is sufficient to check the
- * translation stage to know the TLB stage for now.
- */
- event.u.f_walk_eabt.s2 = (cfg->stage == SMMU_STAGE_2);
- if (PTW_RECORD_FAULT(cfg)) {
- 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;
- }
-
- cached_entry = g_new0(SMMUTLBEntry, 1);
-
- if (smmu_ptw(cfg, aligned_addr, flag, cached_entry, &ptw_info)) {
- /* All faults from PTW has S2 field. */
- event.u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2);
- g_free(cached_entry);
- switch (ptw_info.type) {
- case SMMU_PTW_ERR_WALK_EABT:
- event.type = SMMU_EVT_F_WALK_EABT;
- event.u.f_walk_eabt.addr = addr;
- event.u.f_walk_eabt.rnw = flag & 0x1;
- event.u.f_walk_eabt.class = 0x1;
- event.u.f_walk_eabt.addr2 = ptw_info.addr;
- break;
- case SMMU_PTW_ERR_TRANSLATION:
- if (PTW_RECORD_FAULT(cfg)) {
- event.type = SMMU_EVT_F_TRANSLATION;
- event.u.f_translation.addr = addr;
- event.u.f_translation.rnw = flag & 0x1;
- }
- break;
- case SMMU_PTW_ERR_ADDR_SIZE:
- if (PTW_RECORD_FAULT(cfg)) {
- event.type = SMMU_EVT_F_ADDR_SIZE;
- event.u.f_addr_size.addr = addr;
- event.u.f_addr_size.rnw = flag & 0x1;
- }
- break;
- case SMMU_PTW_ERR_ACCESS:
- if (PTW_RECORD_FAULT(cfg)) {
- event.type = SMMU_EVT_F_ACCESS;
- event.u.f_access.addr = addr;
- event.u.f_access.rnw = flag & 0x1;
- }
- break;
- case SMMU_PTW_ERR_PERMISSION:
- if (PTW_RECORD_FAULT(cfg)) {
- event.type = SMMU_EVT_F_PERMISSION;
- event.u.f_permission.addr = addr;
- event.u.f_permission.rnw = flag & 0x1;
- }
- break;
- default:
- g_assert_not_reached();
- }
- status = SMMU_TRANS_ERROR;
- } else {
- smmu_iotlb_insert(bs, cfg, cached_entry);
- status = SMMU_TRANS_SUCCESS;
- }
+ status = smmuv3_do_translate(s, addr, cfg, &event, flag, &cached_entry);
epilogue:
qemu_mutex_unlock(&s->mutex);
@@ -992,7 +946,8 @@ epilogue:
(addr & cached_entry->entry.addr_mask);
entry.addr_mask = cached_entry->entry.addr_mask;
trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
- entry.translated_addr, entry.perm);
+ entry.translated_addr, entry.perm,
+ cfg->stage);
break;
case SMMU_TRANS_DISABLE:
entry.perm = flag;
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index f1a54a02df..cc12924a84 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -37,7 +37,7 @@ smmuv3_get_ste(uint64_t addr) "STE addr: 0x%"PRIx64
smmuv3_translate_disable(const char *n, uint16_t sid, uint64_t addr, bool is_write) "%s sid=0x%x 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=0x%x 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=0x%x 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=0x%x iova=0x%"PRIx64" translated=0x%"PRIx64" perm=0x%x"
+smmuv3_translate_success(const char *n, uint16_t sid, uint64_t iova, uint64_t translated, int perm, int stage) "%s sid=0x%x iova=0x%"PRIx64" translated=0x%"PRIx64" perm=0x%x stage=%d"
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, bool had) "TT[%d]:tsz:%d ttb:0x%"PRIx64" granule_sz:%d had:%d"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index b3c881f0ee..5944735632 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -183,6 +183,14 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info);
+
+/*
+ * smmu_translate - Look for a translation in TLB, if not, do a PTW.
+ * Returns NULL on PTW error or incase of TLB permission errors.
+ */
+SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
+ IOMMUAccessFlags flag, SMMUPTWEventInfo *info);
+
/**
* select_tt - compute which translation table shall be used according to
* the input iova and translation config and return the TT specific info
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 03/13] hw/arm/smmu: Consolidate ASID and VMID types
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 01/13] hw/arm/smmu: Use enum for SMMU stage Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 02/13] hw/arm/smmu: Split smmuv3_translate() Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-18 7:56 ` Eric Auger
2024-04-08 14:08 ` [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table Mostafa Saleh
` (10 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
ASID and VMID used to be uint16_t in the translation config, however,
in other contexts they can be int as -1 in case of TLB invalidation,
to represent all(don’t care).
When stage-2 was added asid was set to -1 in stage-2 and vmid to -1
in stage-1 configs. However, that meant they were set as (65536),
this was not an issue as nesting was not supported and no
commands/lookup targets both.
With nesting, it’s critical to get this right as translation must be
tagged correctly with ASID/VMID, and with ASID=-1 meaning stage-2.
Represent ASID/VMID everywhere as int.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmu-common.c | 10 +++++-----
hw/arm/smmuv3.c | 4 ++--
include/hw/arm/smmu-common.h | 14 +++++++-------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 20630eb670..771b9c79a3 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -57,7 +57,7 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
(k1->vmid == k2->vmid);
}
-SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
+SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
uint8_t tg, uint8_t level)
{
SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova,
@@ -130,7 +130,7 @@ void smmu_iotlb_inv_all(SMMUState *s)
static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
gpointer user_data)
{
- uint16_t asid = *(uint16_t *)user_data;
+ int asid = *(int *)user_data;
SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
return SMMU_IOTLB_ASID(*iotlb_key) == asid;
@@ -139,7 +139,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
gpointer user_data)
{
- uint16_t vmid = *(uint16_t *)user_data;
+ int vmid = *(int *)user_data;
SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
@@ -191,13 +191,13 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
&info);
}
-void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
+void smmu_iotlb_inv_asid(SMMUState *s, int asid)
{
trace_smmu_iotlb_inv_asid(asid);
g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
}
-inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
+inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
{
trace_smmu_iotlb_inv_vmid(vmid);
g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index f081ff0cc4..897f8fe085 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1235,7 +1235,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
}
case SMMU_CMD_TLBI_NH_ASID:
{
- uint16_t asid = CMD_ASID(&cmd);
+ int asid = CMD_ASID(&cmd);
if (!STAGE1_SUPPORTED(s)) {
cmd_error = SMMU_CERROR_ILL;
@@ -1268,7 +1268,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
break;
case SMMU_CMD_TLBI_S12_VMALL:
{
- uint16_t vmid = CMD_VMID(&cmd);
+ int vmid = CMD_VMID(&cmd);
if (!STAGE2_SUPPORTED(s)) {
cmd_error = SMMU_CERROR_ILL;
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 5944735632..96eb017e50 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -84,7 +84,7 @@ typedef struct SMMUS2Cfg {
bool record_faults; /* Record fault events (S2R) */
uint8_t granule_sz; /* Granule page shift (based on S2TG) */
uint8_t eff_ps; /* Effective PA output range (based on S2PS) */
- uint16_t vmid; /* Virtual Machine ID (S2VMID) */
+ int vmid; /* Virtual Machine ID (S2VMID) */
uint64_t vttb; /* Address of translation table base (S2TTB) */
} SMMUS2Cfg;
@@ -108,7 +108,7 @@ typedef struct SMMUTransCfg {
uint64_t ttb; /* TT base address */
uint8_t oas; /* output address width */
uint8_t tbi; /* Top Byte Ignore */
- uint16_t asid;
+ int asid;
SMMUTransTableInfo tt[2];
/* Used by stage-2 only. */
struct SMMUS2Cfg s2cfg;
@@ -132,8 +132,8 @@ typedef struct SMMUPciBus {
typedef struct SMMUIOTLBKey {
uint64_t iova;
- uint16_t asid;
- uint16_t vmid;
+ int asid;
+ int vmid;
uint8_t tg;
uint8_t level;
} SMMUIOTLBKey;
@@ -205,11 +205,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
SMMUTransTableInfo *tt, hwaddr iova);
void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
-SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
+SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
uint8_t tg, uint8_t level);
void smmu_iotlb_inv_all(SMMUState *s);
-void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
-void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
+void smmu_iotlb_inv_asid(SMMUState *s, int asid);
+void smmu_iotlb_inv_vmid(SMMUState *s, int vmid);
void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (2 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 03/13] hw/arm/smmu: Consolidate ASID and VMID types Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-18 12:51 ` Eric Auger
2024-04-08 14:08 ` [RFC PATCH v2 05/13] hw/arm/smmu-common: Support nested translation Mostafa Saleh
` (9 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
According to the user manual (ARM IHI 0070 F.b),
In "5.2 Stream Table Entry":
[51:6] S1ContextPtr
If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by
stage 2 and the programmed value must be within the range of the IAS.
In "5.4.1 CD notes":
The translation table walks performed from TTB0 or TTB1 are always performed
in IPA space if stage 2 translations are enabled.
So translate both the CD and the TTBx in this patch if nested
translation is requested.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3.c | 49 ++++++++++++++++++++++++++++++------
include/hw/arm/smmu-common.h | 17 +++++++++++++
2 files changed, 59 insertions(+), 7 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 897f8fe085..a7cf543acc 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -337,14 +337,36 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
}
+static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
+ SMMUTransCfg *cfg,
+ SMMUEventInfo *event,
+ IOMMUAccessFlags flag,
+ SMMUTLBEntry **out_entry);
/* @ssid > 0 not supported yet */
-static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
- CD *buf, SMMUEventInfo *event)
+static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg,
+ uint32_t ssid, CD *buf, SMMUEventInfo *event)
{
dma_addr_t addr = STE_CTXPTR(ste);
int ret, i;
+ SMMUTranslationStatus status;
+ SMMUTLBEntry *entry;
trace_smmuv3_get_cd(addr);
+
+ if (cfg->stage == SMMU_NESTED) {
+ CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s, addr,
+ cfg, event, IOMMU_RO, &entry);
+ /*
+ * It is not clear what should happen if this fails, so we return here
+ * which gets propagated as a translation error.
+ */
+ if (status != SMMU_TRANS_SUCCESS) {
+ return -EINVAL;
+ }
+
+ addr = CACHED_ENTRY_TO_ADDR(entry, addr);
+ }
+
/* TODO: guarantee 64-bit single-copy atomicity */
ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf),
MEMTXATTRS_UNSPECIFIED);
@@ -659,10 +681,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
return 0;
}
-static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
+static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
+ CD *cd, SMMUEventInfo *event)
{
int ret = -EINVAL;
int i;
+ SMMUTranslationStatus status;
+ SMMUTLBEntry *entry;
if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
goto bad_cd;
@@ -713,6 +738,17 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
tt->tsz = tsz;
tt->ttb = CD_TTB(cd, i);
+
+ /* Translate the TTBx, from IPA to PA if nesting is enabled. */
+ if (cfg->stage == SMMU_NESTED) {
+ CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s,
+ tt->ttb, cfg, event, IOMMU_RO, &entry);
+ /* See smmu_get_cd(). */
+ if (status != SMMU_TRANS_SUCCESS) {
+ return -EINVAL;
+ }
+ tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb);
+ }
if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) {
goto bad_cd;
}
@@ -767,12 +803,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
return 0;
}
- ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event);
+ ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event);
if (ret) {
return ret;
}
- return decode_cd(cfg, &cd, event);
+ return decode_cd(s, cfg, &cd, event);
}
/**
@@ -942,8 +978,7 @@ epilogue:
switch (status) {
case SMMU_TRANS_SUCCESS:
entry.perm = cached_entry->entry.perm;
- entry.translated_addr = cached_entry->entry.translated_addr +
- (addr & cached_entry->entry.addr_mask);
+ entry.translated_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
entry.addr_mask = cached_entry->entry.addr_mask;
trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
entry.translated_addr, entry.perm,
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 96eb017e50..2772175115 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -37,6 +37,23 @@
#define VMSA_IDXMSK(isz, strd, lvl) ((1ULL << \
VMSA_BIT_LVL(isz, strd, lvl)) - 1)
+#define CACHED_ENTRY_TO_ADDR(ent, addr) (ent)->entry.translated_addr + \
+ ((addr) & (ent)->entry.addr_mask);
+
+/*
+ * From nested context, some functions might need to translate IPA addresses.
+ * As cfg has SMMU_NESTED, this won't work, this macro calls a function with
+ * making it a stage-2 cfg and then restore it after.
+ */
+#define CALL_FUNC_CFG_S2(cfg, ret, fn, ...) ({ \
+ int asid = cfg->asid; \
+ cfg->stage = SMMU_STAGE_2; \
+ cfg->asid = -1; \
+ ret = fn(__VA_ARGS__); \
+ cfg->asid = asid; \
+ cfg->stage = SMMU_NESTED; \
+ })
+
/*
* Page table walk error types
*/
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 05/13] hw/arm/smmu-common: Support nested translation
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (3 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-18 13:54 ` Eric Auger
2024-04-08 14:08 ` [RFC PATCH v2 06/13] hw/arm/smmu: Support nesting in smmuv3_range_inval() Mostafa Saleh
` (8 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
When nested translation is requested, do the following:
- Translate stage-1 IPA using stage-2 to a physical address.
- Translate stage-1 PTW walks using stage-2.
- Combine both to create a single TLB entry, for that we choose
the smallest entry to cache, which means that if the smallest
entry comes from stage-2, and stage-2 use different granule,
TLB lookup for stage-1 (in nested config) will always miss.
Lookup logic is modified for nesting to lookup using stage-2
granule if stage-1 granule missed and they are different.
Also, add more visibility in trace points, to make it easier to debug.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmu-common.c | 153 ++++++++++++++++++++++++++++-------
hw/arm/trace-events | 6 +-
include/hw/arm/smmu-common.h | 3 +-
3 files changed, 131 insertions(+), 31 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 771b9c79a3..2cf27b490b 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
return key;
}
-SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
- SMMUTransTableInfo *tt, hwaddr iova)
+static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs,
+ SMMUTransCfg *cfg,
+ SMMUTransTableInfo *tt,
+ hwaddr iova)
{
uint8_t tg = (tt->granule_sz - 10) / 2;
uint8_t inputsize = 64 - tt->tsz;
@@ -88,10 +90,29 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
}
level++;
}
+ return entry;
+}
+
+SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
+ SMMUTransTableInfo *tt, hwaddr iova)
+{
+ SMMUTLBEntry *entry = NULL;
+
+ entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova);
+ /*
+ * For nested translation also use the s2 granule, as the TLB will insert
+ * the smallest of both, so the entry can be cached with the s2 granule.
+ */
+ if (!entry && (cfg->stage == SMMU_NESTED) &&
+ (cfg->s2cfg.granule_sz != tt->granule_sz)) {
+ tt->granule_sz = cfg->s2cfg.granule_sz;
+ entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova);
+ }
if (entry) {
cfg->iotlb_hits++;
trace_smmu_iotlb_lookup_hit(cfg->asid, cfg->s2cfg.vmid, iova,
+ entry->entry.addr_mask,
cfg->iotlb_hits, cfg->iotlb_misses,
100 * cfg->iotlb_hits /
(cfg->iotlb_hits + cfg->iotlb_misses));
@@ -117,7 +138,7 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new)
*key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
tg, new->level);
trace_smmu_iotlb_insert(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
- tg, new->level);
+ tg, new->level, new->entry.translated_addr);
g_hash_table_insert(bs->iotlb, key, new);
}
@@ -286,6 +307,27 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
return NULL;
}
+/* Return the correct table address based on configuration. */
+static inline int translate_table_s1(dma_addr_t *table_addr, SMMUTransCfg *cfg,
+ SMMUPTWEventInfo *info, SMMUState *bs)
+{
+ dma_addr_t addr = *table_addr;
+ SMMUTLBEntry *cached_entry;
+
+ if (cfg->stage != SMMU_NESTED) {
+ return 0;
+ }
+
+ CALL_FUNC_CFG_S2(cfg, cached_entry, smmu_translate,
+ bs, cfg, addr, IOMMU_RO, info);
+
+ if (cached_entry) {
+ *table_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
+ return 0;
+ }
+ return -EINVAL;
+}
+
/**
* smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
* @cfg: translation config
@@ -301,7 +343,8 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
*/
static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
dma_addr_t iova, IOMMUAccessFlags perm,
- SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
+ SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info,
+ SMMUState *bs)
{
dma_addr_t baseaddr, indexmask;
SMMUStage stage = cfg->stage;
@@ -349,6 +392,10 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
goto error;
}
baseaddr = get_table_pte_address(pte, granule_sz);
+ /* In case of failure, retain stage-2 fault. */
+ if (translate_table_s1(&baseaddr, cfg, info, bs)) {
+ goto error_no_stage;
+ }
level++;
continue;
} else if (is_page_pte(pte, level)) {
@@ -384,7 +431,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
tlbe->entry.translated_addr = gpa;
tlbe->entry.iova = iova & ~mask;
tlbe->entry.addr_mask = mask;
- tlbe->entry.perm = PTE_AP_TO_PERM(ap);
+ tlbe->parent_perm = tlbe->entry.perm = PTE_AP_TO_PERM(ap);
tlbe->level = level;
tlbe->granule = granule_sz;
return 0;
@@ -393,6 +440,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
error:
info->stage = SMMU_STAGE_1;
+error_no_stage:
tlbe->entry.perm = IOMMU_NONE;
return -EINVAL;
}
@@ -505,7 +553,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
tlbe->entry.translated_addr = gpa;
tlbe->entry.iova = ipa & ~mask;
tlbe->entry.addr_mask = mask;
- tlbe->entry.perm = s2ap;
+ tlbe->parent_perm = tlbe->entry.perm = s2ap;
tlbe->level = level;
tlbe->granule = granule_sz;
return 0;
@@ -518,6 +566,28 @@ error:
return -EINVAL;
}
+/* Combine 2 TLB enteries and return in tlbe. */
+static void combine_tlb(SMMUTLBEntry *tlbe, SMMUTLBEntry *tlbe_s2,
+ dma_addr_t iova, SMMUTransCfg *cfg)
+{
+ if (cfg->stage == SMMU_NESTED) {
+ tlbe->entry.addr_mask = MIN(tlbe->entry.addr_mask,
+ tlbe_s2->entry.addr_mask);
+ tlbe->entry.translated_addr = CACHED_ENTRY_TO_ADDR(tlbe_s2,
+ tlbe->entry.translated_addr);
+
+ tlbe->granule = MIN(tlbe->granule, tlbe_s2->granule);
+ tlbe->level = MAX(tlbe->level, tlbe_s2->level);
+ tlbe->entry.iova = iova & ~tlbe->entry.addr_mask;
+ /* parent_perm has s2 perm while perm has s1 perm. */
+ tlbe->parent_perm = tlbe_s2->entry.perm;
+ return;
+ }
+
+ /* That was not nested, use the s2. */
+ memcpy(tlbe, tlbe_s2, sizeof(*tlbe));
+}
+
/**
* smmu_ptw - Walk the page tables for an IOVA, according to @cfg
*
@@ -530,28 +600,59 @@ error:
* return 0 on success
*/
int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
- SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
+ SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs)
{
- if (cfg->stage == SMMU_STAGE_1) {
- return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
- } else if (cfg->stage == SMMU_STAGE_2) {
- /*
- * If bypassing stage 1(or unimplemented), the input address is passed
- * directly to stage 2 as IPA. If the input address of a transaction
- * exceeds the size of the IAS, a stage 1 Address Size fault occurs.
- * For AA64, IAS = OAS according to (IHI 0070.E.a) "3.4 Address sizes"
- */
- if (iova >= (1ULL << cfg->oas)) {
- info->type = SMMU_PTW_ERR_ADDR_SIZE;
- info->stage = SMMU_STAGE_1;
- tlbe->entry.perm = IOMMU_NONE;
- return -EINVAL;
+ int ret = 0;
+ SMMUTLBEntry tlbe_s2;
+ dma_addr_t ipa = iova;
+
+ if (cfg->stage & SMMU_STAGE_1) {
+ ret = smmu_ptw_64_s1(cfg, iova, perm, tlbe, info, bs);
+ if (ret) {
+ return ret;
}
+ /* This is the IPA for next stage.*/
+ ipa = CACHED_ENTRY_TO_ADDR(tlbe, iova);
+ }
- return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
+ /*
+ * The address output from the translation causes a stage 1 Address Size
+ * fault if it exceeds the range of the effective IPA size for the given CD.
+ * If bypassing stage 1(or unimplemented), the input address is passed
+ * directly to stage 2 as IPA. If the input address of a transaction
+ * exceeds the size of the IAS, a stage 1 Address Size fault occurs.
+ * For AA64, IAS = OAS according to (IHI 0070.E.a) "3.4 Address sizes"
+ */
+ if (ipa >= (1ULL << cfg->oas)) {
+ info->type = SMMU_PTW_ERR_ADDR_SIZE;
+ info->stage = SMMU_STAGE_1;
+ tlbe->entry.perm = IOMMU_NONE;
+ return -EINVAL;
}
- g_assert_not_reached();
+ if (cfg->stage & SMMU_STAGE_2) {
+ ret = smmu_ptw_64_s2(cfg, ipa, perm, &tlbe_s2, info);
+ if (ret) {
+ return ret;
+ }
+ combine_tlb(tlbe, &tlbe_s2, iova, cfg);
+ }
+
+ return ret;
+}
+
+static int validate_tlb_entry(SMMUTLBEntry *cached_entry, IOMMUAccessFlags flag,
+ SMMUPTWEventInfo *info)
+{
+ if ((flag & IOMMU_WO) && !(cached_entry->entry.perm &
+ cached_entry->parent_perm & IOMMU_WO)) {
+ info->type = SMMU_PTW_ERR_PERMISSION;
+ info->stage = !(cached_entry->entry.perm & IOMMU_WO) ?
+ SMMU_STAGE_1 :
+ SMMU_STAGE_2;
+ return -EINVAL;
+ }
+ return 0;
}
SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
@@ -595,16 +696,14 @@ SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
cached_entry = smmu_iotlb_lookup(bs, cfg, &tt_combined, aligned_addr);
if (cached_entry) {
- if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
- info->type = SMMU_PTW_ERR_PERMISSION;
- info->stage = cfg->stage;
+ if (validate_tlb_entry(cached_entry, flag, info)) {
return NULL;
}
return cached_entry;
}
cached_entry = g_new0(SMMUTLBEntry, 1);
- status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info);
+ status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info, bs);
if (status) {
g_free(cached_entry);
return NULL;
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index cc12924a84..5f23f0b963 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -15,9 +15,9 @@ smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%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"
-smmu_iotlb_lookup_hit(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
-smmu_iotlb_lookup_miss(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
-smmu_iotlb_insert(uint16_t asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d"
+smmu_iotlb_lookup_hit(int asid, uint16_t vmid, uint64_t addr, uint64_t mask, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" mask=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
+smmu_iotlb_lookup_miss(int asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
+smmu_iotlb_insert(int asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level, uint64_t translate_addr) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d translate_addr=0x%"PRIx64
# 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)"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 2772175115..03ff0f02ba 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -91,6 +91,7 @@ typedef struct SMMUTLBEntry {
IOMMUTLBEntry entry;
uint8_t level;
uint8_t granule;
+ IOMMUAccessFlags parent_perm;
} SMMUTLBEntry;
/* Stage-2 configuration. */
@@ -198,7 +199,7 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
* pair, according to @cfg translation config
*/
int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
- SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info);
+ SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs);
/*
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 06/13] hw/arm/smmu: Support nesting in smmuv3_range_inval()
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (4 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 05/13] hw/arm/smmu-common: Support nested translation Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-18 14:46 ` Eric Auger
2024-04-08 14:08 ` [RFC PATCH v2 07/13] hw/arm/smmu: Support nesting in the rest of commands Mostafa Saleh
` (7 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
With nesting, we would need to invalidate IPAs without
over-invalidating stage-1 IOVAs. This can be done by
distinguishing IPAs in the TLBs by having ASID=-1.
To achieve that, rework the invalidation for IPAs to have a
separate function, while for IOVA invalidation ASID=-1 means
invalidate for all ASIDs.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmu-common.c | 47 ++++++++++++++++++++++++++++++++++++
hw/arm/smmuv3.c | 23 ++++++++++++------
hw/arm/trace-events | 2 +-
include/hw/arm/smmu-common.h | 3 ++-
4 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 2cf27b490b..8b9e59b24b 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -184,6 +184,25 @@ static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
((entry->iova & ~info->mask) == info->iova);
}
+static gboolean smmu_hash_remove_by_vmid_ipa(gpointer key, gpointer value,
+ gpointer user_data)
+{
+ SMMUTLBEntry *iter = (SMMUTLBEntry *)value;
+ IOMMUTLBEntry *entry = &iter->entry;
+ SMMUIOTLBPageInvInfo *info = (SMMUIOTLBPageInvInfo *)user_data;
+ SMMUIOTLBKey iotlb_key = *(SMMUIOTLBKey *)key;
+
+ /* This is a stage-1 address. */
+ if (info->asid >= 0) {
+ return false;
+ }
+ if (info->vmid != SMMU_IOTLB_VMID(iotlb_key)) {
+ return false;
+ }
+ return ((info->iova & ~entry->addr_mask) == entry->iova) ||
+ ((entry->iova & ~info->mask) == info->iova);
+}
+
void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl)
{
@@ -212,6 +231,34 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
&info);
}
+/*
+ * Similar to smmu_iotlb_inv_iova(), but for Stage-2, ASID is always -1,
+ * in Stage-1 invalidation ASID = -1, means don't care.
+ */
+void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
+ uint64_t num_pages, uint8_t ttl)
+{
+ uint8_t granule = tg ? tg * 2 + 10 : 12;
+ int asid = -1;
+
+ if (ttl && (num_pages == 1)) {
+ SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, ipa, tg, ttl);
+
+ if (g_hash_table_remove(s->iotlb, &key)) {
+ return;
+ }
+ }
+
+ SMMUIOTLBPageInvInfo info = {
+ .iova = ipa,
+ .vmid = vmid,
+ .mask = (num_pages * 1 << granule) - 1};
+
+ g_hash_table_foreach_remove(s->iotlb,
+ smmu_hash_remove_by_vmid_ipa,
+ &info);
+}
+
void smmu_iotlb_inv_asid(SMMUState *s, int asid)
{
trace_smmu_iotlb_inv_asid(asid);
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index a7cf543acc..17bbd43c13 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1095,7 +1095,7 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
}
}
-static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
+static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage)
{
dma_addr_t end, addr = CMD_ADDR(cmd);
uint8_t type = CMD_TYPE(cmd);
@@ -1120,9 +1120,13 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
}
if (!tg) {
- trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
+ trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf, stage);
smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1);
- smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
+ if (stage == SMMU_STAGE_1) {
+ smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
+ } else {
+ smmu_iotlb_inv_ipa(s, vmid, addr, tg, 1, ttl);
+ }
return;
}
@@ -1138,9 +1142,14 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
uint64_t mask = dma_aligned_pow2_mask(addr, end, 64);
num_pages = (mask + 1) >> granule;
- trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
+ trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages,
+ ttl, leaf, stage);
smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages);
- smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
+ if (stage == SMMU_STAGE_1) {
+ smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
+ } else {
+ smmu_iotlb_inv_ipa(s, vmid, addr, tg, num_pages, ttl);
+ }
addr += mask + 1;
}
}
@@ -1299,7 +1308,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
cmd_error = SMMU_CERROR_ILL;
break;
}
- smmuv3_range_inval(bs, &cmd);
+ smmuv3_range_inval(bs, &cmd, SMMU_STAGE_1);
break;
case SMMU_CMD_TLBI_S12_VMALL:
{
@@ -1324,7 +1333,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
* As currently only either s1 or s2 are supported
* we can reuse same function for s2.
*/
- smmuv3_range_inval(bs, &cmd);
+ smmuv3_range_inval(bs, &cmd, SMMU_STAGE_2);
break;
case SMMU_CMD_TLBI_EL3_ALL:
case SMMU_CMD_TLBI_EL3_VA:
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 5f23f0b963..f5c361d96e 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -46,7 +46,7 @@ smmuv3_cmdq_cfgi_ste_range(int start, int end) "start=0x%x - end=0x%x"
smmuv3_cmdq_cfgi_cd(uint32_t sid) "sid=0x%x"
smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache HIT for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache MISS for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
-smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
+smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf, int stage) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d stage=%d"
smmuv3_cmdq_tlbi_nh(void) ""
smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 03ff0f02ba..df166d8477 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -230,7 +230,8 @@ void smmu_iotlb_inv_asid(SMMUState *s, int asid);
void smmu_iotlb_inv_vmid(SMMUState *s, int vmid);
void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl);
-
+void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
+ uint64_t num_pages, uint8_t ttl);
/* Unmap the range of all the notifiers registered to any IOMMU mr */
void smmu_inv_notifiers_all(SMMUState *s);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 07/13] hw/arm/smmu: Support nesting in the rest of commands
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (5 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 06/13] hw/arm/smmu: Support nesting in smmuv3_range_inval() Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-18 14:48 ` Eric Auger
2024-04-08 14:08 ` [RFC PATCH v2 08/13] hw/arm/smmuv3: Support nested SMMUs in smmuv3_notify_iova() Mostafa Saleh
` (6 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
Some commands need rework for nesting, as they used to assume S1
and S2 are mutually exclusive:
- CMD_TLBI_NH_ASID: Consider VMID if stage-2 is supported
- CMD_TLBI_NH_ALL: Consider VMID if stage-2 is supported, otherwise
invalidate everything, this required a new vmid invalidation
function for stage-1 only (ASID >= 0)
Also, rework trace events to reflect the new implementation.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmu-common.c | 36 +++++++++++++++++++++++++++++-------
hw/arm/smmuv3.c | 31 +++++++++++++++++++++++++++++--
hw/arm/trace-events | 6 ++++--
include/hw/arm/smmu-common.h | 3 ++-
4 files changed, 64 insertions(+), 12 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 8b9e59b24b..b1cf1303c6 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -148,13 +148,14 @@ void smmu_iotlb_inv_all(SMMUState *s)
g_hash_table_remove_all(s->iotlb);
}
-static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
- gpointer user_data)
+static gboolean smmu_hash_remove_by_asid_vmid(gpointer key, gpointer value,
+ gpointer user_data)
{
- int asid = *(int *)user_data;
+ SMMUIOTLBPageInvInfo *info = (SMMUIOTLBPageInvInfo *)user_data;
SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
- return SMMU_IOTLB_ASID(*iotlb_key) == asid;
+ return (SMMU_IOTLB_ASID(*iotlb_key) == info->asid) &&
+ (SMMU_IOTLB_VMID(*iotlb_key) == info->vmid);
}
static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
@@ -166,6 +167,16 @@ static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
}
+static gboolean smmu_hash_remove_by_vmid_s1(gpointer key, gpointer value,
+ gpointer user_data)
+{
+ int vmid = *(int *)user_data;
+ SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
+
+ return (SMMU_IOTLB_VMID(*iotlb_key) == vmid) &&
+ (SMMU_IOTLB_ASID(*iotlb_key) >= 0);
+}
+
static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
gpointer user_data)
{
@@ -259,10 +270,15 @@ void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
&info);
}
-void smmu_iotlb_inv_asid(SMMUState *s, int asid)
+void smmu_iotlb_inv_asid_vmid(SMMUState *s, int asid, int vmid)
{
- trace_smmu_iotlb_inv_asid(asid);
- g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
+ SMMUIOTLBPageInvInfo info = {
+ .asid = asid,
+ .vmid = vmid,
+ };
+
+ trace_smmu_iotlb_inv_asid_vmid(asid, vmid);
+ g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid_vmid, &info);
}
inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
@@ -271,6 +287,12 @@ inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
}
+inline void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid)
+{
+ trace_smmu_iotlb_inv_vmid_s1(vmid);
+ g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid_s1, &vmid);
+}
+
/* VMSAv8-64 Translation */
/**
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 17bbd43c13..ece647b8bf 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1280,25 +1280,52 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
case SMMU_CMD_TLBI_NH_ASID:
{
int asid = CMD_ASID(&cmd);
+ int vmid = -1;
if (!STAGE1_SUPPORTED(s)) {
cmd_error = SMMU_CERROR_ILL;
break;
}
+ /*
+ * VMID is only matched when stage 2 is supported for the Security
+ * state corresponding to the command queue that the command was
+ * issued in.
+ * QEMU ignores the field by setting to -1, similarly to what STE
+ * decoding does. And invalidation commands ignore VMID < 0.
+ */
+ if (STAGE2_SUPPORTED(s)) {
+ vmid = CMD_VMID(&cmd);
+ }
+
trace_smmuv3_cmdq_tlbi_nh_asid(asid);
smmu_inv_notifiers_all(&s->smmu_state);
- smmu_iotlb_inv_asid(bs, asid);
+ smmu_iotlb_inv_asid_vmid(bs, asid, vmid);
break;
}
case SMMU_CMD_TLBI_NH_ALL:
+ {
+ int vmid = -1;
+
if (!STAGE1_SUPPORTED(s)) {
cmd_error = SMMU_CERROR_ILL;
break;
}
+
+ /*
+ * If stage-2 is supported, invalidate for this VMID only, otherwise
+ * invalidate the whole thing, see SMMU_CMD_TLBI_NH_ASID()
+ */
+ if (STAGE2_SUPPORTED(s)) {
+ vmid = CMD_VMID(&cmd);
+ trace_smmuv3_cmdq_tlbi_nh(vmid);
+ smmu_iotlb_inv_vmid_s1(bs, vmid);
+ break;
+ }
QEMU_FALLTHROUGH;
+ }
case SMMU_CMD_TLBI_NSNH_ALL:
- trace_smmuv3_cmdq_tlbi_nh();
+ trace_smmuv3_cmdq_tlbi_nsnh();
smmu_inv_notifiers_all(&s->smmu_state);
smmu_iotlb_inv_all(bs);
break;
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index f5c361d96e..2556f4721a 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -11,8 +11,9 @@ smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint6
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_inv_all(void) "IOTLB invalidate all"
-smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
+smmu_iotlb_inv_asid_vmid(int asid, uint16_t vmid) "IOTLB invalidate asid=%d vmid=%d"
smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%d"
+smmu_iotlb_inv_vmid_s1(uint16_t vmid) "IOTLB invalidate vmid=%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"
smmu_iotlb_lookup_hit(int asid, uint16_t vmid, uint64_t addr, uint64_t mask, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" mask=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
@@ -47,7 +48,8 @@ smmuv3_cmdq_cfgi_cd(uint32_t sid) "sid=0x%x"
smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache HIT for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache MISS for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf, int stage) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d stage=%d"
-smmuv3_cmdq_tlbi_nh(void) ""
+smmuv3_cmdq_tlbi_nh(int vmid) "vmid=%d"
+smmuv3_cmdq_tlbi_nsnh(void) ""
smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index df166d8477..67db30e85b 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -226,8 +226,9 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
uint8_t tg, uint8_t level);
void smmu_iotlb_inv_all(SMMUState *s);
-void smmu_iotlb_inv_asid(SMMUState *s, int asid);
+void smmu_iotlb_inv_asid_vmid(SMMUState *s, int asid, int vmid);
void smmu_iotlb_inv_vmid(SMMUState *s, int vmid);
+void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid);
void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl);
void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 08/13] hw/arm/smmuv3: Support nested SMMUs in smmuv3_notify_iova()
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (6 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 07/13] hw/arm/smmu: Support nesting in the rest of commands Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 09/13] hw/arm/smmuv3: Support and advertise nesting Mostafa Saleh
` (5 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs keep
the implementation, while only notify for stage-1 invalidation
in case of nesting.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3.c | 23 +++++++++++++++--------
hw/arm/trace-events | 2 +-
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index ece647b8bf..85b3ac6a9c 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1028,7 +1028,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
IOMMUNotifier *n,
int asid, int vmid,
dma_addr_t iova, uint8_t tg,
- uint64_t num_pages)
+ uint64_t num_pages, int stage)
{
SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
IOMMUTLBEvent event;
@@ -1052,14 +1052,21 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
return;
}
- if (STAGE1_SUPPORTED(s)) {
+ /*
+ * IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs
+ * keep the implementation, while only notify for stage-1
+ * invalidation in case of nesting.
+ */
+ if (stage == SMMU_STAGE_1) {
tt = select_tt(cfg, iova);
if (!tt) {
return;
}
granule = tt->granule_sz;
- } else {
+ } else if (!STAGE1_SUPPORTED(s)) {
granule = cfg->s2cfg.granule_sz;
+ } else {
+ return;
}
} else {
@@ -1078,7 +1085,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
/* invalidate an asid/vmid/iova range tuple in all mr's */
static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
dma_addr_t iova, uint8_t tg,
- uint64_t num_pages)
+ uint64_t num_pages, int stage)
{
SMMUDevice *sdev;
@@ -1087,10 +1094,10 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
IOMMUNotifier *n;
trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid,
- iova, tg, num_pages);
+ iova, tg, num_pages, stage);
IOMMU_NOTIFIER_FOREACH(n, mr) {
- smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages);
+ smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages, stage);
}
}
}
@@ -1121,7 +1128,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage)
if (!tg) {
trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf, stage);
- smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1);
+ smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1, stage);
if (stage == SMMU_STAGE_1) {
smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
} else {
@@ -1144,7 +1151,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage)
num_pages = (mask + 1) >> granule;
trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages,
ttl, leaf, stage);
- smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages);
+ smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages, stage);
if (stage == SMMU_STAGE_1) {
smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
} else {
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 2556f4721a..53b9d11feb 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -55,7 +55,7 @@ smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
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, uint16_t vmid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
+smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint16_t vmid, uint64_t iova, uint8_t tg, uint64_t num_pages, int stage) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" stage=%d"
# strongarm.c
strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 09/13] hw/arm/smmuv3: Support and advertise nesting
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (7 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 08/13] hw/arm/smmuv3: Support nested SMMUs in smmuv3_notify_iova() Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 10/13] hw/arm/smmuv3: Advertise S2FWB Mostafa Saleh
` (4 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
Everything is in place, add the last missing bits:
- Handle fault checking according to the actual PTW event and not the
the translation stage.
- Consolidate parsing of STE cfg and setting translation stage.
Advertise nesting if stage requested is "nested".
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3.c | 50 +++++++++++++++++++++++++++++++++----------------
1 file changed, 34 insertions(+), 16 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 85b3ac6a9c..da47411410 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -34,9 +34,10 @@
#include "smmuv3-internal.h"
#include "smmu-internal.h"
-#define PTW_RECORD_FAULT(cfg) (((cfg)->stage == SMMU_STAGE_1) ? \
- (cfg)->record_faults : \
- (cfg)->s2cfg.record_faults)
+#define PTW_RECORD_FAULT(ptw_info, cfg) (((ptw_info).stage == SMMU_STAGE_1 && \
+ (cfg)->record_faults) || \
+ ((ptw_info).stage == SMMU_STAGE_2 && \
+ (cfg)->s2cfg.record_faults))
/**
* smmuv3_trigger_irq - pulse @irq if enabled and update
@@ -260,6 +261,9 @@ static void smmuv3_init_regs(SMMUv3State *s)
/* Based on sys property, the stages supported in smmu will be advertised.*/
if (s->stage && !strcmp("2", s->stage)) {
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
+ } else if (s->stage && !strcmp("nested", s->stage)) {
+ s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
+ s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
} else {
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
}
@@ -425,8 +429,6 @@ static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
{
- cfg->stage = SMMU_STAGE_2;
-
if (STE_S2AA64(ste) == 0x0) {
qemu_log_mask(LOG_UNIMP,
"SMMUv3 AArch32 tables not supported\n");
@@ -509,6 +511,27 @@ bad_ste:
return -EINVAL;
}
+static void decode_ste_config(SMMUTransCfg *cfg, uint32_t config)
+{
+
+ if (STE_CFG_ABORT(config)) {
+ cfg->aborted = true;
+ return;
+ }
+ if (STE_CFG_BYPASS(config)) {
+ cfg->bypassed = true;
+ return;
+ }
+
+ if (STE_CFG_S1_ENABLED(config)) {
+ cfg->stage = SMMU_STAGE_1;
+ }
+
+ if (STE_CFG_S2_ENABLED(config)) {
+ cfg->stage |= SMMU_STAGE_2;
+ }
+}
+
/* Returns < 0 in case of invalid STE, 0 otherwise */
static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
STE *ste, SMMUEventInfo *event)
@@ -525,13 +548,9 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
config = STE_CONFIG(ste);
- if (STE_CFG_ABORT(config)) {
- cfg->aborted = true;
- return 0;
- }
+ decode_ste_config(cfg, config);
- if (STE_CFG_BYPASS(config)) {
- cfg->bypassed = true;
+ if (cfg->aborted || cfg->bypassed) {
return 0;
}
@@ -704,7 +723,6 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
/* we support only those at the moment */
cfg->aa64 = true;
- cfg->stage = SMMU_STAGE_1;
cfg->oas = oas2bits(CD_IPS(cd));
cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
@@ -887,28 +905,28 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
event->u.f_walk_eabt.addr2 = ptw_info.addr;
break;
case SMMU_PTW_ERR_TRANSLATION:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_TRANSLATION;
event->u.f_translation.addr = addr;
event->u.f_translation.rnw = flag & 0x1;
}
break;
case SMMU_PTW_ERR_ADDR_SIZE:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_ADDR_SIZE;
event->u.f_addr_size.addr = addr;
event->u.f_addr_size.rnw = flag & 0x1;
}
break;
case SMMU_PTW_ERR_ACCESS:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_ACCESS;
event->u.f_access.addr = addr;
event->u.f_access.rnw = flag & 0x1;
}
break;
case SMMU_PTW_ERR_PERMISSION:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_PERMISSION;
event->u.f_permission.addr = addr;
event->u.f_permission.rnw = flag & 0x1;
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 10/13] hw/arm/smmuv3: Advertise S2FWB
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (8 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 09/13] hw/arm/smmuv3: Support and advertise nesting Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 11/13] hw/arm/smmu: Refactor SMMU OAS Mostafa Saleh
` (3 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
QEMU doesn's support memory attributes, so FWB is NOP, this
might change in the future if memory attributre would be supported.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index da47411410..0e367c70ad 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -287,6 +287,14 @@ static void smmuv3_init_regs(SMMUv3State *s)
if (FIELD_EX32(s->idr[0], IDR0, S2P)) {
/* XNX is a stage-2-specific feature */
s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
+ if (FIELD_EX32(s->idr[0], IDR0, S1P)) {
+ /*
+ * QEMU doesn's support memory attributes, so FWB is NOP, this
+ * might change in the future if memory attributre would be
+ * supported.
+ */
+ s->idr[3] = FIELD_DP32(s->idr[3], IDR3, FWB, 1);
+ }
}
s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 11/13] hw/arm/smmu: Refactor SMMU OAS
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (9 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 10/13] hw/arm/smmuv3: Advertise S2FWB Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 12/13] hw/arm/smmuv3: Add property for OAS Mostafa Saleh
` (2 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
SMMUv3 OAS is hardcoded to 44 bits, for nested configurations that
can be a problem as stage-2 might be shared with the CPU which might
have different PARANGE, and according to SMMU manual ARM IHI 0070F.b:
6.3.6 SMMU_IDR5, OAS must match the system physical address size.
This patch doesn't change the SMMU OAS, but refactors the code to
make it easier to do that:
- Rely everywhere on IDR5 for reading OAS instead of using the macro so
it is easier just change IDR5 and it propagages correctly.
- Remove unused functions/macros: pa_range/MAX_PA
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmu-common.c | 7 ++++---
hw/arm/smmuv3-internal.h | 13 -------------
hw/arm/smmuv3.c | 35 ++++++++++++++++++++++++++++-------
3 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index b1cf1303c6..0710ee6b7d 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -430,7 +430,8 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
inputsize = 64 - tt->tsz;
level = 4 - (inputsize - 4) / stride;
indexmask = VMSA_IDXMSK(inputsize, stride, level);
- baseaddr = extract64(tt->ttb, 0, 48);
+
+ baseaddr = extract64(tt->ttb, 0, cfg->oas);
baseaddr &= ~indexmask;
while (level < VMSA_LEVELS) {
@@ -543,8 +544,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
* Get the ttb from concatenated structure.
* The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
*/
- uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
- idx * sizeof(uint64_t);
+ uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, cfg->s2cfg.eff_ps) +
+ (1 << stride) * idx * sizeof(uint64_t);
dma_addr_t indexmask = VMSA_IDXMSK(inputsize, stride, level);
baseaddr &= ~indexmask;
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index e4dd11e1e6..b0d7ad6da3 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -596,19 +596,6 @@ static inline int oas2bits(int oas_field)
return -1;
}
-static inline int pa_range(STE *ste)
-{
- int oas_field = MIN(STE_S2PS(ste), SMMU_IDR5_OAS);
-
- if (!STE_S2AA64(ste)) {
- return 40;
- }
-
- return oas2bits(oas_field);
-}
-
-#define MAX_PA(ste) ((1 << pa_range(ste)) - 1)
-
/* CD fields */
#define CD_VALID(x) extract32((x)->word[0], 31, 1)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 0e367c70ad..c377c05379 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -411,10 +411,10 @@ static bool s2t0sz_valid(SMMUTransCfg *cfg)
}
if (cfg->s2cfg.granule_sz == 16) {
- return (cfg->s2cfg.tsz >= 64 - oas2bits(SMMU_IDR5_OAS));
+ return (cfg->s2cfg.tsz >= 64 - cfg->s2cfg.eff_ps);
}
- return (cfg->s2cfg.tsz >= MAX(64 - oas2bits(SMMU_IDR5_OAS), 16));
+ return (cfg->s2cfg.tsz >= MAX(64 - cfg->s2cfg.eff_ps, 16));
}
/*
@@ -435,8 +435,11 @@ static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
return nr_concat <= VMSA_MAX_S2_CONCAT;
}
-static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
+static int decode_ste_s2_cfg(SMMUv3State *s, SMMUTransCfg *cfg,
+ STE *ste)
{
+ uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
+
if (STE_S2AA64(ste) == 0x0) {
qemu_log_mask(LOG_UNIMP,
"SMMUv3 AArch32 tables not supported\n");
@@ -469,7 +472,15 @@ static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
}
/* For AA64, The effective S2PS size is capped to the OAS. */
- cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
+ cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), oas));
+ /*
+ * For SMMUv3.1 and later, when OAS == IAS == 52, the stage 2 input
+ * range is further limited to 48 bits unless STE.S2TG indicates a
+ * 64KB granule.
+ */
+ if (cfg->s2cfg.granule_sz != 16) {
+ cfg->s2cfg.eff_ps = MIN(cfg->s2cfg.eff_ps, 48);
+ }
/*
* It is ILLEGAL for the address in S2TTB to be outside the range
* described by the effective S2PS value.
@@ -545,6 +556,7 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
STE *ste, SMMUEventInfo *event)
{
uint32_t config;
+ uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
int ret;
if (!STE_VALID(ste)) {
@@ -588,8 +600,8 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
* Stage-1 OAS defaults to OAS even if not enabled as it would be used
* in input address check for stage-2.
*/
- cfg->oas = oas2bits(SMMU_IDR5_OAS);
- ret = decode_ste_s2_cfg(cfg, ste);
+ cfg->oas = oas2bits(oas);
+ ret = decode_ste_s2_cfg(s, cfg, ste);
if (ret) {
goto bad_ste;
}
@@ -715,6 +727,7 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
int i;
SMMUTranslationStatus status;
SMMUTLBEntry *entry;
+ uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
goto bad_cd;
@@ -733,7 +746,7 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
cfg->aa64 = true;
cfg->oas = oas2bits(CD_IPS(cd));
- cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
+ cfg->oas = MIN(oas2bits(oas), cfg->oas);
cfg->tbi = CD_TBI(cd);
cfg->asid = CD_ASID(cd);
cfg->affd = CD_AFFD(cd);
@@ -762,6 +775,14 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
goto bad_cd;
}
+ /*
+ * An address greater than 48 bits in size can only be output from a
+ * TTD when, in SMMUv3.1 and later, the effective IPS is 52 and a 64KB
+ * granule is in use for that translation table
+ */
+ if (tt->granule_sz != 16) {
+ cfg->oas = MIN(cfg->oas, 48);
+ }
tt->tsz = tsz;
tt->ttb = CD_TTB(cd, i);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 12/13] hw/arm/smmuv3: Add property for OAS
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (10 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 11/13] hw/arm/smmu: Refactor SMMU OAS Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 13/13] hw/arm/virt: Set SMMU OAS based on CPU PARANGE Mostafa Saleh
2024-04-18 18:11 ` [RFC PATCH v2 00/13] SMMUv3 nested translation support Eric Auger
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
Add property that sets the OAS of the SMMU, this in not used in this
patch.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3-internal.h | 3 ++-
hw/arm/smmuv3.c | 29 ++++++++++++++++++++++++++++-
include/hw/arm/smmuv3.h | 1 +
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index b0d7ad6da3..41612bb9ff 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -105,7 +105,8 @@ REG32(IDR5, 0x14)
FIELD(IDR5, VAX, 10, 2);
FIELD(IDR5, STALL_MAX, 16, 16);
-#define SMMU_IDR5_OAS 4
+#define SMMU_IDR5_OAS_DEF 4 /* 44 bits. */
+#define SMMU_IDR5_OAS_MAX 5 /* 48 bits. */
REG32(IIDR, 0x18)
REG32(AIDR, 0x1c)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index c377c05379..a9e35c41b7 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -299,7 +299,9 @@ static void smmuv3_init_regs(SMMUv3State *s)
s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
- s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
+ /* PTW doesn't support 52 bits. */
+ s->oas = MIN(s->oas, SMMU_IDR5_OAS_MAX);
+ s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, s->oas);
/* 4K, 16K and 64K granule support */
s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
@@ -1878,11 +1880,34 @@ static const VMStateDescription vmstate_gbpa = {
}
};
+static const VMStateDescription vmstate_oas = {
+ .name = "smmuv3/oas",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (const VMStateField[]) {
+ VMSTATE_INT32(oas, SMMUv3State),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static int smmuv3_preload(void *opaque)
+{
+ SMMUv3State *s = opaque;
+
+ /*
+ * In case it wasn't migrated, use the value used
+ * by older QEMU.
+ */
+ s->oas = SMMU_IDR5_OAS_DEF;
+ return 0;
+}
+
static const VMStateDescription vmstate_smmuv3 = {
.name = "smmuv3",
.version_id = 1,
.minimum_version_id = 1,
.priority = MIG_PRI_IOMMU,
+ .pre_load = smmuv3_preload,
.fields = (const VMStateField[]) {
VMSTATE_UINT32(features, SMMUv3State),
VMSTATE_UINT8(sid_size, SMMUv3State),
@@ -1910,6 +1935,7 @@ static const VMStateDescription vmstate_smmuv3 = {
},
.subsections = (const VMStateDescription * const []) {
&vmstate_gbpa,
+ &vmstate_oas,
NULL
}
};
@@ -1922,6 +1948,7 @@ static Property smmuv3_properties[] = {
* Defaults to stage 1
*/
DEFINE_PROP_STRING("stage", SMMUv3State, stage),
+ DEFINE_PROP_INT32("oas", SMMUv3State, oas, SMMU_IDR5_OAS_DEF),
DEFINE_PROP_END_OF_LIST()
};
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index d183a62766..00a9eb4467 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -63,6 +63,7 @@ struct SMMUv3State {
qemu_irq irq[4];
QemuMutex mutex;
char *stage;
+ int32_t oas;
};
typedef enum {
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH v2 13/13] hw/arm/virt: Set SMMU OAS based on CPU PARANGE
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (11 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 12/13] hw/arm/smmuv3: Add property for OAS Mostafa Saleh
@ 2024-04-08 14:08 ` Mostafa Saleh
2024-04-18 18:11 ` [RFC PATCH v2 00/13] SMMUv3 nested translation support Eric Auger
13 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-08 14:08 UTC (permalink / raw)
To: qemu-arm, eric.auger, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz, Mostafa Saleh
Use the new SMMU property to make the SMMU OAS match the CPU PARANGE.
That's according to SMMU manual ARM IHI 0070F.b:
6.3.6 SMMU_IDR5, OAS must match the system physical address size.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/virt.c | 14 ++++++++++++--
target/arm/cpu.h | 2 ++
target/arm/cpu64.c | 5 +++++
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0af1943697..599c0f752b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -235,6 +235,13 @@ static bool ns_el2_virt_timer_present(void)
arm_feature(env, ARM_FEATURE_EL2) && cpu_isar_feature(aa64_vh, cpu);
}
+/* We rely on CPU to define system OAS. */
+static int32_t get_system_oas(void)
+{
+ ARMCPU *cpu = ARM_CPU(qemu_get_cpu(0));
+ return cpu_arm_get_oas(cpu);
+}
+
static void create_fdt(VirtMachineState *vms)
{
MachineState *ms = MACHINE(vms);
@@ -1340,7 +1347,7 @@ static void create_pcie_irq_map(const MachineState *ms,
}
static void create_smmu(const VirtMachineState *vms,
- PCIBus *bus)
+ PCIBus *bus, int32_t oas)
{
char *node;
const char compat[] = "arm,smmu-v3";
@@ -1360,6 +1367,9 @@ static void create_smmu(const VirtMachineState *vms,
object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
&error_abort);
+
+ qdev_prop_set_uint64(dev, "oas", oas);
+
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
for (i = 0; i < NUM_SMMU_IRQS; i++) {
@@ -1534,7 +1544,7 @@ static void create_pcie(VirtMachineState *vms)
switch (vms->iommu) {
case VIRT_IOMMU_SMMUV3:
- create_smmu(vms, vms->bus);
+ create_smmu(vms, vms->bus, get_system_oas());
qemu_fdt_setprop_cells(ms->fdt, nodename, "iommu-map",
0x0, vms->iommu_phandle, 0x0, 0x10000);
break;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a5b3d8f7da..14ef1a9d37 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3408,4 +3408,6 @@ static inline target_ulong cpu_untagged_addr(CPUState *cs, target_ulong x)
}
#endif
+int32_t cpu_arm_get_oas(ARMCPU *cpu);
+
#endif
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 985b1efe16..08da83c082 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -787,6 +787,11 @@ static const gchar *aarch64_gdb_arch_name(CPUState *cs)
return "aarch64";
}
+int32_t cpu_arm_get_oas(ARMCPU *cpu)
+{
+ return FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
+}
+
static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
{
CPUClass *cc = CPU_CLASS(oc);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 03/13] hw/arm/smmu: Consolidate ASID and VMID types
2024-04-08 14:08 ` [RFC PATCH v2 03/13] hw/arm/smmu: Consolidate ASID and VMID types Mostafa Saleh
@ 2024-04-18 7:56 ` Eric Auger
0 siblings, 0 replies; 24+ messages in thread
From: Eric Auger @ 2024-04-18 7:56 UTC (permalink / raw)
To: Mostafa Saleh, qemu-arm, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz
Hi Mostafa,
On 4/8/24 16:08, Mostafa Saleh wrote:
> ASID and VMID used to be uint16_t in the translation config, however,
> in other contexts they can be int as -1 in case of TLB invalidation,
> to represent all(don’t care).
> When stage-2 was added asid was set to -1 in stage-2 and vmid to -1
> in stage-1 configs. However, that meant they were set as (65536),
> this was not an issue as nesting was not supported and no
> commands/lookup targets both.
>
> With nesting, it’s critical to get this right as translation must be
> tagged correctly with ASID/VMID, and with ASID=-1 meaning stage-2.
> Represent ASID/VMID everywhere as int.
small conflict due to
0b796f3810 hw/arm/smmu: Avoid using inlined functions with external
linkage again
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmu-common.c | 10 +++++-----
> hw/arm/smmuv3.c | 4 ++--
> include/hw/arm/smmu-common.h | 14 +++++++-------
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 20630eb670..771b9c79a3 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -57,7 +57,7 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
> (k1->vmid == k2->vmid);
> }
>
> -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
> +SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
> uint8_t tg, uint8_t level)
> {
> SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova,
> @@ -130,7 +130,7 @@ void smmu_iotlb_inv_all(SMMUState *s)
> static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
> gpointer user_data)
> {
> - uint16_t asid = *(uint16_t *)user_data;
> + int asid = *(int *)user_data;
> SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
>
> return SMMU_IOTLB_ASID(*iotlb_key) == asid;
> @@ -139,7 +139,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
> static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> gpointer user_data)
> {
> - uint16_t vmid = *(uint16_t *)user_data;
> + int vmid = *(int *)user_data;
> SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
>
> return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
> @@ -191,13 +191,13 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> &info);
> }
>
> -void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
> +void smmu_iotlb_inv_asid(SMMUState *s, int asid)
> {
> trace_smmu_iotlb_inv_asid(asid);
> g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
> }
>
> -inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
> +inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
> {
> trace_smmu_iotlb_inv_vmid(vmid);
> g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index f081ff0cc4..897f8fe085 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1235,7 +1235,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> }
> case SMMU_CMD_TLBI_NH_ASID:
> {
> - uint16_t asid = CMD_ASID(&cmd);
> + int asid = CMD_ASID(&cmd);
>
> if (!STAGE1_SUPPORTED(s)) {
> cmd_error = SMMU_CERROR_ILL;
> @@ -1268,7 +1268,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> break;
> case SMMU_CMD_TLBI_S12_VMALL:
> {
> - uint16_t vmid = CMD_VMID(&cmd);
> + int vmid = CMD_VMID(&cmd);
>
> if (!STAGE2_SUPPORTED(s)) {
> cmd_error = SMMU_CERROR_ILL;
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 5944735632..96eb017e50 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -84,7 +84,7 @@ typedef struct SMMUS2Cfg {
> bool record_faults; /* Record fault events (S2R) */
> uint8_t granule_sz; /* Granule page shift (based on S2TG) */
> uint8_t eff_ps; /* Effective PA output range (based on S2PS) */
> - uint16_t vmid; /* Virtual Machine ID (S2VMID) */
> + int vmid; /* Virtual Machine ID (S2VMID) */
> uint64_t vttb; /* Address of translation table base (S2TTB) */
> } SMMUS2Cfg;
>
> @@ -108,7 +108,7 @@ typedef struct SMMUTransCfg {
> uint64_t ttb; /* TT base address */
> uint8_t oas; /* output address width */
> uint8_t tbi; /* Top Byte Ignore */
> - uint16_t asid;
> + int asid;
> SMMUTransTableInfo tt[2];
> /* Used by stage-2 only. */
> struct SMMUS2Cfg s2cfg;
> @@ -132,8 +132,8 @@ typedef struct SMMUPciBus {
>
> typedef struct SMMUIOTLBKey {
> uint64_t iova;
> - uint16_t asid;
> - uint16_t vmid;
> + int asid;
> + int vmid;
> uint8_t tg;
> uint8_t level;
> } SMMUIOTLBKey;
> @@ -205,11 +205,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
> SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> SMMUTransTableInfo *tt, hwaddr iova);
> void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
> -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
> +SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
> uint8_t tg, uint8_t level);
> void smmu_iotlb_inv_all(SMMUState *s);
> -void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
> -void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
> +void smmu_iotlb_inv_asid(SMMUState *s, int asid);
> +void smmu_iotlb_inv_vmid(SMMUState *s, int vmid);
> void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> uint8_t tg, uint64_t num_pages, uint8_t ttl);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table
2024-04-08 14:08 ` [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table Mostafa Saleh
@ 2024-04-18 12:51 ` Eric Auger
2024-04-19 10:23 ` Mostafa Saleh
0 siblings, 1 reply; 24+ messages in thread
From: Eric Auger @ 2024-04-18 12:51 UTC (permalink / raw)
To: Mostafa Saleh, qemu-arm, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz
Hi Mostafa,
On 4/8/24 16:08, Mostafa Saleh wrote:
> According to the user manual (ARM IHI 0070 F.b),
s/user manual/ARM SMMU architecture specification
> In "5.2 Stream Table Entry":
> [51:6] S1ContextPtr
> If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by
> stage 2 and the programmed value must be within the range of the IAS.
>
> In "5.4.1 CD notes":
> The translation table walks performed from TTB0 or TTB1 are always performed
> in IPA space if stage 2 translations are enabled.
>
> So translate both the CD and the TTBx in this patch if nested
translate the S1 context descriptor pointer and TTBx base addresses
through the S2 stage (IPA -> PA)
You may describe what you put in place to do the translation in the
commit msg, new functions, macro, ...
> translation is requested.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmuv3.c | 49 ++++++++++++++++++++++++++++++------
> include/hw/arm/smmu-common.h | 17 +++++++++++++
> 2 files changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 897f8fe085..a7cf543acc 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -337,14 +337,36 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
>
> }
>
> +static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> + SMMUTransCfg *cfg,
> + SMMUEventInfo *event,
> + IOMMUAccessFlags flag,
> + SMMUTLBEntry **out_entry);
> /* @ssid > 0 not supported yet */
> -static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
> - CD *buf, SMMUEventInfo *event)
> +static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg,
> + uint32_t ssid, CD *buf, SMMUEventInfo *event)
> {
> dma_addr_t addr = STE_CTXPTR(ste);
> int ret, i;
> + SMMUTranslationStatus status;
> + SMMUTLBEntry *entry;
>
> trace_smmuv3_get_cd(addr);
> +
> + if (cfg->stage == SMMU_NESTED) {
> + CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s, addr,
> + cfg, event, IOMMU_RO, &entry);
the fact we pass 2 times cfg looks pretty weird from a caller pov. See
my comment below.
do we somewhere check addr is within the proper addr range, IAS if S2,
OAS if S1. This was missing for S1 but I think it is worth improving now.
see 3.4.3
> + /*
> + * It is not clear what should happen if this fails, so we return here
> + * which gets propagated as a translation error.
but the error event might be different, no?
> + */
> + if (status != SMMU_TRANS_SUCCESS) {
> + return -EINVAL;
> + }
> +
> + addr = CACHED_ENTRY_TO_ADDR(entry, addr);
> + }
> +
> /* TODO: guarantee 64-bit single-copy atomicity */
> ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf),
> MEMTXATTRS_UNSPECIFIED);
> @@ -659,10 +681,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
> return 0;
> }
>
> -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
> + CD *cd, SMMUEventInfo *event)
> {
> int ret = -EINVAL;
> int i;
> + SMMUTranslationStatus status;
> + SMMUTLBEntry *entry;
>
> if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
> goto bad_cd;
> @@ -713,6 +738,17 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
>
> tt->tsz = tsz;
> tt->ttb = CD_TTB(cd, i);
> +
> + /* Translate the TTBx, from IPA to PA if nesting is enabled. */
> + if (cfg->stage == SMMU_NESTED) {
> + CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s,
> + tt->ttb, cfg, event, IOMMU_RO, &entry);
> + /* See smmu_get_cd(). */
ditto
> + if (status != SMMU_TRANS_SUCCESS) {
> + return -EINVAL;
> + }
> + tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb);
> + }
> if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) {
> goto bad_cd;
> }
> @@ -767,12 +803,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
> return 0;
> }
>
> - ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event);
> + ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event);
> if (ret) {
> return ret;
> }
>
> - return decode_cd(cfg, &cd, event);
> + return decode_cd(s, cfg, &cd, event);
> }
>
> /**
> @@ -942,8 +978,7 @@ epilogue:
> switch (status) {
> case SMMU_TRANS_SUCCESS:
> entry.perm = cached_entry->entry.perm;
> - entry.translated_addr = cached_entry->entry.translated_addr +
> - (addr & cached_entry->entry.addr_mask);
> + entry.translated_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
> entry.addr_mask = cached_entry->entry.addr_mask;
> trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
> entry.translated_addr, entry.perm,
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 96eb017e50..2772175115 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -37,6 +37,23 @@
> #define VMSA_IDXMSK(isz, strd, lvl) ((1ULL << \
> VMSA_BIT_LVL(isz, strd, lvl)) - 1)
>
> +#define CACHED_ENTRY_TO_ADDR(ent, addr) (ent)->entry.translated_addr + \
> + ((addr) & (ent)->entry.addr_mask);
> +
> +/*
> + * From nested context, some functions might need to translate IPA addresses.
> + * As cfg has SMMU_NESTED, this won't work, this macro calls a function with
> + * making it a stage-2 cfg and then restore it after.
> + */
> +#define CALL_FUNC_CFG_S2(cfg, ret, fn, ...) ({ \
> + int asid = cfg->asid; \
> + cfg->stage = SMMU_STAGE_2; \
> + cfg->asid = -1; \
> + ret = fn(__VA_ARGS__); \
At this stage of the reading this is not obvious why you need fn()
parameter, can't you simply call
smmuv3_do_translate(). If this is useful at some point in the series, you shall document that in the commit msg.
Also I think I would prefer a proper function instead of this macro.
Besides, can't we add an extra parameter to the translate function forcing the S2_only translation although the cfg is a nested one. I think this would make things clearer
> + cfg->asid = asid; \
> + cfg->stage = SMMU_NESTED; \
> + })
> +
> /*
> * Page table walk error types
> */
Thanks
Eric
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 05/13] hw/arm/smmu-common: Support nested translation
2024-04-08 14:08 ` [RFC PATCH v2 05/13] hw/arm/smmu-common: Support nested translation Mostafa Saleh
@ 2024-04-18 13:54 ` Eric Auger
2024-04-19 10:54 ` Mostafa Saleh
0 siblings, 1 reply; 24+ messages in thread
From: Eric Auger @ 2024-04-18 13:54 UTC (permalink / raw)
To: Mostafa Saleh, qemu-arm, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz
Hi Mostafa,
On 4/8/24 16:08, Mostafa Saleh wrote:
> When nested translation is requested, do the following:
>
> - Translate stage-1 IPA using stage-2 to a physical address.
> - Translate stage-1 PTW walks using stage-2.
> - Combine both to create a single TLB entry, for that we choose
> the smallest entry to cache, which means that if the smallest
> entry comes from stage-2, and stage-2 use different granule,
> TLB lookup for stage-1 (in nested config) will always miss.
> Lookup logic is modified for nesting to lookup using stage-2
> granule if stage-1 granule missed and they are different.
>
> Also, add more visibility in trace points, to make it easier to debug.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmu-common.c | 153 ++++++++++++++++++++++++++++-------
> hw/arm/trace-events | 6 +-
> include/hw/arm/smmu-common.h | 3 +-
> 3 files changed, 131 insertions(+), 31 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 771b9c79a3..2cf27b490b 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
> return key;
> }
>
> -SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> - SMMUTransTableInfo *tt, hwaddr iova)
> +static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs,
> + SMMUTransCfg *cfg,
> + SMMUTransTableInfo *tt,
> + hwaddr iova)
this helper can be introduced in a separate patch to ease the code review
> {
> uint8_t tg = (tt->granule_sz - 10) / 2;
> uint8_t inputsize = 64 - tt->tsz;
> @@ -88,10 +90,29 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> }
> level++;
> }
> + return entry;
> +}
> +
> +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> + SMMUTransTableInfo *tt, hwaddr iova)
> +{
> + SMMUTLBEntry *entry = NULL;
> +
> + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova);
> + /*
> + * For nested translation also use the s2 granule, as the TLB will insert
> + * the smallest of both, so the entry can be cached with the s2 granule.
> + */
> + if (!entry && (cfg->stage == SMMU_NESTED) &&
> + (cfg->s2cfg.granule_sz != tt->granule_sz)) {
> + tt->granule_sz = cfg->s2cfg.granule_sz;
> + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova);
this is also the kind of stuff that can be introduced and reviewed
separately without tkaing any risk until NESTED is not support. In this
new patch you could also document the TLB strategy.
> + }
>
> if (entry) {
> cfg->iotlb_hits++;
> trace_smmu_iotlb_lookup_hit(cfg->asid, cfg->s2cfg.vmid, iova,
> + entry->entry.addr_mask,
can be moved to a separate fix. same for the trace point changes
> cfg->iotlb_hits, cfg->iotlb_misses,
> 100 * cfg->iotlb_hits /
> (cfg->iotlb_hits + cfg->iotlb_misses));
> @@ -117,7 +138,7 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new)
> *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> tg, new->level);
> trace_smmu_iotlb_insert(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> - tg, new->level);
> + tg, new->level, new->entry.translated_addr);
> g_hash_table_insert(bs->iotlb, key, new);
> }
>
> @@ -286,6 +307,27 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> return NULL;
> }
>
> +/* Return the correct table address based on configuration. */
does the S2 translation for a PTE table?
> +static inline int translate_table_s1(dma_addr_t *table_addr, SMMUTransCfg *cfg,
> + SMMUPTWEventInfo *info, SMMUState *bs)
> +{
> + dma_addr_t addr = *table_addr;
> + SMMUTLBEntry *cached_entry;
> +
> + if (cfg->stage != SMMU_NESTED) {
> + return 0;
> + }
> +
> + CALL_FUNC_CFG_S2(cfg, cached_entry, smmu_translate,
> + bs, cfg, addr, IOMMU_RO, info);
> +
> + if (cached_entry) {
> + *table_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
> + return 0;
> + }
> + return -EINVAL;
> +}
> +
> /**
> * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
> * @cfg: translation config
> @@ -301,7 +343,8 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> */
> static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> dma_addr_t iova, IOMMUAccessFlags perm,
> - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info,
> + SMMUState *bs)
> {
> dma_addr_t baseaddr, indexmask;
> SMMUStage stage = cfg->stage;
> @@ -349,6 +392,10 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> goto error;
> }
> baseaddr = get_table_pte_address(pte, granule_sz);
> + /* In case of failure, retain stage-2 fault. */
link to the doc?
> + if (translate_table_s1(&baseaddr, cfg, info, bs)) {
let's avoid that call if S1 only
What about the error handling. I am not sure we are covering all the
cases listed in 7.3.12 F_WALK_EABT for instance?
> + goto error_no_stage;
> + }
> level++;
> continue;
> } else if (is_page_pte(pte, level)) {
> @@ -384,7 +431,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> tlbe->entry.translated_addr = gpa;
> tlbe->entry.iova = iova & ~mask;
> tlbe->entry.addr_mask = mask;
> - tlbe->entry.perm = PTE_AP_TO_PERM(ap);
> + tlbe->parent_perm = tlbe->entry.perm = PTE_AP_TO_PERM(ap);
> tlbe->level = level;
> tlbe->granule = granule_sz;
> return 0;
> @@ -393,6 +440,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>
> error:
> info->stage = SMMU_STAGE_1;
> +error_no_stage:
> tlbe->entry.perm = IOMMU_NONE;
> return -EINVAL;
> }
> @@ -505,7 +553,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> tlbe->entry.translated_addr = gpa;
> tlbe->entry.iova = ipa & ~mask;
> tlbe->entry.addr_mask = mask;
> - tlbe->entry.perm = s2ap;
> + tlbe->parent_perm = tlbe->entry.perm = s2ap;
> tlbe->level = level;
> tlbe->granule = granule_sz;
> return 0;
> @@ -518,6 +566,28 @@ error:
> return -EINVAL;
> }
>
> +/* Combine 2 TLB enteries and return in tlbe. */
entries.
Would suggest combine
> +static void combine_tlb(SMMUTLBEntry *tlbe, SMMUTLBEntry *tlbe_s2,
> + dma_addr_t iova, SMMUTransCfg *cfg)
> +{
> + if (cfg->stage == SMMU_NESTED) {
> + tlbe->entry.addr_mask = MIN(tlbe->entry.addr_mask,
> + tlbe_s2->entry.addr_mask);
> + tlbe->entry.translated_addr = CACHED_ENTRY_TO_ADDR(tlbe_s2,
> + tlbe->entry.translated_addr);
> +
> + tlbe->granule = MIN(tlbe->granule, tlbe_s2->granule);
could you add a link to the spec so we can clearly check what this code
is written against?
> + tlbe->level = MAX(tlbe->level, tlbe_s2->level);
> + tlbe->entry.iova = iova & ~tlbe->entry.addr_mask;
> + /* parent_perm has s2 perm while perm has s1 perm. */
> + tlbe->parent_perm = tlbe_s2->entry.perm;
> + return;
> + }
> +
> + /* That was not nested, use the s2. */
should be removed and tested ;-)
> + memcpy(tlbe, tlbe_s2, sizeof(*tlbe));
You shall avoid doing that memcpy if not necessary. I would advocate for
separate paths for S2 and nested.
> +}
> +
> /**
> * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
need to update the above args desc
> *
> @@ -530,28 +600,59 @@ error:
> * return 0 on success
> */
> int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs)
> {
> - if (cfg->stage == SMMU_STAGE_1) {
> - return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> - } else if (cfg->stage == SMMU_STAGE_2) {
> - /*
> - * If bypassing stage 1(or unimplemented), the input address is passed
> - * directly to stage 2 as IPA. If the input address of a transaction
> - * exceeds the size of the IAS, a stage 1 Address Size fault occurs.
> - * For AA64, IAS = OAS according to (IHI 0070.E.a) "3.4 Address sizes"
> - */
> - if (iova >= (1ULL << cfg->oas)) {
> - info->type = SMMU_PTW_ERR_ADDR_SIZE;
> - info->stage = SMMU_STAGE_1;
> - tlbe->entry.perm = IOMMU_NONE;
> - return -EINVAL;
> + int ret = 0;
> + SMMUTLBEntry tlbe_s2;
> + dma_addr_t ipa = iova;
> +
> + if (cfg->stage & SMMU_STAGE_1) {
> + ret = smmu_ptw_64_s1(cfg, iova, perm, tlbe, info, bs);
> + if (ret) {
> + return ret;
> }
> + /* This is the IPA for next stage.*/
but you don't necessarily have the S2 enabled so this is not necessarily
an IPA
> + ipa = CACHED_ENTRY_TO_ADDR(tlbe, iova);
> + }
>
> - return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
> + /*
> + * The address output from the translation causes a stage 1 Address Size
> + * fault if it exceeds the range of the effective IPA size for the given CD.
> + * If bypassing stage 1(or unimplemented), the input address is passed
> + * directly to stage 2 as IPA. If the input address of a transaction
> + * exceeds the size of the IAS, a stage 1 Address Size fault occurs.
> + * For AA64, IAS = OAS according to (IHI 0070.E.a) "3.4 Address sizes"
> + */
> + if (ipa >= (1ULL << cfg->oas)) {
in case we have S2 only, would be clearer to use ias instead (despite
above comment say they are the same)
> + info->type = SMMU_PTW_ERR_ADDR_SIZE;
> + info->stage = SMMU_STAGE_1;
What does the stage really means. That should be documented in the
struct I think
> + tlbe->entry.perm = IOMMU_NONE;
> + return -EINVAL;
> }
this check also is introduced for S1 only. If this is a fix this should
be brought separately.
Also the above comment refers to IPA. Does it also hold for S1 only. Is
the check identical in that case?
>
> - g_assert_not_reached();
> + if (cfg->stage & SMMU_STAGE_2) {
> + ret = smmu_ptw_64_s2(cfg, ipa, perm, &tlbe_s2, info);
> + if (ret) {
> + return ret;
> + }
> + combine_tlb(tlbe, &tlbe_s2, iova, cfg);
> + }
I think I would prefer the S1, S2, nested cases cleary separated at the
price of some code duplication. I am afraid serializing stuff make the
code less maintainable.
Also it is important the S1 case is not altered in terms of perf.
> +
> + return ret;
> +}
> +
> +static int validate_tlb_entry(SMMUTLBEntry *cached_entry, IOMMUAccessFlags flag,
> + SMMUPTWEventInfo *info)
> +{
> + if ((flag & IOMMU_WO) && !(cached_entry->entry.perm &
> + cached_entry->parent_perm & IOMMU_WO)) {
> + info->type = SMMU_PTW_ERR_PERMISSION;
> + info->stage = !(cached_entry->entry.perm & IOMMU_WO) ?
> + SMMU_STAGE_1 :
> + SMMU_STAGE_2;
> + return -EINVAL;
> + }
> + return 0;
> }
>
> SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
> @@ -595,16 +696,14 @@ SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
>
> cached_entry = smmu_iotlb_lookup(bs, cfg, &tt_combined, aligned_addr);
> if (cached_entry) {
> - if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
> - info->type = SMMU_PTW_ERR_PERMISSION;
> - info->stage = cfg->stage;
> + if (validate_tlb_entry(cached_entry, flag, info)) {
> return NULL;
> }
> return cached_entry;
> }
>
> cached_entry = g_new0(SMMUTLBEntry, 1);
> - status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info);
> + status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info, bs);
> if (status) {
> g_free(cached_entry);
> return NULL;
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index cc12924a84..5f23f0b963 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -15,9 +15,9 @@ smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
> smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%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"
> -smmu_iotlb_lookup_hit(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> -smmu_iotlb_lookup_miss(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> -smmu_iotlb_insert(uint16_t asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d"
> +smmu_iotlb_lookup_hit(int asid, uint16_t vmid, uint64_t addr, uint64_t mask, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" mask=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> +smmu_iotlb_lookup_miss(int asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> +smmu_iotlb_insert(int asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level, uint64_t translate_addr) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d translate_addr=0x%"PRIx64
>
> # 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)"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 2772175115..03ff0f02ba 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -91,6 +91,7 @@ typedef struct SMMUTLBEntry {
> IOMMUTLBEntry entry;
> uint8_t level;
> uint8_t granule;
> + IOMMUAccessFlags parent_perm;
> } SMMUTLBEntry;
>
> /* Stage-2 configuration. */
> @@ -198,7 +199,7 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
> * pair, according to @cfg translation config
> */
> int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info);
> + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs);
>
>
> /*
To be honest this patch is quite complex to review. I would suggest you
try to split it.
Thanks
Eric
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 06/13] hw/arm/smmu: Support nesting in smmuv3_range_inval()
2024-04-08 14:08 ` [RFC PATCH v2 06/13] hw/arm/smmu: Support nesting in smmuv3_range_inval() Mostafa Saleh
@ 2024-04-18 14:46 ` Eric Auger
0 siblings, 0 replies; 24+ messages in thread
From: Eric Auger @ 2024-04-18 14:46 UTC (permalink / raw)
To: Mostafa Saleh, qemu-arm, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz
Hi Mostafa,
On 4/8/24 16:08, Mostafa Saleh wrote:
> With nesting, we would need to invalidate IPAs without
> over-invalidating stage-1 IOVAs. This can be done by
> distinguishing IPAs in the TLBs by having ASID=-1.
> To achieve that, rework the invalidation for IPAs to have a
> separate function, while for IOVA invalidation ASID=-1 means
> invalidate for all ASIDs.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmu-common.c | 47 ++++++++++++++++++++++++++++++++++++
> hw/arm/smmuv3.c | 23 ++++++++++++------
> hw/arm/trace-events | 2 +-
> include/hw/arm/smmu-common.h | 3 ++-
> 4 files changed, 66 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 2cf27b490b..8b9e59b24b 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -184,6 +184,25 @@ static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
> ((entry->iova & ~info->mask) == info->iova);
> }
>
> +static gboolean smmu_hash_remove_by_vmid_ipa(gpointer key, gpointer value,
> + gpointer user_data)
> +{
> + SMMUTLBEntry *iter = (SMMUTLBEntry *)value;
> + IOMMUTLBEntry *entry = &iter->entry;
> + SMMUIOTLBPageInvInfo *info = (SMMUIOTLBPageInvInfo *)user_data;
> + SMMUIOTLBKey iotlb_key = *(SMMUIOTLBKey *)key;
> +
> + /* This is a stage-1 address. */
> + if (info->asid >= 0) {
nit: I am rather used to have the comment associated to the condition
after the condition check
> + return false;
> + }
> + if (info->vmid != SMMU_IOTLB_VMID(iotlb_key)) {
> + return false;
> + }
> + return ((info->iova & ~entry->addr_mask) == entry->iova) ||
> + ((entry->iova & ~info->mask) == info->iova);
> +}
> +
> void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> uint8_t tg, uint64_t num_pages, uint8_t ttl)
> {
> @@ -212,6 +231,34 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> &info);
> }
>
> +/*
> + * Similar to smmu_iotlb_inv_iova(), but for Stage-2, ASID is always -1,
> + * in Stage-1 invalidation ASID = -1, means don't care.
> + */
> +void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
> + uint64_t num_pages, uint8_t ttl)
> +{
> + uint8_t granule = tg ? tg * 2 + 10 : 12;
> + int asid = -1;
> +
> + if (ttl && (num_pages == 1)) {
> + SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, ipa, tg, ttl);
> +
> + if (g_hash_table_remove(s->iotlb, &key)) {
> + return;
> + }
> + }
> +
> + SMMUIOTLBPageInvInfo info = {
> + .iova = ipa,
> + .vmid = vmid,
> + .mask = (num_pages * 1 << granule) - 1};
> +
> + g_hash_table_foreach_remove(s->iotlb,
> + smmu_hash_remove_by_vmid_ipa,
> + &info);
> +}
> +
> void smmu_iotlb_inv_asid(SMMUState *s, int asid)
> {
> trace_smmu_iotlb_inv_asid(asid);
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index a7cf543acc..17bbd43c13 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1095,7 +1095,7 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
> }
> }
>
> -static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
> +static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage)
> {
> dma_addr_t end, addr = CMD_ADDR(cmd);
> uint8_t type = CMD_TYPE(cmd);
> @@ -1120,9 +1120,13 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
> }
>
> if (!tg) {
> - trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
> + trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf, stage);
> smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1);
> - smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
> + if (stage == SMMU_STAGE_1) {
> + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
> + } else {
> + smmu_iotlb_inv_ipa(s, vmid, addr, tg, 1, ttl);
> + }
> return;
> }
>
> @@ -1138,9 +1142,14 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
> uint64_t mask = dma_aligned_pow2_mask(addr, end, 64);
>
> num_pages = (mask + 1) >> granule;
> - trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
> + trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages,
> + ttl, leaf, stage);
> smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages);
> - smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
> + if (stage == SMMU_STAGE_1) {
> + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
> + } else {
> + smmu_iotlb_inv_ipa(s, vmid, addr, tg, num_pages, ttl);
> + }
> addr += mask + 1;
> }
> }
> @@ -1299,7 +1308,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> cmd_error = SMMU_CERROR_ILL;
> break;
> }
> - smmuv3_range_inval(bs, &cmd);
> + smmuv3_range_inval(bs, &cmd, SMMU_STAGE_1);
> break;
> case SMMU_CMD_TLBI_S12_VMALL:
> {
> @@ -1324,7 +1333,7 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> * As currently only either s1 or s2 are supported
> * we can reuse same function for s2.
> */
> - smmuv3_range_inval(bs, &cmd);
> + smmuv3_range_inval(bs, &cmd, SMMU_STAGE_2);
> break;
> case SMMU_CMD_TLBI_EL3_ALL:
> case SMMU_CMD_TLBI_EL3_VA:
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 5f23f0b963..f5c361d96e 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -46,7 +46,7 @@ smmuv3_cmdq_cfgi_ste_range(int start, int end) "start=0x%x - end=0x%x"
> smmuv3_cmdq_cfgi_cd(uint32_t sid) "sid=0x%x"
> smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache HIT for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache MISS for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> -smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
> +smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf, int stage) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d stage=%d"
> smmuv3_cmdq_tlbi_nh(void) ""
> smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
> smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 03ff0f02ba..df166d8477 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -230,7 +230,8 @@ void smmu_iotlb_inv_asid(SMMUState *s, int asid);
> void smmu_iotlb_inv_vmid(SMMUState *s, int vmid);
> void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> uint8_t tg, uint64_t num_pages, uint8_t ttl);
> -
> +void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
> + uint64_t num_pages, uint8_t ttl);
> /* Unmap the range of all the notifiers registered to any IOMMU mr */
> void smmu_inv_notifiers_all(SMMUState *s);
Besides looks good to me
smmu_hash_remove_by_vmid_ipa
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 07/13] hw/arm/smmu: Support nesting in the rest of commands
2024-04-08 14:08 ` [RFC PATCH v2 07/13] hw/arm/smmu: Support nesting in the rest of commands Mostafa Saleh
@ 2024-04-18 14:48 ` Eric Auger
2024-04-19 10:56 ` Mostafa Saleh
0 siblings, 1 reply; 24+ messages in thread
From: Eric Auger @ 2024-04-18 14:48 UTC (permalink / raw)
To: Mostafa Saleh, qemu-arm, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz
Hi Mostafa,
On 4/8/24 16:08, Mostafa Saleh wrote:
> Some commands need rework for nesting, as they used to assume S1
> and S2 are mutually exclusive:
>
> - CMD_TLBI_NH_ASID: Consider VMID if stage-2 is supported
> - CMD_TLBI_NH_ALL: Consider VMID if stage-2 is supported, otherwise
> invalidate everything, this required a new vmid invalidation
> function for stage-1 only (ASID >= 0)
>
> Also, rework trace events to reflect the new implementation.
This does not apply for me. Could you share a branch or respin?
Thank you in advance
Eric
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmu-common.c | 36 +++++++++++++++++++++++++++++-------
> hw/arm/smmuv3.c | 31 +++++++++++++++++++++++++++++--
> hw/arm/trace-events | 6 ++++--
> include/hw/arm/smmu-common.h | 3 ++-
> 4 files changed, 64 insertions(+), 12 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 8b9e59b24b..b1cf1303c6 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -148,13 +148,14 @@ void smmu_iotlb_inv_all(SMMUState *s)
> g_hash_table_remove_all(s->iotlb);
> }
>
> -static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
> - gpointer user_data)
> +static gboolean smmu_hash_remove_by_asid_vmid(gpointer key, gpointer value,
> + gpointer user_data)
> {
> - int asid = *(int *)user_data;
> + SMMUIOTLBPageInvInfo *info = (SMMUIOTLBPageInvInfo *)user_data;
> SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
>
> - return SMMU_IOTLB_ASID(*iotlb_key) == asid;
> + return (SMMU_IOTLB_ASID(*iotlb_key) == info->asid) &&
> + (SMMU_IOTLB_VMID(*iotlb_key) == info->vmid);
> }
>
> static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> @@ -166,6 +167,16 @@ static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
> }
>
> +static gboolean smmu_hash_remove_by_vmid_s1(gpointer key, gpointer value,
> + gpointer user_data)
> +{
> + int vmid = *(int *)user_data;
> + SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
> +
> + return (SMMU_IOTLB_VMID(*iotlb_key) == vmid) &&
> + (SMMU_IOTLB_ASID(*iotlb_key) >= 0);
> +}
> +
> static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
> gpointer user_data)
> {
> @@ -259,10 +270,15 @@ void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
> &info);
> }
>
> -void smmu_iotlb_inv_asid(SMMUState *s, int asid)
> +void smmu_iotlb_inv_asid_vmid(SMMUState *s, int asid, int vmid)
> {
> - trace_smmu_iotlb_inv_asid(asid);
> - g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
> + SMMUIOTLBPageInvInfo info = {
> + .asid = asid,
> + .vmid = vmid,
> + };
> +
> + trace_smmu_iotlb_inv_asid_vmid(asid, vmid);
> + g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid_vmid, &info);
> }
>
> inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
> @@ -271,6 +287,12 @@ inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
> g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
> }
>
> +inline void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid)
> +{
> + trace_smmu_iotlb_inv_vmid_s1(vmid);
> + g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid_s1, &vmid);
> +}
> +
> /* VMSAv8-64 Translation */
>
> /**
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 17bbd43c13..ece647b8bf 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1280,25 +1280,52 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> case SMMU_CMD_TLBI_NH_ASID:
> {
> int asid = CMD_ASID(&cmd);
> + int vmid = -1;
>
> if (!STAGE1_SUPPORTED(s)) {
> cmd_error = SMMU_CERROR_ILL;
> break;
> }
>
> + /*
> + * VMID is only matched when stage 2 is supported for the Security
> + * state corresponding to the command queue that the command was
> + * issued in.
> + * QEMU ignores the field by setting to -1, similarly to what STE
> + * decoding does. And invalidation commands ignore VMID < 0.
> + */
> + if (STAGE2_SUPPORTED(s)) {
> + vmid = CMD_VMID(&cmd);
> + }
> +
> trace_smmuv3_cmdq_tlbi_nh_asid(asid);
> smmu_inv_notifiers_all(&s->smmu_state);
> - smmu_iotlb_inv_asid(bs, asid);
> + smmu_iotlb_inv_asid_vmid(bs, asid, vmid);
> break;
> }
> case SMMU_CMD_TLBI_NH_ALL:
> + {
> + int vmid = -1;
> +
> if (!STAGE1_SUPPORTED(s)) {
> cmd_error = SMMU_CERROR_ILL;
> break;
> }
> +
> + /*
> + * If stage-2 is supported, invalidate for this VMID only, otherwise
> + * invalidate the whole thing, see SMMU_CMD_TLBI_NH_ASID()
> + */
> + if (STAGE2_SUPPORTED(s)) {
> + vmid = CMD_VMID(&cmd);
> + trace_smmuv3_cmdq_tlbi_nh(vmid);
> + smmu_iotlb_inv_vmid_s1(bs, vmid);
> + break;
> + }
> QEMU_FALLTHROUGH;
> + }
> case SMMU_CMD_TLBI_NSNH_ALL:
> - trace_smmuv3_cmdq_tlbi_nh();
> + trace_smmuv3_cmdq_tlbi_nsnh();
> smmu_inv_notifiers_all(&s->smmu_state);
> smmu_iotlb_inv_all(bs);
> break;
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index f5c361d96e..2556f4721a 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -11,8 +11,9 @@ smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint6
> 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_inv_all(void) "IOTLB invalidate all"
> -smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
> +smmu_iotlb_inv_asid_vmid(int asid, uint16_t vmid) "IOTLB invalidate asid=%d vmid=%d"
> smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%d"
> +smmu_iotlb_inv_vmid_s1(uint16_t vmid) "IOTLB invalidate vmid=%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"
> smmu_iotlb_lookup_hit(int asid, uint16_t vmid, uint64_t addr, uint64_t mask, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" mask=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> @@ -47,7 +48,8 @@ smmuv3_cmdq_cfgi_cd(uint32_t sid) "sid=0x%x"
> smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache HIT for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache MISS for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf, int stage) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d stage=%d"
> -smmuv3_cmdq_tlbi_nh(void) ""
> +smmuv3_cmdq_tlbi_nh(int vmid) "vmid=%d"
> +smmuv3_cmdq_tlbi_nsnh(void) ""
> smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
> smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
> smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index df166d8477..67db30e85b 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -226,8 +226,9 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
> SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
> uint8_t tg, uint8_t level);
> void smmu_iotlb_inv_all(SMMUState *s);
> -void smmu_iotlb_inv_asid(SMMUState *s, int asid);
> +void smmu_iotlb_inv_asid_vmid(SMMUState *s, int asid, int vmid);
> void smmu_iotlb_inv_vmid(SMMUState *s, int vmid);
> +void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid);
> void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> uint8_t tg, uint64_t num_pages, uint8_t ttl);
> void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 00/13] SMMUv3 nested translation support
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
` (12 preceding siblings ...)
2024-04-08 14:08 ` [RFC PATCH v2 13/13] hw/arm/virt: Set SMMU OAS based on CPU PARANGE Mostafa Saleh
@ 2024-04-18 18:11 ` Eric Auger
2024-04-19 9:22 ` Mostafa Saleh
13 siblings, 1 reply; 24+ messages in thread
From: Eric Auger @ 2024-04-18 18:11 UTC (permalink / raw)
To: Mostafa Saleh, qemu-arm, peter.maydell, qemu-devel
Cc: jean-philippe, alex.bennee, maz, nicolinc, julien,
richard.henderson, marcin.juszkiewicz
Hi Mostafa,
On 4/8/24 16:08, Mostafa Saleh wrote:
> Currently, QEMU supports emulating either stage-1 or stage-2 SMMUs
> but not nested instances.
> This patch series adds support for nested translation in SMMUv3,
> this is controlled by property “arm-smmuv3.stage=nested”, and
> advertised to guests as (IDR0.S1P == 1 && IDR0.S2P == 2)
>
> Main changes(architecture):
> ============================
> 1) CDs are considered IPA and translated with stage-2.
> 2) TTBx and tables for stage-1 are considered IPA and translated
> with stage-2.
> 3) Translate the IPA address with stage-2.
>
> TLBs:
> ======
> TLBs are the most tricky part.
>
> 1) General design
> Unified(Combined) design is used, where entries with ASID=-1 are
> IPAs(cached from stage-2 config)
>
> TLBs are also modified to cache 2 permissions, a new permission added
> "parent_perm."
>
> For non-nested configuration, perm == parent_perm and nothing
> changes. This is used to know which stage to use in case there is
> a permission fault from a TLB entry.
>
> 2) Caching in TLB
> Stage-1 and stage-2 are inserted in the TLB as is.
> For nested translation, both entries are combined into one TLB
> entry. The size (level and granule) are chosen from the smallest entries.
> That means that a stage-1 translation can be cached with sage-2
> granule in key, this is take into account lookup.
is that a correct understanding that with the current implementation, in
nested mode, you end up with combined S1 + S2 entries (IOVA -> PA) and
S2 entries (IPA -> PA)?
Out of cusiosity, how did you end up with that choice? Have you made
some perf assessment compared to separate S1 and S2 entries? I guess it
is a complex topic and choice.
Thanks
Eric
>
> 3) TLB Lookup
> TLB lookup already uses ASID in key, so it can distinguish between
> stage-1 and stage-2.
> And as mentioned above, the granule for stage-1 can be different,
> If stage-1 lookup failed, we try again with the stage-2 granule.
>
> 4) TLB invalidation
> - Address invalidation is split, for IOVA(CMD_TLBI_NH_VA
> /CMD_TLBI_NH_VAA) and IPA(CMD_TLBI_S2_IPA) based on ASID value
> - CMD_TLBI_NH_ASID/CMD_TLBI_NH_ALL: Consider VMID if stage-2 is
> supported, and invalidate stage-1 only by VMIDs
>
> As far as I understand, this is compliant with the ARM architecture:
> - ARM ARM DDI 0487J.a: RLGSCG, RTVTYQ, RGNJPZ
> - ARM IHI 0070F.b: 16.2 Caching
>
> An alternative approach would be to instantiate 2 TLBs, one per each
> stage. I haven’t investigated that.
>
> Others
> =======
> - Advertise SMMUv3.2-S2FWB, it is NOP for QEMU as it doesn’t support
> attributes.
>
> - OAS: A typical setup with nesting is to share CPU stage-2 with the
> SMMU, and according to the user manual, SMMU OAS must match the
> system physical address.
>
> This was discussed before in
> https://lore.kernel.org/all/20230226220650.1480786-11-smostafa@google.com/
> The implementation here, follows the discussion, where migration is
> added and oas is set up from the board (virt). However, the OAS is
> chosen based on the CPU PARANGE as there is no fixed one.
>
> - For nested configuration, IOVA notifier only notifies for stage-1
> invalidations (as far as I understand this is the intended
> behaviour as it notifies for IOVA)
>
> - Stop ignoring VMID for stage-1 if stage-2 is also supported.
>
>
> Future improvements:
> =====================
> 1) One small improvement, that I don’t think it’s worth the extra
> complexity, is in case of Stage-1 TLB miss for nested translation,
> we can do stage-1 walk and lookup for stage-2 TLBs, instead of
> doing the full walk.
>
> 2) Patch 0006 (hw/arm/smmuv3: Translate CD and TT using stage-2 table)
> introduces a macro to use functions that rely on cfg for stage-2,
> I don’t like it. However, I didn’t find a simple way around it,
> either we change many functions to have a separate stage argument,
> or add another arg in config, which is probably more code.
>
> Testing
> ========
> 1) IOMMUFD + VFIO
> Kernel: https://lore.kernel.org/all/cover.1683688960.git.nicolinc@nvidia.com/
> VMM: https://qemu-devel.nongnu.narkive.com/o815DqpI/rfc-v5-0-8-arm-smmuv3-emulation-support
>
> By assigning “virtio-net-pci,netdev=net0,disable-legacy=on,iommu_platform=on,ats=on”,
> to a guest VM (on top of QEMU guest) with VIFO and IOMMUFD.
>
> 2) Work in progress prototype I am hacking on for nesting on KVM
> (this is nowhere near complete, and misses many stuff but it
> doesn't require VMs/VFIO) also with virtio-net-pci and git
> cloning a bunch of stuff and also observing traces.
> https://android-kvm.googlesource.com/linux/+log/refs/heads/smostafa/android15-6.6-smmu-nesting-wip
>
> I also modified the Linux driver to test with mixed granules/levels.
>
> hw/arm/smmuv3: Split smmuv3_translate() better viewed with --color-moved
>
> Changes in v2:
> v1: https://lore.kernel.org/qemu-devel/20240325101442.1306300-1-smostafa@google.com/
> - Collected Eric Rbs
> - Rework TLB to rely on VMID/ASID instead of an extra key.
> - Fixed TLB issue with large stage-1 reported by Julian.
> - Cap the OAS to 48 bits as PTW doesn’t support 52 bits.
> - Fix ASID/VMID representation in some contexts as 16 bits while
> they can be -1
> - Increase visibility in trace points
>
>
> Mostafa Saleh (13):
> hw/arm/smmu: Use enum for SMMU stage
> hw/arm/smmu: Split smmuv3_translate()
> hw/arm/smmu: Consolidate ASID and VMID types
> hw/arm/smmuv3: Translate CD and TT using stage-2 table
> hw/arm/smmu-common: Support nested translation
> hw/arm/smmu: Support nesting in smmuv3_range_inval()
> hw/arm/smmu: Support nesting in the rest of commands
> hw/arm/smmuv3: Support nested SMMUs in smmuv3_notify_iova()
> hw/arm/smmuv3: Support and advertise nesting
> hw/arm/smmuv3: Advertise S2FWB
> hw/arm/smmu: Refactor SMMU OAS
> hw/arm/smmuv3: Add property for OAS
> hw/arm/virt: Set SMMU OAS based on CPU PARANGE
>
> hw/arm/smmu-common.c | 306 +++++++++++++++++++++----
> hw/arm/smmuv3-internal.h | 16 +-
> hw/arm/smmuv3.c | 418 ++++++++++++++++++++++-------------
> hw/arm/trace-events | 18 +-
> hw/arm/virt.c | 14 +-
> include/hw/arm/smmu-common.h | 57 ++++-
> include/hw/arm/smmuv3.h | 1 +
> target/arm/cpu.h | 2 +
> target/arm/cpu64.c | 5 +
> 9 files changed, 608 insertions(+), 229 deletions(-)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 00/13] SMMUv3 nested translation support
2024-04-18 18:11 ` [RFC PATCH v2 00/13] SMMUv3 nested translation support Eric Auger
@ 2024-04-19 9:22 ` Mostafa Saleh
0 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-19 9:22 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-arm, peter.maydell, qemu-devel, jean-philippe, alex.bennee,
maz, nicolinc, julien, richard.henderson, marcin.juszkiewicz
Hi Eric,
On Thu, Apr 18, 2024 at 08:11:06PM +0200, Eric Auger wrote:
> Hi Mostafa,
>
> On 4/8/24 16:08, Mostafa Saleh wrote:
> > Currently, QEMU supports emulating either stage-1 or stage-2 SMMUs
> > but not nested instances.
> > This patch series adds support for nested translation in SMMUv3,
> > this is controlled by property “arm-smmuv3.stage=nested”, and
> > advertised to guests as (IDR0.S1P == 1 && IDR0.S2P == 2)
> >
> > Main changes(architecture):
> > ============================
> > 1) CDs are considered IPA and translated with stage-2.
> > 2) TTBx and tables for stage-1 are considered IPA and translated
> > with stage-2.
> > 3) Translate the IPA address with stage-2.
> >
> > TLBs:
> > ======
> > TLBs are the most tricky part.
> >
> > 1) General design
> > Unified(Combined) design is used, where entries with ASID=-1 are
> > IPAs(cached from stage-2 config)
> >
> > TLBs are also modified to cache 2 permissions, a new permission added
> > "parent_perm."
> >
> > For non-nested configuration, perm == parent_perm and nothing
> > changes. This is used to know which stage to use in case there is
> > a permission fault from a TLB entry.
> >
> > 2) Caching in TLB
> > Stage-1 and stage-2 are inserted in the TLB as is.
> > For nested translation, both entries are combined into one TLB
> > entry. The size (level and granule) are chosen from the smallest entries.
> > That means that a stage-1 translation can be cached with sage-2
> > granule in key, this is take into account lookup.
> is that a correct understanding that with the current implementation, in
> nested mode, you end up with combined S1 + S2 entries (IOVA -> PA) and
> S2 entries (IPA -> PA)?
Yes, that’s correct.
> Out of cusiosity, how did you end up with that choice? Have you made
> some perf assessment compared to separate S1 and S2 entries? I guess it
> is a complex topic and choice.
>
I didn’t do any perf, but from my simplistic understanding, combined
TLBs should be faster as they use only one look up for full translation,
also I guess having a single TLB would be better for HW area.
(However my knowledge in this “area” is almost null)
Although in SW, we don’t have tough memory constraints and having more
(or separate) TLBs isn’t a problem.
While implementing this, at some point I thought it’s getting too
complicated and a separate one might have been better, but the
grass is always greener on the other side, and I believe it would
also have its challenges.
One other thing I like about combined TLBs (which I am not sure is
important for qemu) is that it is more relaxed which means it would
catch more SW bugs. For example if the SW only changes an IPA and
only invalidates by IPA, it would have issues with combined TLBs.
I am open to try other designs if you have something else in mind.
Thanks,
Mostafa
> Thanks
>
> Eric
> >
> > 3) TLB Lookup
> > TLB lookup already uses ASID in key, so it can distinguish between
> > stage-1 and stage-2.
> > And as mentioned above, the granule for stage-1 can be different,
> > If stage-1 lookup failed, we try again with the stage-2 granule.
> >
> > 4) TLB invalidation
> > - Address invalidation is split, for IOVA(CMD_TLBI_NH_VA
> > /CMD_TLBI_NH_VAA) and IPA(CMD_TLBI_S2_IPA) based on ASID value
> > - CMD_TLBI_NH_ASID/CMD_TLBI_NH_ALL: Consider VMID if stage-2 is
> > supported, and invalidate stage-1 only by VMIDs
> >
> > As far as I understand, this is compliant with the ARM architecture:
> > - ARM ARM DDI 0487J.a: RLGSCG, RTVTYQ, RGNJPZ
> > - ARM IHI 0070F.b: 16.2 Caching
> >
> > An alternative approach would be to instantiate 2 TLBs, one per each
> > stage. I haven’t investigated that.
> >
> > Others
> > =======
> > - Advertise SMMUv3.2-S2FWB, it is NOP for QEMU as it doesn’t support
> > attributes.
> >
> > - OAS: A typical setup with nesting is to share CPU stage-2 with the
> > SMMU, and according to the user manual, SMMU OAS must match the
> > system physical address.
> >
> > This was discussed before in
> > https://lore.kernel.org/all/20230226220650.1480786-11-smostafa@google.com/
> > The implementation here, follows the discussion, where migration is
> > added and oas is set up from the board (virt). However, the OAS is
> > chosen based on the CPU PARANGE as there is no fixed one.
> >
> > - For nested configuration, IOVA notifier only notifies for stage-1
> > invalidations (as far as I understand this is the intended
> > behaviour as it notifies for IOVA)
> >
> > - Stop ignoring VMID for stage-1 if stage-2 is also supported.
> >
> >
> > Future improvements:
> > =====================
> > 1) One small improvement, that I don’t think it’s worth the extra
> > complexity, is in case of Stage-1 TLB miss for nested translation,
> > we can do stage-1 walk and lookup for stage-2 TLBs, instead of
> > doing the full walk.
> >
> > 2) Patch 0006 (hw/arm/smmuv3: Translate CD and TT using stage-2 table)
> > introduces a macro to use functions that rely on cfg for stage-2,
> > I don’t like it. However, I didn’t find a simple way around it,
> > either we change many functions to have a separate stage argument,
> > or add another arg in config, which is probably more code.
> >
> > Testing
> > ========
> > 1) IOMMUFD + VFIO
> > Kernel: https://lore.kernel.org/all/cover.1683688960.git.nicolinc@nvidia.com/
> > VMM: https://qemu-devel.nongnu.narkive.com/o815DqpI/rfc-v5-0-8-arm-smmuv3-emulation-support
> >
> > By assigning “virtio-net-pci,netdev=net0,disable-legacy=on,iommu_platform=on,ats=on”,
> > to a guest VM (on top of QEMU guest) with VIFO and IOMMUFD.
> >
> > 2) Work in progress prototype I am hacking on for nesting on KVM
> > (this is nowhere near complete, and misses many stuff but it
> > doesn't require VMs/VFIO) also with virtio-net-pci and git
> > cloning a bunch of stuff and also observing traces.
> > https://android-kvm.googlesource.com/linux/+log/refs/heads/smostafa/android15-6.6-smmu-nesting-wip
> >
> > I also modified the Linux driver to test with mixed granules/levels.
> >
> > hw/arm/smmuv3: Split smmuv3_translate() better viewed with --color-moved
> >
> > Changes in v2:
> > v1: https://lore.kernel.org/qemu-devel/20240325101442.1306300-1-smostafa@google.com/
> > - Collected Eric Rbs
> > - Rework TLB to rely on VMID/ASID instead of an extra key.
> > - Fixed TLB issue with large stage-1 reported by Julian.
> > - Cap the OAS to 48 bits as PTW doesn’t support 52 bits.
> > - Fix ASID/VMID representation in some contexts as 16 bits while
> > they can be -1
> > - Increase visibility in trace points
> >
> >
> > Mostafa Saleh (13):
> > hw/arm/smmu: Use enum for SMMU stage
> > hw/arm/smmu: Split smmuv3_translate()
> > hw/arm/smmu: Consolidate ASID and VMID types
> > hw/arm/smmuv3: Translate CD and TT using stage-2 table
> > hw/arm/smmu-common: Support nested translation
> > hw/arm/smmu: Support nesting in smmuv3_range_inval()
> > hw/arm/smmu: Support nesting in the rest of commands
> > hw/arm/smmuv3: Support nested SMMUs in smmuv3_notify_iova()
> > hw/arm/smmuv3: Support and advertise nesting
> > hw/arm/smmuv3: Advertise S2FWB
> > hw/arm/smmu: Refactor SMMU OAS
> > hw/arm/smmuv3: Add property for OAS
> > hw/arm/virt: Set SMMU OAS based on CPU PARANGE
> >
> > hw/arm/smmu-common.c | 306 +++++++++++++++++++++----
> > hw/arm/smmuv3-internal.h | 16 +-
> > hw/arm/smmuv3.c | 418 ++++++++++++++++++++++-------------
> > hw/arm/trace-events | 18 +-
> > hw/arm/virt.c | 14 +-
> > include/hw/arm/smmu-common.h | 57 ++++-
> > include/hw/arm/smmuv3.h | 1 +
> > target/arm/cpu.h | 2 +
> > target/arm/cpu64.c | 5 +
> > 9 files changed, 608 insertions(+), 229 deletions(-)
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table
2024-04-18 12:51 ` Eric Auger
@ 2024-04-19 10:23 ` Mostafa Saleh
0 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-19 10:23 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-arm, peter.maydell, qemu-devel, jean-philippe, alex.bennee,
maz, nicolinc, julien, richard.henderson, marcin.juszkiewicz
Hi Eric,
On Thu, Apr 18, 2024 at 02:51:59PM +0200, Eric Auger wrote:
> Hi Mostafa,
>
> On 4/8/24 16:08, Mostafa Saleh wrote:
> > According to the user manual (ARM IHI 0070 F.b),
> s/user manual/ARM SMMU architecture specification
> > In "5.2 Stream Table Entry":
> > [51:6] S1ContextPtr
> > If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by
> > stage 2 and the programmed value must be within the range of the IAS.
> >
> > In "5.4.1 CD notes":
> > The translation table walks performed from TTB0 or TTB1 are always performed
> > in IPA space if stage 2 translations are enabled.
> >
> > So translate both the CD and the TTBx in this patch if nested
> translate the S1 context descriptor pointer and TTBx base addresses
> through the S2 stage (IPA -> PA)
>
> You may describe what you put in place to do the translation in the
> commit msg, new functions, macro, ...
Will do.
> > translation is requested.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> > hw/arm/smmuv3.c | 49 ++++++++++++++++++++++++++++++------
> > include/hw/arm/smmu-common.h | 17 +++++++++++++
> > 2 files changed, 59 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 897f8fe085..a7cf543acc 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -337,14 +337,36 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
> >
> > }
> >
> > +static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> > + SMMUTransCfg *cfg,
> > + SMMUEventInfo *event,
> > + IOMMUAccessFlags flag,
> > + SMMUTLBEntry **out_entry);
> > /* @ssid > 0 not supported yet */
> > -static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
> > - CD *buf, SMMUEventInfo *event)
> > +static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg,
> > + uint32_t ssid, CD *buf, SMMUEventInfo *event)
> > {
> > dma_addr_t addr = STE_CTXPTR(ste);
> > int ret, i;
> > + SMMUTranslationStatus status;
> > + SMMUTLBEntry *entry;
> >
> > trace_smmuv3_get_cd(addr);
> > +
> > + if (cfg->stage == SMMU_NESTED) {
> > + CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s, addr,
> > + cfg, event, IOMMU_RO, &entry);
> the fact we pass 2 times cfg looks pretty weird from a caller pov. See
> my comment below.
Yes, I don’t like it also as I mentioned in the cover letter,
(see me comment below also)
>
> do we somewhere check addr is within the proper addr range, IAS if S2,
> OAS if S1. This was missing for S1 but I think it is worth improving now.
> see 3.4.3
Yes, this was added in the next patch.
> > + /*
> > + * It is not clear what should happen if this fails, so we return here
> > + * which gets propagated as a translation error.
> but the error event might be different, no?
But the event is passed to the translate function so the right translation
error will be in the event (addr size, permission…).
It isn't clear to me from the specs if this should be a translation error
or some F_CD_FETCH/C_BAD_CD, and hence the comment, but I though the
translation error info would be more useful for SW that's why I used it.
> > + */
> > + if (status != SMMU_TRANS_SUCCESS) {
> > + return -EINVAL;
> > + }
> > +
> > + addr = CACHED_ENTRY_TO_ADDR(entry, addr);
> > + }
> > +
> > /* TODO: guarantee 64-bit single-copy atomicity */
> > ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf),
> > MEMTXATTRS_UNSPECIFIED);
> > @@ -659,10 +681,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
> > return 0;
> > }
> >
> > -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> > +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
> > + CD *cd, SMMUEventInfo *event)
> > {
> > int ret = -EINVAL;
> > int i;
> > + SMMUTranslationStatus status;
> > + SMMUTLBEntry *entry;
> >
> > if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
> > goto bad_cd;
> > @@ -713,6 +738,17 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> >
> > tt->tsz = tsz;
> > tt->ttb = CD_TTB(cd, i);
> > +
> > + /* Translate the TTBx, from IPA to PA if nesting is enabled. */
> > + if (cfg->stage == SMMU_NESTED) {
> > + CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s,
> > + tt->ttb, cfg, event, IOMMU_RO, &entry);
> > + /* See smmu_get_cd(). */
> ditto
> > + if (status != SMMU_TRANS_SUCCESS) {
> > + return -EINVAL;
> > + }
> > + tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb);
> > + }
> > if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) {
> > goto bad_cd;
> > }
> > @@ -767,12 +803,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
> > return 0;
> > }
> >
> > - ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event);
> > + ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event);
> > if (ret) {
> > return ret;
> > }
> >
> > - return decode_cd(cfg, &cd, event);
> > + return decode_cd(s, cfg, &cd, event);
> > }
> >
> > /**
> > @@ -942,8 +978,7 @@ epilogue:
> > switch (status) {
> > case SMMU_TRANS_SUCCESS:
> > entry.perm = cached_entry->entry.perm;
> > - entry.translated_addr = cached_entry->entry.translated_addr +
> > - (addr & cached_entry->entry.addr_mask);
> > + entry.translated_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
> > entry.addr_mask = cached_entry->entry.addr_mask;
> > trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
> > entry.translated_addr, entry.perm,
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index 96eb017e50..2772175115 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -37,6 +37,23 @@
> > #define VMSA_IDXMSK(isz, strd, lvl) ((1ULL << \
> > VMSA_BIT_LVL(isz, strd, lvl)) - 1)
> >
> > +#define CACHED_ENTRY_TO_ADDR(ent, addr) (ent)->entry.translated_addr + \
> > + ((addr) & (ent)->entry.addr_mask);
> > +
> > +/*
> > + * From nested context, some functions might need to translate IPA addresses.
> > + * As cfg has SMMU_NESTED, this won't work, this macro calls a function with
> > + * making it a stage-2 cfg and then restore it after.
> > + */
> > +#define CALL_FUNC_CFG_S2(cfg, ret, fn, ...) ({ \
> > + int asid = cfg->asid; \
> > + cfg->stage = SMMU_STAGE_2; \
> > + cfg->asid = -1; \
> > + ret = fn(__VA_ARGS__); \
> At this stage of the reading this is not obvious why you need fn()
> parameter, can't you simply call
>
> smmuv3_do_translate(). If this is useful at some point in the series, you shall document that in the commit msg.
> Also I think I would prefer a proper function instead of this macro.
>
> Besides, can't we add an extra parameter to the translate function forcing the S2_only translation although the cfg is a nested one. I think this would make things clearer
>
This macro is reused with “smmu_translate” in the next patches, I had a
look with adding a a separate arg and I didn’t like it either, I can try
again or may be have a clear separate wrapper for this.
Thanks,
Mostafa
> > + cfg->asid = asid; \
> > + cfg->stage = SMMU_NESTED; \
> > + })
> > +
> > /*
> > * Page table walk error types
> > */
> Thanks
>
> Eric
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 05/13] hw/arm/smmu-common: Support nested translation
2024-04-18 13:54 ` Eric Auger
@ 2024-04-19 10:54 ` Mostafa Saleh
0 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-19 10:54 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-arm, peter.maydell, qemu-devel, jean-philippe, alex.bennee,
maz, nicolinc, julien, richard.henderson, marcin.juszkiewicz
Hi Eric,
On Thu, Apr 18, 2024 at 03:54:01PM +0200, Eric Auger wrote:
> Hi Mostafa,
>
> On 4/8/24 16:08, Mostafa Saleh wrote:
> > When nested translation is requested, do the following:
> >
> > - Translate stage-1 IPA using stage-2 to a physical address.
> > - Translate stage-1 PTW walks using stage-2.
> > - Combine both to create a single TLB entry, for that we choose
> > the smallest entry to cache, which means that if the smallest
> > entry comes from stage-2, and stage-2 use different granule,
> > TLB lookup for stage-1 (in nested config) will always miss.
> > Lookup logic is modified for nesting to lookup using stage-2
> > granule if stage-1 granule missed and they are different.
> >
> > Also, add more visibility in trace points, to make it easier to debug.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> > hw/arm/smmu-common.c | 153 ++++++++++++++++++++++++++++-------
> > hw/arm/trace-events | 6 +-
> > include/hw/arm/smmu-common.h | 3 +-
> > 3 files changed, 131 insertions(+), 31 deletions(-)
> >
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index 771b9c79a3..2cf27b490b 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
> > return key;
> > }
> >
> > -SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> > - SMMUTransTableInfo *tt, hwaddr iova)
> > +static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs,
> > + SMMUTransCfg *cfg,
> > + SMMUTransTableInfo *tt,
> > + hwaddr iova)
> this helper can be introduced in a separate patch to ease the code review
Will do.
> > {
> > uint8_t tg = (tt->granule_sz - 10) / 2;
> > uint8_t inputsize = 64 - tt->tsz;
> > @@ -88,10 +90,29 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> > }
> > level++;
> > }
> > + return entry;
> > +}
> > +
> > +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> > + SMMUTransTableInfo *tt, hwaddr iova)
> > +{
> > + SMMUTLBEntry *entry = NULL;
> > +
> > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova);
> > + /*
> > + * For nested translation also use the s2 granule, as the TLB will insert
> > + * the smallest of both, so the entry can be cached with the s2 granule.
> > + */
> > + if (!entry && (cfg->stage == SMMU_NESTED) &&
> > + (cfg->s2cfg.granule_sz != tt->granule_sz)) {
> > + tt->granule_sz = cfg->s2cfg.granule_sz;
> > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova);
> this is also the kind of stuff that can be introduced and reviewed
> separately without tkaing any risk until NESTED is not support. In this
> new patch you could also document the TLB strategy.
Will do.
> > + }
> >
> > if (entry) {
> > cfg->iotlb_hits++;
> > trace_smmu_iotlb_lookup_hit(cfg->asid, cfg->s2cfg.vmid, iova,
> > + entry->entry.addr_mask,
> can be moved to a separate fix. same for the trace point changes
Will do.
> > cfg->iotlb_hits, cfg->iotlb_misses,
> > 100 * cfg->iotlb_hits /
> > (cfg->iotlb_hits + cfg->iotlb_misses));
> > @@ -117,7 +138,7 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new)
> > *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> > tg, new->level);
> > trace_smmu_iotlb_insert(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> > - tg, new->level);
> > + tg, new->level, new->entry.translated_addr);
> > g_hash_table_insert(bs->iotlb, key, new);
> > }
> >
> > @@ -286,6 +307,27 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> > return NULL;
> > }
> >
> > +/* Return the correct table address based on configuration. */
> does the S2 translation for a PTE table?
The intention was to abstract that, and it just return the table address
with or with translation, but I can move the check outside and this
always translates.
> > +static inline int translate_table_s1(dma_addr_t *table_addr, SMMUTransCfg *cfg,
> > + SMMUPTWEventInfo *info, SMMUState *bs)
> > +{
> > + dma_addr_t addr = *table_addr;
> > + SMMUTLBEntry *cached_entry;
> > +
> > + if (cfg->stage != SMMU_NESTED) {
> > + return 0;
> > + }
> > +
> > + CALL_FUNC_CFG_S2(cfg, cached_entry, smmu_translate,
> > + bs, cfg, addr, IOMMU_RO, info);
> > +
> > + if (cached_entry) {
> > + *table_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
> > + return 0;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > /**
> > * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
> > * @cfg: translation config
> > @@ -301,7 +343,8 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> > */
> > static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> > dma_addr_t iova, IOMMUAccessFlags perm,
> > - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> > + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info,
> > + SMMUState *bs)
> > {
> > dma_addr_t baseaddr, indexmask;
> > SMMUStage stage = cfg->stage;
> > @@ -349,6 +392,10 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> > goto error;
> > }
> > baseaddr = get_table_pte_address(pte, granule_sz);
> > + /* In case of failure, retain stage-2 fault. */
> link to the doc?
Will do.
> > + if (translate_table_s1(&baseaddr, cfg, info, bs)) {
> let's avoid that call if S1 only
Will do.
>
> What about the error handling. I am not sure we are covering all the
> cases listed in 7.3.12 F_WALK_EABT for instance?
Yes, I think that can be used for some of the S2 fetch errors, it seems
also overlap with other events, for example:
F_WALK_EABT:
Translation of an IPA after successful stage 1 translation (or, in stage 2-only
configuration, an input IPA)
F_TRANSLATION
If translating an IPA for a transaction (whether by input to stage 2-only
configuration, or after successful stage 1 translation), CLASS == IN, and IPA is
provided.
I will have a deeper look and rework any missing events or add comments.
> > + goto error_no_stage;
> > + }
> > level++;
> > continue;
> > } else if (is_page_pte(pte, level)) {
> > @@ -384,7 +431,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> > tlbe->entry.translated_addr = gpa;
> > tlbe->entry.iova = iova & ~mask;
> > tlbe->entry.addr_mask = mask;
> > - tlbe->entry.perm = PTE_AP_TO_PERM(ap);
> > + tlbe->parent_perm = tlbe->entry.perm = PTE_AP_TO_PERM(ap);
> > tlbe->level = level;
> > tlbe->granule = granule_sz;
> > return 0;
> > @@ -393,6 +440,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> >
> > error:
> > info->stage = SMMU_STAGE_1;
> > +error_no_stage:
> > tlbe->entry.perm = IOMMU_NONE;
> > return -EINVAL;
> > }
> > @@ -505,7 +553,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> > tlbe->entry.translated_addr = gpa;
> > tlbe->entry.iova = ipa & ~mask;
> > tlbe->entry.addr_mask = mask;
> > - tlbe->entry.perm = s2ap;
> > + tlbe->parent_perm = tlbe->entry.perm = s2ap;
> > tlbe->level = level;
> > tlbe->granule = granule_sz;
> > return 0;
> > @@ -518,6 +566,28 @@ error:
> > return -EINVAL;
> > }
> >
> > +/* Combine 2 TLB enteries and return in tlbe. */
> entries.
> Would suggest combine
Will do.
> > +static void combine_tlb(SMMUTLBEntry *tlbe, SMMUTLBEntry *tlbe_s2,
> > + dma_addr_t iova, SMMUTransCfg *cfg)
> > +{
> > + if (cfg->stage == SMMU_NESTED) {
> > + tlbe->entry.addr_mask = MIN(tlbe->entry.addr_mask,
> > + tlbe_s2->entry.addr_mask);
> > + tlbe->entry.translated_addr = CACHED_ENTRY_TO_ADDR(tlbe_s2,
> > + tlbe->entry.translated_addr);
> > +
> > + tlbe->granule = MIN(tlbe->granule, tlbe_s2->granule);
> could you add a link to the spec so we can clearly check what this code
> is written against?
The spec doesn’t define the microarchitecture of the TLBs, it is an
implementation choice as long as it satisfies the TLB requirements
(mentioned in the cover letter)
- ARM ARM DDI 0487J.a: RLGSCG, RTVTYQ, RGNJPZ
- ARM IHI 0070F.b: 16.2 Caching
I will better document the TLB architecture.
> > + tlbe->level = MAX(tlbe->level, tlbe_s2->level);
> > + tlbe->entry.iova = iova & ~tlbe->entry.addr_mask;
> > + /* parent_perm has s2 perm while perm has s1 perm. */
> > + tlbe->parent_perm = tlbe_s2->entry.perm;
> > + return;
> > + }
> > +
> > + /* That was not nested, use the s2. */
> should be removed and tested ;-)
Ah, who needs testing :)
> > + memcpy(tlbe, tlbe_s2, sizeof(*tlbe));
> You shall avoid doing that memcpy if not necessary. I would advocate for
> separate paths for S2 and nested.
Will do.
> > +}
> > +
> > /**
> > * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
> need to update the above args desc
Will do.
> > *
> > @@ -530,28 +600,59 @@ error:
> > * return 0 on success
> > */
> > int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> > - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> > + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs)
> > {
> > - if (cfg->stage == SMMU_STAGE_1) {
> > - return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> > - } else if (cfg->stage == SMMU_STAGE_2) {
> > - /*
> > - * If bypassing stage 1(or unimplemented), the input address is passed
> > - * directly to stage 2 as IPA. If the input address of a transaction
> > - * exceeds the size of the IAS, a stage 1 Address Size fault occurs.
> > - * For AA64, IAS = OAS according to (IHI 0070.E.a) "3.4 Address sizes"
> > - */
> > - if (iova >= (1ULL << cfg->oas)) {
> > - info->type = SMMU_PTW_ERR_ADDR_SIZE;
> > - info->stage = SMMU_STAGE_1;
> > - tlbe->entry.perm = IOMMU_NONE;
> > - return -EINVAL;
> > + int ret = 0;
> > + SMMUTLBEntry tlbe_s2;
> > + dma_addr_t ipa = iova;
> > +
> > + if (cfg->stage & SMMU_STAGE_1) {
> > + ret = smmu_ptw_64_s1(cfg, iova, perm, tlbe, info, bs);
> > + if (ret) {
> > + return ret;
> > }
> > + /* This is the IPA for next stage.*/
> but you don't necessarily have the S2 enabled so this is not necessarily
> an IPA
I will update the comment.
> > + ipa = CACHED_ENTRY_TO_ADDR(tlbe, iova);
> > + }
> >
> > - return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
> > + /*
> > + * The address output from the translation causes a stage 1 Address Size
> > + * fault if it exceeds the range of the effective IPA size for the given CD.
> > + * If bypassing stage 1(or unimplemented), the input address is passed
> > + * directly to stage 2 as IPA. If the input address of a transaction
> > + * exceeds the size of the IAS, a stage 1 Address Size fault occurs.
> > + * For AA64, IAS = OAS according to (IHI 0070.E.a) "3.4 Address sizes"
> > + */
> > + if (ipa >= (1ULL << cfg->oas)) {
> in case we have S2 only, would be clearer to use ias instead (despite
> above comment say they are the same)
It was written this as it can be used in both cases where IAS is
limited by the CD, I will double check.
> > + info->type = SMMU_PTW_ERR_ADDR_SIZE;
> > + info->stage = SMMU_STAGE_1;
> What does the stage really means. That should be documented in the
> struct I think
This should update the s2 field in translation events (event->u.*.s2),
I will add a comment.
> > + tlbe->entry.perm = IOMMU_NONE;
> > + return -EINVAL;
> > }
> this check also is introduced for S1 only. If this is a fix this should
> be brought separately.
> Also the above comment refers to IPA. Does it also hold for S1 only. Is
> the check identical in that case?
I believe that is a fix, I will double check and add it in a separate patch.
> >
> > - g_assert_not_reached();
> > + if (cfg->stage & SMMU_STAGE_2) {
> > + ret = smmu_ptw_64_s2(cfg, ipa, perm, &tlbe_s2, info);
> > + if (ret) {
> > + return ret;
> > + }
> > + combine_tlb(tlbe, &tlbe_s2, iova, cfg);
> > + }
> I think I would prefer the S1, S2, nested cases cleary separated at the
> price of some code duplication. I am afraid serializing stuff make the
> code less maintainable.
> Also it is important the S1 case is not altered in terms of perf.
I see, I will have that in mind when splitting the patches.
> > +
> > + return ret;
> > +}
> > +
> > +static int validate_tlb_entry(SMMUTLBEntry *cached_entry, IOMMUAccessFlags flag,
> > + SMMUPTWEventInfo *info)
> > +{
> > + if ((flag & IOMMU_WO) && !(cached_entry->entry.perm &
> > + cached_entry->parent_perm & IOMMU_WO)) {
> > + info->type = SMMU_PTW_ERR_PERMISSION;
> > + info->stage = !(cached_entry->entry.perm & IOMMU_WO) ?
> > + SMMU_STAGE_1 :
> > + SMMU_STAGE_2;
> > + return -EINVAL;
> > + }
> > + return 0;
> > }
> >
> > SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
> > @@ -595,16 +696,14 @@ SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t addr,
> >
> > cached_entry = smmu_iotlb_lookup(bs, cfg, &tt_combined, aligned_addr);
> > if (cached_entry) {
> > - if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
> > - info->type = SMMU_PTW_ERR_PERMISSION;
> > - info->stage = cfg->stage;
> > + if (validate_tlb_entry(cached_entry, flag, info)) {
> > return NULL;
> > }
> > return cached_entry;
> > }
> >
> > cached_entry = g_new0(SMMUTLBEntry, 1);
> > - status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info);
> > + status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info, bs);
> > if (status) {
> > g_free(cached_entry);
> > return NULL;
> > diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> > index cc12924a84..5f23f0b963 100644
> > --- a/hw/arm/trace-events
> > +++ b/hw/arm/trace-events
> > @@ -15,9 +15,9 @@ smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
> > smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%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"
> > -smmu_iotlb_lookup_hit(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> > -smmu_iotlb_lookup_miss(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> > -smmu_iotlb_insert(uint16_t asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d"
> > +smmu_iotlb_lookup_hit(int asid, uint16_t vmid, uint64_t addr, uint64_t mask, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" mask=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> > +smmu_iotlb_lookup_miss(int asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> > +smmu_iotlb_insert(int asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level, uint64_t translate_addr) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d translate_addr=0x%"PRIx64
> >
> > # 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)"
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index 2772175115..03ff0f02ba 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -91,6 +91,7 @@ typedef struct SMMUTLBEntry {
> > IOMMUTLBEntry entry;
> > uint8_t level;
> > uint8_t granule;
> > + IOMMUAccessFlags parent_perm;
> > } SMMUTLBEntry;
> >
> > /* Stage-2 configuration. */
> > @@ -198,7 +199,7 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
> > * pair, according to @cfg translation config
> > */
> > int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> > - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info);
> > + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs);
> >
> >
> > /*
> To be honest this patch is quite complex to review. I would suggest you
> try to split it.
Yes, sorry about that :/ I thought it might be easier to have related
stuff in one patch, I will split it to 4-5 patches.
Thanks,
Mostafa
> Thanks
>
> Eric
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 07/13] hw/arm/smmu: Support nesting in the rest of commands
2024-04-18 14:48 ` Eric Auger
@ 2024-04-19 10:56 ` Mostafa Saleh
0 siblings, 0 replies; 24+ messages in thread
From: Mostafa Saleh @ 2024-04-19 10:56 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-arm, peter.maydell, qemu-devel, jean-philippe, alex.bennee,
maz, nicolinc, julien, richard.henderson, marcin.juszkiewicz
Hi Eric,
On Thu, Apr 18, 2024 at 04:48:39PM +0200, Eric Auger wrote:
> Hi Mostafa,
>
> On 4/8/24 16:08, Mostafa Saleh wrote:
> > Some commands need rework for nesting, as they used to assume S1
> > and S2 are mutually exclusive:
> >
> > - CMD_TLBI_NH_ASID: Consider VMID if stage-2 is supported
> > - CMD_TLBI_NH_ALL: Consider VMID if stage-2 is supported, otherwise
> > invalidate everything, this required a new vmid invalidation
> > function for stage-1 only (ASID >= 0)
> >
> > Also, rework trace events to reflect the new implementation.
>
> This does not apply for me. Could you share a branch or respin?
Oh, Sorry about that. I will address the previous comments and respin.
Thanks,
Mostafa
>
> Thank you in advance
>
> Eric
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> > hw/arm/smmu-common.c | 36 +++++++++++++++++++++++++++++-------
> > hw/arm/smmuv3.c | 31 +++++++++++++++++++++++++++++--
> > hw/arm/trace-events | 6 ++++--
> > include/hw/arm/smmu-common.h | 3 ++-
> > 4 files changed, 64 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index 8b9e59b24b..b1cf1303c6 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -148,13 +148,14 @@ void smmu_iotlb_inv_all(SMMUState *s)
> > g_hash_table_remove_all(s->iotlb);
> > }
> >
> > -static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
> > - gpointer user_data)
> > +static gboolean smmu_hash_remove_by_asid_vmid(gpointer key, gpointer value,
> > + gpointer user_data)
> > {
> > - int asid = *(int *)user_data;
> > + SMMUIOTLBPageInvInfo *info = (SMMUIOTLBPageInvInfo *)user_data;
> > SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
> >
> > - return SMMU_IOTLB_ASID(*iotlb_key) == asid;
> > + return (SMMU_IOTLB_ASID(*iotlb_key) == info->asid) &&
> > + (SMMU_IOTLB_VMID(*iotlb_key) == info->vmid);
> > }
> >
> > static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> > @@ -166,6 +167,16 @@ static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> > return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
> > }
> >
> > +static gboolean smmu_hash_remove_by_vmid_s1(gpointer key, gpointer value,
> > + gpointer user_data)
> > +{
> > + int vmid = *(int *)user_data;
> > + SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
> > +
> > + return (SMMU_IOTLB_VMID(*iotlb_key) == vmid) &&
> > + (SMMU_IOTLB_ASID(*iotlb_key) >= 0);
> > +}
> > +
> > static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
> > gpointer user_data)
> > {
> > @@ -259,10 +270,15 @@ void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
> > &info);
> > }
> >
> > -void smmu_iotlb_inv_asid(SMMUState *s, int asid)
> > +void smmu_iotlb_inv_asid_vmid(SMMUState *s, int asid, int vmid)
> > {
> > - trace_smmu_iotlb_inv_asid(asid);
> > - g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
> > + SMMUIOTLBPageInvInfo info = {
> > + .asid = asid,
> > + .vmid = vmid,
> > + };
> > +
> > + trace_smmu_iotlb_inv_asid_vmid(asid, vmid);
> > + g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid_vmid, &info);
> > }
> >
> > inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
> > @@ -271,6 +287,12 @@ inline void smmu_iotlb_inv_vmid(SMMUState *s, int vmid)
> > g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
> > }
> >
> > +inline void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid)
> > +{
> > + trace_smmu_iotlb_inv_vmid_s1(vmid);
> > + g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid_s1, &vmid);
> > +}
> > +
> > /* VMSAv8-64 Translation */
> >
> > /**
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 17bbd43c13..ece647b8bf 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -1280,25 +1280,52 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> > case SMMU_CMD_TLBI_NH_ASID:
> > {
> > int asid = CMD_ASID(&cmd);
> > + int vmid = -1;
> >
> > if (!STAGE1_SUPPORTED(s)) {
> > cmd_error = SMMU_CERROR_ILL;
> > break;
> > }
> >
> > + /*
> > + * VMID is only matched when stage 2 is supported for the Security
> > + * state corresponding to the command queue that the command was
> > + * issued in.
> > + * QEMU ignores the field by setting to -1, similarly to what STE
> > + * decoding does. And invalidation commands ignore VMID < 0.
> > + */
> > + if (STAGE2_SUPPORTED(s)) {
> > + vmid = CMD_VMID(&cmd);
> > + }
> > +
> > trace_smmuv3_cmdq_tlbi_nh_asid(asid);
> > smmu_inv_notifiers_all(&s->smmu_state);
> > - smmu_iotlb_inv_asid(bs, asid);
> > + smmu_iotlb_inv_asid_vmid(bs, asid, vmid);
> > break;
> > }
> > case SMMU_CMD_TLBI_NH_ALL:
> > + {
> > + int vmid = -1;
> > +
> > if (!STAGE1_SUPPORTED(s)) {
> > cmd_error = SMMU_CERROR_ILL;
> > break;
> > }
> > +
> > + /*
> > + * If stage-2 is supported, invalidate for this VMID only, otherwise
> > + * invalidate the whole thing, see SMMU_CMD_TLBI_NH_ASID()
> > + */
> > + if (STAGE2_SUPPORTED(s)) {
> > + vmid = CMD_VMID(&cmd);
> > + trace_smmuv3_cmdq_tlbi_nh(vmid);
> > + smmu_iotlb_inv_vmid_s1(bs, vmid);
> > + break;
> > + }
> > QEMU_FALLTHROUGH;
> > + }
> > case SMMU_CMD_TLBI_NSNH_ALL:
> > - trace_smmuv3_cmdq_tlbi_nh();
> > + trace_smmuv3_cmdq_tlbi_nsnh();
> > smmu_inv_notifiers_all(&s->smmu_state);
> > smmu_iotlb_inv_all(bs);
> > break;
> > diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> > index f5c361d96e..2556f4721a 100644
> > --- a/hw/arm/trace-events
> > +++ b/hw/arm/trace-events
> > @@ -11,8 +11,9 @@ smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint6
> > 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_inv_all(void) "IOTLB invalidate all"
> > -smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
> > +smmu_iotlb_inv_asid_vmid(int asid, uint16_t vmid) "IOTLB invalidate asid=%d vmid=%d"
> > smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%d"
> > +smmu_iotlb_inv_vmid_s1(uint16_t vmid) "IOTLB invalidate vmid=%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"
> > smmu_iotlb_lookup_hit(int asid, uint16_t vmid, uint64_t addr, uint64_t mask, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" mask=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> > @@ -47,7 +48,8 @@ smmuv3_cmdq_cfgi_cd(uint32_t sid) "sid=0x%x"
> > smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache HIT for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> > smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache MISS for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> > smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf, int stage) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d stage=%d"
> > -smmuv3_cmdq_tlbi_nh(void) ""
> > +smmuv3_cmdq_tlbi_nh(int vmid) "vmid=%d"
> > +smmuv3_cmdq_tlbi_nsnh(void) ""
> > smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
> > smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
> > smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index df166d8477..67db30e85b 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -226,8 +226,9 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
> > SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova,
> > uint8_t tg, uint8_t level);
> > void smmu_iotlb_inv_all(SMMUState *s);
> > -void smmu_iotlb_inv_asid(SMMUState *s, int asid);
> > +void smmu_iotlb_inv_asid_vmid(SMMUState *s, int asid, int vmid);
> > void smmu_iotlb_inv_vmid(SMMUState *s, int vmid);
> > +void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid);
> > void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> > uint8_t tg, uint64_t num_pages, uint8_t ttl);
> > void smmu_iotlb_inv_ipa(SMMUState *s, int vmid, dma_addr_t ipa, uint8_t tg,
>
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2024-04-19 10:57 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-08 14:08 [RFC PATCH v2 00/13] SMMUv3 nested translation support Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 01/13] hw/arm/smmu: Use enum for SMMU stage Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 02/13] hw/arm/smmu: Split smmuv3_translate() Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 03/13] hw/arm/smmu: Consolidate ASID and VMID types Mostafa Saleh
2024-04-18 7:56 ` Eric Auger
2024-04-08 14:08 ` [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table Mostafa Saleh
2024-04-18 12:51 ` Eric Auger
2024-04-19 10:23 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 05/13] hw/arm/smmu-common: Support nested translation Mostafa Saleh
2024-04-18 13:54 ` Eric Auger
2024-04-19 10:54 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 06/13] hw/arm/smmu: Support nesting in smmuv3_range_inval() Mostafa Saleh
2024-04-18 14:46 ` Eric Auger
2024-04-08 14:08 ` [RFC PATCH v2 07/13] hw/arm/smmu: Support nesting in the rest of commands Mostafa Saleh
2024-04-18 14:48 ` Eric Auger
2024-04-19 10:56 ` Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 08/13] hw/arm/smmuv3: Support nested SMMUs in smmuv3_notify_iova() Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 09/13] hw/arm/smmuv3: Support and advertise nesting Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 10/13] hw/arm/smmuv3: Advertise S2FWB Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 11/13] hw/arm/smmu: Refactor SMMU OAS Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 12/13] hw/arm/smmuv3: Add property for OAS Mostafa Saleh
2024-04-08 14:08 ` [RFC PATCH v2 13/13] hw/arm/virt: Set SMMU OAS based on CPU PARANGE Mostafa Saleh
2024-04-18 18:11 ` [RFC PATCH v2 00/13] SMMUv3 nested translation support Eric Auger
2024-04-19 9:22 ` Mostafa Saleh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.