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()
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 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.
> 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.
--
Catalin
[PATCH v11 1/4] arm64: cpufeature: introduce helper cpu_has_hw_af()
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
---
arch/arm64/include/asm/cpufeature.h | 14 ++
1 file changed, 14 insertions(+)
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
--
2.17.1
