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@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx>
> >> ---
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
