Re: [PATCH v2 2/3] KVM: x86: force PDPTRs reload on SMM exit

[Sean Christopherson]

On Mon, Aug 23, 2021, Maxim Levitsky wrote:
> KVM_REQ_GET_NESTED_STATE_PAGES is also used with VM entries that happen
> on exit from SMM mode, and in this case PDPTRS must be always reloaded.
> 
> Thanks to Sean Christopherson for pointing this out.
> 
> Fixes: 0f85722341b0 ("KVM: nVMX: delay loading of PDPTRs to KVM_REQ_GET_NESTED_STATE_PAGES")
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> ---
>  arch/x86/kvm/vmx/vmx.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index fada1055f325..4194fbf5e5d6 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7504,6 +7504,13 @@ static int vmx_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
>  	}
>  
>  	if (vmx->nested.smm.guest_mode) {
> +
> +		/* Exit from the SMM to the non root mode also uses

Just "Exit from SMM to non-root mode", i.e. drop the "the".

Multi-line comments should look like:

		/*
		 * Exit from SMM ...

though oddly checkpatch doesn't complain about that.

That said, for the comment, it'd be more helpful to explain why the PDPTRs should
not come from userspace.  Something like:

		/*
		 * Always reload the guest's version of the PDPTRs when exiting
		 * from SMM to non-root.  If KVM_SET_SREGS2 stuffs PDPTRs while
		 * SMM is active, that state is specifically for SMM context.
		 * On RSM, all guest state is pulled from its architectural
		 * location, whatever that may be.
		 */

Though typing that makes me wonder if this is fixing the wrong thing.  It seems
like pdptrs_from_userspace shouldn't be set when SMM is active, though I suppose
there's a potential ordering issue between KVM_SET_SREGS2 and KVM_SET_VCPU_EVENTS.
Bummer.

> +		 * the KVM_REQ_GET_NESTED_STATE_PAGES request,
> +		 * but in this case the pdptrs must be always reloaded
> +		 */
> +		vcpu->arch.pdptrs_from_userspace = false;
> +
>  		ret = nested_vmx_enter_non_root_mode(vcpu, false);
>  		if (ret)
>  			return ret;
> -- 
> 2.26.3
>