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 14:44:47 +0300 Message-ID: <4DC13C2F.4090402@redhat.com> References: <20110501143026.9eb3c875.takuya.yoshikawa@gmail.com> <20110501143307.1bcfd375.takuya.yoshikawa@gmail.com> <20110504111611.GA26538@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]:32101 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753777Ab1EDLox (ORCPT ); Wed, 4 May 2011 07:44:53 -0400 In-Reply-To: <20110504111611.GA26538@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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