On 12/8/25 19:27, Pierrick Bouvier wrote:
On 8/11/25 3:34 AM, Philippe Mathieu-Daudé wrote:
Hi,
On 10/8/25 18:59, Tao Tang wrote:
On 2025/8/7 05:55, Pierrick Bouvier wrote:
On 8/6/25 8:11 AM, Tao Tang wrote:
This patch enables the secure command queue, providing a dedicated
interface for secure software to issue commands to the SMMU. Based on
the SMMU_S_CMDQ_BASE configuration, the SMMU now fetches command
entries directly from the Secure PA space so that we need to pass the
memory transaction attributes when reading the command queue.
This provides a parallel command mechanism to the non-secure world.
Signed-off-by: Tao Tang <tangtao1...@phytium.com.cn>
---
hw/arm/smmuv3-internal.h | 8 ++++--
hw/arm/smmuv3.c | 55 ++++++++++++++++++++++++
+---------------
hw/arm/trace-events | 2 +-
3 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 1a8b1cb204..5b2ca00832 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -319,9 +319,13 @@ static inline void queue_cons_incr(SMMUQueue *q)
q->cons = deposit32(q->cons, 0, q->log2size + 1, q->cons + 1);
}
-static inline bool smmuv3_cmdq_enabled(SMMUv3State *s)
+static inline bool smmuv3_cmdq_enabled(SMMUv3State *s, bool
is_secure)
{
- return FIELD_EX32(s->cr[0], CR0, CMDQEN);
+ if (is_secure) {
+ return FIELD_EX32(s->secure_cr[0], S_CR0, CMDQEN);
+ } else {
+ return FIELD_EX32(s->cr[0], CR0, CMDQEN);
+ }
}
This looks like a reasonable and readable approach to support secure
and non secure accesses.
Hi Pierrick,
Thank you so much for taking the time to review and for the very
positive feedback.
I'm very relieved to hear you find the approach "reasonable and
readable". I was hoping that explicitly passing the parameter was the
right way to avoid issues with global state or code duplication, and
your confirmation is the best encouragement I could ask for.
An alternative (also suggested in patch #1) is to use index for banked
registers.
For example we use M_REG_NS with Cortex-M cores (target/arm/cpu-qom.h):
/* For M profile, some registers are banked secure vs non-secure;
* these are represented as a 2-element array where the first
* element is the non-secure copy and the second is the secure
copy.
* When the CPU does not have implement the security extension then
* only the first element is used.
* This means that the copy for the current security state can be
* accessed via env->registerfield[env->v7m.secure] (whether the
* security extension is implemented or not).
*/
enum {
M_REG_NS = 0,
M_REG_S = 1,
M_REG_NUM_BANKS = 2,
};
And generally for address spaces (target/arm/cpu.h):
typedef enum ARMASIdx {
ARMASIdx_NS = 0,
ARMASIdx_S = 1,
ARMASIdx_TagNS = 2,
ARMASIdx_TagS = 3,
} ARMASIdx;
(not sure this one is appropriate here).
This could be useful, especially to not duplicate register definitions.
However, for now, we only have two indexes (secure vs non-secure) and
it's not clear if we'll need something more than that in a close future.
I think the current approach (a simple bool) is quite readable, and the
boilerplate is limited, especially versus changing all existing code to
support the new Idx approach.
As well, it can always be refactored easily later, when we'll need it.
That said, I will let the maintainers decide which approach they want to
see implemented, to avoid Tao jumping between different people opinions.
Sure, no problem.
