Hi Tao,
On 8/13/26 6:25 PM, Tao Tang wrote:
> Add access control for SMMU_STRTAB_BASE and SMMU_CR2 registers to
> ensure they can only be modified when the SMMU is disabled.
About SMMU_STRTAB_BASE "
SMMU_STRTAB_BASE is Guarded by SMMU_CR0.SMMUEN and must only be written when
SMMU_CR0.SMMUEN == 0
.../..
In SMMUv3.2 and later, a write while SMMU_CR0.SMMUEN == 1 is IGNORED.
../..
Accessible at offset 0x0080 from SMMUv3_PAGE_0
• When SMMU_IDR1.TABLES_PRESET == 1, accesses to this register are RO.
• When SMMU_CR0.SMMUEN == 0 and SMMU_CR0ACK.SMMUEN == 0, accesses to
this register are
RW.
• Otherwise, accesses to this register are RO.
"
About SMMU_CR2:
"
This register is made read-only when the associated SMMU_CR0.SMMUEN is
Updated to 1. This register must
only be changed when SMMU_CR0.SMMUEN == 0.
A write to this register after SMMU_CR0.SMMUEN has been changed but
before its Update completes is
CONSTRAINED UNPREDICTABLE and has one of the following behaviors:
• Apply the new value.
• Ignore the write. In SMMUv3.2 and later, this is the only permitted
behavior.
"
>
> This implements:
> - smmuv3_smmu_disabled_stable(): Check whether the SMMU is in a stable
> disabled state (CR0.SMMUEN == 0 and CR0ACK.SMMUEN == 0);
> - smmu_strtab_base_writable(): returns true only when IDR1.TABLES_PRESET==0
> and SMMU is completely disabled.
>
> Additionally, mask reserved bits on writes to SMMU_STRTAB_BASE using
> SMMU_STRTAB_BASE_RESERVED.
>
> Fixes: fae4be38b35d ("hw/arm/smmuv3: Implement MMIO write operations")
> Signed-off-by: Tao Tang <[email protected]>
> Reviewed-by: Pierrick Bouvier <[email protected]>
> ---
> hw/arm/smmuv3.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 628911d3f21..de8ca7cda48 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1484,6 +1484,36 @@ smmu_gerror_irq_cfg_writable(SMMUv3State *s,
> SMMUSecSID sec_sid)
> return smmu_irq_cfg_writable(s, sec_sid, SMMU_IRQ_GERROR);
> }
>
> +static inline int smmuv3_get_cr0ack_smmuen(SMMUv3State *s, SMMUSecSID
> sec_sid)
> +{
> + /*
> + * CR0, CR0ACK, S_CR0 and S_CR0ACK are bit-layout compatible, so we reuse
> + * the CR0 field definitions and only switch banks via sec_sid to reduce
> + * code duplication. Also the other bits in CR0/CR0ACK are relevant here.
> + */
> + return FIELD_EX32(s->bank[sec_sid].cr0ack, CR0, SMMUEN);
> +}
> +
> +/* Check if SMMU is disabled in stable status */
> +static inline bool smmuv3_smmu_disabled_stable(SMMUv3State *s, SMMUSecSID
> sec_sid)
s/stable/updated?
SMMU_CR0 spec quote:
> +{
> + int cr0_smmuen = smmu_enabled(s, sec_sid);
> + int cr0ack_smmuen = smmuv3_get_cr0ack_smmuen(s, sec_sid);
> + return (cr0_smmuen == 0 && cr0ack_smmuen == 0);
Each field in this register has a corresponding field in SMMU_CR0ACK. An
individual field is described as
Updated after the value of the field observed in SMMU_CR0ACK matches the
value that was written to the field
in SMMU_CR0.
Besides I don't if its wort that complexity here because the update is
done in the emulation
case A_CR0:
bank->cr[0] = data;
bank->cr0ack = data & ~SMMU_CR0_RESERVED;
Just adding a coment would be enough I think
I would split this patch into 2 sparate patches, first one CR2 and then
STRTAB_BASE
> +}
> +
> +/* Check if STRTAB_BASE register is writable */
> +static bool smmu_strtab_base_writable(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> + /* SMMU_IDR1.TABLES_PRESET applies to both modeled interfaces. */
> + if (FIELD_EX32(s->bank[SMMU_SEC_SID_NS].idr[1], IDR1, TABLES_PRESET)) {
> + return false;
> + }
> +
> + /* Check SMMUEN conditions for the specific security domain */
> + return smmuv3_smmu_disabled_stable(s, sec_sid);
> +}
> +
> static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp, SMMUSecSID
> sec_sid)
> {
> SMMUState *bs = ARM_SMMU(s);
> @@ -1801,7 +1831,14 @@ static MemTxResult smmu_writell(SMMUv3State *s, hwaddr
> offset,
> bank->gerror_irq_cfg0 = data & SMMU_GERROR_IRQ_CFG0_RESERVED;
> return MEMTX_OK;
> case A_STRTAB_BASE:
> - bank->strtab_base = data;
> + if (!smmu_strtab_base_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "STRTAB_BASE write ignored: register is RO\n");
> + return MEMTX_OK;
> + }
> +
> + /* Clear reserved bits according to spec */
> + bank->strtab_base = data & SMMU_STRTAB_BASE_RESERVED;
> return MEMTX_OK;
> case A_CMDQ_BASE:
> bank->cmdq.base = data;
> @@ -1852,7 +1889,15 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr
> offset,
> bank->cr[1] = data;
> break;
> case A_CR2:
> - bank->cr[2] = data;
> + if (smmuv3_smmu_disabled_stable(s, reg_sec_sid)) {
> + /* Allow write: SMMUEN is 0 in both CR0 and CR0ACK */
> + bank->cr[2] = data;
> + } else {
> + /* CONSTRAINED UNPREDICTABLE behavior: Ignore this write */
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "CR2 write ignored: register is read-only when "
> + "CR0.SMMUEN or CR0ACK.SMMUEN is set\n");
not sure those logs are requested.
> + }
> break;
> case A_IRQ_CTRL:
> bank->irq_ctrl = data;
> @@ -1915,12 +1960,32 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr
> offset,
> }
> break;
> case A_STRTAB_BASE: /* 64b */
> + if (!smmu_strtab_base_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "STRTAB_BASE write ignored: register is RO\n");
> + return MEMTX_OK;
> + }
> +
> + data &= SMMU_STRTAB_BASE_RESERVED;
> bank->strtab_base = deposit64(bank->strtab_base, 0, 32, data);
> break;
> case A_STRTAB_BASE + 4:
> + if (!smmu_strtab_base_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "STRTAB_BASE + 4 write ignored: register is RO\n");
> + return MEMTX_OK;
> + }
> +
> + data &= SMMU_STRTAB_BASE_RESERVED;
> bank->strtab_base = deposit64(bank->strtab_base, 32, 32, data);
> break;
> case A_STRTAB_BASE_CFG:
> + if (!smmu_strtab_base_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "STRTAB_BASE_CFG write ignored: register is RO\n");
> + return MEMTX_OK;
> + }
> +
> bank->strtab_base_cfg = data;
> if (FIELD_EX32(data, STRTAB_BASE_CFG, FMT) == 1) {
> bank->sid_split = FIELD_EX32(data, STRTAB_BASE_CFG, SPLIT);
Thanks
Eric
