Hi Pierrick
On 12/18/25 6:51 PM, Pierrick Bouvier wrote:
> On 12/18/25 7:51 AM, Tao Tang wrote:
>> Hi Pierrick,
>>
>> On 2025/12/17 07:57, 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.
>>>
>>> User creatable SMMU are handled as well for virt machine,
>>> by setting the memory properties when device is plugged in.
>>>
>>> Signed-off-by: Pierrick Bouvier <[email protected]>
>>> ---
>>> include/hw/arm/smmu-common.h | 4 ++++
>>> include/hw/arm/virt.h | 2 ++
>>> hw/arm/sbsa-ref.c | 16 ++++++++++++----
>>> hw/arm/smmu-common.c | 25 +++++++++++++++++++++++++
>>> hw/arm/virt.c | 13 +++++++++++--
>>> 5 files changed, 54 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/hw/arm/smmu-common.h
>>> b/include/hw/arm/smmu-common.h
>>> index 80d0fecfde8..d9bade3c803 100644
>>> --- a/include/hw/arm/smmu-common.h
>>> +++ b/include/hw/arm/smmu-common.h
>>> @@ -162,6 +162,10 @@ struct SMMUState {
>>> uint8_t bus_num;
>>> PCIBus *primary_bus;
>>> bool smmu_per_bus; /* SMMU is specific to the primary_bus */
>>> + MemoryRegion *memory;
>>> + AddressSpace memory_as;
>>> + MemoryRegion *secure_memory;
>>> + AddressSpace secure_memory_as;
>>> };
>>> struct SMMUBaseClass {
>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index c77a33f6df2..d3743810338 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -180,6 +180,8 @@ struct VirtMachineState {
>>> bool ns_el2_virt_timer_irq;
>>> CXLState cxl_devices_state;
>>> bool legacy_smmuv3_present;
>>> + MemoryRegion *sysmem;
>>> + MemoryRegion *secure_sysmem;
>>> };
>>> #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
>>> VIRT_PCIE_ECAM)
>>> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
>>> index 2205500a8da..cc9d4385826 100644
>>> --- a/hw/arm/sbsa-ref.c
>>> +++ b/hw/arm/sbsa-ref.c
>>> @@ -613,7 +613,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];
>>> @@ -625,6 +627,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++) {
>>> @@ -633,7 +639,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;
>>> @@ -689,7 +697,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)
>>> @@ -825,7 +833,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 62a76121841..9a67ce857fe 100644
>>> --- a/hw/arm/smmu-common.c
>>> +++ b/hw/arm/smmu-common.c
>>> @@ -944,6 +944,13 @@ static void smmu_base_realize(DeviceState *dev,
>>> Error **errp)
>>> return;
>>> }
>>> + g_assert(s->memory);
>>> + address_space_init(&s->memory_as, s->memory, "smmu-memory-view");
>>> + if (s->secure_memory) {
>>> + address_space_init(&s->secure_memory_as, s->secure_memory,
>>> + "smmu-secure-memory-view");
>>> + }
>>> +
>>> /*
>>> * We only allow default PCIe Root Complex(pcie.0) or
>>> pxb-pcie based extra
>>> * root complexes to be associated with SMMU.
>>> @@ -1008,10 +1015,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 25fb2bab568..603f4b6a1d7 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1514,8 +1514,7 @@ 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)
>>> {
>>> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>> int irq = vms->irqmap[VIRT_SMMU];
>>> @@ -1535,6 +1534,10 @@ static void create_smmu(const
>>> VirtMachineState *vms,
>>> }
>>> object_property_set_link(OBJECT(dev), "primary-bus",
>>> OBJECT(bus),
>>> &error_abort);
>>> + object_property_set_link(OBJECT(dev), "memory",
>>> OBJECT(vms->sysmem),
>>> + &error_abort);
>>> + object_property_set_link(OBJECT(dev), "secure-memory",
>>> OBJECT(vms->secure_sysmem),
>>> + &error_abort);
>>
>>
>> I noticed this line is over 80 characters and the corresponding one in
>> hw/arm/sbsa-ref.c also seems to exceed 80 characters. I’m not fully sure
>> whether QEMU expects us to strictly keep every line under 80 characters,
>> but when I ran scripts/checkpatch.pl I did see two warnings.
>>
>
> Thanks for reporting, it's a warning, not a hard error (90 char).
> From what I understood, I think it's ok to go over a few characters
> for cases like this, to keep code with same layout as lines around.
yes as long as it is a warning it is OK
Thanks
Eric
>
>>
>> ```
>>
>> WARNING: line over 80 characters
>> #47: FILE: hw/arm/sbsa-ref.c:632:
>> + object_property_set_link(OBJECT(dev), "secure-memory",
>> OBJECT(secure_sysmem),
>>
>> WARNING: line over 80 characters
>> #148: FILE: hw/arm/virt.c:1539:
>> + object_property_set_link(OBJECT(dev), "secure-memory",
>> OBJECT(vms->secure_sysmem),
>>
>> total: 0 errors, 2 warnings, 148 lines checked
>>
>> ```
>>
>>
>> Just flagging this in case you’d like to adjust it before the next
>> revision.
>>
>>
>> Besides, I tested the SMMU-related MemoryRegion and AddressSpace
>> instantiation for both user-creatable and machine-wide SMMUv3 devices.
>> As shown in the info mtree output below, everything looks fine on my
>> side.
>>
>>
>> ```
>>
>> (QEMU) info mtree
>> address-space: cpu-secure-memory-0
>> address-space: smmu-secure-memory-view
>> 0000000000000000-ffffffffffffffff (prio 0, i/o): secure-memory
>> 0000000000000000-0000000003ffffff (prio 0, romd): virt.flash0
>>
>> ....
>>
>> address-space: cpu-memory-0
>> address-space: gicv3-its-sysmem
>> address-space: memory
>> address-space: smmu-memory-view
>> 0000000000000000-ffffffffffffffff (prio -1, i/o): system
>> 0000000004000000-0000000007ffffff (prio 0, romd): virt.flash1
>> 0000000008000000-000000000800ffff (prio 0, i/o): gicv3_dist
>>
>> ```
>>
>
> Thanks for testing :)!
>
>>
>>
>> Regards,
>> Tao
>>
>>> 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++) {
>>> @@ -1609,6 +1612,7 @@ static void create_pcie(VirtMachineState *vms)
>>> memory_region_init_alias(ecam_alias, OBJECT(dev), "pcie-ecam",
>>> ecam_reg, 0, size_ecam);
>>> memory_region_add_subregion(get_system_memory(), base_ecam,
>>> ecam_alias);
>>> + vms->sysmem = get_system_memory();
>>> /* Map the MMIO window into system address space so as to
>>> expose
>>> * the section of PCI MMIO space which starts at the same
>>> base address
>>> @@ -2256,6 +2260,7 @@ static void machvirt_init(MachineState *machine)
>>> * devices go in at higher priority and take precedence.
>>> */
>>> secure_sysmem = g_new(MemoryRegion, 1);
>>> + vms->secure_sysmem = secure_sysmem;
>>> memory_region_init(secure_sysmem, OBJECT(machine),
>>> "secure-memory",
>>> UINT64_MAX);
>>> memory_region_add_subregion_overlap(secure_sysmem, 0,
>>> sysmem, -1);
>>> @@ -3051,6 +3056,10 @@ static void
>>> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>> } else if (vms->iommu == VIRT_IOMMU_NONE) {
>>> /* The new SMMUv3 device is specific to the PCI bus */
>>> object_property_set_bool(OBJECT(dev), "smmu_per_bus",
>>> true, NULL);
>>> + object_property_set_link(OBJECT(dev), "memory",
>>> + OBJECT(vms->sysmem), NULL);
>>> + object_property_set_link(OBJECT(dev), "secure-memory",
>>> + OBJECT(vms->secure_sysmem),
>>> NULL);
>>> }
>>> }
>>> }
>>
>
> Cheers,
> Pierrick
>
