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:   Thu, 27 Jul 2023 13:16:36 +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 13/20] KVM:VMX: Emulate read and write to CET MSRs
Message-ID: <ZMH9tIXfPk0dl7ye@chao-email>
References: <20230721030352.72414-1-weijiang.yang@intel.com>
 <20230721030352.72414-14-weijiang.yang@intel.com>
 <ZMDT/r4sEfMj5Bmu@chao-email>
 <3d5fdd07-563c-6841-a867-88369c4dbb36@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <3d5fdd07-563c-6841-a867-88369c4dbb36@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ZTA4ROTzagDUZKPVywaXd0a8KCGc+eHcaQhG4ApAFV1bNgccuK4TLcFm6UEJ?=
 =?us-ascii?Q?TUdBWl1h6hDX1P77mHsOfhM3qg6x5s1Q/KvOqsSWief0VMzbh+I2ZvFVShW2?=
 =?us-ascii?Q?atR+0XvQ2g8P5/y/scwv9n5kJDid/5edra4EcoFnNJR4LQ9mMf+QS1Sb+opW?=
 =?us-ascii?Q?d/Gnc59u1zi33daALDpyi6v2vZzd3k/Krj469hF1+qPd7pd4QbxIyu6K1lWY?=
 =?us-ascii?Q?pdgrgyv+CZZniO+gT0Qxr1kqZ9Dy0SibXXce+WEp18ZW12c5VVj8kGI46LV8?=
 =?us-ascii?Q?lcJuDR+m/FxmitfZINRYzpi04lunUCy4vtotXQbdYMmBkZcCqgomgQ0BPful?=
 =?us-ascii?Q?Ed2+VNK8VGY+Ip5jyDGoSMoHUC2LC6sJOp0QvGv1J7PbisIMTdyiGp9ir7C9?=
 =?us-ascii?Q?vOZcXH6cbQ6NXIxfbHNH8aVFcV5mz8sUk9ypScattysgnutL4abilJX4FMCv?=
 =?us-ascii?Q?Z+CvBffiKUL8i+SI7d3d9wEJ6nFUKKPsngSK82kaUK6r5S3Bs2eWKcj2t7xL?=
 =?us-ascii?Q?eaaUZh/odrdJUiJUb6qwZyPcJp404pFApD6f5cH8INdeyo5i6yfKeGaSLjFG?=
 =?us-ascii?Q?IMGOicW3x+B/y9n0RZtc0ApPy5ydFLCnVfc98xY6CZw6cjtKcHdUOH9/7o6g?=
 =?us-ascii?Q?gtkpJvT6+i9EUIAWUCUfRk/d8xc9W30wYVkse2j2KdvaFNIWgsct96FZ9HI6?=
 =?us-ascii?Q?sUuWJxhfOXVFtQhD0CqmmZtnUXtGaqUwNIJcnCbEK8irrhPkLMVovZ2N43+g?=
 =?us-ascii?Q?G1j9Mqzcxh77Pe1cdsGV4tz/tLfJQ0Z+VaUCkCuNuPnVsH9m/aeZZGmk+agw?=
 =?us-ascii?Q?/cAa4ked9Sj2yI1OYIYKa5SS4eK/cVPCTSW3h9d2RLBGzQ8sV6PGg8gzBSfF?=
 =?us-ascii?Q?BwCXOb4152CCSjviihGD2AoBFkXUhvDlId+UtmUBTPzuXAPfLO8DBkyaxiZ1?=
 =?us-ascii?Q?Kibi15IfRTClk4qQwbK+4FpT1b2BkFV/604nhIxKA0c4k87WBtGFZDn0U1QC?=
 =?us-ascii?Q?0KWM5DMqf+s3Ag/exE83I6gcGHDfZpb4WhkBN5Ebrp6YkMQC1twEgPx2mIZy?=
 =?us-ascii?Q?y5OoPVXK4tEiZpbH0GlR0fR0rknwdY1ckfNMpxDY+tGZQO65zuBDCua7Fg9g?=
 =?us-ascii?Q?TambJY2oCwQcj00KspBPsIKpIzOfHUOqxHqtwNz07iTtE/G+YJuw/tTIV+3w?=
 =?us-ascii?Q?cZiPiL3ovO8yiTAFR/eZgxecD2uV3bCEGvmabN4H3lkGllofnSQB0O6O0Yxm?=
 =?us-ascii?Q?yUG+Rpx+wAga+rOO4FdJBSAQCjmER+lx0W1/RsPXIbRG+4BeBlmev5KDHCyA?=
 =?us-ascii?Q?mTrz1b1m+DDf1BYDnjGqBWd7SY9mPMwv3hGxxTn7/6IH27zYUImKHl6VGbl+?=
 =?us-ascii?Q?8vwbi1ngGhKVD06nyZjio3wkYBSMfsTyGfRY9hW7K9v0Sw0AR31suWDfQgvQ?=
 =?us-ascii?Q?8lf8Wxd247KeBKwhvj2+pPpnrW45rfCzMvqNi2BJ4WAqIzy2mrbitYFCInc5?=
 =?us-ascii?Q?4O1tLjj8oC+Mo90J8RxfEOJS7KNmd9eEFJVICQugZqk7XV9llRHKjE4wQnGc?=
 =?us-ascii?Q?0aa+ovXsZ527GDqpS44YwnYO/Wpqvjg49vGkFUXd?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f34772a5-14b3-4e82-08df-08db8e60ab25
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB6780.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jul 2023 05:16:44.3556
 (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: Gfj3bIDNJH4yhmsV/Kar925IKPvXeleR28wT3MytJKGFhYz89uBcIZNfpBB6tvyH+HDIn8kTkpch7ri7x5a7ag==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6996
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H2,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

>> > +	case MSR_IA32_S_CET:
>> > +	case MSR_KVM_GUEST_SSP:
>> > +	case MSR_IA32_INT_SSP_TAB:
>> > +		if (kvm_get_msr_common(vcpu, msr_info))
>> > +			return 1;
>> > +		if (msr_info->index == MSR_KVM_GUEST_SSP)
>> > +			msr_info->data = vmcs_readl(GUEST_SSP);
>> > +		else if (msr_info->index == MSR_IA32_S_CET)
>> > +			msr_info->data = vmcs_readl(GUEST_S_CET);
>> > +		else if (msr_info->index == MSR_IA32_INT_SSP_TAB)
>> > +			msr_info->data = vmcs_readl(GUEST_INTR_SSP_TABLE);
>> > +		break;
>> > 	case MSR_IA32_DEBUGCTLMSR:
>> > 		msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL);
>> > 		break;
>> > @@ -2402,6 +2417,31 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> > 		else
>> > 			vmx->pt_desc.guest.addr_a[index / 2] = data;
>> > 		break;
>> > +#define VMX_CET_CONTROL_MASK		(~GENMASK_ULL(9, 6))
>> bits9-6 are reserved for both intel and amd. Shouldn't this check be
>> done in the common code?
>
>My thinking is, on AMD platform, bit 63:2 is anyway reserved since it doesn't
>support IBT,

You can only say

	bits 5:2 and bits 63:10 are reserved since AMD doens't support IBT.

bits 9:6 are reserved regardless of the support of IBT.

>
>so the checks in common code for AMD is enough, when the execution flow comes
>here,
>
>it should be vmx, and need this additional check.

The checks against reserved bits are common for AMD and Intel:

1. if SHSTK is supported, bit1:0 are not reserved.
2. if IBT is supported, bit5:2 and bit63:10 are not reserved
3. bit9:6 are always reserved.

There is nothing specific to Intel.

>
>> 
>> > +#define CET_LEG_BITMAP_BASE(data)	((data) >> 12)
>> > +#define CET_EXCLUSIVE_BITS		(CET_SUPPRESS | CET_WAIT_ENDBR)
>> > +	case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
>> > +		return kvm_set_msr_common(vcpu, msr_info);
>> this hunk can be dropped as well.
>
>In patch 16, these lines still need to be added back for PL{0,1,2}_SSP, so
>would like keep it

If that's the case, better to move it to patch 16, where the change
can be justified. And PL3_SSP should be removed anyway. and then
"msr_index != MSR_IA32_PL3_SSP" check in the below code snippet in
patch 16 can go away.

+		/*
+		 * Write to the base SSP MSRs should happen ahead of toggling
+		 * of IA32_S_CET.SH_STK_EN bit.
+		 */
+		if (msr_index != MSR_IA32_PL3_SSP && data) {
+			vmx_disable_write_intercept_sss_msr(vcpu);
+			wrmsrl(msr_index, data);
+		}


>
>here.
>
>> 
>> > +		break;
>> > +	case MSR_IA32_U_CET:
>> > +	case MSR_IA32_S_CET:
>> > +	case MSR_KVM_GUEST_SSP:
>> > +	case MSR_IA32_INT_SSP_TAB:
>> > +		if ((msr_index == MSR_IA32_U_CET ||
>> > +		     msr_index == MSR_IA32_S_CET) &&
>> > +		    ((data & ~VMX_CET_CONTROL_MASK) ||
>> > +		     !IS_ALIGNED(CET_LEG_BITMAP_BASE(data), 4) ||
>> > +		     (data & CET_EXCLUSIVE_BITS) == CET_EXCLUSIVE_BITS))
>> > +			return 1;
>> how about
>> 
>> 	case MSR_IA32_U_CET:
>> 	case MSR_IA32_S_CET:
>> 		if ((data & ~VMX_CET_CONTROL_MASK) || ...
>> 			...
>> 
>> 	case MSR_KVM_GUEST_SSP:
>> 	case MSR_IA32_INT_SSP_TAB:
>
>Do you mean to use "fallthrough"?

Yes.