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, 24 May 2023 16:32:36 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     Xiaoyao Li <xiaoyao.li@intel.com>
CC:     <kvm@vger.kernel.org>, <x86@kernel.org>,
        Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] KVM: x86: Track supported ARCH_CAPABILITIES in
 kvm_caps
Message-ID: <ZG3LpLR7itO4dI8U@chao-email>
References: <20230524061634.54141-1-chao.gao@intel.com>
 <20230524061634.54141-2-chao.gao@intel.com>
 <fc72da6d-2f70-63c8-dd6b-f8f8df862b89@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <fc72da6d-2f70-63c8-dd6b-f8f8df862b89@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?idDkYBwnkvqi9HMosIqvClEH1wOSUFEbA7OswAKFtVDBgdrqlLErC2Qv17o7?=
 =?us-ascii?Q?RYbzqvfKV2YYmVSolGU/a66mgkS1SfgKAEolJAXuHtVd3JhNHoVQY6MgzQLq?=
 =?us-ascii?Q?1NnoVN9Xoz1WaFfqkKcFx4GyrQxWKUPPzHpSGi4W6DWmYMegHHBUu87iQ/Oi?=
 =?us-ascii?Q?9FnEXLXyOK5PXrTPhbry/M3gn62MotKXjc3apMYY1vTkmHSHUFvDXl61UzxN?=
 =?us-ascii?Q?rgLd3jk+imexQmk1VHQHEj8VuNk8qAH55qQpSVuY2KSxz6JZNWFXbXx8Pz+u?=
 =?us-ascii?Q?bGFmX6+SxGzEegTqCMdVvuTcUMWbXhJowYU9JjtqhgzCB6NkTM9t1SU3m/ht?=
 =?us-ascii?Q?6dIM6wPGnUywQ7R4SUhnoatqmkdLtu+nFOEmwmmTtuqzkb7uUK3IgMHDq8Wf?=
 =?us-ascii?Q?EGWq43ZG3nQfREnCkjOamHCHKnIkhwSQUvb7yKZQR804jcKStJCj39CWbgzh?=
 =?us-ascii?Q?P7cKMR+LR92QJ2EykaQg59YEcL7X6WGp7gkU7Cclp5AKqm/dNaJH7H4aog2y?=
 =?us-ascii?Q?h45L/qn/yzGDm57BzWDJ/N9IrhTyYLb6xKlqNZRQ0XlzT6CoFxc1uL0YPHTO?=
 =?us-ascii?Q?HNO7pQqMcPzs++rQ6P229aRpzaq9mFiU7sfyPNF6FNjEPzZP1AmsAkq9NoxK?=
 =?us-ascii?Q?0LyxMRj+QfL9io/skSK9gj6TFPDWZSiEiOljUlNa9mQ1QZJtXNBXxS2y3EH5?=
 =?us-ascii?Q?qq8B+SCIYFTb7kDInyvc2r9F3nOQfIWGM/nH7jcrwEPGMLnPvw8KoZmVdLAU?=
 =?us-ascii?Q?t1rBCXHuNLxHKr9w+VJEVHZAAnLprgwvGjppu0hBZk46zcNFIUR2N/wdVqa0?=
 =?us-ascii?Q?vce3GaFH4b2vH/PS9P9aYNLXu+QEdFLSboB+ift1wg+CPFbiavAgClmBF/1c?=
 =?us-ascii?Q?dRbf2Mda2qVhzM3qLJ7MzNjmdOyoT/YBvoewLXk9nOFVs7+O8A4FHYTNrrHv?=
 =?us-ascii?Q?jSj8VCCVALYXPQcUKx6b/nqadF1jYjeybu8C0UWeYXkFLXbiIO/sH7UwdAQ9?=
 =?us-ascii?Q?e3sfbUcEhmy7rYKxJzAoOHLBe1DeB0KbT/BtA+q/oUbzD+GzXBS8O+gsfPyy?=
 =?us-ascii?Q?gcizhU4Hgt8WeH2dZocv0x1IP3ffOvPTw2EJKvdzYF6GsunwNSbyHWpcq92G?=
 =?us-ascii?Q?Wf4/BqYbbAwMsDVaT2WMSbIAn6IW+yJMLOyQhwLd6APu9xgxw1BlIltBDdeM?=
 =?us-ascii?Q?CoTj+W8ZFnibHPazzrURl0EnPTJuDd0oVsns8kMSGv5JnBEjaw45eMsdk1BR?=
 =?us-ascii?Q?+GD3OLRe5bFaFOrOj11oDKART31HqCKyZxggR9ZPkRw03dONWFflIVZcJ/Gx?=
 =?us-ascii?Q?sb9bQd9y1CjIyWLMeMYc/ovDUlrV7hvckgXGW3YhjjViClDknuNCm11seiC7?=
 =?us-ascii?Q?ySGUhSQZyvnx7Lg62CqCEkn/CrrEvSsSqn3/06nx8sxpmwq6i1W+KLo32N9g?=
 =?us-ascii?Q?TvdNvQiRGrvkVLm7Rpko2ECrXM8jD8k9Vn7nP+20onoyw24O1pBl+AwDOlC9?=
 =?us-ascii?Q?2VeaNI+l3OD/aos+ImRVZXAcIpw7IvvjY2L70vcYft55DF+pnnyoOzrKc3fq?=
 =?us-ascii?Q?6NAxlScsUT4BdTEPgHjYtg+m+8/dZqGyrztmDTCB?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a69258c7-dad2-47eb-14bb-08db5c3173c5
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB6780.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 May 2023 08:32:47.1669
 (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: 91DvpMC28/h/vGwKqQIcYAjJ7MxrqVg4MzgJaFRBx0Mjoojb2i3ogc99UJmCucMl6JKHk/r3/wY5LgCHn80xfQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4993
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,
        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 Wed, May 24, 2023 at 04:14:10PM +0800, Xiaoyao Li wrote:
>On 5/24/2023 2:16 PM, Chao Gao wrote:
>> to avoid computing the supported value at runtime every time.
>> 
>> Toggle the ARCH_CAP_SKIP_VMENTRY_L1DFLUSH bit when l1tf_vmx_mitigation
>> is modified to achieve the same result as runtime computing.
>
>It's not the same result.

it is because ...

>
>In kvm_get_arch_capabilities(), host's value is honored. I.e., when host
>supports ARCH_CAP_SKIP_VMENTRY_L1DFLUSH, l1tf_vmx_mitigation doesn't make any
>difference to the result.

... l1tf_vmx_mitigation should be VMENTER_L1D_FLUSH_NOT_REQUIRED in this
case. l1tf_vmx_mitigation cannot be VMENTER_L1D_FLUSH_NEVER.

>
>> Opportunistically, add a comment to document the problem of allowing
>> changing the supported value of ARCH_CAPABILITIES and the reason why
>> we don't fix it.
>> 
>> No functional change intended.
>> 
>> Link: https://lore.kernel.org/all/ZGZhW%2Fx5OWPmx1qD@google.com/
>> Link: https://lore.kernel.org/all/ZGeU9sYTPxqNGSqI@google.com/
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 25 +++++++++++++++++++++++--
>>   arch/x86/kvm/x86.c     |  7 ++++---
>>   arch/x86/kvm/x86.h     |  1 +
>>   3 files changed, 28 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 44fb619803b8..8274ef5e89e5 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -309,10 +309,31 @@ static int vmx_setup_l1d_flush(enum vmx_l1d_flush_state l1tf)
>>   	l1tf_vmx_mitigation = l1tf;
>> -	if (l1tf != VMENTER_L1D_FLUSH_NEVER)
>> +	/*
>> +	 * Update static keys and supported arch capabilities according to
>> +	 * the new mitigation state.
>> +	 *
>> +	 * ARCH_CAP_SKIP_VMENTRY_L1DFLUSH is toggled because if we do cache
>> +	 * flushes for L1 guests on (nested) vmlaunch/vmresume to L2, L1
>> +	 * guests can skip the flush and if we don't, then L1 guests need
>> +	 * to do a flush.
>> +	 *
>> +	 * Toggling ARCH_CAP_SKIP_VMENTRY_L1DFLUSH may present inconsistent
>> +	 * model to the guest, e.g., if userspace isn't careful, a VM can
>> +	 * have vCPUs with different values for ARCH_CAPABILITIES. But
>> +	 * there is almost no chance to fix the issue. Because, to present
>> +	 * a consistent model, KVM essentially needs to disallow changing
>> +	 * the module param after VMs/vCPUs have been created, but that
>> +	 * would prevent userspace from toggling the param while VMs are
>> +	 * running, e.g., in response to a new vulnerability.
>> +	 */
>> +	if (l1tf != VMENTER_L1D_FLUSH_NEVER) {
>>   		static_branch_enable(&vmx_l1d_should_flush);
>> -	else
>> +		kvm_caps.supported_arch_cap |= ARCH_CAP_SKIP_VMENTRY_L1DFLUSH;
>> +	} else {
>>   		static_branch_disable(&vmx_l1d_should_flush);
>> +		kvm_caps.supported_arch_cap &= ~ARCH_CAP_SKIP_VMENTRY_L1DFLUSH;
>> +	}
>>   	if (l1tf == VMENTER_L1D_FLUSH_COND)
>>   		static_branch_enable(&vmx_l1d_flush_cond);
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index c0778ca39650..2408b5f554b7 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1672,7 +1672,7 @@ static int kvm_get_msr_feature(struct kvm_msr_entry *msr)
>>   {
>>   	switch (msr->index) {
>>   	case MSR_IA32_ARCH_CAPABILITIES:
>> -		msr->data = kvm_get_arch_capabilities();
>> +		msr->data = kvm_caps.supported_arch_cap;
>>   		break;
>>   	case MSR_IA32_PERF_CAPABILITIES:
>>   		msr->data = kvm_caps.supported_perf_cap;
>> @@ -7156,7 +7156,7 @@ static void kvm_probe_msr_to_save(u32 msr_index)
>>   			return;
>>   		break;
>>   	case MSR_IA32_TSX_CTRL:
>> -		if (!(kvm_get_arch_capabilities() & ARCH_CAP_TSX_CTRL_MSR))
>> +		if (!(kvm_caps.supported_arch_cap & ARCH_CAP_TSX_CTRL_MSR))
>>   			return;
>>   		break;
>>   	default:
>> @@ -9532,6 +9532,7 @@ static int __kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
>>   		kvm_caps.max_guest_tsc_khz = max;
>>   	}
>>   	kvm_caps.default_tsc_scaling_ratio = 1ULL << kvm_caps.tsc_scaling_ratio_frac_bits;
>> +	kvm_caps.supported_arch_cap = kvm_get_arch_capabilities();
>>   	kvm_init_msr_lists();
>>   	return 0;
>> @@ -11895,7 +11896,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>   	if (r)
>>   		goto free_guest_fpu;
>> -	vcpu->arch.arch_capabilities = kvm_get_arch_capabilities();
>> +	vcpu->arch.arch_capabilities = kvm_caps.supported_arch_cap;
>>   	vcpu->arch.msr_platform_info = MSR_PLATFORM_INFO_CPUID_FAULT;
>>   	kvm_xen_init_vcpu(vcpu);
>>   	kvm_vcpu_mtrr_init(vcpu);
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index c544602d07a3..d3e524bcc169 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -29,6 +29,7 @@ struct kvm_caps {
>>   	u64 supported_xcr0;
>>   	u64 supported_xss;
>>   	u64 supported_perf_cap;
>> +	u64 supported_arch_cap;
>>   };
>>   void kvm_spurious_fault(void);
>