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

[mingming cao <mmc@linux.ibm.com>]

Thanks for the review... I will response one by one. And addressing them 
in v7

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> 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?
Yes. Paste glitch when that sentence was folded in. v7 drops the second 
copy.
>> 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.
Yes. v7 adds those three names. 0x498 and 0x4A4 stay unnamed; they
are the reserved holes already called out in hvcall.h.

>> 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.
Agreed. v7 lists H_FUNCTION on h_register_logical_lan_queue() and
h_free_logical_lan_queue() as well. h_add_logical_lan_buffers_queue()
already had it. All three opcodes are new; the wrappers return the
hypervisor status unchanged. Later patches map H_FUNCTION on register
to -EOPNOTSUPP / mq_fallback.
>> +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?
No, not intentional. Same H_FUNCTION line on free-queue in v7.


Thanks,
Mingming