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 <[email protected]>
>>>> Reviewed-by: Eric Auger <[email protected]>
>>>> ---
>>>> 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);
>>>> }
>>
>>
>>
>>
