On 12/12/25 10:54 AM, Peter Maydell wrote:
On Fri, 12 Dec 2025 at 18:09, Pierrick Bouvier
<[email protected]> wrote:
On 12/12/25 2:35 AM, Peter Maydell wrote:
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.
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
