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.
- 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).
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.
+ /* Secure state implemented */
+ s->secure_idr[1] = FIELD_DP32(s->secure_idr[1], S_IDR1,
+ SECURE_IMPL, 1);
+ s->secure_idr[1] = FIELD_DP32(s->secure_idr[1], S_IDR1,
+ S_SIDSIZE, SMMU_IDR1_SIDSIZE);
+
+ s->secure_gbpa = SMMU_GBPA_RESET_VAL;
}
static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
@@ -1278,6 +1290,12 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd,
SMMUStage stage)
}
}
+/* Check if the SMMU hardware itself implements secure state features */
+static inline bool smmu_hw_secure_implemented(SMMUv3State *s)
+{
+ return FIELD_EX32(s->secure_idr[1], S_IDR1, SECURE_IMPL);
+}
+
I see that the secure SMMU support is unconditional. So, is this always true?
Also, how that looks with migration?
For the v2 series, my plan is to make SECURE_IMPL a user-configurable
device property. I will add a "secure-enabled" property to
smmuv3_properties and ensure all necessary states are added to the
VMStateDescription to handle migration correctly. Does this approach
sound reasonable to you?
static int smmuv3_cmdq_consume(SMMUv3State *s)
{
SMMUState *bs = ARM_SMMU(s);
@@ -1508,9 +1526,91 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
return 0;
}
+/* Helper function for secure register write validation */
+static bool smmu_validate_secure_write(MemTxAttrs attrs, bool secure_impl,
+ hwaddr offset, const char *reg_name)
+{
+ if (!attrs.secure || !secure_impl) {
+ const char *reason = !attrs.secure ?
+ "Non-secure write attempt" :
+ "SMMU didn't implement Security State";
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: %s at offset 0x%" PRIx64 " (%s, WI)\n",
+ __func__, reason, offset, reg_name);
+ return false;
+ }
+ return true;
+}
+
+/* Helper function for secure register read validation */
+static bool smmu_validate_secure_read(MemTxAttrs attrs, bool secure_impl,
+ hwaddr offset, const char *reg_name,
+ uint64_t *data)
+{
+ if (!attrs.secure || !secure_impl) {
+ const char *reason = !attrs.secure ?
+ "Non-secure read attempt" :
+ "SMMU didn't implement Security State";
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: %s at offset 0x%" PRIx64 " (%s, RAZ)\n",
+ __func__, reason, offset, reg_name);
+ *data = 0; /* RAZ */
+ return false;
+ }
+ return true;
+}
+
+/* Macro for secure write validation - returns early if validation fails */
+#define SMMU_CHECK_SECURE_WRITE(reg_name) \
+ do { \
+ if (!smmu_validate_secure_write(attrs, secure_impl, offset, \
+ reg_name)) { \
+ return MEMTX_OK; \
+ } \
+ } while (0)
+
+/* Macro for attrs.secure only validation */
+#define SMMU_CHECK_ATTRS_SECURE(reg_name) \
+ do { \
+ if (!attrs.secure) { \
+ qemu_log_mask(LOG_GUEST_ERROR, \
+ "%s: Non-secure write attempt at offset " \
+ "0x%" PRIx64 " (%s, WI)\n", \
+ __func__, offset, reg_name); \
+ return MEMTX_OK; \
+ } \
+ } while (0)
+
+/* Macro for secure read validation - returns RAZ if validation fails */
+#define SMMU_CHECK_SECURE_READ(reg_name) \
+ do { \
+ if (!smmu_validate_secure_read(attrs, secure_impl, offset, \
+ reg_name, data)) { \
+ return MEMTX_OK; \
+ } \
+ } while (0)
+
+/* Macro for attrs.secure only validation (read) */
+#define SMMU_CHECK_ATTRS_SECURE_READ(reg_name) \
+ do { \
+ if (!attrs.secure) { \
+ qemu_log_mask(LOG_GUEST_ERROR, \
+ "%s: Non-secure read attempt at offset " \
+ "0x%" PRIx64 " (%s, RAZ)\n", \
+ __func__, offset, reg_name); \
+ *data = 0; \
+ return MEMTX_OK; \
+ } \
+ } while (0)
+
Can’t we just have one check? If the access > SMMU_SECURE_BASE_OFFSET, just
check the security state?
And then based on banking, many of those switches will be common with
non secure cases.
Thanks,
Mostafa
I have already refactored this part in my v2 series, exactly as you
proposed. This also addresses your earlier feedback on patch #1
regarding the overall structure:
As Philippe mentioned, this would be better the secure state is separated
in another instance of the struct, that seems it would reduce a lot of the
duplication later around the logic of MMIO and queues... in the next
patches.
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];
......
};
Thanks again for your valuable feedback. I've outlined my proposed plan
above and would be grateful for any thoughts on it to ensure I'm on the
right track for v2.
Best regards,
Tao
