Re: [PATCH 2/3] KVM: MMU: Introduce a helper to read guest pte

[Avi Kivity <avi@redhat.com>]

On 04/20/2011 12:35 PM, Roedel, Joerg wrote:
> On Wed, Apr 20, 2011 at 05:07:12AM -0400, Avi Kivity wrote:
> >  On 04/18/2011 09:34 PM, Takuya Yoshikawa wrote:
> >  >  From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
> >  >
> >  >  This will be optimized later.
> >  >
> >  >  Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
> >  >  ---
> >  >    arch/x86/kvm/paging_tmpl.h |   12 +++++++++---
> >  >    1 files changed, 9 insertions(+), 3 deletions(-)
> >  >
> >  >  diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> >  >  index 74f8567..109939a 100644
> >  >  --- a/arch/x86/kvm/paging_tmpl.h
> >  >  +++ b/arch/x86/kvm/paging_tmpl.h
> >  >  @@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte)
> >  >    	return access;
> >  >    }
> >  >
> >  >  +static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> >  >  +				 gfn_t table_gfn, int offset, pt_element_t *ptep)
> >  >  +{
> >  >  +	return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep,
> >  >  +				       offset, sizeof(*ptep),
> >  >  +				       PFERR_USER_MASK | PFERR_WRITE_MASK);
> >  >  +}
> >  >  +
> >  >    /*
> >  >     * Fetch a guest pte for a guest virtual address
> >  >     */
> >  >  @@ -160,9 +168,7 @@ walk:
> >  >    		walker->table_gfn[walker->level - 1] = table_gfn;
> >  >    		walker->pte_gpa[walker->level - 1] = pte_gpa;
> >  >
> >  >  -		if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn,&pte,
> >  >  -					    offset, sizeof(pte),
> >  >  -					    PFERR_USER_MASK|PFERR_WRITE_MASK)) {
> >  >  +		if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset,&pte)) {
> >  >    			present = false;
> >  >    			break;
> >  >    		}
> >
> >
> >  I think it's better to avoid a separate function for this.  The reason
> >  is I'd like to use ptep_user for cmpxchg_gpte() later on in
> >  walk_addr_generic(), so we use the same calculation for both read and
> >  write.  So please just inline the new code in walk_addr_generic().
> >
> >  In fact there's probably a bug there for nested npt - we use
> >  gfn_to_page(table_gfn), but table_gfn is actually an ngfn, not a gfn.
> >  Joerg, am I right here?
>
> This patch seems only to introduce another wrapper around
> kvm_read_guest_page_mmu(), so I don't see a problem in this patch.

By patch 3, ptep_user will be computed in this function and no longer 
available for setting the accessed bit later on.

> The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or
> l2-gfn (by calling mmu->translate_gpa).

But cmpxchg_gpte() does not.

-- 
error compiling committee.c: too many arguments to function