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