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
