[PATCH] powerpc/rtas: use correct function name for resetting TCE tables

[PATCH] powerpc/rtas: use correct function name for resetting TCE tables

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathanl@linux.ibm.com>

The PAPR spec spells the function name as

  "ibm,reset-pe-dma-windows"

but in practice firmware uses the singular form:

  "ibm,reset-pe-dma-window"

in the device tree. Since we have the wrong spelling in the RTAS
function table, reverse lookups (token -> name) fail and warn:

  unexpected failed lookup for token 86
  WARNING: CPU: 1 PID: 545 at arch/powerpc/kernel/rtas.c:659 __do_enter_rtas_trace+0x2a4/0x2b4
  CPU: 1 PID: 545 Comm: systemd-udevd Not tainted 6.8.0-rc4 #30
  Hardware name: IBM,9105-22A POWER10 (raw) 0x800200 0xf000006 of:IBM,FW1060.00 (NL1060_028) hv:phyp pSeries
  NIP [c0000000000417f0] __do_enter_rtas_trace+0x2a4/0x2b4
  LR [c0000000000417ec] __do_enter_rtas_trace+0x2a0/0x2b4
  Call Trace:
   __do_enter_rtas_trace+0x2a0/0x2b4 (unreliable)
   rtas_call+0x1f8/0x3e0
   enable_ddw.constprop.0+0x4d0/0xc84
   dma_iommu_dma_supported+0xe8/0x24c
   dma_set_mask+0x5c/0xd8
   mlx5_pci_init.constprop.0+0xf0/0x46c [mlx5_core]
   probe_one+0xfc/0x32c [mlx5_core]
   local_pci_probe+0x68/0x12c
   pci_call_probe+0x68/0x1ec
   pci_device_probe+0xbc/0x1a8
   really_probe+0x104/0x570
   __driver_probe_device+0xb8/0x224
   driver_probe_device+0x54/0x130
   __driver_attach+0x158/0x2b0
   bus_for_each_dev+0xa8/0x120
   driver_attach+0x34/0x48
   bus_add_driver+0x174/0x304
   driver_register+0x8c/0x1c4
   __pci_register_driver+0x68/0x7c
   mlx5_init+0xb8/0x118 [mlx5_core]
   do_one_initcall+0x60/0x388
   do_init_module+0x7c/0x2a4
   init_module_from_file+0xb4/0x108
   idempotent_init_module+0x184/0x34c
   sys_finit_module+0x90/0x114

And oopses are possible when lockdep is enabled or the RTAS
tracepoints are active, since those paths dereference the result of
the lookup.

Use the correct spelling to match firmware's behavior, adjusting the
related constants to match.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
Reported-by: Gaurav Batra <gbatra@linux.ibm.com>
Fixes: 8252b88294d2 ("powerpc/rtas: improve function information lookups")
---
 arch/powerpc/include/asm/rtas.h | 4 ++--
 arch/powerpc/kernel/rtas.c      | 9 +++++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9bb2210c8d44..065ffd1b2f8a 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -69,7 +69,7 @@ enum rtas_function_index {
 	RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE,
 	RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE2,
 	RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW,
-	RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS,
+	RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW,
 	RTAS_FNIDX__IBM_SCAN_LOG_DUMP,
 	RTAS_FNIDX__IBM_SET_DYNAMIC_INDICATOR,
 	RTAS_FNIDX__IBM_SET_EEH_OPTION,
@@ -164,7 +164,7 @@ typedef struct {
 #define RTAS_FN_IBM_READ_SLOT_RESET_STATE         rtas_fn_handle(RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE)
 #define RTAS_FN_IBM_READ_SLOT_RESET_STATE2        rtas_fn_handle(RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE2)
 #define RTAS_FN_IBM_REMOVE_PE_DMA_WINDOW          rtas_fn_handle(RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW)
-#define RTAS_FN_IBM_RESET_PE_DMA_WINDOWS          rtas_fn_handle(RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS)
+#define RTAS_FN_IBM_RESET_PE_DMA_WINDOW           rtas_fn_handle(RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW)
 #define RTAS_FN_IBM_SCAN_LOG_DUMP                 rtas_fn_handle(RTAS_FNIDX__IBM_SCAN_LOG_DUMP)
 #define RTAS_FN_IBM_SET_DYNAMIC_INDICATOR         rtas_fn_handle(RTAS_FNIDX__IBM_SET_DYNAMIC_INDICATOR)
 #define RTAS_FN_IBM_SET_EEH_OPTION                rtas_fn_handle(RTAS_FNIDX__IBM_SET_EEH_OPTION)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 7e793b503e29..8064d9c3de86 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[] __ro_after_init = {
 	[RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = {
 		.name = "ibm,remove-pe-dma-window",
 	},
-	[RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = {
-		.name = "ibm,reset-pe-dma-windows",
+	[RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = {
+		/*
+		 * Note: PAPR+ v2.13 7.3.31.4.1 spells this as
+		 * "ibm,reset-pe-dma-windows" (plural), but RTAS
+		 * implementations use the singular form in practice.
+		 */
+		.name = "ibm,reset-pe-dma-window",
 	},
 	[RTAS_FNIDX__IBM_SCAN_LOG_DUMP] = {
 		.name = "ibm,scan-log-dump",

---
base-commit: b22ea627225b53ec7ce25c19d6df9fa8217d1643
change-id: 20240222-rtas-fix-ibm-reset-pe-dma-window-745dd1824011

Best regards,
-- 
Nathan Lynch <nathanl@linux.ibm.com>

Re: [PATCH] powerpc/rtas: use correct function name for resetting TCE tables

[Michael Ellerman]

Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
writes:
> From: Nathan Lynch <nathanl@linux.ibm.com>
>
> The PAPR spec spells the function name as
>
>   "ibm,reset-pe-dma-windows"
>
> but in practice firmware uses the singular form:

Just to be clear, you're talking about IBM firmware on PowerVM machines.

>   "ibm,reset-pe-dma-window"
>
> in the device tree. Since we have the wrong spelling in the RTAS
> function table, reverse lookups (token -> name) fail and warn:
>
>   unexpected failed lookup for token 86
>   WARNING: CPU: 1 PID: 545 at arch/powerpc/kernel/rtas.c:659 __do_enter_rtas_trace+0x2a4/0x2b4
...
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 7e793b503e29..8064d9c3de86 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[] __ro_after_init = {
>  	[RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = {
>  		.name = "ibm,remove-pe-dma-window",
>  	},
> -	[RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = {
> -		.name = "ibm,reset-pe-dma-windows",
> +	[RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = {
> +		/*
> +		 * Note: PAPR+ v2.13 7.3.31.4.1 spells this as
> +		 * "ibm,reset-pe-dma-windows" (plural), but RTAS
> +		 * implementations use the singular form in practice.
> +		 */
> +		.name = "ibm,reset-pe-dma-window",

Qemu also spells it that way:

$ grep -C 12 ibm,reset-pe-dma-window hw/ppc/spapr_rtas_ddw.c
static void spapr_rtas_ddw_init(void)
{
    spapr_rtas_register(RTAS_IBM_QUERY_PE_DMA_WINDOW,
                        "ibm,query-pe-dma-window",
                        rtas_ibm_query_pe_dma_window);
    spapr_rtas_register(RTAS_IBM_CREATE_PE_DMA_WINDOW,
                        "ibm,create-pe-dma-window",
                        rtas_ibm_create_pe_dma_window);
    spapr_rtas_register(RTAS_IBM_REMOVE_PE_DMA_WINDOW,
                        "ibm,remove-pe-dma-window",
                        rtas_ibm_remove_pe_dma_window);
    spapr_rtas_register(RTAS_IBM_RESET_PE_DMA_WINDOW,
                        "ibm,reset-pe-dma-window",
                        rtas_ibm_reset_pe_dma_window);
}

There's no version in SLOF, it delegates to Qemu.

The old platforms that use RTAS won't implement this call at all, so
there's no issue with the naming there.

So LGTM.

cheers

Re: [PATCH] powerpc/rtas: use correct function name for resetting TCE tables

[Nathan Lynch]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
> writes:
>> From: Nathan Lynch <nathanl@linux.ibm.com>
>>
>> The PAPR spec spells the function name as
>>
>>   "ibm,reset-pe-dma-windows"
>>
>> but in practice firmware uses the singular form:
>
> Just to be clear, you're talking about IBM firmware on PowerVM
> machines.

Yes, although I did check Qemu too.

>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>> index 7e793b503e29..8064d9c3de86 100644
>> --- a/arch/powerpc/kernel/rtas.c
>> +++ b/arch/powerpc/kernel/rtas.c
>> @@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[] __ro_after_init = {
>>  	[RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = {
>>  		.name = "ibm,remove-pe-dma-window",
>>  	},
>> -	[RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = {
>> -		.name = "ibm,reset-pe-dma-windows",
>> +	[RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = {
>> +		/*
>> +		 * Note: PAPR+ v2.13 7.3.31.4.1 spells this as
>> +		 * "ibm,reset-pe-dma-windows" (plural), but RTAS
>> +		 * implementations use the singular form in practice.
>> +		 */
>> +		.name = "ibm,reset-pe-dma-window",
>
> Qemu also spells it that way:
>
> $ grep -C 12 ibm,reset-pe-dma-window hw/ppc/spapr_rtas_ddw.c
> static void spapr_rtas_ddw_init(void)
> {
>     spapr_rtas_register(RTAS_IBM_QUERY_PE_DMA_WINDOW,
>                         "ibm,query-pe-dma-window",
>                         rtas_ibm_query_pe_dma_window);
>     spapr_rtas_register(RTAS_IBM_CREATE_PE_DMA_WINDOW,
>                         "ibm,create-pe-dma-window",
>                         rtas_ibm_create_pe_dma_window);
>     spapr_rtas_register(RTAS_IBM_REMOVE_PE_DMA_WINDOW,
>                         "ibm,remove-pe-dma-window",
>                         rtas_ibm_remove_pe_dma_window);
>     spapr_rtas_register(RTAS_IBM_RESET_PE_DMA_WINDOW,
>                         "ibm,reset-pe-dma-window",
>                         rtas_ibm_reset_pe_dma_window);
> }
>
> There's no version in SLOF, it delegates to Qemu.
>
> The old platforms that use RTAS won't implement this call at all, so
> there's no issue with the naming there.

Thanks for checking. I've added SLOF to my checklist for things like
this.

Re: [PATCH] powerpc/rtas: use correct function name for resetting TCE tables

[Michael Ellerman]

On Thu, 22 Feb 2024 16:19:14 -0600, Nathan Lynch wrote:
> The PAPR spec spells the function name as
> 
>   "ibm,reset-pe-dma-windows"
> 
> but in practice firmware uses the singular form:
> 
>   "ibm,reset-pe-dma-window"
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc/rtas: use correct function name for resetting TCE tables
      https://git.kernel.org/powerpc/c/fad87dbd48156ab940538f052f1820f4b6ed2819

cheers