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 <smost...@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
