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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Pierrick,

On 11/12/25 23: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.

Sorry yesterday it was late for me and I forgot to post the similar
patch :/

> 
> As well, it allows to remove usage of global address_space_memory,
> allowing different SMMU instances to have a specific view of memory.
> 
> 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;

Has SMMU concept of "secure memory"? My understanding is just a
different memory to access GPT, so I'd name it "gpt_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);

As a preliminary cleanup create_pcie() returns the PCI bus, so the
machine_init() can call create_smmu() itself. I'll post later.

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

Allocating AddressSpaces on the heap:

        else {
            s->as_secure_memory = s->as_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);
>       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++) {
> @@ -1587,7 +1592,8 @@ static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
>       }
>   }
>   
> -static void create_pcie(VirtMachineState *vms)
> +static void create_pcie(VirtMachineState *vms,
> +                        MemoryRegion *sysmem, MemoryRegion *secure_sysmem)
>   {
>       hwaddr base_mmio = vms->memmap[VIRT_PCIE_MMIO].base;
>       hwaddr size_mmio = vms->memmap[VIRT_PCIE_MMIO].size;
> @@ -1706,7 +1712,7 @@ static void create_pcie(VirtMachineState *vms)
>   
>           switch (vms->iommu) {
>           case VIRT_IOMMU_SMMUV3:
> -            create_smmu(vms, vms->bus);
> +            create_smmu(vms, vms->bus, sysmem, secure_sysmem);
>               if (!vms->default_bus_bypass_iommu) {
>                   qemu_fdt_setprop_cells(ms->fdt, nodename, "iommu-map",
>                                          0x0, vms->iommu_phandle, 0x0, 0x10000);
> @@ -2520,7 +2526,7 @@ static void machvirt_init(MachineState *machine)
>   
>       create_rtc(vms);
>   
> -    create_pcie(vms);
> +    create_pcie(vms, sysmem, secure_sysmem);
>       create_cxl_host_reg_region(vms);
>   
>       if (aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {