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.
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 <[email protected]>
---
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.
------------------------------<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.
Thanks again for your invaluable guidance.
Best regards,
Tao
