[PATCH 0/6] powerpc/rtas: miscellaneous cleanups, user region allocation

The region exposed to user space for use as work areas passed to
sys_rtas() can be incorrectly allocated on radix, leading to failures
in users of librtas. Correct this and clean up some of the code
visited along the way.

I think the cleanups should be unobjectionable and I've placed them
first in the series. Please check my work on the rtas_rmo_buf
allocation changes; they are only lightly tested so far (slot add on
Power9 PowerVM, and comparison of /memory@0/reg with the contents of
/proc/powerpc/rtas/rmo_buf on qemu Power9 w/radix).

I suspect the per-cpu RTAS argument structures for reentrant calls
need similar measures, but I can add that to the series once there is
consensus on the approach.

Nathan Lynch (6):
  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
  powerpc/rtas: constrain user region allocation to RMA

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

-- 
2.29.2

This constant is unused.

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.29.2

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

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.29.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.

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.29.2

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.

Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/kernel/rtas.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

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

@@ -1051,6 +1051,16 @@ 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 +1069,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 +1176,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 +1214,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.29.2

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/kernel/rtas.c | 80 +++++++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index da65faadbbb2..98dfb112f4df 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -1166,6 +1166,70 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 	return 0;
 }
 
+/*
+ * Memory locations passed to RTAS must be in the RMA as described by
+ * the range in /memory@0.
+ */
+static phys_addr_t rtas_arg_addr_limit(void)
+{
+	unsigned int addr_cells;
+	unsigned int size_cells;
+	struct device_node *np;
+	const __be32 *prop;
+	u64 limit;
+	u64 base;
+
+	/* RTAS is instantiated in 32-bit mode. */
+	limit = 1ULL << 32;
+
+	/* Account for mem=. */
+	if (memory_limit != 0)
+		limit = min(limit, memory_limit);
+
+	np = of_find_node_by_path("/memory@0");
+	if (!np)
+		goto out;
+
+	prop = of_get_property(np, "reg", NULL);
+	if (!prop)
+		goto put;
+
+	addr_cells = of_n_addr_cells(np);
+	base = of_read_number(prop, addr_cells);
+	prop += addr_cells;
+	size_cells = of_n_size_cells(np);
+	limit = min(limit, of_read_number(prop, size_cells));
+put:
+	of_node_put(np);
+out:
+	pr_debug("%s: base = %#llx limit = %#llx", __func__, base, limit);
+
+	return limit;
+}
+
+static void __init rtas_user_region_setup(void)
+{
+	phys_addr_t limit, align, size;
+
+	limit = rtas_arg_addr_limit();
+	size = RTAS_USER_REGION_SIZE;
+
+	/*
+	 * Although work areas need only 4KB alignment, user space
+	 * accesses this region via mmap so it must be placed on a
+	 * page boundary.
+	 */
+	align = PAGE_SIZE;
+
+	rtas_rmo_buf = memblock_phys_alloc_range(size, align, 0, limit);
+	if (rtas_rmo_buf == 0) {
+		panic("Failed to allocate %llu bytes for user region below %pa\n",
+		      size, &limit);
+	}
+
+	pr_debug("RTAS user region allocated at %pa\n", &rtas_rmo_buf);
+}
+
 /*
  * Call early during boot, before mem init, to retrieve the RTAS
  * information from the device-tree and allocate the RMO buffer for userland

@@ -1173,7 +1237,6 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
  */
 void __init rtas_initialize(void)
 {
-	unsigned long rtas_region = RTAS_INSTANTIATE_MAX;
 	u32 base, size, entry;
 	int no_base, no_size, no_entry;
 

@@ -1197,23 +1260,10 @@ void __init rtas_initialize(void)
 	no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry);
 	rtas.entry = no_entry ? rtas.base : entry;
 
-	/* If RTAS was found, allocate the RMO buffer for it and look for
-	 * the stop-self token if any
-	 */
-#ifdef CONFIG_PPC64
-	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_USER_REGION_SIZE, PAGE_SIZE,
-						 0, rtas_region);
-	if (!rtas_rmo_buf)
-		panic("ERROR: RTAS: Failed to allocate %lx bytes below %pa\n",
-		      PAGE_SIZE, &rtas_region);
-
 #ifdef CONFIG_RTAS_ERROR_LOGGING
 	rtas_last_error_token = rtas_token("rtas-last-error");
 #endif
-
+	rtas_user_region_setup();
 	rtas_syscall_filter_init();
 }
 

-- 
2.29.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, and express the value in terms of the
number of work areas allocated.

Signed-off-by: Nathan Lynch <redacted>

squash! powerpc/rtas: rename RTAS_RMOBUF_MAX to RTAS_USER_REGION_SIZE
---
 arch/powerpc/include/asm/rtas.h | 9 ++++++---
 arch/powerpc/kernel/rtas-proc.c | 2 +-
 arch/powerpc/kernel/rtas.c      | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 332e1000ca0f..1aa7ab1cbc84 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -19,8 +19,11 @@
 #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)
+/* Work areas shared with RTAS must be 4K, naturally aligned. */
+#define RTAS_WORK_AREA_SIZE   4096
+
+/* Work areas allocated for user space access. */
+#define RTAS_USER_REGION_SIZE (RTAS_WORK_AREA_SIZE * 16)
 
 /* RTAS return status codes */
 #define RTAS_BUSY		-2    /* RTAS Busy */

@@ -357,7 +360,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 55f6aa170e57..da65faadbbb2 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -1204,7 +1204,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.29.2

On 15/01/2021 09:00, Nathan Lynch wrote:

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add 
extra 64K in prom_instantiate_rtas() and advertise this address 
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do 
not need this RMO area before that point.

And probably do the same with per-cpu RTAS argument structures mentioned 
in the cover letter?

-- 
Alexey

On 15/01/2021 09:00, Nathan Lynch wrote:

This constant is unused.

Signed-off-by: Nathan Lynch <redacted>

Reviewed-by: Alexey Kardashevskiy <redacted>

-- 
Alexey

On 15/01/2021 09:00, Nathan Lynch wrote:

Why exactly 4K and not (for example) PAGE_SIZE?

+#define RTAS_WORK_AREA_SIZE   4096
+
+/* Work areas allocated for user space access. */
+#define RTAS_USER_REGION_SIZE (RTAS_WORK_AREA_SIZE * 16)

This is still 64K but no clarity why. There is 16 of something, what is it?

-- 
Alexey

On 15/01/2021 09:00, Nathan Lynch wrote:

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.

Signed-off-by: Nathan Lynch <redacted>

Reviewed-by: Alexey Kardashevskiy <redacted>

-- 
Alexey

On 15/01/2021 08:59, Nathan Lynch wrote:

mmm ufff wuuuuut argh^w^w^w^w Thanks for documenting it :)

Reviewed-by: Alexey Kardashevskiy <redacted>

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);

-- 
Alexey

On 15/01/2021 09:00, Nathan Lynch wrote:

Unnecessary curly braces (I understand it is cut-n-paste but still) and 
an empty line. Otherwise:

Reviewed-by: Alexey Kardashevskiy <redacted>

-- 
Alexey

On 15/1/21 9:00 am, Nathan Lynch wrote:

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.

Signed-off-by: Nathan Lynch <redacted>

Acked-by: Andrew Donnellan <redacted>

+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);
+	}
+
+}

Unnecessary empty line, but vitally important braces :)


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

On 15/1/21 8:59 am, Nathan Lynch wrote:

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

Signed-off-by: Nathan Lynch <redacted>

Looks good.

Reviewed-by: Andrew Donnellan <redacted>


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

On 15/1/21 9:00 am, Nathan Lynch wrote:

This constant is unused.

Signed-off-by: Nathan Lynch <redacted>

A quick grep agrees.

Reviewed-by: Andrew Donnellan <redacted>

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

On 15/1/21 9:00 am, Nathan Lynch wrote:

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.

Signed-off-by: Nathan Lynch <redacted>

Reviewed-by: Andrew Donnellan <redacted>

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

On 15/1/21 9:00 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, and express the value in terms of the
number of work areas allocated.

Signed-off-by: Nathan Lynch <redacted>

squash! powerpc/rtas: rename RTAS_RMOBUF_MAX to RTAS_USER_REGION_SIZE

I think you meant to get rid of this line...

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

Hi Nathan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.11-rc3 next-20210115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nathan-Lynch/powerpc-rtas-miscellaneous-cleanups-user-region-allocation/20210115-113045
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/656368735841abab983fa593b2ddae52a6273a71
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nathan-Lynch/powerpc-rtas-miscellaneous-cleanups-user-region-allocation/20210115-113045
        git checkout 656368735841abab983fa593b2ddae52a6273a71
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/rtas.c: In function 'in_rmo_buf':

quoted

arch/powerpc/kernel/rtas.c:990:26: error: 'RTAS_RMOBUF_MAX' undeclared (first use in this function)

     990 |   base < (rtas_rmo_buf + RTAS_RMOBUF_MAX) &&
         |                          ^~~~~~~~~~~~~~~
   arch/powerpc/kernel/rtas.c:990:26: note: each undeclared identifier is reported only once for each function it appears in
   arch/powerpc/kernel/rtas.c:994:1: error: control reaches end of non-void function [-Werror=return-type]
     994 | }
         | ^
   cc1: some warnings being treated as errors


vim +/RTAS_RMOBUF_MAX +990 arch/powerpc/kernel/rtas.c

bd59380c5ba4147d Andrew Donnellan 2020-08-20  986  
bd59380c5ba4147d Andrew Donnellan 2020-08-20  987  static bool in_rmo_buf(u32 base, u32 end)
bd59380c5ba4147d Andrew Donnellan 2020-08-20  988  {
bd59380c5ba4147d Andrew Donnellan 2020-08-20  989  	return base >= rtas_rmo_buf &&
bd59380c5ba4147d Andrew Donnellan 2020-08-20 @990  		base < (rtas_rmo_buf + RTAS_RMOBUF_MAX) &&
bd59380c5ba4147d Andrew Donnellan 2020-08-20  991  		base <= end &&
bd59380c5ba4147d Andrew Donnellan 2020-08-20  992  		end >= rtas_rmo_buf &&
bd59380c5ba4147d Andrew Donnellan 2020-08-20  993  		end < (rtas_rmo_buf + RTAS_RMOBUF_MAX);
bd59380c5ba4147d Andrew Donnellan 2020-08-20  994  }
bd59380c5ba4147d Andrew Donnellan 2020-08-20  995  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Alexey Kardashevskiy [off-list ref] writes:

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add 
extra 64K in prom_instantiate_rtas() and advertise this address 
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do 
not need this RMO area before that point.

Can you explain more about what advantage that would bring? I'm not
seeing it. It's a more significant change than what I've written
here. Would it interact well with kexec?

And probably do the same with per-cpu RTAS argument structures mentioned 
in the cover letter?

I don't think so, since those need to be allocated with the pacas and
limited to the maximum possible CPUs, which is discovered by the kernel
much later.

But maybe I misunderstand what you're suggesting.

Alexey Kardashevskiy [off-list ref] writes:

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 332e1000ca0f..1aa7ab1cbc84 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -19,8 +19,11 @@
  #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)
+/* Work areas shared with RTAS must be 4K, naturally aligned. */

Why exactly 4K and not (for example) PAGE_SIZE?

4K is a platform requirement and isn't related to Linux's configured
page size. See the PAPR specification for RTAS functions such as
ibm,configure-connector, ibm,update-nodes, ibm,update-properties.

There are other calls with work area parameters where alignment isn't
specified (e.g. ibm,get-system-parameter) but 4KB alignment is a safe
choice for those.

quoted

+#define RTAS_WORK_AREA_SIZE   4096
+
+/* Work areas allocated for user space access. */
+#define RTAS_USER_REGION_SIZE (RTAS_WORK_AREA_SIZE * 16)

This is still 64K but no clarity why. There is 16 of something, what
is it?

There are 16 4KB work areas in the region. I can name it
RTAS_NR_USER_WORK_AREAS or similar.

Alexey Kardashevskiy [off-list ref] writes:

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

+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);
+	}
+

Unnecessary curly braces (I understand it is cut-n-paste but still) and 
an empty line.

Yes, will fix in v2, thanks.

On 16/01/2021 02:38, Nathan Lynch wrote:

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add
extra 64K in prom_instantiate_rtas() and advertise this address
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do
not need this RMO area before that point.

Can you explain more about what advantage that would bring? I'm not
seeing it. It's a more significant change than what I've written
here.

We already allocate space for RTAS and (like RMO) it needs to be in RMA, 
and RMO is useless without RTAS. We can reuse RTAS allocation code for 
RMO like this:

===

diff --git a/arch/powerpc/kernel/prom_init.c 

b/arch/powerpc/kernel/prom_init.c
index e9d4eb6144e1..d9527d3e01d2 100644

--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -1821,7 +1821,8 @@ static void __init prom_instantiate_rtas(void)
         if (size == 0)
                 return;

-       base = alloc_down(size, PAGE_SIZE, 0);
+       /* One page for RTAS, one for RMO */
+       base = alloc_down(size, PAGE_SIZE + PAGE_SIZE, 0);
         if (base == 0)
                 prom_panic("Could not allocate memory for RTAS\n");

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

@@ -1186,6 +1186,7 @@ void __init rtas_initialize(void)
         rtas.size = size;
         no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", 

&entry);
         rtas.entry = no_entry ? rtas.base : entry;
+       rtas_rmo_buf = rtas.base + PAGE_SIZE;

         /* If RTAS was found, allocate the RMO buffer for it and look for
          * the stop-self token if any

@@ -1196,11 +1197,6 @@ void __init rtas_initialize(void)
                 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);
-       if (!rtas_rmo_buf)
-               panic("ERROR: RTAS: Failed to allocate %lx bytes below 

%pa\n",
-                     PAGE_SIZE, &rtas_region);
===


May be store in the FDT as "linux,rmo-base" next to "linux,rtas-base", 
for clarity, as sharing symbols between prom and main kernel is a bit 
tricky.

The benefit is that we do not do the same thing   (== find 64K in RMA) 
in 2 different ways and if the RMO allocated my way is broken - we'll 
know it much sooner as RTAS itself will break too.

Would it interact well with kexec?

Good point. For this, the easiest will be setting rtas-size in the FDT 
to the allocated RTAS space (PAGE_SIZE*2 with the hunk above applied). 
Probably.

quoted

And probably do the same with per-cpu RTAS argument structures mentioned
in the cover letter?

I don't think so, since those need to be allocated with the pacas and
limited to the maximum possible CPUs, which is discovered by the kernel
much later.

The first cell of /proc/device-tree/cpus/ibm,drc-indexes is the number 
of cores, it is there when RTAS is instantiated, we know SMT after 
"ibm,client-architecture-support" (if I remember correctly).

But maybe I misunderstand what you're suggesting.

Usually it is me missing the bigger picture :)


-- 
Alexey

On 16/01/2021 02:56, Nathan Lynch wrote:

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 332e1000ca0f..1aa7ab1cbc84 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -19,8 +19,11 @@
   #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)
+/* Work areas shared with RTAS must be 4K, naturally aligned. */

Why exactly 4K and not (for example) PAGE_SIZE?

4K is a platform requirement and isn't related to Linux's configured
page size. See the PAPR specification for RTAS functions such as
ibm,configure-connector, ibm,update-nodes, ibm,update-properties.

Good, since we are documenting things here - add to the comment ("per 
PAPR")?

There are other calls with work area parameters where alignment isn't
specified (e.g. ibm,get-system-parameter) but 4KB alignment is a safe
choice for those.

quoted

quoted

+#define RTAS_WORK_AREA_SIZE   4096
+
+/* Work areas allocated for user space access. */
+#define RTAS_USER_REGION_SIZE (RTAS_WORK_AREA_SIZE * 16)

This is still 64K but no clarity why. There is 16 of something, what
is it?

There are 16 4KB work areas in the region. I can name it
RTAS_NR_USER_WORK_AREAS or similar.

Why 16? PAPR (then add "per PAPR") or we just like 16 ("should be enough")?


-- 
Alexey

Nathan Lynch [off-list ref] writes:

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB.

Why does the size of the first memory block matter for radix?

The 1GB limit is sufficient to make it accessible by 32-bit code.

(512MB is a common size of the first memory block according to a small sample of
LPARs I have checked.)

That's the minimum we request, see prom_init.c:

	/* option vector 2: Open Firmware options supported */
	.vec2 = {
		.byte1 = OV2_REAL_MODE,
		.reserved = 0,
		.real_base = cpu_to_be32(0xffffffff),
		.real_size = cpu_to_be32(0xffffffff),
		.virt_base = cpu_to_be32(0xffffffff),
		.virt_size = cpu_to_be32(0xffffffff),
		.load_base = cpu_to_be32(0xffffffff),
		.min_rma = cpu_to_be32(512),		/* 512MB min RMA */

Since v4.12 in 2017.

cheers

Michael Ellerman [off-list ref] writes:

Nathan Lynch [off-list ref] writes:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB.

Why does the size of the first memory block matter for radix?

Here is my understanding: in the platform architecture, the size of the
first memory block equals the RMA, regardless of the MMU mode. It just
so happens that when using radix, Linux can pass ibm,configure-connector
a work area address outside of the RMA because the allocation
constraints for the work area are computed differently. It would be
wrong of the OS to pass RTAS arguments outside of this region with hash
MMU as well.

The 1GB limit is sufficient to make it accessible by 32-bit code.

But the requirement is that memory arguments passed to RTAS reside in
the RMA, which may be less than 1GB.

quoted

(512MB is a common size of the first memory block according to a small sample of
LPARs I have checked.)

That's the minimum we request, see prom_init.c:

	/* option vector 2: Open Firmware options supported */
	.vec2 = {
		.byte1 = OV2_REAL_MODE,
		.reserved = 0,
		.real_base = cpu_to_be32(0xffffffff),
		.real_size = cpu_to_be32(0xffffffff),
		.virt_base = cpu_to_be32(0xffffffff),
		.virt_size = cpu_to_be32(0xffffffff),
		.load_base = cpu_to_be32(0xffffffff),
		.min_rma = cpu_to_be32(512),		/* 512MB min RMA */

Since v4.12 in 2017.

Thanks for the pointer, I had been trying to find this without success.

Alexey Kardashevskiy [off-list ref] writes:

On 16/01/2021 02:38, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add
extra 64K in prom_instantiate_rtas() and advertise this address
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do
not need this RMO area before that point.

Can you explain more about what advantage that would bring? I'm not
seeing it. It's a more significant change than what I've written
here.

We already allocate space for RTAS and (like RMO) it needs to be in RMA, 
and RMO is useless without RTAS. We can reuse RTAS allocation code for 
RMO like this:

When you say RMO I assume you are referring to rtas_rmo_buf? (I don't
think it is well-named.)

One page for RTAS? RTAS is ~20MB on LPARs I've checked:

# lsprop /proc/device-tree/rtas/{rtas-size,linux,rtas-base}
/proc/device-tree/rtas/rtas-size
		 01370000 (20381696)

+       base = alloc_down(size, PAGE_SIZE + PAGE_SIZE, 0);

This changes the alignment but not the size of the allocation.

I think this would overlay the user region on top of the RTAS private
data area, allowing user space to corrupt it.

Implementation details aside... I'll grant that combining the
allocations into one in prom_init reduces some duplication in the sense
that both are subject to the same constraints (mostly - the RTAS data
area must not cross a 256MB boundary, while the user region may). But
they really are distinct concerns. The RTAS private data area is
specified in the platform architecture, the OS is obligated to allocate
it and pass it to instantiate-rtas, etc etc. However the user region
(rtas_rmo_buf) is purely a Linux construct which is there to support
sys_rtas.

Now, there are multiple sites in the kernel proper that must allocate
memory suitable for passing to RTAS. Obviously there is value in
consolidating the logic for that purpose in one place, so I'll work on
adding that in v2. OK?

Alexey Kardashevskiy [off-list ref] writes:

On 16/01/2021 02:56, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 332e1000ca0f..1aa7ab1cbc84 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -19,8 +19,11 @@
   #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)
+/* Work areas shared with RTAS must be 4K, naturally aligned. */

Why exactly 4K and not (for example) PAGE_SIZE?

4K is a platform requirement and isn't related to Linux's configured
page size. See the PAPR specification for RTAS functions such as
ibm,configure-connector, ibm,update-nodes, ibm,update-properties.

Good, since we are documenting things here - add to the comment ("per 
PAPR")?

But almost every constant in this header relates to a specification or
requirement in PAPR.

quoted

There are other calls with work area parameters where alignment isn't
specified (e.g. ibm,get-system-parameter) but 4KB alignment is a safe
choice for those.

quoted

quoted

+#define RTAS_WORK_AREA_SIZE   4096
+
+/* Work areas allocated for user space access. */
+#define RTAS_USER_REGION_SIZE (RTAS_WORK_AREA_SIZE * 16)

This is still 64K but no clarity why. There is 16 of something, what
is it?

There are 16 4KB work areas in the region. I can name it
RTAS_NR_USER_WORK_AREAS or similar.

Why 16? PAPR (then add "per PAPR") or we just like 16 ("should be
enough")?

PAPR doesn't know anything about the user region; it's a Linux
construct. It's been 64KB since pre-git days and I'm not sure what the
original reason is. At this point, maintaining a kernel-user ABI seems
like enough justification for the value.

On 20/01/2021 11:39, Nathan Lynch wrote:

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 16/01/2021 02:38, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add
extra 64K in prom_instantiate_rtas() and advertise this address
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do
not need this RMO area before that point.

Can you explain more about what advantage that would bring? I'm not
seeing it. It's a more significant change than what I've written
here.

We already allocate space for RTAS and (like RMO) it needs to be in RMA,
and RMO is useless without RTAS. We can reuse RTAS allocation code for
RMO like this:

When you say RMO I assume you are referring to rtas_rmo_buf? (I don't
think it is well-named.)

quoted

===

diff --git a/arch/powerpc/kernel/prom_init.c

b/arch/powerpc/kernel/prom_init.c
index e9d4eb6144e1..d9527d3e01d2 100644

--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -1821,7 +1821,8 @@ static void __init prom_instantiate_rtas(void)
          if (size == 0)
                  return;

-       base = alloc_down(size, PAGE_SIZE, 0);
+       /* One page for RTAS, one for RMO */

One page for RTAS? RTAS is ~20MB on LPARs I've checked:

# lsprop /proc/device-tree/rtas/{rtas-size,linux,rtas-base}
/proc/device-tree/rtas/rtas-size
		 01370000 (20381696)

You are right, I did not sleep well when replied, sorry about that :) I 
tried it with KVM where RTAS is just a few KBs (20 constant bytes + MCE 
log, depends on cpu number) so it worked for me.

quoted

+       base = alloc_down(size, PAGE_SIZE + PAGE_SIZE, 0);

This changes the alignment but not the size of the allocation.

Should be:

base = alloc_down(ALIGN_UP(size, PAGE_SIZE) + PAGE_SIZE, PAGE_SIZE, 0);

quoted

          if (base == 0)
                  prom_panic("Could not allocate memory for RTAS\n");

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

@@ -1186,6 +1186,7 @@ void __init rtas_initialize(void)
          rtas.size = size;
          no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry",

&entry);
          rtas.entry = no_entry ? rtas.base : entry;
+       rtas_rmo_buf = rtas.base + PAGE_SIZE;

I think this would overlay the user region on top of the RTAS private
data area, allowing user space to corrupt it.

Right, my bad. Should be:

rtas_rmo_buf = ALIGN_UP(rtas.base + rtas.size, PAGE_SIZE);

quoted

          /* If RTAS was found, allocate the RMO buffer for it and look for
           * the stop-self token if any

@@ -1196,11 +1197,6 @@ void __init rtas_initialize(void)
                  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);
-       if (!rtas_rmo_buf)
-               panic("ERROR: RTAS: Failed to allocate %lx bytes below

%pa\n",
-                     PAGE_SIZE, &rtas_region);
===

May be store in the FDT as "linux,rmo-base" next to "linux,rtas-base",
for clarity, as sharing symbols between prom and main kernel is a bit
tricky.

The benefit is that we do not do the same thing   (== find 64K in RMA)
in 2 different ways and if the RMO allocated my way is broken - we'll
know it much sooner as RTAS itself will break too.

Implementation details aside... I'll grant that combining the
allocations into one in prom_init reduces some duplication in the sense
that both are subject to the same constraints (mostly - the RTAS data
area must not cross a 256MB boundary, while the user region may). But
they really are distinct concerns. The RTAS private data area is
specified in the platform architecture, the OS is obligated to allocate
it and pass it to instantiate-rtas, etc etc. However the user region
(rtas_rmo_buf) is purely a Linux construct which is there to support
sys_rtas.

Not purely - it should be an address which RTAS accepts. Cannot argue 
with the rest though, it all sounds correct.

Now, there are multiple sites in the kernel proper that must allocate
memory suitable for passing to RTAS. Obviously there is value in
consolidating the logic for that purpose in one place, so I'll work on
adding that in v2. OK?

Sure.


-- 
Alexey

On 20/01/2021 12:17, Nathan Lynch wrote:

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 16/01/2021 02:56, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 332e1000ca0f..1aa7ab1cbc84 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -19,8 +19,11 @@
    #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)
+/* Work areas shared with RTAS must be 4K, naturally aligned. */

Why exactly 4K and not (for example) PAGE_SIZE?

4K is a platform requirement and isn't related to Linux's configured
page size. See the PAPR specification for RTAS functions such as
ibm,configure-connector, ibm,update-nodes, ibm,update-properties.

Good, since we are documenting things here - add to the comment ("per
PAPR")?

But almost every constant in this header relates to a specification or
requirement in PAPR.

Yup, "almost".

quoted

quoted

There are other calls with work area parameters where alignment isn't
specified (e.g. ibm,get-system-parameter) but 4KB alignment is a safe
choice for those.

quoted

quoted

+#define RTAS_WORK_AREA_SIZE   4096
+
+/* Work areas allocated for user space access. */
+#define RTAS_USER_REGION_SIZE (RTAS_WORK_AREA_SIZE * 16)

This is still 64K but no clarity why. There is 16 of something, what
is it?

There are 16 4KB work areas in the region. I can name it
RTAS_NR_USER_WORK_AREAS or similar.

Why 16? PAPR (then add "per PAPR") or we just like 16 ("should be
enough")?

PAPR doesn't know anything about the user region; it's a Linux
construct. It's been 64KB since pre-git days and I'm not sure what the
original reason is. At this point, maintaining a kernel-user ABI seems
like enough justification for the value.

I am not arguing keeping the numbers but you are replacing one magic 
number with another and for neither it is horribly obvious where they 
came from. Is 16 the max number of concurrently running sys_rtas system 
calls? Does the userspace ensure there is no more than 16? btw where is 
that userspace code? I thought 
https://github.com/power-ras/ppc64-diag.git but no. Thanks,



-- 
Alexey

Nathan Lynch [off-list ref] writes:

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 16/01/2021 02:38, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add
extra 64K in prom_instantiate_rtas() and advertise this address
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do
not need this RMO area before that point.

Can you explain more about what advantage that would bring? I'm not
seeing it. It's a more significant change than what I've written
here.

We already allocate space for RTAS and (like RMO) it needs to be in RMA, 
and RMO is useless without RTAS. We can reuse RTAS allocation code for 
RMO like this:

When you say RMO I assume you are referring to rtas_rmo_buf? (I don't
think it is well-named.)

...

RMO (Real mode offset) is the old term we used to use to refer to what
is now called the RMA (Real mode area). There are still many references
to RMO in Linux, but they almost certainly all refer to what we now call
the RMA.

quoted

May be store in the FDT as "linux,rmo-base" next to "linux,rtas-base", 
for clarity, as sharing symbols between prom and main kernel is a bit 
tricky.

The benefit is that we do not do the same thing   (== find 64K in RMA) 
in 2 different ways and if the RMO allocated my way is broken - we'll 
know it much sooner as RTAS itself will break too.

Implementation details aside... I'll grant that combining the
allocations into one in prom_init reduces some duplication in the sense
that both are subject to the same constraints (mostly - the RTAS data
area must not cross a 256MB boundary, while the user region may). But
they really are distinct concerns. The RTAS private data area is
specified in the platform architecture, the OS is obligated to allocate
it and pass it to instantiate-rtas, etc etc. However the user region
(rtas_rmo_buf) is purely a Linux construct which is there to support
sys_rtas.

Now, there are multiple sites in the kernel proper that must allocate
memory suitable for passing to RTAS. Obviously there is value in
consolidating the logic for that purpose in one place, so I'll work on
adding that in v2. OK?

I don't think we want to move any allocations into prom_init.c unless we
have to.

It's best thought of as a trampoline, that runs before the kernel
proper, to transition from live OF to a flat DT environment. One thing
that must be done as part of that is instantiating RTAS, because it's
basically a runtime copy of the live OF. But any other allocs are for
Linux to handle later, IMHO.

cheers

Nathan Lynch [off-list ref] writes:

Michael Ellerman [off-list ref] writes:

quoted

Nathan Lynch [off-list ref] writes:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB.

Why does the size of the first memory block matter for radix?

Here is my understanding: in the platform architecture, the size of the
first memory block equals the RMA, regardless of the MMU mode. It just
so happens that when using radix, Linux can pass ibm,configure-connector
a work area address outside of the RMA because the allocation
constraints for the work area are computed differently. It would be
wrong of the OS to pass RTAS arguments outside of this region with hash
MMU as well.

If that's the requirement then shouldn't we be adjusting ppc64_rma_size?
Otherwise aren't other uses of ppc64_rma_size going to run into similar
problems.

Or does the RMA only apply for RTAS calls when using radix?

cheers

Michael Ellerman [off-list ref] writes:

Nathan Lynch [off-list ref] writes:

quoted

Michael Ellerman [off-list ref] writes:

quoted

Nathan Lynch [off-list ref] writes:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB.

Why does the size of the first memory block matter for radix?

Here is my understanding: in the platform architecture, the size of the
first memory block equals the RMA, regardless of the MMU mode. It just
so happens that when using radix, Linux can pass ibm,configure-connector
a work area address outside of the RMA because the allocation
constraints for the work area are computed differently. It would be
wrong of the OS to pass RTAS arguments outside of this region with hash
MMU as well.

If that's the requirement then shouldn't we be adjusting ppc64_rma_size?
Otherwise aren't other uses of ppc64_rma_size going to run into similar
problems.

Not all allocations limited by ppc64_rma_size set up memory that is
passed to RTAS though, do they? e.g. emergency_stack_init and
init_fallback_flush? Those shouldn't be confined to the first LMB
unnecessarily.

That's why I'm thinking what I've written here should be generalized a
bit and placed in an early allocator function that can be used to set up
the user region and the per-cpu reentrant RTAS argument buffers
(see allocate_paca_ptrs/new_rtas_args). So far those two sites are the
only ones I'm convinced need attention.

Alexey Kardashevskiy [off-list ref] writes:

On 20/01/2021 12:17, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 16/01/2021 02:56, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

+#define RTAS_WORK_AREA_SIZE   4096
+
+/* Work areas allocated for user space access. */
+#define RTAS_USER_REGION_SIZE (RTAS_WORK_AREA_SIZE * 16)

This is still 64K but no clarity why. There is 16 of something, what
is it?

There are 16 4KB work areas in the region. I can name it
RTAS_NR_USER_WORK_AREAS or similar.

Why 16? PAPR (then add "per PAPR") or we just like 16 ("should be
enough")?

PAPR doesn't know anything about the user region; it's a Linux
construct. It's been 64KB since pre-git days and I'm not sure what the
original reason is. At this point, maintaining a kernel-user ABI seems
like enough justification for the value.

I am not arguing keeping the numbers but you are replacing one magic 
number with another and for neither it is horribly obvious where they 
came from.

When I wrote it I viewed it as changing one of the factors in (64 *
1024) to a named constant that better expresses how the region is used
and adjusting the remaining factor to arrive at the same end result. I
considered it a net improvement even if we're not sure how 64K was
arrived at in the first place, although I suspect it was chosen to
support multiple concurrent users, and to be compatible with both 4K
and 64K page sizes. Then again 64K pages came a bit after this was
introduced.

The change that introduced RTAS_RMOBUF_MAX (here renamed to
RTAS_USER_REGION_SIZE) does not explain how the value was derived:

================
Author: Andrew Morton [off-list ref]
Date:   Sun Jan 18 18:17:30 2004 -0800

    [PATCH] ppc64: add rtas syscall, from John Rose
    
    From: Anton Blanchard [off-list ref]
    
    Added RTAS syscall.  Reserved lowmem rtas_rmo_buf for userspace use.  Created
    "rmo_buffer" proc file to export bounds of rtas_rmo_buf.

[...]

diff --git a/include/asm-ppc64/rtas.h b/include/asm-ppc64/rtas.h
index 42a0b484077c..d9e426161044 100644
--- a/include/asm-ppc64/rtas.h
+++ b/include/asm-ppc64/rtas.h

@@ -19,6 +19,9 @@
 #define RTAS_UNKNOWN_SERVICE (-1)
 #define RTAS_INSTANTIATE_MAX (1UL<<30) /* Don't instantiate rtas at/above this value */
 
+/* Buffer size for ppc_rtas system call. */
+#define RTAS_RMOBUF_MAX (64 * 1024)
+

================

The comment "Buffer size for ppc_rtas system call" (removed by my
change) is not really appropriate because 1. not all sys_rtas
invocations use the buffer, and 2. no callers use the entire buffer.

Is 16 the max number of concurrently running sys_rtas system 
calls? Does the userspace ensure there is no more than 16?

No and no; not all calls to sys_rtas need to use a work area. However,
librtas uses record locking to arbitrate access to the user region, and
the unit of allocation is 4KB. This is a reasonable choice: many RTAS
calls which take a work area require 4KB alignment. But some do not
(ibm,get-system-parameter), and librtas conceivably could be made to
perform finer-grained allocations.

It's not the kernel's concern how librtas partitions the user region, so
I'm inclined to leave the (64 * 1024) expression alone now. Thanks for
your review.

btw where is that userspace code? I thought
https://github.com/power-ras/ppc64-diag.git but no. Thanks,

librtas, of which ppc64-diag and powerpc-utils are users:

https://github.com/ibm-power-utilities/librtas

Michael Ellerman [off-list ref] writes:

Nathan Lynch [off-list ref] writes:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 16/01/2021 02:38, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add
extra 64K in prom_instantiate_rtas() and advertise this address
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do
not need this RMO area before that point.

Can you explain more about what advantage that would bring? I'm not
seeing it. It's a more significant change than what I've written
here.

We already allocate space for RTAS and (like RMO) it needs to be in RMA, 
and RMO is useless without RTAS. We can reuse RTAS allocation code for 
RMO like this:

When you say RMO I assume you are referring to rtas_rmo_buf? (I don't
think it is well-named.)

...

RMO (Real mode offset) is the old term we used to use to refer to what
is now called the RMA (Real mode area). There are still many references
to RMO in Linux, but they almost certainly all refer to what we now call
the RMA.

Yes... but I think in this discussion Alexey was using RMO to stand in
for rtas_rmo_buf, which was what I was trying to clarify.

quoted

quoted

May be store in the FDT as "linux,rmo-base" next to "linux,rtas-base", 
for clarity, as sharing symbols between prom and main kernel is a bit 
tricky.

The benefit is that we do not do the same thing   (== find 64K in RMA) 
in 2 different ways and if the RMO allocated my way is broken - we'll 
know it much sooner as RTAS itself will break too.

Implementation details aside... I'll grant that combining the
allocations into one in prom_init reduces some duplication in the sense
that both are subject to the same constraints (mostly - the RTAS data
area must not cross a 256MB boundary, while the user region may). But
they really are distinct concerns. The RTAS private data area is
specified in the platform architecture, the OS is obligated to allocate
it and pass it to instantiate-rtas, etc etc. However the user region
(rtas_rmo_buf) is purely a Linux construct which is there to support
sys_rtas.

Now, there are multiple sites in the kernel proper that must allocate
memory suitable for passing to RTAS. Obviously there is value in
consolidating the logic for that purpose in one place, so I'll work on
adding that in v2. OK?

I don't think we want to move any allocations into prom_init.c unless we
have to.

It's best thought of as a trampoline, that runs before the kernel
proper, to transition from live OF to a flat DT environment. One thing
that must be done as part of that is instantiating RTAS, because it's
basically a runtime copy of the live OF. But any other allocs are for
Linux to handle later, IMHO.

Agreed.

On 22/01/2021 02:27, Nathan Lynch wrote:

Michael Ellerman [off-list ref] writes:

quoted

Nathan Lynch [off-list ref] writes:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 16/01/2021 02:38, Nathan Lynch wrote:

quoted

Alexey Kardashevskiy [off-list ref] writes:

quoted

On 15/01/2021 09:00, Nathan Lynch wrote:

quoted

Memory locations passed as arguments from the OS to RTAS usually need
to be addressable in 32-bit mode and must reside in the Real Mode
Area. On PAPR guests, the RMA starts at logical address 0 and is the
first logical memory block reported in the LPAR’s device tree.

On powerpc targets with RTAS, Linux makes available to user space a
region of memory suitable for arguments to be passed to RTAS via
sys_rtas(). This region (rtas_rmo_buf) is allocated via the memblock
API during boot in order to ensure that it satisfies the requirements
described above.

With radix MMU, the upper limit supplied to the memblock allocation
can exceed the bounds of the first logical memory block, since
ppc64_rma_size is ULONG_MAX and RTAS_INSTANTIATE_MAX is 1GB. (512MB is
a common size of the first memory block according to a small sample of
LPARs I have checked.) This leads to failures when user space invokes
an RTAS function that uses a work area, such as
ibm,configure-connector.

Alter the determination of the upper limit for rtas_rmo_buf's
allocation to consult the device tree directly, ensuring placement
within the RMA regardless of the MMU in use.

Can we tie this with RTAS (which also needs to be in RMA) and simply add
extra 64K in prom_instantiate_rtas() and advertise this address
(ALIGH_UP(rtas-base + rtas-size, PAGE_SIZE)) to the user space? We do
not need this RMO area before that point.

Can you explain more about what advantage that would bring? I'm not
seeing it. It's a more significant change than what I've written
here.

We already allocate space for RTAS and (like RMO) it needs to be in RMA,
and RMO is useless without RTAS. We can reuse RTAS allocation code for
RMO like this:

When you say RMO I assume you are referring to rtas_rmo_buf? (I don't
think it is well-named.)

...

RMO (Real mode offset) is the old term we used to use to refer to what
is now called the RMA (Real mode area). There are still many references
to RMO in Linux, but they almost certainly all refer to what we now call
the RMA.

Yes... but I think in this discussion Alexey was using RMO to stand in
for rtas_rmo_buf, which was what I was trying to clarify.

Correct. Thanks for the clarification, appreciated.

quoted

quoted

quoted

May be store in the FDT as "linux,rmo-base" next to "linux,rtas-base",
for clarity, as sharing symbols between prom and main kernel is a bit
tricky.

The benefit is that we do not do the same thing   (== find 64K in RMA)
in 2 different ways and if the RMO allocated my way is broken - we'll
know it much sooner as RTAS itself will break too.

Implementation details aside... I'll grant that combining the
allocations into one in prom_init reduces some duplication in the sense
that both are subject to the same constraints (mostly - the RTAS data
area must not cross a 256MB boundary, while the user region may). But
they really are distinct concerns. The RTAS private data area is
specified in the platform architecture, the OS is obligated to allocate
it and pass it to instantiate-rtas, etc etc. However the user region
(rtas_rmo_buf) is purely a Linux construct which is there to support
sys_rtas.

Now, there are multiple sites in the kernel proper that must allocate
memory suitable for passing to RTAS. Obviously there is value in
consolidating the logic for that purpose in one place, so I'll work on
adding that in v2. OK?

I don't think we want to move any allocations into prom_init.c unless we
have to.

It's best thought of as a trampoline, that runs before the kernel
proper, to transition from live OF to a flat DT environment. One thing
that must be done as part of that is instantiating RTAS, because it's
basically a runtime copy of the live OF. But any other allocs are for
Linux to handle later, IMHO.

Agreed.

Then the only comment I have left is may be use of_address_to_resource() 
+ resource_size() instead of of_n_addr_cells()/of_n_size_cells() (like 
pseries_memory_block_size()). And now I shut up :) Thanks,


-- 
Alexey