RE: [PATCH v11 1/4] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Catanlin
Thanks for the detailed explanation.
Will send out v12 soon after testing
--
Cheers,
Justin (Jia He)
> -Original Message-
> From: Catalin Marinas
> Sent: Friday, October 11, 2019 6:39 PM
> To: Justin He (Arm Technology China)
> Cc: Will Deacon ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Suzuki Poulose ; Borislav
> Petkov ; H. Peter Anvin ; x...@kernel.org;
> Thomas Gleixner ; Andrew Morton foundation.org>; hejia...@gmail.com; Kaly Xin (Arm Technology China)
> ; nd
> Subject: Re: [PATCH v11 1/4] arm64: cpufeature: introduce helper
> cpu_has_hw_af()
>
> On Fri, Oct 11, 2019 at 01:16:36AM +, Justin He (Arm Technology China)
> wrote:
> > From: Catalin Marinas
> > > On Wed, Oct 09, 2019 at 04:42:43PM +0800, Jia He wrote:
> > > > + u64 mmfr1 = read_cpuid(ID_AA64MMFR1_EL1);
> > > > +
> > > > + return !!cpuid_feature_extract_unsigned_field(mmfr1,
> > > > +
> > > ID_AA64MMFR1_HADBS_SHIFT);
> > >
> > > No need for !!, the return type is a bool already.
> >
> > But cpuid_feature_extract_unsigned_field has the return type "unsigned
> int" [1]
> >
> > [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch
> /arm64/include/asm/cpufeature.h#n444
>
> And the C language gives you the automatic conversion from unsigned int
> to bool without the need for !!. The reason we use !! in some places is
> for converting long to int (not bool) and losing the top 32-bit. See
> commit 84fe6826c28f ("arm64: mm: Add double logical invert to pte
> accessors") for an explanation.
>
> --
> Catalin
RE: [PATCH v11 1/4] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Catalin
> -Original Message-
> From: Catalin Marinas
> Sent: Friday, October 11, 2019 12:43 AM
> To: Justin He (Arm Technology China)
> Cc: Will Deacon ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Suzuki Poulose ; Borislav
> Petkov ; H. Peter Anvin ; x...@kernel.org;
> Thomas Gleixner ; Andrew Morton foundation.org>; hejia...@gmail.com; Kaly Xin (Arm Technology China)
> ; nd
> Subject: Re: [PATCH v11 1/4] arm64: cpufeature: introduce helper
> cpu_has_hw_af()
>
> On Wed, Oct 09, 2019 at 04:42:43PM +0800, Jia He wrote:
> > We unconditionally set the HW_AFDBM capability and only enable it on
> > CPUs which really have the feature. But sometimes we need to know
> > whether this cpu has the capability of HW AF. So decouple AF from
> > DBM by a new helper cpu_has_hw_af().
> >
> > Signed-off-by: Jia He
> > Suggested-by: Suzuki Poulose
> > Reviewed-by: Catalin Marinas
>
> I don't think I reviewed this version of the patch.
Sorry about that.
>
> > diff --git a/arch/arm64/include/asm/cpufeature.h
> b/arch/arm64/include/asm/cpufeature.h
> > index 9cde5d2e768f..1a95396ea5c8 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -659,6 +659,20 @@ static inline u32
> id_aa64mmfr0_parange_to_phys_shift(int parange)
> > default: return CONFIG_ARM64_PA_BITS;
> > }
> > }
> > +
> > +/* Check whether hardware update of the Access flag is supported */
> > +static inline bool cpu_has_hw_af(void)
> > +{
> > + if (IS_ENABLED(CONFIG_ARM64_HW_AFDBM)) {
>
> Please just return early here to avoid unnecessary indentation:
Okay
>
> if (!IS_ENABLED(CONFIG_ARM64_HW_AFDBM))
> return false;
>
> > + u64 mmfr1 = read_cpuid(ID_AA64MMFR1_EL1);
> > +
> > + return !!cpuid_feature_extract_unsigned_field(mmfr1,
> > +
> ID_AA64MMFR1_HADBS_SHIFT);
>
> No need for !!, the return type is a bool already.
But cpuid_feature_extract_unsigned_field has the return type "unsigned int" [1]
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/cpufeature.h#n444
>
> Anyway, apart from these nitpicks, the patch is fine you can keep my
> reviewed-by.
Thanks
>
> If later we noticed a potential performance issue on this path, we can
> turn it into a static label as with other CPU features.
Okay
--
Cheers,
Justin (Jia He)
RE: [PATCH v10 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
Hi Will
> -Original Message-
> From: Will Deacon
> Sent: 2019年10月8日 20:40
> To: Justin He (Arm Technology China)
> Cc: Catalin Marinas ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Punit Agrawal ; Thomas
> Gleixner ; Andrew Morton foundation.org>; hejia...@gmail.com; Kaly Xin (Arm Technology China)
> ; nd
> Subject: Re: [PATCH v10 3/3] mm: fix double page fault on arm64 if PTE_AF
> is cleared
>
> On Tue, Oct 08, 2019 at 02:19:05AM +, Justin He (Arm Technology
> China) wrote:
> > > -Original Message-
> > > From: Will Deacon
> > > Sent: 2019年10月1日 20:54
> > > To: Justin He (Arm Technology China)
> > > Cc: Catalin Marinas ; Mark Rutland
> > > ; James Morse ;
> Marc
> > > Zyngier ; Matthew Wilcox ;
> Kirill A.
> > > Shutemov ; linux-arm-
> > > ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > > m...@kvack.org; Punit Agrawal ; Thomas
> > > Gleixner ; Andrew Morton > > foundation.org>; hejia...@gmail.com; Kaly Xin (Arm Technology China)
> > >
> > > Subject: Re: [PATCH v10 3/3] mm: fix double page fault on arm64 if
> PTE_AF
> > > is cleared
> > >
> > > On Mon, Sep 30, 2019 at 09:57:40AM +0800, Jia He wrote:
> > > > diff --git a/mm/memory.c b/mm/memory.c
> > > > index b1ca51a079f2..1f56b0118ef5 100644
> > > > --- a/mm/memory.c
> > > > +++ b/mm/memory.c
> > > > @@ -118,6 +118,13 @@ int randomize_va_space __read_mostly =
> > > > 2;
> > > > #endif
> > > >
> > > > +#ifndef arch_faults_on_old_pte
> > > > +static inline bool arch_faults_on_old_pte(void)
> > > > +{
> > > > + return false;
> > > > +}
> > > > +#endif
> > >
> > > Kirill has acked this, so I'm happy to take the patch as-is, however isn't
> > > it the case that /most/ architectures will want to return true for
> > > arch_faults_on_old_pte()? In which case, wouldn't it make more sense
> for
> > > that to be the default, and have x86 and arm64 provide an override?
> For
> > > example, aren't most architectures still going to hit the double fault
> > > scenario even with your patch applied?
> >
> > No, after applying my patch series, only those architectures which don't
> provide
> > setting access flag by hardware AND don't implement their
> arch_faults_on_old_pte
> > will hit the double page fault.
> >
> > The meaning of true for arch_faults_on_old_pte() is "this arch doesn't
> have the hardware
> > setting access flag way, it might cause page fault on an old pte"
> > I don't want to change other architectures' default behavior here. So by
> default,
> > arch_faults_on_old_pte() is false.
>
> ...and my complaint is that this is the majority of supported architectures,
> so you're fixing something for arm64 which also affects arm, powerpc,
> alpha, mips, riscv, ...
So, IIUC, you suggested that:
1. by default, arch_faults_on_old_pte() return true
2. on X86, let arch_faults_on_old_pte() be overrided as returning false
3. on arm64, let it be as-is my patch set.
4. let other architectures decide the behavior. (But by default, it will set
pte_young)
I am ok with that if no objections from others.
@Kirill A. Shutemov Do you have any comments? Thanks
>
> Chances are, they won't even realise they need to implement
> arch_faults_on_old_pte() until somebody runs into the double fault and
> wastes lots of time debugging it before they spot your patch.
As to this point, I added a WARN_ON in patch 03 to speed up the debugging
process.
--
Cheers,
Justin (Jia He)
>
> > Btw, currently I only observed this double pagefault on arm64's guest
> > (host is ThunderX2). On X86 guest (host is Intel(R) Core(TM) i7-4790 CPU
> > @ 3.60GHz ), there is no such double pagefault. It has the similar setting
> > access flag way by hardware.
>
> Right, and that's why I'm not concerned about x86 for this problem.
>
> Will
RE: [PATCH v10 2/3] arm64: mm: implement arch_faults_on_old_pte() on arm64
> -Original Message-
> From: Justin He (Arm Technology China)
> Sent: 2019年10月8日 9:55
> To: Marc Zyngier ; Will Deacon
> Cc: Catalin Marinas ; Mark Rutland
> ; James Morse ;
> Matthew Wilcox ; Kirill A. Shutemov
> ; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux...@kvack.org; Punit Agrawal
> ; Thomas Gleixner ;
> Andrew Morton ; hejia...@gmail.com; Kaly
> Xin (Arm Technology China) ; nd
> Subject: RE: [PATCH v10 2/3] arm64: mm: implement
> arch_faults_on_old_pte() on arm64
>
> Hi Will and Marc
>
> > -Original Message-
> > From: Marc Zyngier
> > Sent: 2019年10月1日 21:32
> > To: Will Deacon
> > Cc: Justin He (Arm Technology China) ; Catalin
> > Marinas ; Mark Rutland
> > ; James Morse ;
> > Matthew Wilcox ; Kirill A. Shutemov
> > ; linux-arm-ker...@lists.infradead.org;
> > linux-kernel@vger.kernel.org; linux...@kvack.org; Punit Agrawal
> > ; Thomas Gleixner ;
> > Andrew Morton ; hejia...@gmail.com;
> Kaly
> > Xin (Arm Technology China)
> > Subject: Re: [PATCH v10 2/3] arm64: mm: implement
> > arch_faults_on_old_pte() on arm64
> >
> > On Tue, 1 Oct 2019 13:50:32 +0100
> > Will Deacon wrote:
> >
> > > On Mon, Sep 30, 2019 at 09:57:39AM +0800, Jia He wrote:
> > > > On arm64 without hardware Access Flag, copying fromuser will fail
> > because
> > > > the pte is old and cannot be marked young. So we always end up with
> > zeroed
> > > > page after fork() + CoW for pfn mappings. we don't always have a
> > > > hardware-managed access flag on arm64.
> > > >
> > > > Hence implement arch_faults_on_old_pte on arm64 to indicate that
> it
> > might
> > > > cause page fault when accessing old pte.
> > > >
> > > > Signed-off-by: Jia He
> > > > Reviewed-by: Catalin Marinas
> > > > ---
> > > > arch/arm64/include/asm/pgtable.h | 14 ++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/include/asm/pgtable.h
> > b/arch/arm64/include/asm/pgtable.h
> > > > index 7576df00eb50..e96fb82f62de 100644
> > > > --- a/arch/arm64/include/asm/pgtable.h
> > > > +++ b/arch/arm64/include/asm/pgtable.h
> > > > @@ -885,6 +885,20 @@ static inline void update_mmu_cache(struct
> > vm_area_struct *vma,
> > > > #define phys_to_ttbr(addr) (addr)
> > > > #endif
> > > >
> > > > +/*
> > > > + * On arm64 without hardware Access Flag, copying from user will
> fail
> > because
> > > > + * the pte is old and cannot be marked young. So we always end up
> > with zeroed
> > > > + * page after fork() + CoW for pfn mappings. We don't always have a
> > > > + * hardware-managed access flag on arm64.
> > > > + */
> > > > +static inline bool arch_faults_on_old_pte(void)
> > > > +{
> > > > + WARN_ON(preemptible());
> > > > +
> > > > + return !cpu_has_hw_af();
> > > > +}
> > >
> > > Does this work correctly in a KVM guest? (i.e. is the MMFR sanitised in
> > that
> > > case, despite not being the case on the host?)
> >
> > Yup, all the 64bit MMFRs are trapped (HCR_EL2.TID3 is set for an
> > AArch64 guest), and we return the sanitised version.
> Thanks for Marc's explanation. I verified the patch series on a kvm guest (-
> M virt)
> with simulated nvdimm device created by qemu. The host is ThunderX2
> aarch64.
>
> >
> > But that's an interesting remark: we're now trading an extra fault on
> > CPUs that do not support HWAFDBS for a guaranteed trap for each and
> > every guest under the sun that will hit the COW path...
> >
> > My gut feeling is that this is going to be pretty visible. Jia, do you
> > have any numbers for this kind of behaviour?
> It is not a common COW path, but a COW for PFN mapping pages only.
> I add a g_counter before pte_mkyoung in force_mkyoung{} when testing
> vmmalloc_fork at [1].
>
> In this test case, it will start M fork processes and N pthreads. The default
> is
> M=2,N=4. the g_counter is about 241, that is it will hit my patch series for
> 241
> times.
> If I set M=20 and N=40 for TEST3, the g_counter is about 1492.
The time overhead of test vmmalloc_fork is:
real0m5.411s
user0m4.206s
sys 0m2.699s
>
> [1] https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork
>
>
> --
> Cheers,
> Justin (Jia He)
>
RE: [PATCH v10 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
Hi Will
> -Original Message-
> From: Will Deacon
> Sent: 2019年10月1日 20:54
> To: Justin He (Arm Technology China)
> Cc: Catalin Marinas ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Punit Agrawal ; Thomas
> Gleixner ; Andrew Morton foundation.org>; hejia...@gmail.com; Kaly Xin (Arm Technology China)
>
> Subject: Re: [PATCH v10 3/3] mm: fix double page fault on arm64 if PTE_AF
> is cleared
>
> On Mon, Sep 30, 2019 at 09:57:40AM +0800, Jia He wrote:
> > When we tested pmdk unit test [1] vmmalloc_fork TEST1 in arm64 guest,
> there
> > will be a double page fault in __copy_from_user_inatomic of
> cow_user_page.
> >
> > Below call trace is from arm64 do_page_fault for debugging purpose
> > [ 110.016195] Call trace:
> > [ 110.016826] do_page_fault+0x5a4/0x690
> > [ 110.017812] do_mem_abort+0x50/0xb0
> > [ 110.018726] el1_da+0x20/0xc4
> > [ 110.019492] __arch_copy_from_user+0x180/0x280
> > [ 110.020646] do_wp_page+0xb0/0x860
> > [ 110.021517] __handle_mm_fault+0x994/0x1338
> > [ 110.022606] handle_mm_fault+0xe8/0x180
> > [ 110.023584] do_page_fault+0x240/0x690
> > [ 110.024535] do_mem_abort+0x50/0xb0
> > [ 110.025423] el0_da+0x20/0x24
> >
> > The pte info before __copy_from_user_inatomic is (PTE_AF is cleared):
> > [9b007000] pgd=00023d4f8003, pud=00023da9b003,
> pmd=00023d4b3003, pte=36298607bd3
> >
> > As told by Catalin: "On arm64 without hardware Access Flag, copying
> from
> > user will fail because the pte is old and cannot be marked young. So we
> > always end up with zeroed page after fork() + CoW for pfn mappings. we
> > don't always have a hardware-managed access flag on arm64."
> >
> > This patch fix it by calling pte_mkyoung. Also, the parameter is
> > changed because vmf should be passed to cow_user_page()
> >
> > Add a WARN_ON_ONCE when __copy_from_user_inatomic() returns
> error
> > in case there can be some obscure use-case.(by Kirill)
> >
> > [1]
> https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork
> >
> > Signed-off-by: Jia He
> > Reported-by: Yibo Cai
> > Reviewed-by: Catalin Marinas
> > Acked-by: Kirill A. Shutemov
> > ---
> > mm/memory.c | 99
> +
> > 1 file changed, 84 insertions(+), 15 deletions(-)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index b1ca51a079f2..1f56b0118ef5 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -118,6 +118,13 @@ int randomize_va_space __read_mostly =
> > 2;
> > #endif
> >
> > +#ifndef arch_faults_on_old_pte
> > +static inline bool arch_faults_on_old_pte(void)
> > +{
> > + return false;
> > +}
> > +#endif
>
> Kirill has acked this, so I'm happy to take the patch as-is, however isn't
> it the case that /most/ architectures will want to return true for
> arch_faults_on_old_pte()? In which case, wouldn't it make more sense for
> that to be the default, and have x86 and arm64 provide an override? For
> example, aren't most architectures still going to hit the double fault
> scenario even with your patch applied?
No, after applying my patch series, only those architectures which don't provide
setting access flag by hardware AND don't implement their arch_faults_on_old_pte
will hit the double page fault.
The meaning of true for arch_faults_on_old_pte() is "this arch doesn't have the
hardware
setting access flag way, it might cause page fault on an old pte"
I don't want to change other architectures' default behavior here. So by
default,
arch_faults_on_old_pte() is false.
Btw, currently I only observed this double pagefault on arm64's guest (host is
ThunderX2).
On X86 guest (host is Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz ), there is no
such double
pagefault. It has the similar setting access flag way by hardware.
--
Cheers,
Justin (Jia He)
RE: [PATCH v10 2/3] arm64: mm: implement arch_faults_on_old_pte() on arm64
Hi Will and Marc
> -Original Message-
> From: Marc Zyngier
> Sent: 2019年10月1日 21:32
> To: Will Deacon
> Cc: Justin He (Arm Technology China) ; Catalin
> Marinas ; Mark Rutland
> ; James Morse ;
> Matthew Wilcox ; Kirill A. Shutemov
> ; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux...@kvack.org; Punit Agrawal
> ; Thomas Gleixner ;
> Andrew Morton ; hejia...@gmail.com; Kaly
> Xin (Arm Technology China)
> Subject: Re: [PATCH v10 2/3] arm64: mm: implement
> arch_faults_on_old_pte() on arm64
>
> On Tue, 1 Oct 2019 13:50:32 +0100
> Will Deacon wrote:
>
> > On Mon, Sep 30, 2019 at 09:57:39AM +0800, Jia He wrote:
> > > On arm64 without hardware Access Flag, copying fromuser will fail
> because
> > > the pte is old and cannot be marked young. So we always end up with
> zeroed
> > > page after fork() + CoW for pfn mappings. we don't always have a
> > > hardware-managed access flag on arm64.
> > >
> > > Hence implement arch_faults_on_old_pte on arm64 to indicate that it
> might
> > > cause page fault when accessing old pte.
> > >
> > > Signed-off-by: Jia He
> > > Reviewed-by: Catalin Marinas
> > > ---
> > > arch/arm64/include/asm/pgtable.h | 14 ++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/arch/arm64/include/asm/pgtable.h
> b/arch/arm64/include/asm/pgtable.h
> > > index 7576df00eb50..e96fb82f62de 100644
> > > --- a/arch/arm64/include/asm/pgtable.h
> > > +++ b/arch/arm64/include/asm/pgtable.h
> > > @@ -885,6 +885,20 @@ static inline void update_mmu_cache(struct
> vm_area_struct *vma,
> > > #define phys_to_ttbr(addr) (addr)
> > > #endif
> > >
> > > +/*
> > > + * On arm64 without hardware Access Flag, copying from user will fail
> because
> > > + * the pte is old and cannot be marked young. So we always end up
> with zeroed
> > > + * page after fork() + CoW for pfn mappings. We don't always have a
> > > + * hardware-managed access flag on arm64.
> > > + */
> > > +static inline bool arch_faults_on_old_pte(void)
> > > +{
> > > + WARN_ON(preemptible());
> > > +
> > > + return !cpu_has_hw_af();
> > > +}
> >
> > Does this work correctly in a KVM guest? (i.e. is the MMFR sanitised in
> that
> > case, despite not being the case on the host?)
>
> Yup, all the 64bit MMFRs are trapped (HCR_EL2.TID3 is set for an
> AArch64 guest), and we return the sanitised version.
Thanks for Marc's explanation. I verified the patch series on a kvm guest (-M
virt)
with simulated nvdimm device created by qemu. The host is ThunderX2 aarch64.
>
> But that's an interesting remark: we're now trading an extra fault on
> CPUs that do not support HWAFDBS for a guaranteed trap for each and
> every guest under the sun that will hit the COW path...
>
> My gut feeling is that this is going to be pretty visible. Jia, do you
> have any numbers for this kind of behaviour?
It is not a common COW path, but a COW for PFN mapping pages only.
I add a g_counter before pte_mkyoung in force_mkyoung{} when testing
vmmalloc_fork at [1].
In this test case, it will start M fork processes and N pthreads. The default is
M=2,N=4. the g_counter is about 241, that is it will hit my patch series for 241
times.
If I set M=20 and N=40 for TEST3, the g_counter is about 1492.
[1] https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork
--
Cheers,
Justin (Jia He)
RE: [PATCH v10 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Will and Marc
Sorry for the late response, just came back from a vacation.
> -Original Message-
> From: Marc Zyngier
> Sent: 2019年10月1日 21:19
> To: Will Deacon
> Cc: Justin He (Arm Technology China) ; Catalin
> Marinas ; Mark Rutland
> ; James Morse ;
> Matthew Wilcox ; Kirill A. Shutemov
> ; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux...@kvack.org; Punit Agrawal
> ; Thomas Gleixner ;
> Andrew Morton ; hejia...@gmail.com; Kaly
> Xin (Arm Technology China)
> Subject: Re: [PATCH v10 1/3] arm64: cpufeature: introduce helper
> cpu_has_hw_af()
>
> On Tue, 1 Oct 2019 13:54:47 +0100
> Will Deacon wrote:
>
> > On Mon, Sep 30, 2019 at 09:57:38AM +0800, Jia He wrote:
> > > We unconditionally set the HW_AFDBM capability and only enable it on
> > > CPUs which really have the feature. But sometimes we need to know
> > > whether this cpu has the capability of HW AF. So decouple AF from
> > > DBM by new helper cpu_has_hw_af().
> > >
> > > Signed-off-by: Jia He
> > > Suggested-by: Suzuki Poulose
> > > Reviewed-by: Catalin Marinas
> > > ---
> > > arch/arm64/include/asm/cpufeature.h | 10 ++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/arch/arm64/include/asm/cpufeature.h
> b/arch/arm64/include/asm/cpufeature.h
> > > index 9cde5d2e768f..949bc7c85030 100644
> > > --- a/arch/arm64/include/asm/cpufeature.h
> > > +++ b/arch/arm64/include/asm/cpufeature.h
> > > @@ -659,6 +659,16 @@ static inline u32
> id_aa64mmfr0_parange_to_phys_shift(int parange)
> > > default: return CONFIG_ARM64_PA_BITS;
> > > }
> > > }
> > > +
> > > +/* Check whether hardware update of the Access flag is supported */
> > > +static inline bool cpu_has_hw_af(void)
> > > +{
> > > + if (IS_ENABLED(CONFIG_ARM64_HW_AFDBM))
> > > + return read_cpuid(ID_AA64MMFR1_EL1) & 0xf;
> >
> > 0xf? I think we should have a mask in sysreg.h for this constant.
>
> We don't have the mask, but we certainly have the shift.
>
> GENMASK(ID_AA64MMFR1_HADBS_SHIFT + 3,
> ID_AA64MMFR1_HADBS_SHIFT) is a bit
> of a mouthful though. Ideally, we'd have a helper for that.
>
Ok, I will implement the helper if there isn't so far.
And then replace the 0xf with it.
--
Cheers,
Justin (Jia He)
> M.
> --
> Without deviation from the norm, progress is not possible.
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH v9 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Catalin > -Original Message- > From: Catalin Marinas > Sent: 2019年9月25日 22:38 > To: Justin He (Arm Technology China) > Cc: Will Deacon ; Mark Rutland > ; James Morse ; Marc > Zyngier ; Matthew Wilcox ; Kirill A. > Shutemov ; linux-arm- > ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > m...@kvack.org; Suzuki Poulose ; Punit > Agrawal ; Anshuman Khandual > ; Alex Van Brunt > ; Robin Murphy ; > Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell > ; hejia...@gmail.com; Kaly Xin (Arm Technology > China) ; nd > Subject: Re: [PATCH v9 1/3] arm64: cpufeature: introduce helper > cpu_has_hw_af() > > On Wed, Sep 25, 2019 at 10:59:20AM +0800, Jia He wrote: > > We unconditionally set the HW_AFDBM capability and only enable it on > > CPUs which really have the feature. But sometimes we need to know > > whether this cpu has the capability of HW AF. So decouple AF from > > DBM by new helper cpu_has_hw_af(). > > > > Signed-off-by: Jia He > > Suggested-by: Suzuki Poulose > > Reported-by: kbuild test robot > > Which bug did the kbuild robot actually report? I'd drop this line. > This line is added due to [1]: "If you fix the issue, kindly add following tag Reported-by: kbuild test robot " Yes, I know your concern, it is a little bit confusing. But I don't know how to distinguish the case btw a) original bug report b) bug report of my patch implementation? Thanks for any suggestion. [1] https://www.lkml.org/lkml/2019/9/18/940 -- Cheers, Justin (Jia He)
RE: [PATCH v9 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
Hi Matthew and Kirill
I didn't add your previous r-b and a-b tag since I refactored the cow_user_page
and changed the ptl range in v9. Please have a review, thanks
--
Cheers,
Justin (Jia He)
> -Original Message-
> From: Jia He
> Sent: 2019年9月25日 10:59
> To: Catalin Marinas ; Will Deacon
> ; Mark Rutland ; James Morse
> ; Marc Zyngier ; Matthew
> Wilcox ; Kirill A. Shutemov
> ; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux...@kvack.org; Suzuki Poulose
>
> Cc: Punit Agrawal ; Anshuman Khandual
> ; Alex Van Brunt
> ; Robin Murphy ;
> Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell
> ; hejia...@gmail.com; Kaly Xin (Arm Technology
> China) ; nd ; Justin He (Arm
> Technology China)
> Subject: [PATCH v9 3/3] mm: fix double page fault on arm64 if PTE_AF is
> cleared
>
> When we tested pmdk unit test [1] vmmalloc_fork TEST1 in arm64 guest,
> there
> will be a double page fault in __copy_from_user_inatomic of
> cow_user_page.
>
> Below call trace is from arm64 do_page_fault for debugging purpose
> [ 110.016195] Call trace:
> [ 110.016826] do_page_fault+0x5a4/0x690
> [ 110.017812] do_mem_abort+0x50/0xb0
> [ 110.018726] el1_da+0x20/0xc4
> [ 110.019492] __arch_copy_from_user+0x180/0x280
> [ 110.020646] do_wp_page+0xb0/0x860
> [ 110.021517] __handle_mm_fault+0x994/0x1338
> [ 110.022606] handle_mm_fault+0xe8/0x180
> [ 110.023584] do_page_fault+0x240/0x690
> [ 110.024535] do_mem_abort+0x50/0xb0
> [ 110.025423] el0_da+0x20/0x24
>
> The pte info before __copy_from_user_inatomic is (PTE_AF is cleared):
> [9b007000] pgd=00023d4f8003, pud=00023da9b003,
> pmd=00023d4b3003, pte=36298607bd3
>
> As told by Catalin: "On arm64 without hardware Access Flag, copying from
> user will fail because the pte is old and cannot be marked young. So we
> always end up with zeroed page after fork() + CoW for pfn mappings. we
> don't always have a hardware-managed access flag on arm64."
>
> This patch fix it by calling pte_mkyoung. Also, the parameter is
> changed because vmf should be passed to cow_user_page()
>
> Add a WARN_ON_ONCE when __copy_from_user_inatomic() returns error
> in case there can be some obscure use-case.(by Kirill)
>
> [1] https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork
>
> Signed-off-by: Jia He
> Reported-by: Yibo Cai
> ---
> mm/memory.c | 99
> +
> 1 file changed, 84 insertions(+), 15 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index e2bb51b6242e..a0a381b36ff2 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -118,6 +118,13 @@ int randomize_va_space __read_mostly =
> 2;
> #endif
>
> +#ifndef arch_faults_on_old_pte
> +static inline bool arch_faults_on_old_pte(void)
> +{
> + return false;
> +}
> +#endif
> +
> static int __init disable_randmaps(char *s)
> {
> randomize_va_space = 0;
> @@ -2140,32 +2147,82 @@ static inline int pte_unmap_same(struct
> mm_struct *mm, pmd_t *pmd,
> return same;
> }
>
> -static inline void cow_user_page(struct page *dst, struct page *src,
> unsigned long va, struct vm_area_struct *vma)
> +static inline bool cow_user_page(struct page *dst, struct page *src,
> + struct vm_fault *vmf)
> {
> + bool ret;
> + void *kaddr;
> + void __user *uaddr;
> + bool force_mkyoung;
> + struct vm_area_struct *vma = vmf->vma;
> + struct mm_struct *mm = vma->vm_mm;
> + unsigned long addr = vmf->address;
> +
> debug_dma_assert_idle(src);
>
> + if (likely(src)) {
> + copy_user_highpage(dst, src, addr, vma);
> + return true;
> + }
> +
> /*
>* If the source page was a PFN mapping, we don't have
>* a "struct page" for it. We do a best-effort copy by
>* just copying from the original user address. If that
>* fails, we just zero-fill it. Live with it.
>*/
> - if (unlikely(!src)) {
> - void *kaddr = kmap_atomic(dst);
> - void __user *uaddr = (void __user *)(va & PAGE_MASK);
> + kaddr = kmap_atomic(dst);
> + uaddr = (void __user *)(addr & PAGE_MASK);
> +
> + /*
> + * On architectures with software "accessed" bits, we would
> + * take a double page fault, so mark it accessed here.
> + */
> + force_mkyoung = arch_faults_on_old_pte() && !pte_young(vmf-
> >orig_pte);
> + if (force_mkyoung) {
> + pte_t entry
RE: [PATCH v8 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
Hi Catalin
Please see an important comment inline, thanks
> -Original Message-
> From: Catalin Marinas
> Sent: 2019年9月24日 1:05
> To: Justin He (Arm Technology China)
> Cc: Will Deacon ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Suzuki Poulose ; Punit
> Agrawal ; Anshuman Khandual
> ; Alex Van Brunt
> ; Robin Murphy ;
> Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell
> ; hejia...@gmail.com; Kaly Xin (Arm Technology
> China) ; nd
> Subject: Re: [PATCH v8 3/3] mm: fix double page fault on arm64 if PTE_AF
> is cleared
>
> On Sat, Sep 21, 2019 at 09:50:54PM +0800, Jia He wrote:
> > @@ -2151,21 +2163,53 @@ static inline void cow_user_page(struct page
> *dst, struct page *src, unsigned lo
> > * fails, we just zero-fill it. Live with it.
> > */
> > if (unlikely(!src)) {
> > - void *kaddr = kmap_atomic(dst);
> > - void __user *uaddr = (void __user *)(va & PAGE_MASK);
> > + void *kaddr;
> > + pte_t entry;
> > + void __user *uaddr = (void __user *)(addr & PAGE_MASK);
> >
> > + /* On architectures with software "accessed" bits, we would
> > +* take a double page fault, so mark it accessed here.
> > +*/
>
> Nitpick: please follow the kernel coding style for multi-line comments
> (above and the for the rest of the patch):
>
> /*
>* Your multi-line comment.
>*/
>
> > + if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte))
> {
> > + vmf->pte = pte_offset_map_lock(mm, vmf->pmd,
> addr,
> > + >ptl);
> > + if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
> > + entry = pte_mkyoung(vmf->orig_pte);
> > + if (ptep_set_access_flags(vma, addr,
> > + vmf->pte, entry, 0))
> > + update_mmu_cache(vma, addr, vmf-
> >pte);
> > + } else {
> > + /* Other thread has already handled the
> fault
> > +* and we don't need to do anything. If it's
> > +* not the case, the fault will be triggered
> > +* again on the same address.
> > +*/
> > + pte_unmap_unlock(vmf->pte, vmf->ptl);
> > + return false;
> > + }
> > + pte_unmap_unlock(vmf->pte, vmf->ptl);
> > + }
>
> Another nit, you could rewrite this block slightly to avoid too much
> indentation. Something like (untested):
>
> if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte))
> {
> vmf->pte = pte_offset_map_lock(mm, vmf->pmd,
> addr,
> >ptl);
> if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte))) {
> /*
>* Other thread has already handled the fault
>* and we don't need to do anything. If it's
>* not the case, the fault will be triggered
>* again on the same address.
>*/
> pte_unmap_unlock(vmf->pte, vmf->ptl);
> return false;
> }
> entry = pte_mkyoung(vmf->orig_pte);
> if (ptep_set_access_flags(vma, addr,
> vmf->pte, entry, 0))
> update_mmu_cache(vma, addr, vmf->pte);
> pte_unmap_unlock(vmf->pte, vmf->ptl);
> }
>
> > +
> > + kaddr = kmap_atomic(dst);
>
> Since you moved the kmap_atomic() here, could the above
> arch_faults_on_old_pte() run in a preemptible context? I suggested to
> add a WARN_ON in patch 2 to be sure.
Should I move kmap_atomic back to the original line? Thus, we can make sure
that arch_faults_on_old_pte() is in the context of preempt_disabled?
Otherwise, arch_faults_on_old_pte() may cause plenty of wa
RE: [PATCH v8 2/3] arm64: mm: implement arch_faults_on_old_pte() on arm64
> -Original Message-
> From: Catalin Marinas
> Sent: 2019年9月24日 0:18
> To: Justin He (Arm Technology China)
> Cc: Will Deacon ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Suzuki Poulose ; Punit
> Agrawal ; Anshuman Khandual
> ; Alex Van Brunt
> ; Robin Murphy ;
> Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell
> ; hejia...@gmail.com; Kaly Xin (Arm Technology
> China) ; nd
> Subject: Re: [PATCH v8 2/3] arm64: mm: implement
> arch_faults_on_old_pte() on arm64
>
> On Sat, Sep 21, 2019 at 09:50:53PM +0800, Jia He wrote:
> > diff --git a/arch/arm64/include/asm/pgtable.h
> b/arch/arm64/include/asm/pgtable.h
> > index e09760ece844..4a9939615e41 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -868,6 +868,18 @@ static inline void update_mmu_cache(struct
> vm_area_struct *vma,
> > #define phys_to_ttbr(addr) (addr)
> > #endif
> >
> > +/*
> > + * On arm64 without hardware Access Flag, copying fromuser will fail
> because
>
>from user
>
Ok
> > + * the pte is old and cannot be marked young. So we always end up with
> zeroed
> > + * page after fork() + CoW for pfn mappings. we don't always have a
> ^^
> We
>
Ok
> > + * hardware-managed access flag on arm64.
> > + */
> > +static inline bool arch_faults_on_old_pte(void)
> > +{
> > + return !cpu_has_hw_af();
>
> I saw an early incarnation of your patch having a
> WARN_ON(preemptible()). I think we need this back just in case this
> function will be used elsewhere in the future.
Okay
--
Cheers,
Justin (Jia He)
>
> > +}
> > +#define arch_faults_on_old_pte arch_faults_on_old_pte
>
> Otherwise,
>
> Reviewed-by: Catalin Marinas
RE: [PATCH v8 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Catalin
> -Original Message-
> From: Catalin Marinas
> Sent: 2019年9月24日 0:07
> To: Justin He (Arm Technology China)
> Cc: Will Deacon ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Suzuki Poulose ; Punit
> Agrawal ; Anshuman Khandual
> ; Alex Van Brunt
> ; Robin Murphy ;
> Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell
> ; hejia...@gmail.com; Kaly Xin (Arm Technology
> China) ; nd
> Subject: Re: [PATCH v8 1/3] arm64: cpufeature: introduce helper
> cpu_has_hw_af()
>
> On Sat, Sep 21, 2019 at 09:50:52PM +0800, Jia He wrote:
> > We unconditionally set the HW_AFDBM capability and only enable it on
> > CPUs which really have the feature. But sometimes we need to know
> > whether this cpu has the capability of HW AF. So decouple AF from
> > DBM by new helper cpu_has_hw_af().
> >
> > Reported-by: kbuild test robot
> > Suggested-by: Suzuki Poulose
> > Signed-off-by: Jia He
> > ---
> > arch/arm64/include/asm/cpufeature.h | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/cpufeature.h
> b/arch/arm64/include/asm/cpufeature.h
> > index c96ffa4722d3..46caf934ba4e 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -667,6 +667,16 @@ static inline u32
> id_aa64mmfr0_parange_to_phys_shift(int parange)
> > default: return CONFIG_ARM64_PA_BITS;
> > }
> > }
> > +
> > +/* Decouple AF from AFDBM. */
>
> We could do with a better comment here or just remove it altogether. The
> aim of the patch was to decouple AF check from the AF+DBM but the
> comment here should describe what the function does. Maybe something
> like: "Check whether hardware update of the Access flag is supported".
>
Okay, I will update it
--
Cheers,
Justin (Jia He)
> > +static inline bool cpu_has_hw_af(void)
> > +{
> > + if (IS_ENABLED(CONFIG_ARM64_HW_AFDBM))
> > + return read_cpuid(ID_AA64MMFR1_EL1) & 0xf;
> > +
> > + return false;
> > +}
>
> Other than the comment above,
>
> Reviewed-by: Catalin Marinas
RE: [PATCH v7 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
Thanks for your patent review -- Cheers, Justin (Jia He) > -Original Message- > From: Kirill A. Shutemov > Sent: 2019年9月20日 22:21 > To: Justin He (Arm Technology China) > Cc: Catalin Marinas ; Will Deacon > ; Mark Rutland ; James Morse > ; Marc Zyngier ; Matthew > Wilcox ; Kirill A. Shutemov > ; linux-arm-ker...@lists.infradead.org; > linux-kernel@vger.kernel.org; linux...@kvack.org; Suzuki Poulose > ; Punit Agrawal ; > Anshuman Khandual ; Alex Van Brunt > ; Robin Murphy ; > Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell > ; hejia...@gmail.com; Kaly Xin (Arm Technology > China) ; nd > Subject: Re: [PATCH v7 3/3] mm: fix double page fault on arm64 if PTE_AF is > cleared > > On Fri, Sep 20, 2019 at 09:54:37PM +0800, Jia He wrote: > > When we tested pmdk unit test [1] vmmalloc_fork TEST1 in arm64 guest, > there > > will be a double page fault in __copy_from_user_inatomic of > cow_user_page. > > > > Below call trace is from arm64 do_page_fault for debugging purpose > > [ 110.016195] Call trace: > > [ 110.016826] do_page_fault+0x5a4/0x690 > > [ 110.017812] do_mem_abort+0x50/0xb0 > > [ 110.018726] el1_da+0x20/0xc4 > > [ 110.019492] __arch_copy_from_user+0x180/0x280 > > [ 110.020646] do_wp_page+0xb0/0x860 > > [ 110.021517] __handle_mm_fault+0x994/0x1338 > > [ 110.022606] handle_mm_fault+0xe8/0x180 > > [ 110.023584] do_page_fault+0x240/0x690 > > [ 110.024535] do_mem_abort+0x50/0xb0 > > [ 110.025423] el0_da+0x20/0x24 > > > > The pte info before __copy_from_user_inatomic is (PTE_AF is cleared): > > [9b007000] pgd=00023d4f8003, pud=00023da9b003, > pmd=00023d4b3003, pte=36298607bd3 > > > > As told by Catalin: "On arm64 without hardware Access Flag, copying from > > user will fail because the pte is old and cannot be marked young. So we > > always end up with zeroed page after fork() + CoW for pfn mappings. we > > don't always have a hardware-managed access flag on arm64." > > > > This patch fix it by calling pte_mkyoung. Also, the parameter is > > changed because vmf should be passed to cow_user_page() > > > > Add a WARN_ON_ONCE when __copy_from_user_inatomic() returns error > > in case there can be some obscure use-case.(by Kirill) > > > > [1] https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork > > > > Reported-by: Yibo Cai > > Signed-off-by: Jia He > > Acked-by: Kirill A. Shutemov > > -- > Kirill A. Shutemov
RE: [PATCH v5 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
Hi Catalin
> -Original Message-
> From: Catalin Marinas
> Sent: 2019年9月20日 0:42
> To: Justin He (Arm Technology China)
> Cc: Will Deacon ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Suzuki Poulose ; Punit
> Agrawal ; Anshuman Khandual
> ; Alex Van Brunt
> ; Robin Murphy ;
> Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell
> ; hejia...@gmail.com; Kaly Xin (Arm Technology
> China)
> Subject: Re: [PATCH v5 3/3] mm: fix double page fault on arm64 if PTE_AF
> is cleared
>
> On Fri, Sep 20, 2019 at 12:12:04AM +0800, Jia He wrote:
> > @@ -2152,7 +2163,29 @@ static inline void cow_user_page(struct page
> *dst, struct page *src, unsigned lo
> > */
> > if (unlikely(!src)) {
> > void *kaddr = kmap_atomic(dst);
> > - void __user *uaddr = (void __user *)(va & PAGE_MASK);
> > + void __user *uaddr = (void __user *)(addr & PAGE_MASK);
> > + pte_t entry;
> > +
> > + /* On architectures with software "accessed" bits, we would
> > +* take a double page fault, so mark it accessed here.
> > +*/
> > + if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte))
> {
> > + spin_lock(vmf->ptl);
> > + if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
> > + entry = pte_mkyoung(vmf->orig_pte);
> > + if (ptep_set_access_flags(vma, addr,
> > + vmf->pte, entry, 0))
> > + update_mmu_cache(vma, addr, vmf-
> >pte);
> > + } else {
> > + /* Other thread has already handled the
> fault
> > +* and we don't need to do anything. If it's
> > +* not the case, the fault will be triggered
> > +* again on the same address.
> > +*/
> > + return -1;
> > + }
> > + spin_unlock(vmf->ptl);
>
> Returning with the spinlock held doesn't normally go very well ;).
Yes, my bad. Will fix asap
--
Cheers,
Justin (Jia He)
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH v5 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Catalin
> -Original Message-
> From: Catalin Marinas
> Sent: 2019年9月20日 0:37
> To: Justin He (Arm Technology China)
> Cc: Will Deacon ; Mark Rutland
> ; James Morse ; Marc
> Zyngier ; Matthew Wilcox ; Kirill A.
> Shutemov ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> m...@kvack.org; Suzuki Poulose ; Punit
> Agrawal ; Anshuman Khandual
> ; Alex Van Brunt
> ; Robin Murphy ;
> Thomas Gleixner ; Andrew Morton foundation.org>; Jérôme Glisse ; Ralph Campbell
> ; hejia...@gmail.com; Kaly Xin (Arm Technology
> China)
> Subject: Re: [PATCH v5 1/3] arm64: cpufeature: introduce helper
> cpu_has_hw_af()
>
> On Fri, Sep 20, 2019 at 12:12:02AM +0800, Jia He wrote:
> > diff --git a/arch/arm64/kernel/cpufeature.c
> b/arch/arm64/kernel/cpufeature.c
> > index b1fdc486aed8..fb0e9425d286 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -1141,6 +1141,16 @@ static bool has_hw_dbm(const struct
> arm64_cpu_capabilities *cap,
> > return true;
> > }
> >
> > +/* Decouple AF from AFDBM. */
> > +bool cpu_has_hw_af(void)
> > +{
> > + return (read_cpuid(ID_AA64MMFR1_EL1) & 0xf);
> > +}
> > +#else /* CONFIG_ARM64_HW_AFDBM */
> > +bool cpu_has_hw_af(void)
> > +{
> > + return false;
> > +}
> > #endif
>
> Please place this function in cpufeature.h directly, no need for an
> additional function call. Something like:
>
> static inline bool cpu_has_hw_af(void)
> {
> if (IS_ENABLED(CONFIG_ARM64_HW_AFDBM))
> return read_cpuid(ID_AA64MMFR1_EL1) & 0xf;
> return false;
> }
>
Ok, thanks
--
Cheers,
Justin (Jia He)
> --
> Catalin
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH v4 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
Hi Kirill
Thanks for the detailed explanation.
--
Cheers,
Justin (Jia He)
> -Original Message-
> From: Kirill A. Shutemov
> Sent: 2019年9月19日 22:58
> To: Jia He
> Cc: Justin He (Arm Technology China) ; Catalin
> Marinas ; Will Deacon ; Mark
> Rutland ; James Morse
> ; Marc Zyngier ; Matthew
> Wilcox ; Kirill A. Shutemov
> ; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux...@kvack.org; Suzuki Poulose
> ; Punit Agrawal ;
> Anshuman Khandual ; Jun Yao
> ; Alex Van Brunt ;
> Robin Murphy ; Thomas Gleixner
> ; Andrew Morton ;
> Jérôme Glisse ; Ralph Campbell
> ; Kaly Xin (Arm Technology China)
>
> Subject: Re: [PATCH v4 3/3] mm: fix double page fault on arm64 if PTE_AF is
> cleared
>
> On Thu, Sep 19, 2019 at 10:16:34AM +0800, Jia He wrote:
> > Hi Kirill
> >
> > [On behalf of justin...@arm.com because some mails are filted...]
> >
> > On 2019/9/18 22:00, Kirill A. Shutemov wrote:
> > > On Wed, Sep 18, 2019 at 09:19:14PM +0800, Jia He wrote:
> > > > When we tested pmdk unit test [1] vmmalloc_fork TEST1 in arm64
> guest, there
> > > > will be a double page fault in __copy_from_user_inatomic of
> cow_user_page.
> > > >
> > > > Below call trace is from arm64 do_page_fault for debugging purpose
> > > > [ 110.016195] Call trace:
> > > > [ 110.016826] do_page_fault+0x5a4/0x690
> > > > [ 110.017812] do_mem_abort+0x50/0xb0
> > > > [ 110.018726] el1_da+0x20/0xc4
> > > > [ 110.019492] __arch_copy_from_user+0x180/0x280
> > > > [ 110.020646] do_wp_page+0xb0/0x860
> > > > [ 110.021517] __handle_mm_fault+0x994/0x1338
> > > > [ 110.022606] handle_mm_fault+0xe8/0x180
> > > > [ 110.023584] do_page_fault+0x240/0x690
> > > > [ 110.024535] do_mem_abort+0x50/0xb0
> > > > [ 110.025423] el0_da+0x20/0x24
> > > >
> > > > The pte info before __copy_from_user_inatomic is (PTE_AF is cleared):
> > > > [9b007000] pgd=00023d4f8003, pud=00023da9b003,
> pmd=00023d4b3003, pte=36298607bd3
> > > >
> > > > As told by Catalin: "On arm64 without hardware Access Flag, copying
> from
> > > > user will fail because the pte is old and cannot be marked young. So
> we
> > > > always end up with zeroed page after fork() + CoW for pfn mappings.
> we
> > > > don't always have a hardware-managed access flag on arm64."
> > > >
> > > > This patch fix it by calling pte_mkyoung. Also, the parameter is
> > > > changed because vmf should be passed to cow_user_page()
> > > >
> > > > [1]
> https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork
> > > >
> > > > Reported-by: Yibo Cai
> > > > Signed-off-by: Jia He
> > > > ---
> > > > mm/memory.c | 35 ++-
> > > > 1 file changed, 30 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/mm/memory.c b/mm/memory.c
> > > > index e2bb51b6242e..d2c130a5883b 100644
> > > > --- a/mm/memory.c
> > > > +++ b/mm/memory.c
> > > > @@ -118,6 +118,13 @@ int randomize_va_space __read_mostly =
> > > > 2;
> > > > #endif
> > > > +#ifndef arch_faults_on_old_pte
> > > > +static inline bool arch_faults_on_old_pte(void)
> > > > +{
> > > > + return false;
> > > > +}
> > > > +#endif
> > > > +
> > > > static int __init disable_randmaps(char *s)
> > > > {
> > > > randomize_va_space = 0;
> > > > @@ -2140,8 +2147,12 @@ static inline int pte_unmap_same(struct
> mm_struct *mm, pmd_t *pmd,
> > > > return same;
> > > > }
> > > > -static inline void cow_user_page(struct page *dst, struct page *src,
> unsigned long va, struct vm_area_struct *vma)
> > > > +static inline void cow_user_page(struct page *dst, struct page *src,
> > > > +struct vm_fault *vmf)
> > > > {
> > > > + struct vm_area_struct *vma = vmf->vma;
> > > > + unsigned long addr = vmf->address;
> > > > +
> > > > debug_dma_assert_idle(src);
> > > > /*
> > > > @@ -2152,20 +2163,34 @@ static inline void cow_user_page(struct
> page *dst, struct page *src, unsigned lo
> > > >
RE: [PATCH v4 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Suzuki
> -Original Message-
> From: Catalin Marinas
> Sent: 2019年9月19日 0:46
> To: Suzuki Poulose
> Cc: Justin He (Arm Technology China) ; Will Deacon
> ; Mark Rutland ; James Morse
> ; Marc Zyngier ; Matthew
> Wilcox ; Kirill A. Shutemov
> ; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux...@kvack.org; Punit Agrawal
> ; Anshuman Khandual
> ; Jun Yao ;
> Alex Van Brunt ; Robin Murphy
> ; Thomas Gleixner ;
> Andrew Morton ; Jérôme Glisse
> ; Ralph Campbell ;
> hejia...@gmail.com; Kaly Xin (Arm Technology China)
> Subject: Re: [PATCH v4 1/3] arm64: cpufeature: introduce helper
> cpu_has_hw_af()
>
> On Wed, Sep 18, 2019 at 03:20:41PM +0100, Suzuki K Poulose wrote:
> > On 18/09/2019 14:19, Jia He wrote:
> > > diff --git a/arch/arm64/include/asm/cpufeature.h
> b/arch/arm64/include/asm/cpufeature.h
> > > index c96ffa4722d3..206b6e3954cf 100644
> > > --- a/arch/arm64/include/asm/cpufeature.h
> > > +++ b/arch/arm64/include/asm/cpufeature.h
> > > @@ -390,6 +390,7 @@ extern DECLARE_BITMAP(boot_capabilities,
> ARM64_NPATCHABLE);
> > > for_each_set_bit(cap, cpu_hwcaps, ARM64_NCAPS)
> > > bool this_cpu_has_cap(unsigned int cap);
> > > +bool cpu_has_hw_af(void);
> > > void cpu_set_feature(unsigned int num);
> > > bool cpu_have_feature(unsigned int num);
> > > unsigned long cpu_get_elf_hwcap(void);
> > > diff --git a/arch/arm64/kernel/cpufeature.c
> b/arch/arm64/kernel/cpufeature.c
> > > index b1fdc486aed8..c5097f58649d 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -1141,6 +1141,12 @@ static bool has_hw_dbm(const struct
> arm64_cpu_capabilities *cap,
> > > return true;
> > > }
> > > +/* Decouple AF from AFDBM. */
> > > +bool cpu_has_hw_af(void)
> > > +{
> > Sorry for not having asked this earlier. Are we interested in,
> >
> > "whether *this* CPU has AF support ?" or "whether *at least one*
> > CPU has the AF support" ? The following code does the former.
> >
> > > + return (read_cpuid(ID_AA64MMFR1_EL1) & 0xf);
>
> In a non-preemptible context, the former is ok (per-CPU).
Yes, just as what Catalin explained, we need the former because the
pagefault occurred in every cpus
--
Cheers,
Justin (Jia He)
>
> --
> Catalin
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH v4 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
> -Original Message- > From: kbuild test robot > Sent: 2019年9月19日 3:36 > To: Justin He (Arm Technology China) > Cc: kbuild-...@01.org; Catalin Marinas ; Will > Deacon ; Mark Rutland ; > James Morse ; Marc Zyngier ; > Matthew Wilcox ; Kirill A. Shutemov > ; linux-arm-ker...@lists.infradead.org; > linux-kernel@vger.kernel.org; linux...@kvack.org; Suzuki Poulose > ; Punit Agrawal ; > Anshuman Khandual ; Jun Yao > ; Alex Van Brunt ; > Robin Murphy ; Thomas Gleixner > ; Andrew Morton ; > jgli...@redhat.com; Ralph Campbell ; > hejia...@gmail.com; Kaly Xin (Arm Technology China) > ; Justin He (Arm Technology China) > > Subject: Re: [PATCH v4 3/3] mm: fix double page fault on arm64 if PTE_AF > is cleared > > Hi Jia, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [cannot apply to v5.3 next-20190917] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system] > > url:https://github.com/0day-ci/linux/commits/Jia-He/fix-double-page- > fault-on-arm64/20190918-220036 > config: arm64-allnoconfig (attached as .config) > compiler: aarch64-linux-gcc (GCC) 7.4.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp- > tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.4.0 make.cross ARCH=arm64 > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot > > All errors (new ones prefixed by >>): > >mm/memory.o: In function `wp_page_copy': > >> memory.c:(.text+0x8fc): undefined reference to `cpu_has_hw_af' >memory.c:(.text+0x8fc): relocation truncated to fit: R_AARCH64_CALL26 > against undefined symbol `cpu_has_hw_af' > Ah, I should add a stub for CONFIG_ARM64_HW_AFDBM is 'N' on arm64 arch Will fix it asap -- Cheers, Justin (Jia He) > 0-DAY kernel test infrastructureOpen Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
RE: [PATCH v3 2/2] mm: fix double page fault on arm64 if PTE_AF is cleared
Hi Kirill
> -Original Message-
> From: Kirill A. Shutemov
> Sent: 2019年9月16日 17:16
> To: Justin He (Arm Technology China)
> Cc: Catalin Marinas ; Will Deacon
> ; Mark Rutland ; James Morse
> ; Marc Zyngier ; Matthew
> Wilcox ; Kirill A. Shutemov
> ; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux...@kvack.org; Punit Agrawal
> ; Anshuman Khandual
> ; Jun Yao ;
> Alex Van Brunt ; Robin Murphy
> ; Thomas Gleixner ;
> Andrew Morton ; Jérôme Glisse
> ; Ralph Campbell ;
> hejia...@gmail.com
> Subject: Re: [PATCH v3 2/2] mm: fix double page fault on arm64 if PTE_AF
> is cleared
>
> On Sat, Sep 14, 2019 at 12:32:39AM +0800, Jia He wrote:
> > When we tested pmdk unit test [1] vmmalloc_fork TEST1 in arm64 guest,
> there
> > will be a double page fault in __copy_from_user_inatomic of
> cow_user_page.
> >
> > Below call trace is from arm64 do_page_fault for debugging purpose
> > [ 110.016195] Call trace:
> > [ 110.016826] do_page_fault+0x5a4/0x690
> > [ 110.017812] do_mem_abort+0x50/0xb0
> > [ 110.018726] el1_da+0x20/0xc4
> > [ 110.019492] __arch_copy_from_user+0x180/0x280
> > [ 110.020646] do_wp_page+0xb0/0x860
> > [ 110.021517] __handle_mm_fault+0x994/0x1338
> > [ 110.022606] handle_mm_fault+0xe8/0x180
> > [ 110.023584] do_page_fault+0x240/0x690
> > [ 110.024535] do_mem_abort+0x50/0xb0
> > [ 110.025423] el0_da+0x20/0x24
> >
> > The pte info before __copy_from_user_inatomic is (PTE_AF is cleared):
> > [9b007000] pgd=00023d4f8003, pud=00023da9b003,
> pmd=00023d4b3003, pte=36298607bd3
> >
> > As told by Catalin: "On arm64 without hardware Access Flag, copying
> from
> > user will fail because the pte is old and cannot be marked young. So we
> > always end up with zeroed page after fork() + CoW for pfn mappings. we
> > don't always have a hardware-managed access flag on arm64."
> >
> > This patch fix it by calling pte_mkyoung. Also, the parameter is
> > changed because vmf should be passed to cow_user_page()
> >
> > [1]
> https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork
> >
> > Reported-by: Yibo Cai
> > Signed-off-by: Jia He
> > ---
> > mm/memory.c | 30 +-
> > 1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index e2bb51b6242e..a64af6495f71 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -118,6 +118,13 @@ int randomize_va_space __read_mostly =
> > 2;
> > #endif
> >
> > +#ifndef arch_faults_on_old_pte
> > +static inline bool arch_faults_on_old_pte(void)
> > +{
> > + return false;
> > +}
> > +#endif
> > +
> > static int __init disable_randmaps(char *s)
> > {
> > randomize_va_space = 0;
> > @@ -2140,7 +2147,8 @@ static inline int pte_unmap_same(struct
> mm_struct *mm, pmd_t *pmd,
> > return same;
> > }
> >
> > -static inline void cow_user_page(struct page *dst, struct page *src,
> unsigned long va, struct vm_area_struct *vma)
> > +static inline void cow_user_page(struct page *dst, struct page *src,
> > + struct vm_fault *vmf)
> > {
> > debug_dma_assert_idle(src);
> >
> > @@ -2152,20 +2160,32 @@ static inline void cow_user_page(struct page
> *dst, struct page *src, unsigned lo
> > */
> > if (unlikely(!src)) {
> > void *kaddr = kmap_atomic(dst);
> > - void __user *uaddr = (void __user *)(va & PAGE_MASK);
> > + void __user *uaddr = (void __user *)(vmf->address &
> PAGE_MASK);
> > + pte_t entry;
> >
> > /*
> > * This really shouldn't fail, because the page is there
> > * in the page tables. But it might just be unreadable,
> > * in which case we just give up and fill the result with
> > -* zeroes.
> > +* zeroes. If PTE_AF is cleared on arm64, it might
> > +* cause double page fault. So makes pte young here
> > */
> > + if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte))
> {
> > + spin_lock(vmf->ptl);
> > + entry = pte_mkyoung(vmf->orig_pte);
>
> Should't you re-validate that orig_pte after re-taking ptl? It can be
> stale by now.
Thanks, do you mean flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte))
before pte_mkyoung?
--
Ch
RE: [PATCH 1/2] drivers/dax/kmem: use default numa_mem_id if target_node is invalid
Hi
Ping.
The target_node will be -1 if numa disabled. IIUC, it is a generic issue, not
only on arm64.
--
Cheers,
Justin (Jia He)
> -Original Message-
> From: Jia He
> Sent: 2019年8月16日 19:19
> To: Dan Williams ; Vishal Verma
>
> Cc: Keith Busch ; Dave Jiang
> ; linux-nvd...@lists.01.org; linux-
> ker...@vger.kernel.org; Justin He (Arm Technology China)
>
> Subject: [PATCH 1/2] drivers/dax/kmem: use default numa_mem_id if
> target_node is invalid
>
> In some platforms(e.g arm64 guest), the NFIT info might not be ready.
> Then target_node might be -1. But if there is a default numa_mem_id(),
> we can use it to avoid unnecessary fatal EINVL error.
>
> devm_memremap_pages() also uses this logic if nid is invalid, we can
> keep the same page with it.
>
> Signed-off-by: Jia He
> ---
> drivers/dax/kmem.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index a02318c6d28a..ad62d551d94e 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c
> @@ -33,9 +33,9 @@ int dev_dax_kmem_probe(struct device *dev)
>*/
> numa_node = dev_dax->target_node;
> if (numa_node < 0) {
> - dev_warn(dev, "rejecting DAX region %pR with invalid
> node: %d\n",
> - res, numa_node);
> - return -EINVAL;
> + dev_warn(dev, "DAX %pR with invalid node, assume it
> as %d\n",
> + res, numa_node, numa_mem_id());
> + numa_node = numa_mem_id();
> }
>
> /* Hotplug starting at the beginning of the next block: */
> --
> 2.17.1
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH 2/2] lib/test_printf: add test of null/invalid pointer dereference for dentry
Hi Petr
> -Original Message-
> From: Petr Mladek
> Sent: 2019年8月16日 16:27
> To: Justin He (Arm Technology China)
> Cc: Geert Uytterhoeven ; Sergey Senozhatsky
> ; Thomas Gleixner ;
> Andy Shevchenko ; linux-
> ker...@vger.kernel.org; Kees Cook ; Steven
> Rostedt (VMware) ; Shuah Khan
> ; Tobin C. Harding
> Subject: Re: [PATCH 2/2] lib/test_printf: add test of null/invalid pointer
> dereference for dentry
>
> On Fri 2019-08-09 09:24:57, Jia He wrote:
> > This add some additional test cases of null/invalid pointer dereference
> > for dentry and file (%pd and %pD)
> >
> > Signed-off-by: Jia He
> > ---
> > lib/test_printf.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 944eb50f3862..befedffeb476 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -455,6 +455,13 @@ dentry(void)
> > test("foo", "%pd", _dentry[0]);
> > test("foo", "%pd2", _dentry[0]);
> >
> > + /* test the null/invalid pointer case for dentry */
> > + test("(null)", "%pd", NULL);
> > + test("(efault)", "%pd", PTR_INVALID);
> > + /* test the null/invalid pointer case for file */
>
> The two comments mention something that is obvious from the code.
>
No problem, ok with me
--
Cheers,
Justin (Jia He)
> I have pushed the patch as is and removed the comments in
> a follow up patch [1]. Both are in printk.git, branch for-5.4.
>
> > + test("(null)", "%pD", NULL);
> > + test("(efault)", "%pD", PTR_INVALID);
>
> Reference:
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/commi
> t/?h=for-5.4=8ebea6ea1a7ed5d67ecbb2a493c716a2a89c0be2
>
> Best Regards,
> Petr
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH 1/2] vsprintf: Prevent crash when dereferencing invalid pointers for %pD
> -Original Message-
> From: Andy Shevchenko
> Sent: 2019年8月9日 18:52
> To: Justin He (Arm Technology China)
> Cc: Petr Mladek ; Andy Shevchenko
> ; Sergey Senozhatsky
> ; Geert Uytterhoeven
> ; Linux Kernel Mailing List ker...@vger.kernel.org>; Thomas Gleixner ; Steven
> Rostedt (VMware) ; Kees Cook
> ; Shuah Khan ; Tobin C.
> Harding
> Subject: Re: [PATCH 1/2] vsprintf: Prevent crash when dereferencing invalid
> pointers for %pD
>
> On Fri, Aug 9, 2019 at 4:28 AM Jia He wrote:
> >
> > Commit 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing
> invalid
> > pointers") prevents most crash except for %pD.
> > There is an additional pointer dereferencing before dentry_name.
> >
> > At least, vma->file can be NULL and be passed to printk %pD in
> > print_bad_pte, which can cause crash.
> >
> > This patch fixes it with introducing a new file_dentry_name.
> >
>
> Reviewed-by: Andy Shevchenko
>
> Perhaps you need to add a Fixes tag
Thanks, Andy
Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid
pointers")
Need I reposted v2?
--
Cheers,
Justin (Jia He)
>
> > Signed-off-by: Jia He
> > ---
> > lib/vsprintf.c | 13 ++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 63937044c57d..b4a119176fdb 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -869,6 +869,15 @@ char *dentry_name(char *buf, char *end, const
> struct dentry *d, struct printf_sp
> > return widen_string(buf, n, end, spec);
> > }
> >
> > +static noinline_for_stack
> > +char *file_dentry_name(char *buf, char *end, const struct file *f,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > + if (check_pointer(, end, f, spec))
> > + return buf;
> > +
> > + return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> > +}
> > #ifdef CONFIG_BLOCK
> > static noinline_for_stack
> > char *bdev_name(char *buf, char *end, struct block_device *bdev,
> > @@ -2166,9 +2175,7 @@ char *pointer(const char *fmt, char *buf, char
> *end, void *ptr,
> > case 'C':
> > return clock(buf, end, ptr, spec, fmt);
> > case 'D':
> > - return dentry_name(buf, end,
> > - ((const struct file
> > *)ptr)->f_path.dentry,
> > - spec, fmt);
> > + return file_dentry_name(buf, end, ptr, spec, fmt);
> > #ifdef CONFIG_BLOCK
> > case 'g':
> > return bdev_name(buf, end, ptr, spec, fmt);
> > --
> > 2.17.1
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH] arm64: mm: add missing PTE_SPECIAL in pte_mkdevmap on arm64
Hi Anshuman
Thanks for the comments, please see my comments below
> -Original Message-
> From: Anshuman Khandual
> Sent: 2019年8月8日 13:19
> To: Justin He (Arm Technology China) ; Catalin
> Marinas ; Will Deacon ;
> Mark Rutland ; James Morse
>
> Cc: Christoffer Dall ; Punit Agrawal
> ; Qian Cai ; Jun Yao
> ; Alex Van Brunt ;
> Robin Murphy ; Thomas Gleixner
> ; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] arm64: mm: add missing PTE_SPECIAL in
> pte_mkdevmap on arm64
>
[...]
> > diff --git a/arch/arm64/include/asm/pgtable.h
> b/arch/arm64/include/asm/pgtable.h
> > index 5fdcfe237338..e09760ece844 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -209,7 +209,7 @@ static inline pmd_t pmd_mkcont(pmd_t pmd)
> >
> > static inline pte_t pte_mkdevmap(pte_t pte)
> > {
> > - return set_pte_bit(pte, __pgprot(PTE_DEVMAP));
> > + return set_pte_bit(pte, __pgprot(PTE_DEVMAP | PTE_SPECIAL));
> > }
> >
> > static inline void set_pte(pte_t *ptep, pte_t pte)
> > @@ -396,7 +396,10 @@ static inline int pmd_protnone(pmd_t pmd)
> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > #define pmd_devmap(pmd)pte_devmap(pmd_pte(pmd))
> > #endif
> > -#define pmd_mkdevmap(pmd)
> pte_pmd(pte_mkdevmap(pmd_pte(pmd)))
> > +static inline pmd_t pmd_mkdevmap(pmd_t pmd)
> > +{
> > + return pte_pmd(set_pte_bit(pmd_pte(pmd),
> __pgprot(PTE_DEVMAP)));
> > +}
>
> Though I could see other platforms like powerpc and x86 following same
> approach (DEVMAP + SPECIAL) for pte so that it checks positive for
> pte_special() but then just DEVMAP for pmd which could never have a
> pmd_special(). But a more fundamental question is - why should a devmap
> be a special pte as well ?
IIUC, special pte bit make things handling easier compare with those arches
which
have no special bit. The memory codes will regard devmap page as a special one
compared with normal page.
Devmap page structure can be stored in ram/pmem/none.
>
> Also in vm_normal_page() why cannot it tests for pte_devmap() before it
> starts looking for CONFIG_ARCH_HAS_PTE_SPECIAL. Is this the only path
> for
AFAICT, yes, but it changes to much besides arm codes.
> which we need to set SPECIAL bit on a devmap pte or there are other paths
> where this semantics is assumed ?
No idea
--
Cheers,
Justin (Jia He)
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
