Re: [RFC PATCH 07/12] KVM: TDX: Avoid a double-KVM_BUG_ON() in tdx_sept_zap_private_spte()

[Ira Weiny <ira.weiny@xxxxxxxxx>]

Sean Christopherson wrote:
> Return -EIO immediately from tdx_sept_zap_private_spte() if the number of
> to-be-added pages underflows, so that the following "KVM_BUG_ON(err, kvm)"
> isn't also triggered.  Isolating the check from the "is premap error"
> if-statement will also allow adding a lockdep assertion that premap errors
> are encountered if and only if slots_lock is held.
> 
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/vmx/tdx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index ef4ffcad131f..88079e2d45fb 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1773,8 +1773,10 @@ static int tdx_sept_zap_private_spte(struct kvm *kvm, gfn_t gfn,
>  		err = tdh_mem_range_block(&kvm_tdx->td, gpa, tdx_level, &entry, &level_state);
>  		tdx_no_vcpus_enter_stop(kvm);
>  	}
> -	if (tdx_is_sept_zap_err_due_to_premap(kvm_tdx, err, entry, level) &&
> -	    !KVM_BUG_ON(!atomic64_read(&kvm_tdx->nr_premapped), kvm)) {
> +	if (tdx_is_sept_zap_err_due_to_premap(kvm_tdx, err, entry, level)) {
> +		if (KVM_BUG_ON(!atomic64_read(&kvm_tdx->nr_premapped), kvm))
> +			return -EIO;

Won't this -EIO cause the KVM_BUG_ON on in remove_external_spte() to fire too?

static void remove_external_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
                                 int level)
{
	...
	ret = kvm_x86_call(remove_external_spte)(kvm, gfn, level, old_pfn);
	KVM_BUG_ON(ret, kvm);
}


This patch is better than 3 bug ons but wouldn't it be better to make both
KVM_BUG_ON's internal errors or debug?

Something like this:

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 4920ee8ad773..83065f3fe605 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1774,14 +1774,16 @@ static int tdx_sept_zap_private_spte(struct kvm *kvm, gfn_t gfn,
                tdx_no_vcpus_enter_stop(kvm);
        }
        if (tdx_is_sept_zap_err_due_to_premap(kvm_tdx, err, entry, level)) {
-               if (KVM_BUG_ON(!atomic64_read(&kvm_tdx->nr_premapped), kvm))
+               if (!atomic64_read(&kvm_tdx->nr_premapped)) {
+                       pr_err("nr_premapped underflow\n");
                        return -EIO;
+               }
 
                atomic64_dec(&kvm_tdx->nr_premapped);
                return 0;
        }
 
-       if (KVM_BUG_ON(err, kvm)) {
+       if (err) {
                pr_tdx_error_2(TDH_MEM_RANGE_BLOCK, err, entry, level_state);
                return -EIO;
        }