Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
changed those two functions to use pte helpers to determine which
bits to clear and which bits to set.
This change was based on the assumption that bits to be set/cleared
are always the same and can be determined by applying the pte
manipulation helpers on __pte(0).
But on platforms like book3e, the bits depend on whether the page
is a user page or not.
For the time being it more or less works because of _PAGE_EXEC being
used for user pages only and exec right being set at all time on
kernel page. But following patch will clean that and output of
pte_mkexec() will depend on the page being a user or kernel page.
Instead of trying to make an even more complicated helper where bits
would become dependent on the final pte value, come back to a more
static situation like before commit 26973fa5ac0e ("powerpc/mm: use
pte helpers in generic code"), by introducing an 8xx specific
version of __ptep_set_access_flags() and ptep_set_wrprotect().
Fixes: 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
Signed-off-by: Christophe Leroy <[email protected]>
---
v3: No change
v2: New
---
arch/powerpc/include/asm/nohash/32/pgtable.h | 17 +++++++--------
arch/powerpc/include/asm/nohash/32/pte-8xx.h | 22 ++++++++++++++++++++
2 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index 34ce50da1850..11c6849f7864 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -306,30 +306,29 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
}
#define __HAVE_ARCH_PTEP_SET_WRPROTECT
+#ifndef ptep_set_wrprotect
static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr,
pte_t *ptep)
{
- unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));
- unsigned long set = pte_val(pte_wrprotect(__pte(0)));
-
- pte_update(mm, addr, ptep, clr, set, 0);
+ pte_update(mm, addr, ptep, _PAGE_RW, 0, 0);
}
+#endif
+#ifndef __ptep_set_access_flags
static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
pte_t *ptep, pte_t entry,
unsigned long address,
int psize)
{
- pte_t pte_set = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(0)))));
- pte_t pte_clr = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(~0)))));
- unsigned long set = pte_val(entry) & pte_val(pte_set);
- unsigned long clr = ~pte_val(entry) & ~pte_val(pte_clr);
+ unsigned long set = pte_val(entry) &
+ (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
int huge = psize > mmu_virtual_psize ? 1 : 0;
- pte_update(vma->vm_mm, address, ptep, clr, set, huge);
+ pte_update(vma->vm_mm, address, ptep, 0, set, huge);
flush_tlb_page(vma, address);
}
+#endif
static inline int pte_young(pte_t pte)
{
diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
index fcc48d590d88..1a89ebdc3acc 100644
--- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
@@ -136,6 +136,28 @@ static inline pte_t pte_mkhuge(pte_t pte)
#define pte_mkhuge pte_mkhuge
+static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, pte_t *p,
+ unsigned long clr, unsigned long set, int huge);
+
+static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
+{
+ pte_update(mm, addr, ptep, 0, _PAGE_RO, 0);
+}
+#define ptep_set_wrprotect ptep_set_wrprotect
+
+static inline void __ptep_set_access_flags(struct vm_area_struct *vma, pte_t *ptep,
+ pte_t entry, unsigned long address, int psize)
+{
+ unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_EXEC);
+ unsigned long clr = ~pte_val(entry) & _PAGE_RO;
+ int huge = psize > mmu_virtual_psize ? 1 : 0;
+
+ pte_update(vma->vm_mm, address, ptep, clr, set, huge);
+
+ flush_tlb_page(vma, address);
+}
+#define __ptep_set_access_flags __ptep_set_access_flags
+
static inline unsigned long pgd_leaf_size(pgd_t pgd)
{
if (pgd_val(pgd) & _PMD_PAGE_8M)
--
2.31.1
Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
> Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
> changed those two functions to use pte helpers to determine which
> bits to clear and which bits to set.
>
> This change was based on the assumption that bits to be set/cleared
> are always the same and can be determined by applying the pte
> manipulation helpers on __pte(0).
>
> But on platforms like book3e, the bits depend on whether the page
> is a user page or not.
>
> For the time being it more or less works because of _PAGE_EXEC being
> used for user pages only and exec right being set at all time on
> kernel page. But following patch will clean that and output of
> pte_mkexec() will depend on the page being a user or kernel page.
>
> Instead of trying to make an even more complicated helper where bits
> would become dependent on the final pte value, come back to a more
> static situation like before commit 26973fa5ac0e ("powerpc/mm: use
> pte helpers in generic code"), by introducing an 8xx specific
> version of __ptep_set_access_flags() and ptep_set_wrprotect().
What is this actually fixing? Does it change anything itself, or
just a preparation patch?
Thanks,
Nick
>
> Fixes: 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> v3: No change
> v2: New
> ---
> arch/powerpc/include/asm/nohash/32/pgtable.h | 17 +++++++--------
> arch/powerpc/include/asm/nohash/32/pte-8xx.h | 22 ++++++++++++++++++++
> 2 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
> index 34ce50da1850..11c6849f7864 100644
> --- a/arch/powerpc/include/asm/nohash/32/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
> @@ -306,30 +306,29 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
> }
>
> #define __HAVE_ARCH_PTEP_SET_WRPROTECT
> +#ifndef ptep_set_wrprotect
> static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep)
> {
> - unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));
> - unsigned long set = pte_val(pte_wrprotect(__pte(0)));
> -
> - pte_update(mm, addr, ptep, clr, set, 0);
> + pte_update(mm, addr, ptep, _PAGE_RW, 0, 0);
> }
> +#endif
>
> +#ifndef __ptep_set_access_flags
> static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
> pte_t *ptep, pte_t entry,
> unsigned long address,
> int psize)
> {
> - pte_t pte_set = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(0)))));
> - pte_t pte_clr = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(~0)))));
> - unsigned long set = pte_val(entry) & pte_val(pte_set);
> - unsigned long clr = ~pte_val(entry) & ~pte_val(pte_clr);
> + unsigned long set = pte_val(entry) &
> + (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
> int huge = psize > mmu_virtual_psize ? 1 : 0;
>
> - pte_update(vma->vm_mm, address, ptep, clr, set, huge);
> + pte_update(vma->vm_mm, address, ptep, 0, set, huge);
>
> flush_tlb_page(vma, address);
> }
> +#endif
>
> static inline int pte_young(pte_t pte)
> {
> diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
> index fcc48d590d88..1a89ebdc3acc 100644
> --- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
> +++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
> @@ -136,6 +136,28 @@ static inline pte_t pte_mkhuge(pte_t pte)
>
> #define pte_mkhuge pte_mkhuge
>
> +static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, pte_t *p,
> + unsigned long clr, unsigned long set, int huge);
> +
> +static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
> +{
> + pte_update(mm, addr, ptep, 0, _PAGE_RO, 0);
> +}
> +#define ptep_set_wrprotect ptep_set_wrprotect
> +
> +static inline void __ptep_set_access_flags(struct vm_area_struct *vma, pte_t *ptep,
> + pte_t entry, unsigned long address, int psize)
> +{
> + unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_EXEC);
> + unsigned long clr = ~pte_val(entry) & _PAGE_RO;
> + int huge = psize > mmu_virtual_psize ? 1 : 0;
> +
> + pte_update(vma->vm_mm, address, ptep, clr, set, huge);
> +
> + flush_tlb_page(vma, address);
> +}
> +#define __ptep_set_access_flags __ptep_set_access_flags
> +
> static inline unsigned long pgd_leaf_size(pgd_t pgd)
> {
> if (pgd_val(pgd) & _PMD_PAGE_8M)
> --
> 2.31.1
>
>
Le 27/10/2021 à 06:23, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm:
>> Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
>> changed those two functions to use pte helpers to determine which
>> bits to clear and which bits to set.
>>
>> This change was based on the assumption that bits to be set/cleared
>> are always the same and can be determined by applying the pte
>> manipulation helpers on __pte(0).
>>
>> But on platforms like book3e, the bits depend on whether the page
>> is a user page or not.
>>
>> For the time being it more or less works because of _PAGE_EXEC being
>> used for user pages only and exec right being set at all time on
>> kernel page. But following patch will clean that and output of
>> pte_mkexec() will depend on the page being a user or kernel page.
>>
>> Instead of trying to make an even more complicated helper where bits
>> would become dependent on the final pte value, come back to a more
>> static situation like before commit 26973fa5ac0e ("powerpc/mm: use
>> pte helpers in generic code"), by introducing an 8xx specific
>> version of __ptep_set_access_flags() and ptep_set_wrprotect().
>
> What is this actually fixing? Does it change anything itself, or
> just a preparation patch?
Just a preparation patch I think.
I didn't flag it for stable.
Once patch 2 is applied, __ptep_set_access_flags() doesn't work anymore
without this patch, because then pte_mkexec(__pte(0)) sets SX and clears
UX while pte_mkexec(__pte(~0)) sets UX and clears SX
Christophe
>
> Thanks,
> Nick
>
>>
>> Fixes: 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> v3: No change
>> v2: New
>> ---
>> arch/powerpc/include/asm/nohash/32/pgtable.h | 17 +++++++--------
>> arch/powerpc/include/asm/nohash/32/pte-8xx.h | 22 ++++++++++++++++++++
>> 2 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
>> index 34ce50da1850..11c6849f7864 100644
>> --- a/arch/powerpc/include/asm/nohash/32/pgtable.h
>> +++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
>> @@ -306,30 +306,29 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
>> }
>>
>> #define __HAVE_ARCH_PTEP_SET_WRPROTECT
>> +#ifndef ptep_set_wrprotect
>> static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr,
>> pte_t *ptep)
>> {
>> - unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));
>> - unsigned long set = pte_val(pte_wrprotect(__pte(0)));
>> -
>> - pte_update(mm, addr, ptep, clr, set, 0);
>> + pte_update(mm, addr, ptep, _PAGE_RW, 0, 0);
>> }
>> +#endif
>>
>> +#ifndef __ptep_set_access_flags
>> static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
>> pte_t *ptep, pte_t entry,
>> unsigned long address,
>> int psize)
>> {
>> - pte_t pte_set = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(0)))));
>> - pte_t pte_clr = pte_mkyoung(pte_mkdirty(pte_mkwrite(pte_mkexec(__pte(~0)))));
>> - unsigned long set = pte_val(entry) & pte_val(pte_set);
>> - unsigned long clr = ~pte_val(entry) & ~pte_val(pte_clr);
>> + unsigned long set = pte_val(entry) &
>> + (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
>> int huge = psize > mmu_virtual_psize ? 1 : 0;
>>
>> - pte_update(vma->vm_mm, address, ptep, clr, set, huge);
>> + pte_update(vma->vm_mm, address, ptep, 0, set, huge);
>>
>> flush_tlb_page(vma, address);
>> }
>> +#endif
>>
>> static inline int pte_young(pte_t pte)
>> {
>> diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
>> index fcc48d590d88..1a89ebdc3acc 100644
>> --- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
>> +++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
>> @@ -136,6 +136,28 @@ static inline pte_t pte_mkhuge(pte_t pte)
>>
>> #define pte_mkhuge pte_mkhuge
>>
>> +static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, pte_t *p,
>> + unsigned long clr, unsigned long set, int huge);
>> +
>> +static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>> +{
>> + pte_update(mm, addr, ptep, 0, _PAGE_RO, 0);
>> +}
>> +#define ptep_set_wrprotect ptep_set_wrprotect
>> +
>> +static inline void __ptep_set_access_flags(struct vm_area_struct *vma, pte_t *ptep,
>> + pte_t entry, unsigned long address, int psize)
>> +{
>> + unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_EXEC);
>> + unsigned long clr = ~pte_val(entry) & _PAGE_RO;
>> + int huge = psize > mmu_virtual_psize ? 1 : 0;
>> +
>> + pte_update(vma->vm_mm, address, ptep, clr, set, huge);
>> +
>> + flush_tlb_page(vma, address);
>> +}
>> +#define __ptep_set_access_flags __ptep_set_access_flags
>> +
>> static inline unsigned long pgd_leaf_size(pgd_t pgd)
>> {
>> if (pgd_val(pgd) & _PMD_PAGE_8M)
>> --
>> 2.31.1
>>
>>
On Tue, 26 Oct 2021 07:39:24 +0200, Christophe Leroy wrote:
> Commit 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
> changed those two functions to use pte helpers to determine which
> bits to clear and which bits to set.
>
> This change was based on the assumption that bits to be set/cleared
> are always the same and can be determined by applying the pte
> manipulation helpers on __pte(0).
>
> [...]
Applied to powerpc/next.
[1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect()
https://git.kernel.org/powerpc/c/b1b93cb7e794e914787bf7d9936b57a149cdee4f
[2/3] powerpc/book3e: Fix set_memory_x() and set_memory_nx()
https://git.kernel.org/powerpc/c/b6cb20fdc2735f8b2e082937066c33fe376c2ee2
[3/3] powerpc/fsl_booke: Fix setting of exec flag when setting TLBCAMs
https://git.kernel.org/powerpc/c/44c14509b0dabb909ad1ec28800893ea71762732
cheers