Re: [PATCH 5/6 v2] kvm: powerpc: booke: Add linux pte lookup like booke3s
flat view
From: Scott Wood <hidden>
Date: 2013-08-05 19:19:17
Also in:
kvm
On Mon, 2013-08-05 at 09:27 -0500, Bhushan Bharat-R65777 wrote:
quoted hunk ↗ jump to hunk
quoted
-----Original Message----- From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] Sent: Saturday, August 03, 2013 9:54 AM To: Bhushan Bharat-R65777 Cc: Wood Scott-B07421; <redacted>; <redacted>; kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 5/6 v2] kvm: powerpc: booke: Add linux pte lookup like booke3s On Sat, 2013-08-03 at 02:58 +0000, Bhushan Bharat-R65777 wrote:quoted
One of the problem I saw was that if I put this code in asm/pgtable-32.h and asm/pgtable-64.h then pte_persent() and other friend function (on which this code depends) are defined in pgtable.h. And pgtable.h includes asm/pgtable-32.h and asm/pgtable-64.h before it defines pte_present() and friends functions. Ok I move wove this in asm/pgtable*.h, initially I fought with myself to take this code in pgtable* but finally end up doing here (got biased by book3s :)).Is there a reason why these routines can not be completely generic in pgtable.h ?How about the generic function:diff --git a/arch/powerpc/include/asm/pgtable-ppc64.h b/arch/powerpc/include/asm/pgtable-ppc64.h index d257d98..21daf28 100644 --- a/arch/powerpc/include/asm/pgtable-ppc64.h +++ b/arch/powerpc/include/asm/pgtable-ppc64.h@@ -221,6 +221,27 @@ static inline unsigned long pte_update(struct mm_struct *mm, return old; } +static inline unsigned long pte_read(pte_t *p) +{ +#ifdef PTE_ATOMIC_UPDATES + pte_t pte; + pte_t tmp; + __asm__ __volatile__ ( + "1: ldarx %0,0,%3\n" + " andi. %1,%0,%4\n" + " bne- 1b\n" + " ori %1,%0,%4\n" + " stdcx. %1,0,%3\n" + " bne- 1b" + : "=&r" (pte), "=&r" (tmp), "=m" (*p) + : "r" (p), "i" (_PAGE_BUSY) + : "cc"); + + return pte; +#else + return pte_val(*p); +#endif +#endif +} static inline int __ptep_test_and_clear_young(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
Please leave a blank line between functions.
quoted hunk ↗ jump to hunk
{diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h index 690c8c2..dad712c 100644 --- a/arch/powerpc/include/asm/pgtable.h +++ b/arch/powerpc/include/asm/pgtable.h@@ -254,6 +254,45 @@ static inline pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, } #endif /* !CONFIG_HUGETLB_PAGE */ +static inline pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva, + int writing, unsigned long *pte_sizep)
The name implies that it just reads the PTE. Setting accessed/dirty shouldn't be an undocumented side-effect. Why can't the caller do that (or a different function that the caller calls afterward if desired)? Though even then you have the undocumented side effect of locking the PTE on certain targets.
quoted hunk ↗ jump to hunk
+{ + 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); + + if (!pte_present(*ptep)) + return __pte(0); + +#ifdef CONFIG_PPC64 + /* Lock PTE (set _PAGE_BUSY) and read */ + pte = pte_read(ptep); +#else + pte = pte_val(*ptep); +#endif
What about 32-bit platforms that need atomic PTEs? -Scott