Re: [PATCH] linux-user: syscall: ioctls: support DRM_IOCTL_I915_GETPARAM

[Laurent Vivier]

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