Re: [PATCH v6 15/18] KVM: x86/tdp_mmu: Propagate tearing down mirror page tables

[Sean Christopherson <seanjc@google.com>]

On Sun, Dec 22, 2024, Paolo Bonzini wrote:
> +	/* Because write lock is held, operation should success. */

succeed.

> +	ret = static_call(kvm_x86_remove_external_spte)(kvm, gfn, level, old_pfn);
> +	KVM_BUG_ON(ret, kvm);
> +}
> +
>  /**
>   * handle_removed_pt() - handle a page table removed from the TDP structure
>   *
> @@ -435,6 +458,23 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
>  		}
>  		handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn,
>  				    old_spte, FROZEN_SPTE, level, shared);
> +
> +		if (is_mirror_sp(sp)) {
> +			KVM_BUG_ON(shared, kvm);

Should these bail early if the KVM_BUG_ON() is hit?  Calling into the TDX module
after bugging the VM is a bit odd.

> +			remove_external_spte(kvm, gfn, old_spte, level);
> +		}
> +	}
> +
> +	if (is_mirror_sp(sp) &&
> +	    WARN_ON(static_call(kvm_x86_free_external_spt)(kvm, base_gfn, sp->role.level,

WARN_ON_ONCE(). I suspect that if this ever gets hit, it'll come in bunches.

> +							  sp->external_spt))) {
> +		/*
> +		 * Failed to free page table page in mirror page table and
> +		 * there is nothing to do further.
> +		 * Intentionally leak the page to prevent the kernel from
> +		 * accessing the encrypted page.
> +		 */
> +		sp->external_spt = NULL;
>  	}
>  
>  	call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> @@ -608,6 +648,13 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
>  	if (is_mirror_sptep(iter->sptep) && !is_frozen_spte(new_spte)) {
>  		int ret;
>  
> +		/*
> +		 * Users of atomic zapping don't operate on mirror roots,
> +		 * so don't handle it and bug the VM if it's seen.
> +		 */
> +		if (KVM_BUG_ON(!is_shadow_present_pte(new_spte), kvm))
> +			return -EBUSY;
> +
>  		ret = set_external_spte_present(kvm, iter->sptep, iter->gfn,
>  						iter->old_spte, new_spte, iter->level);
>  		if (ret)
> @@ -700,8 +747,10 @@ static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
>  	 * Users that do non-atomic setting of PTEs don't operate on mirror
>  	 * roots, so don't handle it and bug the VM if it's seen.
>  	 */
> -	if (is_mirror_sptep(sptep))
> +	if (is_mirror_sptep(sptep)) {
>  		KVM_BUG_ON(is_shadow_present_pte(new_spte), kvm);
> +		remove_external_spte(kvm, gfn, old_spte, level);
> +	}
>  
>  	return old_spte;
>  }
> -- 
> 2.43.5
> 
>