Re: [PATCH v2 06/14] hw/arm/smmuv3: Add separate address space for secure SMMU accesses

[Eric Auger <eric.auger@redhat.com>]

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 <tangtao1634@phytium.com.cn>
>>> ---
>>>   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
>