| Message ID | 20191009084246.123354-2-justin.he@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix double page fault in cow_user_page for pfn mapping | expand |
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 <justin.he@arm.com> > Suggested-by: Suzuki Poulose <Suzuki.Poulose@arm.com> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> I don't think I reviewed this version of the patch. > 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: 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. Anyway, apart from these nitpicks, the patch is fine you can keep my reviewed-by. If later we noticed a potential performance issue on this path, we can turn it into a static label as with other CPU features.
Hi Catalin > -----Original Message----- > From: Catalin Marinas <catalin.marinas@arm.com> > Sent: Friday, October 11, 2019 12:43 AM > To: Justin He (Arm Technology China) <Justin.He@arm.com> > Cc: Will Deacon <will@kernel.org>; Mark Rutland > <Mark.Rutland@arm.com>; James Morse <James.Morse@arm.com>; Marc > Zyngier <maz@kernel.org>; Matthew Wilcox <willy@infradead.org>; Kirill A. > Shutemov <kirill.shutemov@linux.intel.com>; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > mm@kvack.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Borislav > Petkov <bp@alien8.de>; H. Peter Anvin <hpa@zytor.com>; x86@kernel.org; > Thomas Gleixner <tglx@linutronix.de>; Andrew Morton <akpm@linux- > foundation.org>; hejianet@gmail.com; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; nd <nd@arm.com> > 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 <justin.he@arm.com> > > Suggested-by: Suzuki Poulose <Suzuki.Poulose@arm.com> > > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > > 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
On Fri, Oct 11, 2019 at 01:16:36AM +0000, Justin He (Arm Technology China) wrote: > From: Catalin Marinas <catalin.marinas@arm.com> > > 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.
Hi Catanlin Thanks for the detailed explanation. Will send out v12 soon after testing -- Cheers, Justin (Jia He) > -----Original Message----- > From: Catalin Marinas <catalin.marinas@arm.com> > Sent: Friday, October 11, 2019 6:39 PM > To: Justin He (Arm Technology China) <Justin.He@arm.com> > Cc: Will Deacon <will@kernel.org>; Mark Rutland > <Mark.Rutland@arm.com>; James Morse <James.Morse@arm.com>; Marc > Zyngier <maz@kernel.org>; Matthew Wilcox <willy@infradead.org>; Kirill A. > Shutemov <kirill.shutemov@linux.intel.com>; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > mm@kvack.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Borislav > Petkov <bp@alien8.de>; H. Peter Anvin <hpa@zytor.com>; x86@kernel.org; > Thomas Gleixner <tglx@linutronix.de>; Andrew Morton <akpm@linux- > foundation.org>; hejianet@gmail.com; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; nd <nd@arm.com> > Subject: Re: [PATCH v11 1/4] arm64: cpufeature: introduce helper > cpu_has_hw_af() > > On Fri, Oct 11, 2019 at 01:16:36AM +0000, Justin He (Arm Technology China) > wrote: > > From: Catalin Marinas <catalin.marinas@arm.com> > > > 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
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)) { + u64 mmfr1 = read_cpuid(ID_AA64MMFR1_EL1); + + return !!cpuid_feature_extract_unsigned_field(mmfr1, + ID_AA64MMFR1_HADBS_SHIFT); + } + + return false; +} + #endif /* __ASSEMBLY__ */ #endif