Re: [PATCH] [PATCH v6] linux-user: syscall: ioctls: support DRM_IOCTL_VERSION

[Laurent Vivier]

Le 04/06/2020 à 03:45, chengang@emindsoft.com.cn a écrit :
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> Another DRM_IOCTL_* commands will be done later.
> 
> Signed-off-by: Chen Gang <chengang@emindsoft.com.cn>
> ---
>  configure                  | 10 ++++
>  linux-user/ioctls.h        |  5 ++
>  linux-user/syscall.c       | 95 ++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall_defs.h  | 15 ++++++
>  linux-user/syscall_types.h | 11 +++++
>  5 files changed, 136 insertions(+)
> 
> diff --git a/configure b/configure
> index f087d2bcd1..389dbb1d09 100755
> --- a/configure
> +++ b/configure
> @@ -3136,6 +3136,13 @@ if ! check_include "ifaddrs.h" ; then
>    have_ifaddrs_h=no
>  fi
>  
> +#########################################
> +# libdrm check
> +have_drm_h=no
> +if check_include "libdrm/drm.h" ; then
> +    have_drm_h=yes
> +fi
> +
>  ##########################################
>  # VTE probe
>  
> @@ -7127,6 +7134,9 @@ fi
>  if test "$have_ifaddrs_h" = "yes" ; then
>      echo "HAVE_IFADDRS_H=y" >> $config_host_mak
>  fi
> +if test "$have_drm_h" = "yes" ; then
> +  echo "HAVE_DRM_H=y" >> $config_host_mak
> +fi
>  if test "$have_broken_size_max" = "yes" ; then
>      echo "HAVE_BROKEN_SIZE_MAX=y" >> $config_host_mak
>  fi
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index 0defa1d8c1..f2e2fa9c87 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -574,6 +574,11 @@
>    IOCTL_SPECIAL(SIOCDELRT, IOC_W, do_ioctl_rt,
>                  MK_PTR(MK_STRUCT(STRUCT_rtentry)))
>  
> +#ifdef HAVE_DRM_H
> +  IOCTL_SPECIAL(DRM_IOCTL_VERSION, IOC_RW, do_ioctl_drm,
> +                MK_PTR(MK_STRUCT(STRUCT_drm_version)))
> +#endif
> +
>  #ifdef TARGET_TIOCSTART
>    IOCTL_IGNORE(TIOCSTART)
>    IOCTL_IGNORE(TIOCSTOP)
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 7f6700c54e..1744e7acc7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -112,6 +112,9 @@
>  #include <linux/if_alg.h>
>  #include <linux/rtc.h>
>  #include <sound/asound.h>
> +#ifdef HAVE_DRM_H
> +#include <libdrm/drm.h>
> +#endif
>  #include "linux_loop.h"
>  #include "uname.h"
>  
> @@ -5276,6 +5279,98 @@ static abi_long do_ioctl_tiocgptpeer(const IOCTLEntry 
> *ie, uint8_t *buf_temp,
>  }
>  #endif
>  
> +#ifdef HAVE_DRM_H
> +
> +static void unlock_drm_version(struct drm_version *host_ver)
> +{
> +    unlock_user(host_ver->name, 0UL, 0);
> +    unlock_user(host_ver->date, 0UL, 0);
> +    unlock_user(host_ver->desc, 0UL, 0);
> +}

This is correct but I would prefer to not have this kind of function
with 0UL parameter.

You can use target_ver for the guest pointer and host_ver for the len,
guest pointer will be ignored if host pointer is NULL.

You can have a generic function that not copies data in case of error:

static void unlock_drm_strings(struct drm_version *host_ver,
                               struct target_drm_version *target_ver,
                               int copy)
{
    unlock_user(host_ver->name, target_ver->name,
                copy ? host_ver->name_len : 0);
...

> +
> +static inline abi_long target_to_host_drmversion(struct drm_version 
> *host_ver,
> +                                          struct target_drm_version 
> *target_ver)
> +{
> +    memset(host_ver, 0, sizeof(*host_ver));
> +
> +    __get_user(host_ver->name_len, &target_ver->name_len);
> +    if (host_ver->name_len) {
> +        host_ver->name = lock_user(VERIFY_WRITE, target_ver->name,
> +                                   target_ver->name_len, 0);
> +        if (!host_ver->name) {
> +            goto err;
> +        }
> +    }
> +
> +    __get_user(host_ver->date_len, &target_ver->date_len);
> +    if (host_ver->date_len) {
> +        host_ver->date = lock_user(VERIFY_WRITE, target_ver->date,
> +                                   target_ver->date_len, 0);
> +        if (!host_ver->date) {
> +            goto err;
> +        }
> +    }
> +
> +    __get_user(host_ver->desc_len, &target_ver->desc_len);
> +    if (host_ver->desc_len) {
> +        host_ver->desc = lock_user(VERIFY_WRITE, target_ver->desc,
> +                                   target_ver->desc_len, 0);
> +        if (!host_ver->desc) {
> +            goto err;
> +        }
> +    }
> +
> +    return 0;
> +err:
> +    unlock_drm_version(host_ver);

unlock_drm_strings(host_ver, target_ver, 0);

> +    return -EFAULT;
> +}
> +
> +static inline abi_long host_to_target_drmversion(
> +                                          struct target_drm_version 
> *target_ver,
> +                                          struct drm_version *host_ver)
> +{
> +    __put_user(host_ver->version_major, &target_ver->version_major);
> +    __put_user(host_ver->version_minor, &target_ver->version_minor);
> +    __put_user(host_ver->version_patchlevel, 
> &target_ver->version_patchlevel);
> +    __put_user(host_ver->name_len, &target_ver->name_len);
> +    __put_user(host_ver->date_len, &target_ver->date_len);
> +    __put_user(host_ver->desc_len, &target_ver->desc_len);
> +    unlock_user(host_ver->name, target_ver->name, host_ver->name_len);
> +    unlock_user(host_ver->date, target_ver->date, host_ver->date_len);
> +    unlock_user(host_ver->desc, target_ver->desc, host_ver->desc_len);

unlock_drm_strings(host_ver, target_ver, 1);

> +    return 0;
> +}
> +
> +static abi_long do_ioctl_drm(const IOCTLEntry *ie, uint8_t *buf_temp,
> +                             int fd, int cmd, abi_long arg)
> +{
> +    struct drm_version *ver;
> +    struct target_drm_version *target_ver;
> +    abi_long ret;
> +
> +    switch (ie->host_cmd) {
> +    case DRM_IOCTL_VERSION:
> +        if (!lock_user_struct(VERIFY_WRITE, target_ver, arg, 0)) {
> +            return -TARGET_EFAULT;
> +        }
> +        ver = (struct drm_version *)buf_temp;
> +        ret = target_to_host_drmversion(ver, target_ver);
> +        if (!is_error(ret)) {
> +            ret = get_errno(safe_ioctl(fd, ie->host_cmd, ver));
> +            if (!is_error(ret)) {
> +                ret = host_to_target_drmversion(target_ver, ver);
> +            }
> +            unlock_drm_version(ver);

} else {
    unlock_drm_strings(host_ver, target_ver, 0);
}

Thanks,
Laurent