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

[Amit Machhiwal <amachhiw@linux.ibm.com>]

On 2026/08/07 05:06 PM, Ritesh Harjani wrote:
> Amit Machhiwal <amachhiw@linux.ibm.com> writes:
> 
> > 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 <amachhiw@linux.ibm.com> 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 <vaibhav@linux.ibm.com>
> >> > 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:
> >> >   - 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.
> >
> 
> yup, my bad, should have seen the comment in the code above.

No worries...

> 
> But isn't this a bad design? Tomorrow, say we have a v2, a larger
> version of this struct and if your newer Qemu (with larger struct size)
> is running on an older kernel with a smaller struct size, then you will
> fail it because usize > ksize even though the userspace has zero filled
> the rest of the trailing bytes... 

You're right and thanks for pointing this out. A newer userspace that
zero-initialises the new fields (e.g. struct caps = {}; caps.size =
sizeof(caps);) is a valid forward-compat call — zero means "use default
for unknown fields", which is exactly the contract
copy_struct_from_user() is designed to honour. Forcing a round-trip in
that case is wrong.

I'll drop the manual usize > sizeof(host_caps) pre-check and let
copy_struct_from_user() own the usize > ksize path entirely.

One small correction to your suggested snippet though: put_user()
return value is ignored there. put_user() returns 0 on success or
-EFAULT on failure, so if the write-back fails we'd silently return
-E2BIG instead of -EFAULT. The fix is straightforward:

  diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
  index e64b3cfadd3a..ed48f069fb76 100644
  --- a/arch/powerpc/kvm/powerpc.c
  +++ b/arch/powerpc/kvm/powerpc.c
  @@ -2498,29 +2498,28 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
                  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;
  -               }
  -
                  /*
                   * 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 trailing bytes are zero, else -E2BIG
                   */
                  r = copy_struct_from_user(&host_caps, sizeof(host_caps),
                                            argp, usize);
  -               if (r)
  +               if (r) {
  +                       /*
  +                        * 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 (r == -E2BIG)
  +                               if (put_user((__u64)sizeof(host_caps),
  +                                            (__u64 __user *)argp))
  +                                       r = -EFAULT;
                          goto out;
  +               }
   
                  /* Reserved fields must be zero */
                  r = -EINVAL;

This preserves -EFAULT priority if put_user() fails, consistent with
how the get_user() guard earlier in the same handler works.

Will post v8 with this fix.

Thanks,
Amit

>      if (usize > sizeof(host_caps)) 
> 
> Instead wouldn't it be better if we do something like this?
> 
> 		r = copy_struct_from_user(&host_caps, sizeof(host_caps),
> 					  argp, usize);
> 		if (r) {
> 			if (r == -E2BIG)
> 				put_user(sizeof(host_caps), &argp->size);
> 			goto out;
> 		}
> 
> So, we only fail if the userspace has passed a newer struct and the
> trailing bytes, which are unknown to this older kernel, are zeroed,
> otherwise we return -E2BIG. We also write the ksize back, so the user
> knows what ksize it supports.
> 
> This would avoid an unncessary round trip and will simplify the
> userspace design, isn't it?
> 
> >> 
<snip>