[PATCH v1] kvm: unsigned datatype in ioctl wrapper

Johannes Stoelp posted 1 patch 2 years, 7 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210901213426.360748-1-johannes.stoelp@gmail.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
accel/kvm/kvm-all.c    | 10 +++++-----
accel/kvm/trace-events |  8 ++++----
include/sysemu/kvm.h   |  8 ++++----
3 files changed, 13 insertions(+), 13 deletions(-)
[PATCH v1] kvm: unsigned datatype in ioctl wrapper
Posted by Johannes Stoelp 2 years, 7 months ago
Change the data type of the ioctl _request_ argument from 'int' to
'unsigned long' for the kvm ioctl wrappers.

POSIX defines the request argument as 'int' but the glibc defines the ioctl
call as follows
    int ioctl (int fd, unsigned long int request, ...);

Requests with the 0x8000_0000 bit set will be sign-extended.
Fortunately the Linux Kernel truncates the upper 32bit of the request on
64bit machines [1].

On x86_64 one such example is the KVM_IRQ_LINE_STATUS request.
But requests with the _IOC_READ direction bit set, will have the high
bit set.

[1]: https://sourceware.org/bugzilla/show_bug.cgi?id=14362

Signed-off-by: Johannes Stoelp <johannes.stoelp@gmail.com>
---
v1:
    - Changed ioctl request from 'unsigned' -> 'unsigned long' in kvm ioctl wrapper.
v0:
    - Changed ioctl request from 'int' -> 'unsigned' in kvm ioctl wrapper.
    - L: https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg00945.html

 accel/kvm/kvm-all.c    | 10 +++++-----
 accel/kvm/trace-events |  8 ++++----
 include/sysemu/kvm.h   |  8 ++++----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0125c17edb..5f9379f3cc 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -127,7 +127,7 @@ struct KVMState
     /* The man page (and posix) say ioctl numbers are signed int, but
      * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
      * unsigned, and treating them as signed here can break things */
-    unsigned irq_set_ioctl;
+    unsigned long irq_set_ioctl;
     unsigned int sigmask_len;
     GHashTable *gsimap;
 #ifdef KVM_CAP_IRQ_ROUTING
@@ -2967,7 +2967,7 @@ int kvm_cpu_exec(CPUState *cpu)
     return ret;
 }
 
-int kvm_ioctl(KVMState *s, int type, ...)
+int kvm_ioctl(KVMState *s, unsigned long type, ...)
 {
     int ret;
     void *arg;
@@ -2985,7 +2985,7 @@ int kvm_ioctl(KVMState *s, int type, ...)
     return ret;
 }
 
-int kvm_vm_ioctl(KVMState *s, int type, ...)
+int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
 {
     int ret;
     void *arg;
@@ -3003,7 +3003,7 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
     return ret;
 }
 
-int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
+int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
 {
     int ret;
     void *arg;
@@ -3021,7 +3021,7 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
     return ret;
 }
 
-int kvm_device_ioctl(int fd, int type, ...)
+int kvm_device_ioctl(int fd, unsigned long type, ...)
 {
     int ret;
     void *arg;
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index 399aaeb0ec..a1905fe985 100644
--- a/accel/kvm/trace-events
+++ b/accel/kvm/trace-events
@@ -1,11 +1,11 @@
 # See docs/devel/tracing.rst for syntax documentation.
 
 # kvm-all.c
-kvm_ioctl(int type, void *arg) "type 0x%x, arg %p"
-kvm_vm_ioctl(int type, void *arg) "type 0x%x, arg %p"
-kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type 0x%x, arg %p"
+kvm_ioctl(unsigned long type, void *arg) "type 0x%lx, arg %p"
+kvm_vm_ioctl(unsigned long type, void *arg) "type 0x%lx, arg %p"
+kvm_vcpu_ioctl(int cpu_index, unsigned long type, void *arg) "cpu_index %d, type 0x%lx, arg %p"
 kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
-kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
+kvm_device_ioctl(int fd, unsigned long type, void *arg) "dev fd %d, type 0x%lx, arg %p"
 kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
 kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
 kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a1ab1ee12d..d0f354a897 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -251,11 +251,11 @@ int kvm_on_sigbus(int code, void *addr);
 
 /* internal API */
 
-int kvm_ioctl(KVMState *s, int type, ...);
+int kvm_ioctl(KVMState *s, unsigned long type, ...);
 
-int kvm_vm_ioctl(KVMState *s, int type, ...);
+int kvm_vm_ioctl(KVMState *s, unsigned long type, ...);
 
-int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
+int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...);
 
 /**
  * kvm_device_ioctl - call an ioctl on a kvm device
@@ -264,7 +264,7 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
  *
  * Returns: -errno on error, nonnegative on success
  */
-int kvm_device_ioctl(int fd, int type, ...);
+int kvm_device_ioctl(int fd, unsigned long type, ...);
 
 /**
  * kvm_vm_check_attr - check for existence of a specific vm attribute
-- 
2.32.0


Re: [PATCH v1] kvm: unsigned datatype in ioctl wrapper
Posted by Peter Maydell 2 years, 7 months ago
On Wed, 1 Sept 2021 at 22:34, Johannes Stoelp
<johannes.stoelp@googlemail.com> wrote:
>
> Change the data type of the ioctl _request_ argument from 'int' to
> 'unsigned long' for the kvm ioctl wrappers.
>
> POSIX defines the request argument as 'int' but the glibc defines the ioctl
> call as follows
>     int ioctl (int fd, unsigned long int request, ...);
>
> Requests with the 0x8000_0000 bit set will be sign-extended.
> Fortunately the Linux Kernel truncates the upper 32bit of the request on
> 64bit machines [1].
>
> On x86_64 one such example is the KVM_IRQ_LINE_STATUS request.
> But requests with the _IOC_READ direction bit set, will have the high
> bit set.
>
> [1]: https://sourceware.org/bugzilla/show_bug.cgi?id=14362
>
> Signed-off-by: Johannes Stoelp <johannes.stoelp@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM