Message ID | 20230217231022.816138-2-seanjc@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp114553wrn; Fri, 17 Feb 2023 15:12:24 -0800 (PST) X-Google-Smtp-Source: AK7set9wNmr/GeZkKzViBMz2XLFHDg3qD3F7hitd59Yc0ZM2vwXEJJMEiTuo/DcfFM/olQOoszWk X-Received: by 2002:a17:906:b20d:b0:8b1:62d2:dffc with SMTP id p13-20020a170906b20d00b008b162d2dffcmr7048956ejz.24.1676675543867; Fri, 17 Feb 2023 15:12:23 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1676675543; cv=none; d=google.com; s=arc-20160816; b=eJXZoBUFE6yyC5gJ55+D49tacCE/cszK1On/xIO0VetsHWiN568/w/wfZNNIepAqxB UuG8oLJsEDjVkvEcDynqo4gKM0thx4lI2k2KHreDuKnfUv0oGq6amaMAnUPlYUditPjZ rtN1UtqXF1+zWq1dYtnC+saFdIPVvvbDme9OK+5PMV7WYo4O3pit/BPLBpt495yeB+JV kpdNiz0kh92u88uIswmEd+38q1IhUT7OwGaG5nf/+XlU6N3YifmbisXvpxk7/T8kC8Pj T5cUgkRxkKrHjSuIYXloDwSNiswDIjggIHgloYuZWH9rA9SGTOs0wlbQStcQyLro8V5r elAA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:reply-to:dkim-signature; bh=KioFTda0zEdQSBw0TQsNW7u7HebdEbC7ULL51zMSfbE=; b=WIc3AYlYFP/Iy7ge2KBJSkhuSRasPP1rEc2qDyDSHY/G8C8a/F6VpEZEJPL2QKzU1g b/ZfonlJ/sM+mqY+nvBo1dGhcgiUCRnGYKpcUm0znWJr4Km4eXMyZRZp8pFPbl01fQZr AB+pXyINu5ITK2o8tJYD1dq3I4CRoPFRO5tGiUqyBpBe43+/VIlh/ZVzntlTW6jr3zbJ oKn+0gf/z/bmyCEzluvXTpk02SUpI3yYsC/PZVKYSBzk+FVr3C4M+pTA8MPLyLD1LZ4R h6EAi2thuMYHfb09/v8y2F46aEDnLbneKwIx893BLS0aiNFlZkfaLblKgZBRZCR3tJ/o vlgg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=MCT5iiV4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bo22-20020a0564020b3600b004acbd72dec2si6248019edb.47.2023.02.17.15.12.00; Fri, 17 Feb 2023 15:12:23 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=MCT5iiV4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229928AbjBQXKj (ORCPT <rfc822;daweilics@gmail.com> + 99 others); Fri, 17 Feb 2023 18:10:39 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56250 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229826AbjBQXKb (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 17 Feb 2023 18:10:31 -0500 Received: from mail-yw1-x1149.google.com (mail-yw1-x1149.google.com [IPv6:2607:f8b0:4864:20::1149]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 103E8582BF for <linux-kernel@vger.kernel.org>; Fri, 17 Feb 2023 15:10:29 -0800 (PST) Received: by mail-yw1-x1149.google.com with SMTP id 00721157ae682-5365a8dd33aso17105927b3.22 for <linux-kernel@vger.kernel.org>; Fri, 17 Feb 2023 15:10:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:from:to:cc:subject:date:message-id:reply-to; bh=KioFTda0zEdQSBw0TQsNW7u7HebdEbC7ULL51zMSfbE=; b=MCT5iiV4rc2yE80RyBM9o4KOTMsZUh2bAIbwptEF0tPQ9AAIaLqeXfPV1v20xeQdBJ ngvY5MNXm07+nxgNuRVOw426oKdsi4ub/BMFNHHzgpk+EOmpzHjGpfwdwZBvTVIsXJ1Z ixYtudyqlOIGwVMjR24PG6YIbvmFhc89VzwHoRedtJxRbKHjBrbkpRxTQeXTDHZ53RLv v75nUo1kDzVi4ZVoxG0a8eb1dmu/6aE1NqaDLwhmqbHynV8Y12e7IUCx+nsNis1FHY/k nW0CsLs/BM7+/PNt0iB7yoNFIxk5lLvX8E9NBgK01X2cqZqg5s3MH84roh/Ua50ptKae XOmA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=KioFTda0zEdQSBw0TQsNW7u7HebdEbC7ULL51zMSfbE=; b=4m6DcBcaKVUIdQ8tBovJFQ4XWykMZrU3lP4OBSq9Z5IZlW9ehPsGZuY7B9vdNMjxjr UMXNr04NA8RLm9ZH+2Ups4hqsKh9nAqYpsER+Ob1h/agyU9p8k4TSr/VQ9sD9lCkzPUm mDkFuxGO4/GMp+lFnZqy4G95s/nhnbWH2PriR/fflx4zp1OM/h2QtA5ZLFfWFS6HI0Fq pQJGLGfNBCIXTDicUN/4M1O04IIqqES30uaggLsWEltg4TvlyGBGGXVssU+OLQnO9xHC qHoGLLZsBge/XlpVpi6gcyWS3U+7cFCnZ6YnnJdOe9VaFPs1BJid97/r4Tqm+wK9U26Y yaGw== X-Gm-Message-State: AO0yUKVoeMsY+O8sBR411CSuVpLdxRs9MG9bb7r49GTAukiYRLW4B2KX IHyVhz7eFp7VCiH5ZuIt9kpQgNK0WFg= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a25:9205:0:b0:8ef:90e1:b2f8 with SMTP id b5-20020a259205000000b008ef90e1b2f8mr206400ybo.2.1676675428316; Fri, 17 Feb 2023 15:10:28 -0800 (PST) Reply-To: Sean Christopherson <seanjc@google.com> Date: Fri, 17 Feb 2023 15:10:11 -0800 In-Reply-To: <20230217231022.816138-1-seanjc@google.com> Mime-Version: 1.0 References: <20230217231022.816138-1-seanjc@google.com> X-Mailer: git-send-email 2.39.2.637.g21b0678d19-goog Message-ID: <20230217231022.816138-2-seanjc@google.com> Subject: [PATCH 01/12] KVM: x86: Add a framework for enabling KVM-governed x86 features From: Sean Christopherson <seanjc@google.com> To: Sean Christopherson <seanjc@google.com>, Paolo Bonzini <pbonzini@redhat.com>, Vitaly Kuznetsov <vkuznets@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL 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-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1758121735162693928?= X-GMAIL-MSGID: =?utf-8?q?1758121735162693928?= |
Series |
KVM: x86: Add "governed" X86_FEATURE framework
|
|
Commit Message
Sean Christopherson
Feb. 17, 2023, 11:10 p.m. UTC
Introduce yet another X86_FEATURE flag framework to manage and cache KVM
governed features (for lack of a better term). "Governed" in this case
means that KVM has some level of involvement and/or vested interest in
whether or not an X86_FEATURE can be used by the guest. The intent of the
framework is twofold: to simplify caching of guest CPUID flags that KVM
needs to frequently query, and to add clarity to such caching, e.g. it
isn't immediately obvious that SVM's bundle of flags for "optional nested]
SVM features" track whether or not a flag is exposed to L1.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 11 +++++++
arch/x86/kvm/cpuid.c | 2 ++
arch/x86/kvm/cpuid.h | 51 ++++++++++++++++++++++++++++++++
arch/x86/kvm/governed_features.h | 9 ++++++
4 files changed, 73 insertions(+)
create mode 100644 arch/x86/kvm/governed_features.h
Comments
Sean Christopherson <seanjc@google.com> writes: > Introduce yet another X86_FEATURE flag framework to manage and cache KVM > governed features (for lack of a better term). "Governed" in this case > means that KVM has some level of involvement and/or vested interest in > whether or not an X86_FEATURE can be used by the guest. The intent of the > framework is twofold: to simplify caching of guest CPUID flags that KVM > needs to frequently query, and to add clarity to such caching, e.g. it > isn't immediately obvious that SVM's bundle of flags for "optional nested] Nit: unneeded ']' > SVM features" track whether or not a flag is exposed to L1. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 11 +++++++ > arch/x86/kvm/cpuid.c | 2 ++ > arch/x86/kvm/cpuid.h | 51 ++++++++++++++++++++++++++++++++ > arch/x86/kvm/governed_features.h | 9 ++++++ > 4 files changed, 73 insertions(+) > create mode 100644 arch/x86/kvm/governed_features.h > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 792a6037047a..cd660de02f7b 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { > struct kvm_cpuid_entry2 *cpuid_entries; > struct kvm_hypervisor_cpuid kvm_cpuid; > > + /* > + * Track whether or not the guest is allowed to use features that are > + * governed by KVM, where "governed" means KVM needs to manage state > + * and/or explicitly enable the feature in hardware. Typically, but > + * not always, governed features can be used by the guest if and only > + * if both KVM and userspace want to expose the feature to the guest. > + */ > + struct { > + u32 enabled; > + } governed_features; > + > u64 reserved_gpa_bits; > int maxphyaddr; > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 8f8edeaf8177..013fdc27fc8f 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -335,6 +335,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > struct kvm_lapic *apic = vcpu->arch.apic; > struct kvm_cpuid_entry2 *best; > > + vcpu->arch.governed_features.enabled = 0; > + > best = kvm_find_cpuid_entry(vcpu, 1); > if (best && apic) { > if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index b1658c0de847..f61a2106ba90 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -232,4 +232,55 @@ static __always_inline bool guest_pv_has(struct kvm_vcpu *vcpu, > return vcpu->arch.pv_cpuid.features & (1u << kvm_feature); > } > > +enum kvm_governed_features { > +#define KVM_GOVERNED_FEATURE(x) KVM_GOVERNED_##x, > +#include "governed_features.h" > + KVM_NR_GOVERNED_FEATURES > +}; > + > +static __always_inline int kvm_governed_feature_index(unsigned int x86_feature) > +{ > + switch (x86_feature) { > +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x; > +#include "governed_features.h" > + default: > + return -1; > + } > +} > + > +static __always_inline int kvm_is_governed_feature(unsigned int x86_feature) > +{ > + return kvm_governed_feature_index(x86_feature) >= 0; > +} > + > +static __always_inline u32 kvm_governed_feature_bit(unsigned int x86_feature) > +{ > + int index = kvm_governed_feature_index(x86_feature); > + > + BUILD_BUG_ON(index < 0); > + return BIT(index); > +} > + > +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > > + sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); > + > + vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); > +} > + > +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + if (guest_cpuid_has(vcpu, x86_feature)) > + kvm_governed_feature_set(vcpu, x86_feature); > +} > + > +static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); > +} > + > #endif > diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h > new file mode 100644 > index 000000000000..40ce8e6608cd > --- /dev/null > +++ b/arch/x86/kvm/governed_features.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE) > +BUILD_BUG() > +#endif > + > +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x) > + > +#undef KVM_GOVERNED_X86_FEATURE > +#undef KVM_GOVERNED_FEATURE
On 2/18/2023 7:10 AM, Sean Christopherson wrote: > Introduce yet another X86_FEATURE flag framework to manage and cache KVM > governed features (for lack of a better term). "Governed" in this case > means that KVM has some level of involvement and/or vested interest in > whether or not an X86_FEATURE can be used by the guest. The intent of the > framework is twofold: to simplify caching of guest CPUID flags that KVM > needs to frequently query, and to add clarity to such caching, e.g. it > isn't immediately obvious that SVM's bundle of flags for "optional nested] spare ] > SVM features" track whether or not a flag is exposed to L1. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 11 +++++++ > arch/x86/kvm/cpuid.c | 2 ++ > arch/x86/kvm/cpuid.h | 51 ++++++++++++++++++++++++++++++++ > arch/x86/kvm/governed_features.h | 9 ++++++ > 4 files changed, 73 insertions(+) > create mode 100644 arch/x86/kvm/governed_features.h > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 792a6037047a..cd660de02f7b 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { > struct kvm_cpuid_entry2 *cpuid_entries; > struct kvm_hypervisor_cpuid kvm_cpuid; > > + /* > + * Track whether or not the guest is allowed to use features that are > + * governed by KVM, where "governed" means KVM needs to manage state > + * and/or explicitly enable the feature in hardware. Typically, but > + * not always, governed features can be used by the guest if and only > + * if both KVM and userspace want to expose the feature to the guest. > + */ > + struct { > + u32 enabled; Although there are some guidances/preconditions of using the framework, is it possible that u32 will be ran out quickly after people starts to use the framework? Of course, I noticed there is build bug check on the length, it should be OK to increase the length when needed. > + } governed_features; > + > u64 reserved_gpa_bits; > int maxphyaddr; > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 8f8edeaf8177..013fdc27fc8f 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -335,6 +335,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > struct kvm_lapic *apic = vcpu->arch.apic; > struct kvm_cpuid_entry2 *best; > > + vcpu->arch.governed_features.enabled = 0; > + > best = kvm_find_cpuid_entry(vcpu, 1); > if (best && apic) { > if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index b1658c0de847..f61a2106ba90 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -232,4 +232,55 @@ static __always_inline bool guest_pv_has(struct kvm_vcpu *vcpu, > return vcpu->arch.pv_cpuid.features & (1u << kvm_feature); > } > > +enum kvm_governed_features { > +#define KVM_GOVERNED_FEATURE(x) KVM_GOVERNED_##x, > +#include "governed_features.h" > + KVM_NR_GOVERNED_FEATURES > +}; > + > +static __always_inline int kvm_governed_feature_index(unsigned int x86_feature) > +{ > + switch (x86_feature) { > +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x; > +#include "governed_features.h" > + default: > + return -1; > + } > +} > + > +static __always_inline int kvm_is_governed_feature(unsigned int x86_feature) Is it better to use bool instead of int? > +{ > + return kvm_governed_feature_index(x86_feature) >= 0; > +} > + > +static __always_inline u32 kvm_governed_feature_bit(unsigned int x86_feature) > +{ > + int index = kvm_governed_feature_index(x86_feature); > + > + BUILD_BUG_ON(index < 0); > + return BIT(index); > +} > + > +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > > + sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); > + > + vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); > +} > + > +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + if (guest_cpuid_has(vcpu, x86_feature)) > + kvm_governed_feature_set(vcpu, x86_feature); > +} > + > +static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, > + unsigned int x86_feature) > +{ > + return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); > +} > + > #endif > diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h > new file mode 100644 > index 000000000000..40ce8e6608cd > --- /dev/null > +++ b/arch/x86/kvm/governed_features.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE) > +BUILD_BUG() > +#endif > + > +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x) > + > +#undef KVM_GOVERNED_X86_FEATURE > +#undef KVM_GOVERNED_FEATURE
On Thu, Jun 29, 2023, Binbin Wu wrote: > > > On 2/18/2023 7:10 AM, Sean Christopherson wrote: > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 792a6037047a..cd660de02f7b 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { > > struct kvm_cpuid_entry2 *cpuid_entries; > > struct kvm_hypervisor_cpuid kvm_cpuid; > > + /* > > + * Track whether or not the guest is allowed to use features that are > > + * governed by KVM, where "governed" means KVM needs to manage state > > + * and/or explicitly enable the feature in hardware. Typically, but > > + * not always, governed features can be used by the guest if and only > > + * if both KVM and userspace want to expose the feature to the guest. > > + */ > > + struct { > > + u32 enabled; > Although there are some guidances/preconditions of using the framework, > is it possible that u32 will be ran out quickly after people starts to use > the framework? It's definitely possible. And there's no reason to limit this to a u32, I really have no idea why I did that. Ah, it's because "struct kvm_vcpu_arch" is defined in arch/x86/include/asm/kvm_host.h, and I didn't want to expose governed_features.h in arch/x86/include/asm. Hrm, that's really annoying. Aha! A better workaround for that conudrum would be to define an explicit "max" and use that, with a FIXME to call out that this really should use KVM_NR_GOVERNED_FEATURES directly. I have aspirations of moving kvm_host.h to arch/<arch>/kvm, at which point this can be cleaned up by declaring "enum kvm_governed_features" in kvm_host.h (though it'll likely be named something like kvm_arch.h at that point). /* * FIXME: Drop this macro and use KVM_NR_GOVERNED_FEATURES directly * when "struct kvm_vcpu_arch" is no longer defined in an * arch/x86/include/asm header. The max is mostly arbitrary, i.e. * can be increased as necessary. */ #define KVM_MAX_NR_GOVERNED_FEATURES BITS_PER_LONG /* * Track whether or not the guest is allowed to use features that are * governed by KVM, where "governed" means KVM needs to manage state * and/or explicitly enable the feature in hardware. Typically, but * not always, governed features can be used by the guest if and only * if both KVM and userspace want to expose the feature to the guest. */ struct { DECLARE_BITMAP(enabled, KVM_MAX_NR_GOVERNED_FEATURES); } governed_features; > Of course, I noticed there is build� bug check on the length, it should be > OK to increase the length when needed. > > +static __always_inline int kvm_governed_feature_index(unsigned int x86_feature) > > +{ > > + switch (x86_feature) { > > +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x; > > +#include "governed_features.h" > > + default: > > + return -1; > > + } > > +} > > + > > +static __always_inline int kvm_is_governed_feature(unsigned int x86_feature) > Is it better to use bool instead of int? Yes, this definitely should return a bool. Thanks!
On Fri, Feb 17, 2023 at 03:10:11PM -0800, Sean Christopherson wrote: >+static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, >+ unsigned int x86_feature) >+{ >+ BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > >+ sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); >+ >+ vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); >+} >+ >+static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, >+ unsigned int x86_feature) >+{ >+ if (guest_cpuid_has(vcpu, x86_feature)) Most callers in this series are conditional on either boot_cpu_has() or some local variables. Can we convert them to kvm_cpu_cap_has() and incorporate them within this function? i.e., if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature)) The benefits of doing so are 1. callers needn't repeat if (kvm_cpu_cap_has(x86_feature)) kvm_governed_feature_check_and_set(x86_feature) 2. this fits the idea better that guests can use a governed feature only if host supports it _and_ QEMU exposes it to the guest. >+ kvm_governed_feature_set(vcpu, x86_feature); >+} >+
On Fri, Jun 30, 2023, Chao Gao wrote: > On Fri, Feb 17, 2023 at 03:10:11PM -0800, Sean Christopherson wrote: > >+static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, > >+ unsigned int x86_feature) > >+{ > >+ BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > > >+ sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); > >+ > >+ vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); > >+} > >+ > >+static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, > >+ unsigned int x86_feature) > >+{ > >+ if (guest_cpuid_has(vcpu, x86_feature)) > > Most callers in this series are conditional on either boot_cpu_has() or some > local variables. Can we convert them to kvm_cpu_cap_has() and incorporate them > within this function? i.e., > > if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature)) Hmm, I was going to say "no", as most callers don't check kvm_cpu_cap_has() verbatim, but it doesn't have to be that way. The majority of SVM features factor in module params, but KVM should set the kvm_cpu capability if and only if a feature is supported in hardware *and* enabled by its module param. And arguably that's kinda sorta a bug fix, because this if (lbrv) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LBRV); technically should be if (lbrv && nested) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LBRV); Heh, and it's kinda sorta a bug fix for XSAVES on VMX, because this if (cpu_has_vmx_xsaves() && boot_cpu_has(X86_FEATURE_XSAVE) && guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES); should technically be if (kvm_cpu_cap_has(X86_FEATURE_XSAVES) && boot_cpu_has(X86_FEATURE_XSAVE) && guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES); > The benefits of doing so are > 1. callers needn't repeat > > if (kvm_cpu_cap_has(x86_feature)) > kvm_governed_feature_check_and_set(x86_feature) > > 2. this fits the idea better that guests can use a governed feature only if host > supports it _and_ QEMU exposes it to the guest. Agreed, especially since we'll still have kvm_governed_feature_set() for the extra special cases. Thanks for the input!
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 792a6037047a..cd660de02f7b 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -835,6 +835,17 @@ struct kvm_vcpu_arch { struct kvm_cpuid_entry2 *cpuid_entries; struct kvm_hypervisor_cpuid kvm_cpuid; + /* + * Track whether or not the guest is allowed to use features that are + * governed by KVM, where "governed" means KVM needs to manage state + * and/or explicitly enable the feature in hardware. Typically, but + * not always, governed features can be used by the guest if and only + * if both KVM and userspace want to expose the feature to the guest. + */ + struct { + u32 enabled; + } governed_features; + u64 reserved_gpa_bits; int maxphyaddr; diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 8f8edeaf8177..013fdc27fc8f 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -335,6 +335,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) struct kvm_lapic *apic = vcpu->arch.apic; struct kvm_cpuid_entry2 *best; + vcpu->arch.governed_features.enabled = 0; + best = kvm_find_cpuid_entry(vcpu, 1); if (best && apic) { if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h index b1658c0de847..f61a2106ba90 100644 --- a/arch/x86/kvm/cpuid.h +++ b/arch/x86/kvm/cpuid.h @@ -232,4 +232,55 @@ static __always_inline bool guest_pv_has(struct kvm_vcpu *vcpu, return vcpu->arch.pv_cpuid.features & (1u << kvm_feature); } +enum kvm_governed_features { +#define KVM_GOVERNED_FEATURE(x) KVM_GOVERNED_##x, +#include "governed_features.h" + KVM_NR_GOVERNED_FEATURES +}; + +static __always_inline int kvm_governed_feature_index(unsigned int x86_feature) +{ + switch (x86_feature) { +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x; +#include "governed_features.h" + default: + return -1; + } +} + +static __always_inline int kvm_is_governed_feature(unsigned int x86_feature) +{ + return kvm_governed_feature_index(x86_feature) >= 0; +} + +static __always_inline u32 kvm_governed_feature_bit(unsigned int x86_feature) +{ + int index = kvm_governed_feature_index(x86_feature); + + BUILD_BUG_ON(index < 0); + return BIT(index); +} + +static __always_inline void kvm_governed_feature_set(struct kvm_vcpu *vcpu, + unsigned int x86_feature) +{ + BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > + sizeof(vcpu->arch.governed_features.enabled) * BITS_PER_BYTE); + + vcpu->arch.governed_features.enabled |= kvm_governed_feature_bit(x86_feature); +} + +static __always_inline void kvm_governed_feature_check_and_set(struct kvm_vcpu *vcpu, + unsigned int x86_feature) +{ + if (guest_cpuid_has(vcpu, x86_feature)) + kvm_governed_feature_set(vcpu, x86_feature); +} + +static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, + unsigned int x86_feature) +{ + return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); +} + #endif diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h new file mode 100644 index 000000000000..40ce8e6608cd --- /dev/null +++ b/arch/x86/kvm/governed_features.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#if !defined(KVM_GOVERNED_FEATURE) || defined(KVM_GOVERNED_X86_FEATURE) +BUILD_BUG() +#endif + +#define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x) + +#undef KVM_GOVERNED_X86_FEATURE +#undef KVM_GOVERNED_FEATURE