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
>
