Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1372416 > unrolled thread

[PATCH 0/5] arm64: Fix behavior of maxcpus=n

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] arm64: Fix behavior of maxcpus=n Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-04-06 13:30 +0200
    [PATCH 1/5] arm64: cpufeature: Add scope for capability check Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-04-06 13:30 +0200
      Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check Will Deacon <will.deacon@arm.com> - 2016-04-14 19:40 +0200
        Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-04-14 19:50 +0200
          Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check Will Deacon <will.deacon@arm.com> - 2016-04-15 15:00 +0200
            Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-04-15 15:30 +0200
    [PATCH 3/5] irqchip/gic: Restore CPU interface checking Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-04-06 13:30 +0200

#1372416 — [PATCH 0/5] arm64: Fix behavior of maxcpus=n

This series is an attempt at fixing the maxcpus=n behavior
on arm64. So far we have disabled hotplugging a CPU > n,
when maxcpus=n is in effect, due to following reasons.

 1) Possible cpu feature incompatibilities with the new CPU
    in heterogeneous systems.
 2) New CPU requiring an errata work around which was not detected
    (and the code patched in) at boot time.
 3) Failure to initialise the PMU in case the supported CPUs are
    not online while probing the PMU.

(1) has been mostly solved with our early CPU feature verification
support. This series tries to address (2) & (3).

(2) is solved by iterating over the known erratas and checking if
the new CPU requires an errata not set in the cpu_hwcaps, failing
which, we kill the CPU.

(3) is ignored and will not be fixed as there is no reliable way of
knowing if there would be a CPU that will be online to support the
PMU.

In the process, also restores the capability to check GIC interface settings
by the firmware on individual CPUs.

Tested on Juno with maxcpus=2 (enables A57 cores and A57-PMU) and
maxcpus=1 (disables the A57 cores and A57-PMU).

This series applies on 4.6-rc2 + "arm64: Support for systems without AArch32 state" [1].

The tree is available here :
	git://linux-arm.org/linux-skp.git maxcpus/v1

Marc Zyngier (2):
  arm64: Allow a capability to be checked on a single CPU
  irqchip/gic: Restore CPU interface checking

Suzuki K Poulose (3):
  arm64: cpufeature: Add scope for capability check
  arm64: Verify CPU errata work arounds on hotplugged CPU
  arm64: Fix behavior of maxcpus=N

 arch/arm64/include/asm/cpufeature.h |   12 +++++++--
 arch/arm64/kernel/cpu_errata.c      |   24 ++++++++++++++++-
 arch/arm64/kernel/cpufeature.c      |   49 +++++++++++++++++++++++------------
 arch/arm64/kernel/smp.c             |   10 -------
 drivers/irqchip/irq-gic.c           |    5 ++--
 5 files changed, 67 insertions(+), 33 deletions(-)

-- 
1.7.9.5

[toc] | [next] | [standalone]

#1372437 — [PATCH 1/5] arm64: cpufeature: Add scope for capability check

Add scope parameter to the arm64_cpu_capabilities::matches(),
so that this can be reused for checking the capability on a
given CPU vs the system wide. By default, the system uses
'system' wide values for setting the CPU_HWCAPs and ELF_HWCAPs.

Cc: James Morse <james.morse@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |    8 +++++++-
 arch/arm64/kernel/cpu_errata.c      |    2 +-
 arch/arm64/kernel/cpufeature.c      |   37 +++++++++++++++++++----------------
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index ca8fb4b..5f10344 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -78,10 +78,16 @@ struct arm64_ftr_reg {
 	struct arm64_ftr_bits	*ftr_bits;
 };
 
+/* scope of capability check */
+enum {
+	SCOPE_SYSTEM,
+	SCOPE_CPU,
+};
+
 struct arm64_cpu_capabilities {
 	const char *desc;
 	u16 capability;
-	bool (*matches)(const struct arm64_cpu_capabilities *);
+	bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope);
 	void (*enable)(void *);		/* Called on all active CPUs */
 	union {
 		struct {	/* To be used for erratum handling only */
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 06afd04..2fd5780 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -22,7 +22,7 @@
 #include <asm/cpufeature.h>
 
 static bool __maybe_unused
-is_affected_midr_range(const struct arm64_cpu_capabilities *entry)
+is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int __unused)
 {
 	return MIDR_IS_CPU_MODEL_RANGE(read_cpuid_id(), entry->midr_model,
 				       entry->midr_range_min,
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index c31be96..d3ee1df 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -71,7 +71,9 @@ DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 
 /* meta feature for alternatives */
 static bool __maybe_unused
-cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry);
+cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused);
+
+static u64 __raw_read_system_reg(u32 sys_id);
 
 static struct arm64_ftr_bits ftr_id_aa64isar0[] = {
 	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 32, 32, 0),
@@ -633,19 +635,23 @@ feature_matches(u64 reg, const struct arm64_cpu_capabilities *entry)
 }
 
 static bool
-has_cpuid_feature(const struct arm64_cpu_capabilities *entry)
+has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
 {
 	u64 val;
 
-	val = read_system_reg(entry->sys_reg);
+	if (scope == SCOPE_SYSTEM)
+		val = read_system_reg(entry->sys_reg);
+	else
+		val = __raw_read_system_reg(entry->sys_reg);
+
 	return feature_matches(val, entry);
 }
 
-static bool has_useable_gicv3_cpuif(const struct arm64_cpu_capabilities *entry)
+static bool has_useable_gicv3_cpuif(const struct arm64_cpu_capabilities *entry, int scope)
 {
 	bool has_sre;
 
-	if (!has_cpuid_feature(entry))
+	if (!has_cpuid_feature(entry, scope))
 		return false;
 
 	has_sre = gic_enable_sre();
@@ -656,7 +662,7 @@ static bool has_useable_gicv3_cpuif(const struct arm64_cpu_capabilities *entry)
 	return has_sre;
 }
 
-static bool has_no_hw_prefetch(const struct arm64_cpu_capabilities *entry)
+static bool has_no_hw_prefetch(const struct arm64_cpu_capabilities *entry, int __unused)
 {
 	u32 midr = read_cpuid_id();
 	u32 rv_min, rv_max;
@@ -668,7 +674,7 @@ static bool has_no_hw_prefetch(const struct arm64_cpu_capabilities *entry)
 	return MIDR_IS_CPU_MODEL_RANGE(midr, MIDR_THUNDERX, rv_min, rv_max);
 }
 
-static bool runs_at_el2(const struct arm64_cpu_capabilities *entry)
+static bool runs_at_el2(const struct arm64_cpu_capabilities *entry, int __unused)
 {
 	return is_kernel_in_hyp_mode();
 }
@@ -830,7 +836,7 @@ static bool cpus_have_elf_hwcap(const struct arm64_cpu_capabilities *cap)
 static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps)
 {
 	for (; hwcaps->matches; hwcaps++)
-		if (hwcaps->matches(hwcaps))
+		if (hwcaps->matches(hwcaps, SCOPE_SYSTEM))
 			cap_set_elf_hwcap(hwcaps);
 }
 
@@ -838,7 +844,7 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
 			    const char *info)
 {
 	for (; caps->matches; caps++) {
-		if (!caps->matches(caps))
+		if (!caps->matches(caps, SCOPE_SYSTEM))
 			continue;
 
 		if (!cpus_have_cap(caps->capability) && caps->desc)
@@ -929,28 +935,25 @@ static void
 verify_local_elf_hwcaps(const struct arm64_cpu_capabilities *caps)
 {
 
-	for (; caps->matches; caps++) {
-		if (!cpus_have_elf_hwcap(caps))
-			continue;
-		if (!feature_matches(__raw_read_system_reg(caps->sys_reg), caps)) {
+	for (; caps->matches; caps++)
+		if (cpus_have_elf_hwcap(caps) && !caps->matches(caps, SCOPE_CPU)) {
 			pr_crit("CPU%d: missing HWCAP: %s\n",
 					smp_processor_id(), caps->desc);
 			cpu_die_early();
 		}
-	}
 }
 
 static void
 verify_local_cpu_features(const struct arm64_cpu_capabilities *caps)
 {
 	for (; caps->matches; caps++) {
-		if (!cpus_have_cap(caps->capability) || !caps->sys_reg)
+		if (!cpus_have_cap(caps->capability))
 			continue;
 		/*
 		 * If the new CPU misses an advertised feature, we cannot proceed
 		 * further, park the cpu.
 		 */
-		if (!feature_matches(__raw_read_system_reg(caps->sys_reg), caps)) {
+		if (!caps->matches(caps, SCOPE_CPU)) {
 			pr_crit("CPU%d: missing feature: %s\n",
 					smp_processor_id(), caps->desc);
 			cpu_die_early();
@@ -1021,7 +1024,7 @@ void __init setup_cpu_features(void)
 }
 
 static bool __maybe_unused
-cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry)
+cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused)
 {
 	return (cpus_have_cap(ARM64_HAS_PAN) && !cpus_have_cap(ARM64_HAS_UAO));
 }
-- 
1.7.9.5

[toc] | [prev] | [next] | [standalone]

#1379147 — Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check

Hi Suzuki,

On Wed, Apr 06, 2016 at 12:24:10PM +0100, Suzuki K Poulose wrote:
> Add scope parameter to the arm64_cpu_capabilities::matches(),
> so that this can be reused for checking the capability on a
> given CPU vs the system wide. By default, the system uses
> 'system' wide values for setting the CPU_HWCAPs and ELF_HWCAPs.
> 
> Cc: James Morse <james.morse@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/cpufeature.h |    8 +++++++-
>  arch/arm64/kernel/cpu_errata.c      |    2 +-
>  arch/arm64/kernel/cpufeature.c      |   37 +++++++++++++++++++----------------
>  3 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index ca8fb4b..5f10344 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -78,10 +78,16 @@ struct arm64_ftr_reg {
>  	struct arm64_ftr_bits	*ftr_bits;
>  };
>  
> +/* scope of capability check */
> +enum {
> +	SCOPE_SYSTEM,
> +	SCOPE_CPU,
> +};
> +
>  struct arm64_cpu_capabilities {
>  	const char *desc;
>  	u16 capability;
> -	bool (*matches)(const struct arm64_cpu_capabilities *);
> +	bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope);
>  	void (*enable)(void *);		/* Called on all active CPUs */
>  	union {
>  		struct {	/* To be used for erratum handling only */
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 06afd04..2fd5780 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -22,7 +22,7 @@
>  #include <asm/cpufeature.h>
>  
>  static bool __maybe_unused
> -is_affected_midr_range(const struct arm64_cpu_capabilities *entry)
> +is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int __unused)

Maybe it would be better to WARN if somebody passes SCOPE_SYSTEM, rather
than silently treat it as per-cpu?

Will

[toc] | [prev] | [next] | [standalone]

#1379157 — Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check

On 14/04/16 18:38, Will Deacon wrote:
> Hi Suzuki,
>
> On Wed, Apr 06, 2016 at 12:24:10PM +0100, Suzuki K Poulose wrote:
>> Add scope parameter to the arm64_cpu_capabilities::matches(),
>> so that this can be reused for checking the capability on a
>> given CPU vs the system wide. By default, the system uses
>> 'system' wide values for setting the CPU_HWCAPs and ELF_HWCAPs.

>>   static bool __maybe_unused
>> -is_affected_midr_range(const struct arm64_cpu_capabilities *entry)
>> +is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int __unused)
>
> Maybe it would be better to WARN if somebody passes SCOPE_SYSTEM, rather
> than silently treat it as per-cpu?

Should we worry about errata's which may not necessarily depend on per CPU or
a local capability (GIC) ? If not, we could add a WARN after passing down LOCAL
scope for errata. Right now we always do SCOPE_SYSTEM from update_cpu_capabilities(), even for
cpu_errata table. There is no specific reason for that.

Cheers
Suzuki




>
> Will
>

[toc] | [prev] | [next] | [standalone]

#1379799 — Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check

On Thu, Apr 14, 2016 at 06:47:46PM +0100, Suzuki K Poulose wrote:
> On 14/04/16 18:38, Will Deacon wrote:
> >Hi Suzuki,
> >
> >On Wed, Apr 06, 2016 at 12:24:10PM +0100, Suzuki K Poulose wrote:
> >>Add scope parameter to the arm64_cpu_capabilities::matches(),
> >>so that this can be reused for checking the capability on a
> >>given CPU vs the system wide. By default, the system uses
> >>'system' wide values for setting the CPU_HWCAPs and ELF_HWCAPs.
> 
> >>  static bool __maybe_unused
> >>-is_affected_midr_range(const struct arm64_cpu_capabilities *entry)
> >>+is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int __unused)
> >
> >Maybe it would be better to WARN if somebody passes SCOPE_SYSTEM, rather
> >than silently treat it as per-cpu?
> 
> Should we worry about errata's which may not necessarily depend on per CPU or
> a local capability (GIC) ?

Why would they be calling is_affected_midr_range?

> If not, we could add a WARN after passing down LOCAL
> scope for errata.

But if we don't care about errata that aren't local, then why would we warn
on LOCAL?

> Right now we always do SCOPE_SYSTEM from update_cpu_capabilities(), even for
> cpu_errata table. There is no specific reason for that.

I'm totally confused. Can you define SCOPE_SYSTEM and SCOPE_LOCAL for me,
please?

Will

[toc] | [prev] | [next] | [standalone]

#1379812 — Re: [PATCH 1/5] arm64: cpufeature: Add scope for capability check

On 15/04/16 13:55, Will Deacon wrote:
> On Thu, Apr 14, 2016 at 06:47:46PM +0100, Suzuki K Poulose wrote:
>> On 14/04/16 18:38, Will Deacon wrote:
>>> Hi Suzuki,
>>>
>>> On Wed, Apr 06, 2016 at 12:24:10PM +0100, Suzuki K Poulose wrote:
>>>> Add scope parameter to the arm64_cpu_capabilities::matches(),
>>>> so that this can be reused for checking the capability on a
>>>> given CPU vs the system wide. By default, the system uses
>>>> 'system' wide values for setting the CPU_HWCAPs and ELF_HWCAPs.
>>
>>>>   static bool __maybe_unused
>>>> -is_affected_midr_range(const struct arm64_cpu_capabilities *entry)
>>>> +is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int __unused)
>>>
>>> Maybe it would be better to WARN if somebody passes SCOPE_SYSTEM, rather
>>> than silently treat it as per-cpu?
>>
>> Should we worry about errata's which may not necessarily depend on per CPU or
>> a local capability (GIC) ?

So all of these are arm64_cpu_capabilities, i.e, cpu_errata, ELF_HWCAPS and cpu_hwcaps.
Each of them have a matches() routine, which without this series, checks if the system
as a whole has the capability. Now the matches() could use its own checks using system
wide values (read_system_reg(), used by everything except cpu_errata) or CPU register
values(right now, all of them use MIDR).

The tables are processed by a common routine, update_cpu_capabilities(). The cpu_errata
table is processed for each booting CPU, while the cpu_hwcaps is processed only once,
after all the CPUs have booted.

SCOPE is really a hint to the matches() to tell us the capability of the entire system (SCOPE_SYSTEM)
vs that of a single CPU(SCOPE_LOCAL). This is needed, because matches() may do more than checking
the feature register values (e.g, has_useable_gicv3_cpuif() ), which makes it difficult
to the check if a given CPU has a capability. So we initialise the system capabilities
by passing SCOPE_SYSTEM to the matches() from update_cpu_capabilities(). And we use
SCOPE_LOCAL when we do the early verification check for CPUs that turn online after the
system initialised the bits or this_cpu_has_cap() introduced in this series.
At the moment scope doesn't really matter to cpu_errata, as all of them are using
MIDR checks (btw, it is done for all CPUs unlike, cpu_hwcaps or ELF Hwcaps

>
> Why would they be calling is_affected_midr_range?

They won't be. My point is, since we use a shared structures and shared routines to
process the table, the scope parameter comes from the top, while we go through the
table. The entire table of capabilities::matches() will be called with a specific scope
in either case as explained above.


>> If not, we could add a WARN after passing down LOCAL
>> scope for errata.
>
> But if we don't care about errata that aren't local, then why would we warn
> on LOCAL?

We don't have to.
  

>> Right now we always do SCOPE_SYSTEM from update_cpu_capabilities(), even for
>> cpu_errata table. There is no specific reason for that.
>
> I'm totally confused. Can you define SCOPE_SYSTEM and SCOPE_LOCAL for me,
> please?

Hope it is clear now.

Suzuki

[toc] | [prev] | [next] | [standalone]

#1372441 — [PATCH 3/5] irqchip/gic: Restore CPU interface checking

From: Marc Zyngier <marc.zyngier@arm.com>

When introducing the whole CPU feature detection framework,
we lost the capability to detect a mismatched GIC configuration
(using the GICv2 MMIO interface, but having the system register
interface enabled).

In order to solve this, use the new this_cpu_has_cap() helper.
Also move the check to the CPU interface path in order to catch
systems where the first CPU has been correctly configured,
but the secondaries are not.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/irqchip/irq-gic.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 282344b..095bb5b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -55,7 +55,7 @@
 
 static void gic_check_cpu_features(void)
 {
-	WARN_TAINT_ONCE(cpus_have_cap(ARM64_HAS_SYSREG_GIC_CPUIF),
+	WARN_TAINT_ONCE(this_cpu_has_cap(ARM64_HAS_SYSREG_GIC_CPUIF),
 			TAINT_CPU_OUT_OF_SPEC,
 			"GICv3 system registers enabled, broken firmware!\n");
 }
@@ -490,6 +490,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
 		 * Get what the GIC says our CPU mask is.
 		 */
 		BUG_ON(cpu >= NR_GIC_CPU_IF);
+		gic_check_cpu_features();
 		cpu_mask = gic_get_cpumask(gic);
 		gic_cpu_map[cpu] = cpu_mask;
 
@@ -1021,8 +1022,6 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 
 	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
 
-	gic_check_cpu_features();
-
 	gic = &gic_data[gic_nr];
 
 	/* Initialize irq_chip */
-- 
1.7.9.5

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web