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

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?

[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.

[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