Fix two bugs I introduced when adding the enable_mmio_caching module param.
Bug #1 is that KVM unintentionally makes disabling caching due to a config
incompatibility "sticky", e.g. disabling caching because there are no
reserved PA bits prevents KVM from enabling when "switching" to an EPT
config (doesn't rely on PA bits) or when SVM adjusts the MMIO masks to
account for C-bit shenanigans (even if MAXPHYADDR=52 and C-bit=51, there
can be reserved PA bits due to the "real" MAXPHYADDR being reduced).
Bug #2 is that KVM doesn't explicitly check that MMIO caching is enabled
when doing SEV-ES setup. Prior to the module param, MMIO caching was
guaranteed when SEV-ES could be enabled as SEV-ES-capable CPUs effectively
guarantee there will be at least one reserved PA bit (see above). With
the module param, userspace can explicitly disable MMIO caching, thus
silently breaking SEV-ES.
v2:
- Collect *-by. [Mike, Kai]
- Squash patches 3 and 4 together. [Kai]
v1:
- https://lore.kernel.org/all/[email protected]
Sean Christopherson (3):
KVM: x86: Tag kvm_mmu_x86_module_init() with __init
KVM: x86/mmu: Fully re-evaluate MMIO caching when SPTE masks change
KVM: SVM: Disable SEV-ES support if MMIO caching is disable
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/mmu.h | 2 ++
arch/x86/kvm/mmu/mmu.c | 6 +++++-
arch/x86/kvm/mmu/spte.c | 20 ++++++++++++++++++++
arch/x86/kvm/mmu/spte.h | 3 +--
arch/x86/kvm/svm/sev.c | 10 ++++++++++
arch/x86/kvm/svm/svm.c | 9 ++++++---
7 files changed, 45 insertions(+), 7 deletions(-)
base-commit: 93472b79715378a2386598d6632c654a2223267b
--
2.37.1.559.g78731f0fdb-goog
Fully re-evaluate whether or not MMIO caching can be enabled when SPTE
masks change; simply clearing enable_mmio_caching when a configuration
isn't compatible with caching fails to handle the scenario where the
masks are updated, e.g. by VMX for EPT or by SVM to account for the C-bit
location, and toggle compatibility from false=>true.
Snapshot the original module param so that re-evaluating MMIO caching
preserves userspace's desire to allow caching. Use a snapshot approach
so that enable_mmio_caching still reflects KVM's actual behavior.
Fixes: 8b9e74bfbf8c ("KVM: x86/mmu: Use enable_mmio_caching to track if MMIO caching is enabled")
Reported-by: Michael Roth <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: [email protected]
Tested-by: Michael Roth <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 4 ++++
arch/x86/kvm/mmu/spte.c | 19 +++++++++++++++++++
arch/x86/kvm/mmu/spte.h | 1 +
3 files changed, 24 insertions(+)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index bf808107a56b..48f34016cb0b 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6699,11 +6699,15 @@ static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
/*
* nx_huge_pages needs to be resolved to true/false when kvm.ko is loaded, as
* its default value of -1 is technically undefined behavior for a boolean.
+ * Forward the module init call to SPTE code so that it too can handle module
+ * params that need to be resolved/snapshot.
*/
void __init kvm_mmu_x86_module_init(void)
{
if (nx_huge_pages == -1)
__set_nx_huge_pages(get_nx_auto_mode());
+
+ kvm_mmu_spte_module_init();
}
/*
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index 7314d27d57a4..66f76f5a15bd 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -20,6 +20,7 @@
#include <asm/vmx.h>
bool __read_mostly enable_mmio_caching = true;
+static bool __ro_after_init allow_mmio_caching;
module_param_named(mmio_caching, enable_mmio_caching, bool, 0444);
u64 __read_mostly shadow_host_writable_mask;
@@ -43,6 +44,18 @@ u64 __read_mostly shadow_nonpresent_or_rsvd_lower_gfn_mask;
u8 __read_mostly shadow_phys_bits;
+void __init kvm_mmu_spte_module_init(void)
+{
+ /*
+ * Snapshot userspace's desire to allow MMIO caching. Whether or not
+ * KVM can actually enable MMIO caching depends on vendor-specific
+ * hardware capabilities and other module params that can't be resolved
+ * until the vendor module is loaded, i.e. enable_mmio_caching can and
+ * will change when the vendor module is (re)loaded.
+ */
+ allow_mmio_caching = enable_mmio_caching;
+}
+
static u64 generation_mmio_spte_mask(u64 gen)
{
u64 mask;
@@ -340,6 +353,12 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask)
BUG_ON((u64)(unsigned)access_mask != access_mask);
WARN_ON(mmio_value & shadow_nonpresent_or_rsvd_lower_gfn_mask);
+ /*
+ * Reset to the original module param value to honor userspace's desire
+ * to (dis)allow MMIO caching. Update the param itself so that
+ * userspace can see whether or not KVM is actually using MMIO caching.
+ */
+ enable_mmio_caching = allow_mmio_caching;
if (!enable_mmio_caching)
mmio_value = 0;
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index cabe3fbb4f39..26b144ffd146 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -450,6 +450,7 @@ static inline u64 restore_acc_track_spte(u64 spte)
u64 kvm_mmu_changed_pte_notifier_make_spte(u64 old_spte, kvm_pfn_t new_pfn);
+void __init kvm_mmu_spte_module_init(void);
void kvm_mmu_reset_all_pte_masks(void);
#endif
--
2.37.1.559.g78731f0fdb-goog
Disable SEV-ES if MMIO caching is disabled as SEV-ES relies on MMIO SPTEs
generating #NPF(RSVD), which are reflected by the CPU into the guest as
a #VC. With SEV-ES, the untrusted host, a.k.a. KVM, doesn't have access
to the guest instruction stream or register state and so can't directly
emulate in response to a #NPF on an emulated MMIO GPA. Disabling MMIO
caching means guest accesses to emulated MMIO ranges cause #NPF(!PRESENT),
and those flavors of #NPF cause automatic VM-Exits, not #VC.
Adjust KVM's MMIO masks to account for the C-bit location prior to doing
SEV(-ES) setup, and document that dependency between adjusting the MMIO
SPTE mask and SEV(-ES) setup.
Fixes: b09763da4dd8 ("KVM: x86/mmu: Add module param to disable MMIO caching (for testing)")
Reported-by: Michael Roth <[email protected]>
Tested-by: Michael Roth <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: [email protected]
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/mmu.h | 2 ++
arch/x86/kvm/mmu/spte.c | 1 +
arch/x86/kvm/mmu/spte.h | 2 --
arch/x86/kvm/svm/sev.c | 10 ++++++++++
arch/x86/kvm/svm/svm.c | 9 ++++++---
5 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index a99acec925eb..6bdaacb6faa0 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -6,6 +6,8 @@
#include "kvm_cache_regs.h"
#include "cpuid.h"
+extern bool __read_mostly enable_mmio_caching;
+
#define PT_WRITABLE_SHIFT 1
#define PT_USER_SHIFT 2
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index 66f76f5a15bd..03ca740bf721 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -22,6 +22,7 @@
bool __read_mostly enable_mmio_caching = true;
static bool __ro_after_init allow_mmio_caching;
module_param_named(mmio_caching, enable_mmio_caching, bool, 0444);
+EXPORT_SYMBOL_GPL(enable_mmio_caching);
u64 __read_mostly shadow_host_writable_mask;
u64 __read_mostly shadow_mmu_writable_mask;
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index 26b144ffd146..9a9414b8d1d6 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -5,8 +5,6 @@
#include "mmu_internal.h"
-extern bool __read_mostly enable_mmio_caching;
-
/*
* A MMU present SPTE is backed by actual memory and may or may not be present
* in hardware. E.g. MMIO SPTEs are not considered present. Use bit 11, as it
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index b0e793e7d85c..28064060413a 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -22,6 +22,7 @@
#include <asm/trapnr.h>
#include <asm/fpu/xcr.h>
+#include "mmu.h"
#include "x86.h"
#include "svm.h"
#include "svm_ops.h"
@@ -2221,6 +2222,15 @@ void __init sev_hardware_setup(void)
if (!sev_es_enabled)
goto out;
+ /*
+ * SEV-ES requires MMIO caching as KVM doesn't have access to the guest
+ * instruction stream, i.e. can't emulate in response to a #NPF and
+ * instead relies on #NPF(RSVD) being reflected into the guest as #VC
+ * (the guest can then do a #VMGEXIT to request MMIO emulation).
+ */
+ if (!enable_mmio_caching)
+ goto out;
+
/* Does the CPU support SEV-ES? */
if (!boot_cpu_has(X86_FEATURE_SEV_ES))
goto out;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 38f873cb6f2c..f3813dbacb9f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5034,13 +5034,16 @@ static __init int svm_hardware_setup(void)
/* Setup shadow_me_value and shadow_me_mask */
kvm_mmu_set_me_spte_mask(sme_me_mask, sme_me_mask);
- /* Note, SEV setup consumes npt_enabled. */
+ svm_adjust_mmio_mask();
+
+ /*
+ * Note, SEV setup consumes npt_enabled and enable_mmio_caching (which
+ * may be modified by svm_adjust_mmio_mask()).
+ */
sev_hardware_setup();
svm_hv_hardware_setup();
- svm_adjust_mmio_mask();
-
for_each_possible_cpu(cpu) {
r = svm_cpu_init(cpu);
if (r)
--
2.37.1.559.g78731f0fdb-goog
Mark kvm_mmu_x86_module_init() with __init, the entire reason it exists
is to initialize variables when kvm.ko is loaded, i.e. it must never be
called after module initialization.
Fixes: 1d0e84806047 ("KVM: x86/mmu: Resolve nx_huge_pages when kvm.ko is loaded")
Cc: [email protected]
Reviewed-by: Kai Huang <[email protected]>
Tested-by: Michael Roth <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/mmu/mmu.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e8281d64a431..5ffa578cafe1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1704,7 +1704,7 @@ static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm)
#define kvm_arch_pmi_in_guest(vcpu) \
((vcpu) && (vcpu)->arch.handling_intr_from_guest)
-void kvm_mmu_x86_module_init(void);
+void __init kvm_mmu_x86_module_init(void);
int kvm_mmu_vendor_module_init(void);
void kvm_mmu_vendor_module_exit(void);
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 3e1317325e1f..bf808107a56b 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6700,7 +6700,7 @@ static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
* nx_huge_pages needs to be resolved to true/false when kvm.ko is loaded, as
* its default value of -1 is technically undefined behavior for a boolean.
*/
-void kvm_mmu_x86_module_init(void)
+void __init kvm_mmu_x86_module_init(void)
{
if (nx_huge_pages == -1)
__set_nx_huge_pages(get_nx_auto_mode());
--
2.37.1.559.g78731f0fdb-goog
On Wed, 2022-08-03 at 22:49 +0000, Sean Christopherson wrote:
> Fully re-evaluate whether or not MMIO caching can be enabled when SPTE
> masks change; simply clearing enable_mmio_caching when a configuration
> isn't compatible with caching fails to handle the scenario where the
> masks are updated, e.g. by VMX for EPT or by SVM to account for the C-bit
> location, and toggle compatibility from false=>true.
>
> Snapshot the original module param so that re-evaluating MMIO caching
> preserves userspace's desire to allow caching. Use a snapshot approach
> so that enable_mmio_caching still reflects KVM's actual behavior.
>
> Fixes: 8b9e74bfbf8c ("KVM: x86/mmu: Use enable_mmio_caching to track if MMIO caching is enabled")
> Reported-by: Michael Roth <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: [email protected]
> Tested-by: Michael Roth <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Kai Huang <[email protected]>
One Nit below...
> ---
> arch/x86/kvm/mmu/mmu.c | 4 ++++
> arch/x86/kvm/mmu/spte.c | 19 +++++++++++++++++++
> arch/x86/kvm/mmu/spte.h | 1 +
> 3 files changed, 24 insertions(+)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index bf808107a56b..48f34016cb0b 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6699,11 +6699,15 @@ static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
> /*
> * nx_huge_pages needs to be resolved to true/false when kvm.ko is loaded, as
> * its default value of -1 is technically undefined behavior for a boolean.
> + * Forward the module init call to SPTE code so that it too can handle module
> + * params that need to be resolved/snapshot.
> */
> void __init kvm_mmu_x86_module_init(void)
> {
> if (nx_huge_pages == -1)
> __set_nx_huge_pages(get_nx_auto_mode());
> +
> + kvm_mmu_spte_module_init();
> }
>
> /*
> diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
> index 7314d27d57a4..66f76f5a15bd 100644
> --- a/arch/x86/kvm/mmu/spte.c
> +++ b/arch/x86/kvm/mmu/spte.c
> @@ -20,6 +20,7 @@
> #include <asm/vmx.h>
>
> bool __read_mostly enable_mmio_caching = true;
> +static bool __ro_after_init allow_mmio_caching;
> module_param_named(mmio_caching, enable_mmio_caching, bool, 0444);
>
> u64 __read_mostly shadow_host_writable_mask;
> @@ -43,6 +44,18 @@ u64 __read_mostly shadow_nonpresent_or_rsvd_lower_gfn_mask;
>
> u8 __read_mostly shadow_phys_bits;
>
> +void __init kvm_mmu_spte_module_init(void)
> +{
> + /*
> + * Snapshot userspace's desire to allow MMIO caching. Whether or not
> + * KVM can actually enable MMIO caching depends on vendor-specific
> + * hardware capabilities and other module params that can't be resolved
> + * until the vendor module is loaded, i.e. enable_mmio_caching can and
> + * will change when the vendor module is (re)loaded.
> + */
> + allow_mmio_caching = enable_mmio_caching;
... Perhaps 'use_mmio_caching' or 'want_mmio_caching' is better as it reflects
userspace's desire? Anyway let you decide.
> +}
> +
> static u64 generation_mmio_spte_mask(u64 gen)
> {
> u64 mask;
> @@ -340,6 +353,12 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask)
> BUG_ON((u64)(unsigned)access_mask != access_mask);
> WARN_ON(mmio_value & shadow_nonpresent_or_rsvd_lower_gfn_mask);
>
> + /*
> + * Reset to the original module param value to honor userspace's desire
> + * to (dis)allow MMIO caching. Update the param itself so that
> + * userspace can see whether or not KVM is actually using MMIO caching.
> + */
> + enable_mmio_caching = allow_mmio_caching;
> if (!enable_mmio_caching)
> mmio_value = 0;
>
> diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> index cabe3fbb4f39..26b144ffd146 100644
> --- a/arch/x86/kvm/mmu/spte.h
> +++ b/arch/x86/kvm/mmu/spte.h
> @@ -450,6 +450,7 @@ static inline u64 restore_acc_track_spte(u64 spte)
>
> u64 kvm_mmu_changed_pte_notifier_make_spte(u64 old_spte, kvm_pfn_t new_pfn);
>
> +void __init kvm_mmu_spte_module_init(void);
> void kvm_mmu_reset_all_pte_masks(void);
>
> #endif
On Thu, Aug 04, 2022, Kai Huang wrote:
> On Wed, 2022-08-03 at 22:49 +0000, Sean Christopherson wrote:
> > +void __init kvm_mmu_spte_module_init(void)
> > +{
> > + /*
> > + * Snapshot userspace's desire to allow MMIO caching. Whether or not
> > + * KVM can actually enable MMIO caching depends on vendor-specific
> > + * hardware capabilities and other module params that can't be resolved
> > + * until the vendor module is loaded, i.e. enable_mmio_caching can and
> > + * will change when the vendor module is (re)loaded.
> > + */
> > + allow_mmio_caching = enable_mmio_caching;
>
> ... Perhaps 'use_mmio_caching' or 'want_mmio_caching' is better as it reflects
> userspace's desire? Anyway let you decide.
Part of me likes "want_mmio_caching", but the module param really is only for
testing, i.e. any sane configuration always wants MMIO caching, but sometimes it's
explicitly disallowed purely so that KVM can mimic hardware that doesn't support
MMIO caching.
On Wed, Aug 3, 2022 at 3:50 PM Sean Christopherson <[email protected]> wrote:
>
> Fully re-evaluate whether or not MMIO caching can be enabled when SPTE
> masks change; simply clearing enable_mmio_caching when a configuration
> isn't compatible with caching fails to handle the scenario where the
> masks are updated, e.g. by VMX for EPT or by SVM to account for the C-bit
> location, and toggle compatibility from false=>true.
>
> Snapshot the original module param so that re-evaluating MMIO caching
> preserves userspace's desire to allow caching. Use a snapshot approach
> so that enable_mmio_caching still reflects KVM's actual behavior.
Is updating module parameters to reflect the actual behavior (vs.
userspace desire) something we should do for all module parameters?
I am doing an unrelated refactor to the tdp_mmu module parameter and
noticed it is not updated e.g. if userspace loads kvm_intel with
ept=N.
On 8/19/22 18:21, David Matlack wrote:
> On Wed, Aug 3, 2022 at 3:50 PM Sean Christopherson <[email protected]> wrote:
>>
>> Fully re-evaluate whether or not MMIO caching can be enabled when SPTE
>> masks change; simply clearing enable_mmio_caching when a configuration
>> isn't compatible with caching fails to handle the scenario where the
>> masks are updated, e.g. by VMX for EPT or by SVM to account for the C-bit
>> location, and toggle compatibility from false=>true.
>>
>> Snapshot the original module param so that re-evaluating MMIO caching
>> preserves userspace's desire to allow caching. Use a snapshot approach
>> so that enable_mmio_caching still reflects KVM's actual behavior.
>
> Is updating module parameters to reflect the actual behavior (vs.
> userspace desire) something we should do for all module parameters?
>
> I am doing an unrelated refactor to the tdp_mmu module parameter and
> noticed it is not updated e.g. if userspace loads kvm_intel with
> ept=N.
If it is cheap/easy then yeah, updating the parameters is the right
thing to do. Generally, however, this is only done for
kvm_intel/kvm_amd modules that depend on hardware features, because they
are more important for debugging user issues. (Or at least they were
until vmx features were added to /proc/cpuinfo).
Paolo
On Fri, Aug 19, 2022, Paolo Bonzini wrote:
> On 8/19/22 18:21, David Matlack wrote:
> > On Wed, Aug 3, 2022 at 3:50 PM Sean Christopherson <[email protected]> wrote:
> > >
> > > Fully re-evaluate whether or not MMIO caching can be enabled when SPTE
> > > masks change; simply clearing enable_mmio_caching when a configuration
> > > isn't compatible with caching fails to handle the scenario where the
> > > masks are updated, e.g. by VMX for EPT or by SVM to account for the C-bit
> > > location, and toggle compatibility from false=>true.
> > >
> > > Snapshot the original module param so that re-evaluating MMIO caching
> > > preserves userspace's desire to allow caching. Use a snapshot approach
> > > so that enable_mmio_caching still reflects KVM's actual behavior.
> >
> > Is updating module parameters to reflect the actual behavior (vs.
> > userspace desire) something we should do for all module parameters?
> >
> > I am doing an unrelated refactor to the tdp_mmu module parameter and
> > noticed it is not updated e.g. if userspace loads kvm_intel with
> > ept=N.
>
> If it is cheap/easy then yeah, updating the parameters is the right thing to
> do. Generally, however, this is only done for kvm_intel/kvm_amd modules
> that depend on hardware features, because they are more important for
> debugging user issues. (Or at least they were until vmx features were added
> to /proc/cpuinfo).
IMO, unless it's _really_ hard, KVM should keep the parameters up-to-date with
reality, e.g. so that userspace can assert that a feature is fully enabled, either
for testing purposes (unrestricted guest?) or to prevent running with a "bad" config.
We've had at least one internal OMG-level bug where KVM effectively ran with the
wrong MMU configuration, and IIRC one of the actions taken in response to that was
to assert on KVM's parameters post-load.