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

[Steven Price <steven.price@arm.com>]

On 13/11/2026 16:26, Suzuki K Poulose wrote:
> 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 ?

Ah, yes move the code between patches - I thought you were suggesting
somehow moving the code between functions (hence I was very confused).

Yes, I can move the kvm_destroy_realm() code changes into this patch.

Thanks,
Steve

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