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

[Avi Kivity <avi@redhat.com>]

On 05/04/2011 02:58 PM, Marcelo Tosatti wrote:
> On Wed, May 04, 2011 at 02:44:47PM +0300, Avi Kivity wrote:
> >  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.
>
> Well, he is removing the second translation. So that is skipped.

hva->gpa translation is not supposed to be changed by kvm.

> >  The motivation isn't performance, it's to ensure that cmpxchg_gpte()
> >  operates on the same address as we read it from.
>
> OK, my objection is direct get_user_pages_fast usage. Please pass gfn to
> gfn_to_page.

We do get_user() in read_gpte().  That is equivalent to 
get_user_pages().  So we already broke that layer of abstraction.

> >  (btw, we're missing a mark_page_dirty() here, no?)
>
> No, see line 245.

Ah, yes.  Thanks.

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