Received-SPF: pass (google.com: domain of linux-kernel+bounces-39925-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <ad97d210-0ef3-4500-b26f-65aad0a444a3@intel.com>
Date: Fri, 26 Jan 2024 17:30:09 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 13/27] KVM: x86: Refresh CPUID on write to guest
 MSR_IA32_XSS
Content-Language: en-US
To: Chao Gao <chao.gao@intel.com>
CC: <seanjc@google.com>, <pbonzini@redhat.com>, <dave.hansen@intel.com>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
	<yuan.yao@linux.intel.com>, <peterz@infradead.org>,
	<rick.p.edgecombe@intel.com>, <mlevitsk@redhat.com>, <john.allen@amd.com>,
	Zhang Yi Z <yi.z.zhang@linux.intel.com>
References: <20240124024200.102792-1-weijiang.yang@intel.com>
 <20240124024200.102792-14-weijiang.yang@intel.com>
 <ZbI+pexl9Th0KiiU@chao-email>
From: "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZbI+pexl9Th0KiiU@chao-email>
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?NFVsSGhZV1ZMc3RvVzE0bHZaR2JxM3VOU09vZzdjcytuWmNmbytPNXN6c2tt?=
 =?utf-8?B?dW5GZFZFS3FkcWplWXhjd0swaW5IWnhoQzMrblpQNGIvb1NXazh4Rlc0Y1pj?=
 =?utf-8?B?dUc4cGJ6WlNwMXJWRWtWYVZ6RlZpOXRQSDZQUHVKT2U3cXlPKy8xTmNML3Fw?=
 =?utf-8?B?NzVtWHdLcGtMQU1pT1Jkb1NSSURwQTdsRHl5elFHeHpTSkZjR1U5bjRUaDlh?=
 =?utf-8?B?V1lGRUVNcVcvVFVwQ1RoUm1jQXN3ME9KSTVRSFAwOVlYbnl0VldZYWFPZ1Iw?=
 =?utf-8?B?dUg1VTRwVjBjNHU3YU5CUzlIdGQreDNpTmI3V3NDbmFnSklWYndac2JKWnRB?=
 =?utf-8?B?NnN6WGErTnJRRHBNZkd5VHJ2UTA4UjdLVWtqVm5pUFduOGxkMk5qMWFlS1R1?=
 =?utf-8?B?SjRzakE4ZkNhZDNRUXRQdzNnUWNsU2hkZnhxU0UrTnY1MmRTUFZ5YjJFRVFu?=
 =?utf-8?B?bjdXQWt5TElKVUQ4K3AvanNQanIwYUtpRWlId0RWVWRZRGlpMnpJMzc1UGNs?=
 =?utf-8?B?aFdHNHd5U0dhU05vQll6WGJBL0FLWE51U240SUZ3WUNxUTFLb2dIcXBjYzZN?=
 =?utf-8?B?SysyN01XSE5TNEFsNWhscFI1MUJsMVgxdWg0ZlNDRHlFQXhoSkxxTjVVSWJn?=
 =?utf-8?B?OEVRODJDWFo1WjA5VGw1RHdPMUhKV0pOcGt3RVFNc1djb3F0eVZuRHFtZjVB?=
 =?utf-8?B?ZXN3ak9wWlhZMGZEbmw4YklFMHFzcS9HMU50b0F6QkpEOENtS2lQamlwc0xi?=
 =?utf-8?B?ei9EeEJsWEZxb0hRZEJTUDJoeVI5L1pUams2TDg4NVBQMW5iQ2doYWZIMXls?=
 =?utf-8?B?dDhEWVJBN20vWUVaaWNFd1h5N1YzRUJIT3FFSFVqaHFTeFVZSDFCeVVNWDZL?=
 =?utf-8?B?VTZFcjZ4Zm13dkJKdWNqUER6dVdIdGJBOW81RzZ2VVNZTFJzK280c3NNTmlU?=
 =?utf-8?B?cEFWZm1Id3VrVlVJT0lVcnZKempRZnc0a1hpdlU0aVk3MUh3KzNDMUhITjFY?=
 =?utf-8?B?aUNpajhqV0hleFF5eTdpUWRzUCttQ3dlLzBzU25pMWxuZ091TFNHZlNnaFFN?=
 =?utf-8?B?dmxFS1U4RnJJYVpyMlBoZGl0WkwvcDVWWDY2cHhBd1cyVHpNbUJScldoWnFJ?=
 =?utf-8?B?ZVVXWUhxK2Jpbk1NaEhpTzdLdXBqQ1VVSzc3azk3aUhtaWl2Qi9WVjdwV2xZ?=
 =?utf-8?B?UFhsRzU2UU10bU9MTCtNMEppWVFlVkpWd3ZsWitLS3ZiZHhwTXowU2sxbUtS?=
 =?utf-8?B?WHVKeS9TRUNaT3VDb3lEcURYN1ZONHhBU05CNHVmRnZOMVFoUlNsUExXcTEv?=
 =?utf-8?B?SU9zU3pCMUdTeUQxekdUZ0R3OEJhdVRwb3RIV1NLbGVWQ2U5RnJzNjl4VXRZ?=
 =?utf-8?B?QndoVnlhWkdINUlHcjVnQzNrM2Q1dlQ4SlpYM0h0eElWNVRZdlR2WmYySkcw?=
 =?utf-8?B?VW1PR2xOektSV2ZUQlM5Nk5EZTBSNlgyNTFEby81aXJyUnU2cEwrNDBSUzE0?=
 =?utf-8?B?VmxhY3ZXUElsd1MvbjdvMGRHU2RpWEpPQldhNUUxTXlpNW5zTHoxM1ZwamRN?=
 =?utf-8?B?WFRCclpkVHFCMXdjY3Z3SElYYmM3NUt5N1JLZHlJZUlUdWN4K3hrMnNrakt1?=
 =?utf-8?B?Q1lXUHp5dnhqN09rSklpbDFIN0FUYXlDY0ZESUkxZVU5eW5FODUyQmhBM0ww?=
 =?utf-8?B?N0xUV29kSzBsYUc5L2xqcVozSHBTd1pDYU0xdDdRRjZIMFdWOExBN0ZiVnNN?=
 =?utf-8?B?NEpXaW9sbm56Rk1lYXFRVDRyaFk1S201cUt5V0V2YXhFbG1Sd1lIUkRnTDdN?=
 =?utf-8?B?eHcvUXJ6MTNhRWFtMlNkQ21xUzhQVTJNNFRISytITGk3akZTN2VwSGErd2Mx?=
 =?utf-8?B?YTVxM0I2a2JKdmdOWXltK2ZnQ1ByQjhBQjhzZDNQTE9PMXE2Q1Fvbm5KQktQ?=
 =?utf-8?B?a1k1Y1NMdXM0NXl1SllyYUNJbzV5TmpiREl2ajdQcG85SHE5V0xVb0UrRjdK?=
 =?utf-8?B?WTBYMUhoOUVEN3J0RldRNk45VkhWWEZBV2ZBMndlb3dZVFNOK3VvQ0tjaURJ?=
 =?utf-8?B?RDdjM1pFQzd0R1ZPR0YyV1owM3dBN3N1b0F1aHNidzRRL3h1YWhZbDlwQ3ZT?=
 =?utf-8?B?ZytXUHpJQVFkbG9zYUVtTis5SE5aT2x6TWgyeDBiQW1KdS9KTStsM0N3U3h0?=
 =?utf-8?B?RHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 188c4ffb-f3ce-421c-3f1a-08dc1e516c90
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 09:30:24.5883
 (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: HhVkQ1aIVu3szi7g8AIRTOXboqAD5qmRnBTcSvhi3vL1bKlI2hdoNIIhMf3QRLzgay/uEF/2ftwIXRyQgQ6gvg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4805
X-OriginatorOrg: intel.com

On 1/25/2024 6:57 PM, Chao Gao wrote:
> On Tue, Jan 23, 2024 at 06:41:46PM -0800, Yang Weijiang wrote:
>> Update CPUID.(EAX=0DH,ECX=1).EBX to reflect current required xstate size
>> due to XSS MSR modification.
>> CPUID(EAX=0DH,ECX=1).EBX reports the required storage size of all enabled
>> xstate features in (XCR0 | IA32_XSS). The CPUID value can be used by guest
>> before allocate sufficient xsave buffer.
>>
>> Note, KVM does not yet support any XSS based features, i.e. supported_xss
>> is guaranteed to be zero at this time.
>>
>> Opportunistically modify XSS write access logic as:
>> If XSAVES is not enabled in the guest CPUID, forbid setting IA32_XSS msr
>> to anything but 0, even if the write is host initiated.
> any reason to allow host to write 0? looks we are not doing this for many
> other MSRs.

Paolo mentioned this point for many times, and the latest one can be found at:
Re: [PATCH v5 04/19] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS - Paolo Bonzini (kernel.org) <https://lore.kernel.org/kvm/CABgObfbvr8F8g5hJN6jn95m7u7m2+8ACkqO25KAZwRmJ9AncZg@mail.gmail.com/>

For other MSRs, Sean proposed to enforce the policy in batch, but the work is delayed.

>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>> ---
>> arch/x86/include/asm/kvm_host.h |  3 ++-
>> arch/x86/kvm/cpuid.c            | 15 ++++++++++++++-
>> arch/x86/kvm/x86.c              | 16 ++++++++++++----
>> 3 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 40dd796ea085..6efaaaa15945 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -772,7 +772,6 @@ struct kvm_vcpu_arch {
>> 	bool at_instruction_boundary;
>> 	bool tpr_access_reporting;
>> 	bool xfd_no_write_intercept;
>> -	u64 ia32_xss;
>> 	u64 microcode_version;
>> 	u64 arch_capabilities;
>> 	u64 perf_capabilities;
>> @@ -828,6 +827,8 @@ struct kvm_vcpu_arch {
>>
>> 	u64 xcr0;
>> 	u64 guest_supported_xcr0;
>> +	u64 guest_supported_xss;
>> +	u64 ia32_xss;
>>
>> 	struct kvm_pio_request pio;
>> 	void *pio_data;
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index acc360c76318..3ab133530573 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -275,7 +275,8 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
>> 	best = cpuid_entry2_find(entries, nent, 0xD, 1);
>> 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
>> 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
>> -		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>> +		best->ebx = xstate_required_size(vcpu->arch.xcr0 |
>> +						 vcpu->arch.ia32_xss, true);
>>
>> 	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
>> 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
>> @@ -312,6 +313,17 @@ static u64 vcpu_get_supported_xcr0(struct kvm_vcpu *vcpu)
>> 	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
>> }
>>
>> +static u64 vcpu_get_supported_xss(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm_cpuid_entry2 *best;
>> +
>> +	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 1);
>> +	if (!best)
>> +		return 0;
>> +
>> +	return (best->ecx | ((u64)best->edx << 32)) & kvm_caps.supported_xss;
>> +}
>> +
>> static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent)
>> {
>> #ifdef CONFIG_KVM_HYPERV
>> @@ -362,6 +374,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>> 	}
>>
>> 	vcpu->arch.guest_supported_xcr0 = vcpu_get_supported_xcr0(vcpu);
>> +	vcpu->arch.guest_supported_xss = vcpu_get_supported_xss(vcpu);
>>
>> 	kvm_update_pv_runtime(vcpu);
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index b3a39886e418..7b7a15aab3aa 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3924,20 +3924,28 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> 			vcpu->arch.ia32_tsc_adjust_msr += adj;
>> 		}
>> 		break;
>> -	case MSR_IA32_XSS:
>> -		if (!msr_info->host_initiated &&
>> -		    !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
>> +	case MSR_IA32_XSS: {
> unnecessary bracket.

Yes, will remove it.

>
>> +		/*
>> +		 * If KVM reported support of XSS MSR, even guest CPUID doesn't
> IIUC, below code doesn't check if KVM reported support of XSS MSR. so, the comment
> doesn't match what the code does.

Here I refers to what it does in patch 12.

>> +		 * support XSAVES, still allow userspace to set default value(0)
>> +		 * to this MSR.
>> +		 */
>> +		if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVES) &&
>> +		    !(msr_info->host_initiated && data == 0))
>> 			return 1;
>> 		/*
>> 		 * KVM supports exposing PT to the guest, but does not support
>> 		 * IA32_XSS[bit 8]. Guests have to use RDMSR/WRMSR rather than
>> 		 * XSAVES/XRSTORS to save/restore PT MSRs.
>> 		 */
>> -		if (data & ~kvm_caps.supported_xss)
>> +		if (data & ~vcpu->arch.guest_supported_xss)
>> 			return 1;
>> +		if (vcpu->arch.ia32_xss == data)
>> +			break;
>> 		vcpu->arch.ia32_xss = data;
>> 		kvm_update_cpuid_runtime(vcpu);
>> 		break;
>> +	}
>> 	case MSR_SMI_COUNT:
>> 		if (!msr_info->host_initiated)
>> 			return 1;
>> -- 
>> 2.39.3
>>