| Message ID | 1381212212-29641-4-git-send-email-Bharat.Bhushan@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2013-10-08 at 11:33 +0530, Bharat Bhushan wrote: > We need to search linux "pte" to get "pte" attributes for > setting TLB in KVM. > This patch defines a linux_pte_lookup() function for same. > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> > --- > arch/powerpc/include/asm/pgtable.h | 35 +++++++++++++++++++++++++++++++++++ > 1 files changed, 35 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h > index 7d6eacf..fd26c04 100644 > --- a/arch/powerpc/include/asm/pgtable.h > +++ b/arch/powerpc/include/asm/pgtable.h > @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, > #endif > pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, > unsigned *shift); > + > +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva, > + unsigned long *pte_sizep) > +{ > + pte_t *ptep; > + pte_t pte; > + unsigned long ps = *pte_sizep; > + unsigned int shift; > + > + ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift); > + if (!ptep) > + return __pte(0); > + if (shift) > + *pte_sizep = 1ul << shift; > + else > + *pte_sizep = PAGE_SIZE; > + > + if (ps > *pte_sizep) > + return __pte(0); > + > + /* wait until _PAGE_BUSY is clear */ > + while (1) { > + pte = pte_val(*ptep); > + if (unlikely(pte & _PAGE_BUSY)) { > + cpu_relax(); > + continue; > + } > + } > + > + /* If pte is not present return None */ > + if (unlikely(!(pte & _PAGE_PRESENT))) > + return __pte(0); > + > + return pte; > +} Can lookup_linux_pte_and_update() call lookup_linux_pte()? -Scott -- 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
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, October 09, 2013 3:07 AM > To: Bhushan Bharat-R65777 > Cc: agraf@suse.de; Yoder Stuart-B08248; kvm@vger.kernel.org; kvm- > ppc@vger.kernel.org; paulus@samba.org; Bhushan Bharat-R65777 > Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function > > On Tue, 2013-10-08 at 11:33 +0530, Bharat Bhushan wrote: > > We need to search linux "pte" to get "pte" attributes for setting TLB > > in KVM. > > This patch defines a linux_pte_lookup() function for same. > > > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> > > --- > > arch/powerpc/include/asm/pgtable.h | 35 +++++++++++++++++++++++++++++++++++ > > 1 files changed, 35 insertions(+), 0 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/pgtable.h > > b/arch/powerpc/include/asm/pgtable.h > > index 7d6eacf..fd26c04 100644 > > --- a/arch/powerpc/include/asm/pgtable.h > > +++ b/arch/powerpc/include/asm/pgtable.h > > @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long > > sz, unsigned long addr, #endif pte_t > > *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, > > unsigned *shift); > > + > > +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva, > > + unsigned long *pte_sizep) > > +{ > > + pte_t *ptep; > > + pte_t pte; > > + unsigned long ps = *pte_sizep; > > + unsigned int shift; > > + > > + ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift); > > + if (!ptep) > > + return __pte(0); > > + if (shift) > > + *pte_sizep = 1ul << shift; > > + else > > + *pte_sizep = PAGE_SIZE; > > + > > + if (ps > *pte_sizep) > > + return __pte(0); > > + > > + /* wait until _PAGE_BUSY is clear */ > > + while (1) { > > + pte = pte_val(*ptep); > > + if (unlikely(pte & _PAGE_BUSY)) { > > + cpu_relax(); > > + continue; > > + } > > + } > > + > > + /* If pte is not present return None */ > > + if (unlikely(!(pte & _PAGE_PRESENT))) > > + return __pte(0); > > + > > + return pte; > > +} > > Can lookup_linux_pte_and_update() call lookup_linux_pte()? What lookup_linux_pte_and_update() does:- - find_linux_pte_or_hugepte() - does size and some other trivial checks - Then atomically update the pte:- => while() => wait till _PAGE_BUSY is clear => atomically update the pte => if not updated then go back to while() above else break While what lookup_linux_pte() does:- - find_linux_pte_or_hugepte() - does size and some other trivial checks - wait till _PAGE_BUSY is clear - return pte I am finding it difficult to call lookup_linux_pte() from lookup_linux_pte_and_update(). Thanks -Bharat > > -Scott >
On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote: > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Wednesday, October 09, 2013 3:07 AM > > To: Bhushan Bharat-R65777 > > Cc: agraf@suse.de; Yoder Stuart-B08248; kvm@vger.kernel.org; kvm- > > ppc@vger.kernel.org; paulus@samba.org; Bhushan Bharat-R65777 > > Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function > > > > On Tue, 2013-10-08 at 11:33 +0530, Bharat Bhushan wrote: > > > We need to search linux "pte" to get "pte" attributes for setting TLB > > > in KVM. > > > This patch defines a linux_pte_lookup() function for same. > > > > > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> > > > --- > > > arch/powerpc/include/asm/pgtable.h | 35 +++++++++++++++++++++++++++++++++++ > > > 1 files changed, 35 insertions(+), 0 deletions(-) > > > > > > diff --git a/arch/powerpc/include/asm/pgtable.h > > > b/arch/powerpc/include/asm/pgtable.h > > > index 7d6eacf..fd26c04 100644 > > > --- a/arch/powerpc/include/asm/pgtable.h > > > +++ b/arch/powerpc/include/asm/pgtable.h > > > @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long > > > sz, unsigned long addr, #endif pte_t > > > *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, > > > unsigned *shift); > > > + > > > +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva, > > > + unsigned long *pte_sizep) > > > +{ > > > + pte_t *ptep; > > > + pte_t pte; > > > + unsigned long ps = *pte_sizep; > > > + unsigned int shift; > > > + > > > + ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift); > > > + if (!ptep) > > > + return __pte(0); > > > + if (shift) > > > + *pte_sizep = 1ul << shift; > > > + else > > > + *pte_sizep = PAGE_SIZE; > > > + > > > + if (ps > *pte_sizep) > > > + return __pte(0); > > > + > > > + /* wait until _PAGE_BUSY is clear */ > > > + while (1) { > > > + pte = pte_val(*ptep); > > > + if (unlikely(pte & _PAGE_BUSY)) { > > > + cpu_relax(); > > > + continue; > > > + } > > > + } > > > + > > > + /* If pte is not present return None */ > > > + if (unlikely(!(pte & _PAGE_PRESENT))) > > > + return __pte(0); > > > + > > > + return pte; > > > +} > > > > Can lookup_linux_pte_and_update() call lookup_linux_pte()? > > What lookup_linux_pte_and_update() does:- > - find_linux_pte_or_hugepte() > - does size and some other trivial checks > - Then atomically update the pte:- > => while() > => wait till _PAGE_BUSY is clear > => atomically update the pte > => if not updated then go back to while() above else break > > > While what lookup_linux_pte() does:- > - find_linux_pte_or_hugepte() > - does size and some other trivial checks > - wait till _PAGE_BUSY is clear > - return pte > > I am finding it difficult to call lookup_linux_pte() from lookup_linux_pte_and_update(). You could factor out a common lookup_linux_ptep(). -Scott -- 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
On Tue, Oct 08, 2013 at 11:33:31AM +0530, Bharat Bhushan wrote: > We need to search linux "pte" to get "pte" attributes for > setting TLB in KVM. > This patch defines a linux_pte_lookup() function for same. > [snip] > + /* wait until _PAGE_BUSY is clear */ > + while (1) { > + pte = pte_val(*ptep); > + if (unlikely(pte & _PAGE_BUSY)) { > + cpu_relax(); > + continue; > + } > + } > + > + /* If pte is not present return None */ > + if (unlikely(!(pte & _PAGE_PRESENT))) > + return __pte(0); First, this looks racy to me, since nothing stops the compiler refetching pte from memory, and it might have become busy again in the meantime. However, I don't think you need to do any of this, since the caller in your next patch checks for pte_present(pte) anyway. On book E systems we don't use _PAGE_BUSY, so you don't need the loop for your intended application. I suggest you remove the entire section quoted above. Paul. -- 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
On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote: > On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote: > > > > What lookup_linux_pte_and_update() does:- > > - find_linux_pte_or_hugepte() > > - does size and some other trivial checks > > - Then atomically update the pte:- > > => while() > > => wait till _PAGE_BUSY is clear > > => atomically update the pte > > => if not updated then go back to while() above else break > > > > > > While what lookup_linux_pte() does:- > > - find_linux_pte_or_hugepte() > > - does size and some other trivial checks > > - wait till _PAGE_BUSY is clear > > - return pte > > > > I am finding it difficult to call lookup_linux_pte() from lookup_linux_pte_and_update(). > > You could factor out a common lookup_linux_ptep(). I don't really think it's enough code to be worth wringing out the last drop of duplication. However, if he removed the checks for _PAGE_BUSY and _PAGE_PRESENT as I suggested in another mail, and made it return the pte pointer rather than the value, it would then essentially be a lookup_linux_ptep() as you suggest. Paul. -- 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
> -----Original Message----- > From: Paul Mackerras [mailto:paulus@samba.org] > Sent: Thursday, October 10, 2013 4:06 PM > To: Wood Scott-B07421 > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; agraf@suse.de; Yoder Stuart- > B08248; kvm@vger.kernel.org; kvm-ppc@vger.kernel.org > Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function > > On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote: > > On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote: > > > > > > What lookup_linux_pte_and_update() does:- > > > - find_linux_pte_or_hugepte() > > > - does size and some other trivial checks > > > - Then atomically update the pte:- > > > => while() > > > => wait till _PAGE_BUSY is clear > > > => atomically update the pte > > > => if not updated then go back to while() above else break > > > > > > > > > While what lookup_linux_pte() does:- > > > - find_linux_pte_or_hugepte() > > > - does size and some other trivial checks > > > - wait till _PAGE_BUSY is clear > > > - return pte > > > > > > I am finding it difficult to call lookup_linux_pte() from > lookup_linux_pte_and_update(). > > > > You could factor out a common lookup_linux_ptep(). > > I don't really think it's enough code to be worth wringing out the last drop of > duplication. However, if he removed the checks for _PAGE_BUSY and _PAGE_PRESENT > as I suggested in another mail, and made it return the pte pointer rather than > the value, it would then essentially be a lookup_linux_ptep() as you suggest. Do we want to have lookup_linux_pte() or lookup_linux_ptep() or both where lookup_linux_pte() and lookup_linux_pte_and_update() calls lookup_linux_ptep() ? -Bharat > > Paul. -- 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
On Thu, Oct 10, 2013 at 10:44:54AM +0000, Bhushan Bharat-R65777 wrote: > > > > -----Original Message----- > > From: Paul Mackerras [mailto:paulus@samba.org] > > Sent: Thursday, October 10, 2013 4:06 PM > > To: Wood Scott-B07421 > > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; agraf@suse.de; Yoder Stuart- > > B08248; kvm@vger.kernel.org; kvm-ppc@vger.kernel.org > > Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function > > > > On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote: > > > On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote: > > > > > > > > What lookup_linux_pte_and_update() does:- > > > > - find_linux_pte_or_hugepte() > > > > - does size and some other trivial checks > > > > - Then atomically update the pte:- > > > > => while() > > > > => wait till _PAGE_BUSY is clear > > > > => atomically update the pte > > > > => if not updated then go back to while() above else break > > > > > > > > > > > > While what lookup_linux_pte() does:- > > > > - find_linux_pte_or_hugepte() > > > > - does size and some other trivial checks > > > > - wait till _PAGE_BUSY is clear > > > > - return pte > > > > > > > > I am finding it difficult to call lookup_linux_pte() from > > lookup_linux_pte_and_update(). > > > > > > You could factor out a common lookup_linux_ptep(). > > > > I don't really think it's enough code to be worth wringing out the last drop of > > duplication. However, if he removed the checks for _PAGE_BUSY and _PAGE_PRESENT > > as I suggested in another mail, and made it return the pte pointer rather than > > the value, it would then essentially be a lookup_linux_ptep() as you suggest. > > Do we want to have lookup_linux_pte() or lookup_linux_ptep() or both where lookup_linux_pte() and lookup_linux_pte_and_update() calls lookup_linux_ptep() ? I don't think we want both lookup_linux_pte() and lookup_linux_ptep(). Either have a lookup_linux_ptep() and call it from your book E code and from lookup_linux_pte_and_update(), or keep a separate lookup_linux_pte() and don't make the _and_update() version call it. I don't really care; you decide which looks nicer in the end. Of course someone will then tell you to do it the other way, but at that point it's just a matter of taste. Paul. -- 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
Bharat Bhushan <r65777@freescale.com> writes: > We need to search linux "pte" to get "pte" attributes for > setting TLB in KVM. > This patch defines a linux_pte_lookup() function for same. > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> > --- > arch/powerpc/include/asm/pgtable.h | 35 +++++++++++++++++++++++++++++++++++ > 1 files changed, 35 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h > index 7d6eacf..fd26c04 100644 > --- a/arch/powerpc/include/asm/pgtable.h > +++ b/arch/powerpc/include/asm/pgtable.h > @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, > #endif > pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, > unsigned *shift); > + > +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva, > + unsigned long *pte_sizep) > +{ > + pte_t *ptep; > + pte_t pte; > + unsigned long ps = *pte_sizep; > + unsigned int shift; > + > + ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift); > + if (!ptep) > + return __pte(0); > + if (shift) > + *pte_sizep = 1ul << shift; > + else > + *pte_sizep = PAGE_SIZE; > + > + if (ps > *pte_sizep) > + return __pte(0); > + > + /* wait until _PAGE_BUSY is clear */ > + while (1) { > + pte = pte_val(*ptep); > + if (unlikely(pte & _PAGE_BUSY)) { > + cpu_relax(); > + continue; > + } What if we find a THP page that is splitting ? Older function returned __pte(0) in that case. > + } > + > + /* If pte is not present return None */ > + if (unlikely(!(pte & _PAGE_PRESENT))) > + return __pte(0); > + > + return pte; > +} > #endif /* __ASSEMBLY__ */ > > #endif /* __KERNEL__ */ > -- -aneesh -- 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
Paul Mackerras <paulus@samba.org> writes: > On Wed, Oct 09, 2013 at 12:47:31PM -0500, Scott Wood wrote: >> On Wed, 2013-10-09 at 03:48 -0500, Bhushan Bharat-R65777 wrote: >> > >> > What lookup_linux_pte_and_update() does:- >> > - find_linux_pte_or_hugepte() >> > - does size and some other trivial checks >> > - Then atomically update the pte:- >> > => while() >> > => wait till _PAGE_BUSY is clear >> > => atomically update the pte >> > => if not updated then go back to while() above else break >> > >> > >> > While what lookup_linux_pte() does:- >> > - find_linux_pte_or_hugepte() >> > - does size and some other trivial checks >> > - wait till _PAGE_BUSY is clear >> > - return pte >> > >> > I am finding it difficult to call lookup_linux_pte() from lookup_linux_pte_and_update(). >> >> You could factor out a common lookup_linux_ptep(). > > I don't really think it's enough code to be worth wringing out the > last drop of duplication. However, if he removed the checks for > _PAGE_BUSY and _PAGE_PRESENT as I suggested in another mail, and made > it return the pte pointer rather than the value, it would then > essentially be a lookup_linux_ptep() as you suggest. We also need to check for _PAGE_SPLITTING before going ahead and using the pte value -aneesh -- 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/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h index 7d6eacf..fd26c04 100644 --- a/arch/powerpc/include/asm/pgtable.h +++ b/arch/powerpc/include/asm/pgtable.h @@ -223,6 +223,41 @@ extern int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, #endif pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, unsigned *shift); + +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva, + unsigned long *pte_sizep) +{ + pte_t *ptep; + pte_t pte; + unsigned long ps = *pte_sizep; + unsigned int shift; + + ptep = find_linux_pte_or_hugepte(pgdir, hva, &shift); + if (!ptep) + return __pte(0); + if (shift) + *pte_sizep = 1ul << shift; + else + *pte_sizep = PAGE_SIZE; + + if (ps > *pte_sizep) + return __pte(0); + + /* wait until _PAGE_BUSY is clear */ + while (1) { + pte = pte_val(*ptep); + if (unlikely(pte & _PAGE_BUSY)) { + cpu_relax(); + continue; + } + } + + /* If pte is not present return None */ + if (unlikely(!(pte & _PAGE_PRESENT))) + return __pte(0); + + return pte; +} #endif /* __ASSEMBLY__ */ #endif /* __KERNEL__ */
We need to search linux "pte" to get "pte" attributes for setting TLB in KVM. This patch defines a linux_pte_lookup() function for same. Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> --- arch/powerpc/include/asm/pgtable.h | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)