On 9/29/25 10:33 AM, Tao Tang wrote: > 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. No problem. Your commit messages are pretty well written and we all do such kind of oversights - at least I do ;-) - Eric > >>> 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 >
