| Message ID | 20110419033453.ffa856c9.takuya.yoshikawa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/18/2011 09:34 PM, Takuya Yoshikawa wrote: > From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp> > > This will be optimized later. > > Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp> > --- > arch/x86/kvm/paging_tmpl.h | 12 +++++++++--- > 1 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > index 74f8567..109939a 100644 > --- a/arch/x86/kvm/paging_tmpl.h > +++ b/arch/x86/kvm/paging_tmpl.h > @@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte) > return access; > } > > +static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > + gfn_t table_gfn, int offset, pt_element_t *ptep) > +{ > + return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep, > + offset, sizeof(*ptep), > + PFERR_USER_MASK | PFERR_WRITE_MASK); > +} > + > /* > * Fetch a guest pte for a guest virtual address > */ > @@ -160,9 +168,7 @@ walk: > walker->table_gfn[walker->level - 1] = table_gfn; > walker->pte_gpa[walker->level - 1] = pte_gpa; > > - if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn,&pte, > - offset, sizeof(pte), > - PFERR_USER_MASK|PFERR_WRITE_MASK)) { > + if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset,&pte)) { > present = false; > break; > } I think it's better to avoid a separate function for this. The reason is I'd like to use ptep_user for cmpxchg_gpte() later on in walk_addr_generic(), so we use the same calculation for both read and write. So please just inline the new code in walk_addr_generic(). In fact there's probably a bug there for nested npt - we use gfn_to_page(table_gfn), but table_gfn is actually an ngfn, not a gfn. Joerg, am I right here?
On Wed, Apr 20, 2011 at 05:07:12AM -0400, Avi Kivity wrote: > On 04/18/2011 09:34 PM, Takuya Yoshikawa wrote: > > From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp> > > > > This will be optimized later. > > > > Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp> > > --- > > arch/x86/kvm/paging_tmpl.h | 12 +++++++++--- > > 1 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > > index 74f8567..109939a 100644 > > --- a/arch/x86/kvm/paging_tmpl.h > > +++ b/arch/x86/kvm/paging_tmpl.h > > @@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte) > > return access; > > } > > > > +static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > > + gfn_t table_gfn, int offset, pt_element_t *ptep) > > +{ > > + return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep, > > + offset, sizeof(*ptep), > > + PFERR_USER_MASK | PFERR_WRITE_MASK); > > +} > > + > > /* > > * Fetch a guest pte for a guest virtual address > > */ > > @@ -160,9 +168,7 @@ walk: > > walker->table_gfn[walker->level - 1] = table_gfn; > > walker->pte_gpa[walker->level - 1] = pte_gpa; > > > > - if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn,&pte, > > - offset, sizeof(pte), > > - PFERR_USER_MASK|PFERR_WRITE_MASK)) { > > + if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset,&pte)) { > > present = false; > > break; > > } > > > I think it's better to avoid a separate function for this. The reason > is I'd like to use ptep_user for cmpxchg_gpte() later on in > walk_addr_generic(), so we use the same calculation for both read and > write. So please just inline the new code in walk_addr_generic(). > > In fact there's probably a bug there for nested npt - we use > gfn_to_page(table_gfn), but table_gfn is actually an ngfn, not a gfn. > Joerg, am I right here? This patch seems only to introduce another wrapper around kvm_read_guest_page_mmu(), so I don't see a problem in this patch. The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or l2-gfn (by calling mmu->translate_gpa). Regards, Joerg
On 04/20/2011 12:35 PM, Roedel, Joerg wrote: > On Wed, Apr 20, 2011 at 05:07:12AM -0400, Avi Kivity wrote: > > On 04/18/2011 09:34 PM, Takuya Yoshikawa wrote: > > > From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp> > > > > > > This will be optimized later. > > > > > > Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp> > > > --- > > > arch/x86/kvm/paging_tmpl.h | 12 +++++++++--- > > > 1 files changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > > > index 74f8567..109939a 100644 > > > --- a/arch/x86/kvm/paging_tmpl.h > > > +++ b/arch/x86/kvm/paging_tmpl.h > > > @@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte) > > > return access; > > > } > > > > > > +static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > > > + gfn_t table_gfn, int offset, pt_element_t *ptep) > > > +{ > > > + return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep, > > > + offset, sizeof(*ptep), > > > + PFERR_USER_MASK | PFERR_WRITE_MASK); > > > +} > > > + > > > /* > > > * Fetch a guest pte for a guest virtual address > > > */ > > > @@ -160,9 +168,7 @@ walk: > > > walker->table_gfn[walker->level - 1] = table_gfn; > > > walker->pte_gpa[walker->level - 1] = pte_gpa; > > > > > > - if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn,&pte, > > > - offset, sizeof(pte), > > > - PFERR_USER_MASK|PFERR_WRITE_MASK)) { > > > + if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset,&pte)) { > > > present = false; > > > break; > > > } > > > > > > I think it's better to avoid a separate function for this. The reason > > is I'd like to use ptep_user for cmpxchg_gpte() later on in > > walk_addr_generic(), so we use the same calculation for both read and > > write. So please just inline the new code in walk_addr_generic(). > > > > In fact there's probably a bug there for nested npt - we use > > gfn_to_page(table_gfn), but table_gfn is actually an ngfn, not a gfn. > > Joerg, am I right here? > > This patch seems only to introduce another wrapper around > kvm_read_guest_page_mmu(), so I don't see a problem in this patch. By patch 3, ptep_user will be computed in this function and no longer available for setting the accessed bit later on. > The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or > l2-gfn (by calling mmu->translate_gpa). But cmpxchg_gpte() does not.
On Wed, Apr 20, 2011 at 06:05:08AM -0400, Avi Kivity wrote: > On 04/20/2011 12:35 PM, Roedel, Joerg wrote: > > This patch seems only to introduce another wrapper around > > kvm_read_guest_page_mmu(), so I don't see a problem in this patch. > > By patch 3, ptep_user will be computed in this function and no longer > available for setting the accessed bit later on. > > > The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or > > l2-gfn (by calling mmu->translate_gpa). > > But cmpxchg_gpte() does not. You are right, cmpxchg_gpte needs to handle this too. But the bug is not introduced with this patch-set it was there before. The cmpxchg_gpte function treats all table_gfns as l1-gfns. I'll send a fix soon. Regards, Joerg
On 04/20/2011 02:06 PM, Roedel, Joerg wrote: > On Wed, Apr 20, 2011 at 06:05:08AM -0400, Avi Kivity wrote: > > On 04/20/2011 12:35 PM, Roedel, Joerg wrote: > > > > This patch seems only to introduce another wrapper around > > > kvm_read_guest_page_mmu(), so I don't see a problem in this patch. > > > > By patch 3, ptep_user will be computed in this function and no longer > > available for setting the accessed bit later on. > > > > > The kvm_read_guest_page_mmu takes care whether it gets a l1-gfn or > > > l2-gfn (by calling mmu->translate_gpa). > > > > But cmpxchg_gpte() does not. > > You are right, cmpxchg_gpte needs to handle this too. But the bug is not > introduced with this patch-set it was there before. Correct. The reason I don't want the helper, is so we can use ptep_user in both places (not for efficiency, just to make sure it's exactly the same value). > The cmpxchg_gpte function treats all table_gfns as l1-gfns. I'll send a > fix soon. Thanks.
On Wed, 20 Apr 2011 14:18:12 +0300 Avi Kivity <avi@redhat.com> wrote: > Correct. The reason I don't want the helper, is so we can use ptep_user > in both places (not for efficiency, just to make sure it's exactly the > same value). > Thank you for your explanation, now I've got the picture! I will send a new patch taking into account your advice. Takuya > > The cmpxchg_gpte function treats all table_gfns as l1-gfns. I'll send a > > fix soon. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h index 74f8567..109939a 100644 --- a/arch/x86/kvm/paging_tmpl.h +++ b/arch/x86/kvm/paging_tmpl.h @@ -109,6 +109,14 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte) return access; } +static int FNAME(read_guest_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, + gfn_t table_gfn, int offset, pt_element_t *ptep) +{ + return kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, ptep, + offset, sizeof(*ptep), + PFERR_USER_MASK | PFERR_WRITE_MASK); +} + /* * Fetch a guest pte for a guest virtual address */ @@ -160,9 +168,7 @@ walk: walker->table_gfn[walker->level - 1] = table_gfn; walker->pte_gpa[walker->level - 1] = pte_gpa; - if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, &pte, - offset, sizeof(pte), - PFERR_USER_MASK|PFERR_WRITE_MASK)) { + if (FNAME(read_guest_pte)(vcpu, mmu, table_gfn, offset, &pte)) { present = false; break; }