[RFC 0/3] cpuidle/powernv: Interface to handle idle-stop versioning

[RFC 0/3] cpuidle/powernv: Interface to handle idle-stop versioning

[Pratik Rajesh Sampat]

A design patch series illuminates the idea of handling different
versions of idle-stop, the properties they support and the
quirks that need to be handled before entering or after exiting stop.

It also adds a functionality to identify firmware-enabled-stop and set
the according bits to encapsulate the support and corresponding handling

Corresponding RFC skiboot patch: https://lists.ozlabs.org/pipermail/skiboot/2020-March/016552.html

Pratik Rajesh Sampat (3):
  Interface for an idle-stop dependency structure
  Demonstration of handling an idle-stop quirk version
  Introduce capability for firmware-enabled-stop

 arch/powerpc/include/asm/processor.h  | 19 +++++++++++++++++
 arch/powerpc/kernel/dt_cpu_ftrs.c     | 30 +++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/idle.c | 25 ++++++++++++++++++----
 drivers/cpuidle/cpuidle-powernv.c     |  3 ++-
 4 files changed, 72 insertions(+), 5 deletions(-)

-- 
2.24.1

[RFC 1/3] Interface for an idle-stop dependency structure

[Pratik Rajesh Sampat]

Design patch to introduce the idea of having a dependency structure for
idle-stop. The structure encapsulates the following:
1. Bitmask for version of idle-stop
2. Bitmask for propterties like ENABLE/DISABLE
3. Function pointer which helps handle how the stop must be invoked

The commit lays a foundation for other idle-stop versions to be added
and handled cleanly based on their specified requirments.
Currently it handles the existing "idle-stop" version by setting the
discovery bits and the function pointer.

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 arch/powerpc/include/asm/processor.h  | 17 +++++++++++++++++
 arch/powerpc/kernel/dt_cpu_ftrs.c     |  5 +++++
 arch/powerpc/platforms/powernv/idle.c | 17 +++++++++++++----
 drivers/cpuidle/cpuidle-powernv.c     |  3 ++-
 4 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index eedcbfb9a6ff..da59f01a5c09 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -429,6 +429,23 @@ extern void power4_idle_nap(void);
 extern unsigned long cpuidle_disable;
 enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
 
+#define STOP_ENABLE		0x00000001
+
+#define STOP_VERSION_P9       0x1
+
+/*
+ * Classify the dependencies of the stop states
+ * @idle_stop: function handler to handle the quirk stop version
+ * @cpuidle_prop: Signify support for stop states through kernel and/or firmware
+ * @stop_version: Classify quirk versions for stop states
+ */
+typedef struct {
+	unsigned long (*idle_stop)(unsigned long, bool);
+	uint8_t cpuidle_prop;
+	uint8_t stop_version;
+}stop_deps_t;
+extern stop_deps_t stop_dep;
+
 extern int powersave_nap;	/* set if nap mode can be used in idle loop */
 
 extern void power7_idle_type(unsigned long type);
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 182b4047c1ef..db1a525e090d 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -292,6 +292,8 @@ static int __init feat_enable_idle_stop(struct dt_cpu_feature *f)
 	lpcr |=  LPCR_PECE1;
 	lpcr |=  LPCR_PECE2;
 	mtspr(SPRN_LPCR, lpcr);
+	stop_dep.cpuidle_prop |= STOP_ENABLE;
+	stop_dep.stop_version = STOP_VERSION_P9;
 
 	return 1;
 }
@@ -657,6 +659,9 @@ static void __init cpufeatures_setup_start(u32 isa)
 	}
 }
 
+stop_deps_t stop_dep = {NULL, 0x0, 0x0};
+EXPORT_SYMBOL(stop_dep);
+
 static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
 {
 	const struct dt_cpu_feature_match *m;
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 78599bca66c2..c32cdc37acf4 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -812,7 +812,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
 
 #ifndef CONFIG_KVM_BOOK3S_HV_POSSIBLE
 	__ppc64_runlatch_off();
-	srr1 = power9_idle_stop(psscr, true);
+	srr1 = stop_dep.idle_stop(psscr, true);
 	__ppc64_runlatch_on();
 #else
 	/*
@@ -828,7 +828,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
 	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_IDLE;
 
 	__ppc64_runlatch_off();
-	srr1 = power9_idle_stop(psscr, false);
+	srr1 = stop_dep.idle_stop(psscr, true);
 	__ppc64_runlatch_on();
 
 	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_KERNEL;
@@ -856,7 +856,7 @@ void power9_idle_type(unsigned long stop_psscr_val,
 	psscr = (psscr & ~stop_psscr_mask) | stop_psscr_val;
 
 	__ppc64_runlatch_off();
-	srr1 = power9_idle_stop(psscr, true);
+	srr1 = stop_dep.idle_stop(psscr, true);
 	__ppc64_runlatch_on();
 
 	fini_irq_for_idle_irqsoff();
@@ -1353,8 +1353,17 @@ static int __init pnv_init_idle_states(void)
 	nr_pnv_idle_states = 0;
 	supported_cpuidle_states = 0;
 
-	if (cpuidle_disable != IDLE_NO_OVERRIDE)
+	if (cpuidle_disable != IDLE_NO_OVERRIDE ||
+	    !(stop_dep.cpuidle_prop & STOP_ENABLE))
 		goto out;
+	switch(stop_dep.stop_version) {
+	case STOP_VERSION_P9:
+		stop_dep.idle_stop = power9_idle_stop;
+		break;
+	default:
+		stop_dep.idle_stop = NULL;
+		goto out;
+	}
 	rc = pnv_parse_cpuidle_dt();
 	if (rc)
 		return rc;
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 1b299e801f74..7430a8edf5c9 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -371,7 +371,8 @@ static int powernv_add_idle_states(void)
  */
 static int powernv_idle_probe(void)
 {
-	if (cpuidle_disable != IDLE_NO_OVERRIDE)
+	if (cpuidle_disable != IDLE_NO_OVERRIDE ||
+	    !(stop_dep.cpuidle_prop & STOP_ENABLE))
 		return -ENODEV;
 
 	if (firmware_has_feature(FW_FEATURE_OPAL)) {
-- 
2.24.1

[RFC 2/3] Demonstration of handling an idle-stop quirk version

[Pratik Rajesh Sampat]

Concept patch demonstrating an idle-stop version discovery from the
device tree, along with population its support and versioning. It also
assigns the function pointer to handle any idle-stop specific quirks.

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 arch/powerpc/include/asm/processor.h  |  1 +
 arch/powerpc/kernel/dt_cpu_ftrs.c     | 16 ++++++++++++++++
 arch/powerpc/platforms/powernv/idle.c |  8 ++++++++
 3 files changed, 25 insertions(+)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index da59f01a5c09..277dbabafd02 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -432,6 +432,7 @@ enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
 #define STOP_ENABLE		0x00000001
 
 #define STOP_VERSION_P9       0x1
+#define STOP_VERSION_P9_V1    0x2
 
 /*
  * Classify the dependencies of the stop states
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index db1a525e090d..63e30aa49356 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -298,6 +298,21 @@ static int __init feat_enable_idle_stop(struct dt_cpu_feature *f)
 	return 1;
 }
 
+static int __init feat_enable_idle_stop_quirk(struct dt_cpu_feature *f)
+{
+	u64 lpcr;
+
+	/* Set PECE wakeup modes for ISAv3.0B */
+	lpcr = mfspr(SPRN_LPCR);
+	lpcr |=  LPCR_PECE0;
+	lpcr |=  LPCR_PECE1;
+	lpcr |=  LPCR_PECE2;
+	mtspr(SPRN_LPCR, lpcr);
+	stop_dep.cpuidle_prop |= STOP_ENABLE;
+	stop_dep.stop_version = STOP_VERSION_P9_V1;
+
+	return 1;
+}
 static int __init feat_enable_mmu_hash(struct dt_cpu_feature *f)
 {
 	u64 lpcr;
@@ -592,6 +607,7 @@ static struct dt_cpu_feature_match __initdata
 	{"idle-nap", feat_enable_idle_nap, 0},
 	{"alignment-interrupt-dsisr", feat_enable_align_dsisr, 0},
 	{"idle-stop", feat_enable_idle_stop, 0},
+	{"idle-stop-v1", feat_enable_idle_stop_quirk, 0},
 	{"machine-check-power8", feat_enable_mce_power8, 0},
 	{"performance-monitor-power8", feat_enable_pmu_power8, 0},
 	{"data-stream-control-register", feat_enable_dscr, CPU_FTR_DSCR},
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index c32cdc37acf4..9f5b21da2fc5 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -805,6 +805,11 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
 	return srr1;
 }
 
+static unsigned long power9_idle_quirky_stop(unsigned long psscr, bool mmu_on)
+{
+	return power9_idle_stop(psscr, mmu_on);
+}
+
 #ifdef CONFIG_HOTPLUG_CPU
 static unsigned long power9_offline_stop(unsigned long psscr)
 {
@@ -1360,6 +1365,9 @@ static int __init pnv_init_idle_states(void)
 	case STOP_VERSION_P9:
 		stop_dep.idle_stop = power9_idle_stop;
 		break;
+	case STOP_VERSION_P9_V1:
+		stop_dep.idle_stop = power9_idle_quirky_stop;
+		break;
 	default:
 		stop_dep.idle_stop = NULL;
 		goto out;
-- 
2.24.1

[RFC 3/3] Introduce capability for firmware-enabled-stop

[Pratik Rajesh Sampat]

Design patch that introduces the capability for firmware to handle the
stop states instead. A bit is set based on the discovery of the feature
and correspondingly also the responsibility to handle the stop states.

The commit does not contain calling into the firmware to utilize
firmware enabled stop.

Signed-off-by: Pratik Rajesh Sampat <psampat at linux.ibm.com>
---
 arch/powerpc/include/asm/processor.h | 1 +
 arch/powerpc/kernel/dt_cpu_ftrs.c    | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 277dbabafd02..978fab35d133 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -430,6 +430,7 @@ extern unsigned long cpuidle_disable;
 enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
 
 #define STOP_ENABLE		0x00000001
+#define FIRMWARE_STOP_ENABLE	0x00000010
 
 #define STOP_VERSION_P9       0x1
 #define STOP_VERSION_P9_V1    0x2
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 63e30aa49356..e00f8afabc46 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -313,6 +313,14 @@ static int __init feat_enable_idle_stop_quirk(struct dt_cpu_feature *f)
 
 	return 1;
 }
+
+static int __init feat_enable_firmware_stop(struct dt_cpu_feature *f)
+{
+	stop_dep.cpuidle_prop |= FIRMWARE_STOP_ENABLE;
+
+	return 1;
+}
+
 static int __init feat_enable_mmu_hash(struct dt_cpu_feature *f)
 {
 	u64 lpcr;
@@ -608,6 +616,7 @@ static struct dt_cpu_feature_match __initdata
 	{"alignment-interrupt-dsisr", feat_enable_align_dsisr, 0},
 	{"idle-stop", feat_enable_idle_stop, 0},
 	{"idle-stop-v1", feat_enable_idle_stop_quirk, 0},
+	{"firmware-stop-supported", feat_enable_firmware_stop, 0},
 	{"machine-check-power8", feat_enable_mce_power8, 0},
 	{"performance-monitor-power8", feat_enable_pmu_power8, 0},
 	{"data-stream-control-register", feat_enable_dscr, CPU_FTR_DSCR},
-- 
2.24.1

Re: [RFC 1/3] Interface for an idle-stop dependency structure

[Gautham R Shenoy]

Hi Pratik,

On Wed, Mar 04, 2020 at 09:31:21PM +0530, Pratik Rajesh Sampat wrote:
> Design patch to introduce the idea of having a dependency structure for
> idle-stop. The structure encapsulates the following:
> 1. Bitmask for version of idle-stop
> 2. Bitmask for propterties like ENABLE/DISABLE
> 3. Function pointer which helps handle how the stop must be invoked
> 
> The commit lays a foundation for other idle-stop versions to be added
> and handled cleanly based on their specified requirments.
> Currently it handles the existing "idle-stop" version by setting the
> discovery bits and the function pointer.

So, if this patch is applied, and we are running with an OPAL that
doesn't publish the "idle-stop" dt-cpu-feature, then the goal is to
not enable any stop states. Is this correct ?


> 
> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/processor.h  | 17 +++++++++++++++++
>  arch/powerpc/kernel/dt_cpu_ftrs.c     |  5 +++++
>  arch/powerpc/platforms/powernv/idle.c | 17 +++++++++++++----
>  drivers/cpuidle/cpuidle-powernv.c     |  3 ++-
>  4 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index eedcbfb9a6ff..da59f01a5c09 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -429,6 +429,23 @@ extern void power4_idle_nap(void);
>  extern unsigned long cpuidle_disable;
>  enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
> 
> +#define STOP_ENABLE		0x00000001
> +
> +#define STOP_VERSION_P9       0x1
> +
> +/*
> + * Classify the dependencies of the stop states
> + * @idle_stop: function handler to handle the quirk stop version
> + * @cpuidle_prop: Signify support for stop states through kernel and/or firmware
> + * @stop_version: Classify quirk versions for stop states
> + */
> +typedef struct {
> +	unsigned long (*idle_stop)(unsigned long, bool);
> +	uint8_t cpuidle_prop;
> +	uint8_t stop_version;

Why do we need both cpuidle_prop and stop_version ? 


> +}stop_deps_t;
> +extern stop_deps_t stop_dep;
> +
>  extern int powersave_nap;	/* set if nap mode can be used in idle loop */
> 
>  extern void power7_idle_type(unsigned long type);
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 182b4047c1ef..db1a525e090d 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -292,6 +292,8 @@ static int __init feat_enable_idle_stop(struct dt_cpu_feature *f)
>  	lpcr |=  LPCR_PECE1;
>  	lpcr |=  LPCR_PECE2;
>  	mtspr(SPRN_LPCR, lpcr);
> +	stop_dep.cpuidle_prop |= STOP_ENABLE;
> +	stop_dep.stop_version = STOP_VERSION_P9;

Doesn't stop_version != 0 imply (stop_dep.cpuidle_prop & STOP_ENABLE)?

> 
>  	return 1;
>  }
> @@ -657,6 +659,9 @@ static void __init cpufeatures_setup_start(u32 isa)
>  	}
>  }
> 
> +stop_deps_t stop_dep = {NULL, 0x0, 0x0};
> +EXPORT_SYMBOL(stop_dep);
> +
>  static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
>  {
>  	const struct dt_cpu_feature_match *m;
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 78599bca66c2..c32cdc37acf4 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -812,7 +812,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
> 
>  #ifndef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>  	__ppc64_runlatch_off();
> -	srr1 = power9_idle_stop(psscr, true);
> +	srr1 = stop_dep.idle_stop(psscr, true);
>  	__ppc64_runlatch_on();
>  #else
>  	/*
> @@ -828,7 +828,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
>  	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_IDLE;
> 
>  	__ppc64_runlatch_off();
> -	srr1 = power9_idle_stop(psscr, false);
> +	srr1 = stop_dep.idle_stop(psscr, true);
>  	__ppc64_runlatch_on();
> 
>  	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_KERNEL;
> @@ -856,7 +856,7 @@ void power9_idle_type(unsigned long stop_psscr_val,
>  	psscr = (psscr & ~stop_psscr_mask) | stop_psscr_val;
> 
>  	__ppc64_runlatch_off();
> -	srr1 = power9_idle_stop(psscr, true);
> +	srr1 = stop_dep.idle_stop(psscr, true);
>  	__ppc64_runlatch_on();
>

There is one other place in arch/powerpc/kvm/book3s_hv_rmhandlers.S
where call isa300_idle_stop_mayloss (this is kvm_nap_sequence).

So, if stop states are not supported, then, KVM subsystem should know
about it. Some KVM configurations depend on putting the secondary
threads of the core offline into an idle state whose wakeup is from
0x100 vector. Your patch doesn't address that part.

>  	fini_irq_for_idle_irqsoff();
> @@ -1353,8 +1353,17 @@ static int __init pnv_init_idle_states(void)
>  	nr_pnv_idle_states = 0;
>  	supported_cpuidle_states = 0;
> 
> -	if (cpuidle_disable != IDLE_NO_OVERRIDE)
> +	if (cpuidle_disable != IDLE_NO_OVERRIDE ||
> +	    !(stop_dep.cpuidle_prop & STOP_ENABLE))

This can be (stop_dep.stop_version == STOP_NOT_SUPPORTED) where
#define STOP_NOT_SUPPORTED  0

>  		goto out;
> +	switch(stop_dep.stop_version) {
> +	case STOP_VERSION_P9:
> +		stop_dep.idle_stop = power9_idle_stop;
> +		break;
> +	default:
> +		stop_dep.idle_stop = NULL;

You should add a pr_warn() here that stop state isn't supported
because the kernel doesn't know about the version.



> +		goto out;
> +	}
>  	rc = pnv_parse_cpuidle_dt();
>  	if (rc)
>  		return rc;
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 1b299e801f74..7430a8edf5c9 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -371,7 +371,8 @@ static int powernv_add_idle_states(void)
>   */
>  static int powernv_idle_probe(void)
>  {
> -	if (cpuidle_disable != IDLE_NO_OVERRIDE)
> +	if (cpuidle_disable != IDLE_NO_OVERRIDE ||
> +	    !(stop_dep.cpuidle_prop & STOP_ENABLE))
>  		return -ENODEV;
> 
>  	if (firmware_has_feature(FW_FEATURE_OPAL)) {
> -- 
> 2.24.1
> 

Re: [RFC 3/3] Introduce capability for firmware-enabled-stop

[Gautham R Shenoy]

Hi Pratik,

On Wed, Mar 04, 2020 at 09:31:23PM +0530, Pratik Rajesh Sampat wrote:
> Design patch that introduces the capability for firmware to handle the
> stop states instead. A bit is set based on the discovery of the feature
> and correspondingly also the responsibility to handle the stop states.
> 
> The commit does not contain calling into the firmware to utilize
> firmware enabled stop.
> 
> Signed-off-by: Pratik Rajesh Sampat <psampat at linux.ibm.com>
> ---
>  arch/powerpc/include/asm/processor.h | 1 +
>  arch/powerpc/kernel/dt_cpu_ftrs.c    | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 277dbabafd02..978fab35d133 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -430,6 +430,7 @@ extern unsigned long cpuidle_disable;
>  enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
> 
>  #define STOP_ENABLE		0x00000001
> +#define FIRMWARE_STOP_ENABLE	0x00000010


This could be made a bit in the "version" variable.

> 
>  #define STOP_VERSION_P9       0x1
>  #define STOP_VERSION_P9_V1    0x2
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 63e30aa49356..e00f8afabc46 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -313,6 +313,14 @@ static int __init feat_enable_idle_stop_quirk(struct dt_cpu_feature *f)
> 
>  	return 1;
>  }
> +
> +static int __init feat_enable_firmware_stop(struct dt_cpu_feature *f)
> +{
> +	stop_dep.cpuidle_prop |= FIRMWARE_STOP_ENABLE;

  	stop_dep.cpuidle_version |= FIRMWARE_STOP_V1; or some such
  	variant.


> +
> +	return 1;
> +}
> +
>  static int __init feat_enable_mmu_hash(struct dt_cpu_feature *f)
>  {
>  	u64 lpcr;
> @@ -608,6 +616,7 @@ static struct dt_cpu_feature_match __initdata
>  	{"alignment-interrupt-dsisr", feat_enable_align_dsisr, 0},
>  	{"idle-stop", feat_enable_idle_stop, 0},
>  	{"idle-stop-v1", feat_enable_idle_stop_quirk, 0},
> +	{"firmware-stop-supported", feat_enable_firmware_stop, 0},
>  	{"machine-check-power8", feat_enable_mce_power8, 0},
>  	{"performance-monitor-power8", feat_enable_pmu_power8, 0},
>  	{"data-stream-control-register", feat_enable_dscr, CPU_FTR_DSCR},
> -- 
> 2.24.1
> 

Re: [RFC 1/3] Interface for an idle-stop dependency structure

[Pratik Sampat]

Hello Gautham

On 08/04/20 4:21 pm, Gautham R Shenoy wrote:
> Hi Pratik,
>
> On Wed, Mar 04, 2020 at 09:31:21PM +0530, Pratik Rajesh Sampat wrote:
>> Design patch to introduce the idea of having a dependency structure for
>> idle-stop. The structure encapsulates the following:
>> 1. Bitmask for version of idle-stop
>> 2. Bitmask for propterties like ENABLE/DISABLE
>> 3. Function pointer which helps handle how the stop must be invoked
>>
>> The commit lays a foundation for other idle-stop versions to be added
>> and handled cleanly based on their specified requirments.
>> Currently it handles the existing "idle-stop" version by setting the
>> discovery bits and the function pointer.
> So, if this patch is applied, and we are running with an OPAL that
> doesn't publish the "idle-stop" dt-cpu-feature, then the goal is to
> not enable any stop states. Is this correct ?
>
Yes, all states will be disabled with no power saving.

>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/processor.h  | 17 +++++++++++++++++
>>   arch/powerpc/kernel/dt_cpu_ftrs.c     |  5 +++++
>>   arch/powerpc/platforms/powernv/idle.c | 17 +++++++++++++----
>>   drivers/cpuidle/cpuidle-powernv.c     |  3 ++-
>>   4 files changed, 37 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
>> index eedcbfb9a6ff..da59f01a5c09 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -429,6 +429,23 @@ extern void power4_idle_nap(void);
>>   extern unsigned long cpuidle_disable;
>>   enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
>>
>> +#define STOP_ENABLE		0x00000001
>> +
>> +#define STOP_VERSION_P9       0x1
>> +
>> +/*
>> + * Classify the dependencies of the stop states
>> + * @idle_stop: function handler to handle the quirk stop version
>> + * @cpuidle_prop: Signify support for stop states through kernel and/or firmware
>> + * @stop_version: Classify quirk versions for stop states
>> + */
>> +typedef struct {
>> +	unsigned long (*idle_stop)(unsigned long, bool);
>> +	uint8_t cpuidle_prop;
>> +	uint8_t stop_version;
> Why do we need both cpuidle_prop and stop_version ?

The idea is that each stop_version has house multitude of overlapping properties.
So the idea is to give a clean distinction. However, I can see now that the
versioning and properties could be embedded in a single bitmask


>> @@ -657,6 +659,9 @@ static void __init cpufeatures_setup_start(u32 isa)
>>   	}
>>   }
>>
>> +stop_deps_t stop_dep = {NULL, 0x0, 0x0};
>> +EXPORT_SYMBOL(stop_dep);
>> +
>>   static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
>>   {
>>   	const struct dt_cpu_feature_match *m;
>> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
>> index 78599bca66c2..c32cdc37acf4 100644
>> --- a/arch/powerpc/platforms/powernv/idle.c
>> +++ b/arch/powerpc/platforms/powernv/idle.c
>> @@ -812,7 +812,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
>>
>>   #ifndef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>>   	__ppc64_runlatch_off();
>> -	srr1 = power9_idle_stop(psscr, true);
>> +	srr1 = stop_dep.idle_stop(psscr, true);
>>   	__ppc64_runlatch_on();
>>   #else
>>   	/*
>> @@ -828,7 +828,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
>>   	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_IDLE;
>>
>>   	__ppc64_runlatch_off();
>> -	srr1 = power9_idle_stop(psscr, false);
>> +	srr1 = stop_dep.idle_stop(psscr, true);
>>   	__ppc64_runlatch_on();
>>
>>   	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_KERNEL;
>> @@ -856,7 +856,7 @@ void power9_idle_type(unsigned long stop_psscr_val,
>>   	psscr = (psscr & ~stop_psscr_mask) | stop_psscr_val;
>>
>>   	__ppc64_runlatch_off();
>> -	srr1 = power9_idle_stop(psscr, true);
>> +	srr1 = stop_dep.idle_stop(psscr, true);
>>   	__ppc64_runlatch_on();
>>
> There is one other place in arch/powerpc/kvm/book3s_hv_rmhandlers.S
> where call isa300_idle_stop_mayloss (this is kvm_nap_sequence).
>
> So, if stop states are not supported, then, KVM subsystem should know
> about it. Some KVM configurations depend on putting the secondary
> threads of the core offline into an idle state whose wakeup is from
> 0x100 vector. Your patch doesn't address that part.
>
Sure, I'll make sure to address it there too.

>
>>   		goto out;
>> +	switch(stop_dep.stop_version) {
>> +	case STOP_VERSION_P9:
>> +		stop_dep.idle_stop = power9_idle_stop;
>> +		break;
>> +	default:
>> +		stop_dep.idle_stop = NULL;
> You should add a pr_warn() here that stop state isn't supported
> because the kernel doesn't know about the version.
>
Sure


Thanks
Pratik