Hi Eric, On 2026/3/3 18:14, Eric Auger wrote:
On 2/21/26 11:17 AM, Tao Tang wrote:Make smmuv3_cmdq_consume() security-state aware by passing sec_sid from smmu_writel() call sites (CR0/GERRORN/CMDQ_PROD paths), instead of hardcoding non-secure state. The related AddressSpace and MemTxAttrs are also obtained based on sec_sid. Also move CMD_SSEC legality checking to a single early check before command dispatch: secure commands are rejected on non-secure queues (including future Realm queue) with CERROR_ILL. This removes duplicated per-command checks in CFGI handlers. Finally, extend cmdq trace output with sec_sid so command processing can be correlated with the correct security bank. Signed-off-by: Tao Tang <[email protected]> --- hw/arm/smmuv3.c | 50 ++++++++++++++++++++++----------------------- hw/arm/trace-events | 2 +- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index e33a7babd1c..b2559e80f24 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -111,14 +111,14 @@ static void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t new_gerrorn) trace_smmuv3_write_gerrorn(toggled & pending, bank->gerrorn); }-static inline MemTxResult queue_read(SMMUQueue *q, Cmd *cmd)+static inline MemTxResult queue_read(SMMUQueue *q, Cmd *cmd, + AddressSpace *as, MemTxAttrs attrs) { dma_addr_t addr = Q_CONS_ENTRY(q); MemTxResult ret; int i;- ret = dma_memory_read(&address_space_memory, addr, cmd, sizeof(Cmd),- MEMTXATTRS_UNSPECIFIED); + ret = dma_memory_read(as, addr, cmd, sizeof(Cmd), attrs); if (ret != MEMTX_OK) { return ret; } @@ -1346,14 +1346,17 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage, } }-static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)+static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp, SMMUSecSID sec_sid) { SMMUState *bs = ARM_SMMU(s); SMMUCmdError cmd_error = SMMU_CERROR_NONE; - SMMUSecSID sec_sid = SMMU_SEC_SID_NS; SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid); SMMUQueue *q = &bank->cmdq; SMMUCommandType type = 0; + MemTxAttrs attrs = smmu_get_txattrs(sec_sid); + AddressSpace *as = smmu_get_address_space(bs, sec_sid); + /* Secure AddressSpace must be available, assert if not. */ + g_assert(as);pls remove that check
I'll remove all the unnecessary assertions and check it in lower level instead.
if (!smmuv3_cmdq_enabled(s, sec_sid)) {return 0; @@ -1369,18 +1372,30 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp) uint32_t pending = bank->gerror ^ bank->gerrorn; Cmd cmd;- trace_smmuv3_cmdq_consume(Q_PROD(q), Q_CONS(q),+ trace_smmuv3_cmdq_consume(sec_sid, Q_PROD(q), Q_CONS(q), Q_PROD_WRAP(q), Q_CONS_WRAP(q));if (FIELD_EX32(pending, GERROR, CMDQ_ERR)) {break; }- if (queue_read(q, &cmd) != MEMTX_OK) {+ if (queue_read(q, &cmd, as, attrs) != MEMTX_OK) { cmd_error = SMMU_CERROR_ABT; break; }+ /*+ * Secure Command on Non-secure Command queue, including Realm Command + * queue, is not allowed. CERROR_ILL will be raised according to + * (IHI 0070G.b) 4.1.6 Common command fields, Page 168. + */ + if (CMD_SSEC(&cmd)) { + if (sec_sid != SMMU_SEC_SID_S) {nit combine both checks
OK.
+ cmd_error = SMMU_CERROR_ILL; + break; + } + } + type = CMD_TYPE(&cmd);trace_smmuv3_cmdq_opcode(smmu_cmd_string(type));------------------------------<snip>------------------------------ ------------------------------<snip>------------------------------ diff --git a/hw/arm/trace-events b/hw/arm/trace-events index 4e360b3c0d3..ca8485c96af 100644 --- a/hw/arm/trace-events +++ b/hw/arm/trace-events @@ -35,7 +35,7 @@ smmuv3_trigger_irq(int irq) "irq=%d" smmuv3_write_gerror(uint32_t toggled, uint32_t gerror) "toggled=0x%x, new GERROR=0x%x" smmuv3_write_gerrorn(uint32_t acked, uint32_t gerrorn) "acked=0x%x, new GERRORN=0x%x" smmuv3_unhandled_cmd(uint32_t type) "Unhandled command type=%d" -smmuv3_cmdq_consume(uint32_t prod, uint32_t cons, uint8_t prod_wrap, uint8_t cons_wrap) "prod=%d cons=%d prod.wrap=%d cons.wrap=%d" +smmuv3_cmdq_consume(int sec_sid, uint32_t prod, uint32_t cons, uint8_t prod_wrap, uint8_t cons_wrap) "sec_sid=%d prod=%d cons=%d prod.wrap=%d cons.wrap=%d" smmuv3_cmdq_opcode(const char *opcode) "<--- %s" smmuv3_cmdq_consume_out(uint32_t prod, uint32_t cons, uint8_t prod_wrap, uint8_t cons_wrap) "prod:%d, cons:%d, prod_wrap:%d, cons_wrap:%d " smmuv3_cmdq_consume_error(const char *cmd_name, uint8_t cmd_error) "Error on %s command execution: %d"Besides looks good to me Reviewed-by: Eric Auger <[email protected]> Eric
Thanks, Tao
