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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM_CAP_PPC_COMPAT_CAPS is advertised as supported on pSeries HV KVM, but the KVM_PPC_GET_COMPAT_CAPS ioctl backend implementation is missing, leading to unconditional -ENOTTY failure.
--

--- Patch [1]: [PATCH v8 1/4] KVM: PPC: Introduce KVM_CAP_PPC_COMPAT_CAPS and wire up ioctl ---
commit 3fd26ec35303a5e885f7551c23267fe626ae5e16
Author: Amit Machhiwal <[email protected]>

KVM: PPC: Introduce KVM_CAP_PPC_COMPAT_CAPS and wire up ioctl

This commit introduces the KVM_CAP_PPC_COMPAT_CAPS capability and the
KVM_PPC_GET_COMPAT_CAPS ioctl to expose CPU compatibility modes. It routes
the ioctl to a backend-specific get_compat_caps implementation.

> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 00302399fc37b..2fe4ea0ca4ff1 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -697,6 +697,13 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
[ ... ]
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE)
> +	case KVM_CAP_PPC_COMPAT_CAPS:
> +		r = 0;
> +		if (hv_enabled && kvmhv_on_pseries())
> +			r = 1;
> +		break;
> +#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */

[Severity: High]
Since kvm_vm_ioctl_check_extension() advertises this capability as supported
solely based on platform checks, will userspace expect the ioctl to work?

Looking at the ioctl implementation below, it seems to fail if the backend
is missing. Should this capability check also verify that
kvm->arch.kvm_ops->get_compat_caps is implemented so that it doesn't
advertise a capability the kernel cannot fulfill?

[ ... ]
> @@ -2463,6 +2470,77 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
[ ... ]
> +		/* 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;

[Severity: High]
Does this ioctl currently return -ENOTTY on all platforms?

I don't see any backend implementing get_compat_caps in this patch. If
userspace probes KVM_CAP_PPC_COMPAT_CAPS and gets a positive result, won't
it be unexpectedly broken by receiving -ENOTTY when attempting to use the
ioctl?

Would it be safer to introduce the capability advertisement in the same
patch that wires up the actual backend implementation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1