* [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT @ 2021-11-23 9:12 Huangzhaoyang 2021-11-23 9:57 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Huangzhaoyang @ 2021-11-23 9:12 UTC (permalink / raw) To: Ard Biesheuvel, Catalin Marinas, Will Deacon, Anshuman Khandual, Andrew Morton, Nicholas Piggin, Mike Rapoport, Pavel Tatashin, Christophe Leroy, Jonathan Marek, Zhaoyang Huang, linux-mm, linux-kernel From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> Since there is no PTE_CONT when rodata_full in ARM64, introducing a hook function to apply PTE_CONT on the proper page blocks. Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> --- arch/arm64/include/asm/page.h | 5 +++++ arch/arm64/mm/pageattr.c | 45 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h index f98c91b..53cdd09 100644 --- a/arch/arm64/include/asm/page.h +++ b/arch/arm64/include/asm/page.h @@ -46,6 +46,11 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma, #include <asm/memory.h> +#define HAVE_ARCH_ALLOC_PAGE +#define HAVE_ARCH_FREE_PAGE + +extern void arch_alloc_page(struct page *page, int order); +extern void arch_free_page(struct page *page, int order); #endif /* !__ASSEMBLY__ */ #define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED) diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c index a3bacd7..815a06d 100644 --- a/arch/arm64/mm/pageattr.c +++ b/arch/arm64/mm/pageattr.c @@ -239,3 +239,48 @@ bool kernel_page_present(struct page *page) ptep = pte_offset_kernel(pmdp, addr); return pte_valid(READ_ONCE(*ptep)); } + +void arch_alloc_page(struct page *page, int order) +{ + unsigned long addr; + unsigned long cont_pte_low_bound; + + if (!rodata_full) + return; + + addr = (u64)page_address(page); + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { + order -= 4; + do { + cont_pte_low_bound = addr & CONT_PTE_MASK; + __change_memory_common(cont_pte_low_bound, + (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); + addr = (u64)page_address(page); + page += 4; + order--; + }while (order >= 0); + } +} + +void arch_free_page(struct page *page, int order) +{ + unsigned long addr; + unsigned long cont_pte_low_bound; + + if (!rodata_full) + return; + + addr = (u64)page_address(page); + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { + order -= 4; + do { + cont_pte_low_bound = addr & CONT_PTE_MASK; + __change_memory_common(cont_pte_low_bound, + (~CONT_PTE_MASK + 1), __pgprot(0), __pgprot(PTE_CONT)); + addr = (u64)page_address(page); + page += 4; + order--; + }while (order >= 0); + } +} + -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT 2021-11-23 9:12 [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT Huangzhaoyang @ 2021-11-23 9:57 ` Ard Biesheuvel 2021-11-24 8:08 ` Zhaoyang Huang 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2021-11-23 9:57 UTC (permalink / raw) To: Huangzhaoyang Cc: Catalin Marinas, Will Deacon, Anshuman Khandual, Andrew Morton, Nicholas Piggin, Mike Rapoport, Pavel Tatashin, Christophe Leroy, Jonathan Marek, Zhaoyang Huang, Linux Memory Management List, Linux Kernel Mailing List On Tue, 23 Nov 2021 at 10:13, Huangzhaoyang <huangzhaoyang@gmail.com> wrote: > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > Since there is no PTE_CONT when rodata_full in ARM64, introducing a > hook function to apply PTE_CONT on the proper page blocks. > Given the discussion around your previous patch, I would expect a meticulous explanation here why it is guaranteed to be safe to manipulate the PTE_CONT attribute like this, and how the proposed logic is correct for all supported page sizes. Without using an intermediate invalid mapping for the entire range, this is never going to work reliably (this is the break-before-make requirement). And given that marking the entire block invalid will create intermediate states that are not permitted (a valid PTE_CONT mapping and an invalid ~PTE_CONT mapping covering the same VA), the only way to apply changes like these is to temporarily switch all CPUs to a different translation via TTBR1. And this is not going to happen. Also, you never replied to my question regarding the use case and the performance gain. In summary, NAK to this patch or any of the previous ones regarding PTE_CONT. If you do insist on pursuing this further, please provide an elaborate and rock solid explanation why your approach is 100% valid and correct (for all page sizes). And make sure you include an explanation how your changes comply with the architectural break-before-make requirements around PTE_CONT attributes. > --- > arch/arm64/include/asm/page.h | 5 +++++ > arch/arm64/mm/pageattr.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h > index f98c91b..53cdd09 100644 > --- a/arch/arm64/include/asm/page.h > +++ b/arch/arm64/include/asm/page.h > @@ -46,6 +46,11 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma, > > #include <asm/memory.h> > > +#define HAVE_ARCH_ALLOC_PAGE > +#define HAVE_ARCH_FREE_PAGE > + > +extern void arch_alloc_page(struct page *page, int order); > +extern void arch_free_page(struct page *page, int order); > #endif /* !__ASSEMBLY__ */ > > #define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED) > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c > index a3bacd7..815a06d 100644 > --- a/arch/arm64/mm/pageattr.c > +++ b/arch/arm64/mm/pageattr.c > @@ -239,3 +239,48 @@ bool kernel_page_present(struct page *page) > ptep = pte_offset_kernel(pmdp, addr); > return pte_valid(READ_ONCE(*ptep)); > } > + > +void arch_alloc_page(struct page *page, int order) > +{ > + unsigned long addr; > + unsigned long cont_pte_low_bound; > + > + if (!rodata_full) > + return; > + > + addr = (u64)page_address(page); > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > + order -= 4; > + do { > + cont_pte_low_bound = addr & CONT_PTE_MASK; > + __change_memory_common(cont_pte_low_bound, > + (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); > + addr = (u64)page_address(page); > + page += 4; > + order--; > + }while (order >= 0); > + } > +} > + > +void arch_free_page(struct page *page, int order) > +{ > + unsigned long addr; > + unsigned long cont_pte_low_bound; > + > + if (!rodata_full) > + return; > + > + addr = (u64)page_address(page); > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > + order -= 4; > + do { > + cont_pte_low_bound = addr & CONT_PTE_MASK; > + __change_memory_common(cont_pte_low_bound, > + (~CONT_PTE_MASK + 1), __pgprot(0), __pgprot(PTE_CONT)); > + addr = (u64)page_address(page); > + page += 4; > + order--; > + }while (order >= 0); > + } > +} > + > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT 2021-11-23 9:57 ` Ard Biesheuvel @ 2021-11-24 8:08 ` Zhaoyang Huang 2021-11-24 9:23 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Zhaoyang Huang @ 2021-11-24 8:08 UTC (permalink / raw) To: Ard Biesheuvel Cc: Catalin Marinas, Will Deacon, Anshuman Khandual, Andrew Morton, Nicholas Piggin, Mike Rapoport, Pavel Tatashin, Christophe Leroy, Jonathan Marek, Zhaoyang Huang, Linux Memory Management List, Linux Kernel Mailing List On Tue, Nov 23, 2021 at 5:58 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Tue, 23 Nov 2021 at 10:13, Huangzhaoyang <huangzhaoyang@gmail.com> wrote: > > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > Since there is no PTE_CONT when rodata_full in ARM64, introducing a > > hook function to apply PTE_CONT on the proper page blocks. > > > > Given the discussion around your previous patch, I would expect a > meticulous explanation here why it is guaranteed to be safe to > manipulate the PTE_CONT attribute like this, and how the proposed > logic is correct for all supported page sizes. > > Without using an intermediate invalid mapping for the entire range, > this is never going to work reliably (this is the break-before-make > requirement). And given that marking the entire block invalid will > create intermediate states that are not permitted (a valid PTE_CONT > mapping and an invalid ~PTE_CONT mapping covering the same VA), the > only way to apply changes like these is to temporarily switch all CPUs > to a different translation via TTBR1. And this is not going to happen. As there is no safe way to modify PTE_CONT on a live mapping, please forget all previous patches except current one. > > Also, you never replied to my question regarding the use case and the > performance gain. In our android based system, the multimedia related cases suffers from small pte granularity mostly which use high order page blocks quite a lot. The performance gap even be visible. > > In summary, NAK to this patch or any of the previous ones regarding > PTE_CONT. If you do insist on pursuing this further, please provide an > elaborate and rock solid explanation why your approach is 100% valid > and correct (for all page sizes). And make sure you include an > explanation how your changes comply with the architectural > break-before-make requirements around PTE_CONT attributes. IMHO, It is safe to modify the page block's pte undering *arch_alloc/free_pages* as there is no one else aware of it. Furthermore, I do think tlbflush and barriers are needed for synchronization. With regards to page sizes issue, I think replacing the hard code const value to CONT_PTE_XXX could wrap the difference and make it correct. - if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { - order -= 4; + if ((order >= CONT_PTE_SHIFT) && (addr & ~CONT_PTE_MASK) == 0) { + order -= CONT_PTE_SHIFT; do { cont_pte_low_bound = addr & CONT_PTE_MASK; __change_memory_common(cont_pte_low_bound, (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); addr = (u64)page_address(page); - page += 4; + page += CONT_PTES; order--; }while (order >= 0); > > > > > --- > > arch/arm64/include/asm/page.h | 5 +++++ > > arch/arm64/mm/pageattr.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 50 insertions(+) > > > > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h > > index f98c91b..53cdd09 100644 > > --- a/arch/arm64/include/asm/page.h > > +++ b/arch/arm64/include/asm/page.h > > @@ -46,6 +46,11 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma, > > > > #include <asm/memory.h> > > > > +#define HAVE_ARCH_ALLOC_PAGE > > +#define HAVE_ARCH_FREE_PAGE > > + > > +extern void arch_alloc_page(struct page *page, int order); > > +extern void arch_free_page(struct page *page, int order); > > #endif /* !__ASSEMBLY__ */ > > > > #define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED) > > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c > > index a3bacd7..815a06d 100644 > > --- a/arch/arm64/mm/pageattr.c > > +++ b/arch/arm64/mm/pageattr.c > > @@ -239,3 +239,48 @@ bool kernel_page_present(struct page *page) > > ptep = pte_offset_kernel(pmdp, addr); > > return pte_valid(READ_ONCE(*ptep)); > > } > > + > > +void arch_alloc_page(struct page *page, int order) > > +{ > > + unsigned long addr; > > + unsigned long cont_pte_low_bound; > > + > > + if (!rodata_full) > > + return; > > + > > + addr = (u64)page_address(page); > > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > > + order -= 4; > > + do { > > + cont_pte_low_bound = addr & CONT_PTE_MASK; > > + __change_memory_common(cont_pte_low_bound, > > + (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); > > + addr = (u64)page_address(page); > > + page += 4; > > + order--; > > + }while (order >= 0); > > + } > > +} > > + > > +void arch_free_page(struct page *page, int order) > > +{ > > + unsigned long addr; > > + unsigned long cont_pte_low_bound; > > + > > + if (!rodata_full) > > + return; > > + > > + addr = (u64)page_address(page); > > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > > + order -= 4; > > + do { > > + cont_pte_low_bound = addr & CONT_PTE_MASK; > > + __change_memory_common(cont_pte_low_bound, > > + (~CONT_PTE_MASK + 1), __pgprot(0), __pgprot(PTE_CONT)); > > + addr = (u64)page_address(page); > > + page += 4; > > + order--; > > + }while (order >= 0); > > + } > > +} > > + > > -- > > 1.9.1 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT 2021-11-24 8:08 ` Zhaoyang Huang @ 2021-11-24 9:23 ` Ard Biesheuvel 2021-11-24 12:00 ` Zhaoyang Huang 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2021-11-24 9:23 UTC (permalink / raw) To: Zhaoyang Huang Cc: Catalin Marinas, Will Deacon, Anshuman Khandual, Andrew Morton, Nicholas Piggin, Mike Rapoport, Pavel Tatashin, Christophe Leroy, Jonathan Marek, Zhaoyang Huang, Linux Memory Management List, Linux Kernel Mailing List On Wed, 24 Nov 2021 at 09:08, Zhaoyang Huang <huangzhaoyang@gmail.com> wrote: > > On Tue, Nov 23, 2021 at 5:58 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Tue, 23 Nov 2021 at 10:13, Huangzhaoyang <huangzhaoyang@gmail.com> wrote: > > > > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > > > Since there is no PTE_CONT when rodata_full in ARM64, introducing a > > > hook function to apply PTE_CONT on the proper page blocks. > > > > > > > Given the discussion around your previous patch, I would expect a > > meticulous explanation here why it is guaranteed to be safe to > > manipulate the PTE_CONT attribute like this, and how the proposed > > logic is correct for all supported page sizes. > > > > Without using an intermediate invalid mapping for the entire range, > > this is never going to work reliably (this is the break-before-make > > requirement). And given that marking the entire block invalid will > > create intermediate states that are not permitted (a valid PTE_CONT > > mapping and an invalid ~PTE_CONT mapping covering the same VA), the > > only way to apply changes like these is to temporarily switch all CPUs > > to a different translation via TTBR1. And this is not going to happen. > As there is no safe way to modify PTE_CONT on a live mapping, please > forget all previous patches except current one. OK > > > > Also, you never replied to my question regarding the use case and the > > performance gain. > In our android based system, the multimedia related cases suffers from > small pte granularity mostly which use high order page blocks quite a > lot. The performance gap even be visible. OK, good to know. > > > > In summary, NAK to this patch or any of the previous ones regarding > > PTE_CONT. If you do insist on pursuing this further, please provide an > > elaborate and rock solid explanation why your approach is 100% valid > > and correct (for all page sizes). And make sure you include an > > explanation how your changes comply with the architectural > > break-before-make requirements around PTE_CONT attributes. > IMHO, It is safe to modify the page block's pte undering > *arch_alloc/free_pages* as there is no one else aware of it. Whether the software is 'aware' or not is irrelevant. Speculative accesses could be made at any time, and these will trigger a page table walk if no cached TLB entries exist for the region. If more than one cached TLB entry exists (which would be the case if an adjacent entry has the PTE_CONT attribute but not the entry itself), you will hit a TLB conflict abort. I guess the behavior under invalid mappings might be subtly different, since those should not be cached in a TLB, but the fundamental problem remains: no conflicting entries should exist at any time, and PTE_CONT must be either set or cleared on all entries in the same contiguous group. These are contradictory requirements for live translations, so the only way around it is to uninstall the translation from all CPUs, perform the update, and reinstall it. > Furthermore, I do think tlbflush and barriers are needed for > synchronization. Careful [repeated] TLB maintenance may reduce the size of the window where the TLB conflicts may occur, but it does not solve the issue. > With regards to page sizes issue, I think replacing > the hard code const value to CONT_PTE_XXX could wrap the difference > and make it correct. > > - if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > - order -= 4; > + if ((order >= CONT_PTE_SHIFT) && (addr & ~CONT_PTE_MASK) == 0) { > + order -= CONT_PTE_SHIFT; > do { > cont_pte_low_bound = addr & CONT_PTE_MASK; > __change_memory_common(cont_pte_low_bound, > (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); > addr = (u64)page_address(page); > - page += 4; > + > page += CONT_PTES; > order--; > }while (order >= 0); > > > > > > > > > --- > > > arch/arm64/include/asm/page.h | 5 +++++ > > > arch/arm64/mm/pageattr.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 50 insertions(+) > > > > > > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h > > > index f98c91b..53cdd09 100644 > > > --- a/arch/arm64/include/asm/page.h > > > +++ b/arch/arm64/include/asm/page.h > > > @@ -46,6 +46,11 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma, > > > > > > #include <asm/memory.h> > > > > > > +#define HAVE_ARCH_ALLOC_PAGE > > > +#define HAVE_ARCH_FREE_PAGE > > > + > > > +extern void arch_alloc_page(struct page *page, int order); > > > +extern void arch_free_page(struct page *page, int order); > > > #endif /* !__ASSEMBLY__ */ > > > > > > #define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED) > > > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c > > > index a3bacd7..815a06d 100644 > > > --- a/arch/arm64/mm/pageattr.c > > > +++ b/arch/arm64/mm/pageattr.c > > > @@ -239,3 +239,48 @@ bool kernel_page_present(struct page *page) > > > ptep = pte_offset_kernel(pmdp, addr); > > > return pte_valid(READ_ONCE(*ptep)); > > > } > > > + > > > +void arch_alloc_page(struct page *page, int order) > > > +{ > > > + unsigned long addr; > > > + unsigned long cont_pte_low_bound; > > > + > > > + if (!rodata_full) > > > + return; > > > + > > > + addr = (u64)page_address(page); > > > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > > > + order -= 4; > > > + do { > > > + cont_pte_low_bound = addr & CONT_PTE_MASK; > > > + __change_memory_common(cont_pte_low_bound, > > > + (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); > > > + addr = (u64)page_address(page); > > > + page += 4; > > > + order--; > > > + }while (order >= 0); > > > + } > > > +} > > > + > > > +void arch_free_page(struct page *page, int order) > > > +{ > > > + unsigned long addr; > > > + unsigned long cont_pte_low_bound; > > > + > > > + if (!rodata_full) > > > + return; > > > + > > > + addr = (u64)page_address(page); > > > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > > > + order -= 4; > > > + do { > > > + cont_pte_low_bound = addr & CONT_PTE_MASK; > > > + __change_memory_common(cont_pte_low_bound, > > > + (~CONT_PTE_MASK + 1), __pgprot(0), __pgprot(PTE_CONT)); > > > + addr = (u64)page_address(page); > > > + page += 4; > > > + order--; > > > + }while (order >= 0); > > > + } > > > +} > > > + > > > -- > > > 1.9.1 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT 2021-11-24 9:23 ` Ard Biesheuvel @ 2021-11-24 12:00 ` Zhaoyang Huang 2021-11-24 13:54 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Zhaoyang Huang @ 2021-11-24 12:00 UTC (permalink / raw) To: Ard Biesheuvel Cc: Catalin Marinas, Will Deacon, Anshuman Khandual, Andrew Morton, Nicholas Piggin, Mike Rapoport, Pavel Tatashin, Christophe Leroy, Jonathan Marek, Zhaoyang Huang, Linux Memory Management List, Linux Kernel Mailing List On Wed, Nov 24, 2021 at 5:23 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Wed, 24 Nov 2021 at 09:08, Zhaoyang Huang <huangzhaoyang@gmail.com> wrote: > > > > On Tue, Nov 23, 2021 at 5:58 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Tue, 23 Nov 2021 at 10:13, Huangzhaoyang <huangzhaoyang@gmail.com> wrote: > > > > > > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > > > > > Since there is no PTE_CONT when rodata_full in ARM64, introducing a > > > > hook function to apply PTE_CONT on the proper page blocks. > > > > > > > > > > Given the discussion around your previous patch, I would expect a > > > meticulous explanation here why it is guaranteed to be safe to > > > manipulate the PTE_CONT attribute like this, and how the proposed > > > logic is correct for all supported page sizes. > > > > > > Without using an intermediate invalid mapping for the entire range, > > > this is never going to work reliably (this is the break-before-make > > > requirement). And given that marking the entire block invalid will > > > create intermediate states that are not permitted (a valid PTE_CONT > > > mapping and an invalid ~PTE_CONT mapping covering the same VA), the > > > only way to apply changes like these is to temporarily switch all CPUs > > > to a different translation via TTBR1. And this is not going to happen. > > As there is no safe way to modify PTE_CONT on a live mapping, please > > forget all previous patches except current one. > > OK > > > > > > > Also, you never replied to my question regarding the use case and the > > > performance gain. > > In our android based system, the multimedia related cases suffers from > > small pte granularity mostly which use high order page blocks quite a > > lot. The performance gap even be visible. > > OK, good to know. > > > > > > > In summary, NAK to this patch or any of the previous ones regarding > > > PTE_CONT. If you do insist on pursuing this further, please provide an > > > elaborate and rock solid explanation why your approach is 100% valid > > > and correct (for all page sizes). And make sure you include an > > > explanation how your changes comply with the architectural > > > break-before-make requirements around PTE_CONT attributes. > > IMHO, It is safe to modify the page block's pte undering > > *arch_alloc/free_pages* as there is no one else aware of it. > > Whether the software is 'aware' or not is irrelevant. Speculative > accesses could be made at any time, and these will trigger a page > table walk if no cached TLB entries exist for the region. If more than > one cached TLB entry exists (which would be the case if an adjacent > entry has the PTE_CONT attribute but not the entry itself), you will > hit a TLB conflict abort. Could it be a risk that a speculative load racing with setting pte from VALID to INVALID? > > I guess the behavior under invalid mappings might be subtly different, > since those should not be cached in a TLB, but the fundamental problem > remains: no conflicting entries should exist at any time, and PTE_CONT > must be either set or cleared on all entries in the same contiguous > group. These are contradictory requirements for live translations, so > the only way around it is to uninstall the translation from all CPUs, > perform the update, and reinstall it. > > > Furthermore, I do think tlbflush and barriers are needed for > > synchronization. > > Careful [repeated] TLB maintenance may reduce the size of the window > where the TLB conflicts may occur, but it does not solve the issue. > > > With regards to page sizes issue, I think replacing > > the hard code const value to CONT_PTE_XXX could wrap the difference > > and make it correct. > > > > - if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > > - order -= 4; > > + if ((order >= CONT_PTE_SHIFT) && (addr & ~CONT_PTE_MASK) == 0) { > > + order -= CONT_PTE_SHIFT; > > do { > > cont_pte_low_bound = addr & CONT_PTE_MASK; > > __change_memory_common(cont_pte_low_bound, > > (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); > > addr = (u64)page_address(page); > > - page += 4; > > + > > page += CONT_PTES; > > order--; > > }while (order >= 0); > > > > > > > > > > > > > --- > > > > arch/arm64/include/asm/page.h | 5 +++++ > > > > arch/arm64/mm/pageattr.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 50 insertions(+) > > > > > > > > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h > > > > index f98c91b..53cdd09 100644 > > > > --- a/arch/arm64/include/asm/page.h > > > > +++ b/arch/arm64/include/asm/page.h > > > > @@ -46,6 +46,11 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma, > > > > > > > > #include <asm/memory.h> > > > > > > > > +#define HAVE_ARCH_ALLOC_PAGE > > > > +#define HAVE_ARCH_FREE_PAGE > > > > + > > > > +extern void arch_alloc_page(struct page *page, int order); > > > > +extern void arch_free_page(struct page *page, int order); > > > > #endif /* !__ASSEMBLY__ */ > > > > > > > > #define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED) > > > > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c > > > > index a3bacd7..815a06d 100644 > > > > --- a/arch/arm64/mm/pageattr.c > > > > +++ b/arch/arm64/mm/pageattr.c > > > > @@ -239,3 +239,48 @@ bool kernel_page_present(struct page *page) > > > > ptep = pte_offset_kernel(pmdp, addr); > > > > return pte_valid(READ_ONCE(*ptep)); > > > > } > > > > + > > > > +void arch_alloc_page(struct page *page, int order) > > > > +{ > > > > + unsigned long addr; > > > > + unsigned long cont_pte_low_bound; > > > > + > > > > + if (!rodata_full) > > > > + return; > > > > + > > > > + addr = (u64)page_address(page); > > > > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > > > > + order -= 4; > > > > + do { > > > > + cont_pte_low_bound = addr & CONT_PTE_MASK; > > > > + __change_memory_common(cont_pte_low_bound, > > > > + (~CONT_PTE_MASK + 1), __pgprot(PTE_CONT), __pgprot(0)); > > > > + addr = (u64)page_address(page); > > > > + page += 4; > > > > + order--; > > > > + }while (order >= 0); > > > > + } > > > > +} > > > > + > > > > +void arch_free_page(struct page *page, int order) > > > > +{ > > > > + unsigned long addr; > > > > + unsigned long cont_pte_low_bound; > > > > + > > > > + if (!rodata_full) > > > > + return; > > > > + > > > > + addr = (u64)page_address(page); > > > > + if ((order >= 4) && (addr & ~CONT_PTE_MASK) == 0) { > > > > + order -= 4; > > > > + do { > > > > + cont_pte_low_bound = addr & CONT_PTE_MASK; > > > > + __change_memory_common(cont_pte_low_bound, > > > > + (~CONT_PTE_MASK + 1), __pgprot(0), __pgprot(PTE_CONT)); > > > > + addr = (u64)page_address(page); > > > > + page += 4; > > > > + order--; > > > > + }while (order >= 0); > > > > + } > > > > +} > > > > + > > > > -- > > > > 1.9.1 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT 2021-11-24 12:00 ` Zhaoyang Huang @ 2021-11-24 13:54 ` Ard Biesheuvel 0 siblings, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2021-11-24 13:54 UTC (permalink / raw) To: Zhaoyang Huang Cc: Catalin Marinas, Will Deacon, Anshuman Khandual, Andrew Morton, Nicholas Piggin, Mike Rapoport, Pavel Tatashin, Christophe Leroy, Jonathan Marek, Zhaoyang Huang, Linux Memory Management List, Linux Kernel Mailing List On Wed, 24 Nov 2021 at 13:01, Zhaoyang Huang <huangzhaoyang@gmail.com> wrote: > > On Wed, Nov 24, 2021 at 5:23 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Wed, 24 Nov 2021 at 09:08, Zhaoyang Huang <huangzhaoyang@gmail.com> wrote: > > > > > > On Tue, Nov 23, 2021 at 5:58 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > On Tue, 23 Nov 2021 at 10:13, Huangzhaoyang <huangzhaoyang@gmail.com> wrote: > > > > > > > > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > > > > > > > Since there is no PTE_CONT when rodata_full in ARM64, introducing a > > > > > hook function to apply PTE_CONT on the proper page blocks. > > > > > > > > > > > > > Given the discussion around your previous patch, I would expect a > > > > meticulous explanation here why it is guaranteed to be safe to > > > > manipulate the PTE_CONT attribute like this, and how the proposed > > > > logic is correct for all supported page sizes. > > > > > > > > Without using an intermediate invalid mapping for the entire range, > > > > this is never going to work reliably (this is the break-before-make > > > > requirement). And given that marking the entire block invalid will > > > > create intermediate states that are not permitted (a valid PTE_CONT > > > > mapping and an invalid ~PTE_CONT mapping covering the same VA), the > > > > only way to apply changes like these is to temporarily switch all CPUs > > > > to a different translation via TTBR1. And this is not going to happen. > > > As there is no safe way to modify PTE_CONT on a live mapping, please > > > forget all previous patches except current one. > > > > OK > > > > > > > > > > Also, you never replied to my question regarding the use case and the > > > > performance gain. > > > In our android based system, the multimedia related cases suffers from > > > small pte granularity mostly which use high order page blocks quite a > > > lot. The performance gap even be visible. > > > > OK, good to know. > > > > > > > > > > In summary, NAK to this patch or any of the previous ones regarding > > > > PTE_CONT. If you do insist on pursuing this further, please provide an > > > > elaborate and rock solid explanation why your approach is 100% valid > > > > and correct (for all page sizes). And make sure you include an > > > > explanation how your changes comply with the architectural > > > > break-before-make requirements around PTE_CONT attributes. > > > IMHO, It is safe to modify the page block's pte undering > > > *arch_alloc/free_pages* as there is no one else aware of it. > > > > Whether the software is 'aware' or not is irrelevant. Speculative > > accesses could be made at any time, and these will trigger a page > > table walk if no cached TLB entries exist for the region. If more than > > one cached TLB entry exists (which would be the case if an adjacent > > entry has the PTE_CONT attribute but not the entry itself), you will > > hit a TLB conflict abort. > Could it be a risk that a speculative load racing with setting pte > from VALID to INVALID? Theorizing about what might go wrong is not very useful. The architecture simply does not permit what you are proposing here, and this is reason enough not to do it. The ARM ARM says the following [DDI0487G.a D5.2 "Misprogramming of the Contiguous bit"] """ If one or more of the following errors is made in programming the translation tables, the TLB might contain overlapping entries: - One or more of the contiguous translation table entries does not have the Contiguous bit set to 1. - One or more of the contiguous translation table entries holds an output address that is not consistent with all of the entries pointing to the same aligned contiguous address range. - The attributes and permissions of the contiguous entries are not all the same. Such misprogramming of the translation tables means the output address, memory permissions, or attributes for a lookup might be corrupted, and might be equal to values that are not consistent with any of the programmed translation table values. In some implementations, such misprogramming might also give rise to a TLB Conflict abort. """ This means that none of the entries in the contiguous group are reliable when only one entry in the group deviates, and in addition to TLB conflict aborts, you may trigger all kinds of corruption due to the output address being bogus. So NAK again. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-24 14:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-23 9:12 [RFC PATCH] mm: introduce alloc hook to apply PTE_CONT Huangzhaoyang 2021-11-23 9:57 ` Ard Biesheuvel 2021-11-24 8:08 ` Zhaoyang Huang 2021-11-24 9:23 ` Ard Biesheuvel 2021-11-24 12:00 ` Zhaoyang Huang 2021-11-24 13:54 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).