Re: [PATCH 1/1 v2] KVM: MMU: Use ptep_user for cmpxchg_gpte()

[Avi Kivity <avi@redhat.com>]

On 05/04/2011 02:16 PM, Marcelo Tosatti wrote:
> On Sun, May 01, 2011 at 02:33:07PM +0900, Takuya Yoshikawa wrote:
> >  From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
> >
> >  The address of the gpte was already calculated and stored in ptep_user
> >  before entering cmpxchg_gpte().
> >
> >  This patch makes cmpxchg_gpte() to use that to make it clear that we
> >  are using the same address during walk_addr_generic().
> >
> >  Note that the unlikely annotations are used to show that the conditions
> >  are something unusual rather than for performance.
> >
> >  Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
> >  ---
> >   arch/x86/kvm/paging_tmpl.h |   26 ++++++++++++--------------
> >   1 files changed, 12 insertions(+), 14 deletions(-)
>
> Hi Takuya,
>
> >
> >  diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> >  index 52450a6..f9d9af1 100644
> >  --- a/arch/x86/kvm/paging_tmpl.h
> >  +++ b/arch/x86/kvm/paging_tmpl.h
> >  @@ -79,21 +79,19 @@ static gfn_t gpte_to_gfn_lvl(pt_element_t gpte, int lvl)
> >   }
> >
> >   static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> >  -			 gfn_t table_gfn, unsigned index,
> >  -			 pt_element_t orig_pte, pt_element_t new_pte)
> >  +			       pt_element_t __user *ptep_user, unsigned index,
> >  +			       pt_element_t orig_pte, pt_element_t new_pte)
> >   {
> >  +	int npages;
> >   	pt_element_t ret;
> >   	pt_element_t *table;
> >   	struct page *page;
> >  -	gpa_t gpa;
> >
> >  -	gpa = mmu->translate_gpa(vcpu, table_gfn<<  PAGE_SHIFT,
> >  -				 PFERR_USER_MASK|PFERR_WRITE_MASK);
> >  -	if (gpa == UNMAPPED_GVA)
> >  +	npages = get_user_pages_fast((unsigned long)ptep_user, 1, 1,&page);
> >  +	/* Check if the user is doing something meaningless. */
> >  +	if (unlikely(npages != 1))
> >   		return -EFAULT;
> >
> >  -	page = gfn_to_page(vcpu->kvm, gpa_to_gfn(gpa));
> >  -
>
> gfn_to_page is the interface for mapping guest pages inside KVM,
> and you're bypassing it for IMO no good reason (i doubt there's any
> performance improvement by skipping the translation).

He isn't skipping it - he's using gfn_to_hva() to derive ptep_user, 
which is equivalent.

The motivation isn't performance, it's to ensure that cmpxchg_gpte() 
operates on the same address as we read it from.

(btw, we're missing a mark_page_dirty() here, no?)

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