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 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