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 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
On Wed, Sep 18, 2019 at 07:20:17AM -0700, Matthew Wilcox wrote:
> On Wed, Sep 18, 2019 at 09:19:12PM +0800, Jia He wrote:
> > +/* Decouple AF from AFDBM. */
> > +bool cpu_has_hw_af(void)
> > +{
> > + return (read_cpuid(ID_AA64MMFR1_EL1) & 0xf);
> > +}
> > +
>
> Do you really want to call read_cpuid() every time? I would have thought
> you'd want to use the static branch mechanism to do the right thing at
> boot time. See Documentation/static-keys.txt.
We do have a static branch mechanism for other CPU features but mostly
because on some big.LITTLE systems, the CPUs may differ slightly so we
only advertise the common features.
I guess here the additional instructions for reading and checking the
CPUID here would be lost in the noise compared to the actual page
copying.
--
Catalin
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).
--
Catalin
Re: [PATCH v4 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
Hi Jia,
On 18/09/2019 14:19, 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
---
arch/arm64/include/asm/cpufeature.h | 1 +
arch/arm64/kernel/cpufeature.c | 6 ++
2 files changed, 7 insertions(+)
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);
Getting the latter is tricky, and I think it is what we are looking
for here. In which case we may need something more to report this.
Kind regards
Suzuki
Re: [PATCH v4 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
On Wed, Sep 18, 2019 at 09:19:12PM +0800, Jia He wrote:
> +/* Decouple AF from AFDBM. */
> +bool cpu_has_hw_af(void)
> +{
> + return (read_cpuid(ID_AA64MMFR1_EL1) & 0xf);
> +}
> +
Do you really want to call read_cpuid() every time? I would have thought
you'd want to use the static branch mechanism to do the right thing at
boot time. See Documentation/static-keys.txt.
[PATCH v4 1/3] 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 new helper cpu_has_hw_af().
Signed-off-by: Jia He
Suggested-by: Suzuki Poulose
---
arch/arm64/include/asm/cpufeature.h | 1 +
arch/arm64/kernel/cpufeature.c | 6 ++
2 files changed, 7 insertions(+)
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)
+{
+ return (read_cpuid(ID_AA64MMFR1_EL1) & 0xf);
+}
+
#endif
#ifdef CONFIG_ARM64_VHE
--
2.17.1
