linux-kernel - Re: [PATCH 3/7] KVM: TDX: Retry locally in TDX EPT violation handler on RET_PF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z4rIGv4E7Jdmhl8P@google.com>
Date: Fri, 17 Jan 2025 13:14:02 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Yan Zhao <yan.y.zhao@...el.com>
Cc: pbonzini@...hat.com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	rick.p.edgecombe@...el.com, kai.huang@...el.com, adrian.hunter@...el.com, 
	reinette.chatre@...el.com, xiaoyao.li@...el.com, tony.lindgren@...el.com, 
	binbin.wu@...ux.intel.com, dmatlack@...gle.com, isaku.yamahata@...el.com, 
	isaku.yamahata@...il.com
Subject: Re: [PATCH 3/7] KVM: TDX: Retry locally in TDX EPT violation handler
 on RET_PF_RETRY

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
>