Capitalize KVM please, i.e. "KVM: x86:".
On Mon, Apr 10, 2023, Takahiro Itazuri wrote:
> Modify the KVM_GET_CPUID2 API to return the number of valid entries in
> nent field of kvm_cpuid2, even when the API is successful.
>
> Previously, the KVM_GET_CPUID2 API only updated the nent field when an
Heh, I am so used to KVM_SET_CPUID2 being a source of bugs that it took me at
least three read throughs before I caught that this is fixing the GET side.
> error was returned. If the API was called with an entry count larger
> than necessary (e.g., KVM_MAX_CPUID_ENTRIES), it would succeed, but the
> nent field would continue to show a value larger than the actual number
> of entries filled by the KVM_GET_CPUID2 API. With this change, users can
> rely on the updated nent field and there is no need to traverse
> unnecessary entries and check whether an entry is valid or not.
While I completely agree that the not updating "nent" on success is asinine, I
am mildly concerned about this being a breaking ABI change, e.g. if a VMM has
"nent" marked as a consant/immutable value. I suspect it's ok because AFAICT,
pretty much nothing outside of selftests actually uses KVM_GET_CPUID2. But at
the very least, I'll push this out until 6.5 so that it can soak in linux-next
for a long time.
Paolo, any thoughts/objections?
> Signed-off-by: Takahiro Itazuri <itazur@xxxxxxxxxx>
> ---
> arch/x86/kvm/cpuid.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 599aebec2d52..31838dfddda6 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -523,10 +523,13 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries)
> {
> - int r;
> + int nent, r;
> +
> + nent = cpuid->nent;
> + cpuid->nent = vcpu->arch.cpuid_nent;
>
> r = -E2BIG;
> - if (cpuid->nent < vcpu->arch.cpuid_nent)
> + if (nent < vcpu->arch.cpuid_nent)
> goto out;
> r = -EFAULT;
> if (copy_to_user(entries, vcpu->arch.cpuid_entries,
> @@ -535,7 +538,6 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> return 0;
>
> out:
> - cpuid->nent = vcpu->arch.cpuid_nent;
> return r;
I think we should break from the (IMO) somewhat funky KVM ioctl() pattern of
r = <errno>
if (try something and it fails)
goto out;
and instead set "r" in the error paths. That avoids the need for a scratch "nent",
and IMO makes this much more straightforward.
int r = 0;
if (cpuid->nent < vcpu->arch.cpuid_nent)
r = -E2BIG;
else if (copy_to_user(entries, vcpu->arch.cpuid_entries,
vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
r = -EFAULT;
/*
* Update "nent" even on failure, e.g. so that userspace can fix an
* -E2BIG issue by allocating a larger array.
*/
cpuid->nent = vcpu->arch.cpuid_nent;
return r;
