Received-SPF: pass (google.com: domain of linux-kernel+bounces-213793-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 13 Jun 2024 11:11:41 -0700
In-Reply-To: <20240207172646.3981-18-xin3.li@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240207172646.3981-1-xin3.li@intel.com> <20240207172646.3981-18-xin3.li@intel.com>
Message-ID: <Zms2XcFS5Ve0XLUa@google.com>
Subject: Re: [PATCH v2 17/25] KVM: nVMX: Add support for the secondary VM exit controls
From: Sean Christopherson <seanjc@google.com>
To: Xin Li <xin3.li@intel.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, 
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org, 
	pbonzini@redhat.com, corbet@lwn.net, tglx@linutronix.de, mingo@redhat.com, 
	bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, 
	shuah@kernel.org, vkuznets@redhat.com, peterz@infradead.org, 
	ravi.v.shankar@intel.com, xin@zytor.com
Content-Type: text/plain; charset="us-ascii"

On Wed, Feb 07, 2024, Xin Li wrote:
> @@ -6856,13 +6865,17 @@ static void nested_vmx_setup_exit_ctls(struct vmcs_config *vmcs_conf,
>  		VM_EXIT_HOST_ADDR_SPACE_SIZE |
>  #endif
>  		VM_EXIT_LOAD_IA32_PAT | VM_EXIT_SAVE_IA32_PAT |
> -		VM_EXIT_CLEAR_BNDCFGS;
> +		VM_EXIT_CLEAR_BNDCFGS | VM_EXIT_ACTIVATE_SECONDARY_CONTROLS;
>  	msrs->exit_ctls_high |=
>  		VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
>  		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
>  		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT |
>  		VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
>  
> +	/* secondary exit controls */

Drop the comment, it's pretty obvious what field is being setup.

> +	if (msrs->exit_ctls_high & VM_EXIT_ACTIVATE_SECONDARY_CONTROLS)
> +		rdmsrl(MSR_IA32_VMX_EXIT_CTLS2, msrs->secondary_exit_ctls);

This is wrong, the resulting msrs->secondary_exit_ctls needs to be sanitized
based on what KVM supports for nVMX.

On a very related topic, this should not do a raw RDMSR.  One of the reasons why
KVM uses vmcs_config as the base is to avoid advertising features to L1 that KVM
itself doesn't support, e.g. because the expected entry+exit pairs aren't
supported.

And by pulling state from vmcs_conf->secondary_exit_ctls there's no need to check
the activation bit.

I.e. literaly just this:

	msrs->secondary_exit_ctls = vmcs_conf->secondary_exit_ctls;
	msrs->secondary_exit_ctls &= 0;

and then when nVMX FRED support is ready, it becomes:

	msrs->secondary_exit_ctls = vmcs_conf->secondary_exit_ctls;
	msrs->secondary_exit_ctls &= SECONDARY_VM_EXIT_SAVE_IA32_FRED |
				     SECONDARY_VM_EXIT_LOAD_IA32_FRED;