Re: [net-next,v6,01/15] ibmveth: Add MQ RX hypercall wrappers and call definitions

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 3

- [Low] The commit message body contains a verbatim duplicated sentence.
- [Low] kernel-doc for the three new MQ hcall wrappers added by this
  patch is inconsistent about the H_FUNCTION (unsupported-hcall) return.
- [Low] The in-tree perf hypercall decoder is not updated for the three
  opcodes added by this patch.

--

ibmveth: Add MQ RX hypercall wrappers and call definitions

This adds the PAPR 11.20.00 hcall IDs H_REG_LOGICAL_LAN_QUEUE (0x49C),
H_ADD_LOGICAL_LAN_BUFFERS_QUEUE (0x4A0) and H_FREE_LOGICAL_LAN_QUEUE
(0x4A8), raising MAX_HCALL_OPCODE, and adds matching static inline
wrappers in ibmveth.h plus h_register_logical_lan_with_handle() so
queue 0 can capture the PHYP queue handle.

A few questions below, all on the changelog text and the kernel-doc.

>     Add ibmveth.h wrapper helpers (h_register_logical_lan_queue(),
>     h_add_logical_lan_buffers_queue(), h_free_logical_lan_queue()) with
>     argument ordering and return semantics matching the existing ibmveth
>     hcall wrappers. h_free_logical_lan_queue() uses plpar_hcall_norets()
>     like h_free_logical_lan(). Also add h_register_logical_lan_with_handle()
>     so queue 0 can capture the PHYP queue handle in MQ mode. Both new
>     registration wrappers use plpar_hcall() rather than plpar_hcall9(), so
>     they do not read unwritten stack slots. Both new
>     registration wrappers use plpar_hcall() rather than plpar_hcall9(), so
>     they do not read unwritten stack slots.

[Severity: Low]
This isn't a bug, but the sentence "Both new registration wrappers use
plpar_hcall() rather than plpar_hcall9(), so they do not read unwritten
stack slots." appears twice in a row here.  Was the second copy meant to
be dropped when the v6 rationale was folded into the changelog body?

> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index dff90a7d7f70b..cb0ea53491e6c 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -362,7 +362,11 @@
>  #define H_GUEST_DELETE		0x488
>  #define H_PKS_WRAP_OBJECT	0x490
>  #define H_PKS_UNWRAP_OBJECT	0x494
> -#define MAX_HCALL_OPCODE	H_PKS_UNWRAP_OBJECT
> +/* 0x498 reserved; 0x4A4 = H_FREE_LOGICAL_LAN_BUFFER_QUEUE (unused here) */
> +#define H_REG_LOGICAL_LAN_QUEUE 0x49C
> +#define H_ADD_LOGICAL_LAN_BUFFERS_QUEUE 0x4A0
> +#define H_FREE_LOGICAL_LAN_QUEUE 0x4A8
> +#define MAX_HCALL_OPCODE	H_FREE_LOGICAL_LAN_QUEUE

[Severity: Low]
Should tools/perf/scripts/python/powerpc-hcalls.py get entries for these
three opcodes too?

Its hcall_table currently tracks hvcall.h up to the previous
MAX_HCALL_OPCODE:

	1168: 'H_PKS_WRAP_OBJECT',
	1172: 'H_PKS_UNWRAP_OBJECT',
	# Platform-specific hcalls used by the Ultravisor
	61184: 'H_SVM_PAGE_IN',

There is nothing for 1180 (0x49C), 1184 (0x4A0) or 1192 (0x4A8), so
tracing the new MQ RX path with that script prints the raw numbers rather
than the names.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
> index d87713668ed30..08504d1cafd55 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.h
> +++ b/drivers/net/ethernet/ibm/ibmveth.h
> @@ -66,6 +66,145 @@ static inline long h_add_logical_lan_buffers(unsigned long unit_address,
>  			    desc5, desc6, desc7, desc8);
>  }
>  
> +/**
> + * h_register_logical_lan_queue - Register a subordinate receive queue
> + * @unit_address: Device unit address
> + * @buffer_list: DMA address of 4KB page for tracking registered buffers
> + * @rec_queue: Buffer descriptor of receive queue
> + * @queue_handle: Output queue handle on success (may be NULL)
> + * @irq: Output hypervisor IRQ number on success (may be NULL)
> + *
> + * Registers a subordinate receive queue with the hypervisor.
> + *
> + * Return:
> + *   H_SUCCESS (0) on success
> + *   H_PARAMETER if parameters are invalid

[Severity: Low]
Should H_FUNCTION be listed here as well?

H_REG_LOGICAL_LAN_QUEUE (0x49C) is new in PAPR 11.20.00, so firmware
predating it returns H_FUNCTION, and h_register_logical_lan_queue()
passes the hypervisor status straight back.  The later commit in this
series ("ibmveth: Add RX queue register helpers for MQ") does distinguish
that case:

	if (lpar_rc == H_FUNCTION)
		netdev_err(adapter->netdev,
			   "h_register_logical_lan_queue H_FUNCTION for queue %d (firmware MQ unsupported)\n",

so a caller reading only this Return: section has no documented way to
tell "firmware has no MQ support, fall back to single queue" from a hard
error.

> +static inline long
> +h_register_logical_lan_queue(unsigned long unit_address,
> +			     unsigned long buffer_list,
> +			     unsigned long rec_queue,
> +			     unsigned long *queue_handle,
> +			     unsigned long *irq)
> +{

[ ... ]

> + * Return:
> + *   H_SUCCESS - All buffers added successfully
> + *   H_PARAMETER - Invalid parameters
> + *   H_HARDWARE - Hardware error
> + *   H_FUNCTION - Firmware does not support this hcall
> + */
> +static inline long h_add_logical_lan_buffers_queue(unsigned long unit_address,

[ ... ]

> + * Deregisters and frees all structures associated with the subordinate queue.
> + *
> + * Return:
> + *   H_SUCCESS - Queue freed successfully
> + *   H_PARAMETER - Invalid parameters
> + *   H_HARDWARE - Hardware error
> + *   H_STATE - VIOA not in valid state
> + *   H_BUSY / H_LONG_BUSY_* - Resource busy, retry
> + */
> +static inline long h_free_logical_lan_queue(unsigned long unit_address,
> +					    unsigned long queue_handle)
> +{
> +	return plpar_hcall_norets(H_FREE_LOGICAL_LAN_QUEUE,
> +				  unit_address, queue_handle);
> +}

Same question for h_free_logical_lan_queue(): 0x4A8 is equally new, and
plpar_hcall_norets() returns H_FUNCTION unchanged on firmware that lacks
it, but only h_add_logical_lan_buffers_queue() documents that status.  Was
the difference between the three Return: sections intentional?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788102125.git.mmc%40linux.ibm.com