[PATCH 0/2] target/arm: make granule_protection_check usable from SMMU

[PATCH 0/2] target/arm: make granule_protection_check usable from SMMU

[Pierrick Bouvier]

This series prepare granule_protection_check to be usable from SMMU, for
implementing RME feature.
It's based on Tao's commit [1] extracting ARMSecuritySpace from cpu.h header for
convenience.

[1] https://lore.kernel.org/qemu-devel/20251012150701.4127034-5-tangtao1634@phytium.com.cn/

To demonstrate the purpose, this is the (wip) change to use that from SMMU:

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 80f48df3dda..1acff3bbd66 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1055,12 +1056,36 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
     }

     cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info);
     if (desc_s2_translation) {
         cfg->asid = asid;
         cfg->stage = stage;
     }

+    if (cached_entry) {
+        hwaddr paddress = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
+        ARMSecuritySpace pspace = sec_sid_to_security_space(cfg->sec_sid);
+        ARMSecuritySpace ss = ARMSS_Root;
+        ARMMMUFaultInfo fi;
+
+        ARMGranuleProtectionConfig gpc = {
+            .gpccr = s->root.gpt_base_cfg,
+            .gptbr = s->root.gpt_base >> 12,
+            .parange = 6, /* 52 bits */
+            .support_sel2 = false,
+            .as_secure = &s->smmu_state.as_secure_memory
+        };
+        /* The fields in SMMU_ROOT_GPT_BASE_CFG are the same as for GPCCR_EL3,
+         * except there is no copy of GPCCR_EL3.GPC. See SMMU_ROOT_CR0.GPCEN. */
+        const bool gpc_enabled = FIELD_EX32(s->root.cr0, ROOT_CR0, GPCEN);
+        gpc.gpccr = FIELD_DP64(gpc.gpccr, GPCCR, GPC, gpc_enabled);
+        bool gpc_check = arm_granule_protection_check(gpc, paddress,
+                                                      pspace, ss, &fi);
+        if (!gpc_check) {
+            printf("ERROR: fi.type=%d fi.gpcf=%d\n", fi.type, fi.gpcf);
+            g_assert_not_reached();
+        }
+    }
+
     if (!cached_entry) {
         /* All faults from PTW has S2 field. */
         event->u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2);

Pierrick Bouvier (1):
  target/arm/ptw: make granule_protection_check usable without a cpu

Tao Tang (1):
  target/arm: Move ARMSecuritySpace to a common header

 include/hw/arm/arm-security.h | 54 +++++++++++++++++++++++++++++++++++
 target/arm/cpu.h              | 39 ++++++++++---------------
 target/arm/ptw.c              | 41 +++++++++++++++-----------
 3 files changed, 93 insertions(+), 41 deletions(-)
 create mode 100644 include/hw/arm/arm-security.h

-- 
2.47.3

[PATCH 1/2] target/arm: Move ARMSecuritySpace to a common header

[Pierrick Bouvier]

From: Tao Tang <tangtao1634@phytium.com.cn>

The ARMSecuritySpace enum and its related helpers were defined in the
target-specific header target/arm/cpu.h. This prevented common,
target-agnostic code like the SMMU model from using these definitions
without triggering "cpu.h included from common code" errors.

To resolve this, this commit introduces a new, lightweight header,
include/hw/arm/arm-security.h, which is safe for inclusion by common
code.

The following change was made:

- The ARMSecuritySpace enum and the arm_space_is_secure() and
arm_secure_to_space() helpers have been moved from target/arm/cpu.h
to the new hw/arm/arm-security.h header.

This refactoring decouples the security state definitions from the core
CPU implementation, allowing common hardware models to correctly handle
security states without pulling in heavyweight, target-specific headers.

Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Link: https://lists.nongnu.org/archive/html/qemu-arm/2025-09/msg01288.html
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/hw/arm/arm-security.h | 54 +++++++++++++++++++++++++++++++++++
 target/arm/cpu.h              | 25 +---------------
 2 files changed, 55 insertions(+), 24 deletions(-)
 create mode 100644 include/hw/arm/arm-security.h

diff --git a/include/hw/arm/arm-security.h b/include/hw/arm/arm-security.h
new file mode 100644
index 00000000000..9664c0f95e9
--- /dev/null
+++ b/include/hw/arm/arm-security.h
@@ -0,0 +1,54 @@
+/*
+ * ARM security space helpers
+ *
+ * Provide ARMSecuritySpace and helpers for code that is not tied to CPU.
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_ARM_ARM_SECURITY_H
+#define HW_ARM_ARM_SECURITY_H
+
+#include <stdbool.h>
+
+/*
+ * ARM v9 security states.
+ * The ordering of the enumeration corresponds to the low 2 bits
+ * of the GPI value, and (except for Root) the concat of NSE:NS.
+ */
+
+ typedef enum ARMSecuritySpace {
+    ARMSS_Secure     = 0,
+    ARMSS_NonSecure  = 1,
+    ARMSS_Root       = 2,
+    ARMSS_Realm      = 3,
+} ARMSecuritySpace;
+
+/* Return true if @space is secure, in the pre-v9 sense. */
+static inline bool arm_space_is_secure(ARMSecuritySpace space)
+{
+    return space == ARMSS_Secure || space == ARMSS_Root;
+}
+
+/* Return the ARMSecuritySpace for @secure, assuming !RME or EL[0-2]. */
+static inline ARMSecuritySpace arm_secure_to_space(bool secure)
+{
+    return secure ? ARMSS_Secure : ARMSS_NonSecure;
+}
+
+#endif /* HW_ARM_ARM_SECURITY_H */
+
+
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 39f2b2e54de..efbef0341da 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -31,6 +31,7 @@
 #include "exec/page-protection.h"
 #include "qapi/qapi-types-common.h"
 #include "target/arm/multiprocessing.h"
+#include "hw/arm/arm-security.h"
 #include "target/arm/gtimer.h"
 #include "target/arm/cpu-sysregs.h"
 #include "target/arm/mmuidx.h"
@@ -2102,30 +2103,6 @@ static inline int arm_feature(CPUARMState *env, int feature)
 
 void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp);
 
-/*
- * ARM v9 security states.
- * The ordering of the enumeration corresponds to the low 2 bits
- * of the GPI value, and (except for Root) the concat of NSE:NS.
- */
-
-typedef enum ARMSecuritySpace {
-    ARMSS_Secure     = 0,
-    ARMSS_NonSecure  = 1,
-    ARMSS_Root       = 2,
-    ARMSS_Realm      = 3,
-} ARMSecuritySpace;
-
-/* Return true if @space is secure, in the pre-v9 sense. */
-static inline bool arm_space_is_secure(ARMSecuritySpace space)
-{
-    return space == ARMSS_Secure || space == ARMSS_Root;
-}
-
-/* Return the ARMSecuritySpace for @secure, assuming !RME or EL[0-2]. */
-static inline ARMSecuritySpace arm_secure_to_space(bool secure)
-{
-    return secure ? ARMSS_Secure : ARMSS_NonSecure;
-}
 
 #if !defined(CONFIG_USER_ONLY)
 /**
-- 
2.47.3

Re: [PATCH 1/2] target/arm: Move ARMSecuritySpace to a common header

[Richard Henderson]

On 12/11/25 17:44, Pierrick Bouvier wrote:
> From: Tao Tang <tangtao1634@phytium.com.cn>
> 
> The ARMSecuritySpace enum and its related helpers were defined in the
> target-specific header target/arm/cpu.h. This prevented common,
> target-agnostic code like the SMMU model from using these definitions
> without triggering "cpu.h included from common code" errors.
> 
> To resolve this, this commit introduces a new, lightweight header,
> include/hw/arm/arm-security.h, which is safe for inclusion by common
> code.
> 
> The following change was made:
> 
> - The ARMSecuritySpace enum and the arm_space_is_secure() and
> arm_secure_to_space() helpers have been moved from target/arm/cpu.h
> to the new hw/arm/arm-security.h header.
> 
> This refactoring decouples the security state definitions from the core
> CPU implementation, allowing common hardware models to correctly handle
> security states without pulling in heavyweight, target-specific headers.
> 
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Link: https://lists.nongnu.org/archive/html/qemu-arm/2025-09/msg01288.html
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/hw/arm/arm-security.h | 54 +++++++++++++++++++++++++++++++++++
>   target/arm/cpu.h              | 25 +---------------
>   2 files changed, 55 insertions(+), 24 deletions(-)
>   create mode 100644 include/hw/arm/arm-security.h
> 
> diff --git a/include/hw/arm/arm-security.h b/include/hw/arm/arm-security.h
> new file mode 100644
> index 00000000000..9664c0f95e9
> --- /dev/null
> +++ b/include/hw/arm/arm-security.h
> @@ -0,0 +1,54 @@
> +/*
> + * ARM security space helpers
> + *
> + * Provide ARMSecuritySpace and helpers for code that is not tied to CPU.
> + *
> + *  Copyright (c) 2003 Fabrice Bellard
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */

Don't add the boilerplate to new files, just SPDX-License-Identifier.
This should have tripped a checkpatch error.

> +
> +#ifndef HW_ARM_ARM_SECURITY_H
> +#define HW_ARM_ARM_SECURITY_H
> +
> +#include <stdbool.h>

Already included by osdep.h.

> +#endif /* HW_ARM_ARM_SECURITY_H */
> +
> +

Watch the extra newlines.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/2] target/arm: Move ARMSecuritySpace to a common header

[Pierrick Bouvier]

On 12/12/25 6:56 AM, Richard Henderson wrote:
> On 12/11/25 17:44, Pierrick Bouvier wrote:
>> From: Tao Tang <tangtao1634@phytium.com.cn>
>>
>> The ARMSecuritySpace enum and its related helpers were defined in the
>> target-specific header target/arm/cpu.h. This prevented common,
>> target-agnostic code like the SMMU model from using these definitions
>> without triggering "cpu.h included from common code" errors.
>>
>> To resolve this, this commit introduces a new, lightweight header,
>> include/hw/arm/arm-security.h, which is safe for inclusion by common
>> code.
>>
>> The following change was made:
>>
>> - The ARMSecuritySpace enum and the arm_space_is_secure() and
>> arm_secure_to_space() helpers have been moved from target/arm/cpu.h
>> to the new hw/arm/arm-security.h header.
>>
>> This refactoring decouples the security state definitions from the core
>> CPU implementation, allowing common hardware models to correctly handle
>> security states without pulling in heavyweight, target-specific headers.
>>
>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Link: https://lists.nongnu.org/archive/html/qemu-arm/2025-09/msg01288.html
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/hw/arm/arm-security.h | 54 +++++++++++++++++++++++++++++++++++
>>    target/arm/cpu.h              | 25 +---------------
>>    2 files changed, 55 insertions(+), 24 deletions(-)
>>    create mode 100644 include/hw/arm/arm-security.h
>>
>> diff --git a/include/hw/arm/arm-security.h b/include/hw/arm/arm-security.h
>> new file mode 100644
>> index 00000000000..9664c0f95e9
>> --- /dev/null
>> +++ b/include/hw/arm/arm-security.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * ARM security space helpers
>> + *
>> + * Provide ARMSecuritySpace and helpers for code that is not tied to CPU.
>> + *
>> + *  Copyright (c) 2003 Fabrice Bellard
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> + */
> 
> Don't add the boilerplate to new files, just SPDX-License-Identifier.
> This should have tripped a checkpatch error.
> 
>> +
>> +#ifndef HW_ARM_ARM_SECURITY_H
>> +#define HW_ARM_ARM_SECURITY_H
>> +
>> +#include <stdbool.h>
> 
> Already included by osdep.h.
> 
>> +#endif /* HW_ARM_ARM_SECURITY_H */
>> +
>> +
> 
> Watch the extra newlines.
> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~

I'll fix the nits you reported, just included this patch from Tao's series.

[PATCH 2/2] target/arm/ptw: make granule_protection_check usable without a cpu

[Pierrick Bouvier]

By removing cpu details and use a config struct, we can use the
same granule_protection_check with other devices, like SMMU.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/arm/cpu.h | 14 ++++++++++++++
 target/arm/ptw.c | 41 ++++++++++++++++++++++++-----------------
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index efbef0341da..38cc5823a93 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1216,6 +1216,20 @@ void arm_v7m_cpu_do_interrupt(CPUState *cpu);
 
 hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
                                          MemTxAttrs *attrs);
+
+typedef struct ARMGranuleProtectionConfig {
+    uint64_t gpccr;
+    uint64_t gptbr;
+    uint8_t parange;
+    bool support_sel2;
+    AddressSpace *as_secure;
+} ARMGranuleProtectionConfig;
+
+bool arm_granule_protection_check(ARMGranuleProtectionConfig config,
+                                  uint64_t paddress,
+                                  ARMSecuritySpace pspace,
+                                  ARMSecuritySpace ss,
+                                  ARMMMUFaultInfo *fi);
 #endif /* !CONFIG_USER_ONLY */
 
 int arm_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2e6b149b2d1..2b620b03014 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -330,24 +330,23 @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
     return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
 }
 
-static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
-                                     ARMSecuritySpace pspace,
-                                     ARMSecuritySpace ss,
-                                     ARMMMUFaultInfo *fi)
+bool arm_granule_protection_check(ARMGranuleProtectionConfig config,
+                                  uint64_t paddress,
+                                  ARMSecuritySpace pspace,
+                                  ARMSecuritySpace ss,
+                                  ARMMMUFaultInfo *fi)
 {
     MemTxAttrs attrs = {
         .secure = true,
         .space = ARMSS_Root,
     };
-    ARMCPU *cpu = env_archcpu(env);
-    uint64_t gpccr = env->cp15.gpccr_el3;
+    const uint64_t gpccr = config.gpccr;
     unsigned pps, pgs, l0gptsz, level = 0;
     uint64_t tableaddr, pps_mask, align, entry, index;
-    AddressSpace *as;
     MemTxResult result;
     int gpi;
 
-    if (!FIELD_EX64(gpccr, GPCCR, GPC)) {
+    if (!FIELD_EX64(config.gpccr, GPCCR, GPC)) {
         return true;
     }
 
@@ -362,7 +361,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
      * physical address size is invalid.
      */
     pps = FIELD_EX64(gpccr, GPCCR, PPS);
-    if (pps > FIELD_EX64_IDREG(&cpu->isar, ID_AA64MMFR0, PARANGE)) {
+    if (pps > config.parange) {
         goto fault_walk;
     }
     pps = pamax_map[pps];
@@ -432,7 +431,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
     }
 
     /* GPC Priority 4: the base address of GPTBR_EL3 exceeds PPS. */
-    tableaddr = env->cp15.gptbr_el3 << 12;
+    tableaddr = config.gptbr << 12;
     if (tableaddr & ~pps_mask) {
         goto fault_size;
     }
@@ -446,12 +445,10 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
     align = MAKE_64BIT_MASK(0, align);
     tableaddr &= ~align;
 
-    as = arm_addressspace(env_cpu(env), attrs);
-
     /* Level 0 lookup. */
     index = extract64(paddress, l0gptsz, pps - l0gptsz);
     tableaddr += index * 8;
-    entry = address_space_ldq_le(as, tableaddr, attrs, &result);
+    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);
     if (result != MEMTX_OK) {
         goto fault_eabt;
     }
@@ -479,7 +476,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
     level = 1;
     index = extract64(paddress, pgs + 4, l0gptsz - pgs - 4);
     tableaddr += index * 8;
-    entry = address_space_ldq_le(as, tableaddr, attrs, &result);
+    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);
     if (result != MEMTX_OK) {
         goto fault_eabt;
     }
@@ -513,7 +510,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
     case 0b1111: /* all access */
         return true;
     case 0b1000: /* secure */
-        if (!cpu_isar_feature(aa64_sel2, cpu)) {
+        if (!config.support_sel2) {
             goto fault_walk;
         }
         /* fall through */
@@ -3786,8 +3783,18 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
                             memop, result, fi)) {
         return true;
     }
-    if (!granule_protection_check(env, result->f.phys_addr,
-                                  result->f.attrs.space, ptw->in_space, fi)) {
+
+    ARMCPU *cpu = env_archcpu(env);
+    struct ARMGranuleProtectionConfig gpc = {
+        .gpccr = env->cp15.gpccr_el3,
+        .gptbr = env->cp15.gptbr_el3,
+        .parange = FIELD_EX64_IDREG(&cpu->isar, ID_AA64MMFR0, PARANGE),
+        .support_sel2 = cpu_isar_feature(aa64_sel2, cpu),
+        .as_secure = cpu_get_address_space(env_cpu(env), ARMASIdx_S)
+    };
+    if (!arm_granule_protection_check(gpc, result->f.phys_addr,
+                                      result->f.attrs.space, ptw->in_space,
+                                      fi)) {
         fi->type = ARMFault_GPCFOnOutput;
         return true;
     }
-- 
2.47.3

Re: [PATCH 2/2] target/arm/ptw: make granule_protection_check usable without a cpu

[Peter Maydell]

On Thu, 11 Dec 2025 at 23:44, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> By removing cpu details and use a config struct, we can use the
> same granule_protection_check with other devices, like SMMU.
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

I'm not 100% sure about this approach, mainly because for SMMU
so far we have taken the route of having its page table
walk implementation be completely separate from the one in
the MMU, even though it's pretty similar: the spec for
CPU page table walk and the one for SMMU page table walk
are technically in different documents and don't necessarily
proceed 100% in sync. Still, the function is a pretty big
one and our other option would probably end up being
copy-and-paste, which isn't very attractive.

So my comments below are minor things.

> ---
>  target/arm/cpu.h | 14 ++++++++++++++
>  target/arm/ptw.c | 41 ++++++++++++++++++++++++-----------------
>  2 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index efbef0341da..38cc5823a93 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1216,6 +1216,20 @@ void arm_v7m_cpu_do_interrupt(CPUState *cpu);
>
>  hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
>                                           MemTxAttrs *attrs);
> +
> +typedef struct ARMGranuleProtectionConfig {
> +    uint64_t gpccr;
> +    uint64_t gptbr;
> +    uint8_t parange;
> +    bool support_sel2;
> +    AddressSpace *as_secure;
> +} ARMGranuleProtectionConfig;
> +
> +bool arm_granule_protection_check(ARMGranuleProtectionConfig config,
> +                                  uint64_t paddress,
> +                                  ARMSecuritySpace pspace,
> +                                  ARMSecuritySpace ss,
> +                                  ARMMMUFaultInfo *fi);

Could we have a doc comment for these prototypes, please?

>  #endif /* !CONFIG_USER_ONLY */
>
>  int arm_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 2e6b149b2d1..2b620b03014 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -330,24 +330,23 @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
>      return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
>  }
>
> -static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
> -                                     ARMSecuritySpace pspace,
> -                                     ARMSecuritySpace ss,
> -                                     ARMMMUFaultInfo *fi)
> +bool arm_granule_protection_check(ARMGranuleProtectionConfig config,
> +                                  uint64_t paddress,
> +                                  ARMSecuritySpace pspace,
> +                                  ARMSecuritySpace ss,
> +                                  ARMMMUFaultInfo *fi)
>  {

A comment here noting that we share this function with the SMMU
would probably help us in avoiding inadvertently introducing
CPU-specifics in future.

>      MemTxAttrs attrs = {
>          .secure = true,
>          .space = ARMSS_Root,
>      };
> -    ARMCPU *cpu = env_archcpu(env);
> -    uint64_t gpccr = env->cp15.gpccr_el3;
> +    const uint64_t gpccr = config.gpccr;
>      unsigned pps, pgs, l0gptsz, level = 0;
>      uint64_t tableaddr, pps_mask, align, entry, index;
> -    AddressSpace *as;
>      MemTxResult result;
>      int gpi;
>
> -    if (!FIELD_EX64(gpccr, GPCCR, GPC)) {
> +    if (!FIELD_EX64(config.gpccr, GPCCR, GPC)) {

We just set up the 'gpccr' local so we don't need to change this line,
I think ?

Also, the SMMU's SMMU_ROOT_GPT_BASE_CFG does not have the GPC field
(it keeps its enable bit elsewhere).

>          return true;
>      }
>
> @@ -362,7 +361,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>       * physical address size is invalid.
>       */
>      pps = FIELD_EX64(gpccr, GPCCR, PPS);
> -    if (pps > FIELD_EX64_IDREG(&cpu->isar, ID_AA64MMFR0, PARANGE)) {
> +    if (pps > config.parange) {
>          goto fault_walk;
>      }
>      pps = pamax_map[pps];
> @@ -432,7 +431,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>      }
>
>      /* GPC Priority 4: the base address of GPTBR_EL3 exceeds PPS. */
> -    tableaddr = env->cp15.gptbr_el3 << 12;
> +    tableaddr = config.gptbr << 12;
>      if (tableaddr & ~pps_mask) {
>          goto fault_size;
>      }
> @@ -446,12 +445,10 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>      align = MAKE_64BIT_MASK(0, align);
>      tableaddr &= ~align;
>
> -    as = arm_addressspace(env_cpu(env), attrs);
> -
>      /* Level 0 lookup. */
>      index = extract64(paddress, l0gptsz, pps - l0gptsz);
>      tableaddr += index * 8;
> -    entry = address_space_ldq_le(as, tableaddr, attrs, &result);
> +    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);

as_secure is an odd name for the AS here, because architecturally
GPT walks are done to the Root physical address space. (This is
why in the current code we set attrs.space to ARMSS_Root and then
get the QEMU AddressSpace corresponding to those attrs. It happens
that at the moment that's the same one we use as Secure, but in
theory we could have 4 completely separate ones for NS, S, Root
and Realm.)

>      if (result != MEMTX_OK) {
>          goto fault_eabt;
>      }
> @@ -479,7 +476,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>      level = 1;
>      index = extract64(paddress, pgs + 4, l0gptsz - pgs - 4);
>      tableaddr += index * 8;
> -    entry = address_space_ldq_le(as, tableaddr, attrs, &result);
> +    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);
>      if (result != MEMTX_OK) {
>          goto fault_eabt;
>      }
> @@ -513,7 +510,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>      case 0b1111: /* all access */
>          return true;
>      case 0b1000: /* secure */
> -        if (!cpu_isar_feature(aa64_sel2, cpu)) {
> +        if (!config.support_sel2) {
>              goto fault_walk;
>          }
>          /* fall through */
> @@ -3786,8 +3783,18 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
>                              memop, result, fi)) {
>          return true;
>      }
> -    if (!granule_protection_check(env, result->f.phys_addr,
> -                                  result->f.attrs.space, ptw->in_space, fi)) {
> +
> +    ARMCPU *cpu = env_archcpu(env);
> +    struct ARMGranuleProtectionConfig gpc = {
> +        .gpccr = env->cp15.gpccr_el3,
> +        .gptbr = env->cp15.gptbr_el3,
> +        .parange = FIELD_EX64_IDREG(&cpu->isar, ID_AA64MMFR0, PARANGE),
> +        .support_sel2 = cpu_isar_feature(aa64_sel2, cpu),
> +        .as_secure = cpu_get_address_space(env_cpu(env), ARMASIdx_S)

Directly coding ARMASIDx_S here is a bit awkward, as noted above.

> +    };
> +    if (!arm_granule_protection_check(gpc, result->f.phys_addr,
> +                                      result->f.attrs.space, ptw->in_space,
> +                                      fi)) {
>          fi->type = ARMFault_GPCFOnOutput;
>          return true;
>      }

thanks
-- PMM

Re: [PATCH 2/2] target/arm/ptw: make granule_protection_check usable without a cpu

[Pierrick Bouvier]

On 12/12/25 2:35 AM, Peter Maydell wrote:
> On Thu, 11 Dec 2025 at 23:44, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> By removing cpu details and use a config struct, we can use the
>> same granule_protection_check with other devices, like SMMU.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> I'm not 100% sure about this approach, mainly because for SMMU
> so far we have taken the route of having its page table
> walk implementation be completely separate from the one in
> the MMU, even though it's pretty similar: the spec for
> CPU page table walk and the one for SMMU page table walk
> are technically in different documents and don't necessarily
> proceed 100% in sync. Still, the function is a pretty big
> one and our other option would probably end up being
> copy-and-paste, which isn't very attractive.
>

I understand your concerns, and will try my best to answer ot it.
I mentioned this approach to Richard yesterday and asked if it was 
something that we could do, and he had a positive feedback, at least in 
my perception.

For PTW, we depend on softmmu, which is why it's not reused on SMMU 
side, which doesn't use softmmu. For Granule Protection, we don't rely 
on it (so far), so I was tempted to make things common rather than 
duplicate this.

SMMU spec does not have a specific GPC or PTW description nor pseudo 
code, it just indicates to follow A-profile spec. The difference is in 
details, like which registers contains expected value, and the GPC field 
that is inconsistently stored somewhere else. Thus the idea to isolate 
this in a "config" structure and reuse the implementation. The same 
could have been done with PTW if we didn't rely on softmmu, or if SMMU 
used it, but that's a topic for another day.

I'm not sure from your paragraph if you are open to it or not, so it 
would help if you could be more explicit. Maybe giving a review is a way 
to say yes, but my brain firmware does not have the "indirect 
communication style" upgrade yet :).

> So my comments below are minor things.
> 
>> ---
>>   target/arm/cpu.h | 14 ++++++++++++++
>>   target/arm/ptw.c | 41 ++++++++++++++++++++++++-----------------
>>   2 files changed, 38 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index efbef0341da..38cc5823a93 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -1216,6 +1216,20 @@ void arm_v7m_cpu_do_interrupt(CPUState *cpu);
>>
>>   hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
>>                                            MemTxAttrs *attrs);
>> +
>> +typedef struct ARMGranuleProtectionConfig {
>> +    uint64_t gpccr;
>> +    uint64_t gptbr;
>> +    uint8_t parange;
>> +    bool support_sel2;
>> +    AddressSpace *as_secure;
>> +} ARMGranuleProtectionConfig;
>> +
>> +bool arm_granule_protection_check(ARMGranuleProtectionConfig config,
>> +                                  uint64_t paddress,
>> +                                  ARMSecuritySpace pspace,
>> +                                  ARMSecuritySpace ss,
>> +                                  ARMMMUFaultInfo *fi);
> 
> Could we have a doc comment for these prototypes, please?
>

I can add one yes.

>>   #endif /* !CONFIG_USER_ONLY */
>>
>>   int arm_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
>> index 2e6b149b2d1..2b620b03014 100644
>> --- a/target/arm/ptw.c
>> +++ b/target/arm/ptw.c
>> @@ -330,24 +330,23 @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
>>       return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
>>   }
>>
>> -static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>> -                                     ARMSecuritySpace pspace,
>> -                                     ARMSecuritySpace ss,
>> -                                     ARMMMUFaultInfo *fi)
>> +bool arm_granule_protection_check(ARMGranuleProtectionConfig config,
>> +                                  uint64_t paddress,
>> +                                  ARMSecuritySpace pspace,
>> +                                  ARMSecuritySpace ss,
>> +                                  ARMMMUFaultInfo *fi)
>>   {
> 
> A comment here noting that we share this function with the SMMU
> would probably help us in avoiding inadvertently introducing
> CPU-specifics in future.
>

By definition, removing the cpu from prototype is the guarantee we don't 
use any CPU specifics. I can add a comment though.

>>       MemTxAttrs attrs = {
>>           .secure = true,
>>           .space = ARMSS_Root,
>>       };
>> -    ARMCPU *cpu = env_archcpu(env);
>> -    uint64_t gpccr = env->cp15.gpccr_el3;
>> +    const uint64_t gpccr = config.gpccr;
>>       unsigned pps, pgs, l0gptsz, level = 0;
>>       uint64_t tableaddr, pps_mask, align, entry, index;
>> -    AddressSpace *as;
>>       MemTxResult result;
>>       int gpi;
>>
>> -    if (!FIELD_EX64(gpccr, GPCCR, GPC)) {
>> +    if (!FIELD_EX64(config.gpccr, GPCCR, GPC)) {
> 
> We just set up the 'gpccr' local so we don't need to change this line,
> I think ?
>

Right, just an artifact I forgot to clean up before re-introducing local 
gpccr variable to remove other changed lines.

> Also, the SMMU's SMMU_ROOT_GPT_BASE_CFG does not have the GPC field
> (it keeps its enable bit elsewhere).
>

Yes, you can see in patch attached to cover letter this was handled by 
copying this field.
That said, I can keep a separate bool if you think it's better and 
represent better differences between cpu and smmu.

In SMMU spec, the field for GPC is marked as res0. No idea why they 
chose inconsistency here.

>>           return true;
>>       }
>>
>> @@ -362,7 +361,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>>        * physical address size is invalid.
>>        */
>>       pps = FIELD_EX64(gpccr, GPCCR, PPS);
>> -    if (pps > FIELD_EX64_IDREG(&cpu->isar, ID_AA64MMFR0, PARANGE)) {
>> +    if (pps > config.parange) {
>>           goto fault_walk;
>>       }
>>       pps = pamax_map[pps];
>> @@ -432,7 +431,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>>       }
>>
>>       /* GPC Priority 4: the base address of GPTBR_EL3 exceeds PPS. */
>> -    tableaddr = env->cp15.gptbr_el3 << 12;
>> +    tableaddr = config.gptbr << 12;
>>       if (tableaddr & ~pps_mask) {
>>           goto fault_size;
>>       }
>> @@ -446,12 +445,10 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>>       align = MAKE_64BIT_MASK(0, align);
>>       tableaddr &= ~align;
>>
>> -    as = arm_addressspace(env_cpu(env), attrs);
>> -
>>       /* Level 0 lookup. */
>>       index = extract64(paddress, l0gptsz, pps - l0gptsz);
>>       tableaddr += index * 8;
>> -    entry = address_space_ldq_le(as, tableaddr, attrs, &result);
>> +    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);
> 
> as_secure is an odd name for the AS here, because architecturally
> GPT walks are done to the Root physical address space. (This is
> why in the current code we set attrs.space to ARMSS_Root and then
> get the QEMU AddressSpace corresponding to those attrs. It happens
> that at the moment that's the same one we use as Secure, but in
> theory we could have 4 completely separate ones for NS, S, Root
> and Realm.)
>

If I followed original code correctly, the call was equivalent to:
cpu_get_address_space(env_cpu(env), ARMASIdx_S),
because .secure was set in attrs. See details below.

If you prefer completeness, we can introduce four memory regions, and 
four address spaces. But then, it does not match what cpu implementation 
is doing, so I'm not sure if we should change everything. I would favor 
consistency over preference here.

Let me know what would you prefer, and which naming convention we can 
follow, I'm open to any approach.

>>       if (result != MEMTX_OK) {
>>           goto fault_eabt;
>>       }
>> @@ -479,7 +476,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>>       level = 1;
>>       index = extract64(paddress, pgs + 4, l0gptsz - pgs - 4);
>>       tableaddr += index * 8;
>> -    entry = address_space_ldq_le(as, tableaddr, attrs, &result);
>> +    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);
>>       if (result != MEMTX_OK) {
>>           goto fault_eabt;
>>       }
>> @@ -513,7 +510,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
>>       case 0b1111: /* all access */
>>           return true;
>>       case 0b1000: /* secure */
>> -        if (!cpu_isar_feature(aa64_sel2, cpu)) {
>> +        if (!config.support_sel2) {
>>               goto fault_walk;
>>           }
>>           /* fall through */
>> @@ -3786,8 +3783,18 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
>>                               memop, result, fi)) {
>>           return true;
>>       }
>> -    if (!granule_protection_check(env, result->f.phys_addr,
>> -                                  result->f.attrs.space, ptw->in_space, fi)) {
>> +
>> +    ARMCPU *cpu = env_archcpu(env);
>> +    struct ARMGranuleProtectionConfig gpc = {
>> +        .gpccr = env->cp15.gpccr_el3,
>> +        .gptbr = env->cp15.gptbr_el3,
>> +        .parange = FIELD_EX64_IDREG(&cpu->isar, ID_AA64MMFR0, PARANGE),
>> +        .support_sel2 = cpu_isar_feature(aa64_sel2, cpu),
>> +        .as_secure = cpu_get_address_space(env_cpu(env), ARMASIdx_S)
> 
> Directly coding ARMASIDx_S here is a bit awkward, as noted above.
>

As mentioned above, this is what the original code was doing anyway, 
because of:

      MemTxAttrs attrs = {
          .secure = true,
          .space = ARMSS_Root,
      };

...

      as = arm_addressspace(env_cpu(env), attrs);

...

static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
{
     return cpu_get_address_space(cs, arm_asidx_from_attrs(cs, attrs));
}

....

static inline int arm_asidx_from_attrs(CPUState *cs, MemTxAttrs attrs)
{
     return attrs.secure ? ARMASIdx_S : ARMASIdx_NS;
}

Maybe something was wrong with original implementation though.

>> +    };
>> +    if (!arm_granule_protection_check(gpc, result->f.phys_addr,
>> +                                      result->f.attrs.space, ptw->in_space,
>> +                                      fi)) {
>>           fi->type = ARMFault_GPCFOnOutput;
>>           return true;
>>       }
> 
> thanks
> -- PMM

Thank you for your review Peter!

Re: [PATCH 2/2] target/arm/ptw: make granule_protection_check usable without a cpu

[Peter Maydell]

On Fri, 12 Dec 2025 at 18:09, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 12/12/25 2:35 AM, Peter Maydell wrote:
> > On Thu, 11 Dec 2025 at 23:44, Pierrick Bouvier
> > <pierrick.bouvier@linaro.org> wrote:
> >>
> >> By removing cpu details and use a config struct, we can use the
> >> same granule_protection_check with other devices, like SMMU.
> >>
> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> >
> > I'm not 100% sure about this approach, mainly because for SMMU
> > so far we have taken the route of having its page table
> > walk implementation be completely separate from the one in
> > the MMU, even though it's pretty similar: the spec for
> > CPU page table walk and the one for SMMU page table walk
> > are technically in different documents and don't necessarily
> > proceed 100% in sync. Still, the function is a pretty big
> > one and our other option would probably end up being
> > copy-and-paste, which isn't very attractive.

> I'm not sure from your paragraph if you are open to it or not, so it
> would help if you could be more explicit. Maybe giving a review is a way
> to say yes, but my brain firmware does not have the "indirect
> communication style" upgrade yet :).

Yes, sorry, I was too vague here. I was trying to say "this feels
perhaps a little awkward but overall I agree it's better than our other
alternatives so I'm OK with doing it this way" but I didn't put the last
part actually down in text.

> > Also, the SMMU's SMMU_ROOT_GPT_BASE_CFG does not have the GPC field
> > (it keeps its enable bit elsewhere).
> >
>
> Yes, you can see in patch attached to cover letter this was handled by
> copying this field.
> That said, I can keep a separate bool if you think it's better and
> represent better differences between cpu and smmu.

We could alternatively do the "is GPT enabled?" check at the callsites,
which then can do it using whatever the enable bit is for them. That
also gives you a convenient local scope for the config struct:

   if (gpt enabled) {
       struct ARMGranuleProtectionConfig gpc = {
           etc;
      }
      if (!arm_granule_protection_check(..)) {
         etc
     }
   }


> >> +    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);
> >
> > as_secure is an odd name for the AS here, because architecturally
> > GPT walks are done to the Root physical address space. (This is
> > why in the current code we set attrs.space to ARMSS_Root and then
> > get the QEMU AddressSpace corresponding to those attrs. It happens
> > that at the moment that's the same one we use as Secure, but in
> > theory we could have 4 completely separate ones for NS, S, Root
> > and Realm.)
> >
>
> If I followed original code correctly, the call was equivalent to:
> cpu_get_address_space(env_cpu(env), ARMASIdx_S),
> because .secure was set in attrs. See details below.

The behaviour is the same, but the old code is abstracting away
"which of the AddressSpaces we have now do we want for an
ARMSS_Root access?", where the new code is not. I would
prefer it if we can keep the "how does an ARMSS_XYZ which
indicates an architectural physical address space map to a QEMU
AddressSpace pointer" hidden behind arm_addressspace() somehow.

thanks
-- PMM

Re: [PATCH 2/2] target/arm/ptw: make granule_protection_check usable without a cpu

[Pierrick Bouvier]

On 12/12/25 10:54 AM, Peter Maydell wrote:
> On Fri, 12 Dec 2025 at 18:09, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 12/12/25 2:35 AM, Peter Maydell wrote:
>>> On Thu, 11 Dec 2025 at 23:44, Pierrick Bouvier
>>> <pierrick.bouvier@linaro.org> wrote:
>>>>
>>>> By removing cpu details and use a config struct, we can use the
>>>> same granule_protection_check with other devices, like SMMU.
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>
>>> I'm not 100% sure about this approach, mainly because for SMMU
>>> so far we have taken the route of having its page table
>>> walk implementation be completely separate from the one in
>>> the MMU, even though it's pretty similar: the spec for
>>> CPU page table walk and the one for SMMU page table walk
>>> are technically in different documents and don't necessarily
>>> proceed 100% in sync. Still, the function is a pretty big
>>> one and our other option would probably end up being
>>> copy-and-paste, which isn't very attractive.
> 
>> I'm not sure from your paragraph if you are open to it or not, so it
>> would help if you could be more explicit. Maybe giving a review is a way
>> to say yes, but my brain firmware does not have the "indirect
>> communication style" upgrade yet :).
> 
> Yes, sorry, I was too vague here. I was trying to say "this feels
> perhaps a little awkward but overall I agree it's better than our other
> alternatives so I'm OK with doing it this way" but I didn't put the last
> part actually down in text.
>

Sounds good, thanks!

>>> Also, the SMMU's SMMU_ROOT_GPT_BASE_CFG does not have the GPC field
>>> (it keeps its enable bit elsewhere).
>>>
>>
>> Yes, you can see in patch attached to cover letter this was handled by
>> copying this field.
>> That said, I can keep a separate bool if you think it's better and
>> represent better differences between cpu and smmu.
> 
> We could alternatively do the "is GPT enabled?" check at the callsites,
> which then can do it using whatever the enable bit is for them. That
> also gives you a convenient local scope for the config struct:
> 
>     if (gpt enabled) {
>         struct ARMGranuleProtectionConfig gpc = {
>             etc;
>        }
>        if (!arm_granule_protection_check(..)) {
>           etc
>       }
>     }
> 
> 

It's a good compromise. I was thinking about doing something like this, 
but since the GPC is clearly in GranuleProtectionCheck the Arm pseudo 
code, I thought it was better to mimic it.
That said, I'm with your approach and will proceed with it.

>>>> +    entry = address_space_ldq_le(config.as_secure, tableaddr, attrs, &result);
>>>
>>> as_secure is an odd name for the AS here, because architecturally
>>> GPT walks are done to the Root physical address space. (This is
>>> why in the current code we set attrs.space to ARMSS_Root and then
>>> get the QEMU AddressSpace corresponding to those attrs. It happens
>>> that at the moment that's the same one we use as Secure, but in
>>> theory we could have 4 completely separate ones for NS, S, Root
>>> and Realm.)
>>>
>>
>> If I followed original code correctly, the call was equivalent to:
>> cpu_get_address_space(env_cpu(env), ARMASIdx_S),
>> because .secure was set in attrs. See details below.
> 
> The behaviour is the same, but the old code is abstracting away
> "which of the AddressSpaces we have now do we want for an
> ARMSS_Root access?", where the new code is not. I would
> prefer it if we can keep the "how does an ARMSS_XYZ which
> indicates an architectural physical address space map to a QEMU
> AddressSpace pointer" hidden behind arm_addressspace() somehow.
>

How do we turn that into a concrete implementation?

Two ideas I can see:
- pass a callback taking attrs in param, and return the matching 
AddressSpace for it withing granule_protection_check. Clunky IMHO.
- Duplicate this attr variable out of function with .secure=true, and 
call arm_addressspace with it, and pass result to granule_protection_check.

> thanks
> -- PMM