Re: [RFC v3 19/21] hw/arm/smmuv3: Use iommu_index to represent the security context

[Tao Tang <tangtao1634@phytium.com.cn>]

Hi Eric,

On 2025/12/4 23:05, Eric Auger wrote:
>
> On 10/20/25 10:44 AM, Tao Tang wrote:
>> Hi Pierrick,
>>
>> On 2025/10/16 15:04, Pierrick Bouvier wrote:
>>> On 10/15/25 11:37 PM, Tao Tang wrote:
>>>> Hi Pierrick:
>>>>
>>>> On 2025/10/15 08:02, Pierrick Bouvier wrote:
>>>>> Hi Tao,
>>>>>
>>>>> On 10/12/25 8:15 AM, Tao Tang wrote:
>>>>>> The Arm SMMUv3 architecture uses a SEC_SID (Secure StreamID) to
>>>>>> select
>>>>>> the programming interface. To support future extensions like RME,
>>>>>> which
>>>>>> defines four security states (Non-secure, Secure, Realm, and
>>>>>> Root), the
>>>>>> QEMU model must cleanly separate these contexts for all operations.
>>>>>>
>>>>>> This commit leverages the generic iommu_index to represent this
>>>>>> security context. The core IOMMU layer now uses the SMMU's
>>>>>> .attrs_to_index
>>>>>> callback to map a transaction's ARMSecuritySpace attribute to the
>>>>>> corresponding iommu_index.
>>>>>>
>>>>>> This index is then passed down to smmuv3_translate and used
>>>>>> throughout
>>>>>> the model to select the correct register bank and processing
>>>>>> logic. This
>>>>>> makes the iommu_index the clear QEMU equivalent of the architectural
>>>>>> SEC_SID, cleanly separating the contexts for all subsequent lookups.
>>>>>>
>>>>>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>>>>>> ---
>>>>>>     hw/arm/smmuv3.c | 36 +++++++++++++++++++++++++++++++++++-
>>>>>>     1 file changed, 35 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>>>>> index c9c742c80b..b44859540f 100644
>>>>>> --- a/hw/arm/smmuv3.c
>>>>>> +++ b/hw/arm/smmuv3.c
>>>>>> @@ -1080,6 +1080,38 @@ static void smmuv3_fixup_event(SMMUEventInfo
>>>>>> *event, hwaddr iova)
>>>>>>         }
>>>>>>     }
>>>>>>     +static SMMUSecSID smmuv3_attrs_to_sec_sid(MemTxAttrs attrs)
>>>>>> +{
>>>>>> +    switch (attrs.space) {
>>>>>> +    case ARMSS_Secure:
>>>>>> +        return SMMU_SEC_SID_S;
>>>>>> +    case ARMSS_NonSecure:
>>>>>> +    default:
>>>>>> +        return SMMU_SEC_SID_NS;
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * ARM IOMMU index mapping (implements SEC_SID from ARM SMMU):
>>>>>> + * iommu_idx = 0: Non-secure transactions
>>>>>> + * iommu_idx = 1: Secure transactions
>>>>>> + *
>>>>>> + * The iommu_idx parameter effectively implements the SEC_SID
>>>>>> + * (Security Stream ID) attribute from the ARM SMMU architecture
>>>>>> specification,
>>>>>> + * which allows the SMMU to differentiate between different security
>>>>>> state
>>>>>> + * transactions at the hardware level.
>>>>>> + */
>>>>>> +static int smmuv3_attrs_to_index(IOMMUMemoryRegion *iommu,
>>>>>> MemTxAttrs attrs)
>>>>>> +{
>>>>>> +    return (int)smmuv3_attrs_to_sec_sid(attrs);
>>>>>> +}
>>>>>> +
>>>>>> +static int smmuv3_num_indexes(IOMMUMemoryRegion *iommu)
>>>>>> +{
>>>>>> +    /* Support 2 IOMMU indexes for now: NS/S */
>>>>>> +    return SMMU_SEC_SID_NUM;
>>>>>> +}
>>>>>> +
>>>>>>     /* Entry point to SMMU, does everything. */
>>>>>>     static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr,
>>>>>> hwaddr
>>>>>> addr,
>>>>>>                                           IOMMUAccessFlags flag, int
>>>>>> iommu_idx)
>>>>>> @@ -1087,7 +1119,7 @@ static IOMMUTLBEntry
>>>>>> smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>>>>>         SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
>>>>>>         SMMUv3State *s = sdev->smmu;
>>>>>>         uint32_t sid = smmu_get_sid(sdev);
>>>>>> -    SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
>>>>>> +    SMMUSecSID sec_sid = iommu_idx;
>>>>>>         SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
>>>>>>         SMMUEventInfo event = {.type = SMMU_EVT_NONE,
>>>>>>                                .sid = sid,
>>>>>> @@ -2540,6 +2572,8 @@ static void
>>>>>> smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
>>>>>>           imrc->translate = smmuv3_translate;
>>>>>>         imrc->notify_flag_changed = smmuv3_notify_flag_changed;
>>>>>> +    imrc->attrs_to_index = smmuv3_attrs_to_index;
>>>>>> +    imrc->num_indexes = smmuv3_num_indexes;
>>>>>>     }
>>>>>>       static const TypeInfo smmuv3_type_info = {
>>>>> I noticed that this commit breaks boot of a simple Linux kernel. It
>>>>> was already the case with v2, and it seems there is a deeper issue.
>>>>>
>>>>> Virtio drive initialization hangs up with:
>>>>> [    9.421906] virtio_blk virtio2: [vda] 20971520 512-byte logical
>>>>> blocks (10.7 GB/10.0 GiB)
>>>>> smmuv3_translate_disable smmuv3-iommu-memory-region-24-3 sid=0x18
>>>>> bypass (smmu disabled) iova:0xfffff040 is_write=1
>>>>>
>>>>> You can reproduce that with any kernel/rootfs, but if you want a
>>>>> simple recipe (you need podman and qemu-user-static):
>>>>> $ git clone https://github.com/pbo-linaro/qemu-linux-stack
>>>>> $ cd qemu-linux-stack
>>>>> $ ./build_kernel.sh
>>>>> $ ./build_rootfs.sh
>>>>> $ /path/to/qemu-system-aarch64 \
>>>>> -nographic -M virt,iommu=smmuv3 -cpu max -kernel out/Image.gz \
>>>>> -append "root=/dev/vda rw" out/host.ext4 -trace 'smmuv3*'
>>>>>
>>>>> Looking more closely,
>>>>> we reach SMMU_TRANS_DISABLE, because iommu_idx associated is 1.
>>>>> This values comes from smmuv3_attrs_to_sec_sid, by reading
>>>>> attrs.space, which is ArmSS_Secure.
>>>>>
>>>>> The problem is that it's impossible to have anything Secure given that
>>>>> all the code above runs in NonSecure world.
>>>>> After investigation, the original value read from attrs.space has not
>>>>> been set anywhere, and is just the default zero-initialized value
>>>>> coming from pci_msi_trigger. It happens that it defaults to SEC_SID_S,
>>>>> which probably matches your use case with hafnium, but it's an happy
>>>>> accident.
>>>>>
>>>>> Looking at the SMMU spec, I understand that SEC_SID is configured for
>>>>> each stream, and can change dynamically.
>>>>> On the opposite, a StreamID is fixed and derived from PCI bus and slot
>>>>> for a given device.
>>>>>
>>>>> Thus, I think we are missing some logic here.
>>>>> I'm still trying to understand where the SEC_SID should come from
>>>>> initially.
>>>>> "The association between a device and the Security state of the
>>>>> programming interface is a system-defined property."
>>>>> Does it mean we should be able to set a QEMU property for any device?
>>>>>
>>>>> Does anyone familiar with this has some idea?
>>>>>
>>>>> As well, we should check the SEC_SID found based on
>>>>> SMMU_S_IDR1.SECURE_IMPL.
>>>>> 3.10.1 StreamID Security state (SEC_SID)
>>>>> If SMMU_S_IDR1.SECURE_IMPL == 0, then incoming transactions have a
>>>>> StreamID, and either:
>>>>> • A SEC_SID identifier with a value of 0.
>>>>> • No SEC_SID identifer, and SEC_SID is implicitly treated as 0.
>>>>> If SMMU_S_IDR1.SECURE_IMPL == 1, incoming transactions have a
>>>>> StreamID, and a SEC_SID identifier.
>>>>>
>>>>> Regards,
>>>>> Pierrick
>>>> Thank you very much for your detailed review and in-depth analysis, and
>>>> for pointing out this critical issue that breaks the Linux boot.
>>>>
>>>>
>>>> To be transparent, my initial approach was indeed tailored to my
>>>> specific test case, where I was effectively hardcoding the device's
>>>> StreamID to represent it's a so-called Secure device in my self
>>>> testing.
>>>> This clearly isn't a general solution.
>>>>
>>> It's definitely not a bad approach, and it's a good way to exercise
>>> the secure path. It would have been caught by some of QEMU functional
>>> tests anyway, so it's not a big deal.
>>>
>>> A solution would be to define the secure attribute as a property of
>>> the PCI device, and query that to identify sec_sid accordingly.
>>> As you'll see in 3.10.1 StreamID Security state (SEC_SID), "Whether a
>>> stream is under Secure control or not is a different property to the
>>> target PA space of a transaction.", so we definitely should *not* do
>>> any funky stuff depending on which address is accessed.
>>
>> Thank you for the encouraging and very constructive feedback.
>>
>>
>> Your proposed solution—to define the security attribute as a property
>> on the PCIDevice—is the perfect way forward to resolve Secure device
>> issue. Perhaps we can implement this functionality in V4 as shown in
>> the following code snippet?
>>
>> 1)  define sec_sid in include/hw/pci/pci_device.h:
>>
>> struct PCIDevice {
>>      DeviceState qdev;
>> ......
>>      /* Add SEC_SID property for SMMU security context */
>>      uint8_t sec_sid;  /* 0 = Non-secure, 1 = Secure*/
>> ......
>>
>> }
>>
>>
>> 2) then add sec-sid field in the Property of PCI in hw/pci/pci.c:
>>
>> static const Property pci_props[] = {
>> ......
>>      /* SEC_SID property: 0=NS, 1=S */
>>      DEFINE_PROP_UINT8("sec-sid", PCIDevice, sec_sid, 0),
>>
>> ......
>>
>> };
> As this impacts the PCIe subsystem, I would encourage to submit that
> change in a separate pre-requisite series. This needs to be reviewed by
> Michael and other PCIe specialists.
>
> Thanks
>
> Eric


OK. I'll split this feature into another patch. Thanks for the guidance!


Best regards,
Tao