* [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
@ 2025-12-11 22:17 Pierrick Bouvier
2025-12-11 22:21 ` Pierrick Bouvier
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Pierrick Bouvier @ 2025-12-11 22:17 UTC (permalink / raw)
To: qemu-devel
Cc: Leif Lindholm, tangtao1634, Philippe Mathieu-Daudé, qemu-arm,
Eric Auger, Peter Maydell, Richard Henderson, Radoslaw Biernacki,
Pierrick Bouvier
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.
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);
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)) {
--
2.47.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
2025-12-11 22:17 [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance Pierrick Bouvier
@ 2025-12-11 22:21 ` Pierrick Bouvier
2025-12-12 6:16 ` Philippe Mathieu-Daudé
2025-12-12 16:45 ` Tao Tang
2 siblings, 0 replies; 8+ messages in thread
From: Pierrick Bouvier @ 2025-12-11 22:21 UTC (permalink / raw)
To: qemu-devel
Cc: Leif Lindholm, tangtao1634, Philippe Mathieu-Daudé, qemu-arm,
Eric Auger, Peter Maydell, Richard Henderson, Radoslaw Biernacki
On 12/11/25 2:17 PM, 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.
>
> 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(-)
>
This hopefully solves issue discussed here:
https://lore.kernel.org/qemu-devel/20251012150701.4127034-9-tangtao1634@phytium.com.cn/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
2025-12-11 22:17 [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance Pierrick Bouvier
2025-12-11 22:21 ` Pierrick Bouvier
@ 2025-12-12 6:16 ` Philippe Mathieu-Daudé
2025-12-12 10:50 ` Peter Maydell
2025-12-12 17:06 ` Pierrick Bouvier
2025-12-12 16:45 ` Tao Tang
2 siblings, 2 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-12-12 6:16 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Leif Lindholm, tangtao1634, qemu-arm, Eric Auger, Peter Maydell,
Richard Henderson, Radoslaw Biernacki
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)) {
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
2025-12-12 6:16 ` Philippe Mathieu-Daudé
@ 2025-12-12 10:50 ` Peter Maydell
2025-12-12 17:26 ` Pierrick Bouvier
2025-12-12 17:06 ` Pierrick Bouvier
1 sibling, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2025-12-12 10:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Pierrick Bouvier, qemu-devel, Leif Lindholm, tangtao1634,
qemu-arm, Eric Auger, Richard Henderson, Radoslaw Biernacki
On Fri, 12 Dec 2025 at 06:16, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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"?
Yes -- IHI0070G.b section 3.10.2: when SMMU_S_IDR1.SECURE_IMPL=1
"the SMMU can generate transactions to the memory system, to Secure
PA space (NS=0) and Non-secure PA space (NS=1)".
When the SMMU has Realm support then further (3.10.3)
the output PA space can be Realm, and also GPT checks are
always done to the Root address space.
So an SMMU with Secure and Realm support can emit transactions
to any of the 4 address spaces, the same as a CPU with those
features.
Separately from that, at the moment the way we model Realm
and Root is by using the same QEMU MemoryRegion trees we
have for NS and S and just using MemTxAttrs::space to distinguish,
because we don't have any need for devices that appear only in
Realm and not NS, or only in Root and not S. So for CPUs we
collapse the 4 architectural address spaces down into two QEMU
ones.
For the SMMU we can presumably follow the CPU there
(with an equivalent "give me the AddressSpace I should
use for these MemTxAttrs" function as arm_addressspace()).
> My understanding is just a
> different memory to access GPT, so I'd name it "gpt_memory".
I would prefer that we use the architecturally standard
terminology here.
thanks
-- PMM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
2025-12-11 22:17 [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance Pierrick Bouvier
2025-12-11 22:21 ` Pierrick Bouvier
2025-12-12 6:16 ` Philippe Mathieu-Daudé
@ 2025-12-12 16:45 ` Tao Tang
2025-12-12 17:31 ` Pierrick Bouvier
2 siblings, 1 reply; 8+ messages in thread
From: Tao Tang @ 2025-12-12 16:45 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Leif Lindholm, Philippe Mathieu-Daudé, qemu-arm, Eric Auger,
Peter Maydell, Richard Henderson, Radoslaw Biernacki
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.
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
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
2025-12-12 6:16 ` Philippe Mathieu-Daudé
2025-12-12 10:50 ` Peter Maydell
@ 2025-12-12 17:06 ` Pierrick Bouvier
1 sibling, 0 replies; 8+ messages in thread
From: Pierrick Bouvier @ 2025-12-12 17:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Leif Lindholm, tangtao1634, qemu-arm, Eric Auger, Peter Maydell,
Richard Henderson, Radoslaw Biernacki
On 12/11/25 10:16 PM, Philippe Mathieu-Daudé wrote:
> 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".
>
Yes, it has the concept of secure state, secure devices that can make
transactions that target secure memory. The fact GPT is kept there is a
detail of implementation, so secure_memory is the right term to use
here, and match how we call it for cpu implementation.
>> };
>>
>> 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;
> }
>
What's the benefit?
SMMUState is already heap allocated.
>> /*
>> * 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)) {
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
2025-12-12 10:50 ` Peter Maydell
@ 2025-12-12 17:26 ` Pierrick Bouvier
0 siblings, 0 replies; 8+ messages in thread
From: Pierrick Bouvier @ 2025-12-12 17:26 UTC (permalink / raw)
To: Peter Maydell, Philippe Mathieu-Daudé
Cc: qemu-devel, Leif Lindholm, tangtao1634, qemu-arm, Eric Auger,
Richard Henderson, Radoslaw Biernacki
On 12/12/25 2:50 AM, Peter Maydell wrote:
> On Fri, 12 Dec 2025 at 06:16, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> 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"?
>
> Yes -- IHI0070G.b section 3.10.2: when SMMU_S_IDR1.SECURE_IMPL=1
> "the SMMU can generate transactions to the memory system, to Secure
> PA space (NS=0) and Non-secure PA space (NS=1)".
>
> When the SMMU has Realm support then further (3.10.3)
> the output PA space can be Realm, and also GPT checks are
> always done to the Root address space.
>
> So an SMMU with Secure and Realm support can emit transactions
> to any of the 4 address spaces, the same as a CPU with those
> features.
>
> Separately from that, at the moment the way we model Realm
> and Root is by using the same QEMU MemoryRegion trees we
> have for NS and S and just using MemTxAttrs::space to distinguish,
> because we don't have any need for devices that appear only in
> Realm and not NS, or only in Root and not S. So for CPUs we
> collapse the 4 architectural address spaces down into two QEMU
> ones.
>
Yes, thanks for confirming this.
I was a bit puzzled by the fact I could not read the gpt with the global
address_space_memory, but after talking with Richard and Philippe, it
made sense as this memory is under secure memory. As a consequence, I
understood better your explaination about AddressSpace in our call on
Tuesday, so that's good to see puzzle starting to get assembled.
> For the SMMU we can presumably follow the CPU there
> (with an equivalent "give me the AddressSpace I should
> use for these MemTxAttrs" function as arm_addressspace()).
>
This is exactly what Tao introduced in his series, and that I extended
for Realm on my side.
>> My understanding is just a
>> different memory to access GPT, so I'd name it "gpt_memory".
>
> I would prefer that we use the architecturally standard
> terminology here.
>
> thanks
> -- PMM
Overall, I posted this as it's needed for both Tao's work and mine, and
instead of pushing this to Tao as part of this series, I thought it
could be included already, so we both can benefit without having to play
with series merging.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance
2025-12-12 16:45 ` Tao Tang
@ 2025-12-12 17:31 ` Pierrick Bouvier
0 siblings, 0 replies; 8+ messages in thread
From: Pierrick Bouvier @ 2025-12-12 17:31 UTC (permalink / raw)
To: Tao Tang, qemu-devel
Cc: Leif Lindholm, Philippe Mathieu-Daudé, qemu-arm, Eric Auger,
Peter Maydell, Richard Henderson, Radoslaw Biernacki
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-12-12 17:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-11 22:17 [PATCH] hw/arm/smmu: add memory regions as property for an SMMU instance Pierrick Bouvier
2025-12-11 22:21 ` Pierrick Bouvier
2025-12-12 6:16 ` Philippe Mathieu-Daudé
2025-12-12 10:50 ` Peter Maydell
2025-12-12 17:26 ` Pierrick Bouvier
2025-12-12 17:06 ` Pierrick Bouvier
2025-12-12 16:45 ` Tao Tang
2025-12-12 17:31 ` Pierrick Bouvier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).