Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Apr 2023 08:47:05 -0700
In-Reply-To: <20230410141820.57328-1-itazur@amazon.com>
Mime-Version: 1.0
References: <20230410141820.57328-1-itazur@amazon.com>
Message-ID: <ZDQveaSDYx+4z5t4@google.com>
Subject: Re: [PATCH] kvm: x86: Update KVM_GET_CPUID2 to return valid entry count
From:   Sean Christopherson <seanjc@google.com>
To:     Takahiro Itazuri <itazur@amazon.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        linux-kernel@vger.kernel.org, Takahiro Itazuri <zulinx86@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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@amazon.com>
> ---
>  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;