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: <10511c2d-7b64-374d-cd91-ddac2c1a788e@intel.com>
Date:   Fri, 28 Jul 2023 08:43:10 +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 13/20] KVM:VMX: Emulate read and write to CET MSRs
To:     Sean Christopherson <seanjc@google.com>
CC:     Chao Gao <chao.gao@intel.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-14-weijiang.yang@intel.com>
 <ZMDT/r4sEfMj5Bmu@chao-email>
 <3d5fdd07-563c-6841-a867-88369c4dbb36@intel.com>
 <ZMH9tIXfPk0dl7ye@chao-email>
 <2801b9d6-4f32-c4b2-ae93-c56ffc2b4621@intel.com>
 <ZMKLRnjCTwqTr/MF@google.com>
Content-Language: en-US
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZMKLRnjCTwqTr/MF@google.com>
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?T1p3RnNGa1cwbnJXS3BCUnJQRm5xdHlnU0JCN2E2NEVadXRKQlJCNXR4bTZG?=
 =?utf-8?B?MXRGUVJrcjYwdzVuV09rMmlmRkNrMjdmOGpKTnlNV3JONG8vd25IWExVaTIv?=
 =?utf-8?B?enYvRTJPOE9rZytTM0ovRTJIOC9xRVorRC9PK1Bta0NNcFJzc1ZFWHdCa0xR?=
 =?utf-8?B?WFcraFBwUWdXU0lmOThlNVhlODdkN1d5VWxKOHNvd0hJZzBEaVRySkpFK3pV?=
 =?utf-8?B?YkdrRThyeUlJNlNCVjc2U2kvc2c3YWduL0NXSzhiSTJuanh2TkdLUzZZU0JR?=
 =?utf-8?B?ajVBbUMrcVpaSmVsMVVBMU84eFlyRWxvSlZ3TzB6QXd5QVd5ZGdtYVdnVnkr?=
 =?utf-8?B?M0FQbll1dkxkVWJHL1l5aWcxN1E4OWRqYzdrZmF1UFR2V2xaNDBkeHV1R1Nj?=
 =?utf-8?B?N3QwT1gwQzdrVVRvK0dCS1RJeWUycTU4NlB1aDVaeHg3RGo0dExPZ1VWM09X?=
 =?utf-8?B?cTZmY1pQSkhyWGFvV2ovMWMxUjRHWmNXaXFzTlJ2NTlQeWZJUU8wdzdOeWYz?=
 =?utf-8?B?VXdpbG1MeU4wT0xGc0NIRklBYWU2eDQvKzhuUVArdUFMY3VwMlNPNHl5aEw3?=
 =?utf-8?B?eHBhNnVXZVBZSFlOOG8zQ3dHd0ZSNlRlbzNmYm03RGRwM3FFN1Qwc1BpQnRm?=
 =?utf-8?B?SDdVVWdrYnBhSUVZOE1yRm1MRFdRQTVGeWdvdnh3Q3RrT0E0RXZwcW9hSHli?=
 =?utf-8?B?Z3BOcWtuNHdVZmdnOHQrem1DM0J1R1N0U2xwWkNRQk1vNFo0WVMwODlyR1dE?=
 =?utf-8?B?U3IxVDg5TElmTk9EeE85Y1VuRHptcXJpOFl6OXpHOFFITkNHNGluL1Bjc21S?=
 =?utf-8?B?K2hHVmlsOTF3OWVVWWhyMmZpamtNWWMwRzBUeVBQVkF2TjAxUnhIdkp4VEpl?=
 =?utf-8?B?ZkNPSHFOYk84WTlFckE4bHE1b002Q0lNdDRmRkJaWEhCQytqMWI1OHBHdmhm?=
 =?utf-8?B?Q1NUQnBva0creG11Z1V1SUU1bGJ5a3JXblFtMDNiYkExcWFhVDdNNXVDZmR3?=
 =?utf-8?B?VFhHUUhkcmpKQzVGY3ZnT2QvOHg5NFRVdE90K1FTMlk5Rm9rNDV2VnY5ZjhZ?=
 =?utf-8?B?YmdqQVJrWFpVaUlFRmVCakVDZGJpb3loWVFuR1paRC8rNkRKbVB6THlwcngz?=
 =?utf-8?B?Y3Vad1hpVTVDZmxRMnk5QjdVN1V0QitFeTV3ME15dVZIclFFeU5kbHJMYlMy?=
 =?utf-8?B?NmNWb08yS0ExQkF4Z0lKcnZoMjRtRG9oYmN3d0ljWG0rTm1OZllJc1ovQUF3?=
 =?utf-8?B?SEdUZVF3T1NZZTVZR09nNDFNVTROYXNkd0M3R2FyNjdsOENZN3BIQktnZVNR?=
 =?utf-8?B?Y2VOTmJVT1htV2NlOEMwOGozQjR0ZmZKYTBqRUZvb0hXRXBJVGNvS3pmQy9M?=
 =?utf-8?B?eEZYYThRZDlKVkg1VjRCR25YN2NDcUZTTTQ1a2Izb2Z3M2FxMVkwV3BLcmRZ?=
 =?utf-8?B?b0xpaXFXQU9JQlFKTXZMMXJyMFoyem5ic3Qwc1o4UDkyTFJ0dGxMTzluQlEv?=
 =?utf-8?B?bkM2OUQ5emEyVitGTUNJWElYU04rQmxOd1NQUm9KU3lERjE1V1NxbGdqSC8y?=
 =?utf-8?B?dDBYcm5HOUlGK2pwTFZrUyswNEV3aE9ROGxmdnMwbURBZDNwME9ZcUZTQUYx?=
 =?utf-8?B?OHFjelhwMmNQZlV2TVM5bWx4c1JRaHE0Wi9kQmw2UjhWOVpzSFZYY0xZOFZr?=
 =?utf-8?B?RTFGUm1iVFR2RlNWeEovOUtWY2tBQzQ1cGlRRGU4aEJBZG5tajlwa2ppOWVq?=
 =?utf-8?B?Z1FQejRaZGFPeUlCUWpJRFBYeG9XaWY4VHNUV0ZKUnRydXl6WFVSWVc1ZEg5?=
 =?utf-8?B?M3pRRmNabStEeFVEb3V1R3IxUGdydkdRSEUzT1FpK1ZLUDB5VmF4TVl3S3RJ?=
 =?utf-8?B?Q0lMTjJrTGFBZEdEY2N6WTV6a3JkUTRKRzJhT2xxV2h2OGpRbHd4bGRZVSts?=
 =?utf-8?B?VWFibnJtUlkwaGZXLzl6ZEYwUW5xWlI4VU5oMVdsSC9jK0x3RGZrYWRCVGs4?=
 =?utf-8?B?dTBUcHpDMzBiMHFVazNtTWJaZ0drQndySnpJLzdDTkMzd1ZydnpoTjBqbkx2?=
 =?utf-8?B?M3BpZ29wZjU5S05naDhPZ1ZWVWZWajJSR2Y1VXpVNUtrVmwweC9hRzIvZU9T?=
 =?utf-8?B?SjlTV2dFdGszNDA2SWFjbXZBcG80d2ZFeEU3eDNLOVZqL2N2YlQ2d2JHVzI4?=
 =?utf-8?B?S2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dabe293d-d491-406b-704f-08db8f03a4d9
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jul 2023 00:43:21.9965
 (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: mxGMMrYWVBm3UudLNCsEuREYKWxoZlLTvqEQOSiYXiCg1n2ZkqS1ca9RJ3emQbVcKzoxATAtHXQutnkCavvdhw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7468
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,
        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 7/27/2023 11:20 PM, Sean Christopherson wrote:
> On Thu, Jul 27, 2023, Weijiang Yang wrote:
>> On 7/27/2023 1:16 PM, Chao Gao wrote:
>>>>>> @@ -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.
> +1
>
>> So you want the code to be:
>>
>> +#define CET_IBT_MASK_BITS          (GENMASK_ULL(5, 2) | GENMASK_ULL(63,
>> 10))
>>
>> +#define CET_CTRL_RESERVED_BITS GENMASK(9, 6)
>>
>> +#define CET_SHSTK_MASK_BITSGENMASK(1, 0)
>>
>> +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)) ||
>>
>>                              (data & CET_CTRL_RESERVED_BITS) )
>>
>> ^^^^^^^^^^^^^^^^^^^^^^^^^
> Yes, though I vote to separate each check, e.g.
>
> 	if (data & CET_CTRL_RESERVED_BITS)
> 		return 1;
>
> 	if (!guest_can_use(vcpu, X86_FEATURE_SHSTK) && (data & CET_SHSTK_MASK_BITS))
> 		return 1;
>
> 	if (!guest_can_use(vcpu, X86_FEATURE_IBT) && (data & CET_IBT_MASK_BITS))
> 		return 1;
>
> I would expect the code generation to be similar, if not outright identical, and
> IMO it's easier to quickly understand the flow if each check is a separate if-statement.

It looks good to me! Thank you!