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

[Jakub Kicinski <kuba@kernel.org>]

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.

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?

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?

>  
>  /* 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?

[ ... ]

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

[ ... ]

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

[ ... ]

> +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?

> +{
> +	unsigned long retbuf[PLPAR_HCALL9_BUFSIZE];
> +	long rc;
> +
> +	rc = plpar_hcall9(H_REGISTER_LOGICAL_LAN, retbuf,
> +			  unit_address, buffer_list, rec_queue,
> +			  filter_list, mac_address);
> +
> +	if (rc == H_SUCCESS && queue_handle)
> +		*queue_handle = retbuf[0];
> +
> +	return rc;
> +}

Thanks.
-- 
pw-bot: cr