[PATCH] KVM: Adjust the return type of kvm_vm_ioctl_check_extension_generic()

[PATCH] KVM: Adjust the return type of kvm_vm_ioctl_check_extension_generic()

[Thomas Huth]

kvm_vm_ioctl_check_extension_generic() either returns small constant
numbers or the result of kvm_vm_ioctl_check_extension() which is of type
"int". Looking at the callers of kvm_vm_ioctl_check_extension_generic(),
one stores the result in "int r", the other one in "long r", so the
result has to fit in the smaller "int" in any case. Thus let's adjust
the return value to "int" here so we have one less transition from
"int" -> "long" -> "int" in case of the kvm_vm_ioctl() ->
kvm_vm_ioctl_check_extension_generic() -> kvm_vm_ioctl_check_extension()
call chain.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 This patch is of very low importance - if you don't like it, please just
 ignore. I just came across this nit while looking through the code and
 thought that it might be somewhat nicer this way.

 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 64ec2222a196..e911331fc620 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4309,7 +4309,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 	return 0;
 }
 
-static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
+static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
 {
 	switch (arg) {
 	case KVM_CAP_USER_MEMORY:
-- 
2.31.1

Re: [PATCH] KVM: Adjust the return type of kvm_vm_ioctl_check_extension_generic()

[Sean Christopherson]

On Tue, May 31, 2022, Thomas Huth wrote:
> kvm_vm_ioctl_check_extension_generic() either returns small constant
> numbers or the result of kvm_vm_ioctl_check_extension() which is of type
> "int". Looking at the callers of kvm_vm_ioctl_check_extension_generic(),
> one stores the result in "int r", the other one in "long r", so the
> result has to fit in the smaller "int" in any case. Thus let's adjust
> the return value to "int" here so we have one less transition from
> "int" -> "long" -> "int" in case of the kvm_vm_ioctl() ->
> kvm_vm_ioctl_check_extension_generic() -> kvm_vm_ioctl_check_extension()
> call chain.

LOL, I was going to play devil's advocate and say that it would be just as easy
to adjust kvm_vm_ioctl() to use "long r", but there's actually lurking bug, sort
of.

KVM_GET_NR_MMU_PAGES => kvm_vm_ioctl_get_nr_mmu_pages() returns an "unsigned long",
and it very much can be a value larger than an "int" because KVM_SET_NR_MMU_PAGES
allows setting an arbitrary value (it may also be possible for KVM's default to
be that large, but I don't feel like doing math).

But, the very original commit 82ce2c96831f ("KVM: Allow dynamic allocation of the
mmu shadow cache size") only tracked and returned an "unsigned int".  It was the
relatively recent commit bc8a3d8925a8 ("kvm: mmu: Fix overflow on kvm mmu page
limit calculation") that bumped the internal tracking to "unsigned long" and
overlooked the long/int mess.

Looking at other architectures, kvm_vm_ioctl_create_spapr_tce() also returns a
"long", but that's unnecessary as it only returns -errno or propagates "int" returns.

Ditto for kvm_arch_vm_ioctl_hv(), kvm_arch_vm_ioctl_pr(), and
kvm_vm_ioctl_mte_copy_tags().

Ignoring KVM_GET_NR_MMU_PAGES for the moment, I like the change, but would rather
phrase the justification along the lines of:

  KVM uses "long" return values for functions that are wired up to
  "struct file_operations", but otherwise uses "int" return values for
  functions that can return 0/-errno in order to avoid unintentional
  divergences between 32-bit and 64-bit kernels.

and then make the same change to kvm_arch_vm_ioctl() and all its subordinates.

As for KVM_GET_NR_MMU_PAGES, my vote would be just sweep it under the rug with a
comment and blurb in the documentation that it's broken.  I highly doubt any VMM
actually uses the ioctl() in any meaningful way as it was largely made obsolete by
two-dimensional paging, e.g. neither QEMU nor our VMM use it.

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  This patch is of very low importance - if you don't like it, please just
>  ignore. I just came across this nit while looking through the code and
>  thought that it might be somewhat nicer this way.
> 
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 64ec2222a196..e911331fc620 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4309,7 +4309,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
>  	return 0;
>  }
>  
> -static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
> +static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
>  {
>  	switch (arg) {
>  	case KVM_CAP_USER_MEMORY:
> -- 
> 2.31.1
> 

Re: [PATCH] KVM: Adjust the return type of kvm_vm_ioctl_check_extension_generic()

[Paolo Bonzini]

On Tue, May 31, 2022 at 9:39 PM Sean Christopherson <seanjc@google.com> wrote:
> As for KVM_GET_NR_MMU_PAGES, my vote would be just sweep it under the rug with a
> comment and blurb in the documentation that it's broken.  I highly doubt any VMM
> actually uses the ioctl() in any meaningful way as it was largely made obsolete by
> two-dimensional paging, e.g. neither QEMU nor our VMM use it.

I think we can just remove it, or return 0.

Paolo