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

[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 = &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.