Re: [PATCH 2/2] hw/arm/sbsa-ref.c: Increase SMMU space for Root registers

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

On Fri, Nov 06, 2026 at 11:21:12PM +0100, Gustavo Romero wrote:
> Hi Jim,
> 
> On Fri, Oct 16, 2026 at 10:40:52AM +0100, Jim MacArthur wrote:
> > In our implementation, SMMU Root registers are directly after the
> > non-optional SMMU registers. Realms registers will be added above the
> > root registers later.
> 
> nit: Root registers

Noted.

> 
> 
> > 
> > Signed-off-by: Jim MacArthur <jim.macarthur@linaro.org>
> > ---
> >  hw/arm/sbsa-ref.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> > index 158b461eb4..0747f5ca9f 100644
> > --- a/hw/arm/sbsa-ref.c
> > +++ b/hw/arm/sbsa-ref.c
> > @@ -129,7 +129,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
> >      [SBSA_GPIO] =               { 0x60020000, 0x00001000 },
> >      [SBSA_SECURE_UART] =        { 0x60030000, 0x00001000 },
> >      [SBSA_SECURE_UART_MM] =     { 0x60040000, 0x00001000 },
> > -    [SBSA_SMMU] =               { 0x60050000, 0x00020000 },
> > +    [SBSA_SMMU] =               { 0x60050000, 0x00040000 },
> 
> Why +0x20000 bytes here instead of +0x10000 bytes (1 x 64K page)?

Ultimately the plan is to add the Realm registers in here as well, and
those have to be at a 128K-aligned offset from the SMMU base, so per the
original plan they would have started at 0x40000. Now that you mention it
though, it makes more sense to slot the Realm registers in at offset 0x20000
and move the Root registers to 0x40000 when we do so - which means we still
have space in the memory map for a second SMMU. So for now, I'll trim
this back to +0x10000.

> 
> Most importantly, incresing the size of the SBSA_SMMU index is not enough, we
> need to increase also the size of the "arm-smmuv3" container (memory region).
> 
> With this change applied I still see:
> 
> 0000000060050000-000000006006ffff (prio 0, i/o): arm-smmuv3

Thanks, I hadn't spotted that the actual allocation was specified separately;
I'll fix that in the next version.

> 
> on (qemu) info mtree.
> 
> 2 x 64K pages: Page 0 and Page 1. Rather we should see a range to fit 3 x 64K:
> 
> 0000000060050000-000000006007ffff (prio 0, i/o): arm-smmuv3
> 
> Another issue, afaics, is that the Root register page will be added to
> every machine that uses the SMMUv3 device, right? Because we're not gating the
> addition of Root registers based on the presence of RME.
> 
> So I think we should add one boolean "x-root" property (flag) to the SMMUv3
> device that will be used to increase the size of the "arm-smmuv3" if it's set
> and let it by default set to "false".
> 
> This "x-root" property would be set at the machine level, so via
> object_property_set_bool() in create_smmu. For example:
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 0747f5ca9f..36b0164510 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -630,6 +630,7 @@ static void create_smmu(const SBSAMachineState *sms, PCIBus *bus,
>                               &error_abort);
>      object_property_set_link(OBJECT(dev), "secure-memory", OBJECT(secure_sysmem),
>                               &error_abort);
> +    object_property_set_bool(OBJECT(dev), "x-root", true, &error_abort);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>      for (i = 0; i < NUM_SMMU_IRQS; i++) {
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 9d9bc2601b..9912fbf920 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -2267,6 +2267,7 @@ static void smmu_realize(DeviceState *d, Error **errp)
>      SMMUv3Class *c = ARM_SMMUV3_GET_CLASS(s);
>      SysBusDevice *dev = SYS_BUS_DEVICE(d);
>      Error *local_err = NULL;
> +    uint64_t size = SMMUV3_REGS_SIZE;
>  
>      if (!smmu_validate_property(s, errp)) {
>          return;
> @@ -2291,8 +2292,12 @@ static void smmu_realize(DeviceState *d, Error **errp)
>  
>      qemu_mutex_init(&s->mutex);
>  
> +    if (s->has_root) {
> +        size += SMMUV3_ROOT_PAGE_SIZE;
> +    }
> +
>      memory_region_init_io(&sys->iomem, OBJECT(s),
> -                          &smmu_mem_ops, sys, TYPE_ARM_SMMUV3, 0x20000);
> +                          &smmu_mem_ops, sys, TYPE_ARM_SMMUV3, size);
>  
>      sys->mrtypename = TYPE_SMMUV3_IOMMU_MEMORY_REGION;
>  
> @@ -2382,6 +2387,7 @@ static const Property smmuv3_properties[] = {
>      /* Identifier used for ACPI IORT SMMUv3 (and DSDT for CMDQV) generation */
>      DEFINE_PROP_UINT8("identifier", SMMUv3State, identifier, 0),
>      DEFINE_PROP_BOOL("accel", SMMUv3State, accel, false),
> +    DEFINE_PROP_BOOL("x-root", SMMUv3State, has_root, false),
>      /* GPA of MSI doorbell, for SMMUv3 accel use. */
>      DEFINE_PROP_UINT64("msi-gpa", SMMUv3State, msi_gpa, 0),
>      /*
> diff --git a/include/hw/arm/smmuv3-common.h b/include/hw/arm/smmuv3-common.h
> index a538334066..5d1cb759c3 100644
> --- a/include/hw/arm/smmuv3-common.h
> +++ b/include/hw/arm/smmuv3-common.h
> @@ -411,7 +411,10 @@ REG32(EVENTQ_IRQ_CFG2,     0xbc)
>  
>  #define A_IDREGS           0xfd0
>  
> -#define SMMU_ROOT_REG_START 0x20000
> +#define SMMUV3_PAGE_SIZE        0x10000
> +#define SMMUV3_REGS_SIZE        (2 * SMMUV3_PAGE_SIZE) /* Page 0 and Page 1 */
> +#define SMMUV3_ROOT_REG_OFFSET  0x20000
> +#define SMMUV3_ROOT_PAGE_SIZE   SMMUV3_PAGE_SIZE
>  
>  REG32(ROOT_IDR0,            0x0)
>      FIELD(ROOT_IDR0, ROOT_IMPL,   0,  1)
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index e2708e13fe..a8b354438c 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -92,6 +92,7 @@ struct SMMUv3State {
>      OnOffAuto cmdqv;
>  
>      Notifier machine_done;
> +    bool has_root;
>      SMMUv3RootRegBank root;
>  };
> 
> 
> Cheers,
> Gustavo

Thank you for doing this. The only complication is that we'll probably want
an x-realms flag as well later, and if we do the rearrangement of the map
as above then SMMUV3_ROOT_REG_OFFSET will have to be variable. I'll handle
that when we get to the Realm part.

Jim