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.