Hi Tao,
On 9/15/25 11:14 AM, Mostafa Saleh wrote:
> Hi Tao,
>
> On Thu, Sep 11, 2025 at 11:27:50PM +0800, Tao Tang wrote:
>> Hi Mostafa,
>>
>> First, my apologies for the long delay in getting back to you. I was away on
>> paternity leave for the last few weeks.
> No worries!
>
>> Thank you for the detailed follow-up, your advice is very helpful for
>> simplifying the series.
>>
>>
>> On 2025/8/23 18:41, Mostafa Saleh wrote:
>>> On Wed, Aug 20, 2025 at 11:21:02PM +0800, Tao Tang wrote:
>>>> On 2025/8/19 05:24, Mostafa Saleh wrote:
>>>>> On Wed, Aug 06, 2025 at 11:11:25PM +0800, Tao Tang wrote:
>>>>>> This patch builds upon the previous introduction of secure register
>>>>>> definitions by providing the functional implementation for their access.
>>>>>>
>>>>>> The availability of the secure programming interface is now correctly
>>>>>> gated by the S_IDR1.SECURE_IMPL bit. When this bit indicates that
>>>>>> secure functionality is enabled, the I/O handlers (smmuv3_read and
>>>>>> smmuv3_write) will correctly dispatch accesses to the secure
>>>>>> register space.
>>>>>>
>>>>>> Signed-off-by: Tao Tang <tangtao1...@phytium.com.cn>
>>>>>> ---
>>>>>> hw/arm/smmuv3-internal.h | 5 +
>>>>>> hw/arm/smmuv3.c | 451 +++++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 456 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>>>>>> index 483aaa915e..1a8b1cb204 100644
>>>>>> --- a/hw/arm/smmuv3-internal.h
>>>>>> +++ b/hw/arm/smmuv3-internal.h
>>>>>> @@ -122,6 +122,11 @@ REG32(CR0, 0x20)
>>>>>> #define SMMU_CR0_RESERVED 0xFFFFFC20
>>>>>> +/*
>>>>>> + * BIT1 and BIT4 are RES0 in SMMU_S_CRO
>>>>>> + */
>>>>>> +#define SMMU_S_CR0_RESERVED 0xFFFFFC12
>>>>>> +
>>>>>> REG32(CR0ACK, 0x24)
>>>>>> REG32(CR1, 0x28)
>>>>>> REG32(CR2, 0x2c)
>>>>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>>>>> index ab67972353..619180d204 100644
>>>>>> --- a/hw/arm/smmuv3.c
>>>>>> +++ b/hw/arm/smmuv3.c
>>>>>> @@ -317,6 +317,18 @@ static void smmuv3_init_regs(SMMUv3State *s)
>>>>>> s->gerrorn = 0;
>>>>>> s->statusr = 0;
>>>>>> s->gbpa = SMMU_GBPA_RESET_VAL;
>>>>>> +
>>>>>> + /* Initialize secure state */
>>>>>> + memset(s->secure_idr, 0, sizeof(s->secure_idr));
>>>>>> + /* Secure EL2 and Secure stage 2 support */
>>>>>> + s->secure_idr[1] = FIELD_DP32(s->secure_idr[1], S_IDR1, SEL2, 1);
>>>>> AFAIU, this is wrong, SEL2 means that the SMMU has dual stage-2,
>>>>> one for secure (S_S2TTB) and one for non-secure IPAs(S2TTB).
>>>>> Which is not implemented in this series.
>>>> Hi Mostafa,
>>>>
>>>> Thank you for the very detailed and helpful review. Your feedback is spot
>>>> on, and I'd like to address your points and ask for a quick confirmation on
>>>> them.
>>>>
>>>> Regarding the SEL2 bit, you are absolutely right, my understanding was
>>>> incorrect. I've spent the last few days reviewing the manual to better
>>>> understand the selection between Secure and Non-secure Stage 2
>>>> translations.
>>>> I would be very grateful if you could confirm if my new understanding is
>>>> correct:
>>>>
>>>> - In Stage 2-only mode (Stage 1 bypassed), the choice between a Secure or
>>>> Non-secure IPA translation is determined solely by STE.NSCFG.
>>>>
>>> Yes, but that's only with SMMU_IDR1.ATTR_PERMS_OVR which Qemu doesn't
>>> advertise, so in our case it's always secure.
>>>
>>>> - In Stage 1-enabled mode, STE.NSCFG is ignored. The choice is determined
>>>> by
>>>> the translation process, starting from CD.NSCFGx, with the output NS
>>>> attribute being the result of intermediate NSTable flags and the final
>>>> descriptor.NS bit (TTD.NSTable, TTD.NS).
>>>>
>>> You have to differentiate between the security state of the translation and
>>> the security state of the translation table access.
>>>
>>> For stage-1, the security state is determined by the NS bit in the last
>>> level PTE, which in case of nested translation it will choose between S2TTB
>>> or S_S2TTB.
>>>
>>> Also, note that the stage-2 also have an NS which define the final attribute
>>> of the transaction.
>>>
>>> You have to also be careful around things such as NSCFG{0,1} as it might
>>> change which stage-2 is used for the stage-1 TTB walk.
>>>
>>> I see, in your patches, all the page-table access is done using the secure
>>> state of the SID which is not correct.
>>>
>>>
>>>> Based on this, I plan to have an internal flag, perhaps named
>>>> target_ipa_is_ns in SMMUTransCfg.SMMUS2Cfg struct, to track the outcome of
>>>> this process. This flag will then determine whether S_S2TTB or S2TTB is
>>>> used
>>>> for the Stage 2 translation.
>>>>
>>> I am worried that it's not that simple for a single secure nested
>>> translation
>>> you can have multiple stage-2 walks where some might be secure and others
>>> not,
>>> so I imagine this some how will be determined from each stage-1 walk and
>>> some how returned (maybe in the TLB struct) which is then the stage-2
>>> walk looks into.
>>>
>>> I am not sure how complicated it is to manage 2 stage-2 with the current
>>> code
>>> base, so my advice would be to split the problem; for now you can drop SEL2
>>> from this series and rely on NS stage-2.
>>
>> I would like to confirm my understanding of the implementation. Does this
>> mean that for the current RFC, we should set S_IDR1.SEL2=0, which implies
>> that all Stage-2 translations will begin with a Non-secure IPA? And the
>> final output PA space will then almost always be Non-secure PA, with the
>> sole exception being when S2SW, S2SA, S2NSW, and S2NSA are ALL ZERO.
>>
>>
>> However, since these fields are RES0 when S_IDR1.SEL2=0, it seems we can
>> conclude that for this version, the output will definitively be a Non-secure
>> PA. I believe this is what you meant by your advice to "rely on NS stage-2".
>> I would be grateful if you could let me know whether this interpretation is
>> on the right track.
> Yes, that’s what I meant, I think that simplifies a lot, in this series we
> can focus on the infrastructure for the secure SMMU (registers, TLBs..),
> and then extra features such as secure stage-2 can be added later.
>
>>
>> ------------------------------<snip>------------------------------
>>
>>>> The new code performs a single, necessary security state check at the entry
>>>> point of the MMIO handlers. The rest of the logic relies on the banking
>>>> mechanism, which makes the implementation generic for Non-secure, Secure,
>>>> and future states like Realm/Root. The new structure looks like this:
>>>>
>>>> /* Structure for one register bank */
>>>> typedef struct SMMUv3Bank {
>>>> uint32_t idr[6]; /* IDR0-IDR5, note: IDR5 only used for NS bank */
>>>> uint32_t cr[3]; /* CR0-CR2 */
>>>> uint32_t cr0ack;
>>>> uint32_t init; /* S_INIT register (secure only), reserved for NS
>>>> */
>>>> uint32_t gbpa;
>>>>
>>>> ......
>>>>
>>>> SMMUQueue eventq, cmdq;
>>>> } SMMUv3Bank;
>>>>
>>>> struct SMMUv3State {
>>>> SMMUState smmu_state;
>>>>
>>>> /* Shared (non-banked) registers and state */
>>>> uint32_t features;
>>>> uint8_t sid_size;
>>>> uint8_t sid_split;
>>>>
>>>> ......
>>>>
>>>> /* Banked registers for all access */
>>>> SMMUv3Bank bank[SMMU_SEC_IDX_NUM];
>>>> ......
>>>> };
>>>>
>>> Yes, IMO,that’s the right approach. Although that might make the
>>> migration code more complicated as we changed the state struct.
>>>
>>> Thanks,
>>> Mostafa
>> I have almost completed the refactoring based on this new structure, and I
>> will send out the v2 patch series in the next few days for review.
> Sounds good!
Sorry for having failed to review the RFC on time. Thanks to other
reviewers I think you've got quite a lot of feedbacks already. I will
review v2.
Looking forward to receiving your respin.
Thanks
Eric
>
> Thanks,
> Mostafa
>
>> Thanks again for your invaluable guidance.
>>
>> Best regards,
>>
>> Tao
>>
