On Thu, 11 Dec 2025 at 23:44, Pierrick Bouvier
<[email protected]> 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 <[email protected]>
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
