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 <tangtao1634@phytium.com.cn>
---
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);
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) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+ }
+
type = CMD_TYPE(&cmd);
trace_smmuv3_cmdq_opcode(smmu_cmd_string(type));
@@ -1400,11 +1415,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
uint32_t sid = CMD_SID(&cmd);
SMMUDevice *sdev = smmu_find_sdev(bs, sid);
- if (CMD_SSEC(&cmd)) {
- cmd_error = SMMU_CERROR_ILL;
- break;
- }
-
if (!sdev) {
break;
}
@@ -1424,11 +1434,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
uint8_t range = CMD_STE_RANGE(&cmd);
SMMUSIDRange sid_range;
- if (CMD_SSEC(&cmd)) {
- cmd_error = SMMU_CERROR_ILL;
- break;
- }
-
mask = (1ULL << (range + 1)) - 1;
sid_range.start = sid & ~mask;
sid_range.end = sid_range.start + mask;
@@ -1447,11 +1452,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
uint32_t sid = CMD_SID(&cmd);
SMMUDevice *sdev = smmu_find_sdev(bs, sid);
- if (CMD_SSEC(&cmd)) {
- cmd_error = SMMU_CERROR_ILL;
- break;
- }
-
if (!sdev) {
break;
}
@@ -1666,7 +1666,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
bank->cr[0] = data;
bank->cr0ack = data & ~SMMU_CR0_RESERVED;
/* in case the command queue has been enabled */
- smmuv3_cmdq_consume(s, &local_err);
+ smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
break;
case A_CR1:
bank->cr[1] = data;
@@ -1683,7 +1683,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
* By acknowledging the CMDQ_ERR, SW may notify cmds can
* be processed again
*/
- smmuv3_cmdq_consume(s, &local_err);
+ smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
break;
case A_GERROR_IRQ_CFG0: /* 64b */
bank->gerror_irq_cfg0 = deposit64(bank->gerror_irq_cfg0, 0, 32, data);
@@ -1733,7 +1733,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
break;
case A_CMDQ_PROD:
bank->cmdq.prod = data;
- smmuv3_cmdq_consume(s, &local_err);
+ smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
break;
case A_CMDQ_CONS:
bank->cmdq.cons = data;
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"
--
2.34.1
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 <tangtao1634@phytium.com.cn>
> ---
> 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
>
> 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
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> + }
> +
> type = CMD_TYPE(&cmd);
>
> trace_smmuv3_cmdq_opcode(smmu_cmd_string(type));
> @@ -1400,11 +1415,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
> uint32_t sid = CMD_SID(&cmd);
> SMMUDevice *sdev = smmu_find_sdev(bs, sid);
>
> - if (CMD_SSEC(&cmd)) {
> - cmd_error = SMMU_CERROR_ILL;
> - break;
> - }
> -
> if (!sdev) {
> break;
> }
> @@ -1424,11 +1434,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
> uint8_t range = CMD_STE_RANGE(&cmd);
> SMMUSIDRange sid_range;
>
> - if (CMD_SSEC(&cmd)) {
> - cmd_error = SMMU_CERROR_ILL;
> - break;
> - }
> -
> mask = (1ULL << (range + 1)) - 1;
> sid_range.start = sid & ~mask;
> sid_range.end = sid_range.start + mask;
> @@ -1447,11 +1452,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
> uint32_t sid = CMD_SID(&cmd);
> SMMUDevice *sdev = smmu_find_sdev(bs, sid);
>
> - if (CMD_SSEC(&cmd)) {
> - cmd_error = SMMU_CERROR_ILL;
> - break;
> - }
> -
> if (!sdev) {
> break;
> }
> @@ -1666,7 +1666,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> bank->cr[0] = data;
> bank->cr0ack = data & ~SMMU_CR0_RESERVED;
> /* in case the command queue has been enabled */
> - smmuv3_cmdq_consume(s, &local_err);
> + smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
> break;
> case A_CR1:
> bank->cr[1] = data;
> @@ -1683,7 +1683,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> * By acknowledging the CMDQ_ERR, SW may notify cmds can
> * be processed again
> */
> - smmuv3_cmdq_consume(s, &local_err);
> + smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
> break;
> case A_GERROR_IRQ_CFG0: /* 64b */
> bank->gerror_irq_cfg0 = deposit64(bank->gerror_irq_cfg0, 0, 32, data);
> @@ -1733,7 +1733,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> break;
> case A_CMDQ_PROD:
> bank->cmdq.prod = data;
> - smmuv3_cmdq_consume(s, &local_err);
> + smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
> break;
> case A_CMDQ_CONS:
> bank->cmdq.cons = data;
> 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 <eric.auger@redhat.com>
Eric
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 <tangtao1634@phytium.com.cn>
>> ---
>> 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 <eric.auger@redhat.com>
>
> Eric
Thanks,
Tao
On 2/21/26 2: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.
>
Sounds great!
> Finally, extend cmdq trace output with sec_sid so command processing
> can be correlated with the correct security bank.
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> ---
> 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);
>
This will be automatically be handled if we check in smmu_base_realize
that secure smmu support should be available, as mentioned on one of the
previous patch.
> 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) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> + }
> +
> type = CMD_TYPE(&cmd);
>
> trace_smmuv3_cmdq_opcode(smmu_cmd_string(type));
> @@ -1400,11 +1415,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
> uint32_t sid = CMD_SID(&cmd);
> SMMUDevice *sdev = smmu_find_sdev(bs, sid);
>
> - if (CMD_SSEC(&cmd)) {
> - cmd_error = SMMU_CERROR_ILL;
> - break;
> - }
> -
> if (!sdev) {
> break;
> }
> @@ -1424,11 +1434,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
> uint8_t range = CMD_STE_RANGE(&cmd);
> SMMUSIDRange sid_range;
>
> - if (CMD_SSEC(&cmd)) {
> - cmd_error = SMMU_CERROR_ILL;
> - break;
> - }
> -
> mask = (1ULL << (range + 1)) - 1;
> sid_range.start = sid & ~mask;
> sid_range.end = sid_range.start + mask;
> @@ -1447,11 +1452,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp)
> uint32_t sid = CMD_SID(&cmd);
> SMMUDevice *sdev = smmu_find_sdev(bs, sid);
>
> - if (CMD_SSEC(&cmd)) {
> - cmd_error = SMMU_CERROR_ILL;
> - break;
> - }
> -
> if (!sdev) {
> break;
> }
> @@ -1666,7 +1666,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> bank->cr[0] = data;
> bank->cr0ack = data & ~SMMU_CR0_RESERVED;
> /* in case the command queue has been enabled */
> - smmuv3_cmdq_consume(s, &local_err);
> + smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
> break;
> case A_CR1:
> bank->cr[1] = data;
> @@ -1683,7 +1683,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> * By acknowledging the CMDQ_ERR, SW may notify cmds can
> * be processed again
> */
> - smmuv3_cmdq_consume(s, &local_err);
> + smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
> break;
> case A_GERROR_IRQ_CFG0: /* 64b */
> bank->gerror_irq_cfg0 = deposit64(bank->gerror_irq_cfg0, 0, 32, data);
> @@ -1733,7 +1733,7 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> break;
> case A_CMDQ_PROD:
> bank->cmdq.prod = data;
> - smmuv3_cmdq_consume(s, &local_err);
> + smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
> break;
> case A_CMDQ_CONS:
> bank->cmdq.cons = data;
> 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"
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
© 2016 - 2026 Red Hat, Inc.