It helps us to cleanup release pfn in the later patches

Signed-off-by: Xiao Guangrong <xiaoguangr...@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c         |   29 ++++++++++++++---------------
 arch/x86/kvm/paging_tmpl.h |   18 +++++++++++-------
 2 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index f74c63a..29ce28b 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2496,9 +2496,6 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
                                rmap_recycle(vcpu, sptep, gfn);
                }
        }
-
-       if (!is_error_pfn(pfn))
-               kvm_release_pfn_clean(pfn);
 }

 static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)
@@ -2535,12 +2532,15 @@ static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
        if (ret <= 0)
                return -1;

-       for (i = 0; i < ret; i++, gfn++, start++)
+       for (i = 0; i < ret; i++, gfn++, start++) {
                mmu_set_spte(vcpu, start, ACC_ALL,
                             access, 0, 0, NULL,
                             sp->role.level, gfn,
                             page_to_pfn(pages[i]), true, true);

+               kvm_release_page_clean(pages[i]);
+       }
+
        return 0;
 }

@@ -2863,23 +2863,22 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
                return r;

        spin_lock(&vcpu->kvm->mmu_lock);
-       if (mmu_notifier_retry(vcpu, mmu_seq))
+       if (mmu_notifier_retry(vcpu, mmu_seq)) {
+               r = 0;
                goto out_unlock;
+       }
+
        kvm_mmu_free_some_pages(vcpu);
        if (likely(!force_pt_level))
                transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
        r = __direct_map(vcpu, v, write, map_writable, level, gfn, pfn,
                         prefault);
-       spin_unlock(&vcpu->kvm->mmu_lock);
-
-
-       return r;

 out_unlock:
        spin_unlock(&vcpu->kvm->mmu_lock);
        if (!is_error_pfn(pfn))
                kvm_release_pfn_clean(pfn);
-       return 0;
+       return r;
 }

@@ -3333,22 +3332,22 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,
                return r;

        spin_lock(&vcpu->kvm->mmu_lock);
-       if (mmu_notifier_retry(vcpu, mmu_seq))
+       if (mmu_notifier_retry(vcpu, mmu_seq)) {
+               r = 0;
                goto out_unlock;
+       }
+
        kvm_mmu_free_some_pages(vcpu);
        if (likely(!force_pt_level))
                transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
        r = __direct_map(vcpu, gpa, write, map_writable,
                         level, gfn, pfn, prefault);
-       spin_unlock(&vcpu->kvm->mmu_lock);
-
-       return r;

 out_unlock:
        spin_unlock(&vcpu->kvm->mmu_lock);
        if (!is_error_pfn(pfn))
                kvm_release_pfn_clean(pfn);
-       return 0;
+       return r;
 }

 static void nonpaging_free(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index c004ab6..92f466c 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -380,6 +380,9 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
        mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
                     NULL, PT_PAGE_TABLE_LEVEL,
                     gpte_to_gfn(gpte), pfn, true, true);
+
+       if (!is_error_pfn(pfn))
+               kvm_release_pfn_clean(pfn);
 }

 static bool FNAME(gpte_changed)(struct kvm_vcpu *vcpu,
@@ -452,6 +455,8 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
                mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
                             NULL, PT_PAGE_TABLE_LEVEL, gfn,
                             pfn, true, true);
+               if (!is_error_pfn(pfn))
+                       kvm_release_pfn_clean(pfn);
        }
 }

@@ -544,8 +549,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 out_gpte_changed:
        if (sp)
                kvm_mmu_put_page(sp, it.sptep);
-       if (!is_error_pfn(pfn))
-               kvm_release_pfn_clean(pfn);
+
        return NULL;
 }

@@ -625,8 +629,10 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
                return r;

        spin_lock(&vcpu->kvm->mmu_lock);
-       if (mmu_notifier_retry(vcpu, mmu_seq))
+       if (mmu_notifier_retry(vcpu, mmu_seq)) {
+               r = 0;
                goto out_unlock;
+       }

        kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
        kvm_mmu_free_some_pages(vcpu);
@@ -640,15 +646,13 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,

        ++vcpu->stat.pf_fixed;
        kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
-       spin_unlock(&vcpu->kvm->mmu_lock);
-
-       return emulate;
+       r = emulate;

 out_unlock:
        spin_unlock(&vcpu->kvm->mmu_lock);
        if (!is_error_pfn(pfn))
                kvm_release_pfn_clean(pfn);
-       return 0;
+       return r;
 }

 static gpa_t FNAME(get_level1_sp_gpa)(struct kvm_mmu_page *sp)
--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

The only different thing between FNAME(update_pte) and FNAME(pte_prefetch)
is that the former is allowed to prefetch gfn from dirty logged slot, so
introduce a common function to prefetch spte

Signed-off-by: Xiao Guangrong <xiaoguangr...@linux.vnet.ibm.com>
---
 arch/x86/kvm/paging_tmpl.h |   50 ++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1a738c5..9742eab 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -356,35 +356,45 @@ no_present:
        return true;
 }

-static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-                             u64 *spte, const void *pte)
+static void
+FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+                    u64 *spte, pt_element_t gpte, bool no_dirty_log)
 {
-       pt_element_t gpte;
        unsigned pte_access;
+       gfn_t gfn;
        pfn_t pfn;

-       gpte = *(const pt_element_t *)pte;
        if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
                return;

        pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
+
+       gfn = gpte_to_gfn(gpte);
        pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte, true);
-       pfn = gfn_to_pfn_atomic(vcpu->kvm, gpte_to_gfn(gpte));
+       pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
+                       no_dirty_log && (pte_access & ACC_WRITE_MASK));
        if (mmu_invalid_pfn(pfn))
                return;

        /*
         * we call mmu_set_spte() with host_writable = true because that
-        * vcpu->arch.update_pte.pfn was fetched from get_user_pages(write = 1).
+        * pte_prefetch_gfn_to_pfn always gets a writable pfn.
         */
        mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-                    NULL, PT_PAGE_TABLE_LEVEL,
-                    gpte_to_gfn(gpte), pfn, true, true);
+                    NULL, PT_PAGE_TABLE_LEVEL, gfn, pfn, true, true);

        if (!is_error_pfn(pfn))
                kvm_release_pfn_clean(pfn);
 }

+static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+                             u64 *spte, const void *pte)
+{
+       pt_element_t gpte = *(const pt_element_t *)pte;
+
+       return FNAME(prefetch_gpte)(vcpu, sp, spte, gpte, false);
+}
+
 static bool FNAME(gpte_changed)(struct kvm_vcpu *vcpu,
                                struct guest_walker *gw, int level)
 {
@@ -428,35 +438,13 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
        spte = sp->spt + i;

        for (i = 0; i < PTE_PREFETCH_NUM; i++, spte++) {
-               pt_element_t gpte;
-               unsigned pte_access;
-               gfn_t gfn;
-               pfn_t pfn;
-
                if (spte == sptep)
                        continue;

                if (is_shadow_present_pte(*spte))
                        continue;

-               gpte = gptep[i];
-
-               if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
-                       continue;
-
-               pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte,
-                                                                 true);
-               gfn = gpte_to_gfn(gpte);
-               pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
-                                     pte_access & ACC_WRITE_MASK);
-               if (mmu_invalid_pfn(pfn))
-                       break;
-
-               mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-                            NULL, PT_PAGE_TABLE_LEVEL, gfn,
-                            pfn, true, true);
-               if (!is_error_pfn(pfn))
-                       kvm_release_pfn_clean(pfn);
+               return FNAME(prefetch_gpte)(vcpu, sp, spte, gptep[i], true);
        }
 }

--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Let it return emulate state instead of spte like __direct_map

Signed-off-by: Xiao Guangrong <xiaoguangr...@linux.vnet.ibm.com>
---
 arch/x86/kvm/paging_tmpl.h |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 92f466c..0adf376 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -463,20 +463,18 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
 /*
  * Fetch a shadow pte for a specific level in the paging hierarchy.
  */
-static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
+static int FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
                         struct guest_walker *gw,
                         int user_fault, int write_fault, int hlevel,
-                        int *emulate, pfn_t pfn, bool map_writable,
-                        bool prefault)
+                        pfn_t pfn, bool map_writable, bool prefault)
 {
-       unsigned access = gw->pt_access;
        struct kvm_mmu_page *sp = NULL;
-       int top_level;
-       unsigned direct_access;
        struct kvm_shadow_walk_iterator it;
+       unsigned direct_access, access = gw->pt_access;
+       int top_level, emulate = 0;

        if (!is_present_gpte(gw->ptes[gw->level - 1]))
-               return NULL;
+               return 0;

        direct_access = gw->pte_access;

@@ -540,17 +538,17 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,

        clear_sp_write_flooding_count(it.sptep);
        mmu_set_spte(vcpu, it.sptep, access, gw->pte_access,
-                    user_fault, write_fault, emulate, it.level,
+                    user_fault, write_fault, &emulate, it.level,
                     gw->gfn, pfn, prefault, map_writable);
        FNAME(pte_prefetch)(vcpu, gw, it.sptep);

-       return it.sptep;
+       return emulate;

 out_gpte_changed:
        if (sp)
                kvm_mmu_put_page(sp, it.sptep);

-       return NULL;
+       return 0;
 }

 /*
@@ -573,8 +571,6 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
        int write_fault = error_code & PFERR_WRITE_MASK;
        int user_fault = error_code & PFERR_USER_MASK;
        struct guest_walker walker;
-       u64 *sptep;
-       int emulate = 0;
        int r;
        pfn_t pfn;
        int level = PT_PAGE_TABLE_LEVEL;
@@ -638,15 +634,11 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
        kvm_mmu_free_some_pages(vcpu);
        if (!force_pt_level)
                transparent_hugepage_adjust(vcpu, &walker.gfn, &pfn, &level);
-       sptep = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
-                            level, &emulate, pfn, map_writable, prefault);
-       (void)sptep;
-       pgprintk("%s: shadow pte %p %llx emulate %d\n", __func__,
-                sptep, *sptep, emulate);
+       r = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
+                        level, pfn, map_writable, prefault);

        ++vcpu->stat.pf_fixed;
        kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
-       r = emulate;

 out_unlock:
        spin_unlock(&vcpu->kvm->mmu_lock);
--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

We can not directly call kvm_release_pfn_clean to release the pfn
since we can meet noslot pfn which is used to cache mmio info into
spte

Signed-off-by: Xiao Guangrong <xiaoguangr...@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c         |    6 ++++--
 arch/x86/kvm/paging_tmpl.h |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index aa0b469..f74c63a 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2877,7 +2877,8 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,

 out_unlock:
        spin_unlock(&vcpu->kvm->mmu_lock);
-       kvm_release_pfn_clean(pfn);
+       if (!is_error_pfn(pfn))
+               kvm_release_pfn_clean(pfn);
        return 0;
 }

@@ -3345,7 +3346,8 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,

 out_unlock:
        spin_unlock(&vcpu->kvm->mmu_lock);
-       kvm_release_pfn_clean(pfn);
+       if (!is_error_pfn(pfn))
+               kvm_release_pfn_clean(pfn);
        return 0;
 }

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index bf8c42b..c004ab6 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -544,7 +544,8 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 out_gpte_changed:
        if (sp)
                kvm_mmu_put_page(sp, it.sptep);
-       kvm_release_pfn_clean(pfn);
+       if (!is_error_pfn(pfn))
+               kvm_release_pfn_clean(pfn);
        return NULL;
 }

@@ -645,7 +646,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,

 out_unlock:
        spin_unlock(&vcpu->kvm->mmu_lock);
-       kvm_release_pfn_clean(pfn);
+       if (!is_error_pfn(pfn))
+               kvm_release_pfn_clean(pfn);
        return 0;
 }

--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Wrap the common operations into these two functions

Signed-off-by: Xiao Guangrong <xiaoguangr...@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c         |   53 +++++++++++++++++++++++++++----------------
 arch/x86/kvm/paging_tmpl.h |   16 +++++--------
 2 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 29ce28b..7e7b8cd 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2825,6 +2825,29 @@ exit:
 static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
                         gva_t gva, pfn_t *pfn, bool write, bool *writable);

+static bool
+page_fault_start(struct kvm_vcpu *vcpu, gfn_t *gfnp, pfn_t *pfnp, int *levelp,
+                bool force_pt_level, unsigned long mmu_seq)
+{
+       spin_lock(&vcpu->kvm->mmu_lock);
+       if (mmu_notifier_retry(vcpu, mmu_seq))
+               return false;
+
+       kvm_mmu_free_some_pages(vcpu);
+       if (likely(!force_pt_level))
+               transparent_hugepage_adjust(vcpu, gfnp, pfnp, levelp);
+
+       return true;
+}
+
+static void page_fault_end(struct kvm_vcpu *vcpu, pfn_t pfn)
+{
+       spin_unlock(&vcpu->kvm->mmu_lock);
+
+       if (!is_error_pfn(pfn))
+               kvm_release_pfn_clean(pfn);
+}
+
 static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
                         gfn_t gfn, bool prefault)
 {
@@ -2862,22 +2885,17 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,
        if (handle_abnormal_pfn(vcpu, v, gfn, pfn, ACC_ALL, &r))
                return r;

-       spin_lock(&vcpu->kvm->mmu_lock);
-       if (mmu_notifier_retry(vcpu, mmu_seq)) {
+       if (!page_fault_start(vcpu, &gfn, &pfn, &level, force_pt_level,
+             mmu_seq)) {
                r = 0;
-               goto out_unlock;
+               goto exit;
        }

-       kvm_mmu_free_some_pages(vcpu);
-       if (likely(!force_pt_level))
-               transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
        r = __direct_map(vcpu, v, write, map_writable, level, gfn, pfn,
                         prefault);

-out_unlock:
-       spin_unlock(&vcpu->kvm->mmu_lock);
-       if (!is_error_pfn(pfn))
-               kvm_release_pfn_clean(pfn);
+exit:
+       page_fault_end(vcpu, pfn);
        return r;
 }

@@ -3331,22 +3349,17 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,
        if (handle_abnormal_pfn(vcpu, 0, gfn, pfn, ACC_ALL, &r))
                return r;

-       spin_lock(&vcpu->kvm->mmu_lock);
-       if (mmu_notifier_retry(vcpu, mmu_seq)) {
+       if (!page_fault_start(vcpu, &gfn, &pfn, &level, force_pt_level,
+             mmu_seq)) {
                r = 0;
-               goto out_unlock;
+               goto exit;
        }

-       kvm_mmu_free_some_pages(vcpu);
-       if (likely(!force_pt_level))
-               transparent_hugepage_adjust(vcpu, &gfn, &pfn, &level);
        r = __direct_map(vcpu, gpa, write, map_writable,
                         level, gfn, pfn, prefault);

-out_unlock:
-       spin_unlock(&vcpu->kvm->mmu_lock);
-       if (!is_error_pfn(pfn))
-               kvm_release_pfn_clean(pfn);
+exit:
+       page_fault_end(vcpu, pfn);
        return r;
 }

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 0adf376..1a738c5 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -624,26 +624,22 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,
                                walker.gfn, pfn, walker.pte_access, &r))
                return r;

-       spin_lock(&vcpu->kvm->mmu_lock);
-       if (mmu_notifier_retry(vcpu, mmu_seq)) {
+       if (!page_fault_start(vcpu, &walker.gfn, &pfn, &level,
+             force_pt_level, mmu_seq)) {
                r = 0;
-               goto out_unlock;
+               goto exit;
        }

        kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
-       kvm_mmu_free_some_pages(vcpu);
-       if (!force_pt_level)
-               transparent_hugepage_adjust(vcpu, &walker.gfn, &pfn, &level);
+
        r = FNAME(fetch)(vcpu, addr, &walker, user_fault, write_fault,
                         level, pfn, map_writable, prefault);

        ++vcpu->stat.pf_fixed;
        kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);

-out_unlock:
-       spin_unlock(&vcpu->kvm->mmu_lock);
-       if (!is_error_pfn(pfn))
-               kvm_release_pfn_clean(pfn);
+exit:
+       page_fault_end(vcpu, pfn);
        return r;
 }

--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Release pfn in the mmu code is little special for we allow no-slot pfn
go to spte walk on page fault path, that means, on page fault fail path,
we can not directly call kvm_release_pfn_clean.

This patchset fixes the bug which release no-slot pfn on fail path and
clean up all the paths where kvm_release_pfn_clean is called

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 09/14/2012 05:59 PM, Xiao Guangrong wrote:

Sorry, this was wrong. Update this patch.

[PATCH v2 5/5] KVM: MMU: introduce FNAME(prefetch_gpte)

The only different thing between FNAME(update_pte) and FNAME(pte_prefetch)
is that the former is allowed to prefetch gfn from dirty logged slot, so
introduce a common function to prefetch spte

Signed-off-by: Xiao Guangrong <xiaoguangr...@linux.vnet.ibm.com>
---
 arch/x86/kvm/paging_tmpl.h |   50 ++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1a738c5..32facf7 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -356,35 +356,45 @@ no_present:
        return true;
 }

-static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
-                             u64 *spte, const void *pte)
+static void
+FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+                    u64 *spte, pt_element_t gpte, bool no_dirty_log)
 {
-       pt_element_t gpte;
        unsigned pte_access;
+       gfn_t gfn;
        pfn_t pfn;

-       gpte = *(const pt_element_t *)pte;
        if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
                return;

        pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
+
+       gfn = gpte_to_gfn(gpte);
        pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte, true);
-       pfn = gfn_to_pfn_atomic(vcpu->kvm, gpte_to_gfn(gpte));
+       pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
+                       no_dirty_log && (pte_access & ACC_WRITE_MASK));
        if (mmu_invalid_pfn(pfn))
                return;

        /*
         * we call mmu_set_spte() with host_writable = true because that
-        * vcpu->arch.update_pte.pfn was fetched from get_user_pages(write = 1).
+        * pte_prefetch_gfn_to_pfn always gets a writable pfn.
         */
        mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-                    NULL, PT_PAGE_TABLE_LEVEL,
-                    gpte_to_gfn(gpte), pfn, true, true);
+                    NULL, PT_PAGE_TABLE_LEVEL, gfn, pfn, true, true);

        if (!is_error_pfn(pfn))
                kvm_release_pfn_clean(pfn);
 }

+static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
+                             u64 *spte, const void *pte)
+{
+       pt_element_t gpte = *(const pt_element_t *)pte;
+
+       return FNAME(prefetch_gpte)(vcpu, sp, spte, gpte, false);
+}
+
 static bool FNAME(gpte_changed)(struct kvm_vcpu *vcpu,
                                struct guest_walker *gw, int level)
 {
@@ -428,35 +438,13 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
        spte = sp->spt + i;

        for (i = 0; i < PTE_PREFETCH_NUM; i++, spte++) {
-               pt_element_t gpte;
-               unsigned pte_access;
-               gfn_t gfn;
-               pfn_t pfn;
-
                if (spte == sptep)
                        continue;

                if (is_shadow_present_pte(*spte))
                        continue;

-               gpte = gptep[i];
-
-               if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
-                       continue;
-
-               pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte,
-                                                                 true);
-               gfn = gpte_to_gfn(gpte);
-               pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
-                                     pte_access & ACC_WRITE_MASK);
-               if (mmu_invalid_pfn(pfn))
-                       break;
-
-               mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
-                            NULL, PT_PAGE_TABLE_LEVEL, gfn,
-                            pfn, true, true);
-               if (!is_error_pfn(pfn))
-                       kvm_release_pfn_clean(pfn);
+               FNAME(prefetch_gpte)(vcpu, sp, spte, gptep[i], true);
        }
 }

--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Why? I think people are used to

spin_lock(lock)
sequence
spin_unlock(lock)

So its easy to verify whether access to data structures are protected.

Unrelated to this patch, one opportunity i see to simplify this
code is:

- error pfn / mmio pfn / invalid pfn relation

Have the meaning of this bits unified in a single function/helper, see
comment to patch 1 (perhaps you can further improve).

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

IMHO, for the human reader, the meaning of the two functions is
different and therefore separation is justified. Moreover they already
share common code via FNAME(prefetch_invalid_gpte) (which BTW, is a
confusing name because the function does not prefetch gpte, it validates
gpte).

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Its clearer to the reader if is_invalid_pfn() is used instead of
is_error_pfn.

BTW how about killing this unused helper

static bool mmu_invalid_pfn(pfn_t pfn)
{
        return unlikely(is_invalid_pfn(pfn));

This can be done inlined.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 09/15/2012 11:13 PM, Marcelo Tosatti wrote:

Okay, will do. Thanks Marcelo!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 09/15/2012 11:25 PM, Marcelo Tosatti wrote:

Marcelo,

There are many functions use this style that wrap the lock into the
_start and _end functions in kernel (eg.: cgroup_pidlist_start and
cgroup_pidlist_stop in kernel/cgroup.c).

Actually, i just wanted to remove below duplicate ugly code:

        if (!is_error_pfn(pfn))
                kvm_release_pfn_clean(pfn);

Sorry, more detail?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 09/15/2012 11:31 PM, Marcelo Tosatti wrote:

Actually, these two functions do the same things, both of them prefetch
spte based on gpte. The only different is, in the case of update_pte, we
have high opportunity that the spte can be accessed soon (that why it is
allowed to prefetch dirty logged gfn).

I think it is not too bad for it not just validates gpte, it also can drop
spte if the gpte is invalid, it is also the behaviour that modify spte based
on gpte. ;)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Should force the reader of the code to understand error pfn / mmio pfn /
invalid pfn in a single helper. That is, avoid using error pfn at all.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 09/19/2012 07:43 AM, Marcelo Tosatti wrote:

Reasonable suggestion, i will think about it more. Thank you, Marcelo!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 09/14/2012 12:58 PM, Xiao Guangrong wrote:

Please document the return value in the comment.

--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 09/14/2012 12:59 PM, Xiao Guangrong wrote:

Needs sparse annotations (__acquires, __releases).

These code blocks have nothing in common except for being shared.  Often
that's not good for maintainability because it means that further
changes can affect one path but not the other.  But we can try it out
and see.

--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/