[1/4] KVM: MMU: correct the behavior of mmu_spte_update_no_track
diff mbox series

Message ID 1547733331-16140-2-git-send-email-ann.zhuangyanying@huawei.com
State New
Headers show
Series
  • KVM: MMU: fast cleanup D bit based on fast write protect
Related show

Commit Message

Zhuang Yanying Jan. 17, 2019, 1:55 p.m. UTC
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

This patch introduces the real _no_track() function to update
the spte regardless of A/D bits and rename the original function
to _track()

The _no_track() function will be used by later patches to update
upper spte which need not care of A/D bits indeed

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 arch/x86/kvm/mmu.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Comments

Sean Christopherson Jan. 17, 2019, 3:44 p.m. UTC | #1
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
> 
>

Patch
diff mbox series

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;