Re: [PATCH 1/2] hw/arm/smmuv3: define Root registers

[Jim MacArthur <jim.macarthur@linaro.org>]

On Fri, Nov 06, 2026 at 11:20:56PM +0100, Gustavo Romero wrote:
> > +/*
> > + * Check we are from the Root PA space to read from or write to the
> > + * SMMU Root register area.
> > + */
> > +static bool smmu_check_root_access(SMMUv3State *s, MemTxAttrs attrs,
> > +                                     hwaddr offset, bool is_read)
> 
> Indentation is off ---------------------^
> 
> I think it makes sense also to call this function smmu_root_access(), but no
> strong preference here.

Noted indentation fault. I would prefer to have a verb in the function name,
but similarly have no strong preferences on it.

> 
> 
> > +{
> > +    /* Check if the access is from Root PA */
> > +    if (!(attrs.space == ARMSS_Root)) {
> 
> Could we use the comparison operator (!=) instead of the logical one here? I
> think it's clearer than a negation.
> 
Good point, that is clearer.

> > +    case A_ROOT_GPT_BASE2:
> > +        *data = s->root.gpt_base2;
> > +        return MEMTX_OK;
> > +    case A_ROOT_GPCBW:
> > +        /* Optional, not implemented. Present if ROOT_IDR0.GPTS == 0 */
> 
>                                          GPTS == 1 here? -----^
Correct, thank you.

> > @@ -411,6 +411,28 @@ REG32(EVENTQ_IRQ_CFG2,     0xbc)
> >  
> >  #define A_IDREGS           0xfd0
> >  
> > +#define SMMU_ROOT_REG_START 0x20000
> 
> The base address for the Root Control Page is implementation defined, hence
> 0x20000 is not always true and depends on the machine/platform. We could add
> property to set it externaly but it would also complicate unnecessarily the
> offset math. So I think it's ok to follow what FVP is doing. Please just add a
> note saying it's impl. defined so it might require a tweak in the
> future.
> 
> I think we can follow TF-A code here and rename this macro. So how about
> renaming it to:
> 
> SMMUV3_ROOT_REG_OFFSET ?
> 
I'll add another comment and yes, that name change makes sense.

...

> > +typedef struct SMMUv3RootRegBank {
> 
> How about naming this struct simply SMMUv3Root?

I don't mind renaming it to something other than 'Bank' to avoid conflicts
with another meaning of register bank (e.g. the actually banked registers),
for example SMMUv3RootControlPage would be fine. If it's just to save space
on punch cards, I'm less keen, but will happily follow code conventions.

> 
> 
> > +    uint32_t idr0;
> > +    uint32_t iidr;
> > +    uint32_t cr0;
> > +    uint32_t cr0ack;
> > +    uint64_t gpt_base;
> > +    uint64_t gpt_base_cfg;
> > +    uint64_t gpf_far;
> > +    uint64_t gpt_cfg_far;
> > +    uint64_t gpt_base2;
> 
> Looking at ARM IHI 0070 H.a Non-confidential spec I don't see
> 
> 0x0068 SMMU_ROOT_GPT_BASE_UPDATE 32-bit, R/W and
> 0x0070 SMMU_ROOT_GPCBW 64-bit, R/W
> 
> as Optional like I see SMMU_ROOT_TLBI and SMMU_ROOT_TLBI_CTRL.
> 
> Should they be declared as well?

GPT_BASE_UPDATE is an unusual case; it has side effects when writing hence
I was reluctant to include it as a normal config register, however, it
shouldn't do any harm to add it - users will just see the 'update' bit stick
as 1 forever if they write it.

SMMU_ROOT_GPCBW is RES0 unless ROOT_IDR0.GPTS == 1 as noted above, and we
don't define that bit at the moment, so making storage for it seemed
unnecessary.

> 
> 
> > +} SMMUv3RootRegBank;
> > +
> >  struct SMMUv3State {
> >      SMMUState     smmu_state;
> >  
> > @@ -80,6 +92,7 @@ struct SMMUv3State {
> >      OnOffAuto cmdqv;
> >  
> >      Notifier machine_done;
> > +    SMMUv3RootRegBank root;
> >  };
> >  
> >  typedef enum {
> > 
> > -- 
> > 2.43.0
> > 
> > 
> 
> General comments:
> 
> smmu_read_mmio_root
> smmu_write_mmio_root 
> 
> I think we can drop the "mmio_" for both functions. It's ok to have "mmio_" in
> the "trace_smmuv3_write_mmio" trace function because afaics it has a generic use,
> so its use is not tied to any bank.
Yep, fine by me.

> 
> 
> I think we're missing to reset some registers in smmuv3_reset()? Like:
> 
> s->root.cr0 = 0;
> s->root.cr0ack = 0;
> s->root.gpf_far = 0;
> s->root.gpt_cfg_far = 0;

Good point, we are missing those, thanks.

Thanks for the review!

Jim