Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
tied to the correct register bank.
Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
and event logs always show which security interface emitted the record.
To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
identified as soon as an event record is built.
Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
hw/arm/smmuv3-internal.h | 1 +
hw/arm/smmuv3.c | 20 +++++++++++++-------
hw/arm/trace-events | 2 +-
include/hw/arm/smmu-common.h | 1 +
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 866d62257e3..a1071f7b689 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
/* Encode an event record */
typedef struct SMMUEventInfo {
+ SMMUSecSID sec_sid;
SMMUEventType type;
uint32_t sid;
bool recorded;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 2c107724e77..3438adcecd2 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
return MEMTX_OK;
}
-static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
+static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
+ Evt *evt)
{
- SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
SMMUQueue *q = &bank->eventq;
MemTxResult r;
@@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
{
Evt evt = {};
MemTxResult r;
- SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
+ SMMUSecSID sec_sid = info->sec_sid;
+ g_assert(sec_sid < SMMU_SEC_SID_NUM);
if (!smmuv3_eventq_enabled(s, sec_sid)) {
return;
@@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
g_assert_not_reached();
}
- trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
- r = smmuv3_write_eventq(s, &evt);
+ trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
+ info->sid);
+ r = smmuv3_write_eventq(s, sec_sid, &evt);
if (r != MEMTX_OK) {
smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
}
@@ -917,6 +919,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
100 * sdev->cfg_cache_hits /
(sdev->cfg_cache_hits + sdev->cfg_cache_misses));
cfg = g_new0(SMMUTransCfg, 1);
+ cfg->sec_sid = SMMU_SEC_SID_NS;
if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
g_hash_table_insert(bc->configs, sdev, cfg);
@@ -1074,7 +1077,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
SMMUEventInfo event = {.type = SMMU_EVT_NONE,
.sid = sid,
- .inval_ste_allowed = false};
+ .inval_ste_allowed = false,
+ .sec_sid = sec_sid};
SMMUTranslationStatus status;
SMMUTransCfg *cfg = NULL;
IOMMUTLBEntry entry = {
@@ -1176,7 +1180,9 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
uint64_t num_pages, int stage)
{
SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
- SMMUEventInfo eventinfo = {.inval_ste_allowed = true};
+ SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
+ SMMUEventInfo eventinfo = {.sec_sid = sec_sid,
+ .inval_ste_allowed = true};
SMMUTransCfg *cfg = smmuv3_get_config(sdev, &eventinfo);
IOMMUTLBEvent event;
uint8_t granule;
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 8135c0c7344..9c2cc131ab4 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -40,7 +40,7 @@ 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"
smmuv3_write_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
-smmuv3_record_event(const char *type, uint32_t sid) "%s sid=0x%x"
+smmuv3_record_event(int sec_sid, const char *type, uint32_t sid) "sec_sid=%d %s sid=0x%x"
smmuv3_find_ste(uint16_t sid, uint32_t features, uint16_t sid_split) "sid=0x%x features:0x%x, sid_split:0x%x"
smmuv3_find_ste_2lvl(uint64_t strtab_base, uint64_t l1ptr, int l1_ste_offset, uint64_t l2ptr, int l2_ste_offset, int max_l2_ste) "strtab_base:0x%"PRIx64" l1ptr:0x%"PRIx64" l1_off:0x%x, l2ptr:0x%"PRIx64" l2_off:0x%x max_l2_ste:%d"
smmuv3_get_ste(uint64_t addr) "STE addr: 0x%"PRIx64
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 6ea40f6b074..ae1489717fe 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -107,6 +107,7 @@ typedef struct SMMUS2Cfg {
typedef struct SMMUTransCfg {
/* Shared fields between stage-1 and stage-2. */
SMMUStage stage; /* translation stage */
+ SMMUSecSID sec_sid; /* cached sec sid */
bool disabled; /* smmu is disabled */
bool bypassed; /* translation is bypassed */
bool aborted; /* translation is aborted */
--
2.34.1Hi Tao,
On 2/21/26 11:02 AM, Tao Tang wrote:
> Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
> tied to the correct register bank.
>
> Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
> and event logs always show which security interface emitted the record.
> To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
> identified as soon as an event record is built.
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> hw/arm/smmuv3-internal.h | 1 +
> hw/arm/smmuv3.c | 20 +++++++++++++-------
> hw/arm/trace-events | 2 +-
> include/hw/arm/smmu-common.h | 1 +
> 4 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 866d62257e3..a1071f7b689 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
>
> /* Encode an event record */
> typedef struct SMMUEventInfo {
> + SMMUSecSID sec_sid;
> SMMUEventType type;
> uint32_t sid;
> bool recorded;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 2c107724e77..3438adcecd2 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
> return MEMTX_OK;
> }
>
> -static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
> +static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
> + Evt *evt)
> {
> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
> SMMUQueue *q = &bank->eventq;
> MemTxResult r;
> @@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
> {
> Evt evt = {};
> MemTxResult r;
> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> + SMMUSecSID sec_sid = info->sec_sid;
> + g_assert(sec_sid < SMMU_SEC_SID_NUM);
>
> if (!smmuv3_eventq_enabled(s, sec_sid)) {
> return;
> @@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
> g_assert_not_reached();
> }
>
> - trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
> - r = smmuv3_write_eventq(s, &evt);
> + trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
> + info->sid);
> + r = smmuv3_write_eventq(s, sec_sid, &evt);
> if (r != MEMTX_OK) {
> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
> }
> @@ -917,6 +919,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
> 100 * sdev->cfg_cache_hits /
> (sdev->cfg_cache_hits + sdev->cfg_cache_misses));
> cfg = g_new0(SMMUTransCfg, 1);
> + cfg->sec_sid = SMMU_SEC_SID_NS;
>
> if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
> g_hash_table_insert(bc->configs, sdev, cfg);
> @@ -1074,7 +1077,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
> SMMUEventInfo event = {.type = SMMU_EVT_NONE,
> .sid = sid,
> - .inval_ste_allowed = false};
> + .inval_ste_allowed = false,
> + .sec_sid = sec_sid};
> SMMUTranslationStatus status;
> SMMUTransCfg *cfg = NULL;
> IOMMUTLBEntry entry = {
> @@ -1176,7 +1180,9 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
> uint64_t num_pages, int stage)
> {
> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
> - SMMUEventInfo eventinfo = {.inval_ste_allowed = true};
> + SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> + SMMUEventInfo eventinfo = {.sec_sid = sec_sid,
> + .inval_ste_allowed = true};
> SMMUTransCfg *cfg = smmuv3_get_config(sdev, &eventinfo);
> IOMMUTLBEvent event;
> uint8_t granule;
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 8135c0c7344..9c2cc131ab4 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -40,7 +40,7 @@ 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"
> smmuv3_write_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
> -smmuv3_record_event(const char *type, uint32_t sid) "%s sid=0x%x"
> +smmuv3_record_event(int sec_sid, const char *type, uint32_t sid) "sec_sid=%d %s sid=0x%x"
> smmuv3_find_ste(uint16_t sid, uint32_t features, uint16_t sid_split) "sid=0x%x features:0x%x, sid_split:0x%x"
> smmuv3_find_ste_2lvl(uint64_t strtab_base, uint64_t l1ptr, int l1_ste_offset, uint64_t l2ptr, int l2_ste_offset, int max_l2_ste) "strtab_base:0x%"PRIx64" l1ptr:0x%"PRIx64" l1_off:0x%x, l2ptr:0x%"PRIx64" l2_off:0x%x max_l2_ste:%d"
> smmuv3_get_ste(uint64_t addr) "STE addr: 0x%"PRIx64
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 6ea40f6b074..ae1489717fe 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -107,6 +107,7 @@ typedef struct SMMUS2Cfg {
> typedef struct SMMUTransCfg {
> /* Shared fields between stage-1 and stage-2. */
> SMMUStage stage; /* translation stage */
> + SMMUSecSID sec_sid; /* cached sec sid */
I am now wondering if sec_sid shall be part of the TransCfg as this is
never decoded from STE/CD, sin't it.
Eric
> bool disabled; /* smmu is disabled */
> bool bypassed; /* translation is bypassed */
> bool aborted; /* translation is aborted */
On 3/2/26 5:13 PM, Eric Auger wrote:
> Hi Tao,
>
> On 2/21/26 11:02 AM, Tao Tang wrote:
>> Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
>> tied to the correct register bank.
>>
>> Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
>> and event logs always show which security interface emitted the record.
>> To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
>> identified as soon as an event record is built.
>>
>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> hw/arm/smmuv3-internal.h | 1 +
>> hw/arm/smmuv3.c | 20 +++++++++++++-------
>> hw/arm/trace-events | 2 +-
>> include/hw/arm/smmu-common.h | 1 +
>> 4 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index 866d62257e3..a1071f7b689 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
>>
>> /* Encode an event record */
>> typedef struct SMMUEventInfo {
>> + SMMUSecSID sec_sid;
>> SMMUEventType type;
>> uint32_t sid;
>> bool recorded;
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 2c107724e77..3438adcecd2 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
>> return MEMTX_OK;
>> }
>>
>> -static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
>> +static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
>> + Evt *evt)
>> {
>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
>> SMMUQueue *q = &bank->eventq;
>> MemTxResult r;
>> @@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>> {
>> Evt evt = {};
>> MemTxResult r;
>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>> + SMMUSecSID sec_sid = info->sec_sid;
>> + g_assert(sec_sid < SMMU_SEC_SID_NUM);
>>
>> if (!smmuv3_eventq_enabled(s, sec_sid)) {
>> return;
>> @@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>> g_assert_not_reached();
>> }
>>
>> - trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
>> - r = smmuv3_write_eventq(s, &evt);
>> + trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
>> + info->sid);
>> + r = smmuv3_write_eventq(s, sec_sid, &evt);
>> if (r != MEMTX_OK) {
>> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
>> }
>> @@ -917,6 +919,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
>> 100 * sdev->cfg_cache_hits /
>> (sdev->cfg_cache_hits + sdev->cfg_cache_misses));
>> cfg = g_new0(SMMUTransCfg, 1);
>> + cfg->sec_sid = SMMU_SEC_SID_NS;
>>
>> if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
>> g_hash_table_insert(bc->configs, sdev, cfg);
>> @@ -1074,7 +1077,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
>> SMMUEventInfo event = {.type = SMMU_EVT_NONE,
>> .sid = sid,
>> - .inval_ste_allowed = false};
>> + .inval_ste_allowed = false,
>> + .sec_sid = sec_sid};
>> SMMUTranslationStatus status;
>> SMMUTransCfg *cfg = NULL;
>> IOMMUTLBEntry entry = {
>> @@ -1176,7 +1180,9 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
>> uint64_t num_pages, int stage)
>> {
>> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
>> - SMMUEventInfo eventinfo = {.inval_ste_allowed = true};
>> + SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>> + SMMUEventInfo eventinfo = {.sec_sid = sec_sid,
>> + .inval_ste_allowed = true};
>> SMMUTransCfg *cfg = smmuv3_get_config(sdev, &eventinfo);
>> IOMMUTLBEvent event;
>> uint8_t granule;
>> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
>> index 8135c0c7344..9c2cc131ab4 100644
>> --- a/hw/arm/trace-events
>> +++ b/hw/arm/trace-events
>> @@ -40,7 +40,7 @@ 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"
>> smmuv3_write_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
>> -smmuv3_record_event(const char *type, uint32_t sid) "%s sid=0x%x"
>> +smmuv3_record_event(int sec_sid, const char *type, uint32_t sid) "sec_sid=%d %s sid=0x%x"
>> smmuv3_find_ste(uint16_t sid, uint32_t features, uint16_t sid_split) "sid=0x%x features:0x%x, sid_split:0x%x"
>> smmuv3_find_ste_2lvl(uint64_t strtab_base, uint64_t l1ptr, int l1_ste_offset, uint64_t l2ptr, int l2_ste_offset, int max_l2_ste) "strtab_base:0x%"PRIx64" l1ptr:0x%"PRIx64" l1_off:0x%x, l2ptr:0x%"PRIx64" l2_off:0x%x max_l2_ste:%d"
>> smmuv3_get_ste(uint64_t addr) "STE addr: 0x%"PRIx64
>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>> index 6ea40f6b074..ae1489717fe 100644
>> --- a/include/hw/arm/smmu-common.h
>> +++ b/include/hw/arm/smmu-common.h
>> @@ -107,6 +107,7 @@ typedef struct SMMUS2Cfg {
>> typedef struct SMMUTransCfg {
>> /* Shared fields between stage-1 and stage-2. */
>> SMMUStage stage; /* translation stage */
>> + SMMUSecSID sec_sid; /* cached sec sid */
> I am now wondering if sec_sid shall be part of the TransCfg as this is
> never decoded from STE/CD, sin't it.
Having slept over it, caching sed_id here indicates whether the secure
stream table was used for decoding. So that can be considered as a
result of the config decode process.
a minor comment though, I think I would prefer having cfg->sec_sid
populated in smmuv3_decode_config as the other cfg fields. Meaning I
would rather add sed_sid to smmuv3_decode_config signature.
Thanks
Eric
>
> Eric
>> bool disabled; /* smmu is disabled */
>> bool bypassed; /* translation is bypassed */
>> bool aborted; /* translation is aborted */
>
Hi all,
On 2026/3/3 15:26, Eric Auger wrote:
>
> On 3/2/26 5:13 PM, Eric Auger wrote:
>> Hi Tao,
>>
>> On 2/21/26 11:02 AM, Tao Tang wrote:
>>> Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
>>> tied to the correct register bank.
>>>
>>> Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
>>> and event logs always show which security interface emitted the record.
>>> To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
>>> identified as soon as an event record is built.
>>>
>>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>> hw/arm/smmuv3-internal.h | 1 +
>>> hw/arm/smmuv3.c | 20 +++++++++++++-------
>>> hw/arm/trace-events | 2 +-
>>> include/hw/arm/smmu-common.h | 1 +
>>> 4 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>>> index 866d62257e3..a1071f7b689 100644
>>> --- a/hw/arm/smmuv3-internal.h
>>> +++ b/hw/arm/smmuv3-internal.h
>>> @@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
>>>
>>> /* Encode an event record */
>>> typedef struct SMMUEventInfo {
>>> + SMMUSecSID sec_sid;
>>> SMMUEventType type;
>>> uint32_t sid;
>>> bool recorded;
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>> index 2c107724e77..3438adcecd2 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
>>> return MEMTX_OK;
>>> }
>>>
>>> -static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
>>> +static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
>>> + Evt *evt)
>>> {
>>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>>> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
>>> SMMUQueue *q = &bank->eventq;
>>> MemTxResult r;
>>> @@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>>> {
>>> Evt evt = {};
>>> MemTxResult r;
>>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>>> + SMMUSecSID sec_sid = info->sec_sid;
>>> + g_assert(sec_sid < SMMU_SEC_SID_NUM);
>>>
>>> if (!smmuv3_eventq_enabled(s, sec_sid)) {
>>> return;
>>> @@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>>> g_assert_not_reached();
>>> }
>>>
>>> - trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
>>> - r = smmuv3_write_eventq(s, &evt);
>>> + trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
>>> + info->sid);
>>> + r = smmuv3_write_eventq(s, sec_sid, &evt);
>>> if (r != MEMTX_OK) {
>>> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
>>> }
>>> @@ -917,6 +919,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
>>> 100 * sdev->cfg_cache_hits /
>>> (sdev->cfg_cache_hits + sdev->cfg_cache_misses));
>>> cfg = g_new0(SMMUTransCfg, 1);
>>> + cfg->sec_sid = SMMU_SEC_SID_NS;
>>>
>>> if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
>>> g_hash_table_insert(bc->configs, sdev, cfg);
>>> @@ -1074,7 +1077,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
>>> SMMUEventInfo event = {.type = SMMU_EVT_NONE,
>>> .sid = sid,
>>> - .inval_ste_allowed = false};
>>> + .inval_ste_allowed = false,
>>> + .sec_sid = sec_sid};
>>> SMMUTranslationStatus status;
>>> SMMUTransCfg *cfg = NULL;
>>> IOMMUTLBEntry entry = {
>>> @@ -1176,7 +1180,9 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
>>> uint64_t num_pages, int stage)
>>> {
>>> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
>>> - SMMUEventInfo eventinfo = {.inval_ste_allowed = true};
>>> + SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>>> + SMMUEventInfo eventinfo = {.sec_sid = sec_sid,
>>> + .inval_ste_allowed = true};
>>> SMMUTransCfg *cfg = smmuv3_get_config(sdev, &eventinfo);
>>> IOMMUTLBEvent event;
>>> uint8_t granule;
>>> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
>>> index 8135c0c7344..9c2cc131ab4 100644
>>> --- a/hw/arm/trace-events
>>> +++ b/hw/arm/trace-events
>>> @@ -40,7 +40,7 @@ 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"
>>> smmuv3_write_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
>>> -smmuv3_record_event(const char *type, uint32_t sid) "%s sid=0x%x"
>>> +smmuv3_record_event(int sec_sid, const char *type, uint32_t sid) "sec_sid=%d %s sid=0x%x"
>>> smmuv3_find_ste(uint16_t sid, uint32_t features, uint16_t sid_split) "sid=0x%x features:0x%x, sid_split:0x%x"
>>> smmuv3_find_ste_2lvl(uint64_t strtab_base, uint64_t l1ptr, int l1_ste_offset, uint64_t l2ptr, int l2_ste_offset, int max_l2_ste) "strtab_base:0x%"PRIx64" l1ptr:0x%"PRIx64" l1_off:0x%x, l2ptr:0x%"PRIx64" l2_off:0x%x max_l2_ste:%d"
>>> smmuv3_get_ste(uint64_t addr) "STE addr: 0x%"PRIx64
>>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>>> index 6ea40f6b074..ae1489717fe 100644
>>> --- a/include/hw/arm/smmu-common.h
>>> +++ b/include/hw/arm/smmu-common.h
>>> @@ -107,6 +107,7 @@ typedef struct SMMUS2Cfg {
>>> typedef struct SMMUTransCfg {
>>> /* Shared fields between stage-1 and stage-2. */
>>> SMMUStage stage; /* translation stage */
>>> + SMMUSecSID sec_sid; /* cached sec sid */
>> I am now wondering if sec_sid shall be part of the TransCfg as this is
>> never decoded from STE/CD, sin't it.
> Having slept over it, caching sed_id here indicates whether the secure
> stream table was used for decoding. So that can be considered as a
> result of the config decode process.
>
> a minor comment though, I think I would prefer having cfg->sec_sid
> populated in smmuv3_decode_config as the other cfg fields. Meaning I
> would rather add sed_sid to smmuv3_decode_config signature.
Thanks for the suggestion.
I'll add add sec_sid to smmuv3_decode_config as signature and do
something like `cfg->sec_sid = sec_sid` inside the function.
BTW as Mostafa and Eric discussed in previous thread, a check of sec_sid
would be placed at smmuv3_bank instead of asserting everywhere.
Thank you all for the review. I will start preparing the v5 according to
your feedback.
Tao
> Thanks
>
> Eric
>
>> Eric
>>> bool disabled; /* smmu is disabled */
>>> bool bypassed; /* translation is bypassed */
>>> bool aborted; /* translation is aborted */
On Sat, Feb 21, 2026 at 06:02:25PM +0800, Tao Tang wrote:
> Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
> tied to the correct register bank.
>
> Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
> and event logs always show which security interface emitted the record.
> To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
> identified as soon as an event record is built.
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> hw/arm/smmuv3-internal.h | 1 +
> hw/arm/smmuv3.c | 20 +++++++++++++-------
> hw/arm/trace-events | 2 +-
> include/hw/arm/smmu-common.h | 1 +
> 4 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 866d62257e3..a1071f7b689 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
>
> /* Encode an event record */
> typedef struct SMMUEventInfo {
> + SMMUSecSID sec_sid;
> SMMUEventType type;
> uint32_t sid;
> bool recorded;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 2c107724e77..3438adcecd2 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
> return MEMTX_OK;
> }
>
> -static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
> +static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
> + Evt *evt)
> {
> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
> SMMUQueue *q = &bank->eventq;
> MemTxResult r;
> @@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
> {
> Evt evt = {};
> MemTxResult r;
> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> + SMMUSecSID sec_sid = info->sec_sid;
> + g_assert(sec_sid < SMMU_SEC_SID_NUM);
What does this defend against?
Thanks,
Mostafa
>
> if (!smmuv3_eventq_enabled(s, sec_sid)) {
> return;
> @@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
> g_assert_not_reached();
> }
>
> - trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
> - r = smmuv3_write_eventq(s, &evt);
> + trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
> + info->sid);
> + r = smmuv3_write_eventq(s, sec_sid, &evt);
> if (r != MEMTX_OK) {
> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
> }
> @@ -917,6 +919,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
> 100 * sdev->cfg_cache_hits /
> (sdev->cfg_cache_hits + sdev->cfg_cache_misses));
> cfg = g_new0(SMMUTransCfg, 1);
> + cfg->sec_sid = SMMU_SEC_SID_NS;
>
> if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
> g_hash_table_insert(bc->configs, sdev, cfg);
> @@ -1074,7 +1077,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
> SMMUEventInfo event = {.type = SMMU_EVT_NONE,
> .sid = sid,
> - .inval_ste_allowed = false};
> + .inval_ste_allowed = false,
> + .sec_sid = sec_sid};
> SMMUTranslationStatus status;
> SMMUTransCfg *cfg = NULL;
> IOMMUTLBEntry entry = {
> @@ -1176,7 +1180,9 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
> uint64_t num_pages, int stage)
> {
> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
> - SMMUEventInfo eventinfo = {.inval_ste_allowed = true};
> + SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> + SMMUEventInfo eventinfo = {.sec_sid = sec_sid,
> + .inval_ste_allowed = true};
> SMMUTransCfg *cfg = smmuv3_get_config(sdev, &eventinfo);
> IOMMUTLBEvent event;
> uint8_t granule;
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 8135c0c7344..9c2cc131ab4 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -40,7 +40,7 @@ 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"
> smmuv3_write_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
> -smmuv3_record_event(const char *type, uint32_t sid) "%s sid=0x%x"
> +smmuv3_record_event(int sec_sid, const char *type, uint32_t sid) "sec_sid=%d %s sid=0x%x"
> smmuv3_find_ste(uint16_t sid, uint32_t features, uint16_t sid_split) "sid=0x%x features:0x%x, sid_split:0x%x"
> smmuv3_find_ste_2lvl(uint64_t strtab_base, uint64_t l1ptr, int l1_ste_offset, uint64_t l2ptr, int l2_ste_offset, int max_l2_ste) "strtab_base:0x%"PRIx64" l1ptr:0x%"PRIx64" l1_off:0x%x, l2ptr:0x%"PRIx64" l2_off:0x%x max_l2_ste:%d"
> smmuv3_get_ste(uint64_t addr) "STE addr: 0x%"PRIx64
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 6ea40f6b074..ae1489717fe 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -107,6 +107,7 @@ typedef struct SMMUS2Cfg {
> typedef struct SMMUTransCfg {
> /* Shared fields between stage-1 and stage-2. */
> SMMUStage stage; /* translation stage */
> + SMMUSecSID sec_sid; /* cached sec sid */
> bool disabled; /* smmu is disabled */
> bool bypassed; /* translation is bypassed */
> bool aborted; /* translation is aborted */
> --
> 2.34.1
>
Hi Mostafa,
On 2026/2/27 PM10:39, Mostafa Saleh wrote:
> On Sat, Feb 21, 2026 at 06:02:25PM +0800, Tao Tang wrote:
>> Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
>> tied to the correct register bank.
>>
>> Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
>> and event logs always show which security interface emitted the record.
>> To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
>> identified as soon as an event record is built.
>>
>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> hw/arm/smmuv3-internal.h | 1 +
>> hw/arm/smmuv3.c | 20 +++++++++++++-------
>> hw/arm/trace-events | 2 +-
>> include/hw/arm/smmu-common.h | 1 +
>> 4 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index 866d62257e3..a1071f7b689 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
>>
>> /* Encode an event record */
>> typedef struct SMMUEventInfo {
>> + SMMUSecSID sec_sid;
>> SMMUEventType type;
>> uint32_t sid;
>> bool recorded;
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 2c107724e77..3438adcecd2 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
>> return MEMTX_OK;
>> }
>>
>> -static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
>> +static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
>> + Evt *evt)
>> {
>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
>> SMMUQueue *q = &bank->eventq;
>> MemTxResult r;
>> @@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>> {
>> Evt evt = {};
>> MemTxResult r;
>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>> + SMMUSecSID sec_sid = info->sec_sid;
>> + g_assert(sec_sid < SMMU_SEC_SID_NUM);
> What does this defend against?
sec_sid is now taken from SMMUEventInfo, so the assert is to catch
programming errors early and avoid out-of-bounds bank accesses in
smmuv3_record_event.
>
> Thanks,
> Mostafa
Best regards,
Tao
>
>>
>> if (!smmuv3_eventq_enabled(s, sec_sid)) {
>> return;
>> @@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>> g_assert_not_reached();
>> }
>>
>> - trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
>> - r = smmuv3_write_eventq(s, &evt);
>> + trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
>> + info->sid);
>> + r = smmuv3_write_eventq(s, sec_sid, &evt);
>> if (r != MEMTX_OK) {
>> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
>> }
On Sun, Mar 01, 2026 at 09:53:24PM +0800, Tao Tang wrote:
> Hi Mostafa,
>
> On 2026/2/27 PM10:39, Mostafa Saleh wrote:
> > On Sat, Feb 21, 2026 at 06:02:25PM +0800, Tao Tang wrote:
> > > Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
> > > tied to the correct register bank.
> > >
> > > Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
> > > and event logs always show which security interface emitted the record.
> > > To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
> > > identified as soon as an event record is built.
> > >
> > > Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > > ---
> > > hw/arm/smmuv3-internal.h | 1 +
> > > hw/arm/smmuv3.c | 20 +++++++++++++-------
> > > hw/arm/trace-events | 2 +-
> > > include/hw/arm/smmu-common.h | 1 +
> > > 4 files changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> > > index 866d62257e3..a1071f7b689 100644
> > > --- a/hw/arm/smmuv3-internal.h
> > > +++ b/hw/arm/smmuv3-internal.h
> > > @@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
> > > /* Encode an event record */
> > > typedef struct SMMUEventInfo {
> > > + SMMUSecSID sec_sid;
> > > SMMUEventType type;
> > > uint32_t sid;
> > > bool recorded;
> > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > > index 2c107724e77..3438adcecd2 100644
> > > --- a/hw/arm/smmuv3.c
> > > +++ b/hw/arm/smmuv3.c
> > > @@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
> > > return MEMTX_OK;
> > > }
> > > -static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
> > > +static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
> > > + Evt *evt)
> > > {
> > > - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> > > SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
> > > SMMUQueue *q = &bank->eventq;
> > > MemTxResult r;
> > > @@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
> > > {
> > > Evt evt = {};
> > > MemTxResult r;
> > > - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
> > > + SMMUSecSID sec_sid = info->sec_sid;
> > > + g_assert(sec_sid < SMMU_SEC_SID_NUM);
> > What does this defend against?
>
>
> sec_sid is now taken from SMMUEventInfo, so the assert is to catch
> programming errors early and avoid out-of-bounds bank accesses in
> smmuv3_record_event.
Personally, I don't like this kind of defensive programming, someone can
argue we can add such checks anywhere.
I believe we should definitely know the possible states in the software
and add checks needed at guest inputs.
But that's also up to Eric.
Thanks,
Mostafa
>
> >
> > Thanks,
> > Mostafa
>
>
> Best regards,
>
> Tao
>
> >
> > > if (!smmuv3_eventq_enabled(s, sec_sid)) {
> > > return;
> > > @@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
> > > g_assert_not_reached();
> > > }
> > > - trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
> > > - r = smmuv3_write_eventq(s, &evt);
> > > + trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
> > > + info->sid);
> > > + r = smmuv3_write_eventq(s, sec_sid, &evt);
> > > if (r != MEMTX_OK) {
> > > smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
> > > }
>
>
>
>
>
Hi Tao,
On 3/2/26 11:19 AM, Mostafa Saleh wrote:
> On Sun, Mar 01, 2026 at 09:53:24PM +0800, Tao Tang wrote:
>> Hi Mostafa,
>>
>> On 2026/2/27 PM10:39, Mostafa Saleh wrote:
>>> On Sat, Feb 21, 2026 at 06:02:25PM +0800, Tao Tang wrote:
>>>> Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups
>>>> tied to the correct register bank.
>>>>
>>>> Plumb the SEC_SID through tracepoints and queue helpers so diagnostics
>>>> and event logs always show which security interface emitted the record.
>>>> To support this, the SEC_SID is placed in SMMUEventInfo so the bank is
>>>> identified as soon as an event record is built.
>>>>
>>>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>> hw/arm/smmuv3-internal.h | 1 +
>>>> hw/arm/smmuv3.c | 20 +++++++++++++-------
>>>> hw/arm/trace-events | 2 +-
>>>> include/hw/arm/smmu-common.h | 1 +
>>>> 4 files changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>>>> index 866d62257e3..a1071f7b689 100644
>>>> --- a/hw/arm/smmuv3-internal.h
>>>> +++ b/hw/arm/smmuv3-internal.h
>>>> @@ -274,6 +274,7 @@ static inline const char *smmu_event_string(SMMUEventType type)
>>>> /* Encode an event record */
>>>> typedef struct SMMUEventInfo {
>>>> + SMMUSecSID sec_sid;
>>>> SMMUEventType type;
>>>> uint32_t sid;
>>>> bool recorded;
>>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>>> index 2c107724e77..3438adcecd2 100644
>>>> --- a/hw/arm/smmuv3.c
>>>> +++ b/hw/arm/smmuv3.c
>>>> @@ -148,9 +148,9 @@ static MemTxResult queue_write(SMMUQueue *q, Evt *evt_in)
>>>> return MEMTX_OK;
>>>> }
>>>> -static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
>>>> +static MemTxResult smmuv3_write_eventq(SMMUv3State *s, SMMUSecSID sec_sid,
>>>> + Evt *evt)
>>>> {
>>>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>>>> SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
>>>> SMMUQueue *q = &bank->eventq;
>>>> MemTxResult r;
>>>> @@ -178,7 +178,8 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>>>> {
>>>> Evt evt = {};
>>>> MemTxResult r;
>>>> - SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>>>> + SMMUSecSID sec_sid = info->sec_sid;
>>>> + g_assert(sec_sid < SMMU_SEC_SID_NUM);
>>> What does this defend against?
>>
>> sec_sid is now taken from SMMUEventInfo, so the assert is to catch
>> programming errors early and avoid out-of-bounds bank accesses in
>> smmuv3_record_event.
> Personally, I don't like this kind of defensive programming, someone can
> argue we can add such checks anywhere.
> I believe we should definitely know the possible states in the software
> and add checks needed at guest inputs.
> But that's also up to Eric.
I tend to agree with Mostafa. We shall avoid putting those asserts
everywhere. Would it make sense to pove it to smmuv3_bank directly?
Eric
>
> Thanks,
> Mostafa
>
>>> Thanks,
>>> Mostafa
>>
>> Best regards,
>>
>> Tao
>>
>>>> if (!smmuv3_eventq_enabled(s, sec_sid)) {
>>>> return;
>>>> @@ -258,8 +259,9 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>>>> g_assert_not_reached();
>>>> }
>>>> - trace_smmuv3_record_event(smmu_event_string(info->type), info->sid);
>>>> - r = smmuv3_write_eventq(s, &evt);
>>>> + trace_smmuv3_record_event(sec_sid, smmu_event_string(info->type),
>>>> + info->sid);
>>>> + r = smmuv3_write_eventq(s, sec_sid, &evt);
>>>> if (r != MEMTX_OK) {
>>>> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_EVENTQ_ABT_ERR_MASK);
>>>> }
>>
>>
>>
>>
On 2/21/26 2:02 AM, Tao Tang wrote: > Cache the SEC_SID inside SMMUTransCfg to keep configuration lookups > tied to the correct register bank. > > Plumb the SEC_SID through tracepoints and queue helpers so diagnostics > and event logs always show which security interface emitted the record. > To support this, the SEC_SID is placed in SMMUEventInfo so the bank is > identified as soon as an event record is built. > > Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn> > Reviewed-by: Eric Auger <eric.auger@redhat.com> > --- > hw/arm/smmuv3-internal.h | 1 + > hw/arm/smmuv3.c | 20 +++++++++++++------- > hw/arm/trace-events | 2 +- > include/hw/arm/smmu-common.h | 1 + > 4 files changed, 16 insertions(+), 8 deletions(-) > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
© 2016 - 2026 Red Hat, Inc.