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;
Date:   Wed, 26 Jul 2023 21:46:33 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     "Yang, Weijiang" <weijiang.yang@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>
Subject: Re: [PATCH v4 09/20] KVM:x86: Add common code of CET MSR access
Message-ID: <ZMEjudsdr8WEiw3b@chao-email>
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>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <67250373-c5f4-d1d7-9334-4c9e6a43ab63@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?l/FjcgBetxW5/j2jpRD/uqvW5kuBDEhj4s92rtIv624AWxloKRPrcyV3BC?=
 =?iso-8859-1?Q?mQFhZCVRM54h+j0h5nYD9Iyw47zR/rlT8xyMXTyYLMJeiGuKYc3Ckw1uBb?=
 =?iso-8859-1?Q?vTmd/Dv5PSuOiS4GFj7/IabUp0iaSRLFx+hFx7udJ4HfNn43Iv8kHF8qpf?=
 =?iso-8859-1?Q?PSH0shzQ/XdEKLTskyi/hetmtxo3cl7XDdbk1ZMUFM1hD1ReOkNtfAYTxM?=
 =?iso-8859-1?Q?qAMJxw86+YNfDxFAHtocT1+7Z2Qer6f+8+W1k7HTeTyjkQ4JvJJXV6769n?=
 =?iso-8859-1?Q?gQXu6060ETuKZfRjS8F7ASRmLTneyWWCSLar0vObFCfI87LV6GiA/FRG5N?=
 =?iso-8859-1?Q?kElo7DcyU61B15xIkC4DXTuBRb8VEp8E0nIUjpVyVyJkipUopEF/nlCGga?=
 =?iso-8859-1?Q?4dQYf2QlonK6DDu7vt4N5FqadqLkzrtTMIleuxKxss7Alr4AkkTRcFj9lf?=
 =?iso-8859-1?Q?3dtIWuD2krjaxjgELesBlnqZVS4RvT6GfJwvJO19AUDsih/UvuL7+I7gEI?=
 =?iso-8859-1?Q?CO7OqhukmSyvUBJXNQzjD2lDAvBz0OGlLR6hbo7SNQPLzYPEzyFtx9zhb9?=
 =?iso-8859-1?Q?6ctNBI88TpXihnZBiphYWOte3VnxY/IqJVRql5gbyRLdyQAlBTWY90q2tv?=
 =?iso-8859-1?Q?8mc/d4GLAMsUuHS01AY+Y18MKcmjv+zY7q+CLeNxnAMpkCKRoqhETVURTB?=
 =?iso-8859-1?Q?WFdq/dIEDlTYlPQ9vzyNgZGdmRU+tljGeVgMMQR2jJF6mAaa6AWGAQ7KCt?=
 =?iso-8859-1?Q?clO00k5SBmjRkcfekdoSMR0ZJozMYI7022+smsUewP3hOMMyO3z8SnsLDK?=
 =?iso-8859-1?Q?nBQA5qhFP67eANjt3dd7EwjwbuVAYuVk46rwQTuBcwBmJgqcf/Czs7PpiH?=
 =?iso-8859-1?Q?PkiMiQugaTEVqjwuW6C0Pu9ny0894MZX5BI2p9lwSq434gMMaOhKxkyuAL?=
 =?iso-8859-1?Q?dbRMTSBKTSBsaxVIRM87PC+nkUhmQRBG3cYOxax+d8LO/qCVAxlBQkWrh7?=
 =?iso-8859-1?Q?s7jlVLLmSLkCyXNJxvzG8ODgpwsfmJJarJXpzwz87aWxYeSee3mrqL1erw?=
 =?iso-8859-1?Q?UuC+xk5sI0Yiyw690s0c7hX21mkHMadcEhGLRx0GejTssV3Utddszsb41V?=
 =?iso-8859-1?Q?0OclbJF1VVTGPzzmPZEZJ5DfuzWROObsrQUjj7+8PDPO1pxkkT2+qJ7I8d?=
 =?iso-8859-1?Q?RuKT2owiPSmmCk2y1xUUnwn40akgmvUG+s2WA6BVUA903xAwvcTGoGdoVh?=
 =?iso-8859-1?Q?kaShB+SsoEwaALnUOWWvm+T92zd+kDbGxXKj7ckaxstSImh+WEeBrW7zYX?=
 =?iso-8859-1?Q?5ugbDMIhUe4wVzSniJDf4YTni47fHaRZ/FEvL65HnB/OjroBNHTOOxBkr4?=
 =?iso-8859-1?Q?nPF4r1fqJ/4arwlk10yeifpIOIvgQp/BVmV8uteIvxajoM22BFXYiqR0Yi?=
 =?iso-8859-1?Q?SWNrjLU674znN7eGAhDfPxB73IlKrFFGvyMJNKApVYrT9Sr6sb4gULQDjY?=
 =?iso-8859-1?Q?o9YLfPuk6T/3LsUaBxiGKBhTfn9V4uPXLtNhvJogNuh62MX4UhsMwTNwCm?=
 =?iso-8859-1?Q?V6YWkK9qE/bJQQZ5k8lVfD4MfB3FDGvEbzsjS6HwCkL7E1uAAm/dcXerCj?=
 =?iso-8859-1?Q?vsIK/TQ/sIfLZqeQVQhhr4hmD/CKi3GXyS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 848d8365-abd6-4878-8c6e-08db8ddebf68
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB6780.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jul 2023 13:46:44.0211
 (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: s7xNlShTLRF2zE8atc1Wn8UaTMUBUggM1yyg/F5z64MoL/KeiYcyvrTl1uYGyPWb1WnObEmZO2EZqDokdUY+2w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5390
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,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

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.,

	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).

>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.

>
>> > 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.

>
>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.