LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH -next] usb: gadget: fsl: remove unused variable 'driver_desc'
From: YueHaibing @ 2020-03-26  7:14 UTC (permalink / raw)
  To: leoyang.li, balbi, gregkh
  Cc: YueHaibing, linux-usb, linuxppc-dev, linux-kernel

drivers/usb/gadget/udc/fsl_udc_core.c:56:19:
 warning: 'driver_desc' defined but not used [-Wunused-const-variable=]

It is never used, so remove it.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index ec6eda426223..febabde62f71 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -53,7 +53,6 @@
 #define	DMA_ADDR_INVALID	(~(dma_addr_t)0)
 
 static const char driver_name[] = "fsl-usb2-udc";
-static const char driver_desc[] = DRIVER_DESC;
 
 static struct usb_dr_device __iomem *dr_regs;
 
-- 
2.17.1



^ permalink raw reply related

* [PATCH v6 3/3] powerpc/powernv: Preference optimization for SPRs with constant values
From: Pratik Rajesh Sampat @ 2020-03-26  7:10 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	psampat, pratik.r.sampat
In-Reply-To: <20200326071034.12838-1-psampat@linux.ibm.com>

There are SPRs whose values don't tend to change over time and invoking
self-save on them, where the values are gotten each time may turn out to
be inefficient. In that case calling a self-restore where passing the
value makes more sense as, if the value is same the memory location
is not updated.
SPRs that dont change are as follows:
SPRN_HSPRG0,
SPRN_LPCR,
SPRN_PTCR,
SPRN_HMEER,
SPRN_HID0,

There are also SPRs whose values change and/or their value may not be
correcty determinable in the kernel. Eg: MSR and PSSCR

The value of LPCR is dynamic based on if the CPU is entered a stop
state during cpu idle versus cpu hotplug.

Therefore in this optimization patch, introducing the concept of
preference for each SPR to choose from in the case both self-save and
self-restore is supported.

The preference bitmask is shown as below:
----------------------------
|... | 2nd pref | 1st pref |
----------------------------
MSB			  LSB

The preference from higher to lower is from LSB to MSB with a shift of 8
bits.
Example:
Prefer self save first, if not available then prefer self
restore
The preference mask for this scenario will be seen as below.
((FIRMWARE_RESTORE << PREFERENCE_SHIFT) | FIRMWARE_SELF_SAVE)
---------------------------------
|... | Self restore | Self save |
---------------------------------
MSB			        LSB

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/idle.c | 88 +++++++++++++++++++++------
 1 file changed, 70 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index e77b31621081..4d896df51582 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -43,6 +43,31 @@
 #define FIRMWARE_SELF_SAVE    0x2
 #define KERNEL_SAVE_RESTORE   0x4
 
+#define NR_PREFERENCES    2
+#define PREFERENCE_SHIFT  4
+#define PREFERENCE_MASK   0xf
+/*
+ * Bitmask defining the kind of preferences available.
+ * Note : The higher to lower preference is from LSB to MSB, with a shift of
+ * 4 bits.
+ * ----------------------------
+ * |    | 2nd pref | 1st pref |
+ * ----------------------------
+ * MSB			      LSB
+ */
+/* Prefer Restore if available, otherwise unsupported */
+#define PREFER_SELF_RESTORE_ONLY	FIRMWARE_RESTORE
+/* Prefer Save if available, otherwise unsupported */
+#define PREFER_SELF_SAVE_ONLY		FIRMWARE_SELF_SAVE
+/* Prefer Restore when available, otherwise prefer Save */
+#define PREFER_RESTORE_SAVE		((FIRMWARE_SELF_SAVE << \
+					  PREFERENCE_SHIFT)\
+					  | FIRMWARE_RESTORE)
+/* Prefer Save when available, otherwise prefer Restore*/
+#define PREFER_SAVE_RESTORE		((FIRMWARE_RESTORE <<\
+					  PREFERENCE_SHIFT)\
+					  | FIRMWARE_SELF_SAVE)
+
 static u32 supported_cpuidle_states;
 struct pnv_idle_states_t *pnv_idle_states;
 int nr_pnv_idle_states;
@@ -52,6 +77,7 @@ static bool is_ptcr_self_save;
 
 struct preferred_sprs {
 	u64 spr;
+	u32 preferred_mode;
 	u32 supported_mode;
 };
 
@@ -66,42 +92,52 @@ struct preferred_sprs {
 struct preferred_sprs preferred_sprs[] = {
 	{
 		.spr = SPRN_HSPRG0,
+		.preferred_mode = PREFER_RESTORE_SAVE,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = SPRN_LPCR,
+		.preferred_mode = PREFER_SAVE_RESTORE,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = SPRN_PTCR,
+		.preferred_mode = PREFER_RESTORE_SAVE,
 		.supported_mode = KERNEL_SAVE_RESTORE,
 	},
 	{
 		.spr = SPRN_HMEER,
+		.preferred_mode = PREFER_RESTORE_SAVE,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = SPRN_HID0,
+		.preferred_mode = PREFER_RESTORE_SAVE,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = P9_STOP_SPR_MSR,
+		.preferred_mode = PREFER_SAVE_RESTORE,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = P9_STOP_SPR_PSSCR,
+		.preferred_mode = PREFER_SAVE_RESTORE,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = SPRN_HID1,
+		.preferred_mode = PREFER_RESTORE_SAVE,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = SPRN_HID4,
+		.preferred_mode = PREFER_SELF_RESTORE_ONLY,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
 	{
 		.spr = SPRN_HID5,
+		.preferred_mode = PREFER_SELF_RESTORE_ONLY,
 		.supported_mode = FIRMWARE_RESTORE,
 	}
 };
@@ -218,7 +254,9 @@ static int pnv_self_restore_sprs(u64 pir, int cpu, u64 spr)
 
 static int pnv_self_save_restore_sprs(void)
 {
-	int rc, index, cpu;
+	int rc, index, cpu, k;
+	bool is_initialized;
+	u32 preferred;
 	u64 pir;
 	struct preferred_sprs curr_spr;
 
@@ -234,26 +272,40 @@ static int pnv_self_save_restore_sprs(void)
 			     curr_spr.spr == SPRN_HID4  ||
 			     curr_spr.spr == SPRN_HID5))
 				continue;
-
-			if (curr_spr.supported_mode & FIRMWARE_SELF_SAVE) {
-				rc = opal_slw_self_save_reg(pir,
-							curr_spr.spr);
-				if (rc != 0)
-					return rc;
-				switch (curr_spr.spr) {
-				case SPRN_LPCR:
-					is_lpcr_self_save = true;
+			for (k = 0; k < NR_PREFERENCES; k++) {
+				preferred = curr_spr.preferred_mode
+					& PREFERENCE_MASK;
+				if (preferred & curr_spr.supported_mode &
+				    FIRMWARE_SELF_SAVE) {
+					is_initialized = true;
+					rc = opal_slw_self_save_reg(pir,
+								    curr_spr.spr);
+					if (rc != 0)
+						return rc;
+					switch (curr_spr.spr) {
+					case SPRN_LPCR:
+						is_lpcr_self_save = true;
+						break;
+					case SPRN_PTCR:
+						is_ptcr_self_save = true;
+						break;
+					}
 					break;
-				case SPRN_PTCR:
-					is_ptcr_self_save = true;
+				} else if (preferred & curr_spr.supported_mode &
+					   FIRMWARE_RESTORE) {
+					is_initialized = true;
+					rc = pnv_self_restore_sprs(pir, cpu,
+								   curr_spr.spr);
+					if (rc != 0)
+						return rc;
 					break;
 				}
-			} else if (curr_spr.supported_mode & FIRMWARE_RESTORE) {
-				rc = pnv_self_restore_sprs(pir, cpu,
-							   curr_spr.spr);
-				if (rc != 0)
-					return rc;
-			} else {
+				preferred_sprs[index].preferred_mode =
+					preferred_sprs[index].preferred_mode >>
+					PREFERENCE_SHIFT;
+				curr_spr = preferred_sprs[index];
+			}
+			if (!is_initialized) {
 				if (curr_spr.supported_mode & KERNEL_SAVE_RESTORE ||
 				    (cpu_has_feature(CPU_FTR_ARCH_300) &&
 				     (curr_spr.spr == SPRN_HID1 ||
-- 
2.17.1


^ permalink raw reply related

* [PATCH v6 2/3] powerpc/powernv: Introduce support and parsing for self-save API
From: Pratik Rajesh Sampat @ 2020-03-26  7:10 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	psampat, pratik.r.sampat
In-Reply-To: <20200326071034.12838-1-psampat@linux.ibm.com>

This commit introduces and leverages the Self save API. The difference
between self-save and self-restore is that the value to be saved for the
SPR does not need to be passed to the call.

Add the new Self Save OPAL API call in the list of OPAL calls.
Implement the self saving of the SPRs based on the support populated.
This commit imposes the self-save over self-restore in case both are
supported for a particular SPR.

Along with support for self-save, kernel supported save restore is also
populated in the list. This property is only populated for those SPRs
which encapsulate support from the kernel and hav ethe possibility to
garner support from a firmware mode too.

In addition, the commit also parses the device tree for nodes self-save,
self-restore and populate support for the preferred SPRs based on what
was advertised by the device tree.

In the case a SPR is supported by the firmware self-save, self-restore
and kernel save restore then the preference of execution is also in the
same order as above.

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 .../bindings/powerpc/opal/power-mgt.txt       |  18 +++
 arch/powerpc/include/asm/opal-api.h           |   3 +-
 arch/powerpc/include/asm/opal.h               |   1 +
 arch/powerpc/platforms/powernv/idle.c         | 131 +++++++++++++++++-
 arch/powerpc/platforms/powernv/opal-call.c    |   1 +
 5 files changed, 146 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt b/Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
index 9d619e955576..5fb03c6d7de9 100644
--- a/Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
+++ b/Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
@@ -116,3 +116,21 @@ otherwise. The length of all the property arrays must be the same.
 	which of the fields of the PMICR are set in the corresponding
 	entries in ibm,cpu-idle-state-pmicr. This is an optional
 	property on POWER8 and is absent on POWER9.
+
+- self-restore:
+ Array of unsigned 64-bit values containing a property for sprn-mask
+ with each bit indicating the index of the supported SPR for the
+ functionality. This is an optional property for both Power8 and Power9
+
+- self-save:
+  Array of unsigned 64-bit values containing a property for sprn-mask
+  with each bit indicating the index of the supported SPR for the
+  functionality. This is an optional property for both Power8 and Power9
+
+Example of arrangement of self-restore and self-save arrays:
+For instance if PSSCR is supported, the value is 0x357 = 855.
+Since the array is of 64 bit values, the index of the array is determined by
+855 / 64 = 13th element. Within that index, the bit number is determined by
+855 % 64 = 23rd bit.
+This means that if the 23rd bit in array[13] is set, then that SPR is supported
+by the corresponding self-save or self-restore API.
diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index c1f25a760eb1..1b6e1a68d431 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -214,7 +214,8 @@
 #define OPAL_SECVAR_GET				176
 #define OPAL_SECVAR_GET_NEXT			177
 #define OPAL_SECVAR_ENQUEUE_UPDATE		178
-#define OPAL_LAST				178
+#define OPAL_SLW_SELF_SAVE_REG			181
+#define OPAL_LAST				181
 
 #define QUIESCE_HOLD			1 /* Spin all calls at entry */
 #define QUIESCE_REJECT			2 /* Fail all calls with OPAL_BUSY */
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 9986ac34b8e2..a370b0e8d899 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -204,6 +204,7 @@ int64_t opal_handle_hmi2(__be64 *out_flags);
 int64_t opal_register_dump_region(uint32_t id, uint64_t start, uint64_t end);
 int64_t opal_unregister_dump_region(uint32_t id);
 int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
+int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
 int64_t opal_config_cpu_idle_state(uint64_t state, uint64_t flag);
 int64_t opal_pci_set_phb_cxl_mode(uint64_t phb_id, uint64_t mode, uint64_t pe_number);
 int64_t opal_pci_get_pbcq_tunnel_bar(uint64_t phb_id, uint64_t *addr);
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 858ceb86394d..e77b31621081 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -35,13 +35,20 @@
 /*
  * Type of support for each SPR
  * FIRMWARE_RESTORE: firmware restoration supported: calls self-restore OPAL API
+ * FIRMWARE_SELF_SAVE: firmware save and restore: calls self-save OPAL API
+ * KERNEL_SAVE_RESTORE: kernel handles the saving and restoring of SPR
  */
 #define UNSUPPORTED           0x0
 #define FIRMWARE_RESTORE      0x1
+#define FIRMWARE_SELF_SAVE    0x2
+#define KERNEL_SAVE_RESTORE   0x4
 
 static u32 supported_cpuidle_states;
 struct pnv_idle_states_t *pnv_idle_states;
 int nr_pnv_idle_states;
+/* Caching the lpcr & ptcr support to use later */
+static bool is_lpcr_self_save;
+static bool is_ptcr_self_save;
 
 struct preferred_sprs {
 	u64 spr;
@@ -51,6 +58,10 @@ struct preferred_sprs {
 /*
  * Supported mode: Default support. Can be overwritten during system
  *		   initialization
+ * Note: SPRs with support for KERNEL_SAVE_RESTORE in this list are only those
+ * which have a possibility of support from another firmware mode (i.e self-save
+ * or self-restore)
+ * SPRs with exclusive kernel save support are implicit.
  */
 struct preferred_sprs preferred_sprs[] = {
 	{
@@ -61,6 +72,10 @@ struct preferred_sprs preferred_sprs[] = {
 		.spr = SPRN_LPCR,
 		.supported_mode = FIRMWARE_RESTORE,
 	},
+	{
+		.spr = SPRN_PTCR,
+		.supported_mode = KERNEL_SAVE_RESTORE,
+	},
 	{
 		.spr = SPRN_HMEER,
 		.supported_mode = FIRMWARE_RESTORE,
@@ -219,11 +234,33 @@ static int pnv_self_save_restore_sprs(void)
 			     curr_spr.spr == SPRN_HID4  ||
 			     curr_spr.spr == SPRN_HID5))
 				continue;
-			if (curr_spr.supported_mode & FIRMWARE_RESTORE) {
+
+			if (curr_spr.supported_mode & FIRMWARE_SELF_SAVE) {
+				rc = opal_slw_self_save_reg(pir,
+							curr_spr.spr);
+				if (rc != 0)
+					return rc;
+				switch (curr_spr.spr) {
+				case SPRN_LPCR:
+					is_lpcr_self_save = true;
+					break;
+				case SPRN_PTCR:
+					is_ptcr_self_save = true;
+					break;
+				}
+			} else if (curr_spr.supported_mode & FIRMWARE_RESTORE) {
 				rc = pnv_self_restore_sprs(pir, cpu,
 							   curr_spr.spr);
 				if (rc != 0)
 					return rc;
+			} else {
+				if (curr_spr.supported_mode & KERNEL_SAVE_RESTORE ||
+				    (cpu_has_feature(CPU_FTR_ARCH_300) &&
+				     (curr_spr.spr == SPRN_HID1 ||
+				      curr_spr.spr == SPRN_HID4 ||
+				      curr_spr.spr == SPRN_HID5)))
+					continue;
+				return OPAL_UNSUPPORTED;
 			}
 		}
 	}
@@ -762,7 +799,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
 		mmcr0		= mfspr(SPRN_MMCR0);
 	}
 	if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
-		sprs.lpcr	= mfspr(SPRN_LPCR);
+		if (!is_lpcr_self_save)
+			sprs.lpcr	= mfspr(SPRN_LPCR);
 		sprs.hfscr	= mfspr(SPRN_HFSCR);
 		sprs.fscr	= mfspr(SPRN_FSCR);
 		sprs.pid	= mfspr(SPRN_PID);
@@ -776,7 +814,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
 		sprs.mmcr1	= mfspr(SPRN_MMCR1);
 		sprs.mmcr2	= mfspr(SPRN_MMCR2);
 
-		sprs.ptcr	= mfspr(SPRN_PTCR);
+		if (!is_ptcr_self_save)
+			sprs.ptcr	= mfspr(SPRN_PTCR);
 		sprs.rpr	= mfspr(SPRN_RPR);
 		sprs.tscr	= mfspr(SPRN_TSCR);
 		if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
@@ -860,7 +899,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
 		goto core_woken;
 
 	/* Per-core SPRs */
-	mtspr(SPRN_PTCR,	sprs.ptcr);
+	if (!is_ptcr_self_save)
+		mtspr(SPRN_PTCR,	sprs.ptcr);
 	mtspr(SPRN_RPR,		sprs.rpr);
 	mtspr(SPRN_TSCR,	sprs.tscr);
 
@@ -881,7 +921,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
 	atomic_unlock_and_stop_thread_idle();
 
 	/* Per-thread SPRs */
-	mtspr(SPRN_LPCR,	sprs.lpcr);
+	if (!is_lpcr_self_save)
+		mtspr(SPRN_LPCR,	sprs.lpcr);
 	mtspr(SPRN_HFSCR,	sprs.hfscr);
 	mtspr(SPRN_FSCR,	sprs.fscr);
 	mtspr(SPRN_PID,		sprs.pid);
@@ -1060,8 +1101,10 @@ void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val)
 	 * Program the LPCR via stop-api only if the deepest stop state
 	 * can lose hypervisor context.
 	 */
-	if (supported_cpuidle_states & OPAL_PM_LOSE_FULL_CONTEXT)
-		opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
+	if (supported_cpuidle_states & OPAL_PM_LOSE_FULL_CONTEXT) {
+		if (!is_lpcr_self_save)
+			opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
+	}
 }
 
 /*
@@ -1316,6 +1359,77 @@ static void __init pnv_probe_idle_states(void)
 		supported_cpuidle_states |= pnv_idle_states[i].flags;
 }
 
+/*
+ * Extracts and populates the self save or restore capabilities
+ * passed from the device tree node
+ */
+static int extract_save_restore_state_dt(struct device_node *np, u32 support)
+{
+	int nr_sprns = 0, i, bitmask_index;
+	u64 *temp_u64;
+	u64 bit_pos;
+
+	nr_sprns = of_property_count_u64_elems(np, "sprn-bitmask");
+	if (nr_sprns <= 0)
+		return -EINVAL;
+	temp_u64 = kcalloc(nr_sprns, sizeof(u64), GFP_KERNEL);
+	if (of_property_read_u64_array(np, "sprn-bitmask",
+				       temp_u64, nr_sprns)) {
+		pr_warn("cpuidle-powernv: failed to find registers in DT\n");
+		kfree(temp_u64);
+		return -EINVAL;
+	}
+	/*
+	 * Populate acknowledgment of support for the sprs in the global vector
+	 * gotten by the registers supplied by the firmware.
+	 * The registers are in a bitmask, bit index within
+	 * that specifies the SPR
+	 */
+	for (i = 0; i < nr_preferred_sprs; i++) {
+		bitmask_index = BIT_ULL_WORD(preferred_sprs[i].spr);
+		bit_pos = BIT_ULL_MASK(preferred_sprs[i].spr);
+		if ((temp_u64[bitmask_index] & bit_pos) == 0) {
+			preferred_sprs[i].supported_mode &= ~support;
+			continue;
+		}
+		preferred_sprs[i].supported_mode |= support;
+	}
+
+	kfree(temp_u64);
+	return 0;
+}
+
+static int pnv_parse_deepstate_dt(void)
+{
+	struct device_node *np;
+	int rc = 0, i;
+
+	/*
+	 * Self restore register population
+	 * In the case the node is not found, the support for self-restore for
+	 * already populated SPRs is *not* cut. This is because self-restore
+	 * assumes legacy support. In an event, self-restore is actually not
+	 * supported then the call to the firmware fails and deep stop states
+	 * will be cut.
+	 */
+	np = of_find_compatible_node(NULL, NULL, "ibm,opal-self-restore");
+	if (np) {
+		rc = extract_save_restore_state_dt(np, FIRMWARE_RESTORE);
+		if (rc != 0)
+			return rc;
+	}
+	/* Self save register population */
+	np = of_find_compatible_node(NULL, NULL, "ibm,opal-self-save");
+	if (!np) {
+		for (i = 0; i < nr_preferred_sprs; i++)
+			preferred_sprs[i].supported_mode &= ~FIRMWARE_SELF_SAVE;
+	} else {
+		rc = extract_save_restore_state_dt(np, FIRMWARE_SELF_SAVE);
+	}
+	of_node_put(np);
+	return rc;
+}
+
 /*
  * This function parses device-tree and populates all the information
  * into pnv_idle_states structure. It also sets up nr_pnv_idle_states
@@ -1464,6 +1578,9 @@ static int __init pnv_init_idle_states(void)
 		return rc;
 	pnv_probe_idle_states();
 
+	rc = pnv_parse_deepstate_dt();
+	if (rc)
+		return rc;
 	if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
 		if (!(supported_cpuidle_states & OPAL_PM_SLEEP_ENABLED_ER1)) {
 			power7_fastsleep_workaround_entry = false;
diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
index 5cd0f52d258f..11e0ceb90de0 100644
--- a/arch/powerpc/platforms/powernv/opal-call.c
+++ b/arch/powerpc/platforms/powernv/opal-call.c
@@ -223,6 +223,7 @@ OPAL_CALL(opal_handle_hmi,			OPAL_HANDLE_HMI);
 OPAL_CALL(opal_handle_hmi2,			OPAL_HANDLE_HMI2);
 OPAL_CALL(opal_config_cpu_idle_state,		OPAL_CONFIG_CPU_IDLE_STATE);
 OPAL_CALL(opal_slw_set_reg,			OPAL_SLW_SET_REG);
+OPAL_CALL(opal_slw_self_save_reg,		OPAL_SLW_SELF_SAVE_REG);
 OPAL_CALL(opal_register_dump_region,		OPAL_REGISTER_DUMP_REGION);
 OPAL_CALL(opal_unregister_dump_region,		OPAL_UNREGISTER_DUMP_REGION);
 OPAL_CALL(opal_pci_set_phb_cxl_mode,		OPAL_PCI_SET_PHB_CAPI_MODE);
-- 
2.17.1


^ permalink raw reply related

* [PATCH v6 1/3] powerpc/powernv: Introduce interface for self-restore support
From: Pratik Rajesh Sampat @ 2020-03-26  7:10 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	psampat, pratik.r.sampat
In-Reply-To: <20200326071034.12838-1-psampat@linux.ibm.com>

Introduces an interface that helps determine support for the
self-restore API. The commit is isomorphic to the original interface of
declaring SPRs to self-restore.

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/idle.c | 200 +++++++++++++++++++-------
 1 file changed, 152 insertions(+), 48 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 78599bca66c2..858ceb86394d 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -32,10 +32,67 @@
 #define P9_STOP_SPR_MSR 2000
 #define P9_STOP_SPR_PSSCR      855
 
+/*
+ * Type of support for each SPR
+ * FIRMWARE_RESTORE: firmware restoration supported: calls self-restore OPAL API
+ */
+#define UNSUPPORTED           0x0
+#define FIRMWARE_RESTORE      0x1
+
 static u32 supported_cpuidle_states;
 struct pnv_idle_states_t *pnv_idle_states;
 int nr_pnv_idle_states;
 
+struct preferred_sprs {
+	u64 spr;
+	u32 supported_mode;
+};
+
+/*
+ * Supported mode: Default support. Can be overwritten during system
+ *		   initialization
+ */
+struct preferred_sprs preferred_sprs[] = {
+	{
+		.spr = SPRN_HSPRG0,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = SPRN_LPCR,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = SPRN_HMEER,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = SPRN_HID0,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = P9_STOP_SPR_MSR,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = P9_STOP_SPR_PSSCR,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = SPRN_HID1,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = SPRN_HID4,
+		.supported_mode = FIRMWARE_RESTORE,
+	},
+	{
+		.spr = SPRN_HID5,
+		.supported_mode = FIRMWARE_RESTORE,
+	}
+};
+
+const int nr_preferred_sprs = ARRAY_SIZE(preferred_sprs);
+
 /*
  * The default stop state that will be used by ppc_md.power_save
  * function on platforms that support stop instruction.
@@ -61,78 +118,125 @@ static bool deepest_stop_found;
 
 static unsigned long power7_offline_type;
 
-static int pnv_save_sprs_for_deep_states(void)
+static int pnv_self_restore_sprs(u64 pir, int cpu, u64 spr)
 {
-	int cpu;
+	u64 reg_val;
 	int rc;
 
-	/*
-	 * hid0, hid1, hid4, hid5, hmeer and lpcr values are symmetric across
-	 * all cpus at boot. Get these reg values of current cpu and use the
-	 * same across all cpus.
-	 */
-	uint64_t lpcr_val	= mfspr(SPRN_LPCR);
-	uint64_t hid0_val	= mfspr(SPRN_HID0);
-	uint64_t hid1_val	= mfspr(SPRN_HID1);
-	uint64_t hid4_val	= mfspr(SPRN_HID4);
-	uint64_t hid5_val	= mfspr(SPRN_HID5);
-	uint64_t hmeer_val	= mfspr(SPRN_HMEER);
-	uint64_t msr_val = MSR_IDLE;
-	uint64_t psscr_val = pnv_deepest_stop_psscr_val;
-
-	for_each_present_cpu(cpu) {
-		uint64_t pir = get_hard_smp_processor_id(cpu);
-		uint64_t hsprg0_val = (uint64_t)paca_ptrs[cpu];
-
-		rc = opal_slw_set_reg(pir, SPRN_HSPRG0, hsprg0_val);
+	switch (spr) {
+	case SPRN_HSPRG0:
+		reg_val = (uint64_t)paca_ptrs[cpu];
+		rc = opal_slw_set_reg(pir, SPRN_HSPRG0, reg_val);
 		if (rc != 0)
 			return rc;
-
-		rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
+		break;
+	case SPRN_LPCR:
+		reg_val = mfspr(SPRN_LPCR);
+		rc = opal_slw_set_reg(pir, SPRN_LPCR, reg_val);
 		if (rc != 0)
 			return rc;
-
+		break;
+	case P9_STOP_SPR_MSR:
+		reg_val = MSR_IDLE;
 		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
-			rc = opal_slw_set_reg(pir, P9_STOP_SPR_MSR, msr_val);
+			rc = opal_slw_set_reg(pir, P9_STOP_SPR_MSR, reg_val);
 			if (rc)
 				return rc;
-
-			rc = opal_slw_set_reg(pir,
-					      P9_STOP_SPR_PSSCR, psscr_val);
-
+		}
+		break;
+	case P9_STOP_SPR_PSSCR:
+		reg_val = pnv_deepest_stop_psscr_val;
+		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+			rc = opal_slw_set_reg(pir, P9_STOP_SPR_PSSCR, reg_val);
 			if (rc)
 				return rc;
 		}
-
-		/* HIDs are per core registers */
+		break;
+	case SPRN_HMEER:
+		reg_val = mfspr(SPRN_HMEER);
 		if (cpu_thread_in_core(cpu) == 0) {
-
-			rc = opal_slw_set_reg(pir, SPRN_HMEER, hmeer_val);
-			if (rc != 0)
+			rc = opal_slw_set_reg(pir, SPRN_HMEER, reg_val);
+			if (rc)
 				return rc;
-
-			rc = opal_slw_set_reg(pir, SPRN_HID0, hid0_val);
-			if (rc != 0)
+		}
+		break;
+	case SPRN_HID0:
+		reg_val = mfspr(SPRN_HID0);
+		if (cpu_thread_in_core(cpu) == 0) {
+			rc = opal_slw_set_reg(pir, SPRN_HID0, reg_val);
+			if (rc)
 				return rc;
+		}
+		break;
+	case SPRN_HID1:
+		reg_val = mfspr(SPRN_HID1);
+		if (!cpu_has_feature(CPU_FTR_ARCH_300) &&
+		    cpu_thread_in_core(cpu) == 0) {
+			rc = opal_slw_set_reg(pir, SPRN_HID1, reg_val);
+			if (rc)
+				return rc;
+		}
+		break;
+	case SPRN_HID4:
+		reg_val = mfspr(SPRN_HID4);
+		if (!cpu_has_feature(CPU_FTR_ARCH_300) &&
+		    cpu_thread_in_core(cpu) == 0) {
+			rc = opal_slw_set_reg(pir, SPRN_HID4, reg_val);
+			if (rc)
+				return rc;
+		}
+		break;
+	case SPRN_HID5:
+		reg_val = mfspr(SPRN_HID5);
+		if (!cpu_has_feature(CPU_FTR_ARCH_300) &&
+		    cpu_thread_in_core(cpu) == 0) {
+			rc = opal_slw_set_reg(pir, SPRN_HID5, reg_val);
+			if (rc)
+				return rc;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
 
-			/* Only p8 needs to set extra HID regiters */
-			if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
-
-				rc = opal_slw_set_reg(pir, SPRN_HID1, hid1_val);
-				if (rc != 0)
-					return rc;
-
-				rc = opal_slw_set_reg(pir, SPRN_HID4, hid4_val);
-				if (rc != 0)
-					return rc;
+static int pnv_self_save_restore_sprs(void)
+{
+	int rc, index, cpu;
+	u64 pir;
+	struct preferred_sprs curr_spr;
 
-				rc = opal_slw_set_reg(pir, SPRN_HID5, hid5_val);
+	for_each_present_cpu(cpu) {
+		pir = get_hard_smp_processor_id(cpu);
+		for (index = 0; index < nr_preferred_sprs; index++) {
+			curr_spr = preferred_sprs[index];
+			/* HIDs are per core register */
+			if (cpu_thread_in_core(cpu) != 0 &&
+			    (curr_spr.spr == SPRN_HMEER ||
+			     curr_spr.spr == SPRN_HID0  ||
+			     curr_spr.spr == SPRN_HID1  ||
+			     curr_spr.spr == SPRN_HID4  ||
+			     curr_spr.spr == SPRN_HID5))
+				continue;
+			if (curr_spr.supported_mode & FIRMWARE_RESTORE) {
+				rc = pnv_self_restore_sprs(pir, cpu,
+							   curr_spr.spr);
 				if (rc != 0)
 					return rc;
 			}
 		}
 	}
+	return 0;
+}
 
+static int pnv_save_sprs_for_deep_states(void)
+{
+	int rc;
+
+	rc = pnv_self_save_restore_sprs();
+	if (rc != 0)
+		return rc;
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply related

* [PATCH v6 0/3] powerpc/powernv: Introduce interface for self-restore support
From: Pratik Rajesh Sampat @ 2020-03-26  7:10 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	psampat, pratik.r.sampat

v5: https://lkml.org/lkml/2020/3/17/944
Changelog
v5-->v6
1. Updated background, motivation and illuminated potential design
choices
2. Re-organization of patch-set
  a. Split introducing preference for optimization from 1/1 to patch 3/3
  b. Merge introducing self-save API and parsing the device-tree
  c. Introduce a supported mode called KERNEL_SAVE_RESTORE which
     outlines and makes kernel supported SPRs for save-restore more
     explicit

Background
==========

The power management framework on POWER systems include core idle
states that lose context. Deep idle states namely "winkle" on POWER8
and "stop4" and "stop5" on POWER9 can be entered by a CPU to save
different levels of power, as a consequence of which all the
hypervisor resources such as SPRs and SCOMs are lost.

For most SPRs, saving and restoration of content for SPRs and SCOMs
is handled by the hypervisor kernel prior to entering an post exit
from an idle state respectively. However, there is a small set of
critical SPRs and XSCOMs that are expected to contain sane values even
before the control is transferred to the hypervisor kernel at system
reset vector.

For this purpose, microcode firmware provides a mechanism to restore
values on certain SPRs. The communication mechanism between the
hypervisor kernel and the microcode is a standard interface called
sleep-winkle-engine (SLW) on Power8 and Stop-API on Power9 which is
abstracted by OPAL calls from the hypervisor kernel. The Stop-API
provides an interface known as the self-restore API, to which the SPR
number and a predefined value to be restored on wake-up from a deep
stop state is supplied.


Motivation to introduce a new Stop-API
======================================

The self-restore API expects not just the SPR number but also the
value with which the SPR is restored. This is good for those SPRs such
as HSPRG0 whose values do not change at runtime, since for them, the
kernel can invoke the self-restore API at boot time once the values of
these SPRs are determined.

However, there are use-cases where-in the value to be saved cannot be
known or cannot be updated in the layer it currently is.
The shortcomings and the new use-cases which cannot be served by the
existing self-restore API, serves as motivation for a new API:

Shortcoming1:
------------
In a special wakeup scenario, SPRs such as PSSCR, whose values can
change at runtime, are compelled to make the self-restore API call
every time before entering a deep-idle state rendering it to be
prohibitively expensive

Shortcoming2:
------------
The value of LPCR is dynamic based on if the CPU is entered a stop
state during cpu idle versus cpu hotplug.
Today, an additional self-restore call is made before entering
CPU-Hotplug to clear the PECE1 bit in stop-API so that if we are
woken up by a special wakeup on an offlined CPU, we go back to stop
with the the bit cleared.
There is a overhead of an extra call

New Use-case:
-------------
In the case where the hypervisor is running on an
ultravisor environment, the boot time is too late in the cycle to make
the self-restore API calls, as these cannot be invoked from an
non-secure context anymore

To address these shortcomings, the firmware provides another API known
as the self-save API. The self-save API only takes the SPR number as a
parameter and will ensure that on wakeup from a deep-stop state the
SPR is restored with the value that it contained prior to entering the
deep-stop.

Contrast between self-save and self-restore APIs
================================================

		  Before entering
                  deep idle     |---------------|
                  ------------> | HCODE A       |
                  |             |---------------|
   |---------|    |
   |   CPU   |----|
   |---------|    |
                  |             |---------------|
                  |------------>| HCODE B       |
                  On waking up  |---------------|
                from deep idle




When a self-restore API is invoked, the HCODE inserts instructions
into "HCODE B" region of the above figure to restore the content of
the SPR to the said value. The "HCODE B" region gets executed soon
after the CPU wakes up from a deep idle state, thus executing the
inserted instructions, thereby restoring the contents of the SPRs to
the required values.

When a self-save API is invoked, the HCODE inserts instructions into
the "HCODE A" region of the above figure to save the content of the
SPR into some location in memory. It also inserts instructions into
the "HCODE B" region to restore the content of the SPR to the
corresponding value saved in the memory by the instructions in "HCODE
A" region.

Thus, in contrast with self-restore, the self-save API *does not* need
a value to be passed to it, since it ensures that the value of SPR
before entering deep stop is saved, and subsequently the same value is
restored.

Self-save and self-restore are complementary features since,
self-restore can help in restoring a different value in the SPR on
wakeup from a deep-idle state than what it had before entering the
deep idle state. This was used in POWER8 for HSPRG0 to distinguish a
wakeup from Winkle vs Fastsleep.

Limitations of self-save
========================
Ideally all SPRs should be available for self-save, but HID0 is very
tricky to implement in microcode due to various endianess quirks.
Couple of implementation schemes were buggy and hence HID0 was left
out to be self-restore only.

The fallout of this limitation is as follows:

* In Non PEF environment, no issue. Linux will use self-restore for
  HID0 as it does today and no functional impact.

* In PEF environment, the HID0 restore value is decided by OPAL during
  boot and it is setup for LE hypervisor with radix MMU. This is the
  default and current working configuration of a PEF environment.
  However if there is a change, then HV Linux will try to change the
  HID0 value to something different than what OPAL decided, at which
  time deep-stop states will be disabled under this new PEF
  environment.

A simple and workable design is achieved by scoping the power
management deep-stop state support only to a known default PEF
environment. Any deviation will affect *only* deep stop-state support
(stop4,5) in that environment and not have any functional impediment
to the environment itself.

In future, if there is a need to support changing of HID0 to various
values under PEF environment and support deep-stop states, it can be
worked out via an ultravisor call or improve the microcode design to
include HID0 in self-save.  These future scheme would be an extension
and does not break or make the current implementation scheme
redundant.

Design Choices
==============

Presenting the design choices in front of us:

Design-Choice 1:
----------------
A simple implementation is to just replace self-restore calls with
self-save as it is direct super-set.

Pros:
A simple design, quick to implement


Cons:
* Breaks backward compatibility. Self-restore has historically been
  supported in the firmware and an old firmware running on an new
  kernel will be incompatible and deep stop states will be cut.
* Furthermore, critical SPRs which need to be restored
  before 0x100 vector like HID0 are not supported by self-save.

Design-Choice 2:
----------------
Advertise both self-restore and self-save from OPAL including the set
of registers that each support. The kernel can then choose which API
to go with.
For the sake of simplicity, in case both modes are supported for an
SPR by default self-save would be called for it.

Pros:
* Backwards compatible

Cons:
Overhead in parsing device tree with the SPR list

Possible optimization with Approach2:
-------------------------------------
There are SPRs whose values don't tend to change over time and invoking
self-save on them, where the values are gotten each time may turn out to
be inefficient. In that case calling a self-restore where passing the
value makes more sense as, if the value is same, the memory location
is not updated.
SPRs that dont change are as follows:
SPRN_HSPRG0,
SPRN_LPCR,
SPRN_PTCR,
SPRN_HMEER,
SPRN_HID0,

The values of PSSCR and MSR change at runtime and hence, the kernel
cannot determine during boot time what their values will be before
entering a particular deep-stop state.

Therefore, a preference based interface is introduced for choosing
between self-save or self-restore between for each SPR.
The per-SPR preference is only a refinement of
approach 2 purely for performance reasons. It can be dropped if the
complexity is not deemed worth the returns.

Patches Organization
====================
Design Choice 2 has been chosen as an implementation to demonstrate in
the patch series.

Patch1:
Devises an interface which lists all the interested SPRs, along with
highlighting the support of mode.
It is an isomorphic patch to replicate the functionality of the older
self-restore firmware for the new interface

Patch2:
Introduces the self-save API and leverages upon the struct interface to
add another supported mode in the mix of saving and restoring. It also
enforces that in case both modes are supported self-save is chosen over
self-restore

The commit also parses the device-tree and populate support for
self-save and self-restore in the supported mask

Patch3:
Introduce an optimization to allow preference to choose between one more
over the one when both both modes are supported. This optimization can
allow for better performance for the SPRs that don't change in value and
hence self-restore is a better alternative, and in cases when it is
known for values to change self-save is more convenient.

Pratik Rajesh Sampat (3):
  powerpc/powernv: Introduce interface for self-restore support
  powerpc/powernv: Introduce support and parsing for self-save API
  powerpc/powernv: Preference optimization for SPRs with constant values

 .../bindings/powerpc/opal/power-mgt.txt       |  18 +
 arch/powerpc/include/asm/opal-api.h           |   3 +-
 arch/powerpc/include/asm/opal.h               |   1 +
 arch/powerpc/platforms/powernv/idle.c         | 385 +++++++++++++++---
 arch/powerpc/platforms/powernv/opal-call.c    |   1 +
 5 files changed, 351 insertions(+), 57 deletions(-)

-- 
2.17.1


^ permalink raw reply

* [PATCH v6 4/4] Advertise the self-save and self-restore attributes in the device tree
From: Pratik Rajesh Sampat @ 2020-03-26  7:09 UTC (permalink / raw)
  To: skiboot, oohall, linux-kernel, linuxppc-dev, mpe, ego, linuxram,
	psampat, pratik.r.sampat
In-Reply-To: <20200326070917.12744-1-psampat@linux.ibm.com>

Support for self save and self restore interface is advertised in the
device tree, along with the list of SPRs it supports for each.

The Special Purpose Register identification is encoded in a 2048 bitmask
structure, where each bit signifies the identification key of that SPR
which is consistent with that of the POWER architecture set for that
register.

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 .../ibm,opal/power-mgt/self-restore.rst       |  27 ++++
 .../ibm,opal/power-mgt/self-save.rst          |  27 ++++
 hw/slw.c                                      | 116 ++++++++++++++++++
 include/skiboot.h                             |   1 +
 4 files changed, 171 insertions(+)
 create mode 100644 doc/device-tree/ibm,opal/power-mgt/self-restore.rst
 create mode 100644 doc/device-tree/ibm,opal/power-mgt/self-save.rst

diff --git a/doc/device-tree/ibm,opal/power-mgt/self-restore.rst b/doc/device-tree/ibm,opal/power-mgt/self-restore.rst
new file mode 100644
index 00000000..2a2269f7
--- /dev/null
+++ b/doc/device-tree/ibm,opal/power-mgt/self-restore.rst
@@ -0,0 +1,27 @@
+ibm,opal/power-mgt/self-restore device tree entries
+===================================================
+
+This node exports the bitmask representing the special purpose registers that
+the self-restore API currently supports.
+
+Example:
+
+.. code-block:: dts
+
+  self-restore {
+        sprn-bitmask = <0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x42010000 0x0 0x0
+                        0x20000 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0
+                        0x0 0x0 0x100000 0x900000 0x0 0x0 0x530000 0x0 0x0 0x0
+                        0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0
+                        0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0
+                        0x0 0x10000>;
+        phandle = <0x1c7>;
+  };
+
+sprn-bitmask
+------------
+
+This property is a bitmask of of all the existing SPRs and if the SPR is
+supported, the corresponding bit of the SPR number is set to 1.
+The representation of the bits are left-right, i.e the MSB of the first
+doubleword represants the 0th bit.
diff --git a/doc/device-tree/ibm,opal/power-mgt/self-save.rst b/doc/device-tree/ibm,opal/power-mgt/self-save.rst
new file mode 100644
index 00000000..c367720e
--- /dev/null
+++ b/doc/device-tree/ibm,opal/power-mgt/self-save.rst
@@ -0,0 +1,27 @@
+ibm,opal/power-mgt/self-save device tree entries
+===================================================
+
+This node exports the bitmask representing the special purpose registers that
+the self-save API currently supports.
+
+Example:
+
+.. code-block:: dts
+
+  self-save {
+        sprn-bitmask = <0x0 0x0 0x0 0x0 0x100000 0x0 0x0 0x0 0x42010000 0x0 0x0
+                        0x20000 0x0 0x0 0x0 0x10000 0x0 0x0 0x0 0x0 0x0 0x0 0x0
+                        0x0 0x0 0x0 0x100000 0x840000 0x0 0x0 0x0 0x0 0x0 0x0
+                        0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0
+                        0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0
+                        0x0 0x10000>;
+        phandle = <0x1c8>;
+  };
+
+sprn-bitmask
+------------
+
+This property is a bitmask of of all the existing SPRs and if the SPR is
+supported, the corresponding bit of the SPR number is set to 1.
+The representation of the bits are left-right, i.e the MSB of the first
+doubleword represants the 0th bit.
diff --git a/hw/slw.c b/hw/slw.c
index 6a09cc2c..9d1fe2c5 100644
--- a/hw/slw.c
+++ b/hw/slw.c
@@ -29,6 +29,7 @@
 #include <sbe_xip_image.h>
 
 static uint32_t slw_saved_reset[0x100];
+#define SPR_BITMAP_LENGTH	2048
 
 static bool slw_current_le = false;
 
@@ -750,6 +751,119 @@ static void slw_late_init_p9(struct proc_chip *chip)
 	}
 }
 
+/* Add device tree properties to determine self-save | restore */
+void add_cpu_self_save_restore_properties(void)
+{
+	struct dt_node *self_restore, *self_save, *power_mgt;
+	uint64_t *self_save_mask, *self_restore_mask;
+	bool self_save_supported = true;
+	uint64_t compVector = -1;
+	struct proc_chip *chip;
+	int i, rc;
+
+	const uint64_t self_restore_regs[] = {
+		P8_SPR_HRMOR,
+		P8_SPR_HMEER,
+		P8_SPR_PMICR,
+		P8_SPR_PMCR,
+		P8_SPR_HID0,
+		P8_SPR_HID1,
+		P8_SPR_HID4,
+		P8_SPR_HID5,
+		P8_SPR_HSPRG0,
+		P8_SPR_LPCR,
+		P8_MSR_MSR
+	};
+
+	const uint64_t self_save_regs[] = {
+		P9_STOP_SPR_DAWR,
+		P9_STOP_SPR_HSPRG0,
+		P9_STOP_SPR_LDBAR,
+		P9_STOP_SPR_LPCR,
+		P9_STOP_SPR_PSSCR,
+		P9_STOP_SPR_MSR,
+		P9_STOP_SPR_HRMOR,
+		P9_STOP_SPR_HMEER,
+		P9_STOP_SPR_PMCR,
+		P9_STOP_SPR_PTCR
+	};
+
+	chip = next_chip(NULL);
+	assert(chip);
+	rc = proc_stop_api_discover_capability((void *) chip->homer_base,
+					       &compVector);
+	if (rc == STOP_SAVE_ARG_INVALID_IMG) {
+		prlog(PR_DEBUG, "HOMER BASE INVALID\n");
+		return;
+	} else if (rc == STOP_SAVE_API_IMG_INCOMPATIBLE) {
+		prlog(PR_DEBUG, "STOP API running incompatible versions\n");
+		if ((compVector & SELF_RESTORE_VER_MISMATCH) == 0) {
+			prlog(PR_DEBUG, "Self-save API unsupported\n");
+			self_save_supported = false;
+		}
+	}
+
+	power_mgt = dt_find_by_path(dt_root, "/ibm,opal/power-mgt");
+	if (!power_mgt) {
+		prerror("OCC: dt node /ibm,opal/power-mgt not found\n");
+		return;
+	}
+
+	self_restore = dt_new(power_mgt, "self-restore");
+	if (!self_restore) {
+		prerror("OCC: Failed to create self restore node");
+		return;
+	}
+
+	self_restore_mask = zalloc(SPR_BITMAP_LENGTH / 8);
+	if (!self_restore_mask)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(self_restore_regs); i++) {
+		int bitmask_idx = self_restore_regs[i] / 64;
+		uint64_t bitmask_pos = self_restore_regs[i] % 64;
+		self_restore_mask[bitmask_idx] |= 1ul << bitmask_pos;
+	}
+
+	for (i = 0; i < (SPR_BITMAP_LENGTH / 64); i++) {
+		self_restore_mask[i] = cpu_to_be64(self_restore_mask[i]);
+	}
+
+	dt_add_property(self_restore, "sprn-bitmask", self_restore_mask,
+			SPR_BITMAP_LENGTH / 8);
+	dt_add_property_string(self_restore, "compatible",
+			       "ibm,opal-self-restore");
+	free(self_restore_mask);
+
+	if (proc_gen != proc_gen_p9 || !self_save_supported)
+		return;
+
+	self_save = dt_new(power_mgt, "self-save");
+	if (!self_save) {
+		prerror("OCC: Failed to create self save node");
+		return;
+	}
+
+	self_save_mask = zalloc(SPR_BITMAP_LENGTH / 8);
+	if (!self_save_mask)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(self_save_regs); i++) {
+		int bitmask_idx = self_save_regs[i] / 64;
+		uint64_t bitmask_pos = self_save_regs[i] % 64;
+		self_save_mask[bitmask_idx] |= 1ul << bitmask_pos;
+	}
+
+	for (i = 0; i < (SPR_BITMAP_LENGTH / 64); i++) {
+		self_save_mask[i] = cpu_to_be64(self_save_mask[i]);
+	}
+
+	dt_add_property(self_save, "sprn-bitmask", self_save_mask,
+			SPR_BITMAP_LENGTH / 8);
+	dt_add_property_string(self_save, "compatible", "ibm,opal-self-save");
+	free(self_save_mask);
+}
+
 /* Add device tree properties to describe idle states */
 void add_cpu_idle_state_properties(void)
 {
@@ -1563,4 +1677,6 @@ void slw_init(void)
 		}
 	}
 	add_cpu_idle_state_properties();
+	if (has_deep_states)
+		add_cpu_self_save_restore_properties();
 }
diff --git a/include/skiboot.h b/include/skiboot.h
index 9ced240e..d3631dea 100644
--- a/include/skiboot.h
+++ b/include/skiboot.h
@@ -209,6 +209,7 @@ extern void early_uart_init(void);
 extern void homer_init(void);
 extern void slw_init(void);
 extern void add_cpu_idle_state_properties(void);
+extern void add_cpu_self_save_restore_properties(void);
 extern void lpc_rtc_init(void);
 
 /* flash support */
-- 
2.24.1


^ permalink raw reply related

* [PATCH v6 3/4] API to verify the STOP API and image compatibility
From: Pratik Rajesh Sampat @ 2020-03-26  7:09 UTC (permalink / raw)
  To: skiboot, oohall, linux-kernel, linuxppc-dev, mpe, ego, linuxram,
	psampat, pratik.r.sampat
In-Reply-To: <20200326070917.12744-1-psampat@linux.ibm.com>

From: Prem Shanker Jha <premjha2@in.ibm.com>

Commit defines a new API primarily intended for OPAL to determine
cpu register save API's compatibility with HOMER layout and
self save restore. It can help OPAL determine if version of
API integrated with OPAL is different from hostboot.

Change-Id: Ic0de45a336cfb8b6b6096a10ac1cd3ffbaa44fc0
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/77612
Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot@us.ibm.com>
Tested-by: Jenkins Server <pfd-jenkins+hostboot@us.ibm.com>
Tested-by: Hostboot CI <hostboot-ci+hostboot@us.ibm.com>
Reviewed-by: RANGANATHPRASAD G. BRAHMASAMUDRA <prasadbgr@in.ibm.com>
Reviewed-by: Gregory S Still <stillgs@us.ibm.com>
Reviewed-by: Jennifer A Stofer <stofer@us.ibm.com>
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/77614
Tested-by: Jenkins OP Build CI <op-jenkins+hostboot@us.ibm.com>
Tested-by: Jenkins OP HW <op-hw-jenkins+hostboot@us.ibm.com>
Reviewed-by: Daniel M Crowell <dcrowell@us.ibm.com>
Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 include/p9_stop_api.H                    | 26 +++++++++++
 libpore/p9_cpu_reg_restore_instruction.H |  7 ++-
 libpore/p9_hcd_memmap_base.H             |  7 +++
 libpore/p9_stop_api.C                    | 58 +++++++++++++++++++++++-
 libpore/p9_stop_api.H                    | 26 ++++++++++-
 libpore/p9_stop_util.H                   | 20 ++++----
 6 files changed, 131 insertions(+), 13 deletions(-)

diff --git a/include/p9_stop_api.H b/include/p9_stop_api.H
index c304f70f..09ce3dc1 100644
--- a/include/p9_stop_api.H
+++ b/include/p9_stop_api.H
@@ -110,6 +110,7 @@ typedef enum
     STOP_SAVE_FAIL                       = 14,  // for internal failure within firmware.
     STOP_SAVE_SPR_ENTRY_MISSING          =  15,
     STOP_SAVE_SPR_BIT_POS_RESERVE        =  16,
+    STOP_SAVE_API_IMG_INCOMPATIBLE       =  18,
 } StopReturnCode_t;
 
 /**
@@ -164,6 +165,14 @@ typedef enum
 
 } ScomSection_t;
 
+/**
+ * @brief   versions pertaining relvant to STOP API.
+ */
+typedef enum
+{
+    STOP_API_VER            =   0x00,
+    STOP_API_VER_CONTROL    =   0x02,
+} VersionList_t;
 
 
 /**
@@ -195,6 +204,14 @@ typedef enum
     BIT_POS_USPRG1      =   30,
 } SprBitPositionList_t;
 
+typedef enum
+{
+    SMF_SUPPORT_MISSING_IN_HOMER         =   0x01,
+    SELF_SUPPORT_MISSING_FOR_LE_HYP      =   0x02,
+    IPL_RUNTIME_CPU_SAVE_VER_MISMATCH    =   0x04,
+    SELF_RESTORE_VER_MISMATCH            =   0x08,
+} VersionIncompList_t;
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -247,6 +264,15 @@ StopReturnCode_t p9_stop_save_scom( void* const   i_pImage,
 StopReturnCode_t
 p9_stop_save_cpureg_control( void* i_pImage, const uint64_t i_pir,
                              const uint32_t  i_saveRegVector );
+
+/**
+ * @brief       verifies if API is compatible of current HOMER image.
+ * @param[in]   i_pImage        points to the start of HOMER image of P9 chip.
+ * @param[out]  o_inCompVector  list of incompatibilities found.
+ * @return      STOP_SAVE_SUCCESS if if API succeeds, error code otherwise.
+ */
+StopReturnCode_t proc_stop_api_discover_capability( void* const i_pImage, uint64_t* o_inCompVector );
+
 #ifdef __cplusplus
 } // extern "C"
 };  // namespace stopImageSection ends
diff --git a/libpore/p9_cpu_reg_restore_instruction.H b/libpore/p9_cpu_reg_restore_instruction.H
index d69a4212..5f168855 100644
--- a/libpore/p9_cpu_reg_restore_instruction.H
+++ b/libpore/p9_cpu_reg_restore_instruction.H
@@ -5,7 +5,7 @@
 /*                                                                        */
 /* OpenPOWER HostBoot Project                                             */
 /*                                                                        */
-/* Contributors Listed Below - COPYRIGHT 2015,2018                        */
+/* Contributors Listed Below - COPYRIGHT 2015,2020                        */
 /* [+] International Business Machines Corp.                              */
 /*                                                                        */
 /*                                                                        */
@@ -69,6 +69,11 @@ enum
     OPCODE_18           =   18,
     SELF_SAVE_FUNC_ADD  =   0x2300,
     SELF_SAVE_OFFSET    =   0x180,
+    SKIP_SPR_REST_INST  =   0x4800001c, //b . +0x01c
+    MFLR_R30            =   0x7fc802a6,
+    SKIP_SPR_SELF_SAVE  =   0x3bff0020, //addi r31 r31, 0x20
+    MTLR_INST           =   0x7fc803a6,  //mtlr r30
+    BRANCH_BE_INST      =   0x48000020,
 };
 
 #define MR_R0_TO_R10            0x7c0a0378UL //mr r10 r0
diff --git a/libpore/p9_hcd_memmap_base.H b/libpore/p9_hcd_memmap_base.H
index 000fafef..ddb56728 100644
--- a/libpore/p9_hcd_memmap_base.H
+++ b/libpore/p9_hcd_memmap_base.H
@@ -444,6 +444,13 @@ HCD_CONST(CME_QUAD_PSTATE_SIZE,                 HALF_KB)
 
 HCD_CONST(CME_REGION_SIZE,                      (64 * ONE_KB))
 
+
+// HOMER compatibility
+
+HCD_CONST(STOP_API_CPU_SAVE_VER,                0x02)
+HCD_CONST(SELF_SAVE_RESTORE_VER,                0x02)
+HCD_CONST(SMF_SUPPORT_SIGNATURE_OFFSET,         0x1300)
+HCD_CONST(SMF_SELF_SIGNATURE,                   (0x5f534d46))
 // Debug
 
 HCD_CONST(CPMR_TRACE_REGION_OFFSET,             (512 * ONE_KB))
diff --git a/libpore/p9_stop_api.C b/libpore/p9_stop_api.C
index 2d9bb549..10e050a1 100644
--- a/libpore/p9_stop_api.C
+++ b/libpore/p9_stop_api.C
@@ -5,7 +5,7 @@
 /*                                                                        */
 /* OpenPOWER HostBoot Project                                             */
 /*                                                                        */
-/* Contributors Listed Below - COPYRIGHT 2015,2018                        */
+/* Contributors Listed Below - COPYRIGHT 2015,2020                        */
 /* [+] International Business Machines Corp.                              */
 /*                                                                        */
 /*                                                                        */
@@ -1828,6 +1828,62 @@ StopReturnCode_t proc_stop_init_self_save(  void* const i_pImage, const uint32_t
     return l_rc;
 }
 
+StopReturnCode_t proc_stop_api_discover_capability( void* const i_pImage, uint64_t * o_inCompVector )
+{
+    StopReturnCode_t l_rc       =   STOP_SAVE_SUCCESS;
+    uint64_t l_incompVector     =   0;
+    uint32_t l_tempWord         =   0;
+    *o_inCompVector             =   0;
+
+    do
+    {
+        if( !i_pImage )
+        {
+            l_rc    =   STOP_SAVE_ARG_INVALID_IMG;
+            break;
+        }
+
+        l_tempWord      =
+                *(uint32_t*)((uint8_t*)i_pImage + CPMR_HOMER_OFFSET + SMF_SUPPORT_SIGNATURE_OFFSET);
+
+        if( l_tempWord != SWIZZLE_4_BYTE(SMF_SELF_SIGNATURE) )
+        {
+            l_incompVector  |=  SMF_SUPPORT_MISSING_IN_HOMER;
+        }
+
+        l_tempWord      =   *(uint32_t *)((uint8_t *)i_pImage + CPMR_HOMER_OFFSET + CPMR_HEADER_SIZE );
+
+        if( l_tempWord != SWIZZLE_4_BYTE(BRANCH_BE_INST) )
+        {
+            l_incompVector  |=  SELF_SUPPORT_MISSING_FOR_LE_HYP;
+        }
+
+        l_tempWord      =   *(uint8_t *)((uint8_t *)i_pImage + CPMR_HOMER_OFFSET + CPMR_SELF_RESTORE_VER_BYTE );
+
+        if( l_tempWord < SELF_SAVE_RESTORE_VER )
+        {
+            l_incompVector  |=  SELF_RESTORE_VER_MISMATCH;
+        }
+
+        l_tempWord      =   *(uint8_t *)((uint8_t *)i_pImage + CPMR_HOMER_OFFSET + CPMR_STOP_API_VER_BYTE );
+
+        if( l_tempWord < STOP_API_CPU_SAVE_VER )
+        {
+            l_incompVector  |=  IPL_RUNTIME_CPU_SAVE_VER_MISMATCH;
+        }
+
+        *o_inCompVector     =   l_incompVector;
+
+        if( l_incompVector )
+        {
+            l_rc    =  STOP_SAVE_API_IMG_INCOMPATIBLE;
+        }
+
+    }while(0);
+
+    return l_rc;
+}
+
 #ifdef __cplusplus
 } //namespace stopImageSection ends
 
diff --git a/libpore/p9_stop_api.H b/libpore/p9_stop_api.H
index 3f6420ff..983a3845 100644
--- a/libpore/p9_stop_api.H
+++ b/libpore/p9_stop_api.H
@@ -5,7 +5,7 @@
 /*                                                                        */
 /* OpenPOWER HostBoot Project                                             */
 /*                                                                        */
-/* Contributors Listed Below - COPYRIGHT 2015,2018                        */
+/* Contributors Listed Below - COPYRIGHT 2015,2020                        */
 /* [+] International Business Machines Corp.                              */
 /*                                                                        */
 /*                                                                        */
@@ -114,6 +114,7 @@ typedef enum
     STOP_SAVE_FAIL                       =  14,  // for internal failure within firmware.
     STOP_SAVE_SPR_ENTRY_MISSING          =  15,
     STOP_SAVE_SPR_BIT_POS_RESERVE        =  16,
+    STOP_SAVE_API_IMG_INCOMPATIBLE       =  18,
 } StopReturnCode_t;
 
 /**
@@ -198,6 +199,21 @@ typedef enum
     BIT_POS_USPRG1      =   30,
 } SprBitPositionList_t;
 
+/**
+ * @brief   List of major incompatibilities between API version.
+ * @note    STOP APIs assumes a specific HOMER layout, certain
+ * level of CME-SGPE hcode and certain version of self-save restore
+ * binary. A mismatch can break STOP function.
+ */
+
+typedef enum
+{
+    SMF_SUPPORT_MISSING_IN_HOMER         =   0x01,
+    SELF_SUPPORT_MISSING_FOR_LE_HYP      =   0x02,
+    IPL_RUNTIME_CPU_SAVE_VER_MISMATCH    =   0x04,
+    SELF_RESTORE_VER_MISMATCH            =   0x08,
+} VersionIncompList_t;
+
 
 #ifdef __cplusplus
 extern "C" {
@@ -341,6 +357,14 @@ StopReturnCode_t proc_stop_save_cpureg(  void* const i_pImage,
  */
 StopReturnCode_t proc_stop_init_self_save(  void* const i_pImage, const uint32_t i_corePos );
 
+/**
+ * @brief       verifies if API is compatible of current HOMER image.
+ * @param[in]   i_pImage        points to the start of HOMER image of P9 chip.
+ * @param[out]  o_inCompVector  list of incompatibilities found.
+ * @return      STOP_SAVE_SUCCESS if if API succeeds, error code otherwise.
+ */
+StopReturnCode_t proc_stop_api_discover_capability( void* const i_pImage, uint64_t* o_inCompVector );
+
 #ifdef __cplusplus
 } // extern "C"
 };  // namespace stopImageSection ends
diff --git a/libpore/p9_stop_util.H b/libpore/p9_stop_util.H
index 79b4e959..1328a54b 100644
--- a/libpore/p9_stop_util.H
+++ b/libpore/p9_stop_util.H
@@ -72,18 +72,18 @@ namespace stopImageSection
     ( (((WORD) >> 8) & 0x00FF) | (((WORD) << 8) & 0xFF00) )
 
 #define SWIZZLE_4_BYTE(WORD) \
-    ( (((WORD) >> 24) & 0x000000FF) | (((WORD) >>  8) & 0x0000FF00) | \
-      (((WORD) <<  8) & 0x00FF0000) | (((WORD) << 24) & 0xFF000000) )
+    ( (((WORD) & 0x000000FF) << 24) | (((WORD) & 0x0000FF00) <<  8) | \
+      (((WORD) & 0x00FF0000) >>  8) | (((WORD) & 0xFF000000) >> 24) )
 
 #define SWIZZLE_8_BYTE(WORD) \
-    ( (((WORD) >> 56) & 0x00000000000000FF) |  \
-      (((WORD) >> 40) & 0x000000000000FF00)| \
-      (((WORD) >> 24) & 0x0000000000FF0000) |  \
-      (((WORD) >>  8) & 0x00000000FF000000) |  \
-      (((WORD) <<  8) & 0x000000FF00000000) |  \
-      (((WORD) << 24) & 0x0000FF0000000000) | \
-      (((WORD) << 40) & 0x00FF000000000000) |  \
-      (((WORD) << 56) & 0xFF00000000000000) )
+    ( (((WORD) & 0x00000000000000ffULL) << 56) | \
+      (((WORD) & 0x000000000000ff00ULL) << 40) | \
+      (((WORD) & 0x0000000000ff0000ULL) << 24) | \
+      (((WORD) & 0x00000000ff000000ULL) <<  8) | \
+      (((WORD) & 0x000000ff00000000ULL) >>  8) | \
+      (((WORD) & 0x0000ff0000000000ULL) >> 24) | \
+      (((WORD) & 0x00ff000000000000ULL) >> 40) | \
+      (((WORD) & 0xff00000000000000ULL) >> 56) )
 #endif
 
 /**
-- 
2.24.1


^ permalink raw reply related

* [PATCH v6 2/4] Self save API integration
From: Pratik Rajesh Sampat @ 2020-03-26  7:09 UTC (permalink / raw)
  To: skiboot, oohall, linux-kernel, linuxppc-dev, mpe, ego, linuxram,
	psampat, pratik.r.sampat
In-Reply-To: <20200326070917.12744-1-psampat@linux.ibm.com>

The commit makes the self save API available outside the firmware by defining
an OPAL wrapper.
This wrapper has a similar interface to that of self restore and expects the
cpu pir, SPR number, minus the value of that SPR to be passed in its
paramters and returns OPAL_SUCCESS on success.
The commit also documents both the self-save and the self-restore API
calls along with their working and usage.

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 doc/opal-api/opal-slw-self-save-reg-181.rst | 49 ++++++++++++
 doc/opal-api/opal-slw-set-reg-100.rst       |  5 ++
 doc/power-management.rst                    | 44 ++++++++++
 hw/slw.c                                    | 89 +++++++++++++++++++++
 include/opal-api.h                          |  3 +-
 include/p9_stop_api.H                       | 17 ++++
 include/skiboot.h                           |  3 +
 7 files changed, 209 insertions(+), 1 deletion(-)
 create mode 100644 doc/opal-api/opal-slw-self-save-reg-181.rst

diff --git a/doc/opal-api/opal-slw-self-save-reg-181.rst b/doc/opal-api/opal-slw-self-save-reg-181.rst
new file mode 100644
index 00000000..5aa4c930
--- /dev/null
+++ b/doc/opal-api/opal-slw-self-save-reg-181.rst
@@ -0,0 +1,49 @@
+.. OPAL_SLW_SELF_SAVE_REG:
+
+OPAL_SLW_SELF_SAVE_REG
+======================
+
+.. code-block:: c
+
+   #define OPAL_SLW_SELF_SAVE_REG			181
+
+   int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
+
+:ref:`OPAL_SLW_SELF_SAVE_REG` is used to inform low-level firmware to save
+the current contents of the SPR before entering a state of loss and
+also restore the content back on waking up from a deep stop state.
+
+An OPAL call `OPAL_SLW_SET_REG` exists which is similar in function as
+saving and restoring the SPR, with one difference being that the value of the
+SPR must also be supplied in the parameters.
+Complete reference: doc/opal-api/opal-slw-set-reg-100.rst
+
+Parameters
+----------
+
+``uint64_t cpu_pir``
+  This parameter specifies the pir of the cpu for which the call is being made.
+``uint64_t sprn``
+  This parameter specifies the spr number as mentioned in p9_stop_api.H
+  The list of SPRs supported is as follows. This list is suppiled through the
+  device tree:
+	P9_STOP_SPR_DAWR,
+	P9_STOP_SPR_HSPRG0,
+	P9_STOP_SPR_LDBAR,
+	P9_STOP_SPR_LPCR,
+	P9_STOP_SPR_PSSCR,
+	P9_STOP_SPR_MSR,
+	P9_STOP_SPR_HRMOR,
+	P9_STOP_SPR_HMEER,
+	P9_STOP_SPR_PMCR,
+	P9_STOP_SPR_PTCR
+
+Returns
+-------
+
+:ref:`OPAL_UNSUPPORTED`
+  If spr restore is not supported by pore engine.
+:ref:`OPAL_PARAMETER`
+  Invalid handle for the pir/chip
+:ref:`OPAL_SUCCESS`
+  On success
diff --git a/doc/opal-api/opal-slw-set-reg-100.rst b/doc/opal-api/opal-slw-set-reg-100.rst
index 2e8f1bd6..ee3e68ce 100644
--- a/doc/opal-api/opal-slw-set-reg-100.rst
+++ b/doc/opal-api/opal-slw-set-reg-100.rst
@@ -21,6 +21,11 @@ In Power 9, it uses p9_stop_save_cpureg(), api provided by self restore code,
 to inform the spr with their corresponding values with which they
 must be restored.
 
+An OPAL call `OPAL_SLW_SELF_SAVE_REG` exists which is similar in function
+saving and restoring the SPR, with one difference being that the value of the
+SPR doesn't need to be passed in the parameters, only with the SPR number
+the firmware can identify, save and restore the values for the same.
+Complete reference: doc/opal-api/opal-slw-self-save-reg-181.rst
 
 Parameters
 ----------
diff --git a/doc/power-management.rst b/doc/power-management.rst
index 76491a71..992a18d0 100644
--- a/doc/power-management.rst
+++ b/doc/power-management.rst
@@ -15,3 +15,47 @@ On boot, specific stop states can be disabled via setting a mask. For example,
 to disable all but stop 0,1,2, use ~0xE0000000. ::
 
   nvram -p ibm,skiboot --update-config opal-stop-state-disable-mask=0x1FFFFFFF
+
+Saving and restoring Special Purpose Registers(SPRs)
+----------------------------------------------------
+
+When a CPU wakes up from a deep stop state which can result in
+hypervisor state loss, all the SPRs are lost. The Linux Kernel expects
+a small set of SPRs to contain an expected value when the CPU wakes up
+from such a deep stop state. The microcode firmware provides the
+following two APIs, collectively known as the stop-APIs, to allow the
+kernel/OPAL to specify this set of SPRs and the value that they need
+to be restored with on waking up from a deep stop state.
+
+Self-restore:
+int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
+The SPR number and the value of the that SPR must be restored with on
+wakeup from the deep-stop state must be specified. When this call is
+made, the microcode inserts instruction into the HOMER region to
+restore the content of the SPR to the specified value on wakeup from a
+deep-stop state. These instructions are executed by the CPU as soon as
+it wakes up from a deep stop state. The call is to be made once per
+SPR.
+
+Self-Save:
+int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
+Only the SPR number needs to be specified. When this call is made, the
+microcode inserts instructions into the HOMER region to save the
+current value of the SPR before the CPU goes to a deep stop state, and
+restores the value back when the CPU wakes up from a deep stop state.
+These instructions are correspondingly executed just before and after
+the CPU goes/comes out of a deep stop state. This call can be made
+once per SPR.
+
+The key difference between self-save and self-restore is the
+use-case. If the Kernel expects the SPR to contain a particular value
+on waking up from a deep-stop state, that wasn't the value of that SPR
+before entering deep stop-state, then self-restore is preferable.
+When deep stop states are to be supported in an Ultravisor
+environment, since HOMER is in a secure region, the stop-api cannot
+update the HOMER if invoked from a context when the OPAL/Kernel is
+executing without the ultravisor privilege. In this scenario, at the
+time of early OPAL boot, while OPAL has ultravisor privileges, it can
+make the self-save stop-api call for all the supported SPRs, so that
+the microcode in the HOMER will always save and restore all the
+supported SPRs during entry/exit from a deep stop state.
diff --git a/hw/slw.c b/hw/slw.c
index beb129a8..6a09cc2c 100644
--- a/hw/slw.c
+++ b/hw/slw.c
@@ -35,6 +35,43 @@ static bool slw_current_le = false;
 enum wakeup_engine_states wakeup_engine_state = WAKEUP_ENGINE_NOT_PRESENT;
 bool has_deep_states = false;
 
+/**
+ * The struct and SPR list is partially consistent with libpore/p9_stop_api.c
+ */
+/**
+ * @brief summarizes attributes associated with a SPR register.
+ */
+typedef struct
+{
+    uint32_t iv_sprId;
+    bool     iv_isThreadScope;
+    uint32_t iv_saveMaskPos;
+
+} StopSprReg_t;
+
+/**
+ * @brief a true in the table below means register is of scope thread
+ * whereas a false meanse register is of scope core.
+ * The number is the bit position on a uint32_t mask
+ */
+
+static const StopSprReg_t g_sprRegister[] =
+{
+	{ P9_STOP_SPR_DAWR,      true,  1   },
+	{ P9_STOP_SPR_HSPRG0,    true,  3   },
+	{ P9_STOP_SPR_LDBAR,     true,  4,  },
+	{ P9_STOP_SPR_LPCR,      true,  5   },
+	{ P9_STOP_SPR_PSSCR,     true,  6   },
+	{ P9_STOP_SPR_MSR,       true,  7   },
+	{ P9_STOP_SPR_HRMOR,     false, 255 },
+	{ P9_STOP_SPR_HID,       false, 21  },
+	{ P9_STOP_SPR_HMEER,     false, 22  },
+	{ P9_STOP_SPR_PMCR,      false, 23  },
+	{ P9_STOP_SPR_PTCR,      false, 24  },
+};
+
+static const uint32_t MAX_SPR_SUPPORTED	= ARRAY_SIZE(g_sprRegister);
+
 DEFINE_LOG_ENTRY(OPAL_RC_SLW_INIT, OPAL_PLATFORM_ERR_EVT, OPAL_SLW,
 		 OPAL_PLATFORM_FIRMWARE, OPAL_PREDICTIVE_ERR_GENERAL,
 		 OPAL_NA);
@@ -1446,6 +1483,58 @@ int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val)
 
 opal_call(OPAL_SLW_SET_REG, opal_slw_set_reg, 3);
 
+int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn)
+{
+	struct cpu_thread * c = find_cpu_by_pir(cpu_pir);
+	uint32_t save_reg_vector = 0;
+	struct proc_chip * chip;
+	int rc;
+	int index;
+
+	if (!c) {
+		prlog(PR_DEBUG, "SLW: Unknown thread with pir %x\n",
+		      (u32) cpu_pir);
+		return OPAL_PARAMETER;
+	}
+
+	chip = get_chip(c->chip_id);
+	if (!chip) {
+		prlog(PR_DEBUG, "SLW: Unknown chip for thread with pir %x\n",
+		      (u32) cpu_pir);
+		return OPAL_PARAMETER;
+	}
+	if (proc_gen != proc_gen_p9 || !has_deep_states) {
+		prlog(PR_DEBUG, "SLW: Does not support deep states\n");
+		return OPAL_UNSUPPORTED;
+	}
+	if (wakeup_engine_state != WAKEUP_ENGINE_PRESENT) {
+		log_simple_error(&e_info(OPAL_RC_SLW_REG),
+			"SLW: wakeup_engine in bad state=%d chip=%x\n",
+			wakeup_engine_state, chip->id);
+		return OPAL_INTERNAL_ERROR;
+	}
+	for (index = 0; index < MAX_SPR_SUPPORTED; ++index) {
+		if (sprn == (CpuReg_t) g_sprRegister[index].iv_sprId) {
+			save_reg_vector = PPC_BIT32(
+				g_sprRegister[index].iv_saveMaskPos);
+			break;
+		}
+	}
+	if (save_reg_vector == 0)
+		return OPAL_INTERNAL_ERROR;
+	rc = p9_stop_save_cpureg_control((void *) chip->homer_base,
+						cpu_pir, save_reg_vector);
+
+	if (rc) {
+		log_simple_error(&e_info(OPAL_RC_SLW_REG),
+			"SLW: Failed to save vector %x for CPU %x\n",
+			save_reg_vector, c->pir);
+		return OPAL_INTERNAL_ERROR;
+	}
+	return OPAL_SUCCESS;
+}
+opal_call(OPAL_SLW_SELF_SAVE_REG, opal_slw_self_save_reg, 2);
+
 void slw_init(void)
 {
 	struct proc_chip *chip;
diff --git a/include/opal-api.h b/include/opal-api.h
index e90cab1e..1607a89b 100644
--- a/include/opal-api.h
+++ b/include/opal-api.h
@@ -227,7 +227,8 @@
 #define OPAL_SECVAR_ENQUEUE_UPDATE		178
 #define OPAL_PHB_SET_OPTION			179
 #define OPAL_PHB_GET_OPTION			180
-#define OPAL_LAST				180
+#define OPAL_SLW_SELF_SAVE_REG			181
+#define OPAL_LAST				181
 
 #define QUIESCE_HOLD			1 /* Spin all calls at entry */
 #define QUIESCE_REJECT			2 /* Fail all calls with OPAL_BUSY */
diff --git a/include/p9_stop_api.H b/include/p9_stop_api.H
index 9d3bc1e5..c304f70f 100644
--- a/include/p9_stop_api.H
+++ b/include/p9_stop_api.H
@@ -34,6 +34,8 @@
 ///
 /// @file   p9_stop_api.H
 /// @brief  describes STOP API which  create/manipulate STOP image.
+///         This header need not be consistent, however is a subset of the
+///         libpore/p9_stop_api.H counterpart
 ///
 // *HWP HW Owner    :  Greg Still <stillgs@us.ibm.com>
 // *HWP FW Owner    :  Prem Shanker Jha <premjha2@in.ibm.com>
@@ -58,6 +60,7 @@ typedef enum
     P9_STOP_SPR_HRMOR   =    313,   // core register
     P9_STOP_SPR_LPCR    =    318,   // thread register
     P9_STOP_SPR_HMEER   =    337,   // core register
+    P9_STOP_SPR_PTCR    =    464,   // core register
     P9_STOP_SPR_LDBAR   =    850,   // thread register
     P9_STOP_SPR_PSSCR   =    855,   // thread register
     P9_STOP_SPR_PMCR    =    884,   // core register
@@ -230,6 +233,20 @@ StopReturnCode_t p9_stop_save_scom( void* const   i_pImage,
                                     const ScomOperation_t i_operation,
                                     const ScomSection_t i_section );
 
+/**
+ * @brief       Facilitates self save and restore of a list of SPRs of a thread.
+ * @param[in]   i_pImage        points to the start of HOMER image of P9 chip.
+ * @param[in]   i_pir           PIR associated with thread
+ * @param[in]   i_saveRegVector bit vector representing SPRs that needs to be restored.
+ * @return      STOP_SAVE_SUCCESS if API succeeds, error code otherwise.
+ * @note        SPR save vector is a bit vector. For each SPR supported,
+ *              there is an associated bit position in the bit vector.Refer
+ *              to definition of SprBitPositionList_t to determine bit position
+ *              associated with a particular SPR.
+ */
+StopReturnCode_t
+p9_stop_save_cpureg_control( void* i_pImage, const uint64_t i_pir,
+                             const uint32_t  i_saveRegVector );
 #ifdef __cplusplus
 } // extern "C"
 };  // namespace stopImageSection ends
diff --git a/include/skiboot.h b/include/skiboot.h
index 30ff500c..9ced240e 100644
--- a/include/skiboot.h
+++ b/include/skiboot.h
@@ -306,6 +306,9 @@ extern void nx_p9_rng_late_init(void);
 /* SLW reinit function for switching core settings */
 extern int64_t slw_reinit(uint64_t flags);
 
+/* Self save SPR before entering the stop state */
+extern int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
+
 /* Patch SPR in SLW image */
 extern int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
 
-- 
2.24.1


^ permalink raw reply related

* [PATCH v6 1/4] Self Save: Introducing Support for SPR Self Save
From: Pratik Rajesh Sampat @ 2020-03-26  7:09 UTC (permalink / raw)
  To: skiboot, oohall, linux-kernel, linuxppc-dev, mpe, ego, linuxram,
	psampat, pratik.r.sampat
In-Reply-To: <20200326070917.12744-1-psampat@linux.ibm.com>

From: Prem Shanker Jha <premjha2@in.ibm.com>

The commit is a merger of commits that makes the following changes:
1. Commit fixes some issues with code found during integration test
  -  replacement of addi with xor instruction during self save API.
  -  fixing instruction generation for MFMSR during self save
  -  data struct updates in STOP API
  -  error RC updates for hcode image build
  -  HOMER parser updates.
  -  removed self save support for URMOR and HRMOR
  -  code changes for compilation with OPAL
  -  populating CME Image header with unsecure HOMER address.

Key_Cronus_Test=PM_REGRESS

Change-Id: I7cedcc466267c4245255d8d75c01ed695e316720
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/66580
Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot@us.ibm.com>
Tested-by: HWSV CI <hwsv-ci+hostboot@us.ibm.com>
Tested-by: PPE CI <ppe-ci+hostboot@us.ibm.com>
Tested-by: Jenkins Server <pfd-jenkins+hostboot@us.ibm.com>
Tested-by: Cronus HW CI <cronushw-ci+hostboot@us.ibm.com>
Tested-by: Hostboot CI <hostboot-ci+hostboot@us.ibm.com>
Reviewed-by: Gregory S. Still <stillgs@us.ibm.com>
Reviewed-by: RAHUL BATRA <rbatra@us.ibm.com>
Reviewed-by: Jennifer A. Stofer <stofer@us.ibm.com>
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/66587
Reviewed-by: Christian R. Geddes <crgeddes@us.ibm.com>
Signed-off-by: Prem Shanker Jha <premjha2@in.ibm.com>
Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>

2. The commit also incorporates changes that make STOP API project
agnostic changes include defining wrapper functions which call legacy
API. It also adds duplicate enum members which start with prefix PROC
instead of P9.

Key_Cronus_Test=PM_REGRESS

Change-Id: If87970f3e8cf9b507f33eb1be249e03eb3836a5e
RTC: 201128
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/71307
Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot@us.ibm.com>
Tested-by: Jenkins Server <pfd-jenkins+hostboot@us.ibm.com>
Tested-by: Hostboot CI <hostboot-ci+hostboot@us.ibm.com>
Tested-by: Cronus HW CI <cronushw-ci+hostboot@us.ibm.com>
Reviewed-by: RANGANATHPRASAD G. BRAHMASAMUDRA <prasadbgr@in.ibm.com>
Reviewed-by: Gregory S. Still <stillgs@us.ibm.com>
Reviewed-by: Jennifer A Stofer <stofer@us.ibm.com>
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/71314
Tested-by: Jenkins OP Build CI <op-jenkins+hostboot@us.ibm.com>
Tested-by: Jenkins OP HW <op-hw-jenkins+hostboot@us.ibm.com>
Reviewed-by: Daniel M. Crowell <dcrowell@us.ibm.com>
Signed-off-by: Prem Shanker Jha <premjha2@in.ibm.com>
Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 include/p9_stop_api.H                    |  79 +-
 libpore/p9_cpu_reg_restore_instruction.H |   4 +
 libpore/p9_stop_api.C                    | 954 +++++++++++++----------
 libpore/p9_stop_api.H                    | 115 ++-
 libpore/p9_stop_data_struct.H            |   4 +-
 libpore/p9_stop_util.H                   |   7 +-
 6 files changed, 721 insertions(+), 442 deletions(-)

diff --git a/include/p9_stop_api.H b/include/p9_stop_api.H
index 79abd000..9d3bc1e5 100644
--- a/include/p9_stop_api.H
+++ b/include/p9_stop_api.H
@@ -63,6 +63,26 @@ typedef enum
     P9_STOP_SPR_PMCR    =    884,   // core register
     P9_STOP_SPR_HID     =   1008,   // core register
     P9_STOP_SPR_MSR     =   2000,   // thread register
+
+    //enum members which are project agnostic
+    PROC_STOP_SPR_DAWR    =    180,   // thread register
+    PROC_STOP_SPR_CIABR   =    187,   // thread register
+    PROC_STOP_SPR_DAWRX   =    188,   // thread register
+    PROC_STOP_SPR_HSPRG0  =    304,   // thread register
+    PROC_STOP_SPR_HRMOR   =    313,   // core register
+    PROC_STOP_SPR_LPCR    =    318,   // thread register
+    PROC_STOP_SPR_HMEER   =    337,   // core register
+    PROC_STOP_SPR_PTCR    =    464,   // core register
+    PROC_STOP_SPR_USPRG0  =    496,   // thread register
+    PROC_STOP_SPR_USPRG1  =    497,   // thread register
+    PROC_STOP_SPR_URMOR   =    505,   // core register
+    PROC_STOP_SPR_SMFCTRL =    511,   // thread register
+    PROC_STOP_SPR_LDBAR   =    850,   // thread register
+    PROC_STOP_SPR_PSSCR   =    855,   // thread register
+    PROC_STOP_SPR_PMCR    =    884,   // core register
+    PROC_STOP_SPR_HID     =   1008,   // core register
+    PROC_STOP_SPR_MSR     =   2000,   // thread register
+
 } CpuReg_t;
 
 /**
@@ -85,6 +105,8 @@ typedef enum
     STOP_SAVE_SCOM_ENTRY_UPDATE_FAILED   = 12,
     STOP_SAVE_INVALID_FUSED_CORE_STATUS  = 13,
     STOP_SAVE_FAIL                       = 14,  // for internal failure within firmware.
+    STOP_SAVE_SPR_ENTRY_MISSING          =  15,
+    STOP_SAVE_SPR_BIT_POS_RESERVE        =  16,
 } StopReturnCode_t;
 
 /**
@@ -101,7 +123,20 @@ typedef enum
     P9_STOP_SCOM_RESET      = 6,
     P9_STOP_SCOM_OR_APPEND  = 7,
     P9_STOP_SCOM_AND_APPEND = 8,
-    P9_STOP_SCOM_OP_MAX     = 9
+    P9_STOP_SCOM_OP_MAX     = 9,
+
+    //enum members which are project agnostic
+    PROC_STOP_SCOM_OP_MIN     =   0,
+    PROC_STOP_SCOM_APPEND     =   1,
+    PROC_STOP_SCOM_REPLACE    =   2,
+    PROC_STOP_SCOM_OR         =   3,
+    PROC_STOP_SCOM_AND        =   4,
+    PROC_STOP_SCOM_NOOP       =   5,
+    PROC_STOP_SCOM_RESET      =   6,
+    PROC_STOP_SCOM_OR_APPEND  =   7,
+    PROC_STOP_SCOM_AND_APPEND =   8,
+    PROC_STOP_SCOM_OP_MAX     =   9,
+
 } ScomOperation_t;
 
 /**
@@ -114,9 +149,49 @@ typedef enum
     P9_STOP_SECTION_EQ_SCOM     = 2,
     P9_STOP_SECTION_L2          = 3,
     P9_STOP_SECTION_L3          = 4,
-    P9_STOP_SECTION_MAX         = 5
+    P9_STOP_SECTION_MAX         = 5,
+
+    //enum members which are project agnostic
+    PROC_STOP_SECTION_MIN         =   0,
+    PROC_STOP_SECTION_CORE_SCOM   =   1,
+    PROC_STOP_SECTION_EQ_SCOM     =   2,
+    PROC_STOP_SECTION_L2          =   3,
+    PROC_STOP_SECTION_L3          =   4,
+    PROC_STOP_SECTION_MAX         =   5,
+
 } ScomSection_t;
 
+
+
+/**
+ * @brief   List of major incompatibilities between API version.
+ * @note    STOP APIs assumes a specific HOMER layout, certain
+ * level of CME-SGPE hcode and certain version of self-save restore
+ * binary. A mismatch can break STOP function.
+ */
+
+/**
+ * @brief  Summarizes bit position allocated to SPRs in save bit mask vector.
+ */
+typedef enum
+{
+    BIT_POS_CIABR       =   0,
+    BIT_POS_DAWR        =   1,
+    BIT_POS_DAWRX       =   2,
+    BIT_POS_HSPRG0      =   3,
+    BIT_POS_LDBAR       =   4,
+    BIT_POS_LPCR        =   5,
+    BIT_POS_PSSCR       =   6,
+    BIT_POS_MSR         =   7,
+    BIT_POS_HID         =   21,
+    BIT_POS_HMEER       =   22,
+    BIT_POS_PMCR        =   23,
+    BIT_POS_PTCR        =   24,
+    BIT_POS_SMFCTRL     =   28,
+    BIT_POS_USPRG0      =   29,
+    BIT_POS_USPRG1      =   30,
+} SprBitPositionList_t;
+
 #ifdef __cplusplus
 extern "C" {
 #endif
diff --git a/libpore/p9_cpu_reg_restore_instruction.H b/libpore/p9_cpu_reg_restore_instruction.H
index cf00ff5e..d69a4212 100644
--- a/libpore/p9_cpu_reg_restore_instruction.H
+++ b/libpore/p9_cpu_reg_restore_instruction.H
@@ -62,6 +62,10 @@ enum
     MTSPR_CONST1        =   467,
     MTMSRD_CONST1       =   178,
     MFSPR_CONST         =   339,
+    BLR_INST            =   0x4e800020,
+    MTSPR_BASE_OPCODE   =   0x7c0003a6,
+    MFSPR_BASE_OPCODE   =   0x7c0002a6,
+    ATTN_OPCODE         =   0x00000200,
     OPCODE_18           =   18,
     SELF_SAVE_FUNC_ADD  =   0x2300,
     SELF_SAVE_OFFSET    =   0x180,
diff --git a/libpore/p9_stop_api.C b/libpore/p9_stop_api.C
index 33aaf788..2d9bb549 100644
--- a/libpore/p9_stop_api.C
+++ b/libpore/p9_stop_api.C
@@ -54,33 +54,33 @@ namespace stopImageSection
 
 const StopSprReg_t g_sprRegister[] =
 {
-    { P9_STOP_SPR_CIABR,     true,  0  },
-    { P9_STOP_SPR_DAWR,      true,  1  },
-    { P9_STOP_SPR_DAWRX,     true,  2  },
-    { P9_STOP_SPR_HSPRG0,    true,  3  },
-    { P9_STOP_SPR_LDBAR,     true,  4, },
-    { P9_STOP_SPR_LPCR,      true,  5  },
-    { P9_STOP_SPR_PSSCR,     true,  6  },
-    { P9_STOP_SPR_MSR,       true,  7  },
-    { P9_STOP_SPR_HRMOR,     false, 20 },
-    { P9_STOP_SPR_HID,       false, 21 },
-    { P9_STOP_SPR_HMEER,     false, 22 },
-    { P9_STOP_SPR_PMCR,      false, 23 },
-    { P9_STOP_SPR_PTCR,      false, 24 },
-    { P9_STOP_SPR_SMFCTRL,   true,  28 },
-    { P9_STOP_SPR_USPRG0,    true,  29 },
-    { P9_STOP_SPR_USPRG1,    true,  30 },
-    { P9_STOP_SPR_URMOR,     false, 31 },
+    { P9_STOP_SPR_CIABR,     true,  0   },
+    { P9_STOP_SPR_DAWR,      true,  1   },
+    { P9_STOP_SPR_DAWRX,     true,  2   },
+    { P9_STOP_SPR_HSPRG0,    true,  3   },
+    { P9_STOP_SPR_LDBAR,     true,  4,  },
+    { P9_STOP_SPR_LPCR,      true,  5   },
+    { P9_STOP_SPR_PSSCR,     true,  6   },
+    { P9_STOP_SPR_MSR,       true,  7   },
+    { P9_STOP_SPR_HRMOR,     false, 255 },
+    { P9_STOP_SPR_HID,       false, 21  },
+    { P9_STOP_SPR_HMEER,     false, 22  },
+    { P9_STOP_SPR_PMCR,      false, 23  },
+    { P9_STOP_SPR_PTCR,      false, 24  },
+    { P9_STOP_SPR_SMFCTRL,   true,  28  },
+    { P9_STOP_SPR_USPRG0,    true,  29  },
+    { P9_STOP_SPR_USPRG1,    true,  30  },
+    { P9_STOP_SPR_URMOR,     false, 255 },
 };
 
-const uint32_t MAX_SPR_SUPPORTED =  17;
+const uint32_t MAX_SPR_SUPPORTED            =   17;
 const uint32_t LEGACY_CORE_SCOM_SUPPORTED   =   15;
 const uint32_t LEGACY_QUAD_SCOM_SUPPORTED   =   63;
 
 //-----------------------------------------------------------------------------
 
 /**
- * @brief       vaildated input arguments passed to p9_stop_save_cpureg_control.
+ * @brief       validated input arguments passed to p9_stop_save_cpureg_control.
  * @param[in]   i_pImage            point to start of HOMER
  * @param[in]   i_coreId            id of the core
  * @param[in]   i_threadId          id of the thread
@@ -255,7 +255,7 @@ STATIC uint32_t getOriInstruction( const uint16_t i_Rs, const uint16_t i_Ra,
  */
 STATIC uint32_t genKeyForSprLookup( const CpuReg_t i_regId )
 {
-    return getOriInstruction( 0, 0, (uint16_t) i_regId );
+    return getOriInstruction( 24, 0, (uint16_t) i_regId );
 }
 
 //-----------------------------------------------------------------------------
@@ -330,7 +330,7 @@ STATIC uint32_t getMtsprInstruction( const uint16_t i_Rs, const uint16_t i_Spr )
  */
 STATIC uint32_t getMfmsrInstruction( const uint16_t i_Rt )
 {
-    uint32_t mfmsrInstOpcode  = ((OPCODE_31 << 26) | (i_Rt << 21) | (MFMSR_CONST));
+    uint32_t mfmsrInstOpcode  = ((OPCODE_31 << 26) | (i_Rt << 21) | ((MFMSR_CONST)<< 1));
 
     return SWIZZLE_4_BYTE(mfmsrInstOpcode);
 }
@@ -361,8 +361,13 @@ STATIC uint32_t getRldicrInstruction( const uint16_t i_Ra, const uint16_t i_Rs,
 
 STATIC uint32_t getMfsprInstruction( const uint16_t i_Rt, const uint16_t i_sprNum )
 {
-    uint32_t mfsprInstOpcode    =   0;
-    mfsprInstOpcode =  (( OPCODE_31 << 26 ) | ( i_Rt << 21 ) | ( i_sprNum << 11 ) | ( MFSPR_CONST << 1 ));
+    uint32_t mfsprInstOpcode = 0;
+    uint32_t temp = (( i_sprNum & 0x03FF ) << 11);
+    mfsprInstOpcode = (uint8_t)i_Rt << 21;
+    mfsprInstOpcode |= (( temp  & 0x0000F800 ) << 5);
+    mfsprInstOpcode |= (( temp  & 0x001F0000 ) >> 5);
+    mfsprInstOpcode |= MFSPR_BASE_OPCODE;
+
     return SWIZZLE_4_BYTE(mfsprInstOpcode);
 }
 
@@ -615,14 +620,14 @@ STATIC StopReturnCode_t getSprRegIndexAdjustment( const uint32_t i_saveMaskPos,
 
     do
     {
-        if( (( i_saveMaskPos >= SPR_BIT_POS_8 ) && ( i_saveMaskPos <= SPR_BIT_POS_19 )) ||
+        if( (( i_saveMaskPos >= SPR_BIT_POS_8 ) && ( i_saveMaskPos <= SPR_BIT_POS_20 )) ||
             (( i_saveMaskPos >= SPR_BIT_POS_25 ) && ( i_saveMaskPos <= SPR_BIT_POS_27 )) )
         {
             l_rc = STOP_SAVE_SPR_BIT_POS_RESERVE;
             break;
         }
 
-        if( (i_saveMaskPos > SPR_BIT_POS_19) && (i_saveMaskPos < SPR_BIT_POS_25 ) )
+        if( (i_saveMaskPos > SPR_BIT_POS_20) && (i_saveMaskPos < SPR_BIT_POS_25) )
         {
             *i_sprAdjIndex    =   12;
         }
@@ -646,138 +651,9 @@ StopReturnCode_t p9_stop_save_cpureg(  void* const i_pImage,
                                        const uint64_t  i_regData,
                                        const uint64_t  i_pir )
 {
-    StopReturnCode_t l_rc = STOP_SAVE_SUCCESS;    // procedure return code
-    HomerSection_t*     chipHomer       =    NULL;
-    SmfHomerSection_t*  smfChipHomer    =    NULL;
-
-    do
-    {
-        uint32_t threadId       =   0;
-        uint32_t coreId         =   0;
-        uint32_t lookUpKey      =   0;
-        void* pSprEntryLocation =   NULL;   // an offset w.r.t. to start of image
-        void* pThreadLocation   =   NULL;
-        bool threadScopeReg     =   false;
-        uint8_t l_urmorFix      =   false;
-        uint64_t  l_sprValue    =   0;
-        uint8_t l_selfRestVer   =   0;
-
-        MY_INF(">> p9_stop_save_cpureg" );
-
-        l_rc = getCoreAndThread( i_pImage, i_pir, &coreId, &threadId );
-
-        if( l_rc )
-        {
-            MY_ERR("Failed to determine Core Id and Thread Id from PIR 0x%016llx",
-                   i_pir);
-            break;
-        }
-
-        MY_INF( " PIR 0x%016llx coreId %d threadid %d "
-                " registerId %d", i_pir, coreId,
-                threadId, i_regId );
-
-        // First of all let us validate all input arguments.
-        l_rc =  validateSprImageInputs( i_pImage,
-                                        i_regId,
-                                        coreId,
-                                        &threadId,
-                                        &threadScopeReg );
-
-        if( l_rc )
-        {
-            // Error: bad argument traces out error code
-            MY_ERR("Bad input argument rc %d", l_rc );
-
-            break;
-        }
-
-        l_urmorFix      =   *(uint8_t*)((uint8_t*)i_pImage + CPMR_HOMER_OFFSET + CPMR_URMOR_FIX_BYTE);
-        l_selfRestVer   =   *(uint8_t *)((uint8_t *)i_pImage + CPMR_HOMER_OFFSET + CPMR_SELF_RESTORE_VER_BYTE );
-
-        if( l_selfRestVer )
-        {
-            smfChipHomer = ( SmfHomerSection_t*)i_pImage;
-
-            if( threadScopeReg )
-            {
-                pThreadLocation =
-                    &(smfChipHomer->iv_coreThreadRestore[coreId].iv_threadRestoreArea[threadId][0]);
-            }
-            else
-            {
-                pThreadLocation =
-                    &(smfChipHomer->iv_coreThreadRestore[coreId].iv_coreRestoreArea[0]);
-            }
-        }
-        else    //Old fips or OPAL release that doesn't support SMF
-        {
-            chipHomer = (HomerSection_t*)i_pImage;
-
-            if( threadScopeReg )
-            {
-                pThreadLocation =
-                    &(chipHomer->iv_coreThreadRestore[coreId][threadId].iv_threadArea[0]);
-            }
-            else
-            {
-                pThreadLocation =
-                    &(chipHomer->iv_coreThreadRestore[coreId][threadId].iv_coreArea[0]);
-            }
-        }
-
-        if( ( SWIZZLE_4_BYTE(BLR_INST) == *(uint32_t*)pThreadLocation ) ||
-            ( SWIZZLE_4_BYTE(ATTN_OPCODE) == *(uint32_t*) pThreadLocation ) )
-        {
-            // table for given core id doesn't exit. It needs to be
-            // defined.
-            pSprEntryLocation = pThreadLocation;
-        }
-        else
-        {
-            // an SPR restore section for given core already exists
-            lookUpKey = genKeyForSprLookup( i_regId );
-            l_rc = lookUpSprInImage( (uint32_t*)pThreadLocation,
-                                     lookUpKey,
-                                     threadScopeReg,
-                                     &pSprEntryLocation,
-                                     l_selfRestVer );
-        }
-
-        if( l_rc )
-        {
-            MY_ERR("Invalid or corrupt SPR entry. CoreId 0x%08x threadId ",
-                   "0x%08x regId 0x%08x lookUpKey 0x%08x pThreadLocation 0x%08x"
-                   , coreId, threadId, i_regId, lookUpKey, pThreadLocation );
-            break;
-        }
-
-        if( ( P9_STOP_SPR_URMOR == i_regId ) && ( l_urmorFix ) )
-        {
-            l_sprValue  =  i_regData - URMOR_CORRECTION;
-        }
-        else
-        {
-            l_sprValue  =  i_regData;
-        }
-
-        l_rc = updateSprEntryInImage( (uint32_t*) pSprEntryLocation,
-                                      i_regId,
-                                      l_sprValue,
-                                      UPDATE_SPR_ENTRY );
-
-        if( l_rc )
-        {
-            MY_ERR( " Failed to update the SPR entry of PIR 0x%08x reg"
-                    "0x%08x", i_pir, i_regId );
-            break;
-        }
-
-    }
-    while(0);
+    MY_INF(">> p9_stop_save_cpureg" );
 
-    MY_INF("<< p9_stop_save_cpureg" );
-    return l_rc;
+    return proc_stop_save_cpureg( i_pImage, i_regId, i_regData, i_pir );
 }
 
 //-----------------------------------------------------------------------------
@@ -1003,103 +879,334 @@ StopReturnCode_t p9_stop_save_scom( void* const   i_pImage,
                                     const ScomOperation_t i_operation,
                                     const ScomSection_t i_section )
 {
-    StopReturnCode_t l_rc = STOP_SAVE_SUCCESS;
-    uint32_t entryLimit =   0;
-    uint8_t chipletId   =   0;
-    uint32_t nopInst    =   0;
-    uint32_t index      =   0;
-    uint32_t imageVer   =   0;
-    uint32_t entrySwzHeader = 0;
-    uint32_t l_maxScomRestoreEntry = 0;
-    ScomEntry_t* pScomEntry      =  NULL;
-    ScomEntry_t* pEntryLocation  =  NULL;
-    ScomEntry_t* pNopLocation    =  NULL;
-    ScomEntry_t* pEditScomHeader =  NULL;
-    StopCacheSection_t* pStopCacheScomStart =   NULL;
-    ScomEntry_t* pTableEndLocationtable     =   NULL;
-    uint32_t swizzleAddr;
-    uint64_t swizzleData;
-    uint32_t swizzleAttn;
-    uint32_t swizzleBlr     =   SWIZZLE_4_BYTE(BLR_INST);
-    bool     cacheEntry     =   true;
-
     MY_INF(">> p9_stop_save_scom");
 
-    //Reads SGPE image version info from QPMR Header in HOMER
-    //For backward compatibility, for base version of SGPE Hcode,
-    //STOP API retains default behavior but adds version specific
-    //details in each entry in later versions.
-    imageVer       =  *(uint32_t*)((uint8_t*)i_pImage + QPMR_HOMER_OFFSET + QPMR_BUILD_VER_BYTE);
-    imageVer       =  SWIZZLE_4_BYTE(imageVer);
-
+    return proc_stop_save_scom( i_pImage, i_scomAddress,
+                                i_scomData, i_operation, i_section );
+}
 
-    do
-    {
-        chipletId   =   i_scomAddress >> 24;
-        chipletId   =   chipletId & 0x3F;
+//-----------------------------------------------------------------------------
 
-        l_rc        =   validateScomImageInputs( i_pImage, i_scomAddress, chipletId, i_operation, i_section );
+/**
+ * @brief   searches a self save entry of an SPR in self-save segment.
+ * @param[in]   i_sprBitPos         bit position associated with SPR in save mask vector.
+ * @param[in]   l_pSprSaveStart     start location of SPR save segment
+ * @param[in]   i_searchLength      length of SPR save segment
+ * @param[in]   i_pSaveSprLoc       start location of save entry for a given SPR.
+ * @return      STOP_SAVE_SUCCESS if look up succeeds, error code otherwise.
+ */
+STATIC StopReturnCode_t lookUpSelfSaveSpr( uint32_t i_sprBitPos, uint32_t* l_pSprSaveStart,
+                                    uint32_t  i_searchLength, uint32_t** i_pSaveSprLoc )
+{
+    int32_t l_saveWordLength    =   (int32_t)(i_searchLength >> 2);
+    uint32_t l_oriInst          =   getOriInstruction( 0, 0, i_sprBitPos );
+    StopReturnCode_t l_rc       =   STOP_SAVE_FAIL;
 
-        if( l_rc )
+    while( l_saveWordLength > 0 )
+    {
+        if( l_oriInst == *l_pSprSaveStart )
         {
-            MY_ERR( "invalid argument: aborting");
+            *i_pSaveSprLoc   =   l_pSprSaveStart;
+            l_rc             =   STOP_SAVE_SUCCESS;
             break;
         }
 
-        if( chipletId >= CORE_CHIPLET_ID_MIN )
-        {
-            // chiplet is core. So, let us find the start address of SCOM area
-            // pertaining to a core in STOP image.
-            l_maxScomRestoreEntry   =
-                *(uint32_t*)((uint8_t*)i_pImage + CPMR_HOMER_OFFSET + CPMR_MAX_SCOM_REST_PER_CORE_BYTE);
-            pScomEntry              =   CORE_ID_SCOM_START(i_pImage, chipletId )
-            cacheEntry              =   false;
+        l_pSprSaveStart++;
+        l_saveWordLength--;
+    }
 
-            if( !l_maxScomRestoreEntry )
-            {
-                //Old HB and new STOP API case. Retain legacy Number
-                l_maxScomRestoreEntry   =  SWIZZLE_4_BYTE(LEGACY_CORE_SCOM_SUPPORTED);
-            }
-        }
-        else
-        {
-            l_maxScomRestoreEntry   =
-                *(uint32_t*)((uint8_t*)i_pImage + QPMR_HOMER_OFFSET + QPMR_QUAD_MAX_SCOM_ENTRY_BYTE);
+    return l_rc;
+}
 
-            if( !l_maxScomRestoreEntry )
-            {
-                // Incase of a bad HOMER header initialization, fall back on legacy number.
-                l_maxScomRestoreEntry   =  SWIZZLE_4_BYTE(LEGACY_QUAD_SCOM_SUPPORTED);
-            }
-            // chiplet is a cache. let us find start address of cache section
-            // associated with given chiplet. A cache section associated with
-            // given chiplet is split in to L2, L3 and EQ area.
-            pStopCacheScomStart = CACHE_SECTN_START(i_pImage,
-                                                    chipletId);
-        }
+//-----------------------------------------------------------------------------
 
-        l_maxScomRestoreEntry   =   SWIZZLE_4_BYTE(l_maxScomRestoreEntry);
+/**
+ * @brief   searches a self save entry of an SPR in self-save segment.
+ * @param[in]   i_pSaveReg  start of editable location of a SPR save entry.
+ * @param[in]   i_sprNum    Id of the SPR for which entry needs to be edited.
+ * @return      STOP_SAVE_SUCCESS if look up succeeds, error code otherwise.
+ */
+STATIC StopReturnCode_t updateSelfSaveEntry( uint32_t* i_pSaveReg, uint16_t i_sprNum )
+{
+    StopReturnCode_t l_rc   =   STOP_SAVE_SUCCESS;
 
-        if(( !pStopCacheScomStart ) && ( !pScomEntry) )
+    do
+    {
+        if( !i_pSaveReg )
         {
-            //Error invalid pointer to SCOM entry in cache or core section
-            //of STOP image.
-            MY_ERR("invalid start location for chiplet %d",
-                   chipletId );
+            l_rc    =   STOP_SAVE_FAIL;
+            MY_ERR( "Failed to update self save area for SPR 0x%04x", i_sprNum );
             break;
         }
 
-        switch( i_section )
+        if( P9_STOP_SPR_MSR == i_sprNum )
         {
-            case P9_STOP_SECTION_EQ_SCOM:
-                pScomEntry = pStopCacheScomStart->nonCacheArea;
-                entryLimit = MAX_EQ_SCOM_ENTRIES;
-                break;
+            *i_pSaveReg     =    getMfmsrInstruction( 1 );
+        }
+        else
+        {
+            *i_pSaveReg     =   getMfsprInstruction( 1, i_sprNum );
+        }
 
-            case P9_STOP_SECTION_L2:
-                pScomEntry = pStopCacheScomStart->l2CacheArea;
-                entryLimit = MAX_L2_SCOM_ENTRIES;
-                break;
+        i_pSaveReg++;
+
+        *i_pSaveReg         =   getBranchLinkRegInstruction( );
+    }
+    while(0);
+
+    return l_rc;
+}
+
+//-----------------------------------------------------------------------------
+
+StopReturnCode_t p9_stop_save_cpureg_control(  void* i_pImage,
+        const uint64_t i_pir,
+        const uint32_t i_saveRegVector )
+{
+    MY_INF( ">> p9_stop_save_cpureg_control" );
+
+    return proc_stop_save_cpureg_control( i_pImage, i_pir, i_saveRegVector );
+}
+
+//-----------------------------------------------------------------------------------------------------
+
+StopReturnCode_t p9_stop_init_cpureg(  void* const i_pImage, const uint32_t i_corePos )
+{
+    MY_INF( ">> p9_stop_init_cpureg" );
+
+    return proc_stop_init_cpureg( i_pImage, i_corePos );
+}
+
+//-----------------------------------------------------------------------------------------------------
+
+StopReturnCode_t p9_stop_init_self_save(  void* const i_pImage, const uint32_t i_corePos )
+{
+    MY_INF( ">> p9_stop_init_self_save" );
+
+    return proc_stop_init_self_save( i_pImage, i_corePos );
+}
+
+//-----------------------------------------------------------------------------------------------------
+
+StopReturnCode_t proc_stop_init_cpureg(  void* const i_pImage, const uint32_t i_corePos )
+{
+
+    StopReturnCode_t    l_rc        =   STOP_SAVE_SUCCESS;
+    uint32_t* l_pRestoreStart       =   NULL;
+    void* l_pTempLoc                =   NULL;
+    SmfHomerSection_t* l_pHomer     =   NULL;
+    uint32_t l_threadPos            =   0;
+    uint32_t l_lookUpKey            =   0;
+    uint32_t l_sprIndex             =   0;
+    uint8_t l_selfRestVer           =   0;
+
+    MY_INF( ">> proc_stop_init_cpureg" );
+
+    do
+    {
+        if( !i_pImage )
+        {
+            l_rc    =   STOP_SAVE_ARG_INVALID_IMG;
+            break;
+        }
+
+        if( i_corePos > MAX_CORE_ID_SUPPORTED )
+        {
+            l_rc    =  STOP_SAVE_ARG_INVALID_CORE;
+            break;
+        }
+
+        l_pHomer        =   ( SmfHomerSection_t * ) i_pImage;
+        l_selfRestVer   =   *(uint8_t *)((uint8_t *)i_pImage + CPMR_HOMER_OFFSET + CPMR_SELF_RESTORE_VER_BYTE );
+
+        for( l_sprIndex = 0; l_sprIndex < MAX_SPR_SUPPORTED; l_sprIndex++ )
+        {
+            //Check if a given SPR needs to be self-saved each time on STOP entry
+
+            l_lookUpKey     =   genKeyForSprLookup( ( CpuReg_t )g_sprRegister[l_sprIndex].iv_sprId );
+
+            if( g_sprRegister[l_sprIndex].iv_isThreadScope )
+            {
+                for( l_threadPos = 0; l_threadPos < MAX_THREADS_PER_CORE; l_threadPos++ )
+                {
+                    l_pRestoreStart =
+                        (uint32_t*)&l_pHomer->iv_coreThreadRestore[i_corePos].iv_threadRestoreArea[l_threadPos][0];
+
+                    l_rc    =   lookUpSprInImage( (uint32_t*)l_pRestoreStart, l_lookUpKey,
+                                                  g_sprRegister[l_sprIndex].iv_isThreadScope,
+                                                  &l_pTempLoc,
+                                                  l_selfRestVer );
+
+                    if( l_rc )
+                    {
+                        MY_ERR( "Thread SPR lookup failed in p9_stop_init_cpureg SPR %d Core %d Thread %d Index %d",
+                                g_sprRegister[l_sprIndex].iv_sprId, i_corePos, l_threadPos, l_sprIndex );
+                        break;
+                    }
+
+                    l_rc = updateSprEntryInImage( (uint32_t*) l_pTempLoc,
+                                                  ( CpuReg_t )g_sprRegister[l_sprIndex].iv_sprId,
+                                                  0x00,
+                                                  INIT_SPR_REGION );
+
+                    if( l_rc )
+                    {
+                        MY_ERR( "Thread SPR region init failed. Core %d SPR Id %d",
+                                i_corePos, g_sprRegister[l_sprIndex].iv_sprId );
+                        break;
+                    }
+
+                }//end for thread
+
+                if( l_rc )
+                {
+                    break;
+                }
+
+            }//end if SPR threadscope
+            else
+            {
+                l_pRestoreStart     =   (uint32_t*)&l_pHomer->iv_coreThreadRestore[i_corePos].iv_coreRestoreArea[0];
+
+                l_rc                =   lookUpSprInImage( (uint32_t*)l_pRestoreStart, l_lookUpKey,
+                                        g_sprRegister[l_sprIndex].iv_isThreadScope,
+                                        &l_pTempLoc, l_selfRestVer );
+
+                if( l_rc )
+                {
+                    MY_ERR( "Core SPR lookup failed in p9_stop_init_cpureg" );
+                    break;
+                }
+
+                l_rc    =   updateSprEntryInImage( (uint32_t*) l_pTempLoc,
+                                                   ( CpuReg_t )g_sprRegister[l_sprIndex].iv_sprId,
+                                                   0x00,
+                                                   INIT_SPR_REGION );
+
+                if( l_rc )
+                {
+                    MY_ERR( "Core SPR region init failed. Core %d SPR Id %d SPR Index %d",
+                            i_corePos, g_sprRegister[l_sprIndex].iv_sprId, l_sprIndex );
+                    break;
+                }
+
+            }// end else
+
+        }// end for l_sprIndex
+
+    }
+    while(0);
+
+    MY_INF( "<< proc_stop_init_cpureg" );
+
+    return l_rc;
+}
+
+//-----------------------------------------------------------------------------------------------------
+
+StopReturnCode_t proc_stop_save_scom( void* const   i_pImage,
+                                      const uint32_t i_scomAddress,
+                                      const uint64_t i_scomData,
+                                      const ScomOperation_t i_operation,
+                                      const ScomSection_t i_section )
+{
+    StopReturnCode_t l_rc = STOP_SAVE_SUCCESS;
+    uint32_t entryLimit =   0;
+    uint8_t chipletId   =   0;
+    uint32_t nopInst    =   0;
+    uint32_t index      =   0;
+    uint32_t imageVer   =   0;
+    uint32_t entrySwzHeader = 0;
+    uint32_t l_maxScomRestoreEntry = 0;
+    ScomEntry_t* pScomEntry      =  NULL;
+    ScomEntry_t* pEntryLocation  =  NULL;
+    ScomEntry_t* pNopLocation    =  NULL;
+    ScomEntry_t* pEditScomHeader =  NULL;
+    StopCacheSection_t* pStopCacheScomStart =   NULL;
+    ScomEntry_t* pTableEndLocationtable     =   NULL;
+    uint32_t swizzleAddr;
+    uint64_t swizzleData;
+    uint32_t swizzleAttn;
+    uint32_t swizzleBlr     =   SWIZZLE_4_BYTE(BLR_INST);
+    bool     cacheEntry     =   true;
+
+    MY_INF( ">> proc_stop_save_scom" );
+
+    //Reads SGPE image version info from QPMR Header in HOMER
+    //For backward compatibility, for base version of SGPE Hcode,
+    //STOP API retains default behavior but adds version specific
+    //details in each entry in later versions.
+    imageVer       =  *(uint32_t*)((uint8_t*)i_pImage + QPMR_HOMER_OFFSET + QPMR_BUILD_VER_BYTE);
+    imageVer       =  SWIZZLE_4_BYTE(imageVer);
+
+
+    do
+    {
+        chipletId   =   i_scomAddress >> 24;
+        chipletId   =   chipletId & 0x3F;
+
+        l_rc        =   validateScomImageInputs( i_pImage, i_scomAddress, chipletId, i_operation, i_section );
+
+        if( l_rc )
+        {
+            MY_ERR( "invalid argument: aborting");
+            break;
+        }
+
+        if( chipletId >= CORE_CHIPLET_ID_MIN )
+        {
+            // chiplet is core. So, let us find the start address of SCOM area
+            // pertaining to a core in STOP image.
+            l_maxScomRestoreEntry   =
+                *(uint32_t*)((uint8_t*)i_pImage + CPMR_HOMER_OFFSET + CPMR_MAX_SCOM_REST_PER_CORE_BYTE);
+            pScomEntry              =   CORE_ID_SCOM_START(i_pImage, chipletId )
+            cacheEntry              =   false;
+
+            if( !l_maxScomRestoreEntry )
+            {
+                //Old HB and new STOP API case. Retain legacy Number
+                l_maxScomRestoreEntry   =  SWIZZLE_4_BYTE(LEGACY_CORE_SCOM_SUPPORTED);
+            }
+        }
+        else
+        {
+            l_maxScomRestoreEntry   =
+                *(uint32_t*)((uint8_t*)i_pImage + QPMR_HOMER_OFFSET + QPMR_QUAD_MAX_SCOM_ENTRY_BYTE);
+
+            if( !l_maxScomRestoreEntry )
+            {
+                // Incase of a bad HOMER header initialization, fall back on legacy number.
+                l_maxScomRestoreEntry   =  SWIZZLE_4_BYTE(LEGACY_QUAD_SCOM_SUPPORTED);
+            }
+            // chiplet is a cache. let us find start address of cache section
+            // associated with given chiplet. A cache section associated with
+            // given chiplet is split in to L2, L3 and EQ area.
+            pStopCacheScomStart = CACHE_SECTN_START(i_pImage,
+                                                    chipletId);
+        }
+
+        l_maxScomRestoreEntry   =   SWIZZLE_4_BYTE(l_maxScomRestoreEntry);
+
+        if(( !pStopCacheScomStart ) && ( !pScomEntry) )
+        {
+            //Error invalid pointer to SCOM entry in cache or core section
+            //of STOP image.
+            MY_ERR("invalid start location for chiplet %d",
+                   chipletId );
+            break;
+        }
+
+        switch( i_section )
+        {
+            case P9_STOP_SECTION_EQ_SCOM:
+                pScomEntry = pStopCacheScomStart->nonCacheArea;
+                entryLimit = MAX_EQ_SCOM_ENTRIES;
+                break;
+
+            case P9_STOP_SECTION_L2:
+                pScomEntry = pStopCacheScomStart->l2CacheArea;
+                entryLimit = MAX_L2_SCOM_ENTRIES;
+                break;
 
             case P9_STOP_SECTION_L3:
                 pScomEntry = pStopCacheScomStart->l3CacheArea;
@@ -1274,131 +1381,60 @@ StopReturnCode_t p9_stop_save_scom( void* const   i_pImage,
                     if( NULL == pEntryLocation )
                     {
                         editAppend = pTableEndLocationtable;
-                    }
-                    else
-                    {
-                        editAppend = pEntryLocation;
-
-                        if( P9_STOP_SCOM_OR_APPEND == i_operation )
-                        {
-                            tempOperation = P9_STOP_SCOM_OR;
-                        }
-                        else
-                        {
-                            tempOperation = P9_STOP_SCOM_AND;
-                        }
-                    }
-
-                    l_rc = editScomEntry( swizzleAddr,
-                                          swizzleData,
-                                          editAppend,
-                                          tempOperation );
-
-                    pEditScomHeader = editAppend;
-                }
-                break;
-
-            default:
-                l_rc = STOP_SAVE_SCOM_INVALID_OPERATION;
-                break;
-        }
-    }
-    while(0);
-
-    if( l_rc )
-    {
-        MY_ERR("SCOM image operation 0x%08x failed for chiplet 0x%08x addr"
-               "0x%08x", i_operation, chipletId ,
-               i_scomAddress );
-    }
-    else
-    {
-        //Update SCOM Restore entry with version and memory layout
-        //info
-        updateEntryHeader( pEditScomHeader, imageVer, l_maxScomRestoreEntry );
-    }
-
-    MY_INF("<< p9_stop_save_scom");
-    return l_rc;
-}
-
-//-----------------------------------------------------------------------------
-
-/**
- * @brief   searches a self save entry of an SPR in self-save segment.
- * @param[in]   i_sprBitPos         bit position associated with SPR in save mask vector.
- * @param[in]   l_pSprSaveStart     start location of SPR save segment
- * @param[in]   i_searchLength      length of SPR save segment
- * @param[in]   i_pSaveSprLoc       start location of save entry for a given SPR.
- * @return      STOP_SAVE_SUCCESS if look up succeeds, error code otherwise.
- */
-STATIC StopReturnCode_t lookUpSelfSaveSpr( uint32_t i_sprBitPos, uint32_t* l_pSprSaveStart,
-                                    uint32_t  i_searchLength, uint32_t** i_pSaveSprLoc )
-{
-    int32_t l_saveWordLength    =   (int32_t)(i_searchLength >> 2);
-    uint32_t l_oriInst          =   getOriInstruction( 0, 0, i_sprBitPos );
-    StopReturnCode_t l_rc       =   STOP_SAVE_FAIL;
-
-    while( l_saveWordLength > 0 )
-    {
-        if( l_oriInst == *l_pSprSaveStart )
-        {
-            *i_pSaveSprLoc   =   l_pSprSaveStart;
-            l_rc             =   STOP_SAVE_SUCCESS;
-            break;
-        }
-
-        l_pSprSaveStart++;
-        l_saveWordLength--;
-    }
-
-    return l_rc;
-}
-
-//-----------------------------------------------------------------------------
-
-/**
- * @brief   searches a self save entry of an SPR in self-save segment.
- * @param[in]   i_pSaveReg  start of editable location of a SPR save entry.
- * @param[in]   i_sprNum    Id of the SPR for which entry needs to be edited.
- * @return      STOP_SAVE_SUCCESS if look up succeeds, error code otherwise.
- */
-STATIC StopReturnCode_t updateSelfSaveEntry( uint32_t* i_pSaveReg, uint16_t i_sprNum )
-{
-    StopReturnCode_t l_rc   =   STOP_SAVE_SUCCESS;
-
-    do
-    {
-        if( !i_pSaveReg )
-        {
-            l_rc    =   STOP_SAVE_FAIL;
-            MY_ERR( "Failed to update self save area for SPR 0x%04x", i_sprNum );
-            break;
-        }
+                    }
+                    else
+                    {
+                        editAppend = pEntryLocation;
 
-        if( P9_STOP_SPR_MSR == i_sprNum )
-        {
-            *i_pSaveReg     =    getMfmsrInstruction( 1 );
-        }
-        else
-        {
-            *i_pSaveReg     =   getMfsprInstruction( 1, i_sprNum );
-        }
+                        if( P9_STOP_SCOM_OR_APPEND == i_operation )
+                        {
+                            tempOperation = P9_STOP_SCOM_OR;
+                        }
+                        else
+                        {
+                            tempOperation = P9_STOP_SCOM_AND;
+                        }
+                    }
 
-        i_pSaveReg++;
+                    l_rc = editScomEntry( swizzleAddr,
+                                          swizzleData,
+                                          editAppend,
+                                          tempOperation );
 
-        *i_pSaveReg         =   getBranchLinkRegInstruction( );
+                    pEditScomHeader = editAppend;
+                }
+                break;
+
+            default:
+                l_rc = STOP_SAVE_SCOM_INVALID_OPERATION;
+                break;
+        }
     }
     while(0);
 
+    if( l_rc )
+    {
+        MY_ERR("SCOM image operation 0x%08x failed for chiplet 0x%08x addr"
+               "0x%08x", i_operation, chipletId ,
+               i_scomAddress );
+    }
+    else
+    {
+        //Update SCOM Restore entry with version and memory layout
+        //info
+        updateEntryHeader( pEditScomHeader, imageVer, l_maxScomRestoreEntry );
+    }
+
+    MY_INF( "<< proc_stop_save_scom" );
+
     return l_rc;
 }
 
-//-----------------------------------------------------------------------------
+//-----------------------------------------------------------------------------------------------------
 
-StopReturnCode_t p9_stop_save_cpureg_control(  void* i_pImage,
-        const uint64_t i_pir,
-        const uint32_t i_saveRegVector )
+StopReturnCode_t proc_stop_save_cpureg_control(  void* i_pImage,
+                                                 const uint64_t i_pir,
+                                                 const uint32_t i_saveRegVector )
 {
     StopReturnCode_t l_rc   =   STOP_SAVE_SUCCESS;
     uint32_t l_coreId       =   0;
@@ -1411,8 +1447,10 @@ StopReturnCode_t p9_stop_save_cpureg_control(  void* i_pImage,
     uint32_t* l_pRestoreStart       =   NULL;
     uint32_t* l_pSprSave            =   NULL;
     void* l_pTempLoc                =   NULL;
+    uint32_t * l_pTempWord          =   NULL;
     SmfHomerSection_t* l_pHomer     =   NULL;
     uint8_t l_selfRestVer           =   0;
+    MY_INF(">> proc_stop_save_cpureg_control" );
 
     do
     {
@@ -1440,6 +1478,11 @@ StopReturnCode_t p9_stop_save_cpureg_control(  void* i_pImage,
         {
             l_sprPos    =    g_sprRegister[l_sprIndex].iv_saveMaskPos;
 
+            if( l_sprPos > MAX_SPR_BIT_POS )
+            {
+                continue;
+            }
+
             //Check if a given SPR needs to be self-saved each time on STOP entry
 
             if( i_saveRegVector & ( TEST_BIT_PATTERN >> l_sprPos ) )
@@ -1493,139 +1536,187 @@ StopReturnCode_t p9_stop_save_cpureg_control(  void* i_pImage,
                 //update specific instructions of self save region to enable saving for SPR
                 l_rc    =   updateSelfSaveEntry( l_pSprSave, g_sprRegister[l_sprIndex].iv_sprId );
 
+                if( l_rc )
+                {
+                    MY_ERR( "Failed to update self save instructions for 0x%08x",
+                            (uint32_t) g_sprRegister[l_sprIndex].iv_sprId );
+                }
+
+                if( l_pTempLoc )
+                {
+                    l_pTempWord      =   (uint32_t *)l_pTempLoc;
+                    l_pTempWord++;
+                    *l_pTempWord     =   getXorInstruction( 0, 0, 0 );
+                }
+
             }// end if( i_saveRegVector..)
         }// end for
     }
     while(0);
 
+    MY_INF("<< proc_stop_save_cpureg_control" );
+
     return l_rc;
+
 }
 
 //-----------------------------------------------------------------------------------------------------
 
-StopReturnCode_t p9_stop_init_cpureg(  void* const i_pImage, const uint32_t i_corePos )
+StopReturnCode_t proc_stop_save_cpureg(  void* const i_pImage,
+                                       const CpuReg_t  i_regId,
+                                       const uint64_t  i_regData,
+                                       const uint64_t  i_pir )
 {
-    StopReturnCode_t    l_rc        =   STOP_SAVE_SUCCESS;
-    uint32_t* l_pRestoreStart       =   NULL;
-    void* l_pTempLoc                =   NULL;
-    SmfHomerSection_t* l_pHomer     =   NULL;
-    uint32_t l_threadPos            =   0;
-    uint32_t l_lookUpKey            =   0;
-    uint32_t l_sprIndex             =   0;
-    uint8_t l_selfRestVer           =   0;
 
-    MY_INF( ">> p9_stop_init_cpureg" );
+    StopReturnCode_t l_rc = STOP_SAVE_SUCCESS;    // procedure return code
+    HomerSection_t*     chipHomer       =    NULL;
+    SmfHomerSection_t*  smfChipHomer    =    NULL;
+
+    MY_INF(">> proc_stop_save_cpureg" );
 
     do
     {
-        if( !i_pImage )
+        uint32_t threadId       =   0;
+        uint32_t coreId         =   0;
+        uint32_t lookUpKey      =   0;
+        void* pSprEntryLocation =   NULL;   // an offset w.r.t. to start of image
+        void* pThreadLocation   =   NULL;
+        bool threadScopeReg     =   false;
+        uint8_t l_urmorFix      =   false;
+        uint64_t  l_sprValue    =   0;
+        uint8_t l_selfRestVer   =   0;
+
+
+        l_rc = getCoreAndThread( i_pImage, i_pir, &coreId, &threadId );
+
+        if( l_rc )
         {
-            l_rc    =   STOP_SAVE_ARG_INVALID_IMG;
+            MY_ERR("Failed to determine Core Id and Thread Id from PIR 0x%016llx",
+                   i_pir);
             break;
         }
 
-        if( i_corePos > MAX_CORE_ID_SUPPORTED )
+        MY_INF( " PIR 0x%016llx coreId %d threadid %d "
+                " registerId %d", i_pir, coreId,
+                threadId, i_regId );
+
+        // First of all let us validate all input arguments.
+        l_rc =  validateSprImageInputs( i_pImage,
+                                        i_regId,
+                                        coreId,
+                                        &threadId,
+                                        &threadScopeReg );
+
+        if( l_rc )
         {
-            l_rc    =  STOP_SAVE_ARG_INVALID_CORE;
+            // Error: bad argument traces out error code
+            MY_ERR("Bad input argument rc %d", l_rc );
+
             break;
         }
 
-        l_pHomer        =   ( SmfHomerSection_t * ) i_pImage;
+        l_urmorFix      =   *(uint8_t*)((uint8_t*)i_pImage + CPMR_HOMER_OFFSET + CPMR_URMOR_FIX_BYTE);
         l_selfRestVer   =   *(uint8_t *)((uint8_t *)i_pImage + CPMR_HOMER_OFFSET + CPMR_SELF_RESTORE_VER_BYTE );
 
-        for( l_sprIndex = 0; l_sprIndex < MAX_SPR_SUPPORTED; l_sprIndex++ )
+        if( l_selfRestVer )
         {
-            //Check if a given SPR needs to be self-saved each time on STOP entry
-
-            l_lookUpKey     =   genKeyForSprLookup( ( CpuReg_t )g_sprRegister[l_sprIndex].iv_sprId );
+            smfChipHomer = ( SmfHomerSection_t*)i_pImage;
 
-            if( g_sprRegister[l_sprIndex].iv_isThreadScope )
+            if( threadScopeReg )
             {
-                for( l_threadPos = 0; l_threadPos < MAX_THREADS_PER_CORE; l_threadPos++ )
-                {
-                    l_pRestoreStart =
-                        (uint32_t*)&l_pHomer->iv_coreThreadRestore[i_corePos].iv_threadRestoreArea[l_threadPos][0];
-
-                    l_rc    =   lookUpSprInImage( (uint32_t*)l_pRestoreStart, l_lookUpKey,
-                                                  g_sprRegister[l_sprIndex].iv_isThreadScope,
-                                                  &l_pTempLoc,
-                                                  l_selfRestVer );
-
-                    if( l_rc )
-                    {
-                        MY_ERR( "Thread SPR lookup failed in p9_stop_init_cpureg SPR %d Core %d Thread %d Index %d",
-                                g_sprRegister[l_sprIndex].iv_sprId, i_corePos, l_threadPos, l_sprIndex );
-                        break;
-                    }
-
-                    l_rc = updateSprEntryInImage( (uint32_t*) l_pTempLoc,
-                                                  ( CpuReg_t )g_sprRegister[l_sprIndex].iv_sprId,
-                                                  0x00,
-                                                  INIT_SPR_REGION );
-
-                    if( l_rc )
-                    {
-                        MY_ERR( "Thread SPR region init failed. Core %d SPR Id %d",
-                                i_corePos, g_sprRegister[l_sprIndex].iv_sprId );
-                        break;
-                    }
-
-                }//end for thread
-
-                if( l_rc )
-                {
-                    break;
-                }
-
-            }//end if SPR threadscope
+                pThreadLocation =
+                    &(smfChipHomer->iv_coreThreadRestore[coreId].iv_threadRestoreArea[threadId][0]);
+            }
             else
             {
-                l_pRestoreStart     =   (uint32_t*)&l_pHomer->iv_coreThreadRestore[i_corePos].iv_coreRestoreArea[0];
+                pThreadLocation =
+                    &(smfChipHomer->iv_coreThreadRestore[coreId].iv_coreRestoreArea[0]);
+            }
+        }
+        else    //Old fips or OPAL release that doesn't support SMF
+        {
+            chipHomer = (HomerSection_t*)i_pImage;
 
-                l_rc                =   lookUpSprInImage( (uint32_t*)l_pRestoreStart, l_lookUpKey,
-                                        g_sprRegister[l_sprIndex].iv_isThreadScope,
-                                        &l_pTempLoc, l_selfRestVer );
+            if( threadScopeReg )
+            {
+                pThreadLocation =
+                    &(chipHomer->iv_coreThreadRestore[coreId][threadId].iv_threadArea[0]);
+            }
+            else
+            {
+                pThreadLocation =
+                    &(chipHomer->iv_coreThreadRestore[coreId][threadId].iv_coreArea[0]);
+            }
+        }
 
-                if( l_rc )
-                {
-                    MY_ERR( "Core SPR lookup failed in p9_stop_init_cpureg" );
-                    break;
-                }
+        if( ( SWIZZLE_4_BYTE(BLR_INST) == *(uint32_t*)pThreadLocation ) ||
+            ( SWIZZLE_4_BYTE(ATTN_OPCODE) == *(uint32_t*) pThreadLocation ) )
+        {
+            // table for given core id doesn't exit. It needs to be
+            // defined.
+            pSprEntryLocation = pThreadLocation;
+        }
+        else
+        {
+            // an SPR restore section for given core already exists
+            lookUpKey = genKeyForSprLookup( i_regId );
+            l_rc = lookUpSprInImage( (uint32_t*)pThreadLocation,
+                                     lookUpKey,
+                                     threadScopeReg,
+                                     &pSprEntryLocation,
+                                     l_selfRestVer );
+        }
 
-                l_rc    =   updateSprEntryInImage( (uint32_t*) l_pTempLoc,
-                                                   ( CpuReg_t )g_sprRegister[l_sprIndex].iv_sprId,
-                                                   0x00,
-                                                   INIT_SPR_REGION );
+        if( l_rc )
+        {
+            MY_ERR("Invalid or corrupt SPR entry. CoreId 0x%08x threadId ",
+                   "0x%08x regId 0x%08x lookUpKey 0x%08x pThreadLocation 0x%08x"
+                   , coreId, threadId, i_regId, lookUpKey, pThreadLocation );
+            break;
+        }
 
-                if( l_rc )
-                {
-                    MY_ERR( "Core SPR region init failed. Core %d SPR Id %d SPR Index %d",
-                            i_corePos, g_sprRegister[l_sprIndex].iv_sprId, l_sprIndex );
-                    break;
-                }
+        if( ( P9_STOP_SPR_URMOR == i_regId ) && ( l_urmorFix ) )
+        {
+            l_sprValue  =  i_regData - URMOR_CORRECTION;
+        }
+        else
+        {
+            l_sprValue  =  i_regData;
+        }
 
-            }// end else
+        l_rc = updateSprEntryInImage( (uint32_t*) pSprEntryLocation,
+                                      i_regId,
+                                      l_sprValue,
+                                      UPDATE_SPR_ENTRY );
 
-        }// end for l_sprIndex
+        if( l_rc )
+        {
+            MY_ERR( " Failed to update the SPR entry of PIR 0x%08x reg"
+                    "0x%08x", i_pir, i_regId );
+            break;
+        }
 
     }
     while(0);
 
-    MY_INF( "<< p9_stop_init_cpureg" );
+    MY_INF("<< proc_stop_save_cpureg" );
+
     return l_rc;
 }
 
 //-----------------------------------------------------------------------------------------------------
 
-StopReturnCode_t p9_stop_init_self_save(  void* const i_pImage, const uint32_t i_corePos )
+StopReturnCode_t proc_stop_init_self_save(  void* const i_pImage, const uint32_t i_corePos )
 {
+
     StopReturnCode_t    l_rc        =   STOP_SAVE_SUCCESS;
     uint32_t* l_pSaveStart          =   NULL;
     SmfHomerSection_t *  l_pHomer   =   NULL;
     uint32_t l_threadPos            =   0;
     uint32_t l_sprBitPos            =   0;
     uint32_t l_sprIndexAdj          =   0;
-    MY_INF( ">> p9_stop_init_self_save" );
+
+    MY_INF(">> proc_stop_init_self_save" );
 
     do
     {
@@ -1732,7 +1823,8 @@ StopReturnCode_t p9_stop_init_self_save(  void* const i_pImage, const uint32_t i
     }
     while(0);
 
-    MY_INF( "<< p9_stop_init_self_save" );
+    MY_INF("<< proc_stop_init_self_save" );
+
     return l_rc;
 }
 
diff --git a/libpore/p9_stop_api.H b/libpore/p9_stop_api.H
index 17caedb3..3f6420ff 100644
--- a/libpore/p9_stop_api.H
+++ b/libpore/p9_stop_api.H
@@ -70,6 +70,26 @@ typedef enum
     P9_STOP_SPR_PMCR    =    884,   // core register
     P9_STOP_SPR_HID     =   1008,   // core register
     P9_STOP_SPR_MSR     =   2000,   // thread register
+
+    //enum members which are project agnostic
+    PROC_STOP_SPR_DAWR    =    180,   // thread register
+    PROC_STOP_SPR_CIABR   =    187,   // thread register
+    PROC_STOP_SPR_DAWRX   =    188,   // thread register
+    PROC_STOP_SPR_HSPRG0  =    304,   // thread register
+    PROC_STOP_SPR_HRMOR   =    313,   // core register
+    PROC_STOP_SPR_LPCR    =    318,   // thread register
+    PROC_STOP_SPR_HMEER   =    337,   // core register
+    PROC_STOP_SPR_PTCR    =    464,   // core register
+    PROC_STOP_SPR_USPRG0  =    496,   // thread register
+    PROC_STOP_SPR_USPRG1  =    497,   // thread register
+    PROC_STOP_SPR_URMOR   =    505,   // core register
+    PROC_STOP_SPR_SMFCTRL =    511,   // thread register
+    PROC_STOP_SPR_LDBAR   =    850,   // thread register
+    PROC_STOP_SPR_PSSCR   =    855,   // thread register
+    PROC_STOP_SPR_PMCR    =    884,   // core register
+    PROC_STOP_SPR_HID     =   1008,   // core register
+    PROC_STOP_SPR_MSR     =   2000,   // thread register
+
 } CpuReg_t;
 
 /**
@@ -110,7 +130,20 @@ typedef enum
     P9_STOP_SCOM_RESET      =   6,
     P9_STOP_SCOM_OR_APPEND  =   7,
     P9_STOP_SCOM_AND_APPEND =   8,
-    P9_STOP_SCOM_OP_MAX     =   9
+    P9_STOP_SCOM_OP_MAX     =   9,
+
+    //enum members which are project agnostic
+    PROC_STOP_SCOM_OP_MIN     =   0,
+    PROC_STOP_SCOM_APPEND     =   1,
+    PROC_STOP_SCOM_REPLACE    =   2,
+    PROC_STOP_SCOM_OR         =   3,
+    PROC_STOP_SCOM_AND        =   4,
+    PROC_STOP_SCOM_NOOP       =   5,
+    PROC_STOP_SCOM_RESET      =   6,
+    PROC_STOP_SCOM_OR_APPEND  =   7,
+    PROC_STOP_SCOM_AND_APPEND =   8,
+    PROC_STOP_SCOM_OP_MAX     =   9,
+
 } ScomOperation_t;
 
 /**
@@ -123,7 +156,15 @@ typedef enum
     P9_STOP_SECTION_EQ_SCOM     =   2,
     P9_STOP_SECTION_L2          =   3,
     P9_STOP_SECTION_L3          =   4,
-    P9_STOP_SECTION_MAX         =   5
+    P9_STOP_SECTION_MAX         =   5,
+
+    //enum members which are project agnostic
+    PROC_STOP_SECTION_MIN         =   0,
+    PROC_STOP_SECTION_CORE_SCOM   =   1,
+    PROC_STOP_SECTION_EQ_SCOM     =   2,
+    PROC_STOP_SECTION_L2          =   3,
+    PROC_STOP_SECTION_L3          =   4,
+    PROC_STOP_SECTION_MAX         =   5,
 } ScomSection_t;
 
 /**
@@ -148,7 +189,6 @@ typedef enum
     BIT_POS_LPCR        =   5,
     BIT_POS_PSSCR       =   6,
     BIT_POS_MSR         =   7,
-    BIT_POS_HRMOR       =   20,
     BIT_POS_HID         =   21,
     BIT_POS_HMEER       =   22,
     BIT_POS_PMCR        =   23,
@@ -156,7 +196,6 @@ typedef enum
     BIT_POS_SMFCTRL     =   28,
     BIT_POS_USPRG0      =   29,
     BIT_POS_USPRG1      =   30,
-    BIT_POS_URMOR       =   31,
 } SprBitPositionList_t;
 
 
@@ -229,13 +268,79 @@ p9_stop_save_cpureg_control( void* i_pImage, const uint64_t i_pir,
  * @brief       initializes self-save region with specific instruction.
  * @param[in]   i_pImage    start address of homer image of P9 chip.
  * @param[in]   i_corePos   physical core's relative position within processor chip.
- * @return      STOP_SAVE_SUCCESS SUCCESS if self-save is initialized successfully,
+ * @return      STOP_SAVE_SUCCESS  if self-save is initialized successfully,
  *              error code otherwise.
  * @note        API is intended only for use case of HOMER build. There is no explicit
  *              effort to support any other use case.
  */
 StopReturnCode_t p9_stop_init_self_save(  void* const i_pImage, const uint32_t i_corePos );
 
+/**
+ * @brief   creates SCOM restore entry for a given scom adress in HOMER.
+ * @param   i_pImage        points to start address of HOMER image.
+ * @param   i_scomAddress   address associated with SCOM restore entry.
+ * @param   i_scomData      data associated with SCOM restore entry.
+ * @param   i_operation     operation type requested for API.
+ * @param   i_section       section of HOMER in which restore entry needs to be created.
+ * @return  STOP_SAVE_SUCCESS if API succeeds, error code otherwise.
+ * @note    It is an API for creating SCOM restore entry in HOMER. It is agnostic to
+ *          generation of POWER processor.
+ */
+
+StopReturnCode_t proc_stop_save_scom( void* const   i_pImage,
+                                      const uint32_t i_scomAddress,
+                                      const uint64_t i_scomData,
+                                      const ScomOperation_t i_operation,
+                                      const ScomSection_t i_section );
+
+/**
+ * @brief       initializes self save restore region of HOMER.
+ * @param[in]   i_pImage    points to base of HOMER image.
+ * @param[in]   i_corePos   position of the physical core.
+ * @return      STOP_SAVE_SUCCESS if API succeeds, error code otherwise.
+ * @note        It is an API for initializing self restore region in HOMER. It is agnostic to
+ *              generation of POWER processor.
+ */
+StopReturnCode_t proc_stop_init_cpureg(  void* const i_pImage, const uint32_t i_corePos );
+
+/**
+ * @brief       enables self save for a given set of SPRs
+ * @param[in]   i_pImage        points to start address of HOMER image.
+ * @param[in]   i_pir           PIR value associated with core and thread.
+ * @param[in]   i_saveRegVector bit vector representing the SPRs that needs to be self saved.
+ * @return      STOP_SAVE_SUCCESS if API succeeds, error code otherwise.
+ * @note        It is an API for enabling self save of SPRs  and it is agnostic to
+ *              generation of POWER processor.
+ */
+StopReturnCode_t proc_stop_save_cpureg_control(  void* i_pImage,
+        const uint64_t i_pir,
+        const uint32_t i_saveRegVector );
+
+/**
+ * @brief       creates an SPR restore entry in HOMER
+ * @param[in]   i_pImage        points to start address of HOMER image.
+ * @param[in]   i_pir           PIR value associated with core and thread.
+ * @param[in]   i_saveRegVector bit vector representing the SPRs that needs to be self saved.
+ * @return      STOP_SAVE_SUCCESS if API succeeds, error code otherwise.
+ * @note        It is an API for enabling self save of SPRs  and it is agnostic to
+ *              generation of POWER processor.
+ */
+StopReturnCode_t proc_stop_save_cpureg(  void* const i_pImage,
+        const CpuReg_t  i_regId,
+        const uint64_t  i_regData,
+        const uint64_t  i_pir );
+
+/**
+ * @brief       initializes self-save region with specific instruction.
+ * @param[in]   i_pImage    start address of homer image.
+ * @param[in]   i_corePos   physical core's relative position within processor chip.
+ * @return      STOP_SAVE_SUCCESS  if self-save is initialized successfully,
+ *              error code otherwise.
+ * @note        API is project agnostic and is intended only for use case of HOMER build.
+ *              There is no explicit effort to support any other use case.
+ */
+StopReturnCode_t proc_stop_init_self_save(  void* const i_pImage, const uint32_t i_corePos );
+
 #ifdef __cplusplus
 } // extern "C"
 };  // namespace stopImageSection ends
diff --git a/libpore/p9_stop_data_struct.H b/libpore/p9_stop_data_struct.H
index 1e9721e0..4e73aab5 100644
--- a/libpore/p9_stop_data_struct.H
+++ b/libpore/p9_stop_data_struct.H
@@ -67,9 +67,9 @@ enum
     SIZE_PER_SPR_RESTORE_INST   =   ((4 * sizeof(uint8_t)) / sizeof(uint32_t)),
     MAX_THREAD_LEVEL_SPRS       =   11,
     MAX_CORE_LEVEL_SPRS         =   6,
-    MAX_SPR_BIT_POS             =   31,
+    MAX_SPR_BIT_POS             =   30,
     SPR_BIT_POS_8               =    8,
-    SPR_BIT_POS_19              =   19,
+    SPR_BIT_POS_20              =   20,
     SPR_BIT_POS_25              =   25,
     SPR_BIT_POS_27              =   27,
 };
diff --git a/libpore/p9_stop_util.H b/libpore/p9_stop_util.H
index 3266fdef..79b4e959 100644
--- a/libpore/p9_stop_util.H
+++ b/libpore/p9_stop_util.H
@@ -5,7 +5,7 @@
 /*                                                                        */
 /* OpenPOWER HostBoot Project                                             */
 /*                                                                        */
-/* Contributors Listed Below - COPYRIGHT 2016,2017                        */
+/* Contributors Listed Below - COPYRIGHT 2016,2018                        */
 /* [+] International Business Machines Corp.                              */
 /*                                                                        */
 /*                                                                        */
@@ -95,7 +95,10 @@ typedef struct
     uint64_t cpmrMagicWord;
     uint32_t buildDate;
     uint32_t version;
-    uint8_t  reserve1[7];
+    uint8_t  reserve1[4];
+    uint8_t  selfRestoreVer;
+    uint8_t  stopApiVer;
+    uint8_t  urmorFix;
     uint8_t  fusedModeStatus;
     uint32_t cmeImgOffset;
     uint32_t cmeImgLength;
-- 
2.24.1


^ permalink raw reply related

* [PATCH v6 0/4] Support for Self Save API in OPAL
From: Pratik Rajesh Sampat @ 2020-03-26  7:09 UTC (permalink / raw)
  To: skiboot, oohall, linux-kernel, linuxppc-dev, mpe, ego, linuxram,
	psampat, pratik.r.sampat

v5:https://lists.ozlabs.org/pipermail/skiboot/2020-March/016538.html
Changelog
v5 --> v6
Updated background, motivation and illuminated potential design choices

Background
==========

The power management framework on POWER systems include core idle
states that lose context. Deep idle states namely "winkle" on POWER8
and "stop4" and "stop5" on POWER9 can be entered by a CPU to save
different levels of power, as a consequence of which all the
hypervisor resources such as SPRs and SCOMs are lost.

For most SPRs, saving and restoration of content for SPRs and SCOMs
is handled by the hypervisor kernel prior to entering an post exit
from an idle state respectively. However, there is a small set of
critical SPRs and XSCOMs that are expected to contain sane values even
before the control is transferred to the hypervisor kernel at system
reset vector.

For this purpose, microcode firmware provides a mechanism to restore
values on certain SPRs. The communication mechanism between the
hypervisor kernel and the microcode is a standard interface called
sleep-winkle-engine (SLW) on Power8 and Stop-API on Power9 which is
abstracted by OPAL calls from the hypervisor kernel. The Stop-API
provides an interface known as the self-restore API, to which the SPR
number and a predefined value to be restored on wake-up from a deep
stop state is supplied.


Motivation to introduce a new Stop-API
======================================

The self-restore API expects not just the SPR number but also the
value with which the SPR is restored. This is good for those SPRs such
as HSPRG0 whose values do not change at runtime, since for them, the
kernel can invoke the self-restore API at boot time once the values of
these SPRs are determined.

However, there are use-cases where-in the value to be saved cannot be
known or cannot be updated in the layer it currently is.
The shortcomings and the new use-cases which cannot be served by the
existing self-restore API, serves as motivation for a new API:

Shortcoming1:
------------
In a special wakeup scenario, SPRs such as PSSCR, whose values can
change at runtime, are compelled to make the self-restore API call
every time before entering a deep-idle state rendering it to be
prohibitively expensive

Shortcoming2:
------------
The value of LPCR is dynamic based on if the CPU is entered a stop
state during cpu idle versus cpu hotplug.
Today, an additional self-restore call is made before entering
CPU-Hotplug to clear the PECE1 bit in stop-API so that if we are
woken up by a special wakeup on an offlined CPU, we go back to stop
with the the bit cleared.
There is a overhead of an extra call

New Use-case:
-------------
In the case where the hypervisor is running on an
ultravisor environment, the boot time is too late in the cycle to make
the self-restore API calls, as these cannot be invoked from an
non-secure context anymore

To address these shortcomings, the firmware provides another API known
as the self-save API. The self-save API only takes the SPR number as a
parameter and will ensure that on wakeup from a deep-stop state the
SPR is restored with the value that it contained prior to entering the
deep-stop.

Contrast between self-save and self-restore APIs
================================================

		  Before entering
                  deep idle     |---------------|
                  ------------> | HCODE A       |                
                  |             |---------------|
   |---------|    |
   |   CPU   |----|
   |---------|    |             
                  |             |---------------|
                  |------------>| HCODE B       |
                  On waking up  |---------------|
                from deep idle




When a self-restore API is invoked, the HCODE inserts instructions
into "HCODE B" region of the above figure to restore the content of
the SPR to the said value. The "HCODE B" region gets executed soon
after the CPU wakes up from a deep idle state, thus executing the
inserted instructions, thereby restoring the contents of the SPRs to
the required values.

When a self-save API is invoked, the HCODE inserts instructions into
the "HCODE A" region of the above figure to save the content of the
SPR into some location in memory. It also inserts instructions into
the "HCODE B" region to restore the content of the SPR to the
corresponding value saved in the memory by the instructions in "HCODE
A" region.

Thus, in contrast with self-restore, the self-save API *does not* need
a value to be passed to it, since it ensures that the value of SPR
before entering deep stop is saved, and subsequently the same value is
restored.

Self-save and self-restore are complementary features since,
self-restore can help in restoring a different value in the SPR on
wakeup from a deep-idle state than what it had before entering the
deep idle state. This was used in POWER8 for HSPRG0 to distinguish a
wakeup from Winkle vs Fastsleep.

Limitations of self-save
========================
Ideally all SPRs should be available for self-save, but HID0 is very
tricky to implement in microcode due to various endianess quirks.
Couple of implementation schemes were buggy and hence HID0 was left
out to be self-restore only.

The fallout of this limitation is as follows:

* In Non PEF environment, no issue. Linux will use self-restore for
  HID0 as it does today and no functional impact.

* In PEF environment, the HID0 restore value is decided by OPAL during
  boot and it is setup for LE hypervisor with radix MMU. This is the
  default and current working configuration of a PEF environment.
  However if there is a change, then HV Linux will try to change the
  HID0 value to something different than what OPAL decided, at which
  time deep-stop states will be disabled under this new PEF
  environment.

A simple and workable design is achieved by scoping the power
management deep-stop state support only to a known default PEF
environment. Any deviation will affect *only* deep stop-state support
(stop4,5) in that environment and not have any functional impediment
to the environment itself.

In future, if there is a need to support changing of HID0 to various
values under PEF environment and support deep-stop states, it can be
worked out via an ultravisor call or improve the microcode design to
include HID0 in self-save.  These future scheme would be an extension
and does not break or make the current implementation scheme
redundant.

Design Choices
==============

Presenting the design choices in front of us:

Design-Choice 1:
----------------
Only expose one of self-save or self-restore for all the SPRs. Prefer
Self-save

Pros:
   - Simplifies the design heavily, since the Kernel can unambiguously
   make one API call for all the SPRs on discovering the presence of
   the API type.

Cons:
    - Breaks backward compatibility if OPAL always chooses to expose
      only the self-save API as the older kernels assume the existence
      of self-restore.

    - The set of SPRs supported by self-save and self-restore are not
      identical. Eg: HID0 is not supported by self-save API. PSSCR
      support via self-restore is not robust during special-wakeup.

    - As discussed above, self-save and self-restore are
      complementary. Thus OPAL apriory choosing one over the other for
      all SPRs takes away the flexibility from the kernel.


Design-Choice 2:
----------------
Expose two arrays of SPRs: One set of SPRs that are supported by
self-save. Another set of SPRs supported by self-restore. These two
sets do not intersect. Further, if an SPR is supported by both
self-save and self-restore APIs, expose it only via self-save.

Pros:
     - For an SPR the choice for the kernel is unambiguous.

Cons:
    - Breaks backward compatibility if OPAL always chooses to expose
      the legacy SPRs only via the self-save API as the older kernels
      assume the existence of self-restore.

    - By making the decision early on, we take away the flexibility
       from the kernel to use an API of its choice for an SPR.


Design-Choice 3
---------------
Expose two arrays of SPRs. One set of SPRs that are supported by
self-save API. Another set of SPRs supported by self-restore API. Let
the kernel choose which API to invoke. Even if it wants to always
prefer self-save over self-restore, let that be kernel's choice.

Pros:
     - Keeps the design flexible to allow the kernel to take a
       decision based on its functional and performance requirements.
       Thus, the kernel for instance can make a choice to invoke
       self-restore API (when available) for SPRs whose values do not
       evolve at runtime, and invoke the self-save API (when
       available)
       for SPRs whose values will change during runtime.

     - Design is backward compatible with older kernels.

Cons:
     - The Kernel code will have additional complexity for parsing two
     lists of SPRs and making a choice w.r.t invocation of a specific
     stop-api.



Patches Organization
====================
Design choice 3 has been chosen as an implementation to demonstrate in
this patch series.

Patch 1:
Commit adds support calling into the self save firmware API.
Also adds abstraction for making platform agnostic calls.

Patch 2:
Commit adds wrappers for the Self Save API for which an OPAL call can
be made.

Patch 3:
Commit adds API to determine the version of the STOP API. This helps
to identify support for self save in the firmware

Patch 4:
Commit adds device tree attributes to advertise self save and self
restore functionality along with the register set as a bitmask
currently supported in the firmware. It also uses the versioning API
to determine support for the self-save feature as a whole.

Pratik Rajesh Sampat (2):
  Self save API integration
  Advertise the self-save and self-restore attributes in the device tree

Prem Shanker Jha (2):
  Self Save: Introducing Support for SPR Self Save
  API to verify the STOP API and image compatibility

 .../ibm,opal/power-mgt/self-restore.rst       |  27 +
 .../ibm,opal/power-mgt/self-save.rst          |  27 +
 doc/opal-api/opal-slw-self-save-reg-181.rst   |  49 +
 doc/opal-api/opal-slw-set-reg-100.rst         |   5 +
 doc/power-management.rst                      |  44 +
 hw/slw.c                                      | 205 ++++
 include/opal-api.h                            |   3 +-
 include/p9_stop_api.H                         | 122 ++-
 include/skiboot.h                             |   4 +
 libpore/p9_cpu_reg_restore_instruction.H      |  11 +-
 libpore/p9_hcd_memmap_base.H                  |   7 +
 libpore/p9_stop_api.C                         | 964 ++++++++++--------
 libpore/p9_stop_api.H                         | 141 ++-
 libpore/p9_stop_data_struct.H                 |   4 +-
 libpore/p9_stop_util.H                        |  27 +-
 15 files changed, 1208 insertions(+), 432 deletions(-)
 create mode 100644 doc/device-tree/ibm,opal/power-mgt/self-restore.rst
 create mode 100644 doc/device-tree/ibm,opal/power-mgt/self-save.rst
 create mode 100644 doc/opal-api/opal-slw-self-save-reg-181.rst

-- 
2.24.1


^ permalink raw reply

* [PATCH v2] powerpc xmon: use `dcbf` inplace of `dcbi` instruction for 64bit Book3S
From: Balamuruhan S @ 2020-03-26  6:15 UTC (permalink / raw)
  To: mpe
  Cc: ravi.bangoria, jniethe5, Balamuruhan S, paulus, sandipan,
	naveen.n.rao, linuxppc-dev

Data Cache Block Invalidate (dcbi) instruction was implemented back in PowerPC
architecture version 2.03. It is obsolete and attempt to use of this illegal
instruction results in a hypervisor emulation assistance interrupt. So, ifdef
it out the option `i` in xmon for 64bit Book3S.

0:mon> fi
cpu 0x0: Vector: 700 (Program Check) at [c000000003be74a0]
    pc: c000000000102030: cacheflush+0x180/0x1a0
    lr: c000000000101f3c: cacheflush+0x8c/0x1a0
    sp: c000000003be7730
   msr: 8000000000081033
  current = 0xc0000000035e5c00
  paca    = 0xc000000001910000   irqmask: 0x03   irq_happened: 0x01
    pid   = 1025, comm = bash
Linux version 5.6.0-rc5-g5aa19adac (root@ltc-wspoon6) (gcc version 7.4.0
(Ubuntu 7.4.0-1ubuntu1~18.04.1)) #1 SMP Tue Mar 10 04:38:41 CDT 2020
cpu 0x0: Exception 700 (Program Check) in xmon, returning to main loop
[c000000003be7c50] c00000000084abb0 __handle_sysrq+0xf0/0x2a0
[c000000003be7d00] c00000000084b3c0 write_sysrq_trigger+0xb0/0xe0
[c000000003be7d30] c0000000004d1edc proc_reg_write+0x8c/0x130
[c000000003be7d60] c00000000040dc7c __vfs_write+0x3c/0x70
[c000000003be7d80] c000000000410e70 vfs_write+0xd0/0x210
[c000000003be7dd0] c00000000041126c ksys_write+0xdc/0x130
[c000000003be7e20] c00000000000b9d0 system_call+0x5c/0x68
--- Exception: c01 (System Call) at 00007fffa345e420
SP (7ffff0b08ab0) is in userspace

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 arch/powerpc/xmon/xmon.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
---
changes in v2:
-------------
Fix review comments from Segher and Michael,
	* change incorrect architecture version 2.01 to 2.03 in commit
	  message.
	* ifdef it out the option `i` for PPC_BOOK3S_64 instead to drop it
	  and change the commit message accordingly.

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 0ec9640335bb..bfd5a97689cd 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -335,10 +335,12 @@ static inline void cflush(void *p)
 	asm volatile ("dcbf 0,%0; icbi 0,%0" : : "r" (p));
 }
 
+#ifndef CONFIG_PPC_BOOK3S_64
 static inline void cinval(void *p)
 {
 	asm volatile ("dcbi 0,%0; icbi 0,%0" : : "r" (p));
 }
+#endif
 
 /**
  * write_ciabr() - write the CIABR SPR
@@ -1791,8 +1793,9 @@ static void prregs(struct pt_regs *fp)
 
 static void cacheflush(void)
 {
-	int cmd;
 	unsigned long nflush;
+#ifndef CONFIG_PPC_BOOK3S_64
+	int cmd;
 
 	cmd = inchar();
 	if (cmd != 'i')
@@ -1800,13 +1803,14 @@ static void cacheflush(void)
 	scanhex((void *)&adrs);
 	if (termch != '\n')
 		termch = 0;
+#endif
 	nflush = 1;
 	scanhex(&nflush);
 	nflush = (nflush + L1_CACHE_BYTES - 1) / L1_CACHE_BYTES;
 	if (setjmp(bus_error_jmp) == 0) {
 		catch_memory_errors = 1;
 		sync();
-
+#ifndef CONFIG_PPC_BOOK3S_64
 		if (cmd != 'i') {
 			for (; nflush > 0; --nflush, adrs += L1_CACHE_BYTES)
 				cflush((void *) adrs);
@@ -1814,6 +1818,10 @@ static void cacheflush(void)
 			for (; nflush > 0; --nflush, adrs += L1_CACHE_BYTES)
 				cinval((void *) adrs);
 		}
+#else
+		for (; nflush > 0; --nflush, adrs += L1_CACHE_BYTES)
+			cflush((void *)adrs);
+#endif
 		sync();
 		/* wait a little while to see if we get a machine check */
 		__delay(200);

base-commit: a87b93bdf800a4d7a42d95683624a4516e516b4f
-- 
2.24.1


^ permalink raw reply related

* [PATCH] selftests/eeh: Skip ahci adapters
From: Michael Ellerman @ 2020-03-26  6:11 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: oohall

The ahci driver doesn't support error recovery, and if your root
filesystem is attached to it the eeh-basic.sh test will likely kill
your machine.

So skip any device we see using the ahci driver.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 tools/testing/selftests/powerpc/eeh/eeh-basic.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/powerpc/eeh/eeh-basic.sh b/tools/testing/selftests/powerpc/eeh/eeh-basic.sh
index f988d2f42e8f..8a8d0f456946 100755
--- a/tools/testing/selftests/powerpc/eeh/eeh-basic.sh
+++ b/tools/testing/selftests/powerpc/eeh/eeh-basic.sh
@@ -41,6 +41,11 @@ for dev in `ls -1 /sys/bus/pci/devices/ | grep '\.0$'` ; do
 		continue;
 	fi
 
+	if [ "ahci" = "$(basename $(realpath /sys/bus/pci/devices/$dev/driver))" ] ; then
+		echo "$dev, Skipped: ahci doesn't support recovery"
+		continue
+	fi
+
 	# Don't inject errosr into an already-frozen PE. This happens with
 	# PEs that contain multiple PCI devices (e.g. multi-function cards)
 	# and injecting new errors during the recovery process will probably

base-commit: 11a48a5a18c63fd7621bb050228cebf13566e4d8
prerequisite-patch-id: 2d4d04fc1422098aacadc8743efec84d3929bc0b
prerequisite-patch-id: 6063d411906b0b6f41dfce6cfd3f4795f25e9c11
prerequisite-patch-id: 20393e4806e464b171d542d3de4e41bbba07d521
prerequisite-patch-id: b1c3d27aaaeca2b94cab75a82008db7e4d3fc9f4
prerequisite-patch-id: 721316c5f0021ac0dbc4d03f68c21a77eef88d08
prerequisite-patch-id: 69a161a8f9f2175cce8b4a62c0e86b4bbdab5f10
prerequisite-patch-id: e47c4610cf53f8a30fb4a3541751292dec47a1c9
prerequisite-patch-id: f39dd059e3ad8af1bd91712b3cdc3f4b1b4c2215
prerequisite-patch-id: 717bf28a8254643211185582bc16f73cdde337ad
prerequisite-patch-id: bf548a1a2f6ccd8a2afdd967ddcab3c21b6fa584
prerequisite-patch-id: 2a2e7a1230dff4cf3329406a745f04668d44deef
prerequisite-patch-id: 333c6a243645d61627cfa1db1b476d831f685733
prerequisite-patch-id: b501b455480e2b45f81e07d7f02856df979644f8
prerequisite-patch-id: 893d532f9f307918862be03babd8dedf7cc5c696
prerequisite-patch-id: cab07afe25c3eb8c7aa3bf4d80e045ac422df1d6
prerequisite-patch-id: 52d5c5e98ad0695d631e336c5746b2074f08469b
prerequisite-patch-id: c8ea5b2983fb74822ab08e2c8a478cb17ff3803d
prerequisite-patch-id: 6dc50fcb267fb29bf8a6ac2e9597371e0772aec1
prerequisite-patch-id: 8552363b8e81fa95829e48d31dd12c5659338ef2
prerequisite-patch-id: 46d96230b4c749208a533d730e4f75b500b3bfac
prerequisite-patch-id: 224ac5e18a1d8674d1e7cf49b7f3e2008c4a213e
prerequisite-patch-id: a52022bfd2c0f6391cfa476202e2ddd5cd676eb0
prerequisite-patch-id: 58cfb87f55f07f147f882f3c9109eb5ad70bfbe1
prerequisite-patch-id: c3752d1c343f9fdbfccd757746e0aa9b53c0c8ae
prerequisite-patch-id: 37e1aeadd0ee2f14c15e29b2e2faee113f12f1cd
prerequisite-patch-id: eb75889d6d31b8807e42c8f03956730c7c8d577a
prerequisite-patch-id: 67484b56a17bd7ac6988ef6a15159279fca59e98
prerequisite-patch-id: 6168f9ec8beb6cb4e711510cafd9a87e717002ea
prerequisite-patch-id: 7646f6fb17ff2fc4f4734d2aa246053dc7dca410
prerequisite-patch-id: d79707796ed09bcf0b495a18aa2d52d00674c69f
prerequisite-patch-id: 648730d1c7e784f873a3b2cf2b6447279a573b5b
prerequisite-patch-id: ef5983802b94d095a5c9cbf962b51cf461981750
prerequisite-patch-id: 2e95a065d81d983ed0056b238ff047c187f5cb91
prerequisite-patch-id: 607615b23dee8b92047aaef1ebb8e50c20f68bcb
prerequisite-patch-id: a30f22e4e13063eb4adc2209b88ec96059078ff5
prerequisite-patch-id: b80bbfd5aa13fc43d332968944f8e1e58f059878
prerequisite-patch-id: 94923ed7ac933489c5ed4f2d61d8f267424f4d43
prerequisite-patch-id: b4820918562f815e50d12e07f264286321f7f711
prerequisite-patch-id: a34fb1f33467b3215196d48ecafbf5483fbb74f3
prerequisite-patch-id: 49f0ce9ceb6adb8c208f57180a8b7348dc5bba06
prerequisite-patch-id: 252d2696d7a34bbae579f15e98324abf843ce08c
prerequisite-patch-id: efc754c834ee2bdcf99fb52f9b942cc5a3dc117d
prerequisite-patch-id: 3b26fa3c1eb1fdf26766a2d5d81e3e171f4539a9
prerequisite-patch-id: c8279b3c9f52ad6420f462cf60d4e52da28ad854
prerequisite-patch-id: 3bbc77d19e638aa15e7d587f6aa4d295a6cf13ca
prerequisite-patch-id: ff39259dc64c2bbe01a88d3b03fdbca4b4836742
prerequisite-patch-id: 720aa5f2bf14f55f28e5ca370c81e7f39bce2f78
prerequisite-patch-id: bfe24b8ed9962db12a48cf819880f4423b54ac1b
prerequisite-patch-id: f25df222a309609258a24bbd334bf91ef3a33ee4
prerequisite-patch-id: b0e3e990e1282bde385d92c0a044be24d30beb9c
prerequisite-patch-id: 30953eacc324ccfc71838fd6428572b14fa86568
prerequisite-patch-id: b59be8efdfc2ca1067275009a83cb74269f82d52
prerequisite-patch-id: f1baea64b666a8d85dff464c25a27b7d4fc616cb
prerequisite-patch-id: d821a76d44ebab904b3b0225d77e52f29c51b346
prerequisite-patch-id: 9a4c443a42166871abb4646c64a90802d4a50275
prerequisite-patch-id: 8b273fe00374c0df5d520206ff5ebdcd74e1b30d
prerequisite-patch-id: 2ac5d698941f4ff6ee12869496554abe201ff69c
prerequisite-patch-id: a9149c87dfe0a855ebfaa2dae5af255d0ca4c236
prerequisite-patch-id: 4cd89ebdfc17035c5e34b60968c3c48253d5c57b
prerequisite-patch-id: 8f3294718baa9f9d2ca2323895554b44517d00d1
prerequisite-patch-id: 27fb2d81a279c08e15fbe45e415bcc3216587b94
prerequisite-patch-id: 90830ae4818c76d8bf6203eff694d7fcba02be92
prerequisite-patch-id: fca67e4393f1f83d2edf93e1e1a1305e777f122d
prerequisite-patch-id: 685354a4dcd12db2e8f85df129fddc409491744d
prerequisite-patch-id: 4a77df82d7fe010beb0b251288006e0fac7b0126
prerequisite-patch-id: bd610e237ea26c096b97b7396dda22a0340bdb95
prerequisite-patch-id: 5480ab41818af05b68154e6f6b0e9251d7de13a1
prerequisite-patch-id: d7f43a5775c32b0c2bbc6427149bff0cdfb7b9f7
prerequisite-patch-id: a4eff81f404c8c543cfd1a78d2715a51bc9184be
prerequisite-patch-id: d0c4d212c1890cd976d1638b4cd5572bbfb5e6eb
prerequisite-patch-id: d3a94fa7de5e9b3ff4f0c28a4ff7af94b06d619c
prerequisite-patch-id: b8f766f3e08241dc218de06211b9745eeb1d5733
prerequisite-patch-id: 16a84abee274f1f819300e325ca1c3d6c9900fd9
prerequisite-patch-id: 89f32cc81d135697e3e7242be6a0c84c9b576972
prerequisite-patch-id: c271d8516dd39526e848dfa95ae38c9205002d2c
prerequisite-patch-id: 927bdde07068e23446aad1958fcbc156b700f516
prerequisite-patch-id: e72a423ecd2cea371d73a60a443b2795875e1cc5
prerequisite-patch-id: e3ddba7e9caefd15c4eec34d1fabb10c786659db
prerequisite-patch-id: f846893eaa5744e41ff077ead047d27bdc8d2ef7
prerequisite-patch-id: 1eb659811a3edc87c9cfff890bf7628c42bf8946
prerequisite-patch-id: 5da0949d5826053fa166b3bd6341e944ba386294
prerequisite-patch-id: b78bb91b72db55bae1d08ba2525d5808a582bf8d
prerequisite-patch-id: 7563087dc5d114383025fbb0236ef8daaa9ae5e6
prerequisite-patch-id: 355de69b4b62f5fc6257ec4b326d843ecaf2c38c
prerequisite-patch-id: ebf08af330da94d0ca102aa21a8872da66969bba
prerequisite-patch-id: 0e57b29bc26e059c72949ef52640fff3eb9aad00
prerequisite-patch-id: 0b421b5c51a0d35696056543e59bc045ac5c22fc
prerequisite-patch-id: c3e73514122d2f5efeff57d891648cdbaeec6adb
prerequisite-patch-id: 3bd82106b77dd35c5b7efc2e48c86d5c4768b485
prerequisite-patch-id: ab3725338ced943c0ebbee475a5f31fa94f144be
prerequisite-patch-id: d93fe877a2f9228f7452710553a18c9b84a3f80c
prerequisite-patch-id: 10251357b689645aa95f5af29dd46d28fb17dc6a
prerequisite-patch-id: bc688bb33ad7c454c17fe59e5ccdde5d9318a30b
prerequisite-patch-id: 6469cbb8d6df8d6c114d86d654c93b89b30d83df
prerequisite-patch-id: 85923199da315460a7aa7fff1ad5635fa08befdf
prerequisite-patch-id: a30c16bcef004fb0fd5a1a813f05c04d3b154bb4
prerequisite-patch-id: 1336ac0045e8680bdcecd2a88ea06c3908f0dd52
prerequisite-patch-id: 45c3b42999004eb772a3159c91abdda69d9e05d4
prerequisite-patch-id: 3772da730640e7d04cf93e8d9901b825137ade2c
-- 
2.25.1


^ permalink raw reply related

* Re: [PATCH 07/15] powerpc/watchpoint: Get watchpoint count dynamically while disabling them
From: Ravi Bangoria @ 2020-03-26  3:32 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: apopple, mikey, Ravi Bangoria, peterz, oleg, npiggin,
	linux-kernel, paulus, jolsa, fweisbec, naveen.n.rao, linuxppc-dev,
	mingo
In-Reply-To: <cca28aeb-d1f8-5668-0743-2269f621e926@linux.ibm.com>



On 3/18/20 12:27 PM, Ravi Bangoria wrote:
> 
> 
> On 3/17/20 4:02 PM, Christophe Leroy wrote:
>>
>>
>> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>>> Instead of disabling only one watchpooint, get num of available
>>> watchpoints dynamically and disable all of them.
>>>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>> ---
>>>   arch/powerpc/include/asm/hw_breakpoint.h | 15 +++++++--------
>>>   1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
>>> index 980ac7d9f267..ec61e2b7195c 100644
>>> --- a/arch/powerpc/include/asm/hw_breakpoint.h
>>> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
>>> @@ -75,14 +75,13 @@ extern void ptrace_triggered(struct perf_event *bp,
>>>               struct perf_sample_data *data, struct pt_regs *regs);
>>>   static inline void hw_breakpoint_disable(void)
>>>   {
>>> -    struct arch_hw_breakpoint brk;
>>> -
>>> -    brk.address = 0;
>>> -    brk.type = 0;
>>> -    brk.len = 0;
>>> -    brk.hw_len = 0;
>>> -    if (ppc_breakpoint_available())
>>> -        __set_breakpoint(&brk, 0);
>>> +    int i;
>>> +    struct arch_hw_breakpoint null_brk = {0};
>>> +
>>> +    if (ppc_breakpoint_available()) {
>>
>> I think this test should go into nr_wp_slots() which should return zero when no breakpoint is available.
> 
> Seems possible. Will change it in next version.

Once we move ppc_breakpoint_available() logic into nr_wp_slots(),
'dawr_force_enable' variable is checked in nr_wp_slots() before
it gets initialized in dawr_force_setup():

     start_kernel()
     |-> perf_event_init()
     |     init_hw_breakpoint()
     |       hw_breakpoint_slots()
     |         nr_wp_slots()
     |           /* Check dawr_force_enable variable */
     |
     |-> arch_call_rest_init()
           rest_init()
             kernel_thread(kernel_init, ...)
               ...
                 kernel_init()
                   kernel_init_freeable()
                     ...
                     do_one_initcall()
                       dawr_force_setup()
                         /* Set dawr_force_enable = true */

Because of this, hw-breakpoint infrastructure is initialized with
no DAWRs. So I'm thinking to keep the code as it is i.e. not moving
ppc_breakpoint_available() test inside nr_wp_slots().

Ravi


^ permalink raw reply

* Re: [PATCH v2] Documentation/locking/locktypes: minor copy editor fixes
From: Paul E. McKenney @ 2020-03-26  2:40 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: linux-ia64, Peter Zijlstra, linux-pci, Sebastian Siewior,
	Oleg Nesterov, Guo Ren, Joel Fernandes, Vincent Chen,
	Thomas Gleixner, Davidlohr Bueso, linux-acpi, Brian Cain,
	Jonathan Corbet, linux-hexagon, Rafael J. Wysocki, linux-csky,
	Ingo Molnar, Linus Torvalds, Darren Hart, Zhang Rui, Len Brown,
	Fenghua Yu, Arnd Bergmann, linux-pm, linuxppc-dev, Greentime Hu,
	Bjorn Helgaas, Kurt Schwemmer, platform-driver-x86, Kalle Valo,
	kbuild test robot, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
	Geoff Levand, Greg Kroah-Hartman, linux-usb, linux-wireless, LKML,
	Davidlohr Bueso, netdev, Logan Gunthorpe, David S. Miller,
	Andy Shevchenko
In-Reply-To: <ac615f36-0b44-408d-aeab-d76e4241add4@infradead.org>

On Wed, Mar 25, 2020 at 09:58:14AM -0700, Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@infradead.org>
> 
> Minor editorial fixes:
> - add some hyphens in multi-word adjectives
> - add some periods for consistency
> - add "'" for possessive CPU's
> - capitalize IRQ when it's an acronym and not part of a function name
> 
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Paul McKenney <paulmck@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sebastian Siewior <bigeasy@linutronix.de>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>

Some nits below, but with or without those suggested changes:

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  Documentation/locking/locktypes.rst |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> --- linux-next-20200325.orig/Documentation/locking/locktypes.rst
> +++ linux-next-20200325/Documentation/locking/locktypes.rst
> @@ -84,7 +84,7 @@ rtmutex
>  
>  RT-mutexes are mutexes with support for priority inheritance (PI).
>  
> -PI has limitations on non PREEMPT_RT enabled kernels due to preemption and
> +PI has limitations on non-PREEMPT_RT-enabled kernels due to preemption and

Or just drop the " enabled".

>  interrupt disabled sections.
>  
>  PI clearly cannot preempt preemption-disabled or interrupt-disabled
> @@ -150,7 +150,7 @@ kernel configuration including PREEMPT_R
>  
>  raw_spinlock_t is a strict spinning lock implementation in all kernels,
>  including PREEMPT_RT kernels.  Use raw_spinlock_t only in real critical
> -core code, low level interrupt handling and places where disabling
> +core code, low-level interrupt handling and places where disabling
>  preemption or interrupts is required, for example, to safely access
>  hardware state.  raw_spinlock_t can sometimes also be used when the
>  critical section is tiny, thus avoiding RT-mutex overhead.
> @@ -160,20 +160,20 @@ spinlock_t
>  
>  The semantics of spinlock_t change with the state of PREEMPT_RT.
>  
> -On a non PREEMPT_RT enabled kernel spinlock_t is mapped to raw_spinlock_t
> +On a non-PREEMPT_RT-enabled kernel spinlock_t is mapped to raw_spinlock_t

Ditto.

>  and has exactly the same semantics.
>  
>  spinlock_t and PREEMPT_RT
>  -------------------------
>  
> -On a PREEMPT_RT enabled kernel spinlock_t is mapped to a separate
> +On a PREEMPT_RT-enabled kernel spinlock_t is mapped to a separate

And here as well.

>  implementation based on rt_mutex which changes the semantics:
>  
> - - Preemption is not disabled
> + - Preemption is not disabled.
>  
>   - The hard interrupt related suffixes for spin_lock / spin_unlock
> -   operations (_irq, _irqsave / _irqrestore) do not affect the CPUs
> -   interrupt disabled state
> +   operations (_irq, _irqsave / _irqrestore) do not affect the CPU's
> +   interrupt disabled state.
>  
>   - The soft interrupt related suffix (_bh()) still disables softirq
>     handlers.
> @@ -279,7 +279,7 @@ fully preemptible context.  Instead, use
>  spin_lock_irqsave() and their unlock counterparts.  In cases where the
>  interrupt disabling and locking must remain separate, PREEMPT_RT offers a
>  local_lock mechanism.  Acquiring the local_lock pins the task to a CPU,
> -allowing things like per-CPU irq-disabled locks to be acquired.  However,
> +allowing things like per-CPU IRQ-disabled locks to be acquired.  However,

Quite a bit of text in the kernel uses "irq", lower case.  Another
option is to spell out "interrupt".

>  this approach should be used only where absolutely necessary.
>  
>  
> 

^ permalink raw reply

* Re: [PATCH V2 0/3] mm/debug: Add more arch page table helper tests
From: Anshuman Khandual @ 2020-03-26  2:23 UTC (permalink / raw)
  To: linux-mm
  Cc: linux-doc, Heiko Carstens, Paul Mackerras, H. Peter Anvin,
	linux-riscv, Will Deacon, linux-arch, linux-s390, Jonathan Corbet,
	x86, Mike Rapoport, Christian Borntraeger, Ingo Molnar,
	Catalin Marinas, linux-snps-arc, Vasily Gorbik, Borislav Petkov,
	Paul Walmsley, Kirill A . Shutemov, Thomas Gleixner,
	linux-arm-kernel, Vineet Gupta, linux-kernel, Palmer Dabbelt,
	Andrew Morton, linuxppc-dev
In-Reply-To: <1585027375-9997-1-git-send-email-anshuman.khandual@arm.com>



On 03/24/2020 10:52 AM, Anshuman Khandual wrote:
> This series adds more arch page table helper tests. The new tests here are
> either related to core memory functions and advanced arch pgtable helpers.
> This also creates a documentation file enlisting all expected semantics as
> suggested by Mike Rapoport (https://lkml.org/lkml/2020/1/30/40).
> 
> This series has been tested on arm64 and x86 platforms.

If folks can test these patches out on remaining ARCH_HAS_DEBUG_VM_PGTABLE
enabled platforms i.e s390, arc, powerpc (32 and 64), that will be really
appreciated. Thank you.

- Anshuman

^ permalink raw reply

* Re: [PATCH V2 1/3] mm/debug: Add tests validating arch page table helpers for core features
From: Anshuman Khandual @ 2020-03-26  2:18 UTC (permalink / raw)
  To: Zi Yan
  Cc: Heiko Carstens, linux-mm, Paul Mackerras, H. Peter Anvin,
	linux-riscv, Will Deacon, linux-arch, linux-s390, x86,
	Mike Rapoport, Christian Borntraeger, Ingo Molnar,
	Catalin Marinas, linux-snps-arc, Vasily Gorbik, Borislav Petkov,
	Paul Walmsley, Kirill A . Shutemov, Thomas Gleixner,
	linux-arm-kernel, Vineet Gupta, linux-kernel, Palmer Dabbelt,
	Andrew Morton, linuxppc-dev
In-Reply-To: <89E72C74-A32F-4A5B-B5F3-8A63428507A5@nvidia.com>


On 03/24/2020 06:59 PM, Zi Yan wrote:
> On 24 Mar 2020, at 1:22, Anshuman Khandual wrote:
> 
>> This adds new tests validating arch page table helpers for these following
>> core memory features. These tests create and test specific mapping types at
>> various page table levels.
>>
>> 1. SPECIAL mapping
>> 2. PROTNONE mapping
>> 3. DEVMAP mapping
>> 4. SOFTDIRTY mapping
>> 5. SWAP mapping
>> 6. MIGRATION mapping
>> 7. HUGETLB mapping
>> 8. THP mapping
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Mike Rapoport <rppt@linux.ibm.com>
>> Cc: Vineet Gupta <vgupta@synopsys.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Kirill A. Shutemov <kirill@shutemov.name>
>> Cc: Paul Walmsley <paul.walmsley@sifive.com>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Cc: linux-snps-arc@lists.infradead.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-s390@vger.kernel.org
>> Cc: linux-riscv@lists.infradead.org
>> Cc: x86@kernel.org
>> Cc: linux-arch@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  mm/debug_vm_pgtable.c | 291 +++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 290 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 98990a515268..15055a8f6478 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -289,6 +289,267 @@ static void __init pmd_populate_tests(struct mm_struct *mm, pmd_t *pmdp,
>>  	WARN_ON(pmd_bad(pmd));
>>  }
>>
>> +static void __init pte_special_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pte_t pte = pfn_pte(pfn, prot);
>> +
>> +	if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL))
>> +		return;
>> +
>> +	WARN_ON(!pte_special(pte_mkspecial(pte)));
>> +}
>> +
>> +static void __init pte_protnone_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pte_t pte = pfn_pte(pfn, prot);
>> +
>> +	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
>> +		return;
>> +
>> +	WARN_ON(!pte_protnone(pte));
>> +	WARN_ON(!pte_present(pte));
>> +}
>> +
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pmd_t pmd = pfn_pmd(pfn, prot);
>> +
>> +	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
>> +		return;
>> +
>> +	WARN_ON(!pmd_protnone(pmd));
>> +	WARN_ON(!pmd_present(pmd));
>> +}
>> +#else
>> +static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t prot) { }
>> +#endif
>> +
>> +#ifdef CONFIG_ARCH_HAS_PTE_DEVMAP
>> +static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pte_t pte = pfn_pte(pfn, prot);
>> +
>> +	WARN_ON(!pte_devmap(pte_mkdevmap(pte)));
>> +}
>> +
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pmd_t pmd = pfn_pmd(pfn, prot);
>> +
>> +	WARN_ON(!pmd_devmap(pmd_mkdevmap(pmd)));
>> +}
>> +
>> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pud_t pud = pfn_pud(pfn, prot);
>> +
>> +	WARN_ON(!pud_devmap(pud_mkdevmap(pud)));
>> +}
>> +#else
>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>> +#endif
>> +#else
>> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>> +#endif
>> +#else
>> +static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>> +#endif
>> +
>> +static void __init pte_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pte_t pte = pfn_pte(pfn, prot);
>> +
>> +	if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY))
>> +		return;
>> +
>> +	WARN_ON(!pte_soft_dirty(pte_mksoft_dirty(pte)));
>> +	WARN_ON(pte_soft_dirty(pte_clear_soft_dirty(pte)));
>> +}
>> +
>> +static void __init pte_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pte_t pte = pfn_pte(pfn, prot);
>> +
>> +	if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY))
>> +		return;
>> +
>> +	WARN_ON(!pte_swp_soft_dirty(pte_swp_mksoft_dirty(pte)));
>> +	WARN_ON(pte_swp_soft_dirty(pte_swp_clear_soft_dirty(pte)));
>> +}
>> +
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +static void __init pmd_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pmd_t pmd = pfn_pmd(pfn, prot);
>> +
>> +	if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY))
>> +		return;
>> +
>> +	WARN_ON(!pmd_soft_dirty(pmd_mksoft_dirty(pmd)));
>> +	WARN_ON(pmd_soft_dirty(pmd_clear_soft_dirty(pmd)));
>> +}
>> +
>> +static void __init pmd_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pmd_t pmd = pfn_pmd(pfn, prot);
>> +
>> +	if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY) ||
>> +		!IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION))
>> +		return;
>> +
>> +	WARN_ON(!pmd_swp_soft_dirty(pmd_swp_mksoft_dirty(pmd)));
>> +	WARN_ON(pmd_swp_soft_dirty(pmd_swp_clear_soft_dirty(pmd)));
>> +}
>> +#else
>> +static void __init pmd_soft_dirty_tests(unsigned long pfn, pgprot_t prot) { }
>> +static void __init pmd_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +}
>> +#endif
>> +
>> +static void __init pte_swap_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	swp_entry_t swp;
>> +	pte_t pte;
>> +
>> +	pte = pfn_pte(pfn, prot);
>> +	swp = __pte_to_swp_entry(pte);
>> +	WARN_ON(!pte_same(pte, __swp_entry_to_pte(swp)));
>> +}
>> +
>> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>> +static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	swp_entry_t swp;
>> +	pmd_t pmd;
>> +
>> +	pmd = pfn_pmd(pfn, prot);
>> +	swp = __pmd_to_swp_entry(pmd);
>> +	WARN_ON(!pmd_same(pmd, __swp_entry_to_pmd(swp)));
>> +}
>> +#else
>> +static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot) { }
>> +#endif
>> +
>> +static void __init swap_migration_tests(void)
>> +{
>> +	struct page *page;
>> +	swp_entry_t swp;
>> +
>> +	if (!IS_ENABLED(CONFIG_MIGRATION))
>> +		return;
>> +	/*
>> +	 * swap_migration_tests() requires a dedicated page as it needs to
>> +	 * be locked before creating a migration entry from it. Locking the
>> +	 * page that actually maps kernel text ('start_kernel') can be real
>> +	 * problematic. Lets allocate a dedicated page explicitly for this
>> +	 * purpose that will be freed subsequently.
>> +	 */
>> +	page = alloc_page(GFP_KERNEL);
>> +	if (!page) {
>> +		pr_err("page allocation failed\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * make_migration_entry() expects given page to be
>> +	 * locked, otherwise it stumbles upon a BUG_ON().
>> +	 */
>> +	__SetPageLocked(page);
>> +	swp = make_migration_entry(page, 1);
>> +	WARN_ON(!is_migration_entry(swp));
>> +	WARN_ON(!is_write_migration_entry(swp));
>> +
>> +	make_migration_entry_read(&swp);
>> +	WARN_ON(!is_migration_entry(swp));
>> +	WARN_ON(is_write_migration_entry(swp));
>> +
>> +	swp = make_migration_entry(page, 0);
>> +	WARN_ON(!is_migration_entry(swp));
>> +	WARN_ON(is_write_migration_entry(swp));
>> +	__ClearPageLocked(page);
>> +	__free_page(page);
>> +}
>> +
>> +#ifdef CONFIG_HUGETLB_PAGE
>> +static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	struct page *page;
>> +	pte_t pte;
>> +
>> +	/*
>> +	 * Accessing the page associated with the pfn is safe here,
>> +	 * as it was previously derived from a real kernel symbol.
>> +	 */
>> +	page = pfn_to_page(pfn);
>> +	pte = mk_huge_pte(page, prot);
>> +
>> +	WARN_ON(!huge_pte_dirty(huge_pte_mkdirty(pte)));
>> +	WARN_ON(!huge_pte_write(huge_pte_mkwrite(huge_pte_wrprotect(pte))));
>> +	WARN_ON(huge_pte_write(huge_pte_wrprotect(huge_pte_mkwrite(pte))));
>> +
>> +#ifdef CONFIG_ARCH_WANT_GENERAL_HUGETLB
>> +	pte = pfn_pte(pfn, prot);
>> +
>> +	WARN_ON(!pte_huge(pte_mkhuge(pte)));
>> +#endif
>> +}
>> +#else
>> +static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>> +#endif
>> +
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +	pmd_t pmd;
>> +
>> +	/*
>> +	 * pmd_trans_huge() and pmd_present() must return positive
>> +	 * after MMU invalidation with pmd_mknotpresent().
>> +	 */
>> +	pmd = pfn_pmd(pfn, prot);
>> +	WARN_ON(!pmd_trans_huge(pmd_mkhuge(pmd)));
>> +
>> +#ifndef __HAVE_ARCH_PMDP_INVALIDATE
>> +	WARN_ON(!pmd_trans_huge(pmd_mknotpresent(pmd_mkhuge(pmd))));
>> +	WARN_ON(!pmd_present(pmd_mknotpresent(pmd_mkhuge(pmd))));
>> +#endif
> 
> I think we need a better comment here, because requiring pmd_trans_huge() and
> pmd_present() returning true after pmd_mknotpresent() is not straightforward.

Thats right.

> 
> According to Andrea Arcangeli’s email (https://lore.kernel.org/linux-mm/20181017020930.GN30832@redhat.com/),
> This behavior is an optimization for transparent huge page.
> pmd_trans_huge() must be true if pmd_page() returns you a valid THP to avoid
> taking the pmd_lock when others walk over non transhuge pmds (i.e. there are no
> THP allocated). Especially when we split a THP, removing the present bit from
> the pmd, pmd_trans_huge() still needs to return true. pmd_present() should
> be true whenever pmd_trans_huge() returns true.

Sure, will modify the existing comment here like this.

	/*
	 * pmd_trans_huge() and pmd_present() must return positive after
	 * MMU invalidation with pmd_mknotpresent(). This behavior is an
	 * optimization for transparent huge page. pmd_trans_huge() must
	 * be true if pmd_page() returns a valid THP to avoid taking the
	 * pmd_lock when others walk over non transhuge pmds (i.e. there
	 * are no THP allocated). Especially when splitting a THP and
	 * removing the present bit from the pmd, pmd_trans_huge() still
	 * needs to return true. pmd_present() should be true whenever
	 * pmd_trans_huge() returns true.
	 */

> 
> I think it is also worth either putting Andres’s email or the link to it
> in the rst file in your 3rd patch. It is a good documentation for this special
> case.

Makes sense. Will update Andrea's email link in the .rst file as well.

> 
> —
> Best Regards,
> Yan Zi
> 

^ permalink raw reply

* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-03-26  1:26 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Aneesh Kumar K.V, Joerg Roedel, Robin Murphy, linux-kernel, iommu,
	Greg Kroah-Hartman, linuxppc-dev, Lu Baolu
In-Reply-To: <20200325083740.GC21605@lst.de>



On 25/03/2020 19:37, Christoph Hellwig wrote:
> On Wed, Mar 25, 2020 at 03:51:36PM +1100, Alexey Kardashevskiy wrote:
>>>> This is for persistent memory which you can DMA to/from but yet it does
>>>> not appear in the system as a normal memory and therefore requires
>>>> special handling anyway (O_DIRECT or DAX, I do not know the exact
>>>> mechanics). All other devices in the system should just run as usual,
>>>> i.e. use 1:1 mapping if possible.
>>>
>>> On other systems (x86 and arm) pmem as long as it is page backed does
>>> not require any special handling.  This must be some weird way powerpc
>>> fucked up again, and I suspect you'll have to suffer from it.
>>
>>
>> It does not matter if it is backed by pages or not, the problem may also
>> appear if we wanted for example p2p PCI via IOMMU (between PHBs) and
>> MMIO might be mapped way too high in the system address space and make
>> 1:1 impossible.
> 
> How can it be mapped too high for a direct mapping with a 64-bit DMA
> mask?

The window size is limited and often it is not even sparse. It requires
an 8 byte entry per an IOMMU page (which is most commonly is 64k max) so
1TB limit (a guest RAM size) is a quite real thing. MMIO is mapped to
guest physical address space outside of this 1TB (on PPC).


-- 
Alexey

^ permalink raw reply

* Re: [PATCH] mm/sparse: Fix kernel crash with pfn_section_valid check
From: Andrew Morton @ 2020-03-26  0:38 UTC (permalink / raw)
  To: Aneesh Kumar K.V
  Cc: Sachin Sant, Pankaj Gupta, Michal Hocko, Baoquan He,
	David Hildenbrand, linux-kernel, Mike Rapoport, linux-mm,
	Wei Yang, linuxppc-dev, Oscar Salvador
In-Reply-To: <20200325031914.107660-1-aneesh.kumar@linux.ibm.com>

On Wed, 25 Mar 2020 08:49:14 +0530 "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> wrote:

> Fixes the below crash

(cc's added)

> BUG: Kernel NULL pointer dereference on read at 0x00000000
> Faulting instruction address: 0xc000000000c3447c
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> CPU: 11 PID: 7519 Comm: lt-ndctl Not tainted 5.6.0-rc7-autotest #1
> ...
> NIP [c000000000c3447c] vmemmap_populated+0x98/0xc0
> LR [c000000000088354] vmemmap_free+0x144/0x320
> Call Trace:
>  section_deactivate+0x220/0x240
>  __remove_pages+0x118/0x170
>  arch_remove_memory+0x3c/0x150
>  memunmap_pages+0x1cc/0x2f0
>  devm_action_release+0x30/0x50
>  release_nodes+0x2f8/0x3e0
>  device_release_driver_internal+0x168/0x270
>  unbind_store+0x130/0x170
>  drv_attr_store+0x44/0x60
>  sysfs_kf_write+0x68/0x80
>  kernfs_fop_write+0x100/0x290
>  __vfs_write+0x3c/0x70
>  vfs_write+0xcc/0x240
>  ksys_write+0x7c/0x140
>  system_call+0x5c/0x68
> 
> With commit: d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case")
> section_mem_map is set to NULL after depopulate_section_mem(). This
> was done so that pfn_page() can work correctly with kernel config that disables
> SPARSEMEM_VMEMMAP. With that config pfn_to_page does
> 
> 	__section_mem_map_addr(__sec) + __pfn;
> where
> 
> static inline struct page *__section_mem_map_addr(struct mem_section *section)
> {
> 	unsigned long map = section->section_mem_map;
> 	map &= SECTION_MAP_MASK;
> 	return (struct page *)map;
> }
> 
> Now with SPASEMEM_VMEMAP enabled, mem_section->usage->subsection_map is used to
> check the pfn validity (pfn_valid()). Since section_deactivate release
> mem_section->usage if a section is fully deactivated, pfn_valid() check after
> a subsection_deactivate cause a kernel crash.
> 
> static inline int pfn_valid(unsigned long pfn)
> {
> ...
> 	return early_section(ms) || pfn_section_valid(ms, pfn);
> }
> 
> where
> 
> static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
> {
> 	int idx = subsection_map_index(pfn);
> 
> 	return test_bit(idx, ms->usage->subsection_map);
> }
> 
> Avoid this by clearing SECTION_HAS_MEM_MAP when mem_section->usage is freed.
> 
> Fixes: d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case")

d41e2f3bd546 had cc:stable, so I shall add cc:stable to this one as well.

> Cc: Baoquan He <bhe@redhat.com>
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/sparse.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index aadb7298dcef..3012d1f3771a 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -781,6 +781,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
>  			ms->usage = NULL;
>  		}
>  		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
> +		/* Mark the section invalid */
> +		ms->section_mem_map &= ~SECTION_HAS_MEM_MAP;
>  	}
>  
>  	if (section_is_early && memmap)


^ permalink raw reply

* Re: hardcoded SIGSEGV in __die() ?
From: Michael Ellerman @ 2020-03-26  0:28 UTC (permalink / raw)
  To: Joakim Tjernlund, christophe.leroy@c-s.fr,
	linuxppc-dev@ozlabs.org
In-Reply-To: <4f4f2c97f7393f21f507c58def88514c9f670e0a.camel@infinera.com>

Joakim Tjernlund <Joakim.Tjernlund@infinera.com> writes:
> On Mon, 2020-03-23 at 15:45 +0100, Christophe Leroy wrote:
>> Le 23/03/2020 à 15:43, Christophe Leroy a écrit :
>> > Le 23/03/2020 à 15:17, Joakim Tjernlund a écrit :
>> > > In __die(), see below, there is this call to notify_send() with
>> > > SIGSEGV hardcoded, this seems odd
>> > > to me as the variable "err" holds the true signal(in my case SIGBUS)
>> > > Should not SIGSEGV be replaced with the true signal no.?
>> > 
>> > As far as I can see, comes from
>> > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3D66fcb1059&amp;data=02%7C01%7CJoakim.Tjernlund%40infinera.com%7C4291ac1b501e4296869a08d7cf38cdb4%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637205715189366995&amp;sdata=Z2bFsmDlD2MKhLACQvayk9ejz0dqgMEOlBTlocAmtTg%3D&amp;reserved=0
>> > 
>> 
>> And
>> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3Dae87221d3ce49d9de1e43756da834fd0bf05a2ad&amp;data=02%7C01%7CJoakim.Tjernlund%40infinera.com%7C4291ac1b501e4296869a08d7cf38cdb4%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637205715189366995&amp;sdata=97kyz3Ur88BhDUUYzya5t%2FFQVhXYu6qiHoW8hsEg81s%3D&amp;reserved=0
>> shows it is (was?) similar on x86.
>> 
>
> I tried to follow that chain thinking it would end up sending a signal to user space but I cannot see
> that happens. Seems to be related to debugging.
>
> In short, I cannot see any signal being delivered to user space. If so that would explain why
> our user space process never dies.
> Is there a signal hidden in machine_check handler for SIGBUS I cannot see?

It's platform specific. What platform are you on?

See the ppc_md & cur_cpu_spec calls here:

void machine_check_exception(struct pt_regs *regs)
{
	int recover = 0;
	bool nested = in_nmi();
	if (!nested)
		nmi_enter();

	__this_cpu_inc(irq_stat.mce_exceptions);

	add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);

	/* See if any machine dependent calls. In theory, we would want
	 * to call the CPU first, and call the ppc_md. one if the CPU
	 * one returns a positive number. However there is existing code
	 * that assumes the board gets a first chance, so let's keep it
	 * that way for now and fix things later. --BenH.
	 */
	if (ppc_md.machine_check_exception)
		recover = ppc_md.machine_check_exception(regs);
	else if (cur_cpu_spec->machine_check)
		recover = cur_cpu_spec->machine_check(regs);

	if (recover > 0)
		goto bail;


Either the ppc_md or cpu_spec handlers can send a signal, but after a
bit of grepping I think only the pseries and powernv ones do.

If you get into die() then it's an oops, which is not the same as a
normal signal.

cheers

^ permalink raw reply

* Re: hardcoded SIGSEGV in __die() ?
From: Joakim Tjernlund @ 2020-03-25 17:09 UTC (permalink / raw)
  To: christophe.leroy@c-s.fr, linuxppc-dev@ozlabs.org,
	David.Laight@ACULAB.COM
In-Reply-To: <1bec238369f24e978e0da14f79b9c55f@AcuMS.aculab.com>

On Wed, 2020-03-25 at 17:02 +0000, David Laight wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you recognize the sender
> and know the content is safe.
> 
> 
> From: Joakim Tjernlund
> > Sent: 23 March 2020 15:45
> ...
> > > > I tried to follow that chain thinking it would end up sending a
> > > > signal to user space but I cannot
> > see
> > > > that happens. Seems to be related to debugging.
> > > > 
> > > > In short, I cannot see any signal being delivered to user
> > > > space. If so that would explain why
> > > > our user space process never dies.
> > > > Is there a signal hidden in machine_check handler for SIGBUS I
> > > > cannot see?
> > > > 
> > > 
> > > Isn't it done in do_exit(), called from oops_end() ?
> > 
> > hmm, so it seems. The odd thing though is that do_exit takes an
> > exit code, not signal number.
> > Also, feels a bit odd to force an exit(that we haven't seen
> > happening) rather than just a signal.
> 
> Isn't there something 'magic' that converts EFAULT into SIGSEGV?

I have tried to find out and I cannot see a signal beeing sent.
Also, SEGV is wrong, this is a SIGBUS fault.

> 
>         David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)

^ permalink raw reply

* RE: hardcoded SIGSEGV in __die() ?
From: David Laight @ 2020-03-25 17:02 UTC (permalink / raw)
  To: 'Joakim Tjernlund', christophe.leroy@c-s.fr,
	linuxppc-dev@ozlabs.org
In-Reply-To: <6b3484a7bf0f760570fbe7c8b22c36a244c19ff6.camel@infinera.com>

From: Joakim Tjernlund
> Sent: 23 March 2020 15:45
...
> > > I tried to follow that chain thinking it would end up sending a signal to user space but I cannot
> see
> > > that happens. Seems to be related to debugging.
> > >
> > > In short, I cannot see any signal being delivered to user space. If so that would explain why
> > > our user space process never dies.
> > > Is there a signal hidden in machine_check handler for SIGBUS I cannot see?
> > >
> >
> > Isn't it done in do_exit(), called from oops_end() ?
> 
> hmm, so it seems. The odd thing though is that do_exit takes an exit code, not signal number.
> Also, feels a bit odd to force an exit(that we haven't seen happening) rather than just a signal.

Isn't there something 'magic' that converts EFAULT into SIGSEGV?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply

* [PATCH v2] Documentation/locking/locktypes: minor copy editor fixes
From: Randy Dunlap @ 2020-03-25 16:58 UTC (permalink / raw)
  To: Thomas Gleixner, paulmck
  Cc: linux-ia64, Peter Zijlstra, linux-pci, Sebastian Siewior,
	Oleg Nesterov, Guo Ren, Joel Fernandes, Vincent Chen, Ingo Molnar,
	Davidlohr Bueso, linux-acpi, Brian Cain, Jonathan Corbet,
	linux-hexagon, Rafael J. Wysocki, linux-csky, Linus Torvalds,
	Darren Hart, Zhang Rui, Len Brown, Fenghua Yu, Arnd Bergmann,
	linux-pm, linuxppc-dev, Greentime Hu, Bjorn Helgaas,
	Kurt Schwemmer, platform-driver-x86, Kalle Valo,
	kbuild test robot, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
	Geoff Levand, netdev, linux-usb, linux-wireless, LKML,
	Davidlohr Bueso, Greg Kroah-Hartman, Logan Gunthorpe,
	David S. Miller, Andy Shevchenko
In-Reply-To: <87wo78y5yy.fsf@nanos.tec.linutronix.de>

From: Randy Dunlap <rdunlap@infradead.org>

Minor editorial fixes:
- add some hyphens in multi-word adjectives
- add some periods for consistency
- add "'" for possessive CPU's
- capitalize IRQ when it's an acronym and not part of a function name

Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Paul McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 Documentation/locking/locktypes.rst |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

--- linux-next-20200325.orig/Documentation/locking/locktypes.rst
+++ linux-next-20200325/Documentation/locking/locktypes.rst
@@ -84,7 +84,7 @@ rtmutex
 
 RT-mutexes are mutexes with support for priority inheritance (PI).
 
-PI has limitations on non PREEMPT_RT enabled kernels due to preemption and
+PI has limitations on non-PREEMPT_RT-enabled kernels due to preemption and
 interrupt disabled sections.
 
 PI clearly cannot preempt preemption-disabled or interrupt-disabled
@@ -150,7 +150,7 @@ kernel configuration including PREEMPT_R
 
 raw_spinlock_t is a strict spinning lock implementation in all kernels,
 including PREEMPT_RT kernels.  Use raw_spinlock_t only in real critical
-core code, low level interrupt handling and places where disabling
+core code, low-level interrupt handling and places where disabling
 preemption or interrupts is required, for example, to safely access
 hardware state.  raw_spinlock_t can sometimes also be used when the
 critical section is tiny, thus avoiding RT-mutex overhead.
@@ -160,20 +160,20 @@ spinlock_t
 
 The semantics of spinlock_t change with the state of PREEMPT_RT.
 
-On a non PREEMPT_RT enabled kernel spinlock_t is mapped to raw_spinlock_t
+On a non-PREEMPT_RT-enabled kernel spinlock_t is mapped to raw_spinlock_t
 and has exactly the same semantics.
 
 spinlock_t and PREEMPT_RT
 -------------------------
 
-On a PREEMPT_RT enabled kernel spinlock_t is mapped to a separate
+On a PREEMPT_RT-enabled kernel spinlock_t is mapped to a separate
 implementation based on rt_mutex which changes the semantics:
 
- - Preemption is not disabled
+ - Preemption is not disabled.
 
  - The hard interrupt related suffixes for spin_lock / spin_unlock
-   operations (_irq, _irqsave / _irqrestore) do not affect the CPUs
-   interrupt disabled state
+   operations (_irq, _irqsave / _irqrestore) do not affect the CPU's
+   interrupt disabled state.
 
  - The soft interrupt related suffix (_bh()) still disables softirq
    handlers.
@@ -279,7 +279,7 @@ fully preemptible context.  Instead, use
 spin_lock_irqsave() and their unlock counterparts.  In cases where the
 interrupt disabling and locking must remain separate, PREEMPT_RT offers a
 local_lock mechanism.  Acquiring the local_lock pins the task to a CPU,
-allowing things like per-CPU irq-disabled locks to be acquired.  However,
+allowing things like per-CPU IRQ-disabled locks to be acquired.  However,
 this approach should be used only where absolutely necessary.
 
 


^ permalink raw reply

* Re: Documentation/locking/locktypes: Further clarifications and wordsmithing
From: Sebastian Siewior @ 2020-03-25 16:54 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-usb, linux-ia64, Peter Zijlstra, linux-pci, Oleg Nesterov,
	Guo Ren, Joel Fernandes, Vincent Chen, Thomas Gleixner,
	Davidlohr Bueso, linux-acpi, Brian Cain, Jonathan Corbet,
	linux-hexagon, Rafael J. Wysocki, linux-csky, Ingo Molnar,
	Linus Torvalds, Darren Hart, Zhang Rui, Len Brown, Fenghua Yu,
	Arnd Bergmann, linux-pm, linuxppc-dev, Greentime Hu,
	Bjorn Helgaas, Kurt Schwemmer, platform-driver-x86, Kalle Valo,
	kbuild test robot, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
	Geoff Levand, Greg Kroah-Hartman, Randy Dunlap, linux-wireless,
	LKML, Davidlohr Bueso, netdev, Logan Gunthorpe, David S. Miller,
	Andy Shevchenko
In-Reply-To: <20200325163919.GU19865@paulmck-ThinkPad-P72>

On 2020-03-25 09:39:19 [-0700], Paul E. McKenney wrote:
> > > --- a/Documentation/locking/locktypes.rst
> > > +++ b/Documentation/locking/locktypes.rst
> > …
> > > +rw_semaphore
> > > +============
> > > +
> > > +rw_semaphore is a multiple readers and single writer lock mechanism.
> > > +
> > > +On non-PREEMPT_RT kernels the implementation is fair, thus preventing
> > > +writer starvation.
> > > +
> > > +rw_semaphore complies by default with the strict owner semantics, but there
> > > +exist special-purpose interfaces that allow non-owner release for readers.
> > > +These work independent of the kernel configuration.
> > 
> > This reads funny, could be my English. "This works independent …" maybe?
> 
> The "These" refers to "interfaces", which is plural, so "These" rather
> than "This".  But yes, it is a bit awkward, because you have to skip
> back past "readers", "release", and "non-owner" to find the implied
> subject of that last sentence.
> 
> So how about this instead, making the implied subject explicit?
> 
> rw_semaphore complies by default with the strict owner semantics, but there
> exist special-purpose interfaces that allow non-owner release for readers.
> These interfaces work independent of the kernel configuration.

Yes, perfect. Thank you.

> 							Thanx, Paul

Sebastian

^ permalink raw reply

* [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start
From: Fangrui Song @ 2020-03-25 16:42 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Fangrui Song, Alan Modra, Nick Desaulniers, clang-built-linux,
	Joel Stanley

.globl sets the symbol binding to STB_GLOBAL while .weak sets the
binding to STB_WEAK. GNU as let .weak override .globl since binutils-gdb
5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996). Clang integrated
assembler let the last win but it may error in the future.

Since it is a convention that only one binding directive is used, just
delete .globl.

Fixes: cd197ffcf10b "[POWERPC] zImage: Cleanup and improve zImage entry point"
Fixes: ee9d21b3b358 "powerpc/boot: Ensure _zimage_start is a weak symbol"
Link: https://github.com/ClangBuiltLinux/linux/issues/937
Signed-off-by: Fangrui Song <maskray@google.com>
Cc: Alan Modra <amodra@gmail.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: clang-built-linux@googlegroups.com
---
 arch/powerpc/boot/crt0.S | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
index 92608f34d312..1d83966f5ef6 100644
--- a/arch/powerpc/boot/crt0.S
+++ b/arch/powerpc/boot/crt0.S
@@ -44,9 +44,6 @@ p_end:		.long	_end
 p_pstack:	.long	_platform_stack_top
 #endif
 
-	.globl	_zimage_start
-	/* Clang appears to require the .weak directive to be after the symbol
-	 * is defined. See https://bugs.llvm.org/show_bug.cgi?id=38921  */
 	.weak	_zimage_start
 _zimage_start:
 	.globl	_zimage_start_lib
-- 
2.25.1.696.g5e7596f4ac-goog


^ permalink raw reply related

* Re: Documentation/locking/locktypes: Further clarifications and wordsmithing
From: Paul E. McKenney @ 2020-03-25 16:39 UTC (permalink / raw)
  To: Sebastian Siewior
  Cc: linux-usb, linux-ia64, Peter Zijlstra, linux-pci, Oleg Nesterov,
	Guo Ren, Joel Fernandes, Vincent Chen, Thomas Gleixner,
	Davidlohr Bueso, linux-acpi, Brian Cain, Jonathan Corbet,
	linux-hexagon, Rafael J. Wysocki, linux-csky, Ingo Molnar,
	Linus Torvalds, Darren Hart, Zhang Rui, Len Brown, Fenghua Yu,
	Arnd Bergmann, linux-pm, linuxppc-dev, Greentime Hu,
	Bjorn Helgaas, Kurt Schwemmer, platform-driver-x86, Kalle Valo,
	kbuild test robot, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
	Geoff Levand, Greg Kroah-Hartman, Randy Dunlap, linux-wireless,
	LKML, Davidlohr Bueso, netdev, Logan Gunthorpe, David S. Miller,
	Andy Shevchenko
In-Reply-To: <20200325160212.oavrni7gmzudnczv@linutronix.de>

On Wed, Mar 25, 2020 at 05:02:12PM +0100, Sebastian Siewior wrote:
> On 2020-03-25 13:27:49 [+0100], Thomas Gleixner wrote:
> > The documentation of rw_semaphores is wrong as it claims that the non-owner
> > reader release is not supported by RT. That's just history biased memory
> > distortion.
> > 
> > Split the 'Owner semantics' section up and add separate sections for
> > semaphore and rw_semaphore to reflect reality.
> > 
> > Aside of that the following updates are done:
> > 
> >  - Add pseudo code to document the spinlock state preserving mechanism on
> >    PREEMPT_RT
> > 
> >  - Wordsmith the bitspinlock and lock nesting sections
> > 
> > Co-developed-by: Paul McKenney <paulmck@kernel.org>
> > Signed-off-by: Paul McKenney <paulmck@kernel.org>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> > --- a/Documentation/locking/locktypes.rst
> > +++ b/Documentation/locking/locktypes.rst
> …
> > +rw_semaphore
> > +============
> > +
> > +rw_semaphore is a multiple readers and single writer lock mechanism.
> > +
> > +On non-PREEMPT_RT kernels the implementation is fair, thus preventing
> > +writer starvation.
> > +
> > +rw_semaphore complies by default with the strict owner semantics, but there
> > +exist special-purpose interfaces that allow non-owner release for readers.
> > +These work independent of the kernel configuration.
> 
> This reads funny, could be my English. "This works independent …" maybe?

The "These" refers to "interfaces", which is plural, so "These" rather
than "This".  But yes, it is a bit awkward, because you have to skip
back past "readers", "release", and "non-owner" to find the implied
subject of that last sentence.

So how about this instead, making the implied subject explicit?

rw_semaphore complies by default with the strict owner semantics, but there
exist special-purpose interfaces that allow non-owner release for readers.
These interfaces work independent of the kernel configuration.

							Thanx, Paul

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox