LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3, 3/4] powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-16  7:41 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200416074141.3555-1-wenhu.wang@vivo.com>

Function instantiate_cache_sram should not be linked into the init
section for its caller mpc85xx_l2ctlr_of_probe is none-__init.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>

Warning information:
  MODPOST vmlinux.o
WARNING: modpost: vmlinux.o(.text+0x1e540): Section mismatch in reference from the function mpc85xx_l2ctlr_of_probe() to the function .init.text:instantiate_cache_sram()
The function mpc85xx_l2ctlr_of_probe() references
the function __init instantiate_cache_sram().
This is often because mpc85xx_l2ctlr_of_probe lacks a __init
annotation or the annotation of instantiate_cache_sram is wrong.
---
Changes since v1:
 * None
Changes since v2:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index be3aef4229d7..3de5ac8382c0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -68,7 +68,7 @@ void mpc85xx_cache_sram_free(void *ptr)
 }
 EXPORT_SYMBOL(mpc85xx_cache_sram_free);
 
-int __init instantiate_cache_sram(struct platform_device *dev,
+int instantiate_cache_sram(struct platform_device *dev,
 		struct sram_parameters sram_params)
 {
 	int ret = 0;
-- 
2.17.1


^ permalink raw reply related

* [PATCH v3, 2/4] powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-16  7:41 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200416074141.3555-1-wenhu.wang@vivo.com>

Include linux/io.h into fsl_85xx_cache_sram.c to fix the
implicit-declaration compile error when building Cache-Sram.

arch/powerpc/sysdev/fsl_85xx_cache_sram.c: In function ‘instantiate_cache_sram’:
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:26: error: implicit declaration of function ‘ioremap_coherent’; did you mean ‘bitmap_complement’? [-Werror=implicit-function-declaration]
  cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
                          ^~~~~~~~~~~~~~~~
                          bitmap_complement
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:24: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
  cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
                        ^
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:123:2: error: implicit declaration of function ‘iounmap’; did you mean ‘roundup’? [-Werror=implicit-function-declaration]
  iounmap(cache_sram->base_virt);
  ^~~~~~~
  roundup
cc1: all warnings being treated as errors

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: WANG Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
 * None
Changes since v2:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index f6c665dac725..be3aef4229d7 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -17,6 +17,7 @@
 #include <linux/of_platform.h>
 #include <asm/pgtable.h>
 #include <asm/fsl_85xx_cache_sram.h>
+#include <linux/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
 
-- 
2.17.1


^ permalink raw reply related

* [PATCH v3, 1/4] powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
From: Wang Wenhu @ 2020-04-16  7:41 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200416074141.3555-1-wenhu.wang@vivo.com>

Include "linux/of_address.h" to fix the compile error for
mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.

  CC      arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function ‘mpc85xx_l2ctlr_of_probe’:
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration of function ‘of_iomap’; did you mean ‘pci_iomap’? [-Werror=implicit-function-declaration]
  l2ctlr = of_iomap(dev->dev.of_node, 0);
           ^~~~~~~~
           pci_iomap
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
  l2ctlr = of_iomap(dev->dev.of_node, 0);
         ^
cc1: all warnings being treated as errors
scripts/Makefile.build:267: recipe for target 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
 * None
Changes since v2:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
index 2d0af0c517bb..7533572492f0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
+++ b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <asm/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH] KVM: PPC: Book3S HV: Handle non-present PTEs in page fault functions
From: Cédric Le Goater @ 2020-04-16  8:07 UTC (permalink / raw)
  To: Paul Mackerras, kvm, kvm-ppc
  Cc: David Gibson, linuxppc-dev, linux-kernel, groug
In-Reply-To: <20200416050335.GB10545@blackberry>

On 4/16/20 7:03 AM, Paul Mackerras wrote:
> Since cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT
> page fault handler", it's been possible in fairly rare circumstances to
> load a non-present PTE in kvmppc_book3s_hv_page_fault() when running a
> guest on a POWER8 host.
> 
> Because that case wasn't checked for, we could misinterpret the non-present
> PTE as being a cache-inhibited PTE.  That could mismatch with the
> corresponding hash PTE, which would cause the function to fail with -EFAULT
> a little further down.  That would propagate up to the KVM_RUN ioctl()
> generally causing the KVM userspace (usually qemu) to fall over.
> 
> This addresses the problem by catching that case and returning to the guest
> instead, letting it fault again, and retrying the whole page fault from
> the beginning.
> 
> For completeness, this fixes the radix page fault handler in the same
> way.  For radix this didn't cause any obvious misbehaviour, because we
> ended up putting the non-present PTE into the guest's partition-scoped
> page tables, leading immediately to another hypervisor data/instruction
> storage interrupt, which would go through the page fault path again
> and fix things up.
> 
> Fixes: cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT page fault handler"
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1820402
> Reported-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

I didn't see the reported issue with the current 5.7-rc1. Anyhow I gave
this patch a try on a P8 host and a P9 host with a radix guest and a hash 
guest (using rhel6). Passthrough is fine also.

Tested-by: Cédric Le Goater <clg@kaod.org>

The code looks correct,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 


> ---
> This is a reworked version of the patch David Gibson sent recently,
> with the fix applied to the radix case as well. The commit message
> is mostly stolen from David's patch.
> 
>  arch/powerpc/kvm/book3s_64_mmu_hv.c    | 9 +++++----
>  arch/powerpc/kvm/book3s_64_mmu_radix.c | 9 +++++----
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 3aecec8..20b7dce 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -604,18 +604,19 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  	 */
>  	local_irq_disable();
>  	ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
> +	pte = __pte(0);
> +	if (ptep)
> +		pte = *ptep;
> +	local_irq_enable();
>  	/*
>  	 * If the PTE disappeared temporarily due to a THP
>  	 * collapse, just return and let the guest try again.
>  	 */
> -	if (!ptep) {
> -		local_irq_enable();
> +	if (!pte_present(pte)) {
>  		if (page)
>  			put_page(page);
>  		return RESUME_GUEST;
>  	}
> -	pte = *ptep;
> -	local_irq_enable();
>  	hpa = pte_pfn(pte) << PAGE_SHIFT;
>  	pte_size = PAGE_SIZE;
>  	if (shift)
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> index 134fbc1..7bf94ba 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> @@ -815,18 +815,19 @@ int kvmppc_book3s_instantiate_page(struct kvm_vcpu *vcpu,
>  	 */
>  	local_irq_disable();
>  	ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
> +	pte = __pte(0);
> +	if (ptep)
> +		pte = *ptep;
> +	local_irq_enable();
>  	/*
>  	 * If the PTE disappeared temporarily due to a THP
>  	 * collapse, just return and let the guest try again.
>  	 */
> -	if (!ptep) {
> -		local_irq_enable();
> +	if (!pte_present(pte)) {
>  		if (page)
>  			put_page(page);
>  		return RESUME_GUEST;
>  	}
> -	pte = *ptep;
> -	local_irq_enable();
>  
>  	/* If we're logging dirty pages, always map single pages */
>  	large_enable = !(memslot->flags & KVM_MEM_LOG_DIRTY_PAGES);
> 


^ permalink raw reply

* [PATCH v3,0/4] drivers: uio: new driver uio_fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-16  7:41 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200415152442.122873-1-wenhu.wang@vivo.com>

This series add a new uio driver for freescale 85xx platforms to
access the Cache-Sram form user level. This is extremely helpful
for the user-space applications that require high performance memory
accesses.

It fixes the compile errors and warning of the hardware level drivers
and implements the uio driver in uio_fsl_85xx_cache_sram.c.

Changes since v1:
 * Addressed comments from Greg K-H
 * Moved kfree(info->name) into uio_info_free_internal()

Changes since v2:
 * Drop the patch that modifies Kconfigs of arch/powerpc/platforms
   and modified the sequence of patches:
    01:dropped, 02->03, 03->02, 04->01, 05->04
 * Addressed comments from Greg, Scott and Christophe
 * Use "uiomem->internal_addr" as if condition for sram memory free,
   and memset the uiomem entry
 * Modified of_match_table make the driver apart from Cache-Sram HW info
   which belong to the HW level driver fsl_85xx_cache_sram to match
 * Use roundup_pow_of_two for align calc(really learned a lot from Christophe)
 * Remove useless clear block of uiomem entries.
 * Use UIO_INFO_VER micro for info->version, and define it as
   "devicetree,pseudo", meaning this is pseudo device and probed from
   device tree configuration
 * Select FSL_85XX_CACHE_SRAM rather than depends on it

Wang Wenhu (4):
  powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
  powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
  powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
  drivers: uio: new driver for fsl_85xx_cache_sram

 arch/powerpc/sysdev/fsl_85xx_cache_sram.c |   3 +-
 arch/powerpc/sysdev/fsl_85xx_l2ctlr.c     |   1 +
 drivers/uio/Kconfig                       |   9 ++
 drivers/uio/Makefile                      |   1 +
 drivers/uio/uio_fsl_85xx_cache_sram.c     | 158 ++++++++++++++++++++++
 5 files changed, 171 insertions(+), 1 deletion(-)
 create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c

-- 
2.17.1


^ permalink raw reply

* [PATCH v3, 3/4] powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-16  7:29 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200416072929.3489-1-wenhu.wang@vivo.com>

Function instantiate_cache_sram should not be linked into the init
section for its caller mpc85xx_l2ctlr_of_probe is none-__init.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>

Warning information:
  MODPOST vmlinux.o
WARNING: modpost: vmlinux.o(.text+0x1e540): Section mismatch in reference from the function mpc85xx_l2ctlr_of_probe() to the function .init.text:instantiate_cache_sram()
The function mpc85xx_l2ctlr_of_probe() references
the function __init instantiate_cache_sram().
This is often because mpc85xx_l2ctlr_of_probe lacks a __init
annotation or the annotation of instantiate_cache_sram is wrong.
---
Changes since v1:
 * None
Changes since v2:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index be3aef4229d7..3de5ac8382c0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -68,7 +68,7 @@ void mpc85xx_cache_sram_free(void *ptr)
 }
 EXPORT_SYMBOL(mpc85xx_cache_sram_free);
 
-int __init instantiate_cache_sram(struct platform_device *dev,
+int instantiate_cache_sram(struct platform_device *dev,
 		struct sram_parameters sram_params)
 {
 	int ret = 0;
-- 
2.17.1


^ permalink raw reply related

* [PATCH v3,0/4] drivers: uio: new driver uio_fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-16  7:29 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200415152442.122873-1-wenhu.wang@vivo.com>

This series add a new uio driver for freescale 85xx platforms to
access the Cache-Sram form user level. This is extremely helpful
for the user-space applications that require high performance memory
accesses.

It fixes the compile errors and warning of the hardware level drivers
and implements the uio driver in uio_fsl_85xx_cache_sram.c.

Changes since v1:
 * Addressed comments from Greg K-H
 * Moved kfree(info->name) into uio_info_free_internal()

Changes since v2:
 * Drop the patch that modifies Kconfigs of arch/powerpc/platforms
   and modified the sequence of patches:
    01:dropped, 02->03, 03->02, 04->01, 05->04
 * Addressed comments from Greg, Scott and Christophe
 * Use "uiomem->internal_addr" as if condition for sram memory free,
   and memset the uiomem entry
 * Modified of_match_table make the driver apart from Cache-Sram HW info
   which belong to the HW level driver fsl_85xx_cache_sram to match
 * Use roundup_pow_of_two for align calc(really learned a lot from Christophe)
 * Remove useless clear block of uiomem entries.
 * Use UIO_INFO_VER micro for info->version, and define it as
   "devicetree,pseudo", meaning this is pseudo device and probed from
   device tree configuration
 * Select FSL_85XX_CACHE_SRAM rather than depends on it

Wang Wenhu (4):
  powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
  powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
  powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
  drivers: uio: new driver for fsl_85xx_cache_sram

 arch/powerpc/sysdev/fsl_85xx_cache_sram.c |   3 +-
 arch/powerpc/sysdev/fsl_85xx_l2ctlr.c     |   1 +
 drivers/uio/Kconfig                       |   9 ++
 drivers/uio/Makefile                      |   1 +
 drivers/uio/uio_fsl_85xx_cache_sram.c     | 158 ++++++++++++++++++++++
 5 files changed, 171 insertions(+), 1 deletion(-)
 create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c

-- 
2.17.1


^ permalink raw reply

* [PATCH v3, 2/4] powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-16  7:29 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200416072929.3489-1-wenhu.wang@vivo.com>

Include linux/io.h into fsl_85xx_cache_sram.c to fix the
implicit-declaration compile error when building Cache-Sram.

arch/powerpc/sysdev/fsl_85xx_cache_sram.c: In function ‘instantiate_cache_sram’:
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:26: error: implicit declaration of function ‘ioremap_coherent’; did you mean ‘bitmap_complement’? [-Werror=implicit-function-declaration]
  cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
                          ^~~~~~~~~~~~~~~~
                          bitmap_complement
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:24: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
  cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
                        ^
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:123:2: error: implicit declaration of function ‘iounmap’; did you mean ‘roundup’? [-Werror=implicit-function-declaration]
  iounmap(cache_sram->base_virt);
  ^~~~~~~
  roundup
cc1: all warnings being treated as errors

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: WANG Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
 * None
Changes since v2:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index f6c665dac725..be3aef4229d7 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -17,6 +17,7 @@
 #include <linux/of_platform.h>
 #include <asm/pgtable.h>
 #include <asm/fsl_85xx_cache_sram.h>
+#include <linux/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
 
-- 
2.17.1


^ permalink raw reply related

* [PATCH v3, 1/4] powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
From: Wang Wenhu @ 2020-04-16  7:29 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200416072929.3489-1-wenhu.wang@vivo.com>

Include "linux/of_address.h" to fix the compile error for
mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.

  CC      arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function ‘mpc85xx_l2ctlr_of_probe’:
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration of function ‘of_iomap’; did you mean ‘pci_iomap’? [-Werror=implicit-function-declaration]
  l2ctlr = of_iomap(dev->dev.of_node, 0);
           ^~~~~~~~
           pci_iomap
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
  l2ctlr = of_iomap(dev->dev.of_node, 0);
         ^
cc1: all warnings being treated as errors
scripts/Makefile.build:267: recipe for target 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
 * None
Changes since v2:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
index 2d0af0c517bb..7533572492f0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
+++ b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <asm/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
-- 
2.17.1


^ permalink raw reply related

* [PATCH v3,4/4] drivers: uio: new driver for fsl_85xx_cache_sram
From: Wang Wenhu @ 2020-04-16  7:29 UTC (permalink / raw)
  To: gregkh, linux-kernel, oss, christophe.leroy, linuxppc-dev
  Cc: kernel, Wang Wenhu
In-Reply-To: <20200416072929.3489-1-wenhu.wang@vivo.com>

A driver for freescale 85xx platforms to access the Cache-Sram form
user level. This is extremely helpful for some user-space applications
that require high performance memory accesses.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
 * Addressed comments from Greg K-H
 * Moved kfree(info->name) into uio_info_free_internal()
Changes since v2:
 * Addressed comments from Greg, Scott and Christophe
 * Use "uiomem->internal_addr" as if condition for sram memory free,
   and memset the uiomem entry
 * Modified of_match_table make the driver apart from Cache-Sram HW info
   which belong to the HW level driver fsl_85xx_cache_sram to match
 * Use roundup_pow_of_two for align calculation
 * Remove useless clear block of uiomem entries.
 * Use UIO_INFO_VER micro for info->version, and define it as
   "devicetree,pseudo", meaning this is pseudo device and probed from
   device tree configuration
 * Select FSL_85XX_CACHE_SRAM rather than depends on it
---
 drivers/uio/Kconfig                   |   9 ++
 drivers/uio/Makefile                  |   1 +
 drivers/uio/uio_fsl_85xx_cache_sram.c | 158 ++++++++++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 202ee81cfc2b..9c3b47461b71 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -105,6 +105,15 @@ config UIO_NETX
 	  To compile this driver as a module, choose M here; the module
 	  will be called uio_netx.
 
+config UIO_FSL_85XX_CACHE_SRAM
+	tristate "Freescale 85xx Cache-Sram driver"
+	depends on FSL_SOC_BOOKE && PPC32
+	select FSL_85XX_CACHE_SRAM
+	help
+	  Generic driver for accessing the Cache-Sram form user level. This
+	  is extremely helpful for some user-space applications that require
+	  high performance memory accesses.
+
 config UIO_FSL_ELBC_GPCM
 	tristate "eLBC/GPCM driver"
 	depends on FSL_LBC
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index c285dd2a4539..be2056cffc21 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
 obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
 obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
 obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
+obj-$(CONFIG_UIO_FSL_85XX_CACHE_SRAM)	+= uio_fsl_85xx_cache_sram.o
 obj-$(CONFIG_UIO_HV_GENERIC)	+= uio_hv_generic.o
diff --git a/drivers/uio/uio_fsl_85xx_cache_sram.c b/drivers/uio/uio_fsl_85xx_cache_sram.c
new file mode 100644
index 000000000000..8701df695307
--- /dev/null
+++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
+ * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
+ * All rights reserved.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/stringify.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <asm/fsl_85xx_cache_sram.h>
+
+#define DRIVER_NAME	"uio_fsl_85xx_cache_sram"
+#define UIO_INFO_VER	"devicetree,pseudo"
+#define UIO_NAME	"uio_cache_sram"
+
+static void uio_info_free_internal(struct uio_info *info)
+{
+	struct uio_mem *uiomem = info->mem;
+
+	while (uiomem < &info->mem[MAX_UIO_MAPS]) {
+		if (uiomem->internal_addr) {
+			mpc85xx_cache_sram_free(uiomem->internal_addr);
+			kfree(uiomem->name);
+			memset(uiomem, 0, sizeof(*uiomem));
+		}
+		uiomem++;
+	}
+
+	kfree(info->name);
+}
+
+static int uio_fsl_85xx_cache_sram_probe(struct platform_device *pdev)
+{
+	struct device_node *parent = pdev->dev.of_node;
+	struct device_node *node = NULL;
+	struct uio_info *info;
+	struct uio_mem *uiomem;
+	const char *dt_name;
+	u32 mem_size;
+	int ret;
+
+	/* alloc uio_info for one device */
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	/* get optional uio name */
+	if (of_property_read_string(parent, "uio_name", &dt_name))
+		dt_name = UIO_NAME;
+
+	info->name = kstrdup(dt_name, GFP_KERNEL);
+	if (!info->name) {
+		ret = -ENOMEM;
+		goto err_info_free;
+	}
+
+	uiomem = info->mem;
+	for_each_child_of_node(parent, node) {
+		void *virt;
+		phys_addr_t phys;
+
+		ret = of_property_read_u32(node, "cache-mem-size", &mem_size);
+		if (ret) {
+			ret = -EINVAL;
+			goto err_info_free_internal;
+		}
+
+		if (mem_size == 0) {
+			dev_err(&pdev->dev, "cache-mem-size should not be 0\n");
+			ret = -EINVAL;
+			goto err_info_free_internal;
+		}
+
+		virt = mpc85xx_cache_sram_alloc(mem_size,
+						&phys,
+						roundup_pow_of_two(mem_size));
+		if (!virt) {
+			/* mpc85xx_cache_sram_alloc to define the real cause */
+			ret = -ENOMEM;
+			goto err_info_free_internal;
+		}
+
+		uiomem->memtype = UIO_MEM_PHYS;
+		uiomem->addr = phys;
+		uiomem->size = mem_size;
+		uiomem->name = kstrdup(node->name, GFP_KERNEL);;
+		uiomem->internal_addr = virt;
+		uiomem++;
+
+		if (uiomem >= &info->mem[MAX_UIO_MAPS]) {
+			dev_warn(&pdev->dev, "more than %d uio-maps for device.\n",
+				 MAX_UIO_MAPS);
+			break;
+		}
+	}
+
+	if (uiomem == info->mem) {
+		dev_err(&pdev->dev, "error no valid uio-map configured\n");
+		ret = -EINVAL;
+		goto err_info_free_internal;
+	}
+
+	info->version = UIO_INFO_VER;
+
+	/* register uio device */
+	if (uio_register_device(&pdev->dev, info)) {
+		dev_err(&pdev->dev, "uio registration failed\n");
+		ret = -ENODEV;
+		goto err_info_free_internal;
+	}
+
+	platform_set_drvdata(pdev, info);
+
+	return 0;
+err_info_free_internal:
+	uio_info_free_internal(info);
+err_info_free:
+	kfree(info);
+	return ret;
+}
+
+static int uio_fsl_85xx_cache_sram_remove(struct platform_device *pdev)
+{
+	struct uio_info *info = platform_get_drvdata(pdev);
+
+	uio_unregister_device(info);
+
+	uio_info_free_internal(info);
+
+	kfree(info);
+
+	return 0;
+}
+
+static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
+	{	.compatible = "uio,mpc85xx-cache-sram",	},
+	{},
+};
+
+static struct platform_driver uio_fsl_85xx_cache_sram = {
+	.probe = uio_fsl_85xx_cache_sram_probe,
+	.remove = uio_fsl_85xx_cache_sram_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table	= uio_mpc85xx_l2ctlr_of_match,
+	},
+};
+
+module_platform_driver(uio_fsl_85xx_cache_sram);
+
+MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
+MODULE_DESCRIPTION("Freescale MPC85xx Cache-Sram UIO Platform Driver");
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH 0/8] Simplefs: group and simplify linux fs code
From: Luis Chamberlain @ 2020-04-16  6:59 UTC (permalink / raw)
  To: Emanuele Giuseppe Esposito
  Cc: Song Liu, linux-usb, bpf, Rafael J. Wysocki, David Airlie,
	Heiko Carstens, Alexei Starovoitov, dri-devel, J. Bruce Fields,
	Joseph Qi, Hugh Dickins, Paul Mackerras, John Johansen, netdev,
	linux-s390, ocfs2-devel, Christoph Hellwig, Andrew Donnellan,
	Matthew Garrett, linux-efi, Arnd Bergmann, Daniel Borkmann,
	Christian Borntraeger, linux-rdma, Mark Fasheh, Anton Vorontsov,
	John Fastabend, James Morris, Ard Biesheuvel, Jason Gunthorpe,
	Doug Ledford, oprofile-list, Yonghong Song, Ian Kent,
	Andrii Nakryiko, Alexey Dobriyan, Serge E. Hallyn, Robert Richter,
	Thomas Zimmermann, Vasily Gorbik, Tony Luck, Kees Cook,
	James E.J. Bottomley, autofs, Maarten Lankhorst, Mike Marciniszyn,
	Maxime Ripard, linux-fsdevel, Manoj N. Kumar, Uma Krishnan,
	Jakub Kicinski, KP Singh, Trond Myklebust, Matthew R. Ochs,
	David S. Miller, Felipe Balbi, linux-nfs, Iurii Zaikin,
	linux-scsi, Martin K. Petersen, linux-mm, Greg Kroah-Hartman,
	Dennis Dalessandro, Miklos Szeredi, linux-kernel, Anna Schumaker,
	linux-security-module, Chuck Lever, Jeremy Kerr, Daniel Vetter,
	Colin Cross, Frederic Barrat, Paolo Bonzini, Andrew Morton,
	Mike Kravetz, linuxppc-dev, Martin KaFai Lau, Joel Becker,
	Alexander Viro
In-Reply-To: <20200414124304.4470-1-eesposit@redhat.com>

On Tue, Apr 14, 2020 at 02:42:54PM +0200, Emanuele Giuseppe Esposito wrote:
> This series of patches introduce wrappers for functions,
> arguments simplification in functions calls and most importantly
> groups duplicated code in a single header, simplefs, to avoid redundancy
> in the linux fs, especially debugfs and tracefs.

The general goal seems worthy, but here I don't see explained why hasn't
this gone through libfs, and what the intention was long term. For
instance, you added some other generalizations which you have found. It
was not clear that this was the goal here, to expand on these paths.

What if common code on fs is found which are not part of debugfs and
tracefs, how does one decide if to move to libfs or simplefs?

  Luis

^ permalink raw reply

* Re: [PATCH 2/8] fs: extract simple_pin/release_fs to separate files
From: Luis Chamberlain @ 2020-04-16  6:52 UTC (permalink / raw)
  To: Emanuele Giuseppe Esposito
  Cc: Song Liu, linux-usb, bpf, Rafael J. Wysocki, David Airlie,
	Heiko Carstens, Alexei Starovoitov, dri-devel, J. Bruce Fields,
	Joseph Qi, Hugh Dickins, Paul Mackerras, John Johansen, netdev,
	linux-s390, ocfs2-devel, Christoph Hellwig, Andrew Donnellan,
	Matthew Garrett, linux-efi, Arnd Bergmann, Daniel Borkmann,
	Christian Borntraeger, linux-rdma, Mark Fasheh, Anton Vorontsov,
	John Fastabend, James Morris, Ard Biesheuvel, Jason Gunthorpe,
	Doug Ledford, oprofile-list, Yonghong Song, Ian Kent,
	Andrii Nakryiko, Alexey Dobriyan, Serge E. Hallyn, Robert Richter,
	Thomas Zimmermann, Vasily Gorbik, Tony Luck, Kees Cook,
	James E.J. Bottomley, autofs, Maarten Lankhorst, Mike Marciniszyn,
	Maxime Ripard, linux-fsdevel, Manoj N. Kumar, Uma Krishnan,
	Jakub Kicinski, KP Singh, Trond Myklebust, Matthew R. Ochs,
	David S. Miller, Felipe Balbi, linux-nfs, Iurii Zaikin,
	linux-scsi, Martin K. Petersen, linux-mm, Greg Kroah-Hartman,
	Dennis Dalessandro, Miklos Szeredi, linux-kernel, Anna Schumaker,
	linux-security-module, Chuck Lever, Jeremy Kerr, Daniel Vetter,
	Colin Cross, Frederic Barrat, Paolo Bonzini, Andrew Morton,
	Mike Kravetz, linuxppc-dev, Martin KaFai Lau, Joel Becker,
	Alexander Viro
In-Reply-To: <20200414124304.4470-3-eesposit@redhat.com>

On Tue, Apr 14, 2020 at 02:42:56PM +0200, Emanuele Giuseppe Esposito wrote:
> We will augment this family of functions with inode management.  To avoid
> littering include/linux/fs.h and fs/libfs.c, move them to a separate header,
> with a Kconfig symbol to enable them.

If there are no functional changes, indicating that on the commit log
will make the review much easier.

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d1398cef3b18..fc38a6f0fc11 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -288,12 +288,16 @@ config STRIP_ASM_SYMS
>  
>  config READABLE_ASM
>  	bool "Generate readable assembler code"
> -	depends on DEBUG_KERNEL
> -	help
> -	  Disable some compiler optimizations that tend to generate human unreadable
> -	  assembler output. This may make the kernel slightly slower, but it helps
> -	  to keep kernel developers who have to stare a lot at assembler listings
> -	  sane.
> +    depends on DEBUG_KERNEL
> +    help
> +      Disable some compiler optimizations that tend to generate human unreadable
> +      assembler output. This may make the kernel slightly slower, but it helps
> +      to keep kernel developers who have to stare a lot at assembler listings
> +      sane.
> +	  

This minor change above should just be a separate patch. Its just noise
otherwise.

> +config DEBUG_FS
> +	bool "Debug Filesystem"
> +	select SIMPLEFS

I'm at a loss reviewing this,  my lib/Kconfig.debug already has a config
DEBUG_FS.  But above I see it is being added for the very first time.
I'm sure there is some odd conditional which is obscuring this, can
this be explained in the commit log?

  Luis

^ permalink raw reply

* Re: [PATCH 1/8] apparmor: just use vfs_kern_mount to make .null
From: Luis Chamberlain @ 2020-04-16  6:44 UTC (permalink / raw)
  To: Emanuele Giuseppe Esposito, Goldwyn Rodrigues
  Cc: Song Liu, linux-usb, bpf, Rafael J. Wysocki, David Airlie,
	Heiko Carstens, Alexei Starovoitov, dri-devel, J. Bruce Fields,
	Joseph Qi, Hugh Dickins, Paul Mackerras, John Johansen, netdev,
	linux-s390, ocfs2-devel, Christoph Hellwig, Andrew Donnellan,
	Matthew Garrett, linux-efi, Arnd Bergmann, Daniel Borkmann,
	Christian Borntraeger, linux-rdma, Mark Fasheh, Anton Vorontsov,
	John Fastabend, James Morris, Ard Biesheuvel, Jason Gunthorpe,
	Doug Ledford, oprofile-list, Yonghong Song, Ian Kent,
	Andrii Nakryiko, Alexey Dobriyan, Serge E. Hallyn, Robert Richter,
	Thomas Zimmermann, Vasily Gorbik, Tony Luck, Kees Cook,
	James E.J. Bottomley, autofs, Maarten Lankhorst, Mike Marciniszyn,
	Maxime Ripard, linux-fsdevel, Manoj N. Kumar, Uma Krishnan,
	Jakub Kicinski, KP Singh, Trond Myklebust, Matthew R. Ochs,
	David S. Miller, Felipe Balbi, linux-nfs, Iurii Zaikin,
	linux-scsi, Martin K. Petersen, linux-mm, Greg Kroah-Hartman,
	Dennis Dalessandro, Miklos Szeredi, linux-kernel, Anna Schumaker,
	linux-security-module, Chuck Lever, Jeremy Kerr, Daniel Vetter,
	Colin Cross, Frederic Barrat, Paolo Bonzini, Andrew Morton,
	Mike Kravetz, linuxppc-dev, Martin KaFai Lau, Joel Becker,
	Alexander Viro
In-Reply-To: <20200414124304.4470-2-eesposit@redhat.com>

On Tue, Apr 14, 2020 at 02:42:55PM +0200, Emanuele Giuseppe Esposito wrote:
> aa_mk_null_file is using simple_pin_fs/simple_release_fs with local
> variables as arguments, for what would amount to a simple
> vfs_kern_mount/mntput pair if everything was inlined.  Just use
> the normal filesystem API since the reference counting is not needed
> here.

*Why* is refcounting not needed here?

   Luis

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  security/apparmor/apparmorfs.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 280741fc0f5f..828bb1eb77ea 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -2525,14 +2525,14 @@ struct path aa_null;
>  
>  static int aa_mk_null_file(struct dentry *parent)
>  {
> -	struct vfsmount *mount = NULL;
> +	struct file_system_type *type = parent->d_sb->s_type;
> +	struct vfsmount *mount;
>  	struct dentry *dentry;
>  	struct inode *inode;
> -	int count = 0;
> -	int error = simple_pin_fs(parent->d_sb->s_type, &mount, &count);
>  
> -	if (error)
> -		return error;
> +	mount = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL);
> +	if (IS_ERR(mount))
> +		return PTR_ERR(mount);
>  
>  	inode_lock(d_inode(parent));
>  	dentry = lookup_one_len(NULL_FILE_NAME, parent, strlen(NULL_FILE_NAME));
> @@ -2561,7 +2561,7 @@ static int aa_mk_null_file(struct dentry *parent)
>  	dput(dentry);
>  out:
>  	inode_unlock(d_inode(parent));
> -	simple_release_fs(&mount, &count);
> +	mntput(mount);
>  	return error;
>  }
>  
> -- 
> 2.25.2
> 

^ permalink raw reply

* [PATCH v7 3/3] powerpc/powernv: Preference optimization for SPRs with constant values
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat
In-Reply-To: <20200416075358.75155-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 fdcb18a8a05b..daa2f920bd05 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 @@ DEFINE_STATIC_KEY_FALSE(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:
-					static_branch_enable(&is_lpcr_self_save);
+			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:
+						static_branch_enable(&is_lpcr_self_save);
+						break;
+					case SPRN_PTCR:
+						static_branch_enable(&is_ptcr_self_save);
+						break;
+					}
 					break;
-				case SPRN_PTCR:
-					static_branch_enable(&is_ptcr_self_save);
+				} 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 v7 2/3] powerpc/powernv: Introduce support and parsing for self-save API
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat
In-Reply-To: <20200416075358.75155-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 prefers 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 have the 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         | 135 +++++++++++++++++-
 arch/powerpc/platforms/powernv/opal-call.c    |   1 +
 5 files changed, 150 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 1dffa3cb16ba..7ba698369083 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..fdcb18a8a05b 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 */
+DEFINE_STATIC_KEY_FALSE(is_lpcr_self_save);
+DEFINE_STATIC_KEY_FALSE(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:
+					static_branch_enable(&is_lpcr_self_save);
+					break;
+				case SPRN_PTCR:
+					static_branch_enable(&is_ptcr_self_save);
+					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 (!static_branch_unlikely(&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 (!static_branch_unlikely(&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 (!static_branch_unlikely(&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 (!static_branch_unlikely(&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 (!static_branch_unlikely(&is_lpcr_self_save))
+			opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
+	}
 }
 
 /*
@@ -1316,6 +1359,81 @@ 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
+ * @np: /ibm,opal/power-mgt/self-save or
+ *      /ibm,opal/power-mgt/self-restore device node
+ * @support: Activation bit for each SPR to define support for the save-restore
+ *           mode
+ */
+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 +1582,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 v7 1/3] powerpc/powernv: Introduce interface for self-restore support
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat
In-Reply-To: <20200416075358.75155-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>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.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 v7 0/3] powerpc/powernv: Introduce interface for self-restore support
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat

v6: https://lkml.org/lkml/2020/3/26/99
Changelog
v6-->v7
Based on comments from Gautham Shenoy
1. Using static keys instead of booleans to cache support
2. extract_save_restore_state_dt device tree parser function documented

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         | 389 +++++++++++++++---
 arch/powerpc/platforms/powernv/opal-call.c    |   1 +
 5 files changed, 355 insertions(+), 57 deletions(-)

-- 
2.17.1


^ permalink raw reply

* [PATCH v7 4/4] Advertise the self-save and self-restore attributes in the device tree
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat
In-Reply-To: <20200416075341.75268-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>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 .../ibm,opal/power-mgt/self-restore.rst       |  27 ++++
 .../ibm,opal/power-mgt/self-save.rst          |  27 ++++
 hw/slw.c                                      | 118 ++++++++++++++++++
 include/skiboot.h                             |   1 +
 4 files changed, 173 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 9751c04f..fb14bd0c 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,121 @@ 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("SLW: dt node /ibm,opal/power-mgt not found\n");
+		return;
+	}
+
+	self_restore = dt_new(power_mgt, "self-restore");
+	if (!self_restore) {
+		prerror("SLW: 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) {
+		prlog(PR_INFO, "SLW: self-save not supported on this platform");
+		return;
+	}
+
+	self_save = dt_new(power_mgt, "self-save");
+	if (!self_save) {
+		prerror("SLW: 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 +1679,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.25.1


^ permalink raw reply related

* [PATCH v7 3/4] API to verify the STOP API and image compatibility
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat
In-Reply-To: <20200416075341.75268-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.25.1


^ permalink raw reply related

* [PATCH v7 1/4] Self Save: Introducing Support for SPR Self Save
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat
In-Reply-To: <20200416075341.75268-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.25.1


^ permalink raw reply related

* [PATCH v7 2/4] Self save API integration
From: Pratik Rajesh Sampat @ 2020-04-16  7:53 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, mpe, skiboot, oohall, ego, linuxram,
	pratik.r.sampat, psampat
In-Reply-To: <20200416075341.75268-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>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.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                    | 48 +++++++++++
 hw/slw.c                                    | 89 +++++++++++++++++++++
 include/opal-api.h                          |  3 +-
 include/p9_stop_api.H                       | 17 ++++
 include/skiboot.h                           |  3 +
 7 files changed, 213 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..d6bd5358 100644
--- a/doc/power-management.rst
+++ b/doc/power-management.rst
@@ -15,3 +15,51 @@ 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.
+Also in a case where SPR does not change across the lifetime
+self-restore is more efficient as when the value is same the memeory location
+is not updated.
+
+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..9751c04f 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: Self-save feature unsupported\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.25.1


^ permalink raw reply related

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

v6: https://lists.ozlabs.org/pipermail/skiboot/2020-March/016645.html
Changelog
v6 --> v7
1. Addressed comments from Gautham for reporting warnings and errors

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 when a CPU is woken up in stop4/5 and
after the task is done, the HCODE puts it back to stop. The value of
PSSCR is passed to the HCODE via the self-restore API. The kernel
currently provides the value of the deepest stop state due to being
conservative. Thus if a core that was in stop4 was woken up due to
special wakeup, the HCODE will now put it back to stop5 thus increasing
the subsequent wakeup latency to ~200us.
A mechanism is needed in place to update the PSSCR value each time the
core is woken up due to special wakeup.

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                      |  48 +
 hw/slw.c                                      | 207 ++++
 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, 1214 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.25.1


^ permalink raw reply

* Re: [EXTERNAL] [PATCH] target/ppc: Fix mtmsr(d) L=1 variant that loses interrupts
From: Nicholas Piggin @ 2020-04-16  7:53 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-devel
  Cc: qemu-stable, qemu-ppc, Nathan Chancellor, linuxppc-dev,
	David Gibson
In-Reply-To: <74e47708-fcc0-d3db-5f6b-2a513722fef9@kaod.org>

Excerpts from Cédric Le Goater's message of April 15, 2020 4:49 pm:
> On 4/14/20 1:11 PM, Nicholas Piggin wrote:
>> 
>> The confusion arises from L=0 being "context synchronizing" whereas L=1
>> is "execution synchronizing", which is a weaker semantic. However this
>> is not a relaxation of the requirement that these exceptions cause
>> interrupts when MSR[EE]=1 (e.g., when mtmsr executes to completion as
>> TCG is doing here), rather it specifies how a pipelined processor can
>> have multiple instructions in flight where one may influence how another
>> behaves.
> 
> I was expecting more changes but this looks fine. 

It _seems_ to just be these, from what I could see, but could quite 
easily be other issues I missed.

There is at least one other "funny" thing with this synchronization, 
which is the TLB flushing. I don't think it has a bug, but comments
are a bit suspect. tlbie/l doesn't have anything to do with context
/ execution synchronization, so it's a bit interesting to check them
in isync and rfi etc.

ptesync is required because the page table walkers are not necessarily 
coherent with the main CPU's memory pipeline, so you store a new value 
to a PTE then do a tlbiel, you can't have the MMU reload the TLB with 
the old PTE because the store was sitting in the store queue that 
doesn't forward to the table walker. This condition can persist after
the store instruction itself has completed so no amount of this
context synchronization would help.

It does kind of make sense to check the tlb flush in rfi, so you catch 
stray ones that didn't have the right ptesync/tlbsync, but it would 
almost be a condition you could catch and add a warn for. isync doesn't
make a lot of sense though, as far as I can see.

Thanks,
Nick

> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Sorry I always get your email wrong, phantom address book entry.


^ permalink raw reply

* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function
From: Vitaly Kuznetsov @ 2020-04-16  7:03 UTC (permalink / raw)
  To: Tianjia Zhang
  Cc: christoffer.dall, wanpengli, kvm, david, heiko.carstens, peterx,
	linux-kernel, hpa, kvmarm, linux-s390, frankja, joro, x86,
	borntraeger, mingo, julien.thierry.kdev, thuth, gor,
	suzuki.poulose, kvm-ppc, bp, tglx, linux-arm-kernel, jmattson,
	tsbogend, tianjia.zhang, cohuck, linux-mips,
	sean.j.christopherson, james.morse, maz, pbonzini, linuxppc-dev
In-Reply-To: <20200416051057.26526-1-tianjia.zhang@linux.alibaba.com>

Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes:

> In earlier versions of kvm, 'kvm_run' is an independent structure
> and is not included in the vcpu structure. At present, 'kvm_run'
> is already included in the vcpu structure, so the parameter
> 'kvm_run' is redundant.
>
> This patch simplify the function definition, removes the extra
> 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure
> if necessary.
>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> ---
>
> v2 change:
>   remove 'kvm_run' parameter and extract it from 'kvm_vcpu'
>
>  arch/mips/kvm/mips.c       |  3 ++-
>  arch/powerpc/kvm/powerpc.c |  3 ++-
>  arch/s390/kvm/kvm-s390.c   |  3 ++-
>  arch/x86/kvm/x86.c         | 11 ++++++-----
>  include/linux/kvm_host.h   |  2 +-
>  virt/kvm/arm/arm.c         |  6 +++---
>  virt/kvm/kvm_main.c        |  2 +-
>  7 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 8f05dd0a0f4e..ec24adf4857e 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -439,8 +439,9 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  	return -ENOIOCTLCMD;
>  }
>  
> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_run *run = vcpu->run;
>  	int r = -EINTR;
>  
>  	vcpu_load(vcpu);
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index e15166b0a16d..7e24691e138a 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1764,8 +1764,9 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
>  	return r;
>  }
>  
> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_run *run = vcpu->run;
>  	int r;
>  
>  	vcpu_load(vcpu);
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 19a81024fe16..443af3ead739 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4333,8 +4333,9 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		store_regs_fmt2(vcpu, kvm_run);
>  }
>  
> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_run *kvm_run = vcpu->run;
>  	int rc;
>  
>  	if (kvm_run->immediate_exit)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3bf2ecafd027..a0338e86c90f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8707,8 +8707,9 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
>  	trace_kvm_fpu(0);
>  }
>  
> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_run *kvm_run = vcpu->run;
>  	int r;
>  
>  	vcpu_load(vcpu);
> @@ -8726,18 +8727,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		r = -EAGAIN;
>  		if (signal_pending(current)) {
>  			r = -EINTR;
> -			vcpu->run->exit_reason = KVM_EXIT_INTR;
> +			kvm_run->exit_reason = KVM_EXIT_INTR;
>  			++vcpu->stat.signal_exits;
>  		}
>  		goto out;
>  	}
>  
> -	if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) {
> +	if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) {
>  		r = -EINVAL;
>  		goto out;
>  	}
>  
> -	if (vcpu->run->kvm_dirty_regs) {
> +	if (kvm_run->kvm_dirty_regs) {
>  		r = sync_regs(vcpu);
>  		if (r != 0)
>  			goto out;
> @@ -8767,7 +8768,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  
>  out:
>  	kvm_put_guest_fpu(vcpu);
> -	if (vcpu->run->kvm_valid_regs)
> +	if (kvm_run->kvm_valid_regs)
>  		store_regs(vcpu);
>  	post_kvm_run_save(vcpu);
>  	kvm_sigset_deactivate(vcpu);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6d58beb65454..1e17ef719595 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -866,7 +866,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  				    struct kvm_mp_state *mp_state);
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg);
> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu);
>  
>  int kvm_arch_init(void *opaque);
>  void kvm_arch_exit(void);
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 48d0ec44ad77..f5390ac2165b 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -639,7 +639,6 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  /**
>   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
>   * @vcpu:	The VCPU pointer
> - * @run:	The kvm_run structure pointer used for userspace state exchange
>   *
>   * This function is called through the VCPU_RUN ioctl called from user space. It
>   * will execute VM code in a loop until the time slice for the process is used
> @@ -647,8 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>   * return with return value 0 and with the kvm_run structure filled in with the
>   * required data for the requested emulation.
>   */
> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_run *run = vcpu->run;
>  	int ret;
>  
>  	if (unlikely(!kvm_vcpu_initialized(vcpu)))
> @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		return ret;
>  
>  	if (run->exit_reason == KVM_EXIT_MMIO) {
> -		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> +		ret = kvm_handle_mmio_return(vcpu, run);

I don't know much about ARM but this also seems redundant,
kvm_handle_mmio_return() is also able to extruct 'struct kvm_run' from'
'struct kvm_vcpu'. This likely deserves it's own patch though.

>  		if (ret)
>  			return ret;
>  	}
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 74bdb7bf3295..e18faea89146 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3135,7 +3135,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
>  				synchronize_rcu();
>  			put_pid(oldpid);
>  		}
> -		r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run);
> +		r = kvm_arch_vcpu_ioctl_run(vcpu);
>  		trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
>  		break;
>  	}

Looked at non-x86 arches just briefly but there seems to be no
controversy here, so

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply

* Re: [PATCH v2,5/5] drivers: uio: new driver for fsl_85xx_cache_sram
From: Greg KH @ 2020-04-16  6:30 UTC (permalink / raw)
  To: Scott Wood; +Cc: linux-kernel, kernel, Wang Wenhu, linuxppc-dev
In-Reply-To: <f2a1f91f92c0fe4bce46c28222dea355d96e2090.camel@buserror.net>

On Wed, Apr 15, 2020 at 02:27:51PM -0500, Scott Wood wrote:
> > > +		dev_err(&pdev->dev, "error no valid uio-map configured\n");
> > > +		ret = -EINVAL;
> > > +		goto err_info_free_internel;
> > > +	}
> > > +
> > > +	info->version = "0.1.0";
> > 
> > Could you define some DRIVER_VERSION in the top of the file next to 
> > DRIVER_NAME instead of hard coding in the middle on a function ?
> 
> That's what v1 had, and Greg KH said to remove it.  I'm guessing that he
> thought it was the common-but-pointless practice of having the driver print a
> version number that never gets updated, rather than something the UIO API
> (unfortunately, compared to a feature query interface) expects.  That said,
> I'm not sure what the value is of making it a macro since it should only be
> used once, that use is self documenting, it isn't tunable, etc.  Though if
> this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
> again, it should be UIO_VERSION, not DRIVER_VERSION).
> 
> Does this really need a three-part version scheme?  What's wrong with a
> version of "1", to be changed to "2" in the hopefully-unlikely event that the
> userspace API changes?  Assuming UIO is used for this at all, which doesn't
> seem like a great fit to me.

No driver version numbers at all please, they do not make any sense when
the driver is included in the kernel tree.

greg k-h

^ 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