[PATCH 0/6] powerpc/pseries - bugfixes/cleanups for PLPKS driver

[PATCH 0/6] powerpc/pseries - bugfixes/cleanups for PLPKS driver

[Nayna Jain]

This patchset fixes some bugs and does some cleanups.

Nayna Jain (6):
  powerpc/pseries: fix the object owners enum value in plpks driver
  powerpc/pseries: Fix the H_CALL error code in PLPKS driver
  powerpc/pseries: Return -EIO instead of -EINTR for H_ABORTED error
  powerpc/pseries: cleanup error logs in plpks driver
  powerpc/pseries: replace kmalloc with kzalloc in PLPKS driver
  powerpc/pseries: fix plpks_read_var() code for different consumers

 arch/powerpc/include/asm/hvcall.h      |  3 +-
 arch/powerpc/platforms/pseries/plpks.c | 50 ++++++++++++--------------
 arch/powerpc/platforms/pseries/plpks.h |  2 +-
 3 files changed, 24 insertions(+), 31 deletions(-)

-- 
2.31.1

[PATCH 1/6] powerpc/pseries: fix the object owners enum value in plpks driver

[Nayna Jain]

OS_VAR_LINUX enum in PLPKS driver should be 0x02 instead of 0x01.

Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform KeyStore")
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/plpks.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/plpks.h b/arch/powerpc/platforms/pseries/plpks.h
index c6a291367bb1..275ccd86bfb5 100644
--- a/arch/powerpc/platforms/pseries/plpks.h
+++ b/arch/powerpc/platforms/pseries/plpks.h
@@ -17,7 +17,7 @@
 #define WORLDREADABLE 0x08000000
 #define SIGNEDUPDATE 0x01000000
 
-#define PLPKS_VAR_LINUX	0x01
+#define PLPKS_VAR_LINUX	0x02
 #define PLPKS_VAR_COMMON	0x04
 
 struct plpks_var {
-- 
2.31.1

[PATCH 2/6] powerpc/pseries: Fix the H_CALL error code in PLPKS driver

[Nayna Jain]

PAPR Spec defines H_P1 actually as H_PARAMETER and maps H_ABORTED to
a different numerical value.

Fix the error codes as per PAPR Specification.

Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform KeyStore")
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/include/asm/hvcall.h      | 3 +--
 arch/powerpc/platforms/pseries/plpks.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 8abae463f6c1..95fd7f9485d5 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -79,7 +79,7 @@
 #define H_NOT_ENOUGH_RESOURCES -44
 #define H_R_STATE       -45
 #define H_RESCINDED     -46
-#define H_P1		-54
+#define H_ABORTED	-54
 #define H_P2		-55
 #define H_P3		-56
 #define H_P4		-57
@@ -100,7 +100,6 @@
 #define H_COP_HW	-74
 #define H_STATE		-75
 #define H_IN_USE	-77
-#define H_ABORTED	-78
 #define H_UNSUPPORTED_FLAG_START	-256
 #define H_UNSUPPORTED_FLAG_END		-511
 #define H_MULTI_THREADS_ACTIVE	-9005
diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index f4b5b5a64db3..32ce4d780d8f 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -75,7 +75,7 @@ static int pseries_status_to_err(int rc)
 	case H_FUNCTION:
 		err = -ENXIO;
 		break;
-	case H_P1:
+	case H_PARAMETER:
 	case H_P2:
 	case H_P3:
 	case H_P4:
-- 
2.31.1

[PATCH 3/6] powerpc/pseries: Return -EIO instead of -EINTR for H_ABORTED error

[Nayna Jain]

Some commands for eg. "cat" might continue to retry on encountering
EINTR. This is not expected for original error code H_ABORTED.

Map H_ABORTED to more relevant Linux error code EIO.

Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform KeyStore")
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/plpks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index 32ce4d780d8f..cbea447122ca 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -111,7 +111,7 @@ static int pseries_status_to_err(int rc)
 		err = -EEXIST;
 		break;
 	case H_ABORTED:
-		err = -EINTR;
+		err = -EIO;
 		break;
 	default:
 		err = -EINVAL;
-- 
2.31.1

[PATCH 4/6] powerpc/pseries: cleanup error logs in plpks driver

[Nayna Jain]

Logging H_CALL return codes in PLPKS driver are easy to confuse with
Linux error codes.

Let the caller of the function log the converted linux error code.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/plpks.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index cbea447122ca..72d9debf18c0 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -312,10 +312,6 @@ int plpks_write_var(struct plpks_var var)
 	if (!rc)
 		rc = plpks_confirm_object_flushed(label, auth);
 
-	if (rc)
-		pr_err("Failed to write variable %s for component %s with error %d\n",
-		       var.name, var.component, rc);
-
 	rc = pseries_status_to_err(rc);
 	kfree(label);
 out:
@@ -350,10 +346,6 @@ int plpks_remove_var(char *component, u8 varos, struct plpks_var_name vname)
 	if (!rc)
 		rc = plpks_confirm_object_flushed(label, auth);
 
-	if (rc)
-		pr_err("Failed to remove variable %s for component %s with error %d\n",
-		       vname.name, component, rc);
-
 	rc = pseries_status_to_err(rc);
 	kfree(label);
 out:
@@ -395,8 +387,6 @@ static int plpks_read_var(u8 consumer, struct plpks_var *var)
 			 maxobjsize);
 
 	if (rc != H_SUCCESS) {
-		pr_err("Failed to read variable %s for component %s with error %d\n",
-		       var->name, var->component, rc);
 		rc = pseries_status_to_err(rc);
 		goto out_free_output;
 	}
-- 
2.31.1

[PATCH 5/6] powerpc/pseries: replace kmalloc with kzalloc in PLPKS driver

[Nayna Jain]

Replace kmalloc with kzalloc in construct_auth() function to default
initialize structure with zeroes.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/plpks.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index 72d9debf18c0..e8c02735b702 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -162,19 +162,15 @@ static struct plpks_auth *construct_auth(u8 consumer)
 	if (consumer > PKS_OS_OWNER)
 		return ERR_PTR(-EINVAL);
 
-	auth = kmalloc(struct_size(auth, password, maxpwsize), GFP_KERNEL);
+	auth = kzalloc(struct_size(auth, password, maxpwsize), GFP_KERNEL);
 	if (!auth)
 		return ERR_PTR(-ENOMEM);
 
 	auth->version = 1;
 	auth->consumer = consumer;
-	auth->rsvd0 = 0;
-	auth->rsvd1 = 0;
 
-	if (consumer == PKS_FW_OWNER || consumer == PKS_BOOTLOADER_OWNER) {
-		auth->passwordlength = 0;
+	if (consumer == PKS_FW_OWNER || consumer == PKS_BOOTLOADER_OWNER)
 		return auth;
-	}
 
 	memcpy(auth->password, ospassword, ospasswordlength);
 
-- 
2.31.1

[PATCH 6/6] powerpc/pseries: fix plpks_read_var() code for different consumers

[Nayna Jain]

Even though plpks_read_var() is currently called to read variables
owned by different consumers, it internally supports only OS consumer.

Fix plpks_read_var() to handle different consumers correctly.

Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform KeyStore")
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/plpks.c | 28 +++++++++++++++++---------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index e8c02735b702..4edd1585e245 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -354,22 +354,24 @@ static int plpks_read_var(u8 consumer, struct plpks_var *var)
 {
 	unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 };
 	struct plpks_auth *auth;
-	struct label *label;
+	struct label *label = NULL;
 	u8 *output;
 	int rc;
 
 	if (var->namelen > MAX_NAME_SIZE)
 		return -EINVAL;
 
-	auth = construct_auth(PKS_OS_OWNER);
+	auth = construct_auth(consumer);
 	if (IS_ERR(auth))
 		return PTR_ERR(auth);
 
-	label = construct_label(var->component, var->os, var->name,
-				var->namelen);
-	if (IS_ERR(label)) {
-		rc = PTR_ERR(label);
-		goto out_free_auth;
+	if (consumer == PKS_OS_OWNER) {
+		label = construct_label(var->component, var->os, var->name,
+					var->namelen);
+		if (IS_ERR(label)) {
+			rc = PTR_ERR(label);
+			goto out_free_auth;
+		}
 	}
 
 	output = kzalloc(maxobjsize, GFP_KERNEL);
@@ -378,9 +380,15 @@ static int plpks_read_var(u8 consumer, struct plpks_var *var)
 		goto out_free_label;
 	}
 
-	rc = plpar_hcall(H_PKS_READ_OBJECT, retbuf, virt_to_phys(auth),
-			 virt_to_phys(label), label->size, virt_to_phys(output),
-			 maxobjsize);
+	if (consumer == PKS_OS_OWNER)
+		rc = plpar_hcall(H_PKS_READ_OBJECT, retbuf, virt_to_phys(auth),
+				 virt_to_phys(label), label->size, virt_to_phys(output),
+				 maxobjsize);
+	else
+		rc = plpar_hcall(H_PKS_READ_OBJECT, retbuf, virt_to_phys(auth),
+				 virt_to_phys(var->name), var->namelen, virt_to_phys(output),
+				 maxobjsize);
+
 
 	if (rc != H_SUCCESS) {
 		rc = pseries_status_to_err(rc);
-- 
2.31.1

Re: [PATCH 5/6] powerpc/pseries: replace kmalloc with kzalloc in PLPKS driver

[Andrew Donnellan]

On Sun, 2022-11-06 at 15:58 -0500, Nayna Jain wrote:
> Replace kmalloc with kzalloc in construct_auth() function to default
> initialize structure with zeroes.
> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>

This seems sensible.

> ---
>  arch/powerpc/platforms/pseries/plpks.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/plpks.c
> b/arch/powerpc/platforms/pseries/plpks.c
> index 72d9debf18c0..e8c02735b702 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
> @@ -162,19 +162,15 @@ static struct plpks_auth *construct_auth(u8
> consumer)
>         if (consumer > PKS_OS_OWNER)
>                 return ERR_PTR(-EINVAL);
>  
> -       auth = kmalloc(struct_size(auth, password, maxpwsize),
> GFP_KERNEL);
> +       auth = kzalloc(struct_size(auth, password, maxpwsize),
> GFP_KERNEL);
>         if (!auth)
>                 return ERR_PTR(-ENOMEM);
>  
>         auth->version = 1;
>         auth->consumer = consumer;
> -       auth->rsvd0 = 0;
> -       auth->rsvd1 = 0;
>  
> -       if (consumer == PKS_FW_OWNER || consumer ==
> PKS_BOOTLOADER_OWNER) {
> -               auth->passwordlength = 0;
> +       if (consumer == PKS_FW_OWNER || consumer ==
> PKS_BOOTLOADER_OWNER)

>                 return auth;
> -       }

This bit seems spurious.

>  
>         memcpy(auth->password, ospassword, ospasswordlength);
>  

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 1/6] powerpc/pseries: fix the object owners enum value in plpks driver

[Andrew Donnellan]

On Sun, 2022-11-06 at 15:58 -0500, Nayna Jain wrote:
> OS_VAR_LINUX enum in PLPKS driver should be 0x02 instead of 0x01.

This should be PLPKS_VAR_LINUX. And it's a macro, not an enum.

The new value does indeed match the (currently IBM internal)
specification - 0x01 is AIX.

> 
> Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform
> KeyStore")
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>

Should be noted that at present, this macro isn't used anywhere - it is
used in a future series.

> ---
>  arch/powerpc/platforms/pseries/plpks.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/plpks.h
> b/arch/powerpc/platforms/pseries/plpks.h
> index c6a291367bb1..275ccd86bfb5 100644
> --- a/arch/powerpc/platforms/pseries/plpks.h
> +++ b/arch/powerpc/platforms/pseries/plpks.h
> @@ -17,7 +17,7 @@
>  #define WORLDREADABLE 0x08000000
>  #define SIGNEDUPDATE 0x01000000
>  
> -#define PLPKS_VAR_LINUX        0x01
> +#define PLPKS_VAR_LINUX        0x02
>  #define PLPKS_VAR_COMMON       0x04
>  
>  struct plpks_var {

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 2/6] powerpc/pseries: Fix the H_CALL error code in PLPKS driver

[Andrew Donnellan]

On Sun, 2022-11-06 at 15:58 -0500, Nayna Jain wrote:
> PAPR Spec defines H_P1 actually as H_PARAMETER and maps H_ABORTED to
> a different numerical value.
> 
> Fix the error codes as per PAPR Specification.
> 
> Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform
> KeyStore")
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>

This does indeed match my understanding of the PAPR spec.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/include/asm/hvcall.h      | 3 +--
>  arch/powerpc/platforms/pseries/plpks.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/hvcall.h
> b/arch/powerpc/include/asm/hvcall.h
> index 8abae463f6c1..95fd7f9485d5 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -79,7 +79,7 @@
>  #define H_NOT_ENOUGH_RESOURCES -44
>  #define H_R_STATE       -45
>  #define H_RESCINDED     -46
> -#define H_P1           -54
> +#define H_ABORTED      -54
>  #define H_P2           -55
>  #define H_P3           -56
>  #define H_P4           -57
> @@ -100,7 +100,6 @@
>  #define H_COP_HW       -74
>  #define H_STATE                -75
>  #define H_IN_USE       -77
> -#define H_ABORTED      -78
>  #define H_UNSUPPORTED_FLAG_START       -256
>  #define H_UNSUPPORTED_FLAG_END         -511
>  #define H_MULTI_THREADS_ACTIVE -9005
> diff --git a/arch/powerpc/platforms/pseries/plpks.c
> b/arch/powerpc/platforms/pseries/plpks.c
> index f4b5b5a64db3..32ce4d780d8f 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
> @@ -75,7 +75,7 @@ static int pseries_status_to_err(int rc)
>         case H_FUNCTION:
>                 err = -ENXIO;
>                 break;
> -       case H_P1:
> +       case H_PARAMETER:
>         case H_P2:
>         case H_P3:
>         case H_P4:

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 3/6] powerpc/pseries: Return -EIO instead of -EINTR for H_ABORTED error

[Andrew Donnellan]

On Sun, 2022-11-06 at 15:58 -0500, Nayna Jain wrote:
> Some commands for eg. "cat" might continue to retry on encountering
> EINTR. This is not expected for original error code H_ABORTED.
> 
> Map H_ABORTED to more relevant Linux error code EIO.
> 
> Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform
> KeyStore")
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>

The interface spec states that, for PKS-related hcalls, H_Aborted means
"error occurred processing request" rather than something that would
specifically map to EINTR, so I think EIO is appropriate here.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  arch/powerpc/platforms/pseries/plpks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/plpks.c
> b/arch/powerpc/platforms/pseries/plpks.c
> index 32ce4d780d8f..cbea447122ca 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
> @@ -111,7 +111,7 @@ static int pseries_status_to_err(int rc)
>                 err = -EEXIST;
>                 break;
>         case H_ABORTED:
> -               err = -EINTR;
> +               err = -EIO;
>                 break;
>         default:
>                 err = -EINVAL;

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 0/6] powerpc/pseries - bugfixes/cleanups for PLPKS driver

[Michael Ellerman]

On Sun, 6 Nov 2022 15:58:33 -0500, Nayna Jain wrote:
> This patchset fixes some bugs and does some cleanups.
> 
> Nayna Jain (6):
>   powerpc/pseries: fix the object owners enum value in plpks driver
>   powerpc/pseries: Fix the H_CALL error code in PLPKS driver
>   powerpc/pseries: Return -EIO instead of -EINTR for H_ABORTED error
>   powerpc/pseries: cleanup error logs in plpks driver
>   powerpc/pseries: replace kmalloc with kzalloc in PLPKS driver
>   powerpc/pseries: fix plpks_read_var() code for different consumers
> 
> [...]

Applied to powerpc/next.

[1/6] powerpc/pseries: fix the object owners enum value in plpks driver
      https://git.kernel.org/powerpc/c/2330757e0be0acad88852e211dcd6106390a729b
[2/6] powerpc/pseries: Fix the H_CALL error code in PLPKS driver
      https://git.kernel.org/powerpc/c/af223e1728c448073d1e12fe464bf344310edeba
[3/6] powerpc/pseries: Return -EIO instead of -EINTR for H_ABORTED error
      https://git.kernel.org/powerpc/c/bb8e4c7cb759b90a04f2e94056b50288ff46a0ed
[4/6] powerpc/pseries: cleanup error logs in plpks driver
      https://git.kernel.org/powerpc/c/8888ea772972323362660e9a1339175294664a6c
[5/6] powerpc/pseries: replace kmalloc with kzalloc in PLPKS driver
      https://git.kernel.org/powerpc/c/212dd5cfbee7815f3c665a51c501701edb881599
[6/6] powerpc/pseries: fix plpks_read_var() code for different consumers
      https://git.kernel.org/powerpc/c/1f622f3f80cbf8999ff5955a2fcfbd801a1f32e0

cheers