Hi Jim, Thanks for sending it.
On Fri, Oct 16, 2026 at 10:40:51AM +0100, Jim MacArthur wrote: > From: Pierrick Bouvier <[email protected]> > > Adds all the non-optional SMMUv3.3 Root registers. These registers > do nothing at the moment, but RW registers can be written and all > registers can be read. > > Based on original work by Pierrick Bouvier <[email protected]> > > Signed-off-by: Jim MacArthur <[email protected]> > --- > hw/arm/smmuv3.c | 229 > +++++++++++++++++++++++++++++++++++++++++ > include/hw/arm/smmuv3-common.h | 22 ++++ > include/hw/arm/smmuv3.h | 13 +++ > 3 files changed, 264 insertions(+) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index ed19536a4d..9d9bc2601b 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -316,6 +316,11 @@ static void smmuv3_init_id_regs(SMMUv3State *s) > s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1); > s->aidr = 0x1; > smmuv3_accel_idr_override(s); > + > + /* Initialize Root bank */ > + s->root.idr0 = FIELD_DP32(s->root.idr0, ROOT_IDR0, ROOT_IMPL, 1); > + /* Set this to 1 when SMMU Realms are implemented */ > + s->root.idr0 = FIELD_DP32(s->root.idr0, ROOT_IDR0, REALM_IMPL, 0); > } > > bool smmuv3_ats_enabled(SMMUv3State *s) > @@ -1769,6 +1774,111 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr > offset, > return MEMTX_OK; > } > > +/* > + * Check we are from the Root PA space to read from or write to the > + * SMMU Root register area. > + */ > +static bool smmu_check_root_access(SMMUv3State *s, MemTxAttrs attrs, > + hwaddr offset, bool is_read) Indentation is off ---------------------^ I think it makes sense also to call this function smmu_root_access(), but no strong preference here. > +{ > + /* Check if the access is from Root PA */ > + if (!(attrs.space == ARMSS_Root)) { Could we use the comparison operator (!=) instead of the logical one here? I think it's clearer than a negation. > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: Non-root %s attempt at offset 0x%" PRIx64 " (%s)\n", > + __func__, is_read ? "read" : "write", offset, > + is_read ? "RAZ" : "WI"); > + return false; > + } > + return true; > +} > + > +static MemTxResult smmu_root_writel(SMMUv3State *s, hwaddr offset, > + uint64_t data, MemTxAttrs attrs) > +{ > + switch (offset) { > + case A_ROOT_IDR0: > + case A_ROOT_IIDR: > + case A_ROOT_CR0ACK: > + /* RO registers, write ignored */ > + return MEMTX_OK; > + case A_ROOT_TLBI_CTRL: > + /* Optional, not implemented. Present if IDR0.RGPTM == 1 */ > + return MEMTX_OK; > + case A_ROOT_GPT_BASE_UPDATE: > + /* Unimplemented at present; has side effects */ > + return MEMTX_OK; > + case A_ROOT_CR0: > + s->root.cr0 = data; > + s->root.cr0ack = s->root.cr0; > + return MEMTX_OK; > + default: > + qemu_log_mask(LOG_UNIMP, > + "%s unhandled 32-bit access at 0x%"PRIx64" (WI)\n", > + __func__, offset); > + /* > + * Locations not associated with a register (including registers > that we > + * have not implemented yet) are RES0, and will follow RAZ/WI > behaviour. > + */ > + return MEMTX_OK; > + } > +} > + > +static MemTxResult smmu_root_writell(SMMUv3State *s, hwaddr offset, > + uint64_t data, MemTxAttrs attrs) > +{ > + switch (offset) { > + case A_ROOT_GPT_BASE: > + s->root.gpt_base = data; > + return MEMTX_OK; > + case A_ROOT_GPT_BASE_CFG: > + s->root.gpt_base_cfg = data; > + return MEMTX_OK; > + case A_ROOT_GPF_FAR: > + s->root.gpf_far = data; > + return MEMTX_OK; > + case A_ROOT_GPT_CFG_FAR: > + s->root.gpt_cfg_far = data; > + return MEMTX_OK; > + case A_ROOT_TLBI: > + /* Optional, not implemented. Present if ROOT_IDR0.RGPTM == 1 */ > + return MEMTX_OK; > + case A_ROOT_GPT_BASE2: > + s->root.gpt_base2 = data; > + return MEMTX_OK; > + case A_ROOT_GPCBW: > + /* Optional, not implemented. Present if ROOT_IDR0.GPTS == 1 */ > + return MEMTX_OK; > + default: > + qemu_log_mask(LOG_UNIMP, > + "%s unhandled 64-bit access at 0x%"PRIx64" (WI)\n", > + __func__, offset); > + /* RES0; See smmu_root_writel */ > + return MEMTX_OK; > + } > +} We might have some issues down the lane since TF-A seems to rely on the spec that says that 64b registers can be written in 32b halves, since I see in the TF-A code, drivers/arm/smmu/smmu_v3.c, in smmuv3_init(): [...] /* * TODO: SMMU_ROOT_GPT_BASE_CFG is 64b in the spec, * but SMMU model only accepts 32b access. */ mmio_write_32(smmu_base + SMMU_ROOT_GPT_BASE_CFG, gpccr_el3); But for now it's ok. We can revisit it later. > + > +static MemTxResult smmu_write_mmio_root(SMMUv3State *s, hwaddr offset, > + uint64_t data, unsigned size, > + MemTxAttrs attrs) > +{ > + MemTxResult r; > + > + switch (size) { > + case 8: > + r = smmu_root_writell(s, offset, data, attrs); > + break; > + case 4: > + r = smmu_root_writel(s, offset, data, attrs); > + break; > + default: > + r = MEMTX_ERROR; > + break; > + } > + > + trace_smmuv3_write_mmio(offset, data, size, r); > + return r; > +} > + > static MemTxResult smmu_write_mmio(void *opaque, hwaddr offset, uint64_t > data, > unsigned size, MemTxAttrs attrs) > { > @@ -1776,6 +1886,18 @@ static MemTxResult smmu_write_mmio(void *opaque, > hwaddr offset, uint64_t data, > SMMUv3State *s = ARM_SMMUV3(sys); > MemTxResult r; > > + if (offset >= SMMU_ROOT_REG_START) { > + if (!smmu_check_root_access(s, attrs, offset, false)) { > + /* > + * As per 6.2.8, accesses in any PA space other > + * than Root are completed as RAZ/WI. > + */ > + trace_smmuv3_write_mmio(offset, data, size, MEMTX_OK); > + return MEMTX_OK; > + } > + return smmu_write_mmio_root(s, offset, data, size, attrs); > + } > + > /* CONSTRAINED UNPREDICTABLE choice to have page0/1 be exact aliases */ > offset &= ~0x10000; > > @@ -1918,6 +2040,100 @@ static MemTxResult smmu_readl(SMMUv3State *s, hwaddr > offset, > } > } > > +static MemTxResult smmu_root_readl(SMMUv3State *s, hwaddr offset, > + uint64_t *data, MemTxAttrs attrs) > +{ > + switch (offset) { > + case A_ROOT_IDR0: > + *data = s->root.idr0; > + return MEMTX_OK; > + case A_ROOT_IIDR: > + *data = s->root.iidr; > + return MEMTX_OK; > + case A_ROOT_TLBI_CTRL: > + /* Optional, not implemented. Present if ROOT_IDR0.RGPTM == 1 */ > + *data = 0; > + return MEMTX_OK; > + case A_ROOT_GPT_BASE_UPDATE: > + /* Not a regular register; reads as 0 once update is complete */ > + *data = 0; > + return MEMTX_OK; > + case A_ROOT_CR0: > + *data = s->root.cr0; > + return MEMTX_OK; > + case A_ROOT_CR0ACK: > + *data = s->root.cr0ack; > + return MEMTX_OK; > + default: > + qemu_log_mask(LOG_UNIMP, > + "%s unhandled 32-bit access at 0x%"PRIx64" (RAZ)\n", > + __func__, offset); > + *data = 0; > + /* RES0; See smmu_root_writel */ > + return MEMTX_OK; > + } > +} > + > +static MemTxResult smmu_root_readll(SMMUv3State *s, hwaddr offset, > + uint64_t *data, MemTxAttrs attrs) > +{ > + switch (offset) { > + case A_ROOT_GPT_BASE: > + *data = s->root.gpt_base; > + return MEMTX_OK; > + case A_ROOT_GPT_BASE_CFG: > + *data = s->root.gpt_base_cfg; > + return MEMTX_OK; > + case A_ROOT_GPF_FAR: > + *data = s->root.gpf_far; > + return MEMTX_OK; > + case A_ROOT_GPT_CFG_FAR: > + *data = s->root.gpt_cfg_far; > + return MEMTX_OK; > + case A_ROOT_TLBI: > + /* Optional, not implemented. Present if ROOT_IDR0.RGPTM == 1 */ > + *data = 0; > + return MEMTX_OK; > + case A_ROOT_GPT_BASE2: > + *data = s->root.gpt_base2; > + return MEMTX_OK; > + case A_ROOT_GPCBW: > + /* Optional, not implemented. Present if ROOT_IDR0.GPTS == 0 */ GPTS == 1 here? -----^ > + *data = 0; > + return MEMTX_OK; > + default: > + qemu_log_mask(LOG_UNIMP, > + "%s unhandled 64-bit access at 0x%"PRIx64" (RAZ)\n", > + __func__, offset); > + *data = 0; > + /* RES0; See smmu_root_writel */ > + return MEMTX_OK; > + } > +} > + > +static MemTxResult smmu_read_mmio_root(SMMUv3State *s, hwaddr offset, > + uint64_t *data, unsigned size, > + MemTxAttrs attrs) > +{ > + MemTxResult r; > + > + switch (size) { > + case 8: > + r = smmu_root_readll(s, offset, data, attrs); > + break; > + case 4: > + r = smmu_root_readl(s, offset, data, attrs); > + break; > + default: > + r = MEMTX_ERROR; > + break; > + } > + > + trace_smmuv3_read_mmio(offset, *data, size, r); > + return r; > +} > + > + > static MemTxResult smmu_read_mmio(void *opaque, hwaddr offset, uint64_t > *data, > unsigned size, MemTxAttrs attrs) > { > @@ -1925,6 +2141,19 @@ static MemTxResult smmu_read_mmio(void *opaque, hwaddr > offset, uint64_t *data, > SMMUv3State *s = ARM_SMMUV3(sys); > MemTxResult r; > > + if (offset >= SMMU_ROOT_REG_START) { > + if (!smmu_check_root_access(s, attrs, offset, true)) { > + /* > + * As per 6.2.8, accesses in any PA space other > + * than Root are completed as RAZ/WI. > + */ > + trace_smmuv3_read_mmio(offset, *data, size, MEMTX_OK); > + return MEMTX_OK; > + } > + > + return smmu_read_mmio_root(s, offset, data, size, attrs); > + } > + > /* CONSTRAINED UNPREDICTABLE choice to have page0/1 be exact aliases */ > offset &= ~0x10000; > > diff --git a/include/hw/arm/smmuv3-common.h b/include/hw/arm/smmuv3-common.h > index 4609975edf..a538334066 100644 > --- a/include/hw/arm/smmuv3-common.h > +++ b/include/hw/arm/smmuv3-common.h > @@ -411,6 +411,28 @@ REG32(EVENTQ_IRQ_CFG2, 0xbc) > > #define A_IDREGS 0xfd0 > > +#define SMMU_ROOT_REG_START 0x20000 The base address for the Root Control Page is implementation defined, hence 0x20000 is not always true and depends on the machine/platform. We could add property to set it externaly but it would also complicate unnecessarily the offset math. So I think it's ok to follow what FVP is doing. Please just add a note saying it's impl. defined so it might require a tweak in the future. I think we can follow TF-A code here and rename this macro. So how about renaming it to: SMMUV3_ROOT_REG_OFFSET ? > + > +REG32(ROOT_IDR0, 0x0) > + FIELD(ROOT_IDR0, ROOT_IMPL, 0, 1) > + FIELD(ROOT_IDR0, BGTPM, 1, 1) Typo: BGPTM ------------^ > + FIELD(ROOT_IDR0, RGPTM, 2, 1) > + FIELD(ROOT_IDR0, REALM_IMPL, 3, 1) > + FIELD(ROOT_IDR0, BA_REALM, 22, 10) > + > +REG32(ROOT_IIDR, 0x8) > +REG32(ROOT_CR0, 0x20) > +REG32(ROOT_CR0ACK, 0x24) > +REG64(ROOT_GPT_BASE, 0x28) > +REG64(ROOT_GPT_BASE_CFG, 0x30) > +REG64(ROOT_GPF_FAR, 0x38) > +REG64(ROOT_GPT_CFG_FAR, 0x40) > +REG64(ROOT_TLBI, 0x50) > +REG32(ROOT_TLBI_CTRL, 0x58) > +REG64(ROOT_GPT_BASE2, 0x60) > +REG32(ROOT_GPT_BASE_UPDATE, 0x68) > +REG32(ROOT_GPCBW, 0x70) > + > /* Commands */ > > typedef enum SMMUCommandType { > diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h > index d39fe8850b..e2708e13fe 100644 > --- a/include/hw/arm/smmuv3.h > +++ b/include/hw/arm/smmuv3.h > @@ -34,6 +34,18 @@ typedef struct SMMUQueue { > uint8_t log2size; > } SMMUQueue; > > +typedef struct SMMUv3RootRegBank { How about naming this struct simply SMMUv3Root? > + uint32_t idr0; > + uint32_t iidr; > + uint32_t cr0; > + uint32_t cr0ack; > + uint64_t gpt_base; > + uint64_t gpt_base_cfg; > + uint64_t gpf_far; > + uint64_t gpt_cfg_far; > + uint64_t gpt_base2; Looking at ARM IHI 0070 H.a Non-confidential spec I don't see 0x0068 SMMU_ROOT_GPT_BASE_UPDATE 32-bit, R/W and 0x0070 SMMU_ROOT_GPCBW 64-bit, R/W as Optional like I see SMMU_ROOT_TLBI and SMMU_ROOT_TLBI_CTRL. Should they be declared as well? > +} SMMUv3RootRegBank; > + > struct SMMUv3State { > SMMUState smmu_state; > > @@ -80,6 +92,7 @@ struct SMMUv3State { > OnOffAuto cmdqv; > > Notifier machine_done; > + SMMUv3RootRegBank root; > }; > > typedef enum { > > -- > 2.43.0 > > General comments: smmu_read_mmio_root smmu_write_mmio_root I think we can drop the "mmio_" for both functions. It's ok to have "mmio_" in the "trace_smmuv3_write_mmio" trace function because afaics it has a generic use, so its use is not tied to any bank. I think we're missing to reset some registers in smmuv3_reset()? Like: s->root.cr0 = 0; s->root.cr0ack = 0; s->root.gpf_far = 0; s->root.gpt_cfg_far = 0; Cheers, Gustavo
