Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 12/12/25 8:45 AM, Tao Tang wrote:
> Hi Pierrick,
> 
> On 2025/12/12 06:17, Pierrick Bouvier wrote:
>> This will be used to access non-secure and secure memory. Secure support
>> and Granule Protection Check (for RME) for SMMU need to access secure
>> memory.
>>
>> As well, it allows to remove usage of global address_space_memory,
>> allowing different SMMU instances to have a specific view of memory.
> 
> 
> Thanks for the patch.
> 
> I have to admit that in my earlier series I didn’t find a clean way to
> model SMMU’s AddressSpace handling properly, so I ended up using weak
> globals as a pragmatic workaround. Your approach is much cleaner and
> fits QEMU’s usual memory model.
> 
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/hw/arm/smmu-common.h |  4 ++++
>>    hw/arm/sbsa-ref.c            | 16 ++++++++++++----
>>    hw/arm/smmu-common.c         | 24 ++++++++++++++++++++++++
>>    hw/arm/virt.c                | 16 +++++++++++-----
>>    4 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>> index a6bdb67a983..0f08ae080c9 100644
>> --- a/include/hw/arm/smmu-common.h
>> +++ b/include/hw/arm/smmu-common.h
>> @@ -227,6 +227,10 @@ struct SMMUState {
>>        uint8_t bus_num;
>>        PCIBus *primary_bus;
>>        bool smmu_per_bus; /* SMMU is specific to the primary_bus */
>> +    MemoryRegion *memory;
>> +    AddressSpace as_memory;
>> +    MemoryRegion *secure_memory;
>> +    AddressSpace as_secure_memory;
>>    };
>>    
>>    struct SMMUBaseClass {
>> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
>> index 45d2e3e946d..840b1a216f4 100644
>> --- a/hw/arm/sbsa-ref.c
>> +++ b/hw/arm/sbsa-ref.c
>> @@ -616,7 +616,9 @@ static void create_xhci(const SBSAMachineState *sms)
>>        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(sms->gic, irq));
>>    }
>>    
>> -static void create_smmu(const SBSAMachineState *sms, PCIBus *bus)
>> +static void create_smmu(const SBSAMachineState *sms, PCIBus *bus,
>> +                        MemoryRegion *sysmem,
>> +                        MemoryRegion *secure_sysmem)
>>    {
>>        hwaddr base = sbsa_ref_memmap[SBSA_SMMU].base;
>>        int irq =  sbsa_ref_irqmap[SBSA_SMMU];
>> @@ -628,6 +630,10 @@ static void create_smmu(const SBSAMachineState *sms, PCIBus *bus)
>>        object_property_set_str(OBJECT(dev), "stage", "nested", &error_abort);
>>        object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>>                                 &error_abort);
>> +    object_property_set_link(OBJECT(dev), "memory", OBJECT(sysmem),
>> +                             &error_abort);
>> +    object_property_set_link(OBJECT(dev), "secure-memory", OBJECT(secure_sysmem),
>> +                             &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++) {
>> @@ -636,7 +642,9 @@ static void create_smmu(const SBSAMachineState *sms, PCIBus *bus)
>>        }
>>    }
>>    
>> -static void create_pcie(SBSAMachineState *sms)
>> +static void create_pcie(SBSAMachineState *sms,
>> +                        MemoryRegion *sysmem,
>> +                        MemoryRegion *secure_sysmem)
>>    {
>>        hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
>>        hwaddr size_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].size;
>> @@ -692,7 +700,7 @@ static void create_pcie(SBSAMachineState *sms)
>>    
>>        pci_create_simple(pci->bus, -1, "bochs-display");
>>    
>> -    create_smmu(sms, pci->bus);
>> +    create_smmu(sms, pci->bus, sysmem, secure_sysmem);
>>    }
>>    
>>    static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>> @@ -831,7 +839,7 @@ static void sbsa_ref_init(MachineState *machine)
>>    
>>        create_xhci(sms);
>>    
>> -    create_pcie(sms);
>> +    create_pcie(sms, sysmem, secure_sysmem);
>>    
>>        create_secure_ec(secure_sysmem);
>>    
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index 66367adc2a4..5fbfe825fd0 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -1171,6 +1171,12 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
>>            return;
>>        }
>>    
>> +    g_assert(s->memory);
>> +    address_space_init(&s->as_memory, s->memory, "memory");
>> +    if (s->secure_memory) {
>> +        address_space_init(&s->as_secure_memory, s->secure_memory, "secure-memory");
>> +    }
>> +
>>        /*
>>         * We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based extra
>>         * root complexes to be associated with SMMU.
>> @@ -1235,10 +1241,28 @@ static void smmu_base_class_init(ObjectClass *klass, const void *data)
>>        rc->phases.exit = smmu_base_reset_exit;
>>    }
>>    
>> +static void smmu_base_instance_init(Object *obj)
>> +{
>> +    SMMUState *s = ARM_SMMU(obj);
>> +
>> +    object_property_add_link(obj, "memory",
>> +                             TYPE_MEMORY_REGION,
>> +                             (Object **)&s->memory,
>> +                             qdev_prop_allow_set_link_before_realize,
>> +                             OBJ_PROP_LINK_STRONG);
>> +
>> +    object_property_add_link(obj, "secure-memory",
>> +                             TYPE_MEMORY_REGION,
>> +                             (Object **)&s->secure_memory,
>> +                             qdev_prop_allow_set_link_before_realize,
>> +                             OBJ_PROP_LINK_STRONG);
>> +}
>> +
>>    static const TypeInfo smmu_base_info = {
>>        .name          = TYPE_ARM_SMMU,
>>        .parent        = TYPE_SYS_BUS_DEVICE,
>>        .instance_size = sizeof(SMMUState),
>> +    .instance_init = smmu_base_instance_init,
>>        .class_data    = NULL,
>>        .class_size    = sizeof(SMMUBaseClass),
>>        .class_init    = smmu_base_class_init,
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 5d205eff3a1..d446c3349e9 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1514,8 +1514,9 @@ static void create_smmuv3_dev_dtb(VirtMachineState *vms,
>>                               0x0, vms->iommu_phandle, 0x0, 0x10000);
>>    }
>>    
>> -static void create_smmu(const VirtMachineState *vms,
>> -                        PCIBus *bus)
>> +static void create_smmu(const VirtMachineState *vms, PCIBus *bus,
>> +                        MemoryRegion *sysmem,
>> +                        MemoryRegion *secure_sysmem)
>>    {
>>        VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>        int irq =  vms->irqmap[VIRT_SMMU];
>> @@ -1549,6 +1550,10 @@ static void create_smmu(const VirtMachineState *vms,
>>        object_property_set_str(OBJECT(dev), "stage", stage, &error_fatal);
> 
> This unchanged line doesn't seem to match the current master branch
> code. It was actually modified last year by Peter in commit 8a934f1c4a
> which changed the virt board's default stage. So I failed to apply this
> patch. Anyway this discrepancy doesn't affect the core functionality of
> the patch.
> 

Sorry about it, it's a local (and personal) patch I have to override the 
default stage, so it didn't apply cleanly on upstream/master. I'll fix 
the conflict in next version.

>  
> Applying: hw/arm/smmu: add memory regions as property for an SMMU instance
> error: patch failed: hw/arm/virt.c:1549
> error: hw/arm/virt.c: patch does not apply
> Patch failed at 0001 hw/arm/smmu: add memory regions as property for an
> SMMU instance
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> 
> 
>>        object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>>                                 &error_abort);
>> +    object_property_set_link(OBJECT(dev), "memory", OBJECT(sysmem),
>> +                             &error_abort);
> 
> I noticed when using info mtree: after applying the patch, I see two
> address-space: memory which may confuse others. Is it necessary to
> distinguish the names here?
> 
> 
> address-space: cpu-memory-0
> address-space: gicv3-its-sysmem
> address-space: memory
> address-space: memory
>     0000000000000000-ffffffffffffffff (prio 0, i/o): system
>       0000000000000000-0000000003ffffff (prio 0, romd): virt.flash0
>       0000000004000000-0000000007ffffff (prio 0, romd): virt.flash1
>       0000000008000000-000000000800ffff (prio 0, i/o): gicv3_dist
> 

I thought about naming it smmu-memory, smmu-secure-memory, so your 
remark tends to confirm it.
@Peter, which name would be the best for you?

> 
> For the functionality, based on this patch plus attachment #2 from the
> earlier thread [1], the NS/S SMMU code path in smmu_get_address_space
> works fine in my setup.
> 
> 
> [1]
> https://lore.kernel.org/qemu-devel/db7fde79-85fa-4bd4-83ca-021ed3f09353@linaro.org/
> 
> 
> Thanks,
> 
> Tao
>