diff mbox series

linux-user: syscall: ioctls: support DRM_IOCTL_I915_GETPARAM

Message ID 20200712034102.23355-1-chengang@emindsoft.com.cn (mailing list archive)
State New, archived
Headers show
Series linux-user: syscall: ioctls: support DRM_IOCTL_I915_GETPARAM | expand

Commit Message

Chen Gang July 12, 2020, 3:41 a.m. UTC 
From: Chen Gang <chengang@emindsoft.com.cn>

It is for i915 drm command, and next, I shall send another i915 commands
implementations.

Signed-off-by: Chen Gang <chengang@emindsoft.com.cn>
---
 linux-user/ioctls.h        |  3 +++
 linux-user/syscall.c       | 39 ++++++++++++++++++++++++++++++++++++++
 linux-user/syscall_defs.h  |  9 +++++++++
 linux-user/syscall_types.h |  4 ++++
 4 files changed, 55 insertions(+)

Comments

Laurent Vivier July 13, 2020, 6:46 p.m. UTC | #1 
Le 12/07/2020 à 05:41, chengang@emindsoft.com.cn a écrit :
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> It is for i915 drm command, and next, I shall send another i915 commands
> implementations.
> 
> Signed-off-by: Chen Gang <chengang@emindsoft.com.cn>
> ---
>  linux-user/ioctls.h        |  3 +++
>  linux-user/syscall.c       | 39 ++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall_defs.h  |  9 +++++++++
>  linux-user/syscall_types.h |  4 ++++
>  4 files changed, 55 insertions(+)
> 
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index f2e2fa9c87..83e045deba 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -577,6 +577,9 @@
>  #ifdef HAVE_DRM_H
>    IOCTL_SPECIAL(DRM_IOCTL_VERSION, IOC_RW, do_ioctl_drm,
>                  MK_PTR(MK_STRUCT(STRUCT_drm_version)))
> +
> +  IOCTL_SPECIAL(DRM_IOCTL_I915_GETPARAM, IOC_RW, do_ioctl_drm_i915,
> +                MK_PTR(MK_STRUCT(STRUCT_drm_i915_getparam)))
>  #endif
>  
>  #ifdef TARGET_TIOCSTART
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 17ed7f8d6b..6fab9064af 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -114,6 +114,7 @@
>  #include <sound/asound.h>
>  #ifdef HAVE_DRM_H
>  #include <libdrm/drm.h>
> +#include <libdrm/i915_drm.h>
>  #endif
>  #include "linux_loop.h"
>  #include "uname.h"
> @@ -5372,6 +5373,44 @@ static abi_long do_ioctl_drm(const IOCTLEntry *ie, uint8_t *buf_temp,
>      return -TARGET_ENOSYS;
>  }
>  
> +static abi_long do_ioctl_drm_i915_getparam(const IOCTLEntry *ie,
> +                                           struct drm_i915_getparam *gparam,
> +                                           int fd, abi_long arg)
> +{
> +    abi_long ret;
> +    struct target_drm_i915_getparam *target_gparam;
> +
> +    if (!lock_user_struct(VERIFY_READ, target_gparam, arg, 0)) {
> +        return -TARGET_EFAULT;
> +    }
> +    __get_user(gparam->param, &target_gparam->param);
> +    gparam->value = lock_user(VERIFY_WRITE, target_gparam->value,
> +                             sizeof(*gparam->value), 0);

I don't think you should use directly the guest memory.
You should have something like that:

     int value;

     gparam->value = &value;

> +    if (!gparam->value) {
> +        unlock_user_struct(target_gparam, arg, 0);
> +        return -TARGET_EFAULT;
> +    }
> +
> +    ret = get_errno(safe_ioctl(fd, ie->host_cmd, gparam));

and then:

put_user_s32(value, target_gparam->value);

> +
> +    unlock_user(gparam->value, target_gparam->value, sizeof(*gparam->value));
> +    unlock_user_struct(target_gparam, arg, 0);
> +    return ret;
> +}
> +
> +static abi_long do_ioctl_drm_i915(const IOCTLEntry *ie, uint8_t *buf_temp,
> +                                  int fd, int cmd, abi_long arg)
> +{
> +    switch (ie->host_cmd) {
> +    case DRM_IOCTL_I915_GETPARAM:
> +        return do_ioctl_drm_i915_getparam(ie,
> +                                          (struct drm_i915_getparam *)buf_temp,
> +                                          fd, arg);
> +    default:
> +        return -TARGET_ENOSYS;
> +    }
> +}

There is a better way to register a struct with convertion functions:
you might use STRUCT_SPECIAL() to declare the structure rather than
IOCTL_SPECIAL() to declare the ioctl command.
(I think STRUCT_SPECIAL() could also be used with DRM_IOCTL_VERSION)

> +
>  #endif
>  
>  static IOCTLEntry ioctl_entries[] = {
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 3c261cff0e..9082f6c2bc 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -1170,6 +1170,9 @@ struct target_rtc_pll_info {
>  /* drm ioctls */
>  #define TARGET_DRM_IOCTL_VERSION      TARGET_IOWRU('d', 0x00)
>  
> +/* drm i915 ioctls */
> +#define TARGET_DRM_IOCTL_I915_GETPARAM              TARGET_IOWRU('d', 0x46)

why do you use the U variant?

TARGET_IOWR('d', 0x46, struct target_drm_i915_getparam)

> +
>  /* from asm/termbits.h */
>  
>  #define TARGET_NCC 8
> @@ -2613,6 +2616,12 @@ struct target_drm_version {
>      abi_ulong desc;
>  };
>  
> +struct target_drm_i915_getparam {
> +    int param;
> +    abi_ulong value;
> +};
> +
> +
>  #include "socket.h"
>  
>  #include "errno_defs.h"
> diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h
> index e2b0484f50..ef60d5f38c 100644
> --- a/linux-user/syscall_types.h
> +++ b/linux-user/syscall_types.h
> @@ -303,6 +303,10 @@ STRUCT(drm_version,
>         TYPE_ULONG, /* desc_len */
>         TYPE_PTRVOID) /* desc */
>  
> +STRUCT(drm_i915_getparam,
> +       TYPE_INT, /* param */
> +       TYPE_PTRVOID) /* value */
> +
>  STRUCT(file_clone_range,
>         TYPE_LONGLONG, /* src_fd */
>         TYPE_ULONGLONG, /* src_offset */
> 

Thanks,
Laurent
Chen Gang July 18, 2020, 11:23 p.m. UTC | #2 
On 2020/7/14 上午2:46, Laurent Vivier wrote:
>> +    gparam->value = lock_user(VERIFY_WRITE, target_gparam->value,
>> +                             sizeof(*gparam->value), 0);
> 
> I don't think you should use directly the guest memory.
> You should have something like that:
> 
>      int value;
> 
>      gparam->value = &value;
> 
>> +    if (!gparam->value) {
>> +        unlock_user_struct(target_gparam, arg, 0);
>> +        return -TARGET_EFAULT;
>> +    }
>> +
>> +    ret = get_errno(safe_ioctl(fd, ie->host_cmd, gparam));
> 
> and then:
> 
> put_user_s32(value, target_gparam->value);
> 

OK, thanks. It will be better.

>> +
>> +    unlock_user(gparam->value, target_gparam->value, sizeof(*gparam->value));
>> +    unlock_user_struct(target_gparam, arg, 0);
>> +    return ret;
>> +}
>> +
>> +static abi_long do_ioctl_drm_i915(const IOCTLEntry *ie, uint8_t *buf_temp,
>> +                                  int fd, int cmd, abi_long arg)
>> +{
>> +    switch (ie->host_cmd) {
>> +    case DRM_IOCTL_I915_GETPARAM:
>> +        return do_ioctl_drm_i915_getparam(ie,
>> +                                          (struct drm_i915_getparam *)buf_temp,
>> +                                          fd, arg);
>> +    default:
>> +        return -TARGET_ENOSYS;
>> +    }
>> +}
> 
> There is a better way to register a struct with convertion functions:
> you might use STRUCT_SPECIAL() to declare the structure rather than
> IOCTL_SPECIAL() to declare the ioctl command.
> (I think STRUCT_SPECIAL() could also be used with DRM_IOCTL_VERSION)
> 

For me, STRUCT_SPECIAL sounds good for the complex structures, but for
drm_i915_getparam which is simple enough, it is not quite suitable.

For me, STRUCT_SPECIAL is much suitable for DRM_IOCTL_VERSION.

Welcome your additional ideas.

>>  
>>  static IOCTLEntry ioctl_entries[] = {
>> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
>> index 3c261cff0e..9082f6c2bc 100644
>> --- a/linux-user/syscall_defs.h
>> +++ b/linux-user/syscall_defs.h
>> @@ -1170,6 +1170,9 @@ struct target_rtc_pll_info {
>>  /* drm ioctls */
>>  #define TARGET_DRM_IOCTL_VERSION      TARGET_IOWRU('d', 0x00)
>>  
>> +/* drm i915 ioctls */
>> +#define TARGET_DRM_IOCTL_I915_GETPARAM              TARGET_IOWRU('d', 0x46)
> 
> why do you use the U variant?
> 
> TARGET_IOWR('d', 0x46, struct target_drm_i915_getparam)
> 

Because qemu will automatically set the size with the target structure
size in syscall_init(). It'll be more easier. e.g. usb ioctls and device
mapper ioctls also do like this.
diff mbox series

Patch

diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index f2e2fa9c87..83e045deba 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -577,6 +577,9 @@ 
 #ifdef HAVE_DRM_H
   IOCTL_SPECIAL(DRM_IOCTL_VERSION, IOC_RW, do_ioctl_drm,
                 MK_PTR(MK_STRUCT(STRUCT_drm_version)))
+
+  IOCTL_SPECIAL(DRM_IOCTL_I915_GETPARAM, IOC_RW, do_ioctl_drm_i915,
+                MK_PTR(MK_STRUCT(STRUCT_drm_i915_getparam)))
 #endif
 
 #ifdef TARGET_TIOCSTART
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 17ed7f8d6b..6fab9064af 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -114,6 +114,7 @@ 
 #include <sound/asound.h>
 #ifdef HAVE_DRM_H
 #include <libdrm/drm.h>
+#include <libdrm/i915_drm.h>
 #endif
 #include "linux_loop.h"
 #include "uname.h"
@@ -5372,6 +5373,44 @@  static abi_long do_ioctl_drm(const IOCTLEntry *ie, uint8_t *buf_temp,
     return -TARGET_ENOSYS;
 }
 
+static abi_long do_ioctl_drm_i915_getparam(const IOCTLEntry *ie,
+                                           struct drm_i915_getparam *gparam,
+                                           int fd, abi_long arg)
+{
+    abi_long ret;
+    struct target_drm_i915_getparam *target_gparam;
+
+    if (!lock_user_struct(VERIFY_READ, target_gparam, arg, 0)) {
+        return -TARGET_EFAULT;
+    }
+    __get_user(gparam->param, &target_gparam->param);
+    gparam->value = lock_user(VERIFY_WRITE, target_gparam->value,
+                             sizeof(*gparam->value), 0);
+    if (!gparam->value) {
+        unlock_user_struct(target_gparam, arg, 0);
+        return -TARGET_EFAULT;
+    }
+
+    ret = get_errno(safe_ioctl(fd, ie->host_cmd, gparam));
+
+    unlock_user(gparam->value, target_gparam->value, sizeof(*gparam->value));
+    unlock_user_struct(target_gparam, arg, 0);
+    return ret;
+}
+
+static abi_long do_ioctl_drm_i915(const IOCTLEntry *ie, uint8_t *buf_temp,
+                                  int fd, int cmd, abi_long arg)
+{
+    switch (ie->host_cmd) {
+    case DRM_IOCTL_I915_GETPARAM:
+        return do_ioctl_drm_i915_getparam(ie,
+                                          (struct drm_i915_getparam *)buf_temp,
+                                          fd, arg);
+    default:
+        return -TARGET_ENOSYS;
+    }
+}
+
 #endif
 
 static IOCTLEntry ioctl_entries[] = {
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 3c261cff0e..9082f6c2bc 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -1170,6 +1170,9 @@  struct target_rtc_pll_info {
 /* drm ioctls */
 #define TARGET_DRM_IOCTL_VERSION      TARGET_IOWRU('d', 0x00)
 
+/* drm i915 ioctls */
+#define TARGET_DRM_IOCTL_I915_GETPARAM              TARGET_IOWRU('d', 0x46)
+
 /* from asm/termbits.h */
 
 #define TARGET_NCC 8
@@ -2613,6 +2616,12 @@  struct target_drm_version {
     abi_ulong desc;
 };
 
+struct target_drm_i915_getparam {
+    int param;
+    abi_ulong value;
+};
+
+
 #include "socket.h"
 
 #include "errno_defs.h"
diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h
index e2b0484f50..ef60d5f38c 100644
--- a/linux-user/syscall_types.h
+++ b/linux-user/syscall_types.h
@@ -303,6 +303,10 @@  STRUCT(drm_version,
        TYPE_ULONG, /* desc_len */
        TYPE_PTRVOID) /* desc */
 
+STRUCT(drm_i915_getparam,
+       TYPE_INT, /* param */
+       TYPE_PTRVOID) /* value */
+
 STRUCT(file_clone_range,
        TYPE_LONGLONG, /* src_fd */
        TYPE_ULONGLONG, /* src_offset */