Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <10fd9a3e-1bc2-7d4d-0535-162854fc5e9d@intel.com>
Date:   Fri, 3 Nov 2023 16:46:29 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v6 14/25] KVM: x86: Load guest FPU state when access
 XSAVE-managed MSRs
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>,
        Maxim Levitsky <mlevitsk@redhat.com>
CC:     <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <dave.hansen@intel.com>,
        <peterz@infradead.org>, <chao.gao@intel.com>,
        <rick.p.edgecombe@intel.com>, <john.allen@amd.com>
References: <20230914063325.85503-1-weijiang.yang@intel.com>
 <20230914063325.85503-15-weijiang.yang@intel.com>
 <2b1973ee44498039c97d4d11de3a93b0f3b1d2d8.camel@redhat.com>
 <ZUKTd_a00fs1nyyk@google.com>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZUKTd_a00fs1nyyk@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TDE4VnBnOEhCVlJTaEQ4cUV6Q3A2cHY2VEVjTFI0ZnB0SDRoV2tOc3lkYll3?=
 =?utf-8?B?dm9XWW02dFNiakZrMUJDRkNtRDZJOEhZVFNYNDFvMFNJM2I4aWczUzJMSnJT?=
 =?utf-8?B?a2kzekZ3eE01Qk1CdjNQcmlQeWRpZnB5QWFwVzJHTGFEek5BRjBFTTdNZzR5?=
 =?utf-8?B?VDVYV05YK0tIK0FQNkkvTytMT0RIWitHaldPNTRIRk9mRW5ucEZCSTlJQk1k?=
 =?utf-8?B?Y21jMThhbzBFVVl2aVcrcys3RGc2ckd2TjVjVEJBbXpmOEhqQm9ZMkF5SXJw?=
 =?utf-8?B?ckJyTTUyT1FVZE40OGhQOCtOL2M5SmR1NS8raHV0bHh2Z3orSG1pZ3RyUFJ0?=
 =?utf-8?B?dnd1RWxrcGxQREtVb295S0dXV1J4U1hwLzVTQWNkOHppcEw2T3dGZFhuemgy?=
 =?utf-8?B?eXNYd2s1SlVFMnZCVlUzVUVnSExhVGdjamxEUVRvUnNYTHhLNEh3R0tialIw?=
 =?utf-8?B?THptYTFFN01tS091NVR2U3Y5R09ua1dGUkVhUWROZ2RpSG1pQm9Xbmx0T25k?=
 =?utf-8?B?QkFDSjdnazVsMHJINWIvMlgxWitSRFcwdC85cVcvNFZSdjcyMmpMbWNXck8x?=
 =?utf-8?B?Q3NvL3A4YnZNYjBCL09UcEFiUmtlTHBkZHpmVDVtaUpDNmJadzd3TU45UllB?=
 =?utf-8?B?MFFYa2JxaHc5Z2lNRjdVL1pCRWJJSVAzeDBacWtPYjcwOUZ0K0xhMVZxOUFo?=
 =?utf-8?B?Q2l3eEEyVjFjRDJwKzBxSWErUWlSMng3TFVkOURhT09aUG9WeGRENzViK1Y2?=
 =?utf-8?B?WXk0UkljRGdpc0VuOGFrQU9reFJrc0hvZ3k2NCtHWW4yY08rTHJwRjlacHI2?=
 =?utf-8?B?UXM5eVdzWkIyMTExTU9DSFIxV3VaVFhlVlErTVRDNG53QWRsRkRkWXYzdlpV?=
 =?utf-8?B?RkhTekx0TmZYeGxsOFhHT3Rway8rTGtSaHdXTS9WclhOcHJKL2o1cVkxVnU5?=
 =?utf-8?B?cllFcWFBWjNER214M1hVblpPbmFrTjBaMklYTklMcXh2Y1dqTis0SlJ6b2p3?=
 =?utf-8?B?aXZPbFNvRTZ1dXU5VnA5ZUpuRXlTdlY1T2Y0ZTUvdmpvaEE1am55Q0t3MW10?=
 =?utf-8?B?VFZzMzF3UU1mbXNQVmlzRXdZK0ljSWdCTlJ0blRRWFlPNVJxZ0IrNFk5anVG?=
 =?utf-8?B?SUoxMXgxZXhkSVpVQnhzOStzUXU4SW1mNS9raXhBd2ZNcmVBVWp2UEJOa2xG?=
 =?utf-8?B?UGdOVVBVSXI0TVhoZTU1VW5lSmdLejQ3U3ZkRE9GblpUakdTK0tzMkl6UmRt?=
 =?utf-8?B?U0lVaUt3T2V0eXBwWFhxUmpQZU9iRldsd0VWaEE4R2VsYzkyeitJN1BiS3RP?=
 =?utf-8?B?ZDNIYWVMcmNmdjYxOVlXT08yQnQzVENLYmR6NzRvL2MxZDNiNnNITHJDcU9K?=
 =?utf-8?B?UFppLzJablBLZEs5T29yM0Rxb0FHUS9BaE5qdEpJZXdZVHZzcWZxdXptRnlC?=
 =?utf-8?B?MVhQamhXVjNIWmxoWTQyT1VSdm1pb1d4YXVtK1h3OHoyemExaTl5SUJGRU5n?=
 =?utf-8?B?ZzBGR2EwR2ZMd3VJa3FHc1BkbXJXdTZ4bHNGcG91YlRDc0x0akdzbkUzSVZr?=
 =?utf-8?B?VzNlcDhUVFBrQ3M4TmZqYmJqR1dvWmwraUo5a0F5b3ZvWnh4L2laRkR3Mm52?=
 =?utf-8?B?Q2dBVndLc2pJQUtPRVNKVVhleXcwT0dUV01NOVVUK1BseGRLdUJvb0UvYzJj?=
 =?utf-8?B?QzVjTlNGWldVbUNlUHhlTWxrdzFiTHYrU05pUFI3YzhjZlRlT2ZtZFYwVzZ2?=
 =?utf-8?B?K21hcmNMOCtTN0VBbXFLZnFDcHQ1a3ljN2tPWHl1ckhoQ2hyYzloSXJ5enZK?=
 =?utf-8?B?TkJvc1Zob0RwQ1JVc2ttQnVCZ3VubzU0cTYrSTN4NERkWjJSck9MWWMrV1Jq?=
 =?utf-8?B?UXB5K3B4YXpTeXkvdVFhck8zV2xsRE82bmd0SllabTEvU0YrTnhoYmIwRWNh?=
 =?utf-8?B?c0hwbHpSZkdVLzVyS2V4QTdDNTRFdGt0WmkvWFFzTEZrM2Rja25kdWhUQVRr?=
 =?utf-8?B?M1V6d1BmN3VYUGkwL21uOGNKMWo0MGVzR3V3dTBxV3hDTTAxSC84SnA1L04r?=
 =?utf-8?B?b1dUSDVJQi9IeUc1bGxsNTNlbjB5U05KMysrWmJXMjlOUU5XQjIwVzZEOGJ3?=
 =?utf-8?B?U2NzMzZsSmxmT2FNQk5QVnRkZXRaaVZLbzVPK1c0VmNSUEwwUWtVemtjTW9S?=
 =?utf-8?B?aEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a89d51b-bd11-4d70-f563-08dbdc49643c
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Nov 2023 08:46:37.7365
 (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: yFhx4v82eH66vjkbJS8eFUR5srJFUaQLfW2sEMwRySKBhFoblPhCJpr26//aRoFzRo5EMNfSBjt2p0Slai+CIw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6505
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.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_BLOCKED,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
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Fri, 03 Nov 2023 01:46:58 -0700 (PDT)

On 11/2/2023 2:05 AM, Sean Christopherson wrote:
> On Tue, Oct 31, 2023, Maxim Levitsky wrote:
>> On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 66edbed25db8..a091764bf1d2 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -133,6 +133,9 @@ static int __set_sregs2(struct kvm_vcpu *vcpu, struct kvm_sregs2 *sregs2);
>>>   static void __get_sregs2(struct kvm_vcpu *vcpu, struct kvm_sregs2 *sregs2);
>>>   
>>>   static DEFINE_MUTEX(vendor_module_lock);
>>> +static void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
>>> +static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
>>> +
>>>   struct kvm_x86_ops kvm_x86_ops __read_mostly;
>>>   
>>>   #define KVM_X86_OP(func)					     \
>>> @@ -4372,6 +4375,22 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>   }
>>>   EXPORT_SYMBOL_GPL(kvm_get_msr_common);
>>>   
>>> +static const u32 xstate_msrs[] = {
>>> +	MSR_IA32_U_CET, MSR_IA32_PL0_SSP, MSR_IA32_PL1_SSP,
>>> +	MSR_IA32_PL2_SSP, MSR_IA32_PL3_SSP,
>>> +};
>>> +
>>> +static bool is_xstate_msr(u32 index)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(xstate_msrs); i++) {
>>> +		if (index == xstate_msrs[i])
>>> +			return true;
>>> +	}
>>> +	return false;
>>> +}
>> The name 'xstate_msr' IMHO is not clear.
>>
>> How about naming it 'guest_fpu_state_msrs', together with adding a comment like that:
> Maybe xstate_managed_msrs?  I'd prefer not to include "guest" because the behavior
> is more a property of the architecture and/or the host kernel.  I understand where
> you're coming from, but it's the MSR *values* are part of guest state, whereas the
> check is a query on how KVM manages the MSR value, if that makes sense.
>
> And I really don't like "FPU".  I get why the the kernel uses the "FPU" terminology,
> but for this check in particular I want to tie the behavior back to the architecture,
> i.e. provide the hint that the reason why these MSRs are special is because Intel
> defined them to be context switched via XSTATE.
>
> Actually, this is unnecesary bikeshedding to some extent, using an array is silly.
> It's easier and likely far more performant (not that that matters in this case)
> to use a switch statement.
>
> Is this better?

The change looks good to me! Thanks!

> /*
>   * Returns true if the MSR in question is managed via XSTATE, i.e. is context
>   * switched with the rest of guest FPU state.
>   */
> static bool is_xstate_managed_msr(u32 index)

How about is_xfeature_msr()? xfeature is XSAVE-Supported-Feature, just to align with SDM
convention.

> {
> 	switch (index) {
> 	case MSR_IA32_U_CET:
> 	case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
> 		return true;
> 	default:
> 		return false;
> 	}
> }
>
> /*
>   * Read or write a bunch of msrs. All parameters are kernel addresses.
>   *
>   * @return number of msrs set successfully.
>   */
> static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,
> 		    struct kvm_msr_entry *entries,
> 		    int (*do_msr)(struct kvm_vcpu *vcpu,
> 				  unsigned index, u64 *data))
> {
> 	bool fpu_loaded = false;
> 	int i;
>
> 	for (i = 0; i < msrs->nmsrs; ++i) {
> 		/*
> 	 	 * If userspace is accessing one or more XSTATE-managed MSRs,
> 		 * temporarily load the guest's FPU state so that the guest's
> 		 * MSR value(s) is resident in hardware, i.e. so that KVM can
> 		 * get/set the MSR via RDMSR/WRMSR.
> 	 	 */
> 		if (vcpu && !fpu_loaded && kvm_caps.supported_xss &&
> 		    is_xstate_managed_msr(entries[i].index)) {
> 			kvm_load_guest_fpu(vcpu);
> 			fpu_loaded = true;
> 		}
> 		if (do_msr(vcpu, entries[i].index, &entries[i].data))
> 			break;
> 	}
> 	if (fpu_loaded)
> 		kvm_put_guest_fpu(vcpu);
>
> 	return i;
> }