Re: [PATCH v7 1/4] KVM: PPC: Introduce KVM_CAP_PPC_COMPAT_CAPS and wire up ioctl

Hi Ritesh,

Thanks for reviewing this patch. Please find my response inline below.

On 2026/08/07 08:38 AM, Ritesh Harjani wrote:
> Amit Machhiwal <[email protected]> writes:
> 
> > Introduce a new capability and ioctl to expose CPU compatibility modes
> > supported by the host processor for nested guests.
> >
> > On IBM POWER systems, newer processor generations (N) can operate in
> > compatibility modes corresponding to earlier generations, like (N-1) and
> > (N-2). This is particularly relevant for nested virtualization, where
> > nested KVM guests may need to run with a specific processor compatibility
> > level.
> >
> > Introduce KVM_CAP_PPC_COMPAT_CAPS capability and the corresponding
> > KVM_PPC_GET_COMPAT_CAPS vm ioctl. The ioctl returns a bitmap describing
> > the compatibility modes supported by the host in respective bit numbers,
> > allowing userspace (e.g., QEMU) to select an appropriate compatibility
> > level when configuring nested KVM guests.
> >
> > The ioctl handling is added in kvm_arch_vm_ioctl() and retrieves host
> > CPU compatibility capabilities via a PowerPC-specific backend
> > implementation when available.
> >
> > The struct kvm_ppc_compat_caps places the 'size' field first so it can
> > be read alone via get_user() before copy_struct_from_user() is called,
> > avoiding pointer arithmetic to locate the size field.
> >
> > The ioctl is defined using _IO so the ioctl number remains stable even if
> > the struct grows in future versions. It uses copy_struct_from_user() and
> > copy_struct_to_user() to provide forward- and backward-compatible
> > extensibility: older userspace passing a smaller struct to a newer kernel
> > gets zero-padded trailing fields, while newer userspace passing a larger
> > struct to an older kernel (usize > ksize) gets sizeof(struct
> > kvm_ppc_compat_caps) written back to host_caps.size so it can retry with the
> > older kernel-supported size, after which the kernel returns -E2BIG.
> >
> > KVM_PPC_COMPAT_CAPS_SIZE_VER0 is defined as a frozen integer constant
> > (24) marking the size of the initial struct version, used as the
> > minimum floor for size field validation, similar to other versioned
> > struct interfaces in the kernel.
> >
> > The 'flags' field is reserved for future use. The kernel rejects any
> > call where flags is non-zero with -EINVAL, preventing garbage values
> > from being baked into ABI permanently.
> >
> > The ioctl returns appropriate error codes: EINVAL for an invalid size
> > or non-zero reserved fields, E2BIG if new userspace provides a larger
> > struct than the kernel knows about (with ksize written back into
> > host_caps.size for the retry), EFAULT for failed copy operations, and
> > ENOTTY if the backend doesn't implement get_compat_caps.
> >
> > Suggested-by: Vaibhav Jain <[email protected]>
> > Tested-by: Gautam Menghani <[email protected]>
> > Reviewed-by: Gautam Menghani <[email protected]>
> > Tested-by: Anushree Mathur <[email protected]>
> > Signed-off-by: Amit Machhiwal <[email protected]>
> > ---
> > Changes in this version:
> >   - KVM_CAP_PPC_COMPAT_CAPS: add hv_enabled guard to align the capability
> >     check with ioctl availability; a PR KVM VM on pseries now correctly
> >     returns 0 for the capability [Sashiko]
> >
> >  arch/powerpc/include/asm/kvm_ppc.h  |  1 +
> >  arch/powerpc/include/uapi/asm/kvm.h |  8 ++++
> >  arch/powerpc/kvm/powerpc.c          | 71 +++++++++++++++++++++++++++++
> >  include/uapi/linux/kvm.h            |  3 ++
> >  4 files changed, 83 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> > index 0953f2daa466..169ea6a7fbad 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -319,6 +319,7 @@ struct kvmppc_ops {
> >  	bool (*hash_v3_possible)(void);
> >  	int (*create_vm_debugfs)(struct kvm *kvm);
> >  	int (*create_vcpu_debugfs)(struct kvm_vcpu *vcpu, struct dentry *debugfs_dentry);
> > +	int (*get_compat_caps)(struct kvm_ppc_compat_caps *host_caps);
> >  };
> >  
> >  extern struct kvmppc_ops *kvmppc_hv_ops;
> > diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> > index 077c5437f521..19e53d5ae540 100644
> > --- a/arch/powerpc/include/uapi/asm/kvm.h
> > +++ b/arch/powerpc/include/uapi/asm/kvm.h
> > @@ -437,6 +437,14 @@ struct kvm_ppc_cpu_char {
> >  	__u64	behaviour_mask;		/* valid bits in behaviour */
> >  };
> >  
> > +/* For KVM_PPC_GET_COMPAT_CAPS */
> > +struct kvm_ppc_compat_caps {
> > +	__u64	size;			/* Size of this structure */
> > +	__u64	flags;			/* Reserved for future use */
> > +	__u64	compat_capabilities;	/* Capabilities supported by the host */
> > +};
> > +#define KVM_PPC_COMPAT_CAPS_SIZE_VER0	24 /* sizeof first published struct */
> > +
> >  /*
> >   * Values for character and character_mask.
> >   * These are identical to the values used by H_GET_CPU_CHARACTERISTICS.
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index b6b83fe3233f..e64b3cfadd3a 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -703,6 +703,13 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >  			}
> >  		}
> >  		break;
> > +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE)
> > +	case KVM_CAP_PPC_COMPAT_CAPS:
> > +		r = 0;
> > +		if (hv_enabled && kvmhv_on_pseries())
> 
> I think sashiko is just complaining in the 1st patch because we have not
> yet wired up the kvmppc_hv_ops->get_compat_caps() yet in patch-1. I
> think it is expecting..
> 
> if (hv_enabled && kvmhv_on_pseries() && kvmppc_hv_ops->get_compat_caps)
> 
> But either way is fine.
> 
> 
> > +			r = 1;
> > +		break;
> > +#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
> >  	default:
> >  		r = 0;
> >  		break;
> > @@ -2469,6 +2476,70 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> >  		r = kvm->arch.kvm_ops->svm_off(kvm);
> >  		break;
> >  	}
> > +	case KVM_PPC_GET_COMPAT_CAPS: {
> > +		struct kvm_ppc_compat_caps host_caps = {};
> > +		u64 usize;
> > +
> > +		/*
> > +		 * Read the size field first to drive copy_struct_from_user.
> > +		 * size must be the first field of the struct.
> > +		 */
> > +		r = -EFAULT;
> > +		if (get_user(usize, (__u64 __user *)argp))
> > +			goto out;
> > +
> > +		/*
> > +		 * Enforce a minimum: reject buffers smaller than the initial
> > +		 * struct version (VER0). This allows old userspace compiled
> > +		 * against the original struct to still work on a newer kernel
> > +		 * that has grown the struct with appended fields.
> > +		 */
> > +		r = -EINVAL;
> > +		if (usize < KVM_PPC_COMPAT_CAPS_SIZE_VER0)
> > +			goto out;
> > +
> > +		/*
> > +		 * New userspace with a larger struct called an older kernel.
> > +		 * Write back ksize in host_caps.size so userspace knows which
> > +		 * older struct to retry with, then fail with -E2BIG.
> > +		 */
> > +		if (usize > sizeof(host_caps)) {
> > +			host_caps.size = sizeof(host_caps);
> > +			r = -EFAULT;
> > +			if (put_user(host_caps.size, (__u64 __user *)argp))
> > +				goto out;
> > +			r = -E2BIG;
> > +			goto out;
> > +		}
> 
> You anyways mentioned copy_struct_from_user() is taking care of both
> forward and backward compat. Then what is the point of this check?
> shouldn't we get rid of this complete if logic? I don't see a point of
> this if we are anyway using copy_struct_from_user().

The pre-check is intentional and serves a purpose that
copy_struct_from_user() alone cannot provide: explicit kernel struct
size discovery.

copy_struct_from_user() returns -E2BIG when usize > ksize and trailing
bytes are non-zero — but it gives userspace no way to know what ksize to
retry with. It also silently succeeds when trailing bytes are zero,
which means a new userspace on an old kernel would never learn the
kernel's struct size at all.

This design is different by intent: when new userspace passes a larger
struct, we always return -E2BIG and write back sizeof(host_caps) into
the size field so userspace learns the exact kernel-supported size and
can retry with it. This explicit negotiation is verified in the ABI
extensibility testing in the cover letter ("Newer struct on QEMU, older
kernel -> works"). The explicit -E2BIG + ksize writeback makes the
version negotiation unambiguous.

> 
> > +
> > +		/*
> > +		 * copy_struct_from_user() handles forward/backward compat:
> > +		 *   usize == ksize: verbatim copy
> > +		 *   usize <  ksize: zero-pad trailing (old userspace, new kernel)
> 
> +		 *   usize >  ksize: succeed iff the trailing bytes userspace
> +		 *                   sent are zero, else -E2BIG
> 
> shouldn't we add usize > ksize case details, like ^^^,  which
> copy_struct_from_user() handles since we are already adding the details
> of other 2 cases.

The comment is intentionally limited to two cases. By the time we reach
copy_struct_from_user(), the usize > sizeof(host_caps) case has already
been caught by the pre-check above and returned -E2BIG. So
copy_struct_from_user() can only ever see usize == ksize or usize <
ksize at this point — those are the only two cases worth documenting
here.

Adding the usize > ksize case would be misleading since that code path
is unreachable for this call site.

> 
> 
> </btw snip from copy_struct_from_user kdoc>
> 
>  * There are three cases to consider:
>  *  * If @usize == @ksize, then it's copied verbatim.
>  *  * If @usize < @ksize, then the userspace has passed an old struct to a
>  *    newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
>  *    are to be zero-filled.
>  *  * If @usize > @ksize, then the userspace has passed a new struct to an
>  *    older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
>  *    are checked to ensure they are zeroed, otherwise -E2BIG is returned.
>  *
>  * Returns (in all cases, some data may have been copied):
>  *  * -E2BIG:  (@usize > @ksize) and there are non-zero trailing bytes in @src.
>  *  * -EFAULT: access to userspace failed.
>  
> > +		 */
> > +		r = copy_struct_from_user(&host_caps, sizeof(host_caps),
> > +					  argp, usize);
> > +		if (r)
> > +			goto out;
> > +
> > +		/* Reserved fields must be zero */
> > +		r = -EINVAL;
> > +		if (host_caps.flags)
> > +			goto out;
> > +
> > +		r = -ENOTTY;
> > +		if (!kvm->arch.kvm_ops->get_compat_caps)
> > +			goto out;
> > +
> > +		r = kvm->arch.kvm_ops->get_compat_caps(&host_caps);
> > +		if (r)
> > +			goto out;
> > +
> > +		host_caps.size = sizeof(host_caps);
> 
> shouldn't this be, since we don't want to be reporting a larger size to
> the user?
> 
>           host_caps.size = min_t(u64, usize, sizeof(host_caps));

At this point in the code, usize has already been bounds-checked to
[KVM_PPC_COMPAT_CAPS_SIZE_VER0, sizeof(host_caps)], so min_t(u64, usize,
sizeof(host_caps)) is always equal to usize here — the min_t never
actually clamps anything.

Thanks,
Amit