Patchwork [3/4] kvm: powerpc: define a linux pte lookup function

login
register
mail settings
Submitter Bharat Bhushan
Date Oct. 8, 2013, 6:03 a.m.
Message ID <1381212212-29641-4-git-send-email-Bharat.Bhushan@freescale.com>
Download mbox | patch
Permalink /patch/281329/
State New
Headers show

Comments

Bharat Bhushan - Oct. 8, 2013, 6:03 a.m.
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(-)
Scott Wood - Oct. 8, 2013, 9:36 p.m.
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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan - Oct. 9, 2013, 8:48 a.m.
> -----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

>
Scott Wood - Oct. 9, 2013, 5:47 p.m.
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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - Oct. 10, 2013, 10:33 a.m.
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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - Oct. 10, 2013, 10:35 a.m.
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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan - Oct. 10, 2013, 10:44 a.m.
> -----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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - Oct. 10, 2013, 10:52 a.m.
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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aneesh Kumar K.V - Oct. 15, 2013, 5:05 a.m.
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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aneesh Kumar K.V - Oct. 15, 2013, 5:07 a.m.
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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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__ */