Re: [PATCH v8 4/4] KVM: PPC: Document KVM_PPC_GET_COMPAT_CAPS ioctl

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Amit Machhiwal <amachhiw@linux.ibm.com> writes:

> Add documentation for the KVM_PPC_GET_COMPAT_CAPS ioctl to the KVM API
> documentation.
>
> The ioctl exposes host processor compatibility modes supported for
> nested KVM guests on PowerPC systems. The documentation covers error
> code descriptions including E2BIG for forward compatibility, the
> extensible size-based versioning contract using
> KVM_PPC_COMPAT_CAPS_SIZE_VER0, the rationale for rejecting non-zero
> reserved fields to prevent ABI ambiguity, bit numbering clarification
> for IBM MSB-0 convention, and KVM-specific capability bit constants.
>
> Tested-by: Gautam Menghani <gautam@linux.ibm.com>
> Reviewed-by: Gautam Menghani <gautam@linux.ibm.com>
> Tested-by: Anushree Mathur <anushree.mathur@linux.ibm.com>
> Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
> ---
> Changes in this version:
>   - Update E2BIG description: document PAGE_SIZE guard as first case;
>     -E2BIG for usize > ksize is only returned when trailing bytes are
>     non-zero; zero trailing bytes now succeed [Ritesh]
>   - Rewrite versioning paragraph as three explicit cases to match the
>     corrected copy_struct_from_user() / copy_struct_to_user() contract,
>     including the usize > ksize zero-trailing-bytes success path [Ritesh]
>
>  Documentation/virt/kvm/api.rst | 89 ++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index e3003a241d5b..e656d117cd0b 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6566,6 +6566,95 @@ KVM_S390_KEYOP_SSKE
>    Sets the storage key for the guest address ``guest_addr`` to the key
>    specified in ``key``, returning the previous value in ``key``.
>  
> +4.145 KVM_PPC_GET_COMPAT_CAPS
> +-----------------------------
> +:Capability: KVM_CAP_PPC_COMPAT_CAPS
> +:Architectures: powerpc
> +:Type: vm ioctl
> +:Parameters: struct kvm_ppc_compat_caps (in/out)
> +:Returns: 0 on success, negative value on failure
> +
> +Errors include:
> +
> +  ======== ============================================================
> +  EFAULT   if ``struct kvm_ppc_compat_caps`` cannot be read from or
> +           written to userspace
> +  EINVAL   if the ``size`` field is smaller than
> +           ``KVM_PPC_COMPAT_CAPS_SIZE_VER0``, if the ``flags`` field
> +           is non-zero, or if the backend fails to retrieve or map
> +           CPU compatibility capabilities
> +  E2BIG    if ``size`` exceeds ``PAGE_SIZE`` (pathological input guard),
> +           or if ``size`` is larger than the kernel's struct size and
> +           the unknown trailing bytes are non-zero (new userspace on
> +           old kernel with non-default fields set); in the latter case
> +           the kernel writes back its own struct size into the ``size``
> +           field so userspace can retry with the correct size
> +  ENOTTY   if the backend does not implement the ``get_compat_caps``
> +           operation (e.g., on non-HV KVM implementations where the
> +           required KVM operations are not available)
> +  ======== ============================================================
> +
> +IBM POWER system server-based processors provide a compatibility mode feature
> +where an Nth generation processor can operate in modes consistent with earlier
> +generations such as (N-1) and (N-2).
> +
> +This ioctl provides userspace with information about the CPU compatibility modes
> +supported by the current host processor for booting the nested KVM guests on
> +KVM on PowerNV (nested API v1) and KVM on PowerVM (nested API v2) platforms.
> +
> +::
> +
> +  struct kvm_ppc_compat_caps {
> +	__u64	size;			/* Size of this structure */
> +	__u64	flags;			/* Reserved for future use, must be 0 */
> +	__u64	compat_capabilities;	/* Capabilities supported by the host */
> +  };
> +
> +Before calling this ioctl, userspace must set the ``size`` field to
> +``sizeof(struct kvm_ppc_compat_caps)`` and zero the ``flags`` field.
> +The kernel rejects non-zero ``flags`` with ``-EINVAL`` to prevent
> +uninitialized stack values from being silently accepted, keeping the
> +field available for future use without ABI ambiguity.
> +
> +The ioctl uses ``copy_struct_from_user()`` and ``copy_struct_to_user()``
> +to support extensible versioning across three cases:
> +
> +- If ``size`` is smaller than the kernel's struct size (old userspace,
> +  new kernel), the kernel zero-pads the unknown trailing fields before
> +  returning, and writes back ``size`` unchanged so userspace knows how
> +  many bytes were filled.

I agree with Sashiko comment here. This para is slightly misleading.
This sounds like we are zero padding to userspace struct before
returning. Whereas what we intend to say here is, when we copy user
struct into kernel (copy_struct_from_user()), we zero pad the trailing
bytes in kernel's struct.

> +- If ``size`` equals the kernel's struct size, the struct is copied
> +  verbatim.
> +- If ``size`` is larger than the kernel's struct size (new userspace,
> +  old kernel) and the unknown trailing bytes are all zero, the call
> +  succeeds as if the sizes matched. If any trailing bytes are non-zero,
> +  the kernel returns ``-E2BIG`` and writes back its own struct size into
> +  the ``size`` field so userspace can retry with the correct size.
>

BTW - I anyway feel this is too much. We can get rid of all 3 points
which explains how struct copying is working. We have more than enough
documentation around how copy_struct_{from|to}_user() works and we have
also added the comments around the code. So I think this is just
unnecessary.

We can just say:
   +The ioctl uses ``copy_struct_from_user()`` and ``copy_struct_to_user()``
   +to support extensible versioning.


With that taken care, please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>