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

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

Hi Jakub,

Thanks for the review, and I appreciate the detailed review across the 
whole series.

More broadly, I agree with the overall direction of your review. For v5
I plan to fold the follow-up fixes back into the patches where they
belong and make the changelogs clearer, so the series reads more cleanly
end to end.

On 8/6/26 11:36 AM, 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.
> ---
> ibmveth: Add MQ RX hypercall wrappers and call definitions
>
> This adds three new PHYP call IDs for multi-queue receive support and four
> static inline hcall wrapper helpers in ibmveth.h, bumping MAX_HCALL_OPCODE
> to the highest new opcode. No callers are added by this patch.
>
> A couple of questions below about the new wrappers and about the hvcall.h
> hunk.
>> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
>> index dff90a7d7f70b..d79baf2e38273 100644
>> --- a/arch/powerpc/include/asm/hvcall.h
>> +++ b/arch/powerpc/include/asm/hvcall.h
>> @@ -362,7 +362,10 @@
>>   #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
>> +#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]
> Could the changelog cite the PAPR/PHYP document and revision that assigns
> 0x49C, 0x4A0 and 0x4A8?  These become architecture-wide constants shared
> with the KVM hcall emulation, so it would help to have the provenance
> recorded in the commit.
Agreed. The first PAPR revision that contains the MQ hcalls is
PAPR 11.20.00, and in v5 I will cite that revision explicitly in the
patch-1 changelog.
> There are also holes left at 0x498 and at 0x4A4 (the latter from dropping
> H_FREE_LOGICAL_LAN_BUFFER_QUEUE in v4).  Are those numbers reserved by the
> architecture for something else, or are they simply unused?

For the gaps, `0x4A4` remains reserved for
`H_FREE_LOGICAL_LAN_BUFFER_QUEUE`, and `0x498` is reserved. This series
does not need `H_FREE_LOGICAL_LAN_BUFFER_QUEUE` for the normal queue
teardown path, so I am not adding it here.

>
> Separately, MAX_HCALL_OPCODE is not driver-local.  It sizes
> kvm_arch.enabled_hcalls:
>
>    arch/powerpc/include/asm/kvm_host.h:
>        DECLARE_BITMAP(enabled_hcalls, MAX_HCALL_OPCODE/4 + 1);
>
> and it bounds the userspace-visible opcode range accepted by
> KVM_CAP_PPC_ENABLE_HCALL:
>
>    arch/powerpc/kvm/powerpc.c:kvm_vm_ioctl_enable_cap() {
>        ...
>        if (hcall > MAX_HCALL_OPCODE || (hcall & 3) ||
>            cap->args[1] > 1)
>        ...
>    }
>
> It also sizes the per-CPU hcall statistics array in
> arch/powerpc/platforms/pseries/hvCall_inst.c and gates
> kvmppc_pseries_do_hcall() and kvmppc_h_pr().  As far as I can tell the bump
> is behaviourally inert for these new opcodes, since kvm_vm_ioctl_enable_cap()
> still rejects them via kvmppc_book3s_hcall_implemented(), and the KVM
> dispatch path ends up returning RESUME_HOST either way.  Would it be worth
> mentioning the cross-subsystem effect in the changelog so the powerpc/KVM
> side has something to ack against?
Sure, for `MAX_HCALL_OPCODE`, I agree that the bump has a 
cross-subsystem effect
because it sizes KVM's enabled-hcall bitmap and bounds
`KVM_CAP_PPC_ENABLE_HCALL`. In v5 I will call that out explicitly in the
patch-1 changelog rather than leaving it implicit.

>>   
>>   /* Scope args for H_SCM_UNBIND_ALL */
>>   #define H_UNBIND_SCOPE_ALL (0x1)
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
>> index d87713668ed30..c88dbeb7cd967 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
>> @@ -66,6 +66,143 @@ static inline long h_add_logical_lan_buffers(unsigned long unit_address,
>>   			    desc5, desc6, desc7, desc8);
>>   }
>>   
>> +/**
>> + * h_reg_logical_lan_queue - Register a subordinate receive queue
> [Severity: Low]
> This isn't a bug, but the naming here diverges from the existing wrappers in
> this file, which all spell it out as h_register_logical_lan() and
> h_free_logical_lan().  Would h_register_logical_lan_queue() read more
> consistently?
>
> [ ... ]
Agreed. In v5 I will rename it to `h_register_logical_lan_queue()` so it
matches the existing wrapper naming.

>> +static inline long h_reg_logical_lan_queue(unsigned long unit_address,
>> +					   unsigned long buffer_list,
>> +					   unsigned long rec_queue,
>> +					   unsigned long *queue_handle,
>> +					   unsigned long *irq)
>                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> [Severity: Low]
> This helper returns the queue handle as unsigned long *, which matches
> retbuf's element type and matches the other output parameters in this header
> (h_send_logical_lan()'s corellator_out, h_illan_attributes()'s
> ret_attributes).
>
> But h_register_logical_lan_with_handle() below declares the same conceptual
> value, the PHYP queue handle read out of retbuf[0], as u64 *.  Can these two
> be made to agree?  As it stands, callers have to pick a different storage
> type depending on which of the two helpers they call, even though both are
> returning the same thing.
>
> The commit message says the new wrappers have "argument ordering and return
> semantics matching the existing ibmveth hcall wrappers", which holds for
> this one but not for the u64 * variant.
>
> [ ... ]
Agreed. In v5 both `h_register_logical_lan_queue()` and
`h_register_logical_lan_with_handle()` will use `unsigned long
*queue_handle`, matching the sibling wrappers and avoiding different
caller-side storage depending on which helper is used.

>> +static inline long h_free_logical_lan_queue(unsigned long unit_address,
>> +					    unsigned long queue_handle)
>> +{
>> +	unsigned long retbuf[PLPAR_HCALL9_BUFSIZE];
>> +
>> +	return plpar_hcall9(H_FREE_LOGICAL_LAN_QUEUE,
>> +			    retbuf, unit_address, queue_handle);
>> +}
> [Severity: Low]
> The kerneldoc above documents no output data for this hcall, and retbuf is
> never read.  Why not use plpar_hcall_norets() here, the way the existing
> counterpart in this file does?
>
>    #define h_free_logical_lan(ua) \
>      plpar_hcall_norets(H_FREE_LOGICAL_LAN, ua)
>
> That would drop the dead nine-entry stack buffer.
>
> [ ... ]
Agreed. In v5 I will switch `h_free_logical_lan_queue()` to
`plpar_hcall_norets()` and drop the unused return buffer.

>> +static inline long
>> +h_register_logical_lan_with_handle(unsigned long unit_address,
>> +				   unsigned long buffer_list,
>> +				   unsigned long rec_queue,
>> +				   unsigned long filter_list,
>> +				   unsigned long mac_address,
>> +				   u64 *queue_handle)
>                                      ^^^^^^^^^^^^^^^^^^
>
> [Severity: Low]
> This is the u64 * side of the inconsistency noted above.  Both types are
> 64-bit on ppc64, which is the only configuration that builds ibmveth, and
> the callers added later in the series do compile against both prototypes, so
> this looks like a readability issue rather than a functional one.  Could both
> helpers use unsigned long * for the handle?

|In v5 both helpers will use unsigned long *queue_handle.|

Thanks again for the detailed review,
Mingming