* [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation
@ 2022-01-25 23:05 David Matlack
2022-01-25 23:05 ` [PATCH 1/5] KVM: x86/mmu: Move SPTE writable invariant checks to a helper function David Matlack
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: David Matlack @ 2022-01-25 23:05 UTC (permalink / raw)
To: pbonzini; +Cc: seanjc, vkuznets, wanpengli, jmattson, joro, kvm, David Matlack
This series cleans up some documentation and WARNings related to
MMU-writable and Host-writable bits based on suggestions from Sean on
another patch [1].
[1] https://lore.kernel.org/kvm/YeH5QlwgGcpStZyp@google.com/
David Matlack (5):
KVM: x86/mmu: Move SPTE writable invariant checks to a helper function
KVM: x86/mmu: Check SPTE writable invariants when setting leaf SPTEs
KVM: x86/mmu: Move is_writable_pte() to spte.h
KVM: x86/mmu: Rename DEFAULT_SPTE_MMU_WRITEABLE to
DEFAULT_SPTE_MMU_WRITABLE
KVM: x86/mmu: Consolidate comments about {Host,MMU}-writable
arch/x86/kvm/mmu.h | 38 -------------
arch/x86/kvm/mmu/mmu.c | 11 ++--
arch/x86/kvm/mmu/spte.c | 13 +----
arch/x86/kvm/mmu/spte.h | 113 +++++++++++++++++++++++++++----------
arch/x86/kvm/mmu/tdp_mmu.c | 3 +
5 files changed, 93 insertions(+), 85 deletions(-)
base-commit: e2e83a73d7ce66f62c7830a85619542ef59c90e4
--
2.35.0.rc0.227.g00780c9af4-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/5] KVM: x86/mmu: Move SPTE writable invariant checks to a helper function
2022-01-25 23:05 [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
@ 2022-01-25 23:05 ` David Matlack
2022-01-25 23:05 ` [PATCH 2/5] KVM: x86/mmu: Check SPTE writable invariants when setting leaf SPTEs David Matlack
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: David Matlack @ 2022-01-25 23:05 UTC (permalink / raw)
To: pbonzini; +Cc: seanjc, vkuznets, wanpengli, jmattson, joro, kvm, David Matlack
Move the WARNs in spte_can_locklessly_be_made_writable() to a separate
helper function. This is in preparation for moving these checks to the
places where SPTEs are set.
Opportunistically add warning error messages that include the SPTE to
make future debugging of these warnings easier.
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
arch/x86/kvm/mmu/spte.h | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index be6a007a4af3..912e66859ea0 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -339,15 +339,21 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
__is_rsvd_bits_set(rsvd_check, spte, level);
}
-static inline bool spte_can_locklessly_be_made_writable(u64 spte)
+static inline void check_spte_writable_invariants(u64 spte)
{
- if (spte & shadow_mmu_writable_mask) {
- WARN_ON_ONCE(!(spte & shadow_host_writable_mask));
- return true;
- }
+ if (spte & shadow_mmu_writable_mask)
+ WARN_ONCE(!(spte & shadow_host_writable_mask),
+ "kvm: MMU-writable SPTE is not Host-writable: %llx",
+ spte);
+ else
+ WARN_ONCE(spte & PT_WRITABLE_MASK,
+ "kvm: Writable SPTE is not MMU-writable: %llx", spte);
+}
- WARN_ON_ONCE(spte & PT_WRITABLE_MASK);
- return false;
+static inline bool spte_can_locklessly_be_made_writable(u64 spte)
+{
+ check_spte_writable_invariants(spte);
+ return spte & shadow_mmu_writable_mask;
}
static inline u64 get_mmio_spte_generation(u64 spte)
base-commit: e2e83a73d7ce66f62c7830a85619542ef59c90e4
--
2.35.0.rc0.227.g00780c9af4-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/5] KVM: x86/mmu: Check SPTE writable invariants when setting leaf SPTEs
2022-01-25 23:05 [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
2022-01-25 23:05 ` [PATCH 1/5] KVM: x86/mmu: Move SPTE writable invariant checks to a helper function David Matlack
@ 2022-01-25 23:05 ` David Matlack
2022-01-25 23:05 ` [PATCH 3/5] KVM: x86/mmu: Move is_writable_pte() to spte.h David Matlack
2022-01-25 23:16 ` [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
3 siblings, 0 replies; 6+ messages in thread
From: David Matlack @ 2022-01-25 23:05 UTC (permalink / raw)
To: pbonzini; +Cc: seanjc, vkuznets, wanpengli, jmattson, joro, kvm, David Matlack
Check SPTE writable invariants when setting SPTEs rather than in
spte_can_locklessly_be_made_writable(). By the time KVM checks
spte_can_locklessly_be_made_writable(), the SPTE has long been since
corrupted.
Note that these invariants only apply to shadow-present leaf SPTEs (i.e.
not to MMIO SPTEs, non-leaf SPTEs, etc.). Add a comment explaining the
restriction and only instrument the code paths that set shadow-present
leaf SPTEs.
To account for access tracking, also check the SPTE writable invariants
when marking an SPTE as an access track SPTE. This also lets us remove
a redundant WARN from mark_spte_for_access_track().
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
arch/x86/kvm/mmu/mmu.c | 1 +
arch/x86/kvm/mmu/spte.c | 9 +--------
arch/x86/kvm/mmu/spte.h | 2 +-
arch/x86/kvm/mmu/tdp_mmu.c | 3 +++
4 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 593093b52395..795db506c230 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -529,6 +529,7 @@ static u64 mmu_spte_update_no_track(u64 *sptep, u64 new_spte)
u64 old_spte = *sptep;
WARN_ON(!is_shadow_present_pte(new_spte));
+ check_spte_writable_invariants(new_spte);
if (!is_shadow_present_pte(old_spte)) {
mmu_spte_set(sptep, new_spte);
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index f8677404c93c..24d66bb899a4 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -249,14 +249,7 @@ u64 mark_spte_for_access_track(u64 spte)
if (is_access_track_spte(spte))
return spte;
- /*
- * Making an Access Tracking PTE will result in removal of write access
- * from the PTE. So, verify that we will be able to restore the write
- * access in the fast page fault path later on.
- */
- WARN_ONCE((spte & PT_WRITABLE_MASK) &&
- !spte_can_locklessly_be_made_writable(spte),
- "kvm: Writable SPTE is not locklessly dirty-trackable\n");
+ check_spte_writable_invariants(spte);
WARN_ONCE(spte & (SHADOW_ACC_TRACK_SAVED_BITS_MASK <<
SHADOW_ACC_TRACK_SAVED_BITS_SHIFT),
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index 912e66859ea0..b8fd055acdbd 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -339,6 +339,7 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
__is_rsvd_bits_set(rsvd_check, spte, level);
}
+/* Note: spte must be a shadow-present leaf SPTE. */
static inline void check_spte_writable_invariants(u64 spte)
{
if (spte & shadow_mmu_writable_mask)
@@ -352,7 +353,6 @@ static inline void check_spte_writable_invariants(u64 spte)
static inline bool spte_can_locklessly_be_made_writable(u64 spte)
{
- check_spte_writable_invariants(spte);
return spte & shadow_mmu_writable_mask;
}
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index bc9e3553fba2..814c42def6e7 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -435,6 +435,9 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
trace_kvm_tdp_mmu_spte_changed(as_id, gfn, level, old_spte, new_spte);
+ if (is_leaf)
+ check_spte_writable_invariants(new_spte);
+
/*
* The only times a SPTE should be changed from a non-present to
* non-present state is when an MMIO entry is installed/modified/
--
2.35.0.rc0.227.g00780c9af4-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/5] KVM: x86/mmu: Move is_writable_pte() to spte.h
2022-01-25 23:05 [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
2022-01-25 23:05 ` [PATCH 1/5] KVM: x86/mmu: Move SPTE writable invariant checks to a helper function David Matlack
2022-01-25 23:05 ` [PATCH 2/5] KVM: x86/mmu: Check SPTE writable invariants when setting leaf SPTEs David Matlack
@ 2022-01-25 23:05 ` David Matlack
2022-01-25 23:16 ` [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
3 siblings, 0 replies; 6+ messages in thread
From: David Matlack @ 2022-01-25 23:05 UTC (permalink / raw)
To: pbonzini; +Cc: seanjc, vkuznets, wanpengli, jmattson, joro, kvm, David Matlack
Move is_writable_pte() close to the other functions that check
writability information about SPTEs. While here opportunistically
replace the open-coded bit arithmetic in
check_spte_writable_invariants() with a call to is_writable_pte().
No functional change intended.
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
arch/x86/kvm/mmu.h | 38 --------------------------------------
arch/x86/kvm/mmu/spte.h | 40 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index e9fbb2c8bbe2..51faa2c76ca5 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -202,44 +202,6 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
return vcpu->arch.mmu->page_fault(vcpu, &fault);
}
-/*
- * Currently, we have two sorts of write-protection, a) the first one
- * write-protects guest page to sync the guest modification, b) another one is
- * used to sync dirty bitmap when we do KVM_GET_DIRTY_LOG. The differences
- * between these two sorts are:
- * 1) the first case clears MMU-writable bit.
- * 2) the first case requires flushing tlb immediately avoiding corrupting
- * shadow page table between all vcpus so it should be in the protection of
- * mmu-lock. And the another case does not need to flush tlb until returning
- * the dirty bitmap to userspace since it only write-protects the page
- * logged in the bitmap, that means the page in the dirty bitmap is not
- * missed, so it can flush tlb out of mmu-lock.
- *
- * So, there is the problem: the first case can meet the corrupted tlb caused
- * by another case which write-protects pages but without flush tlb
- * immediately. In order to making the first case be aware this problem we let
- * it flush tlb if we try to write-protect a spte whose MMU-writable bit
- * is set, it works since another case never touches MMU-writable bit.
- *
- * Anyway, whenever a spte is updated (only permission and status bits are
- * changed) we need to check whether the spte with MMU-writable becomes
- * readonly, if that happens, we need to flush tlb. Fortunately,
- * mmu_spte_update() has already handled it perfectly.
- *
- * The rules to use MMU-writable and PT_WRITABLE_MASK:
- * - if we want to see if it has writable tlb entry or if the spte can be
- * writable on the mmu mapping, check MMU-writable, this is the most
- * case, otherwise
- * - if we fix page fault on the spte or do write-protection by dirty logging,
- * check PT_WRITABLE_MASK.
- *
- * TODO: introduce APIs to split these two cases.
- */
-static inline bool is_writable_pte(unsigned long pte)
-{
- return pte & PT_WRITABLE_MASK;
-}
-
/*
* Check if a given access (described through the I/D, W/R and U/S bits of a
* page fault error code pfec) causes a permission fault with the given PTE
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index b8fd055acdbd..e1ddba45bba1 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -339,6 +339,44 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
__is_rsvd_bits_set(rsvd_check, spte, level);
}
+/*
+ * Currently, we have two sorts of write-protection, a) the first one
+ * write-protects guest page to sync the guest modification, b) another one is
+ * used to sync dirty bitmap when we do KVM_GET_DIRTY_LOG. The differences
+ * between these two sorts are:
+ * 1) the first case clears MMU-writable bit.
+ * 2) the first case requires flushing tlb immediately avoiding corrupting
+ * shadow page table between all vcpus so it should be in the protection of
+ * mmu-lock. And the another case does not need to flush tlb until returning
+ * the dirty bitmap to userspace since it only write-protects the page
+ * logged in the bitmap, that means the page in the dirty bitmap is not
+ * missed, so it can flush tlb out of mmu-lock.
+ *
+ * So, there is the problem: the first case can meet the corrupted tlb caused
+ * by another case which write-protects pages but without flush tlb
+ * immediately. In order to making the first case be aware this problem we let
+ * it flush tlb if we try to write-protect a spte whose MMU-writable bit
+ * is set, it works since another case never touches MMU-writable bit.
+ *
+ * Anyway, whenever a spte is updated (only permission and status bits are
+ * changed) we need to check whether the spte with MMU-writable becomes
+ * readonly, if that happens, we need to flush tlb. Fortunately,
+ * mmu_spte_update() has already handled it perfectly.
+ *
+ * The rules to use MMU-writable and PT_WRITABLE_MASK:
+ * - if we want to see if it has writable tlb entry or if the spte can be
+ * writable on the mmu mapping, check MMU-writable, this is the most
+ * case, otherwise
+ * - if we fix page fault on the spte or do write-protection by dirty logging,
+ * check PT_WRITABLE_MASK.
+ *
+ * TODO: introduce APIs to split these two cases.
+ */
+static inline bool is_writable_pte(unsigned long pte)
+{
+ return pte & PT_WRITABLE_MASK;
+}
+
/* Note: spte must be a shadow-present leaf SPTE. */
static inline void check_spte_writable_invariants(u64 spte)
{
@@ -347,7 +385,7 @@ static inline void check_spte_writable_invariants(u64 spte)
"kvm: MMU-writable SPTE is not Host-writable: %llx",
spte);
else
- WARN_ONCE(spte & PT_WRITABLE_MASK,
+ WARN_ONCE(is_writable_pte(spte),
"kvm: Writable SPTE is not MMU-writable: %llx", spte);
}
--
2.35.0.rc0.227.g00780c9af4-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation
2022-01-25 23:05 [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
` (2 preceding siblings ...)
2022-01-25 23:05 ` [PATCH 3/5] KVM: x86/mmu: Move is_writable_pte() to spte.h David Matlack
@ 2022-01-25 23:16 ` David Matlack
2022-02-01 12:22 ` Paolo Bonzini
3 siblings, 1 reply; 6+ messages in thread
From: David Matlack @ 2022-01-25 23:16 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm list
On Tue, Jan 25, 2022 at 3:05 PM David Matlack <dmatlack@google.com> wrote:
>
> This series cleans up some documentation and WARNings related to
> MMU-writable and Host-writable bits based on suggestions from Sean on
> another patch [1].
>
> [1] https://lore.kernel.org/kvm/YeH5QlwgGcpStZyp@google.com/
>
> David Matlack (5):
> KVM: x86/mmu: Move SPTE writable invariant checks to a helper function
> KVM: x86/mmu: Check SPTE writable invariants when setting leaf SPTEs
> KVM: x86/mmu: Move is_writable_pte() to spte.h
> KVM: x86/mmu: Rename DEFAULT_SPTE_MMU_WRITEABLE to
> DEFAULT_SPTE_MMU_WRITABLE
> KVM: x86/mmu: Consolidate comments about {Host,MMU}-writable
The email threading on this series got a bit messed up (at least on
lore). I had a misspelling in the signed-off-by tag in patch 4 that
was caught by git-send-email after sending the cover letter and
patches 1-3. So I fixed it and sent patch 4 and 5 separately.
I can resend the series again if anyone prefers.
>
> arch/x86/kvm/mmu.h | 38 -------------
> arch/x86/kvm/mmu/mmu.c | 11 ++--
> arch/x86/kvm/mmu/spte.c | 13 +----
> arch/x86/kvm/mmu/spte.h | 113 +++++++++++++++++++++++++++----------
> arch/x86/kvm/mmu/tdp_mmu.c | 3 +
> 5 files changed, 93 insertions(+), 85 deletions(-)
>
>
> base-commit: e2e83a73d7ce66f62c7830a85619542ef59c90e4
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation
2022-01-25 23:16 ` [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
@ 2022-02-01 12:22 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2022-02-01 12:22 UTC (permalink / raw)
To: David Matlack
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm list
On 1/26/22 00:16, David Matlack wrote:
>>
>> David Matlack (5):
>> KVM: x86/mmu: Move SPTE writable invariant checks to a helper function
>> KVM: x86/mmu: Check SPTE writable invariants when setting leaf SPTEs
>> KVM: x86/mmu: Move is_writable_pte() to spte.h
>> KVM: x86/mmu: Rename DEFAULT_SPTE_MMU_WRITEABLE to
>> DEFAULT_SPTE_MMU_WRITABLE
>> KVM: x86/mmu: Consolidate comments about {Host,MMU}-writable
> The email threading on this series got a bit messed up (at least on
> lore). I had a misspelling in the signed-off-by tag in patch 4 that
> was caught by git-send-email after sending the cover letter and
> patches 1-3. So I fixed it and sent patch 4 and 5 separately.
>
> I can resend the series again if anyone prefers.
>
Not sure how that's even possible, but Thunderbird showed me correct
threading. Patch queued now, thanks.
Next time this happens you can send the other patches with --in-reply-to
--no-thread:
--in-reply-to=<identifier>
Make the first mail (or all the mails with --no-thread)
appear as a reply to the given Message-Id [...]
Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-01 12:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25 23:05 [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
2022-01-25 23:05 ` [PATCH 1/5] KVM: x86/mmu: Move SPTE writable invariant checks to a helper function David Matlack
2022-01-25 23:05 ` [PATCH 2/5] KVM: x86/mmu: Check SPTE writable invariants when setting leaf SPTEs David Matlack
2022-01-25 23:05 ` [PATCH 3/5] KVM: x86/mmu: Move is_writable_pte() to spte.h David Matlack
2022-01-25 23:16 ` [PATCH 0/5] KVM: x86/mmu: Clean up {Host,MMU}-writable documentation and validation David Matlack
2022-02-01 12:22 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).