Re: [PATCH v3 00/10] powerpc/pseries: New character devices for system parameters and VPD

2023-11-13 Thread Nathan Lynch
Michal Suchánek  writes:
> What's the status here?
>
> Can this move on with the 4th patch skipped, or is new revision
> expected?

I would like to get some feedback on the idea for coarse per-function
locking in patch #2 "Facilitate high-level call sequences" since that is
a core change to the RTAS subsystem and new in this revision. But I
intend to send a new version (without a boot bug) this week regardless.


Re: [PATCH v3 00/10] powerpc/pseries: New character devices for system parameters and VPD

2023-11-13 Thread Michal Suchánek
Hello,

On Thu, Oct 26, 2023 at 06:56:36PM -0500, Nathan Lynch wrote:
> Nathan Lynch via B4 Relay 
> writes:
> > I have made changes to librtas to prefer the new interfaces and
> > verified that existing clients work correctly with the new code.
> 
> Unfortunately I made a mistake in testing this time and introduced a
> boot-time oops:
> 
> BUG: Kernel NULL pointer dereference on read at 0x0018
> Faulting instruction address: 0xc004223c
> Oops: Kernel access of bad area, sig: 7 [#1]
> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Tainted: GW  6.6.0-rc2+ #129
> NIP:  c004223c LR: c0042238 CTR: 
> REGS: c2c579d0 TRAP: 0300   Tainted: GW   (6.6.0-rc2+)
> MSR:  80001033   CR: 28000284  XER: 
> CFAR: c0042008 DAR: 0018 DSISR: 0008 IRQMASK: 3 
> GPR00: c0042238 c2c57c70 c1f5eb00  
> GPR04: c294cd08 0002 c2c579b4  
> GPR08:  0002 c2c0da80  
> GPR12:  c5e4  02097728 
> GPR16:  0001 02097b80 020975b8 
> GPR20: 020976f0 020974e8 030feb00 030feb00 
> GPR24: 2008  0001 c28f3d70 
> GPR28: 02d31020 c2cac268 c2d31020  
> NIP [c004223c] do_enter_rtas+0xcc/0x460
> LR [c0042238] do_enter_rtas+0xc8/0x460
> Call Trace:
> [c2c57c70] [c0042238] do_enter_rtas+0xc8/0x460 (unreliable)
> [c2c57cc0] [c0042e34] rtas_call+0x434/0x490
> [c2c57d20] [c00fd584] papr_sysparm_get+0xe4/0x230
> [c2c57db0] [c20267d0] pSeries_probe+0x2f0/0x5fc
> [c2c57e80] [c200a318] setup_arch+0x11c/0x524
> [c2c57f10] [c200418c] start_kernel+0xcc/0xc1c
> [c2c57fe0] [c000e788] start_here_common+0x1c/0x20
> 
> This was introduced by patch #4 "powerpc/rtas: Warn if per-function lock
> isn't held": __do_enter_rtas() is now attempting token -> descriptor
> lookups unconditionally, before the xarray for that has been initialized.
> 
> With that change reverted, the series tests OK.

What's the status here?

Can this move on with the 4th patch skipped, or is new revision
expected?

Thanks

Michal


Re: [PATCH v3 00/10] powerpc/pseries: New character devices for system parameters and VPD

2023-10-26 Thread Nathan Lynch
Nathan Lynch via B4 Relay 
writes:
> I have made changes to librtas to prefer the new interfaces and
> verified that existing clients work correctly with the new code.

Unfortunately I made a mistake in testing this time and introduced a
boot-time oops:

BUG: Kernel NULL pointer dereference on read at 0x0018
Faulting instruction address: 0xc004223c
Oops: Kernel access of bad area, sig: 7 [#1]
LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Tainted: GW  6.6.0-rc2+ #129
NIP:  c004223c LR: c0042238 CTR: 
REGS: c2c579d0 TRAP: 0300   Tainted: GW   (6.6.0-rc2+)
MSR:  80001033   CR: 28000284  XER: 
CFAR: c0042008 DAR: 0018 DSISR: 0008 IRQMASK: 3 
GPR00: c0042238 c2c57c70 c1f5eb00  
GPR04: c294cd08 0002 c2c579b4  
GPR08:  0002 c2c0da80  
GPR12:  c5e4  02097728 
GPR16:  0001 02097b80 020975b8 
GPR20: 020976f0 020974e8 030feb00 030feb00 
GPR24: 2008  0001 c28f3d70 
GPR28: 02d31020 c2cac268 c2d31020  
NIP [c004223c] do_enter_rtas+0xcc/0x460
LR [c0042238] do_enter_rtas+0xc8/0x460
Call Trace:
[c2c57c70] [c0042238] do_enter_rtas+0xc8/0x460 (unreliable)
[c2c57cc0] [c0042e34] rtas_call+0x434/0x490
[c2c57d20] [c00fd584] papr_sysparm_get+0xe4/0x230
[c2c57db0] [c20267d0] pSeries_probe+0x2f0/0x5fc
[c2c57e80] [c200a318] setup_arch+0x11c/0x524
[c2c57f10] [c200418c] start_kernel+0xcc/0xc1c
[c2c57fe0] [c000e788] start_here_common+0x1c/0x20

This was introduced by patch #4 "powerpc/rtas: Warn if per-function lock
isn't held": __do_enter_rtas() is now attempting token -> descriptor
lookups unconditionally, before the xarray for that has been initialized.

With that change reverted, the series tests OK.


[PATCH v3 00/10] powerpc/pseries: New character devices for system parameters and VPD

2023-10-25 Thread Nathan Lynch via B4 Relay
Add character devices that expose PAPR-specific system parameters and
VPD to user space.

The problem: important platform features are enabled on Linux VMs
through the powerpc-specific rtas() syscall in combination with
writeable mappings of /dev/mem. In typical usage, this is encapsulated
behind APIs provided by the librtas library. This paradigm is
incompatible with lockdown, which prohibits /dev/mem access. It also
is too low-level in many cases: a single logical operation may require
multiple sys_rtas() calls in succession to complete. This carries the
risk that a process may exit while leaving an operation unfinished. It
also means that callers must coordinate their use of the syscall for
functions that cannot tolerate multiple concurrent clients, such as
ibm,get-vpd.

The solution presented here is to add a pair of small pseries-specific
"drivers," one for VPD and one for system parameters. The new drivers
expose these facilities to user space in ways that are compatible with
lockdown and require no coordination between their clients.

Since the ibm,get-vpd call sequence performed by the papr-vpd driver
must be serialized against all other uses of the function, the series
begins by adding some new APIs to the core RTAS support code for this
purpose.

Both drivers could potentially support poll() methods to notify
clients of changes to parameters or VPD that happen due to partition
migration and other events. But that should be safe to leave for
later, assuming there's any interest.

I have made changes to librtas to prefer the new interfaces and
verified that existing clients work correctly with the new code. A
draft PR for that work is here:

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

The user-space ABI has not changed since v1 of this series.

I expect to propose at least one more small driver in this style for
platform dump retrieval in a separate submission in the future.

---
Changes in v3:
- Add new rtas_function_lock()/unlock() APIs and convert existing code
  to use them.
- Convert papr-vpd to use rtas_function_lock()/unlock() instead of
  having sys_rtas() obtain a driver-private mutex.
- Rebase on current powerpc/next.
- Link to v2: 
https://lore.kernel.org/r/20231013-papr-sys_rtas-vs-lockdown-v2-0-ead01ce01...@linux.ibm.com

Changes in v2:
- Fix unused-but-set variable warning in papr-sysparm code.
- Rebase on powerpc/next branch.
- Link to v1: 
https://lore.kernel.org/r/20231006-papr-sys_rtas-vs-lockdown-v1-0-3a36bfb66...@linux.ibm.com

Changes in v1 vs initial RFC:
- Add papr-sysparm driver and tests.
- Add a papr-miscdev.h uapi header.
- Prevent sys_rtas() from interfering with papr-vpd call sequences.
- Handle -4 ("VPD changed") status in papr-vpd.
- Include string_helpers.h in papr-vpd.c, per Michal Suchánek
- Link to RFC: 
https://lore.kernel.org/r/20230822-papr-sys_rtas-vs-lockdown-v1-0-932623cf3...@linux.ibm.com

---
Nathan Lynch (10):
  powerpc/rtas: Factor out function descriptor lookup
  powerpc/rtas: Facilitate high-level call sequences
  powerpc/rtas: Serialize firmware activation sequences
  powerpc/rtas: Warn if per-function lock isn't held
  powerpc/uapi: Export papr-miscdev.h header
  powerpc/pseries: Add papr-vpd character driver for VPD retrieval
  powerpc/pseries/papr-sysparm: Validate buffer object lengths
  powerpc/pseries/papr-sysparm: Expose character device to user space
  powerpc/selftests: Add test for papr-vpd
  powerpc/selftests: Add test for papr-sysparm

 Documentation/userspace-api/ioctl/ioctl-number.rst |   4 +
 arch/powerpc/include/asm/papr-sysparm.h|  17 +-
 arch/powerpc/include/asm/rtas.h|   2 +
 arch/powerpc/include/uapi/asm/papr-miscdev.h   |   9 +
 arch/powerpc/include/uapi/asm/papr-sysparm.h   |  58 +++
 arch/powerpc/include/uapi/asm/papr-vpd.h   |  22 +
 arch/powerpc/kernel/rtas.c | 157 ++-
 arch/powerpc/platforms/pseries/Makefile|   1 +
 arch/powerpc/platforms/pseries/papr-sysparm.c  | 201 +++-
 arch/powerpc/platforms/pseries/papr-vpd.c  | 522 +
 tools/testing/selftests/powerpc/Makefile   |   2 +
 .../selftests/powerpc/papr_sysparm/.gitignore  |   1 +
 .../selftests/powerpc/papr_sysparm/Makefile|  12 +
 .../selftests/powerpc/papr_sysparm/papr_sysparm.c  | 164 +++
 .../testing/selftests/powerpc/papr_vpd/.gitignore  |   1 +
 tools/testing/selftests/powerpc/papr_vpd/Makefile  |  12 +
 .../testing/selftests/powerpc/papr_vpd/papr_vpd.c  | 352 ++
 17 files changed, 1503 insertions(+), 34 deletions(-)
---
base-commit: 36e826b568e412f61d68fedc02a67b4d8b7583cc
change-id: 20230817-papr-sys_rtas-vs-lockdown-5c54505db792

Best regards,
-- 
Nathan Lynch