Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <c9f65fc1-ab55-4959-a8ec-390aee51ee3a@intel.com>
Date:   Thu, 16 Nov 2023 11:16:39 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/9] KVM: x86: Initialize guest cpu_caps based on guest
 CPUID
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
CC:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Maxim Levitsky <mlevitsk@redhat.com>
References: <20231110235528.1561679-1-seanjc@google.com>
 <20231110235528.1561679-4-seanjc@google.com>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <20231110235528.1561679-4-seanjc@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dHRZTk42MmdRQmZJbzE2NG9RejBKV2oxZG9VV0lZdlBYZjcwMSthOXRqcGFa?=
 =?utf-8?B?TUxiTVhlS05CcXJDaktILzA4Nm1Jd2pXekl5STdQb1pFMXNkQTR0NVhrY2hk?=
 =?utf-8?B?UkpteDBvZmtQdWJwaS93ZnFrSTZlNVROblExLzloN1dXTkJJQktxakdiYkJz?=
 =?utf-8?B?WmVLcUJ3YUt3bnJ6ejdkbnFYRG1DaGczSEVPY1ZKVHl6YlovRzcxR1h3ajZv?=
 =?utf-8?B?QVNDYkVGa3ZTOGVMZG1tVzF4aGF0WjFCQWxmU0FNM1VJOHRhcTdXSXpQUlBn?=
 =?utf-8?B?N1NpNHo4MU5DeFVMUFFUOVE1ZTFnTnczOEhIa2wvTnFJQ3VidkgySVZ5aVVr?=
 =?utf-8?B?dEFWQnF4Tjk5SFhaV1Z3QjRSc3lHTVR4WGMwdDRMREN4a0hWVmxwYzRXalhW?=
 =?utf-8?B?UDJCM3ZPMlRiSnVMdXZzQkE3Qk1yckdjQ0xBM3JqOElsZlVEWWtGZFdGUm02?=
 =?utf-8?B?cTBqZWh0em1GV0NzQnNNNXVVUTE2QXEyTzJoVlUzVGVrTllGYlRzc0hiVDhT?=
 =?utf-8?B?RnZUSVA5QVljcXpqcDUrWkhzRUtyZkVoQUdyQ0xyUTFNaUI2WVd6T3E3aHJt?=
 =?utf-8?B?d2FyV2JJaEtzK3BSdHMzZWNUMFRZa1RLWkFkU3lWLzRRN0lldk45QnNvZWRD?=
 =?utf-8?B?UDVlZk1pc2hydzNHTGhtVmN5VTZyVG8vNEFVSkpKTkxwSGV6L1BXK01MMnpm?=
 =?utf-8?B?YWtINS9JdGRLL0J5U3pkcG45dlRwSU1VdSthUVVtUWJqNHdKajBOSUduUlFy?=
 =?utf-8?B?VVNQN1gwU1VIc3VrdVV3Q2RnYm5IU2dnTmQyZDdlSmprWGdqbVUwSHg5NWQz?=
 =?utf-8?B?TWJJMU8rV3EyTG9iT1pEYzAxZnMvOHVLWHFyUVd5SDdxVHJ1S3N2cjZ1MVc5?=
 =?utf-8?B?V0NSR1YvOFlWZHVmQ2JPN3dJdUozTmxQTFdaZmdBa2l4WEdWUkxRZGxDK1pv?=
 =?utf-8?B?MU5nVyt0cGNKcDJ0bDlxOWNuOTNVNkhKa0JkTUdldDU1Tkp3NkFXUmxTYzFl?=
 =?utf-8?B?QlExbndsNll0Rk9YZFJJV1IvaFNxb0E4L1U1UlZUYzlUZTNKSE9oOEtZLzhq?=
 =?utf-8?B?cmdkWnI5VzhJbGs0dW51SWN5S3VmaW9kOFNrdzVxMHFIWnFJTWFDbmhIbmVy?=
 =?utf-8?B?Qk9sYlJKSHpxTlZFSW0zc2gwTDk0OFlLMFl1VFlZWXNBZHRHL2Z1QUVibUFu?=
 =?utf-8?B?aEFjOHJiNXprakJNeUdNL3dCaDhHL0VudjhDbVI0MTZFcXhHbUlFT2JPY3lS?=
 =?utf-8?B?Z0trbWVJZWpkRHpmK1VTV1Z3VnZ4T3JzY3ZUZXFKN3lpVEZ3ZU1HUTZnK2dq?=
 =?utf-8?B?RU9XWS9Vd1A1SDJGZkwvaVNXK2Fhc3gxZytWT3lYQndrSERmNnNxSitGOWtq?=
 =?utf-8?B?SEVWVW1sbkh3b0NGUHRKYm0xb0p2UGNvdllxVjQwb1NoSlplakhTc1FHV0hW?=
 =?utf-8?B?amJpZ3ZIY2dTSVV1cW5JSUJSMjlLaXVBQk1uMTFLOGdGRUNzV0dMODhBa3Ri?=
 =?utf-8?B?bFBla0RwUVFmNHJoVlpxY1g0VENobkRET1lzRTVUTUtSSFRKRUROSUM2ckpY?=
 =?utf-8?B?REx5WURzM0crdnBob3BvYnhOZUhHR3NDU1E3K3ZSQWtlWFYvMGhWVTJUZkZi?=
 =?utf-8?B?eWNhU2k0QVhBMEJlZUlyMktmVFNsR1RZaGdROXRKVFJzdk1lU004bWNsMG5j?=
 =?utf-8?B?ME9ycy9BWWo1bi93ZFpvbzE3V2lwSUxWQmlEVVo0cHk1VFZ4SWhISnQ4NkJP?=
 =?utf-8?B?bkRRTUFzdzcyaCtNTzd6SGs2dEV6dWk4b3RUTExLUk5LczdyL2cremt4TlRu?=
 =?utf-8?B?ajMzd25zVi9BeGJLenkrbW1LWm5zNkRRbmFxS3hydzNXSnY4Qk8vNVFLa1Fl?=
 =?utf-8?B?cDYyUWEwMEhNNEM5SXJmb0NQbjU5YUVDMld2UFFKN1lWaHREelJjaDRGNU9T?=
 =?utf-8?B?bGcvckU5VSt3TlZYUzRlaDRmdXBKcE5CVVAvT3VGNFFncDZ4S3l4bFRJK1Mz?=
 =?utf-8?B?ZWdYZitqU0Izc3FyY0F1ZnEvaUpXMGs5U0cxRmVkSFBUbWFvSUxuamViTTlJ?=
 =?utf-8?B?cSsyQkY2QTN4YytqeXRJOWhEOWdSZ3hCYjVRaUxvWXpJcU1vNnNkeFZ1eDNt?=
 =?utf-8?B?MXViSmxQWHlkUmtCL1BrRmxCR3VaUjdwdjN4WVU1OGlDTWVKcHgzd2dhZ0Nz?=
 =?utf-8?B?TUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: da585d2f-c222-4fc7-e0cf-08dbe65278fb
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Nov 2023 03:16:49.9426
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jrJHsW1gvsA3jUij2OaEFCGq7waJPnhFAGdrbD7MfKdnf+qc4ONJcI00XcpGdfUvfYvDALA4DE2iQyrPmHdSYA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7854
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 15 Nov 2023 19:17:38 -0800 (PST)

On 11/11/2023 7:55 AM, Sean Christopherson wrote:

[...]

> -static __always_inline void guest_cpu_cap_check_and_set(struct kvm_vcpu *vcpu,
> -							unsigned int x86_feature)
> +static __always_inline void guest_cpu_cap_clear(struct kvm_vcpu *vcpu,
> +						unsigned int x86_feature)
>   {
> -	if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
> +	unsigned int x86_leaf = __feature_leaf(x86_feature);
> +
> +	reverse_cpuid_check(x86_leaf);
> +	vcpu->arch.cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature);
> +}
> +
> +static __always_inline void guest_cpu_cap_change(struct kvm_vcpu *vcpu,
> +						 unsigned int x86_feature,
> +						 bool guest_has_cap)
> +{
> +	if (guest_has_cap)
>   		guest_cpu_cap_set(vcpu, x86_feature);
> +	else
> +		guest_cpu_cap_clear(vcpu, x86_feature);
> +}

I don't see any necessity to add 3 functions, i.e., guest_cpu_cap_{set, clear, change}, for
guest_cpu_cap update. IMHO one function is enough, e.g,:

static __always_inline void guest_cpu_cap_update(struct kvm_vcpu *vcpu,
                                                  unsigned int x86_feature,
                                                  bool guest_has_cap)
{
         unsigned int x86_leaf = __feature_leaf(x86_feature);

reverse_cpuid_check(x86_leaf);
         if (guest_has_cap)
                 vcpu->arch.cpu_caps[x86_leaf] |= __feature_bit(x86_feature);
else
                 vcpu->arch.cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature);
}

> +
> +static __always_inline void guest_cpu_cap_restrict(struct kvm_vcpu *vcpu,
> +						   unsigned int x86_feature)
> +{
> +	if (!kvm_cpu_cap_has(x86_feature))
> +		guest_cpu_cap_clear(vcpu, x86_feature);
>   }

_restrict is not clear to me for what the function actually does -- it conditionally clears
guest cap depending on KVM support of the feature.

How about renaming it to guest_cpu_cap_sync()?

>   
>   static __always_inline bool guest_cpu_cap_has(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8a99a73b6ee5..5827328e30f1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4315,14 +4315,14 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   	 * XSS on VM-Enter/VM-Exit.  Failure to do so would effectively give
>   	 * the guest read/write access to the host's XSS.
>   	 */
> -	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> -	    boot_cpu_has(X86_FEATURE_XSAVES) &&
> -	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> -		guest_cpu_cap_set(vcpu, X86_FEATURE_XSAVES);
> +	guest_cpu_cap_change(vcpu, X86_FEATURE_XSAVES,
> +			     boot_cpu_has(X86_FEATURE_XSAVE) &&
> +			     boot_cpu_has(X86_FEATURE_XSAVES) &&
> +			     guest_cpuid_has(vcpu, X86_FEATURE_XSAVE));
>   
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_NRIPS);
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_LBRV);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_NRIPS);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_TSCRATEMSR);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_LBRV);
>   
>   	/*
>   	 * Intercept VMLOAD if the vCPU mode is Intel in order to emulate that
> @@ -4330,12 +4330,12 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   	 * SVM on Intel is bonkers and extremely unlikely to work).
>   	 */
>   	if (!guest_cpuid_is_intel(vcpu))
> -		guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
> +		guest_cpu_cap_restrict(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
>   
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VGIF);
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VNMI);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_PAUSEFILTER);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_PFTHRESHOLD);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_VGIF);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_VNMI);
>   
>   	svm_recalc_instruction_intercepts(vcpu, svm);
>   
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6328f0d47c64..5a056ad1ae55 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7757,9 +7757,11 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   	 */
>   	if (boot_cpu_has(X86_FEATURE_XSAVE) &&
>   	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> -		guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_XSAVES);
> +		guest_cpu_cap_restrict(vcpu, X86_FEATURE_XSAVES);
> +	else
> +		guest_cpu_cap_clear(vcpu, X86_FEATURE_XSAVES);
>   
> -	guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VMX);
> +	guest_cpu_cap_restrict(vcpu, X86_FEATURE_VMX);
>   
>   	vmx_setup_uret_msrs(vmx);
>