Re: [PATCH 3/7] KVM: TDX: Retry locally in TDX EPT violation handler on RET_PF_RETRY

[Sean Christopherson <seanjc@google.com>]

On Mon, Jan 13, 2025, Yan Zhao wrote:
> @@ -1884,7 +1904,24 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
>  	}
>  
>  	trace_kvm_page_fault(vcpu, tdexit_gpa(vcpu), exit_qual);
> -	return __vmx_handle_ept_violation(vcpu, tdexit_gpa(vcpu), exit_qual);
> +
> +	while (1) {
> +		ret = __vmx_handle_ept_violation(vcpu, gpa, exit_qual);
> +
> +		if (ret != RET_PF_RETRY || !local_retry)
> +			break;
> +
> +		/*
> +		 * Break and keep the orig return value.

Wrap at 80.

> +		 * Signal & irq handling will be done later in vcpu_run()

Please don't use "&" as shorthand.  It saves all of two characters.  That said,
I don't see any point in adding this comment, if the reader can't follow the
logic of this code, these comments aren't going to help them.  And the comment
about vcpu_run() in particular is misleading, as posted interrupts aren't truly
handled by vcpu_run(), rather they're handled by hardware (although KVM does send
a self-IPI).

> +		 */
> +		if (signal_pending(current) || pi_has_pending_interrupt(vcpu) ||
> +		    kvm_test_request(KVM_REQ_NMI, vcpu) || vcpu->arch.nmi_pending)

This needs to check that the IRQ/NMI is actually allowed.  I guess it doesn't
matter for IRQs, but it does matter for NMIs.  Why not use kvm_vcpu_has_events()?
Ah, it's a local function.  At a glance, I don't see any harm in exposing that
to TDX.

> +			break;
> +
> +		cond_resched();
> +	}

Nit, IMO this reads better as:

	do {
		ret = __vmx_handle_ept_violation(vcpu, gpa, exit_qual);
	} while (ret == RET_PF_RETY && local_retry &&
		 !kvm_vcpu_has_events(vcpu) && !signal_pending(current));

> +	return ret;
>  }
>  
>  int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> -- 
> 2.43.2
>