[PATCH v2 0/5] powerpc/rtas: miscellaneous cleanups

This is a reroll of the series posted here:
https://lore.kernel.org/linuxppc-dev/20210114220004.1138993-1-nathanl@linux.ibm.com/

Originally this work was prompted by failures on radix MMU PowerVM
guests when passing buffers to RTAS that lay outside of its idea of
the RMA. In v1 I approached this as a problem to be solved in Linux,
but RTAS development has since decided to change their code so that
the RMA restriction does not apply with radix.

So in v2 I retain the cleanups and discard the more significant change
which accommodated the misbehaving RTAS versions.

Changes since v1:
- Correct missing conversion of RTAS_RMOBUF_MAX ->
  RTAS_USER_REGION_SIZE in in_rmo_buf().
- Remove unnecessary braces in rtas_syscall_filter_init().
- Leave expression of RTAS_WORK_AREA_SIZE as-is instead of changing
  the factors in a confusing way, per discussion with Alexey.
- Drop "powerpc/rtas: constrain user region allocation to RMA"

Nathan Lynch (5):
  powerpc/rtas: improve ppc_rtas_rmo_buf_show documentation
  powerpc/rtas-proc: remove unused RMO_READ_BUF_MAX
  powerpc/rtas: remove ibm_suspend_me_token
  powerpc/rtas: move syscall filter setup into separate function
  powerpc/rtas: rename RTAS_RMOBUF_MAX to RTAS_USER_REGION_SIZE

 arch/powerpc/include/asm/rtas.h |  6 +++---
 arch/powerpc/kernel/rtas-proc.c | 15 +++++++++++----
 arch/powerpc/kernel/rtas.c      | 34 +++++++++++++++++----------------
 3 files changed, 32 insertions(+), 23 deletions(-)

-- 
2.30.2

Add kerneldoc for ppc_rtas_rmo_buf_show(), the callback for
/proc/powerpc/rtas/rmo_buffer, explaining its expected use.

Reviewed-by: Alexey Kardashevskiy <redacted>
Reviewed-by: Andrew Donnellan <redacted>
Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/kernel/rtas-proc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/rtas-proc.c b/arch/powerpc/kernel/rtas-proc.c
index 2d33f342a293..e0f8329966d6 100644
--- a/arch/powerpc/kernel/rtas-proc.c
+++ b/arch/powerpc/kernel/rtas-proc.c

@@ -757,7 +757,16 @@ static int ppc_rtas_tone_volume_show(struct seq_file *m, void *v)
 
 #define RMO_READ_BUF_MAX 30
 
-/* RTAS Userspace access */
+/**
+ * ppc_rtas_rmo_buf_show() - Describe RTAS-addressable region for user space.
+ *
+ * Base + size description of a range of RTAS-addressable memory set
+ * aside for user space to use as work area(s) for certain RTAS
+ * functions. User space accesses this region via /dev/mem. Apart from
+ * security policies, the kernel does not arbitrate or serialize
+ * access to this region, and user space must ensure that concurrent
+ * users do not interfere with each other.
+ */
 static int ppc_rtas_rmo_buf_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "%016lx %x\n", rtas_rmo_buf, RTAS_RMOBUF_MAX);

-- 
2.30.2

RTAS_RMOBUF_MAX doesn't actually describe a "maximum" value in any
sense. It represents the size of an area of memory set aside for user
space to use as work areas for certain RTAS calls.

Rename it to RTAS_USER_REGION_SIZE.

Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/include/asm/rtas.h | 6 +++---
 arch/powerpc/kernel/rtas-proc.c | 2 +-
 arch/powerpc/kernel/rtas.c      | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 658448ca5b8a..9dc97d2f9d27 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -19,8 +19,8 @@
 #define RTAS_UNKNOWN_SERVICE (-1)
 #define RTAS_INSTANTIATE_MAX (1ULL<<30) /* Don't instantiate rtas at/above this value */
 
-/* Buffer size for ppc_rtas system call. */
-#define RTAS_RMOBUF_MAX (64 * 1024)
+/* Memory set aside for sys_rtas to use with calls that need a work area. */
+#define RTAS_USER_REGION_SIZE (64 * 1024)
 
 /* RTAS return status codes */
 #define RTAS_BUSY		-2    /* RTAS Busy */

@@ -357,7 +357,7 @@ extern void rtas_take_timebase(void);
 static inline int page_is_rtas_user_buf(unsigned long pfn)
 {
 	unsigned long paddr = (pfn << PAGE_SHIFT);
-	if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
+	if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + RTAS_USER_REGION_SIZE))
 		return 1;
 	return 0;
 }

diff --git a/arch/powerpc/kernel/rtas-proc.c b/arch/powerpc/kernel/rtas-proc.c
index d2b0d99824a4..6857a5b0a1c3 100644
--- a/arch/powerpc/kernel/rtas-proc.c
+++ b/arch/powerpc/kernel/rtas-proc.c

@@ -767,6 +767,6 @@ static int ppc_rtas_tone_volume_show(struct seq_file *m, void *v)
  */
 static int ppc_rtas_rmo_buf_show(struct seq_file *m, void *v)
 {
-	seq_printf(m, "%016lx %x\n", rtas_rmo_buf, RTAS_RMOBUF_MAX);
+	seq_printf(m, "%016lx %x\n", rtas_rmo_buf, RTAS_USER_REGION_SIZE);
 	return 0;
 }

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 24dc7bc463a8..6bada744402b 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -987,10 +987,10 @@ static struct rtas_filter rtas_filters[] __ro_after_init = {
 static bool in_rmo_buf(u32 base, u32 end)
 {
 	return base >= rtas_rmo_buf &&
-		base < (rtas_rmo_buf + RTAS_RMOBUF_MAX) &&
+		base < (rtas_rmo_buf + RTAS_USER_REGION_SIZE) &&
 		base <= end &&
 		end >= rtas_rmo_buf &&
-		end < (rtas_rmo_buf + RTAS_RMOBUF_MAX);
+		end < (rtas_rmo_buf + RTAS_USER_REGION_SIZE);
 }
 
 static bool block_rtas_call(int token, int nargs,

@@ -1202,7 +1202,7 @@ void __init rtas_initialize(void)
 	if (firmware_has_feature(FW_FEATURE_LPAR))
 		rtas_region = min(ppc64_rma_size, RTAS_INSTANTIATE_MAX);
 #endif
-	rtas_rmo_buf = memblock_phys_alloc_range(RTAS_RMOBUF_MAX, PAGE_SIZE,
+	rtas_rmo_buf = memblock_phys_alloc_range(RTAS_USER_REGION_SIZE, PAGE_SIZE,
 						 0, rtas_region);
 	if (!rtas_rmo_buf)
 		panic("ERROR: RTAS: Failed to allocate %lx bytes below %pa\n",

-- 
2.30.2

On 9/4/21 12:06 am, Nathan Lynch wrote:

RTAS_RMOBUF_MAX doesn't actually describe a "maximum" value in any
sense. It represents the size of an area of memory set aside for user
space to use as work areas for certain RTAS calls.

Rename it to RTAS_USER_REGION_SIZE.

Signed-off-by: Nathan Lynch <redacted>

This appears to correctly rename all users of the macro.

Reviewed-by: Andrew Donnellan <redacted>

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

Reduce conditionally compiled sections within rtas_initialize() by
moving the filter table initialization into its own function already
guarded by CONFIG_PPC_RTAS_FILTER. No behavior change intended.

Reviewed-by: Alexey Kardashevskiy <redacted>
Acked-by: Andrew Donnellan <redacted>
Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/kernel/rtas.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 60fcf7f7b0b8..24dc7bc463a8 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -1051,6 +1051,14 @@ static bool block_rtas_call(int token, int nargs,
 	return true;
 }
 
+static void __init rtas_syscall_filter_init(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++)
+		rtas_filters[i].token = rtas_token(rtas_filters[i].name);
+}
+
 #else
 
 static bool block_rtas_call(int token, int nargs,

@@ -1059,6 +1067,10 @@ static bool block_rtas_call(int token, int nargs,
 	return false;
 }
 
+static void __init rtas_syscall_filter_init(void)
+{
+}
+
 #endif /* CONFIG_PPC_RTAS_FILTER */
 
 /* We assume to be passed big endian arguments */

@@ -1162,9 +1174,6 @@ void __init rtas_initialize(void)
 	unsigned long rtas_region = RTAS_INSTANTIATE_MAX;
 	u32 base, size, entry;
 	int no_base, no_size, no_entry;
-#ifdef CONFIG_PPC_RTAS_FILTER
-	int i;
-#endif
 
 	/* Get RTAS dev node and fill up our "rtas" structure with infos
 	 * about it.

@@ -1203,11 +1212,7 @@ void __init rtas_initialize(void)
 	rtas_last_error_token = rtas_token("rtas-last-error");
 #endif
 
-#ifdef CONFIG_PPC_RTAS_FILTER
-	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
-		rtas_filters[i].token = rtas_token(rtas_filters[i].name);
-	}
-#endif
+	rtas_syscall_filter_init();
 }
 
 int __init early_init_dt_scan_rtas(unsigned long node,

-- 
2.30.2

This constant is unused.

Reviewed-by: Alexey Kardashevskiy <redacted>
Reviewed-by: Andrew Donnellan <redacted>
Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/kernel/rtas-proc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/kernel/rtas-proc.c b/arch/powerpc/kernel/rtas-proc.c
index e0f8329966d6..d2b0d99824a4 100644
--- a/arch/powerpc/kernel/rtas-proc.c
+++ b/arch/powerpc/kernel/rtas-proc.c

@@ -755,8 +755,6 @@ static int ppc_rtas_tone_volume_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-#define RMO_READ_BUF_MAX 30
-
 /**
  * ppc_rtas_rmo_buf_show() - Describe RTAS-addressable region for user space.
  *

-- 
2.30.2

There's not a compelling reason to cache the value of the token for
the ibm,suspend-me function. Just look it up when needed in the RTAS
syscall's special case for it.

Reviewed-by: Alexey Kardashevskiy <redacted>
Reviewed-by: Andrew Donnellan <redacted>
Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/kernel/rtas.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index d126d71ea5bd..60fcf7f7b0b8 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -828,7 +828,6 @@ void rtas_activate_firmware(void)
 		pr_err("ibm,activate-firmware failed (%i)\n", fwrc);
 }
 
-static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
 #ifdef CONFIG_PPC_PSERIES
 /**
  * rtas_call_reentrant() - Used for reentrant rtas calls

@@ -1103,7 +1102,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 		return -EINVAL;
 
 	/* Need to handle ibm,suspend_me call specially */
-	if (token == ibm_suspend_me_token) {
+	if (token == rtas_token("ibm,suspend-me")) {
 
 		/*
 		 * rtas_ibm_suspend_me assumes the streamid handle is in cpu

@@ -1191,10 +1190,8 @@ void __init rtas_initialize(void)
 	 * the stop-self token if any
 	 */
 #ifdef CONFIG_PPC64
-	if (firmware_has_feature(FW_FEATURE_LPAR)) {
+	if (firmware_has_feature(FW_FEATURE_LPAR))
 		rtas_region = min(ppc64_rma_size, RTAS_INSTANTIATE_MAX);
-		ibm_suspend_me_token = rtas_token("ibm,suspend-me");
-	}
 #endif
 	rtas_rmo_buf = memblock_phys_alloc_range(RTAS_RMOBUF_MAX, PAGE_SIZE,
 						 0, rtas_region);

-- 
2.30.2

Le 08/04/2021 à 16:06, Nathan Lynch a écrit :

This is a reroll of the series posted here:
https://lore.kernel.org/linuxppc-dev/20210114220004.1138993-1-nathanl@linux.ibm.com/

Originally this work was prompted by failures on radix MMU PowerVM
guests when passing buffers to RTAS that lay outside of its idea of
the RMA. In v1 I approached this as a problem to be solved in Linux,
but RTAS development has since decided to change their code so that
the RMA restriction does not apply with radix.

So in v2 I retain the cleanups and discard the more significant change
which accommodated the misbehaving RTAS versions.

Is there a link with https://github.com/linuxppc/issues/issues/252 ?

Changes since v1:
- Correct missing conversion of RTAS_RMOBUF_MAX ->
   RTAS_USER_REGION_SIZE in in_rmo_buf().
- Remove unnecessary braces in rtas_syscall_filter_init().
- Leave expression of RTAS_WORK_AREA_SIZE as-is instead of changing
   the factors in a confusing way, per discussion with Alexey.
- Drop "powerpc/rtas: constrain user region allocation to RMA"

Nathan Lynch (5):
   powerpc/rtas: improve ppc_rtas_rmo_buf_show documentation
   powerpc/rtas-proc: remove unused RMO_READ_BUF_MAX
   powerpc/rtas: remove ibm_suspend_me_token
   powerpc/rtas: move syscall filter setup into separate function
   powerpc/rtas: rename RTAS_RMOBUF_MAX to RTAS_USER_REGION_SIZE

  arch/powerpc/include/asm/rtas.h |  6 +++---
  arch/powerpc/kernel/rtas-proc.c | 15 +++++++++++----
  arch/powerpc/kernel/rtas.c      | 34 +++++++++++++++++----------------
  3 files changed, 32 insertions(+), 23 deletions(-)

Christophe Leroy [off-list ref] writes:

Le 08/04/2021 à 16:06, Nathan Lynch a écrit :

quoted

This is a reroll of the series posted here:
https://lore.kernel.org/linuxppc-dev/20210114220004.1138993-1-nathanl@linux.ibm.com/

Originally this work was prompted by failures on radix MMU PowerVM
guests when passing buffers to RTAS that lay outside of its idea of
the RMA. In v1 I approached this as a problem to be solved in Linux,
but RTAS development has since decided to change their code so that
the RMA restriction does not apply with radix.

So in v2 I retain the cleanups and discard the more significant change
which accommodated the misbehaving RTAS versions.

Is there a link with https://github.com/linuxppc/issues/issues/252 ?

No, not really.

On Thu, 8 Apr 2021 09:06:25 -0500, Nathan Lynch wrote:

This is a reroll of the series posted here:
https://lore.kernel.org/linuxppc-dev/20210114220004.1138993-1-nathanl@linux.ibm.com/

Originally this work was prompted by failures on radix MMU PowerVM
guests when passing buffers to RTAS that lay outside of its idea of
the RMA. In v1 I approached this as a problem to be solved in Linux,
but RTAS development has since decided to change their code so that
the RMA restriction does not apply with radix.

[...]

Applied to powerpc/next.

[1/5] powerpc/rtas: improve ppc_rtas_rmo_buf_show documentation
      https://git.kernel.org/powerpc/c/c13ff6f3251318f5e1ff5b1a6d05f76996db672a
[2/5] powerpc/rtas-proc: remove unused RMO_READ_BUF_MAX
      https://git.kernel.org/powerpc/c/01c1b9984a12a379f332c39c4b1fd96e473b93b0
[3/5] powerpc/rtas: remove ibm_suspend_me_token
      https://git.kernel.org/powerpc/c/0ab1c929ae38262c4deb18b4a2e03a4f0cb5c5ed
[4/5] powerpc/rtas: move syscall filter setup into separate function
      https://git.kernel.org/powerpc/c/0649cdc8237943c15fc977e96033dc8ae28cc2bd
[5/5] powerpc/rtas: rename RTAS_RMOBUF_MAX to RTAS_USER_REGION_SIZE
      https://git.kernel.org/powerpc/c/e5d56763525e65417dad0d46572b234fa0008e40

cheers