| Message ID | 20230203192822.106773-2-vipinsh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Optimize clear dirty log | expand |
On Fri, Feb 3, 2023 at 11:28 AM Vipin Sharma <vipinsh@google.com> wrote: > > Move condition checks in kvm_tdp_mmu_write_spte() for writing spte > atomically in a separate function. > > New function will be used in future commits to clear bits in SPTE. > > Signed-off-by: Vipin Sharma <vipinsh@google.com> Reviewed-by: Ben Gardon <bgardon@google.com> > --- > arch/x86/kvm/mmu/tdp_iter.h | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h > index f0af385c56e0..30a52e5e68de 100644 > --- a/arch/x86/kvm/mmu/tdp_iter.h > +++ b/arch/x86/kvm/mmu/tdp_iter.h > @@ -29,11 +29,10 @@ static inline void __kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 new_spte) > WRITE_ONCE(*rcu_dereference(sptep), new_spte); > } > > -static inline u64 kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 old_spte, > - u64 new_spte, int level) > +static inline bool kvm_tdp_mmu_spte_has_volatile_bits(u64 old_spte, int level) > { > /* > - * Atomically write the SPTE if it is a shadow-present, leaf SPTE with > + * Atomically write SPTEs if it is a shadow-present, leaf SPTE with Nit: SPTEs must be modified atomically if they are shadow-present, leaf SPTEs with > * volatile bits, i.e. has bits that can be set outside of mmu_lock. > * The Writable bit can be set by KVM's fast page fault handler, and > * Accessed and Dirty bits can be set by the CPU. > @@ -44,8 +43,15 @@ static inline u64 kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 old_spte, > * logic needs to be reassessed if KVM were to use non-leaf Accessed > * bits, e.g. to skip stepping down into child SPTEs when aging SPTEs. > */ > - if (is_shadow_present_pte(old_spte) && is_last_spte(old_spte, level) && > - spte_has_volatile_bits(old_spte)) > + return is_shadow_present_pte(old_spte) && > + is_last_spte(old_spte, level) && > + spte_has_volatile_bits(old_spte); > +} > + > +static inline u64 kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 old_spte, > + u64 new_spte, int level) > +{ > + if (kvm_tdp_mmu_spte_has_volatile_bits(old_spte, level)) > return kvm_tdp_mmu_write_spte_atomic(sptep, new_spte); > > __kvm_tdp_mmu_write_spte(sptep, new_spte); > -- > 2.39.1.519.gcb327c4b5f-goog >
The shortlog is difficult to understand. - I think it's more common to use "Add" or "Introduce" when talking about adding a new function, rather than "Make". - "atomic write conditions" does not mirror the code naming, which checks for "volatile bits". e.g. The function is not called kvm_tdp_mmu_spte_need_atomic_write(). "volatile bits" is, at this point, pretty standard terminology in KVM MMU to refer to "bits that can change outside the MMU lock". So I would suggest leaning on that here. So something like this: KVM: x86/mmu: Add helper function to check if an SPTE has volatile bits On Fri, Feb 03, 2023 at 11:28:18AM -0800, Vipin Sharma wrote: > Move condition checks in kvm_tdp_mmu_write_spte() for writing spte > atomically in a separate function. s/in a separate function/to a separate function/ > > New function will be used inc nit: Use complete sentences. e.g. "This new function ..." or just state the name directly, e.g. "kvm_tdp_mmu_spte_has_volatile_bits() will be used in ...". > future commits to clear bits in SPTE. s/to clear bits in SPTE/to optimize clearing bits in SPTEs/ > > Signed-off-by: Vipin Sharma <vipinsh@google.com> Code looks fine, just grammar/writing nits above. Reviewed-by: David Matlack <dmatlack@google.com>
diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h index f0af385c56e0..30a52e5e68de 100644 --- a/arch/x86/kvm/mmu/tdp_iter.h +++ b/arch/x86/kvm/mmu/tdp_iter.h @@ -29,11 +29,10 @@ static inline void __kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 new_spte) WRITE_ONCE(*rcu_dereference(sptep), new_spte); } -static inline u64 kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 old_spte, - u64 new_spte, int level) +static inline bool kvm_tdp_mmu_spte_has_volatile_bits(u64 old_spte, int level) { /* - * Atomically write the SPTE if it is a shadow-present, leaf SPTE with + * Atomically write SPTEs if it is a shadow-present, leaf SPTE with * volatile bits, i.e. has bits that can be set outside of mmu_lock. * The Writable bit can be set by KVM's fast page fault handler, and * Accessed and Dirty bits can be set by the CPU. @@ -44,8 +43,15 @@ static inline u64 kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 old_spte, * logic needs to be reassessed if KVM were to use non-leaf Accessed * bits, e.g. to skip stepping down into child SPTEs when aging SPTEs. */ - if (is_shadow_present_pte(old_spte) && is_last_spte(old_spte, level) && - spte_has_volatile_bits(old_spte)) + return is_shadow_present_pte(old_spte) && + is_last_spte(old_spte, level) && + spte_has_volatile_bits(old_spte); +} + +static inline u64 kvm_tdp_mmu_write_spte(tdp_ptep_t sptep, u64 old_spte, + u64 new_spte, int level) +{ + if (kvm_tdp_mmu_spte_has_volatile_bits(old_spte, level)) return kvm_tdp_mmu_write_spte_atomic(sptep, new_spte); __kvm_tdp_mmu_write_spte(sptep, new_spte);
Move condition checks in kvm_tdp_mmu_write_spte() for writing spte atomically in a separate function. New function will be used in future commits to clear bits in SPTE. Signed-off-by: Vipin Sharma <vipinsh@google.com> --- arch/x86/kvm/mmu/tdp_iter.h | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)