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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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!