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

On 8/17/26 6:47 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
Hi Jakub,

|Thanks for the v5 review. v6 is 
posted:https://lore.kernel.org/r/[email protected] |

|Per-comment replies below.|

> ---
> ibmveth: Add MQ RX hypercall wrappers and call definitions
>
> This adds the PHYP call IDs for H_REG_LOGICAL_LAN_QUEUE,
> H_ADD_LOGICAL_LAN_BUFFERS_QUEUE and H_FREE_LOGICAL_LAN_QUEUE, raising
> MAX_HCALL_OPCODE accordingly, and adds matching static inline wrappers in
> ibmveth.h.  It is plumbing only, with no runtime behaviour change until a
> later commit enables multi_queue.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
>> index d87713668ed30..c0ef03cad9b9a 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
>> @@ -66,6 +66,142 @@ static inline long h_add_logical_lan_buffers(unsigned long unit_address,
> [ ... ]
>
>> +/**
>> + * h_add_logical_lan_buffers_queue - Add buffers to subordinate queue
>> + * @unit_address: Device unit address
>> + * @queue_handle: Queue handle from h_register_logical_lan_queue()
> [Severity: Low]
> This isn't a bug, but do these two kernel-doc blocks in the same header
> contradict each other about which handles are valid here?
>
> h_add_logical_lan_buffers_queue() is documented as "Add buffers to
> subordinate queue" with "@queue_handle: Queue handle from
> h_register_logical_lan_queue()", while h_register_logical_lan_with_handle()
> below says its queue 0 handle:
>
>   * Registers the primary receive queue (queue 0) with the hypervisor and
>   * returns the queue handle. This is needed in multi-queue mode to use
>   * h_add_logical_lan_buffers_queue() for all queues including queue 0.
>
> By the end of the series the second wording is the one that matches the
> code: ibmveth_add_logical_lan_buffers() in
> drivers/net/ethernet/ibm/ibmveth.c passes
> adapter->queue_handle[queue_index] for every index, queue_index == 0
> included, and that handle comes from h_register_logical_lan_with_handle().
>
> Would it be clearer to widen the add-buffers kdoc to say the handle may
> come from either h_register_logical_lan_queue() or
> h_register_logical_lan_with_handle(), so a future caller doesn't conclude
> queue 0 must use the legacy buffer hcalls?
Agreed. v6 widens @queue_handle on both add-buffers and free-queue so
the handle may come from either register helper.

>> + * @buffersznum: Buffer size (upper 32 bits) | count (lower 32 bits)
>> + * @ioba12: Buffer addresses 1 and 2 packed ((addr1 << 32) | addr2)
>> + * @ioba34: Buffer addresses 3 and 4 packed
>> + * @ioba56: Buffer addresses 5 and 6 packed
>> + * @ioba78: Buffer addresses 7 and 8 packed
>> + * @ioba910: Buffer addresses 9 and 10 packed
>> + * @ioba1112: Buffer addresses 11 and 12 packed
>> + *
>> + * Return:
>> + *   H_SUCCESS - All buffers added successfully
>> + *   H_PARAMETER - Invalid parameters
>> + *   H_HARDWARE - Hardware error
>> + */
>> +static inline long h_add_logical_lan_buffers_queue(unsigned long unit_address,
>> +						   unsigned long queue_handle,
> [ ... ]
>
>> +/**
>> + * h_register_logical_lan_with_handle - Register primary queue and get handle
>> + * @unit_address: Device unit address
>> + * @buffer_list: DMA address of buffer list
>> + * @rec_queue: Buffer descriptor of receive queue
>> + * @filter_list: DMA address of filter list
>> + * @mac_address: MAC address
>> + * @queue_handle: Output parameter for queue handle (may be NULL)
>> + *
>> + * Registers the primary receive queue (queue 0) with the hypervisor and
>> + * returns the queue handle. This is needed in multi-queue mode to use
>> + * h_add_logical_lan_buffers_queue() for all queues including queue 0.
>> + *
>> + * Return: H_SUCCESS (0) on success, error code otherwise
>> + */
> [ ... ]
>
> Cross-instance finding from sashiko-gemini (34d1289184ae332db8c101e251d3dec96e9969f2e8dee8d98c3efff05039c9d0):
> [Severity: High]
> Calling plpar_hcall9() with missing arguments causes an out-of-bounds stack read, leaking uninitialized kernel stack memory to the hypervisor.

Yes. Fixed in v6. Both new registration wrappers use plpar_hcall().
h_add_logical_lan_buffers_queue() still needs all nine args. I left
the pre-existing h_send_logical_lan() underfill alone; happy to send
that separately.

Thanks

Mingming