Re: [RFC v4 06/31] hw/arm/smmuv3: Track SEC_SID in configs and events

[Mostafa Saleh <smostafa@google.com>]

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);
> > >       }
> 
> 
> 
> 
>