From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/1 v2] KVM: MMU: Use ptep_user for cmpxchg_gpte() Date: Wed, 04 May 2011 15:11:35 +0300 Message-ID: <4DC14277.4090006@redhat.com> References: <20110501143026.9eb3c875.takuya.yoshikawa@gmail.com> <20110501143307.1bcfd375.takuya.yoshikawa@gmail.com> <20110504111611.GA26538@amt.cnet> <4DC13C2F.4090402@redhat.com> <20110504115827.GA28957@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Takuya Yoshikawa , kvm@vger.kernel.org, yoshikawa.takuya@oss.ntt.co.jp To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:50056 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754529Ab1EDMLl (ORCPT ); Wed, 4 May 2011 08:11:41 -0400 In-Reply-To: <20110504115827.GA28957@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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 > > >> > > >> 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 > > >> --- > > >> 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