Received-SPF: pass (google.com: domain of linux-kernel+bounces-169409-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6eb40c65-d9f8-4615-811d-6c9139a0e626@intel.com>
Date: Mon, 6 May 2024 15:26:29 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v10 10/27] KVM: x86: Refine xsave-managed guest
 register/MSR reset handling
To: Sean Christopherson <seanjc@google.com>
CC: <pbonzini@redhat.com>, <dave.hansen@intel.com>, <x86@kernel.org>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<peterz@infradead.org>, <chao.gao@intel.com>, <rick.p.edgecombe@intel.com>,
	<mlevitsk@redhat.com>, <john.allen@amd.com>
References: <20240219074733.122080-1-weijiang.yang@intel.com>
 <20240219074733.122080-11-weijiang.yang@intel.com>
 <ZjKouS2ZyH7cXOqM@google.com>
Content-Language: en-US
From: "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZjKouS2ZyH7cXOqM@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SFBaZ1Y3UW5hVjk2dDFiSGVZeS84aGx5SUZtRHNQMVhNTHpQK3o2OVZIaXQ1?=
 =?utf-8?B?MmpYcFJialpzUXJnZENjZmZ1QTBHbWxwV2pVMzFEUGpVT2dhVGovZWgzbkVh?=
 =?utf-8?B?SVBXcUN1dVlOOXZVNHRMRXlWSlR3OXMrUTZYVksvK08wSlBQT2ptQTFoYkN4?=
 =?utf-8?B?MCtlM3ZaZHVZajZBZkJpNFNzc2lwRGVreFhoM0tNK0Z2Rnd0VmlkK2VLeThx?=
 =?utf-8?B?cnhpelZ3UzdIdFFTMlV3d25icHFyaUd5YVZxUXhQVXU5bGVyRUtJWFpyZTVr?=
 =?utf-8?B?TUsveC90S2lDRng5SGZiTlJsVjYxSkpPdDc0a2llUUh4Tjh1MnFndGJJamhS?=
 =?utf-8?B?cVBlbTNXb05PNWdCR2JWYVhrQUdZVHRGYTFGNyttenNyRmU3cHhlY3c3WUZa?=
 =?utf-8?B?aHZSR1Myekp1cGRJMFJEcWlRTTlVemxyQ0E4K2FRWVFvVXRheFpBcCtZck9v?=
 =?utf-8?B?UGU1MTVNNGxkaUordmt4c1N4cFJmWUJoM2l6V1o5SU5LaTIzWGdKQlNHTnFz?=
 =?utf-8?B?c3V1LzdYdno1eHBaTlc0Y3pWTWVHTlhIbnV2cEhZN3FkVjQ5bWNLQU9EMjg1?=
 =?utf-8?B?aUltYzBudXIwL3JKVGVqYmdlQVYyRjM4Y2dxSXdVOXVEaXlFVzQ1UlIwOWlY?=
 =?utf-8?B?R2dzUGNkK1F4L0V3b0xkdTdqUHZGWkwyZlMrR21yMHdvSWUrOExTZXNXRnFz?=
 =?utf-8?B?R1E0MnVZRnZqWEZwUDU5L2p1RERTZFJmb05pYW1WK0VIU0kzRW5mbXZzaEJs?=
 =?utf-8?B?MGo2eC9walhYRksvajJ2RXlqak95Yzh3RXJrSGpkTEVWNG5SaWpEakY5UXBu?=
 =?utf-8?B?dk00YUpOSmdGbisyRHE2Y1RMSHc3cXNyZGlQWGJvdmwzYnVrSmdiWGFaS2ky?=
 =?utf-8?B?T21rUHp2a2dVZU9FenJwU1YxS1BSOUc2TGtRR3Y4NUNmZDl3VjJSbjZMRlZz?=
 =?utf-8?B?T3NYYjBYTHNTRFNNOVFtVHRraTdSTi9Eam4yY2kza0hSYXRHUnBDZEZOY1R0?=
 =?utf-8?B?NEhPZWk2bktGYXlpOXdmRHV6YXBPaXRPcDd5andxNG45TDlXVURnbTJXajE3?=
 =?utf-8?B?ZmdnYW81NlBsQ0RyUUhYWDkwYmdSbzVlZmxmUHlJRGJOS2NBTXZaRFdhMy9X?=
 =?utf-8?B?dy9EM1lEc0ZIWVVoZ2Q1b0pLa0N6YnkyemN4SmV5Z2JGNUxvR0h4V3ZaOWVx?=
 =?utf-8?B?YXVMZ2FncGpaY3A1R212SC9ZVkxFc1MxWlV2M3dqQzNXREs5dHB4OW8yMzha?=
 =?utf-8?B?anU0V0FPOU53a09hcjZnTXVxMXBKNGhhWWg2WGwyVldvTFdieC9ybFJXdlZD?=
 =?utf-8?B?a2VqUytjYVRlRjNua3dLWHd0VzBza1pwTi9zNWJQY0JSQkE4SXp0ZXI3dlZU?=
 =?utf-8?B?M0IxZmZ6UTlzUTJUd3RFUnBSdi9ld25ESmJ1emZsMmZQRUFTZ0Y1dG1NS3NC?=
 =?utf-8?B?UGF4Z0tTN1lkdWpxWDVBcENoekF5bFN2ak9ycTh3QTZqajNWa3YzNEp4aURz?=
 =?utf-8?B?YWxaNGhLQ3VXR2drbGRHRVVFMnBoN21UelRKbkYybXdLSE5xcnp0VkJzZDRB?=
 =?utf-8?B?REZUNlB2U0pmS3VSQzM4Umd2SGU5RzhrYm9sZWJZRVBTb2U4OXJnVEZ1MWla?=
 =?utf-8?B?cGx6SlVZd1B0R0dlNk84Z1ZaQXMyU1dHeE1pWFJGYlRSL3ZTSWoraXFKNnFn?=
 =?utf-8?B?VjJiK0RoaFVTbTN3UzE0d1YwY3g0QVRnSnhHdU1Ldkw1WkxNeHJNZkQ4T296?=
 =?utf-8?B?YlVlZHMrMU5mTzZrc3NnTFVyYWNIazJJQk51V3dmM1pHLzAyZ3ZxS0t4ZFNt?=
 =?utf-8?B?dXkvbVZNWnFIQ2FWUGFCVlU3RW9mdDNNRzJGSkQ1Rm9mb1JmcGI4dnJTdFVu?=
 =?utf-8?B?VmhuTUJ4OUd3SC8wY1BxM0R2d3BFOTYrUkRycElJRW1haTdIR0E4WXVzUE5S?=
 =?utf-8?B?dkZFSHM1d0pVdzVtVE4rdEJQQ3cyM0dPaTIzcWFaNHNwWFg3ZWFNUXZrRzMw?=
 =?utf-8?B?aW5RTnN4MnFUTlEyUXArQVFHbnV0QXVMd3lBbWxhbFBGRzIzWVIveUQxTlVI?=
 =?utf-8?B?THJ1MTlGU041T0JlUGQvMWNkd2ExS0pkaVEyY21PZ0dXdjFZT2NtbEFuNEVs?=
 =?utf-8?B?VlZHYkl6d0gxektLdVBFa1FDb1dJTGc1ZU5zOWpHeUdYdmZPb1hNK3hRUStG?=
 =?utf-8?B?aUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 47dba287-9896-4071-58ce-08dc6d9ddd0e
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 07:26:37.1690
 (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: ysPgH1d1ubFW1Xg2wJHw5BO73GqAZRiOTH/w3S0uS2trrC+TuucLYLlXo4S2b3XdwtWgYoWPlvIWqnpBd21dSA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4790
X-OriginatorOrg: intel.com

On 5/2/2024 4:40 AM, Sean Christopherson wrote:
> The shortlog is a bit stale now that it only deals with XSTATE.  This?
>
>    KVM: x86: Zero XSTATE components on INIT by iterating over supported features

OK, will change it.

>
> On Sun, Feb 18, 2024, Yang Weijiang wrote:
>> Tweak the code a bit to facilitate resetting more xstate components in
>> the future, e.g., CET's xstate-managed MSRs.
>>
>> No functional change intended.
>>
>> Suggested-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>   arch/x86/kvm/x86.c | 30 +++++++++++++++++++++++++++---
>>   1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 10847e1cc413..5a9c07751c0e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -12217,11 +12217,27 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>>   		static_branch_dec(&kvm_has_noapic_vcpu);
>>   }
>>   
>> +#define XSTATE_NEED_RESET_MASK	(XFEATURE_MASK_BNDREGS | \
>> +				 XFEATURE_MASK_BNDCSR)
>> +
>> +static bool kvm_vcpu_has_xstate(unsigned long xfeature)
>> +{
>> +	switch (xfeature) {
>> +	case XFEATURE_MASK_BNDREGS:
>> +	case XFEATURE_MASK_BNDCSR:
>> +		return kvm_cpu_cap_has(X86_FEATURE_MPX);
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>   void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>   {
>>   	struct kvm_cpuid_entry2 *cpuid_0x1;
>>   	unsigned long old_cr0 = kvm_read_cr0(vcpu);
>> +	DECLARE_BITMAP(reset_mask, 64);
> I vote to use a u64 instead of a bitmask.  The result cast isn't exactly pretty,
> but it's not all that uncommon, and it's easy enough to make it "safe" by adding
> BUILD_BUG_ON().
>
> On the flip side, using the bitmap_*() APIs for super simple bitwise-OR/AND/TEST
> operations makes the code harder to read.

Make sense.

>
>>   	unsigned long new_cr0;
>> +	unsigned int i;
>>   
>>   	/*
>>   	 * Several of the "set" flows, e.g. ->set_cr0(), read other registers
>> @@ -12274,7 +12290,12 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>   	kvm_async_pf_hash_reset(vcpu);
>>   	vcpu->arch.apf.halted = false;
>>   
>> -	if (vcpu->arch.guest_fpu.fpstate && kvm_mpx_supported()) {
>> +	bitmap_from_u64(reset_mask, (kvm_caps.supported_xcr0 |
>> +				     kvm_caps.supported_xss) &
>> +				    XSTATE_NEED_RESET_MASK);
>> +
>> +	if (vcpu->arch.guest_fpu.fpstate &&
>> +	    !bitmap_empty(reset_mask, XFEATURE_MAX)) {
>>   		struct fpstate *fpstate = vcpu->arch.guest_fpu.fpstate;
>>   
>>   		/*
>> @@ -12284,8 +12305,11 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>   		if (init_event)
>>   			kvm_put_guest_fpu(vcpu);
>>   
>> -		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
>> -		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
>> +		for_each_set_bit(i, reset_mask, XFEATURE_MAX) {
>> +			if (!kvm_vcpu_has_xstate(i))
>> +				continue;
>> +			fpstate_clear_xstate_component(fpstate, i);
>> +		}
> A few intertwined thoughts:
>
>   1. fpstate is zero allocated, and KVM absolutely relies on that, e.g. KVM doesn't
>      manually zero out the XSAVE fields that are preserved on INIT, but zeroed on
>      RESET.
>
>   2. That means there is no need to manually clear XSTATE components during RESET,
>      as KVM doesn't support standalone RESET, i.e. it's only cleared during vCPU
>      creation, when guest FPU state is guaranteed to be '0'.
>
>   3. That makes XSTATE_NEED_RESET_MASK a misnomer, as it's literally the !RESET
>      path that is relevant.  E.g. it should be XSTATE_CLEAR_ON_INIT_MASK or so.
>
>   4. If we add a helper, then XSTATE_NEED_RESET_MASK is probably unneeded.

Fair enough.
For #4, I still prefer to add all relevant xstate bits in a macro so thatxfeatures_mask initialization line length keeps shorter and constant.
>
> So, what if we slot in the below (compile tested only) patch as prep work?  Then
> this patch becomes:

Thanks! I'll replace this patch with the one your attached.

>
> ---
>   arch/x86/kvm/x86.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b441bf61b541..b00730353a28 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12220,6 +12220,8 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>   static void kvm_xstate_reset(struct kvm_vcpu *vcpu, bool init_event)
>   {
>   	struct fpstate *fpstate = vcpu->arch.guest_fpu.fpstate;
> +	u64 xfeatures_mask;
> +	int i;
>   
>   	/*
>   	 * Guest FPU state is zero allocated and so doesn't need to be manually
> @@ -12233,16 +12235,20 @@ static void kvm_xstate_reset(struct kvm_vcpu *vcpu, bool init_event)
>   	 * are unchanged.  Currently, the only components that are zeroed and
>   	 * supported by KVM are MPX related.
>   	 */
> -	if (!kvm_mpx_supported())
> +	xfeatures_mask = (kvm_caps.supported_xcr0 | kvm_caps.supported_xss) &
> +			 (XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
> +	if (!xfeatures_mask)
>   		return;
>   
> +	BUILD_BUG_ON(XFEATURE_MAX >= sizeof(xfeatures_mask));
> +
>   	/*
>   	 * All paths that lead to INIT are required to load the guest's FPU
>   	 * state (because most paths are buried in KVM_RUN).
>   	 */
>   	kvm_put_guest_fpu(vcpu);
> -	fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
> -	fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
> +	for_each_set_bit(i, xfeatures_mask, XFEATURE_MAX)
> +		fpstate_clear_xstate_component(fpstate, i);
>   	kvm_load_guest_fpu(vcpu);
>   }
>   
>
> base-commit: efca8b27900dfec160b6ba90820fa2ced81de904