On Fri, Nov 12, 2010 at 06:35:38PM +0800, Xiao Guangrong wrote:
> Some operation of these functions is very similar, so introduce a
> common function to cleanup them
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxxxxx>
> ---
> arch/x86/kvm/mmu.c | 3 -
> arch/x86/kvm/paging_tmpl.h | 191 ++++++++++++++++++++++++-------------------
> 2 files changed, 107 insertions(+), 87 deletions(-)
This makes the code more complicated and error prone IMO, because there
are specialities of
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 94d157f..d0bcca2 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3108,9 +3108,6 @@ static void mmu_pte_write_new_pte(struct kvm_vcpu *vcpu,
> return;
> }
>
> - if (is_rsvd_bits_set(&vcpu->arch.mmu, *(u64 *)new, PT_PAGE_TABLE_LEVEL))
> - return;
> -
> ++vcpu->kvm->stat.mmu_pte_updated;
> if (!sp->role.cr4_pae)
> paging32_update_pte(vcpu, sp, spte, new);
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 952357a..1a1a0b9 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -299,42 +299,90 @@ static int FNAME(walk_addr_nested)(struct guest_walker *walker,
> addr, access);
> }
>
> -static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
> - u64 *spte, const void *pte)
> +static bool FNAME(fetch_guest_pte)(struct kvm_vcpu *vcpu,
> + struct kvm_mmu_page *sp, u64 *spte,
> + bool clear_unsync, pt_element_t gpte,
> + pfn_t (get_pfn)(struct kvm_vcpu *, u64 *,
> + pt_element_t, unsigned, bool *))
> {
> - pt_element_t gpte;
> unsigned pte_access;
> + u64 nonpresent = shadow_trap_nonpresent_pte;
> + gfn_t gfn;
> pfn_t pfn;
> - u64 new_spte;
> + bool dirty, host_writeable;
>
> - gpte = *(const pt_element_t *)pte;
> - if (~gpte & (PT_PRESENT_MASK | PT_ACCESSED_MASK)) {
> - if (!is_present_gpte(gpte)) {
> - if (sp->unsync)
> - new_spte = shadow_trap_nonpresent_pte;
> - else
> - new_spte = shadow_notrap_nonpresent_pte;
> - __set_spte(spte, new_spte);
> - }
> - return;
> + if (!is_present_gpte(gpte) ||
> + is_rsvd_bits_set(&vcpu->arch.mmu, gpte, PT_PAGE_TABLE_LEVEL)) {
> + if (!sp->unsync && !clear_unsync)
> + nonpresent = shadow_notrap_nonpresent_pte;
> + goto no_present;
> }
> - pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
> +
> + if (!(gpte & PT_ACCESSED_MASK))
> + goto no_present;
> +
> pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte);
> + gfn = gpte_to_gfn(gpte);
> + dirty = is_dirty_gpte(gpte);
> + pfn = get_pfn(vcpu, spte, gpte, pte_access, &host_writeable);
> +
> + if (is_error_pfn(pfn))
> + goto no_present;
> +
> + if (!host_writeable)
> + pte_access &= ~ACC_WRITE_MASK;
> +
> + if (spte_to_pfn(*spte) == pfn)
> + set_spte(vcpu, spte, pte_access, 0, 0,
> + dirty, PT_PAGE_TABLE_LEVEL, gfn,
> + pfn, true, false, host_writeable);
> + else
> + mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
> + dirty, NULL, PT_PAGE_TABLE_LEVEL, gfn,
> + pfn, true, host_writeable);
For example, the update path should always go through mmu_set_spte to
update last_pte_updated, last_pte_gfn.
Also the callbacks make it harder to read the code. Maybe the
unification works if you use common functions for common parts.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
