Re: [PATCH v2 07/12] KVM: arm64: CCA: Tear down RTTs

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 13/11/2026 15:05, Steven Price wrote:
> On 13/11/2026 01:04, Suzuki K Poulose wrote:
>> On 11/11/2026 14:57, Steven Price wrote:
>>> The RMM owns the stage 2 page tables for a realm, and KVM must request
>>> that the RMM creates/destroys entries as necessary. The physical pages
>>> to store the page tables are delegated to the realm as required, and can
>>> be undelegated when no longer used.
>>>
>>> Creating new RTTs is the easy part, tearing down is a little more
>>> tricky. The result of realm_rtt_destroy() can be used to effectively
>>> walk the tree and destroy the entries (undelegating pages that were
>>> given to the realm).
>>>
>>> Tear down a created Realm whether it is still new or has been activated.
>>> Unmap the Realm IPA space once during stage-2 teardown. Defer
>>> destruction of non-root RTTs until final Realm destruction, after
>>> REALM_TERMINATE and before destroying the RD; root RTTs remain until the
>>> RD has gone away.
>>>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>

...

>>>      #endif /* __ASM_KVM_RMI_H */
>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>> index 9df3f8d5b4ef..be014f6c00e7 100644
>>> --- a/arch/arm64/kvm/mmu.c
>>> +++ b/arch/arm64/kvm/mmu.c
>>> @@ -1145,9 +1145,25 @@ int kvm_init_stage2_mmu(struct kvm *kvm, struct
>>> kvm_s2_mmu *mmu, unsigned long t
>>>        return err;
>>>    }

...

>>>    void kvm_uninit_stage2_mmu(struct kvm *kvm)
>>>    {
>>> -    kvm_free_stage2_pgd(&kvm->arch.mmu);
>>> +    if (kvm_realm_is_created(kvm))
>>> +        kvm_realm_uninit_stage2(&kvm->arch.mmu);
>>> +    else
>>> +        kvm_free_stage2_pgd(&kvm->arch.mmu);
>>
>> We added kvm_free_stage2_pgd() from kvm_destroy_realm() for Realms in
>> the previous patch. Should we move that here since, this is where we
>>
>> 1. Deal with tearing down stage2
>> 2. And more importantly we are skipping it for Realms.
> 
> I'm not quite sure what you're asking for here. We can't free the stage2
> PGD while the realm is existing (because the RMM has the pages) - so we
> defer it.

I understand. My point is, we are deferring the stage2 free for Realm
in this patch, without explicitly showing where it is done. We have
introduced that as part of kvm_destroy_realm() in Patch 6.

My recommendation is to add kvm_free_stage2_pgd() to kvm_destroy_realm()
in this patch, where we are showing when this is deferred to. e.g.,
this patch functionally leaves the reader with no clue when this is
actually freed.

Does that make sense ?


> 
>>>        kvm_mmu_free_memory_cache(&kvm->arch.mmu.split_page_cache);
>>>    }
>>>    diff --git a/arch/arm64/kvm/rmi.c b/arch/arm64/kvm/rmi.c
>>> index 69c628cfb6c7..b904e6541fb0 100644
>>> --- a/arch/arm64/kvm/rmi.c
>>> +++ b/arch/arm64/kvm/rmi.c
>>> @@ -11,16 +11,193 @@
>>>    #include <asm/kvm_pgtable.h>
>>>    #include <asm/virt.h>
>>>    +static inline unsigned long rmi_rtt_level_mapsize(int level)
>>> +{
>>> +    if (WARN_ON(level > KVM_PGTABLE_LAST_LEVEL))
>>> +        return PAGE_SIZE;
>>> +
>>> +    return (1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(level));
>>> +}
>>> +
>>
>> We agreed to use kvm_granule_size() instead of this ?
>>
>> https://lore.kernel.org/all/cb183290-3fee-4535-aa3e-5cc172c2323a@arm.com
> 
> That was a screw-up when I was squashing fixes back in - the next patch
> removes the code. I'll fix this up in the next version so it never
> appears. Thanks for pointing it out.
> 
>>>    u32 kvm_rmm_ipa_limit(void)
>>>    {
>>>        return u64_get_bits(rmi_feat_reg(0), RMI_FEATURE_REGISTER_0_S2SZ);
>>>    }
>>>    +static int get_start_level(struct realm *realm)
>>> +{
>>> +    return 4 - max(2, stage2_pgtable_levels(realm->ia_bits));
>>
>> I don't think we need this "max()". ia_bits is guaranteed to be
>>> = 32bit (ARM64_MIN_PARANGE_BITS, see kvm_init_ipa_range()), implying
>> that we have at least 2 levels on all page sizes.
> 
> Ah, very true - Sashiko complained, but I hadn't noticed
> ARM64_MIN_PARANGE_BITS provided an effective minimum. I'll remove the
> max() and add a comment.
> 
>>> +}
>>> +
>>> +static void free_rtt(phys_addr_t phys)
>>> +{
>>> +    kvm_account_pgtable_pages(phys_to_virt(phys), -1);
>>> +    if (rmi_free_delegated_page(phys))
>>> +        kvm_account_pgtable_pages(phys_to_virt(phys), 1);
>>
>> Is there a reason why remove the account before freeing ? Could we do
>> this after we have reclaimed it ?
> 
> Sashiko again:
> 
>> Does calling kvm_account_pgtable_pages() after rmi_free_delegated_page()
>> access the page after it has already been freed and uncharged from its
>> memcg?
> 
> Basically the problem is that rmi_free_delegated_page() frees the page
> back to the allocator. So doing the account after freeing means that
> technically the page could have been reused.

Thanks, for the explanation, that makes sense.

Cheers
Suzuki