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

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

On 03/11/2026 15:20, Suzuki K Poulose wrote:
> On 02/11/2026 14:12, 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).
>>
>> 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>
>> ---
>>   arch/arm64/include/asm/kvm_rmi.h |   9 ++
>>   arch/arm64/kvm/mmu.c             |  17 ++-
>>   arch/arm64/kvm/rmi.c             | 189 +++++++++++++++++++++++++++++++
>>   3 files changed, 214 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_rmi.h b/arch/arm64/include/
>> asm/kvm_rmi.h
>> index f1a1ed6b91f5..58d825e40bdf 100644
>> --- a/arch/arm64/include/asm/kvm_rmi.h
>> +++ b/arch/arm64/include/asm/kvm_rmi.h
>> @@ -57,6 +57,7 @@ enum realm_state {
>>    * @sro: Preallocated SRO state context for Realm MMU operations
>>    * @state: The lifetime state machine for the realm
>>    * @ia_bits: Number of valid Input Address bits in the IPA
>> + * @stage2_unmapped: The Realm stage-2 mappings have been removed
>>    */
>>   struct realm {
>>       void *rd;
>> @@ -70,6 +71,7 @@ struct realm {
>>       struct rmi_sro_state *sro;
>>       enum realm_state state;
>>       unsigned int ia_bits;
>> +    bool stage2_unmapped;
>>   };
>>     void kvm_init_rmi(void);
>> @@ -77,5 +79,12 @@ u32 kvm_rmm_ipa_limit(void);
>>     int kvm_init_realm(struct kvm *kvm);
>>   void kvm_destroy_realm(struct kvm *kvm);
>> +int kvm_realm_teardown_stage2(struct kvm *kvm);
>> +
>> +static inline bool kvm_realm_is_private_address(struct realm *realm,
>> +                        unsigned long addr)
>> +{
>> +    return !(addr & BIT(realm->ia_bits - 1));
>> +}
>>     #endif /* __ASM_KVM_RMI_H */
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index b049ea67d9af..51fda7b2ff7f 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -1145,9 +1145,24 @@ int kvm_init_stage2_mmu(struct kvm *kvm, struct
>> kvm_s2_mmu *mmu, unsigned long t
>>       return err;
>>   }
>>   +static void kvm_realm_uninit_stage2(struct kvm_s2_mmu *mmu)
>> +{
>> +    struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
>> +
>> +    mutex_lock(&kvm->arch.config_lock);
>> +    if (kvm_realm_state(kvm) == REALM_STATE_ACTIVE) {
>> +        kvm_set_realm_state(kvm, REALM_STATE_DYING);
>> +        WARN_ON(kvm_realm_teardown_stage2(kvm));
>> +    }
>> +    mutex_unlock(&kvm->arch.config_lock);
>> +}
>> +
>>   void kvm_uninit_stage2_mmu(struct kvm *kvm)
>>   {
>> -    kvm_free_stage2_pgd(&kvm->arch.mmu);
>> +    if (kvm_vm_is_realm(kvm))
>> +        kvm_realm_uninit_stage2(&kvm->arch.mmu);
>> +    else
>> +        kvm_free_stage2_pgd(&kvm->arch.mmu);
>>       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 45ecbaa467d1..f190143b33ba 100644
>> --- a/arch/arm64/kvm/rmi.c
>> +++ b/arch/arm64/kvm/rmi.c
>> @@ -11,6 +11,14 @@
>>   #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));
>> +}
> 
> 
> Now that we are using the same PAGE_SIZE as GRANULE size, we could
> replace the above helper with kvm_granule_size() ?

Makes sense - I added all these helpers when the RMM was always 4k page
size. Obviously I haven't yet managed to remove them all yet! ;)

>>   static bool rmi_has_feature(int reg, unsigned long feature)
>>   {
>>       return !!u64_get_bits(rmi_feat_reg(reg), feature);
>> @@ -21,11 +29,180 @@ 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 - stage2_pgtable_levels(realm->ia_bits);
>> +}
>> +
>> +static void free_rtt(phys_addr_t phys)
>> +{
>> +    if (rmi_free_delegated_page(phys))
>> +        return;
>> +
>> +    kvm_account_pgtable_pages(phys_to_virt(phys), -1);
>> +}
>> +
> 
> ...
> 
>> +
>> +static int realm_tear_down_rtt_level(struct realm *realm, int level,
>> +                     unsigned long start, unsigned long end)
>> +{
>> +    ssize_t map_size;
>> +    unsigned long addr, next_addr;
>> +
>> +    if (WARN_ON(level > KVM_PGTABLE_LAST_LEVEL))
>> +        return -EINVAL;
>> +
>> +    map_size = rmi_rtt_level_mapsize(level - 1);
>> +
>> +    for (addr = start; addr < end; addr = next_addr) {
>> +        phys_addr_t rtt_granule;
>> +        long ret;
>> +        unsigned long align_addr = ALIGN(addr, map_size);
> 
> We assume that the start is aligned to the kvm_granule_size(level - 1)
> and thus this works ? Should we enforce this before the loop ?

I'll add a WARN_ON() path to check this.

>> +
>> +        next_addr = ALIGN(addr + 1, map_size);
> 
> So that we can be sure we have (align_addr != next_addr).
> 
>> +
>> +        if (next_addr > end || align_addr != addr) {
>> +            /*
>> +             * The target range is smaller than what this level
>> +             * covers, recurse deeper.
>> +             */
>> +            ret = realm_tear_down_rtt_level(realm,
>> +                            level + 1,
>> +                            addr,
>> +                            min(next_addr, end));
>> +            if (ret)
>> +                return ret;
>> +            continue;
>> +        }
>> +
>> +        ret = realm_rtt_destroy(realm, addr, level,
>> +                    &rtt_granule, &next_addr);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        switch (RMI_RESULT_STATUS(ret)) {
>> +        case RMI_SUCCESS:
>> +            free_rtt(rtt_granule);
>> +            break;
>> +        case RMI_ERROR_RTT:
>> +            if (next_addr > addr) {
>> +                /* Missing RTT, skip */
>> +                break;
>> +            }
>> +            /*
>> +             * We tear down the RTT range for the full IPA
>> +             * space, after everything is unmapped. Also we
>> +             * descend down only if we cannot tear down a
>> +             * top level RTT. Thus RMM must be able to walk
>> +             * to the requested level. e.g., a block mapping
>> +             * exists at L1 or L2.
>> +             */
>> +            if (WARN_ON(RMI_RESULT_DATA_LEVEL(ret) != level))
>> +                return -EBUSY;
>> +            if (WARN_ON(level == KVM_PGTABLE_LAST_LEVEL))
>> +                return -EBUSY;
>> +
>> +            /*
>> +             * The table has active entries in it, recurse deeper
>> +             * and tear down the RTTs.
>> +             */
>> +            next_addr = ALIGN(addr + 1, map_size);
>> +            ret = realm_tear_down_rtt_level(realm,
>> +                            level + 1,
>> +                            addr,
>> +                            next_addr);
>> +            if (ret)
>> +                return ret;
>> +            /*
>> +             * Now that the child RTTs are destroyed,
>> +             * retry at this level.
>> +             */
>> +            next_addr = addr;
>> +            break;
>> +        default:
>> +            WARN_ON(1);
>> +            return -ENXIO;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int realm_tear_down_rtt_range(struct realm *realm,
>> +                     unsigned long start, unsigned long end)
>> +{
>> +    /*
>> +     * Root level RTTs can only be destroyed after the RD is
>> destroyed. So
>> +     * tear down everything below the root level
>> +     */
>> +    return realm_tear_down_rtt_level(realm, get_start_level(realm) + 1,
>> +                     start, end);
>> +}
>> +
>> +static int realm_destroy_rtts(struct kvm *kvm)
>> +{
>> +    struct realm *realm = &kvm->arch.realm;
>> +    unsigned int ia_bits = realm->ia_bits;
>> +
>> +    lockdep_assert_held(&kvm->arch.config_lock);
> 
> Should we assert that realm->stage2_unmapped == true here, since we
> expect to not have any valid mappings ?

Will add.

>> +
>> +    return realm_tear_down_rtt_range(realm, 0, (1UL << ia_bits));
>> +}
>> +
>> +static void realm_unmap_stage2(struct kvm *kvm)
>> +{
>> +    struct realm *realm = &kvm->arch.realm;
>> +
>> +    lockdep_assert_held(&kvm->arch.config_lock);
>> +
>> +    if (realm->stage2_unmapped)
>> +        return;
>> +
>> +    write_lock(&kvm->mmu_lock);
>> +    kvm_stage2_unmap_range(&kvm->arch.mmu, 0,
>> +                   BIT(realm->ia_bits - 1), true);
> 
> 
>> +    write_unlock(&kvm->mmu_lock);
> 
> Do we need this write_lock() ? IIUC, the Realm is dying and this is
> called from kvm_destroy_realm()/ kvm_free_stage2_pgd(). KVM assumes that
> stage2_destroy_range() is called without kvm->mmu_lock held (along with
> other relaxations).  The comment above stage2_destroy_range() says:

I think you might be right that we don't *need* it. The main requirement
from the current code is the cond_resched_rwlock_write() call in
realm_unmap_xxxx_range(). So the options I see are:

 1. Hold the lock even though we don't need to.

 2. Force may_block to false even though it could be true.

 3. Add an extra parameter to allow a call to cond_resched() rather than
cond_resched_rwlock_write().

My preference is to keep the code simple and hold the lock - do you see
any specific problem with taking the lock here?

Thanks,
Steve

> 
> /*
>  * Assume that @pgt is valid and unlinked from the KVM MMU to free the
>  * page-table without taking the kvm_mmu_lock and without performing any
>  * TLB invalidations.
>  *
>  * Also, the range of addresses can be large enough to cause need_resched
>  * warnings, for instance on CONFIG_PREEMPT_NONE kernels. Hence, invoke
>  * cond_resched() periodically to prevent hogging the CPU for a long time
>  * and schedule something else, if required.
>  */
> 
> So, I believe we should be safe here without the lock ?
> 
> Also, could we not call the realm unmap routine directly here, rather
> than going through the generic helper to get back to what we want ?
> 
> 
> Suzuki
> 
> 
>> +
>> +    realm->stage2_unmapped = true;
>> +}
>> +
>> +int kvm_realm_teardown_stage2(struct kvm *kvm)
>> +{
>> +    lockdep_assert_held(&kvm->arch.config_lock);
>> +
>> +    realm_unmap_stage2(kvm);
>> +    return 0;
>> +}
>> +
>>   void kvm_destroy_realm(struct kvm *kvm)
>>   {
>>       struct realm *realm = &kvm->arch.realm;
>>       size_t pgd_size = kvm_pgtable_stage2_pgd_size(kvm->arch.mmu.vtcr);
>>   +    guard(mutex)(&kvm->arch.config_lock);
>> +
>>       if (!kvm_realm_is_created(kvm)) {
>>           kfree(realm->sro);
>>           realm->sro = NULL;
>> @@ -34,12 +211,24 @@ void kvm_destroy_realm(struct kvm *kvm)
>>         kvm_set_realm_state(kvm, REALM_STATE_DYING);
>>   +    /*
>> +     * REALM_DESTROY requires the realm to be non-live: all RECs must
>> have
>> +     * been destroyed and the root RTTs must be empty. Unmap the IPA
>> space
>> +     * and destroy any non-root RTTs before tearing down the RD. The
>> root
>> +     * RTT pages are still owned by the RMM at this point, so keep
>> the KVM
>> +     * pgtable alive until after REALM_DESTROY and undelegation.
>> +     */
>> +    realm_unmap_stage2(kvm);
>> +
>>       if (realm->rd) {
>>           phys_addr_t rd_phys = virt_to_phys(realm->rd);
>>             if (WARN_ON(rmi_realm_terminate(rd_phys, realm->sro)))
>>               return;
>>   +        if (WARN_ON(realm_destroy_rtts(kvm)))
>> +            return;
>> +
>>           if (WARN_ON(rmi_realm_destroy(rd_phys, realm->sro)))
>>               return;
>>           rmi_free_delegated_page(rd_phys);
>