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

2020-07-18 Thread Chen Gang
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 = 
> 
>> +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.





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

2020-07-13 Thread Laurent Vivier
Le 12/07/2020 à 05:41, cheng...@emindsoft.com.cn a écrit :
> From: Chen Gang 
> 
> It is for i915 drm command, and next, I shall send another i915 commands
> implementations.
> 
> Signed-off-by: Chen Gang 
> ---
>  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 
>  #ifdef HAVE_DRM_H
>  #include 
> +#include 
>  #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, _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 = 

> +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