Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4565ab4b-f386-7b70-4634-627e92acbb45@intel.com>
Date:   Thu, 27 Jul 2023 14:06:11 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v4 09/20] KVM:x86: Add common code of CET MSR access
Content-Language: en-US
To:     Chao Gao <chao.gao@intel.com>
CC:     <seanjc@google.com>, <pbonzini@redhat.com>, <peterz@infradead.org>,
        <john.allen@amd.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <rick.p.edgecombe@intel.com>,
        <binbin.wu@linux.intel.com>
References: <20230721030352.72414-1-weijiang.yang@intel.com>
 <20230721030352.72414-10-weijiang.yang@intel.com>
 <ZMDMQHwlj9m7C39s@chao-email>
 <67250373-c5f4-d1d7-9334-4c9e6a43ab63@intel.com>
 <ZMEjudsdr8WEiw3b@chao-email>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZMEjudsdr8WEiw3b@chao-email>
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?MFNSYzN5WFBGcEo4K1pQdTRyVTFXeFRJaldyMmZGSW54WDNGOWVZcEtwSk91?=
 =?utf-8?B?N20rN29EMnBRQmNLdE94K3Q5M2tuRmpDWnRJanJHREVXUmc3UGZITkdHL1NY?=
 =?utf-8?B?a3ZUaC92UGxwWURuWklSWHRNamdoT1JwUi9ZQVpjNWJOYXVmcUNwUy9reDNy?=
 =?utf-8?B?Z1FzOVR2T0tnZU51VENQb2ZWQTRzQWdTVzYwSlphNnVuakdPMU9OTmVwTXFS?=
 =?utf-8?B?QktvbXVTcFBNUFBpTFhQbWpHcVEzTVA4QVEyN1h6d1VXZzZsaXZRbFJaTkU4?=
 =?utf-8?B?V1FsK0FDSGppQkhDVlZzOTJ5ZnJ3SzY4MEFta3IweGVMMDNPcS92U1ZhMHF5?=
 =?utf-8?B?THVraFhmVUJ4OVBJbUJ2bDBkejQxNjlpem43TWdkRUc0Y0pPVUVzWXlmZFQr?=
 =?utf-8?B?aXJRTVdWYXcxUjhlTWl4SHBObVRESG5oYjFWMjJLWmwyWDh0SFV6UFJDLzRP?=
 =?utf-8?B?UHdsU01HaUlraXNRTXNTY2lJT0FnTTNycEsvKzlmdFN4QzY1M3FsSzNnbmxa?=
 =?utf-8?B?TjU1ODB1TTNKc3dTSmI5clMxV1dOaDR1QXlVQ1B2WVN5RlB5cmdMWk92bFRH?=
 =?utf-8?B?L05Rc09oczRzdXVUTGNhZHA0aXMvWXNHRzdhZzVpM0NUNEdZbUFvd2tody96?=
 =?utf-8?B?cnA3YkJrb2tKVit5OEl0NTBMT2JoNGd0ckxVZzJuTkVJeklkMkd5bzlFUE40?=
 =?utf-8?B?Tkp0M3RFdElXQVcvSEFUTG16eDR6anpMbTE5VmRnK0pvT1N1NzgxY2U0Y0FD?=
 =?utf-8?B?U05tM01jSHk0Y0d4TEszSTdLejJUVXBzZElwZGFmSkF5M0w2RXE4WEwwdGRJ?=
 =?utf-8?B?Wlk3Vndkc0Fjay91Vy9uV0lJR25NN3MwL3hPVHl0VWlobnVTSnFlWk5TWm1w?=
 =?utf-8?B?ejZEMUhlQ0p4N1RwL3cyOHA4REhnckl2SDk2Qk1aV2lMMVJPTzFOLzhjWSsr?=
 =?utf-8?B?cEJsd0I2TGdnNGt3OHZCT3ZuT2g3ZmQyajZqb2lYUm9DamFKYVdjaVZYVS9y?=
 =?utf-8?B?WElzcnhTYVFQb2plbXp2bDhkOCszRmNpYXAwRE5ldDhQZzRIZ2g3SllYUDFh?=
 =?utf-8?B?Z05sVHMxREl5NWR2WmZtZ01yY2JrVjF4TEFUNmQxUXIwSkkrRWVuYTV5RSsz?=
 =?utf-8?B?SmhPVERNRkl3VnFrS01jeUhSU3N5bVpQaWxlRjRmZFVadzc2dENaNjBGUUl6?=
 =?utf-8?B?ek12MVdqWkRSQVZIZ3AyNHZpUjRCbXFORWo2cmJyNUU3SjR0LzA2M2JnNXU1?=
 =?utf-8?B?U2dvQTVraWZXSE16MGtYZVMwVys1Q1o5VzhlRUxqYkoxUzM0b2Zjdm1BVEVX?=
 =?utf-8?B?cFEyU0p0TzNrRXVhOEF3bldBNFJTWjdVUHRoQ0dxTTErNHZxblNheHNPZE9q?=
 =?utf-8?B?OHVWOWJMSlo3bFBsS3ZFd01SdHlPeFFjWW8xZnZ6YUV2azQ2c21FejlXZk9G?=
 =?utf-8?B?cnF5b2xnNm5uVWVsZExiMnVyNFpkdm5qSjdvbGlrejZhM25CWEdERGk3RldB?=
 =?utf-8?B?YW1PcXAyVWVxbUxVMmFlQlBRaFhtOVJ5TzFvRXpkTzBvUXZ5WitDbzVYSzJ1?=
 =?utf-8?B?SERndWlOWEl2U3htcmRsUmswQVFXOFR6YnlRcWo4dG9VSjM4VFVjVEJ2MXVZ?=
 =?utf-8?B?OGRPNFF5a2RCa21aT1pWaXBVdUwwYjVmYTBJM3lkNHRBNDlaTHJrOWgwbUlz?=
 =?utf-8?B?QWlENFBaQXZlRlRZbFJ6TW1PL0x0THlTZHhweTMxNDVyTFFrN3Z4djUyaTUx?=
 =?utf-8?B?eUkvRUhQZ2NwNTY1Z3ZueEhIMTc3SngydUZJS3IxeTJDbGYvWS9iQStxSXN5?=
 =?utf-8?B?Wmlpd3Z2N2NibUVqMkVLV0pjWSthWktBSlVzR2duZnpNUGNQNXVyZlpFWjlw?=
 =?utf-8?B?UUgyWkNoZmg2eHFZK1pnNU1XcjlubWVOZFIyVTArOUgwK1V3RzBnSEVudGVG?=
 =?utf-8?B?N2F1bU9oSmIrc3V0R0F6dzU2SitpbnNhekNnTWtZRzZ4NlZWc3hDbDZ6aVNF?=
 =?utf-8?B?VDRaek1mekdaMWJLWHl6aDVzNk1GaGF4dWNUSDI4MUxXTUNkQkRiNWxESE0v?=
 =?utf-8?B?TklJWFkzRzYra2lqaVBlUi9XMFNReXEyYmRWclZwMUo2eDZyQ09YT1lEYnd6?=
 =?utf-8?B?blRGNkV6K2R5Z3YreTdUZWErbjJCdWtTdEJaUUFzaDdXcktLMUdQM0Mzc2dr?=
 =?utf-8?B?NUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d0bc99a9-804e-460c-f1c8-08db8e679bb9
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jul 2023 06:06:25.1386
 (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: a2bj+JEVjd3MCfyEkl5vt4ItKAsDuszPPIrMqwvFm9LTT67dw8TTaIKI5i8qYKS31TPmRrKr290sIUBaBfDLYg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7416
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        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


On 7/26/2023 9:46 PM, Chao Gao wrote:
> On Wed, Jul 26, 2023 at 04:26:06PM +0800, Yang, Weijiang wrote:
>>>> +	/*
>>>> +	 * This function cannot work without later CET MSR read/write
>>>> +	 * emulation patch.
>>> Probably you should consider merging the "later" patch into this one.
>>> Then you can get rid of this comment and make this patch easier for
>>> review ...
>> Which later patch you mean? If you mean [13/20] KVM:VMX: Emulate read and
>> write to CET MSRs,
>>
>> then I intentionally separate these two, this one is for CET MSR common
>> checks and operations,
>>
>> the latter is specific to VMX, and add the above comments in case someone is
> The problem of this organization is the handling of S_CET, SSP, INT_SSP_TABLE
> MSR is incomplete in this patch. I think a better organization is to either
> merge this patch and patch 13, or move all changes related to S_CET, SSP,
> INT_SSP_TABLE into patch 13? e.g.,

Yes, I'm thinking of merging this patch with patch 13 to make it 
complete, thanks for

the suggestion!

>
> 	case MSR_IA32_U_CET:
> -	case MSR_IA32_S_CET:
> 		if (!kvm_cet_is_msr_accessible(vcpu, msr_info))
> 			return 1;
> 		if ((!guest_can_use(vcpu, X86_FEATURE_SHSTK) &&
> 		     (data & CET_SHSTK_MASK_BITS)) ||
> 		    (!guest_can_use(vcpu, X86_FEATURE_IBT) &&
> 		     (data & CET_IBT_MASK_BITS)))
> 			return 1;
> -		if (msr == MSR_IA32_U_CET)
> -			kvm_set_xsave_msr(msr_info);
> 		kvm_set_xsave_msr(msr_info);
> 		break;
> -	case MSR_KVM_GUEST_SSP:
> -	case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB:
> 	case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
> 		if (!kvm_cet_is_msr_accessible(vcpu, msr_info))
> 			return 1;
> 		if (is_noncanonical_address(data, vcpu))
> 			return 1;
> 		if (!IS_ALIGNED(data, 4))
> 			return 1;
> 		if (msr == MSR_IA32_PL0_SSP || msr == MSR_IA32_PL1_SSP ||
> 		    msr == MSR_IA32_PL2_SSP) {
> 			vcpu->arch.cet_s_ssp[msr - MSR_IA32_PL0_SSP] = data;
> 		} else if (msr == MSR_IA32_PL3_SSP) {
> 			kvm_set_xsave_msr(msr_info);
> 		}
> 		break;
>
>
>
> BTW, shouldn't bit2:0 of MSR_KVM_GUEST_SSP be 0? i.e., for MSR_KVM_GUEST_SSP,
> the alignment check should be IS_ALIGNED(data, 8).

The check for GUEST_SSP should be consistent with that of PLx_SSPs, 
otherwise there would

be issues, maybe I need to change the alignment check as :

#ifdef CONFIG_X86_64

if (!IS_ALIGNED(data, 8))
	return 1;
#else
if (!IS_ALIGNED(data, 4))

	return 1;
#endif

>
>> bisecting
>>
>> the patches and happens to split at this patch, then it would faulted and
>> take some actions.
> I am not sure what kind of issue you are worrying about. In my understanding,
> KVM hasn't advertised the support of IBT and SHSTK, so,
> kvm_cpu_cap_has(X86_FEATURE_SHSTK/IBT) will always return false. and then
> kvm_cet_is_msr_accessible() is guaranteed to return false.
>
> If there is any issue in your mind, you can fix it or reorganize your patches to
> avoid the issue. To me, adding a comment and a warning is not a good solution.

I will reorganize the patches and merge the code in this patch to patch 13.

>
>>>> int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>> {
>>>> 	u32 msr = msr_info->index;
>>>> @@ -3982,6 +4023,35 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>> 		vcpu->arch.guest_fpu.xfd_err = data;
>>>> 		break;
>>>> #endif
>>>> +#define CET_IBT_MASK_BITS	GENMASK_ULL(63, 2)
>>> bit9:6 are reserved even if IBT is supported.
>> Yes, as IBT is only available on Intel platforms, I move the handling of bit
>> 9:6 to VMX  related patch.
> IIUC, bits 9:6 are not reserved for IBT. I don't get how IBT availability
> affects the handling of bits 9:6.

I handle it in this way,  when IBT is not available all bits 63:2 should 
be handled as reserved. When IBT is

available, additional checks for bits 9:6 should be enforced.

>
>> Here's the common check in case IBT is not available.
>>
>>>> @@ -12131,6 +12217,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>>
>>>> 	vcpu->arch.cr3 = 0;
>>>> 	kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
>>>> +	memset(vcpu->arch.cet_s_ssp, 0, sizeof(vcpu->arch.cet_s_ssp));
>>> ... this begs the question: where other MSRs are reset. I suppose
>>> U_CET/PL3_SSP are handled when resetting guest FPU. But how about S_CET
>>> and INT_SSP_TAB? there is no answer in this patch.
>> I think the related guest VMCS fields(S_CET/INT_SSP_TAB/SSP) should be reset
>> to 0 in vmx_vcpu_reset(),
>>
>> do you think so?
> Yes, looks good.