Re: [PATCH 5/6] powerpc/rtas: rename RTAS_RMOBUF_MAX to RTAS_USER_REGION_SIZE

[Nathan Lynch <nathanl@linux.ibm.com>]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> On 20/01/2021 12:17, Nathan Lynch wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>> On 16/01/2021 02:56, Nathan Lynch wrote:
>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>>> On 15/01/2021 09:00, Nathan Lynch wrote:
>>>>>> +#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 <akpm@osdl.org>
Date:   Sun Jan 18 18:17:30 2004 -0800

    [PATCH] ppc64: add rtas syscall, from John Rose
    
    From: Anton Blanchard <anton@samba.org>
    
    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