| Message ID | 1547733331-16140-2-git-send-email-ann.zhuangyanying@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: MMU: fast cleanup D bit based on fast write protect | expand |
On Thu, Jan 17, 2019 at 01:55:28PM +0000, Zhuangyanying wrote: > From: Xiao Guangrong <xiaoguangrong@tencent.com> > > Current behavior of mmu_spte_update_no_track() does not match > the name of _no_track() as actually the A/D bits are tracked > and returned to the caller Sentences should be terminated with periods. > This patch introduces the real _no_track() function to update "This patch" is redundant, e.g. simply state "Introduce ...". > the spte regardless of A/D bits and rename the original function > to _track() The function also avoids __update_clear_spte_slow(), i.e. AFAICT it doesn't guarantee volatile bits will be preserved. I assume this is intentional, but it'd be nice to explain why this is ok. > The _no_track() function will be used by later patches to update > upper spte which need not care of A/D bits indeed The _no_track() variant is already used (by mmu_spte_age()), I don't see any point in having this blurb on the changelog, e.g. it led me to incorrectly think an unused function was being introduced. > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> > --- > arch/x86/kvm/mmu.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index ce770b4..eeb3bac 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -731,10 +731,29 @@ static void mmu_spte_set(u64 *sptep, u64 new_spte) > } > > /* > - * Update the SPTE (excluding the PFN), but do not track changes in its > + * Update the SPTE (excluding the PFN) regardless of accessed/dirty > + * status which is used to update the upper level spte. > + */ > +static void mmu_spte_update_no_track(u64 *sptep, u64 new_spte) > +{ > + u64 old_spte = *sptep; No need to snapshot the old spte since it's not being returned. > + WARN_ON(!is_shadow_present_pte(new_spte)); > + > + if (!is_shadow_present_pte(old_spte)) { > + mmu_spte_set(sptep, new_spte); > + return; Similarly, this is more complex than it needs to be, e.g. the function can be simplified to: static void mmu_spte_update_no_track(u64 *sptep, u64 new_spte) { WARN_ON(!is_shadow_present_pte(new_spte)); if (!is_shadow_present_pte(*sptep)) mmu_spte_set(sptep, new_spte); else __update_clear_spte_fast(sptep, new_spte); } > + } > + > + __update_clear_spte_fast(sptep, new_spte); > +} > + > +/* > + * Update the SPTE (excluding the PFN), the original value is > + * returned, based on it, the caller can track changes of its > * accessed/dirty status. > */ > -static u64 mmu_spte_update_no_track(u64 *sptep, u64 new_spte) > +static u64 mmu_spte_update_track(u64 *sptep, u64 new_spte) > { > u64 old_spte = *sptep; > > @@ -769,7 +788,7 @@ static u64 mmu_spte_update_no_track(u64 *sptep, u64 new_spte) > static bool mmu_spte_update(u64 *sptep, u64 new_spte) > { > bool flush = false; > - u64 old_spte = mmu_spte_update_no_track(sptep, new_spte); > + u64 old_spte = mmu_spte_update_track(sptep, new_spte); > > if (!is_shadow_present_pte(old_spte)) > return false; > -- > 1.8.3.1 > >
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index ce770b4..eeb3bac 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -731,10 +731,29 @@ static void mmu_spte_set(u64 *sptep, u64 new_spte) } /* - * Update the SPTE (excluding the PFN), but do not track changes in its + * Update the SPTE (excluding the PFN) regardless of accessed/dirty + * status which is used to update the upper level spte. + */ +static void mmu_spte_update_no_track(u64 *sptep, u64 new_spte) +{ + u64 old_spte = *sptep; + + WARN_ON(!is_shadow_present_pte(new_spte)); + + if (!is_shadow_present_pte(old_spte)) { + mmu_spte_set(sptep, new_spte); + return; + } + + __update_clear_spte_fast(sptep, new_spte); +} + +/* + * Update the SPTE (excluding the PFN), the original value is + * returned, based on it, the caller can track changes of its * accessed/dirty status. */ -static u64 mmu_spte_update_no_track(u64 *sptep, u64 new_spte) +static u64 mmu_spte_update_track(u64 *sptep, u64 new_spte) { u64 old_spte = *sptep; @@ -769,7 +788,7 @@ static u64 mmu_spte_update_no_track(u64 *sptep, u64 new_spte) static bool mmu_spte_update(u64 *sptep, u64 new_spte) { bool flush = false; - u64 old_spte = mmu_spte_update_no_track(sptep, new_spte); + u64 old_spte = mmu_spte_update_track(sptep, new_spte); if (!is_shadow_present_pte(old_spte)) return false;