Hi Eric, On 2025/9/29 15:44, Eric Auger wrote:
Hi Tao,On 9/25/25 6:26 PM, Tao Tang wrote:According to the Arm architecture, SMMU-originated memory accesses, such as fetching commands or writing events for a secure stream, must target the Secure Physical Address (PA) space. The existing model sends all DMA to the global address_space_memory. This patch introduces the infrastructure to differentiate between secure and non-secure memory accesses. A weak global symbol, arm_secure_address_space, is added, which can be provided by the machine model to represent the Secure PA space. A new helper, smmu_get_address_space(), selects the target address space based on the is_secure context. All internal DMA calls (dma_memory_read/write) are updated to use this helper. Additionally, the attrs.secure bit is set on transactions targeting the secure address space.The last sentence does not seem to be implemented in that patch?
You are right to point this out, and my apologies for the confusion. As I was preparing the series, the patches were intertwined, and I didn't manage their boundaries clearly. This led me to mistakenly describe a feature in this commit message that is only implemented in a subsequent patch #07.
I'm very sorry for the confusion and the unnecessary time this has cost you. In all future community interactions, I will pay special attention to ensuring each patch and its description are atomic and self-contained to reduce the review burden for everyone. Thank you for your guidance on this.
Signed-off-by: Tao Tang <[email protected]> --- hw/arm/smmu-common.c | 8 ++++++++ hw/arm/virt.c | 5 +++++ include/hw/arm/smmu-common.h | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index 62a7612184..24db448683 100644 --- a/hw/arm/smmu-common.c +++ b/hw/arm/smmu-common.c @@ -30,6 +30,14 @@ #include "hw/arm/smmu-common.h" #include "smmu-internal.h"+/* Global state for secure address space availability */+bool arm_secure_as_available; + +void smmu_enable_secure_address_space(void) +{ + arm_secure_as_available = true; +} + /* IOTLB Management */static guint smmu_iotlb_key_hash(gconstpointer v)diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 02209fadcf..805d9aadb7 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -92,6 +92,8 @@ #include "hw/cxl/cxl_host.h" #include "qemu/guest-random.h"+AddressSpace arm_secure_address_space;+ static GlobalProperty arm_virt_compat[] = { { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" }, }; @@ -2243,6 +2245,9 @@ static void machvirt_init(MachineState *machine) memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory", UINT64_MAX); memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1); + address_space_init(&arm_secure_address_space, secure_sysmem, + "secure-memory-space"); + smmu_enable_secure_address_space(); }firmware_loaded = virt_firmware_init(vms, sysmem,diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h index 3df82b83eb..cd61c5e126 100644 --- a/include/hw/arm/smmu-common.h +++ b/include/hw/arm/smmu-common.h @@ -53,6 +53,26 @@ typedef enum SMMUSecurityIndex { SMMU_SEC_IDX_NUM, } SMMUSecurityIndex;+extern AddressSpace __attribute__((weak)) arm_secure_address_space;+extern bool arm_secure_as_available; +void smmu_enable_secure_address_space(void); + +static inline AddressSpace *smmu_get_address_space(SMMUSecurityIndex sec_sid) +{ + switch (sec_sid) { + case SMMU_SEC_IDX_S: + { + if (arm_secure_as_available) { + return &arm_secure_address_space; + }don't you want to return NULL or at least emit an error in case !arm_secure_as_available. When adding Realm support this will avoid to return NS AS.
That's a great point. Silently falling back to the non-secure address space is indeed dangerous. I will update the logic to return NULL and emit an error if the secure address space is requested but not available.
+ } + QEMU_FALLTHROUGH; + case SMMU_SEC_IDX_NS: + default:Maybe return an error here in case of other value than NS
Also I will change the default case to handle unexpected values by returning NULL, which will make the code safer for future extensions like Realm. Then a check for the NULL return value at the call sites of smmu_get_address_space will be applied to handle the error appropriately in v3 series.
Thanks again for your helpful feedback. Best, Tao
+ return &address_space_memory; + } +} + /* * Page table walk error types */Thanks Eric
