[PATCH v3 0/2] linux-user: fixes for sched_ syscalls

[PATCH v3 0/2] linux-user: fixes for sched_ syscalls

[Tonis Tiigi]

This patchset improves support for sched_* syscalls under user emulation. The 
first commit adds support for sched_g/setattr that was previously not 
implemented. There is no equivalent for these syscalls in libc, so I needed to 
redefine the struct from kernel. It can't be included directly because of 
conflict with libc sched headers.

The second commit fixes sched_g/setscheduler and sched_g/setparam, when QEMU is 
built with musl. Musl does not implement these due to conflict between what 
these functions should do in syscalls and libc 
https://git.musl-libc.org/cgit/musl/commit/?id=1e21e78bf7a5c24c217446d8760be7b7188711c2
 . I've changed it to call syscall directly what should always be the expected 
behavior for the user.

Via https://github.com/tonistiigi/binfmt/pull/70 with additional tests.

Changes v3->v2:
- Fix wrong property name for sched_flags
- Validate size parameter and handle E2BIG errors same way as kernel does. There
is one case where it can't be done completely correctly but clients should still
be able to handle it: when client sends a bigger non-zero structure than current
kernel definition we will send E2BIG with the struct size known to qemu. If now
the client sends structure with this size it may still get another E2BIG error
from the kernel if kernel is old and doesn't implement util_min/util_max. I don't
think this can be handled without making additional syscalls to kernel.

Changes v1->v2:
- Locking guest addresses for sched_attr is now based on size inputs, not local 
struct size. Also did the same for setter where I now read only the size field 
of the struct first.
- Use offsetof() when checking if optional fields are supported.
- `target_sched_attr` now uses aligned types as requested. I didn't quite 
understand why this is needed as I don't see same in kernel headers, but as 
this type uses only constant width fields and is already aligned by default it 
can't break anything.
- Fixed formatting.
- Defined own `target_sched_param` struct as requested.


Tonis Tiigi (2):
  linux-user: add sched_getattr support
  linux-user: call set/getscheduler set/getparam directly

 linux-user/syscall.c      | 134 ++++++++++++++++++++++++++++++++++----
 linux-user/syscall_defs.h |  18 +++++
 2 files changed, 139 insertions(+), 13 deletions(-)

-- 
2.32.0 (Apple Git-132)

[PATCH v3 1/2] linux-user: add sched_getattr support

[Tonis Tiigi]

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
---
 linux-user/syscall.c      | 94 +++++++++++++++++++++++++++++++++++++++
 linux-user/syscall_defs.h | 14 ++++++
 2 files changed, 108 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f1cfcc8104..2f5a0fac5a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -339,6 +339,12 @@ _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
 #define __NR_sys_sched_setaffinity __NR_sched_setaffinity
 _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned int, len,
           unsigned long *, user_mask_ptr);
+#define __NR_sys_sched_getattr __NR_sched_getattr
+_syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
+          unsigned int, size, unsigned int, flags);
+#define __NR_sys_sched_setattr __NR_sched_setattr
+_syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
+          unsigned int, flags);
 #define __NR_sys_getcpu __NR_getcpu
 _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
 _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
@@ -10593,6 +10599,94 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
         }
     case TARGET_NR_sched_getscheduler:
         return get_errno(sched_getscheduler(arg1));
+    case TARGET_NR_sched_getattr:
+        {
+            struct target_sched_attr *target_scha;
+            struct target_sched_attr scha;
+            if (arg2 == 0) {
+                return -TARGET_EINVAL;
+            }
+            if (arg3 > sizeof(scha)) {
+                arg3 = sizeof(scha);
+            }
+            ret = get_errno(sys_sched_getattr(arg1, &scha, arg3, arg4));
+            if (!is_error(ret)) {
+                target_scha = lock_user(VERIFY_WRITE, arg2, arg3, 0);
+                if (!target_scha) {
+                    return -TARGET_EFAULT;
+                }
+                target_scha->size = tswap32(scha.size);
+                target_scha->sched_policy = tswap32(scha.sched_policy);
+                target_scha->sched_flags = tswap64(scha.sched_flags);
+                target_scha->sched_nice = tswap32(scha.sched_nice);
+                target_scha->sched_priority = tswap32(scha.sched_priority);
+                target_scha->sched_runtime = tswap64(scha.sched_runtime);
+                target_scha->sched_deadline = tswap64(scha.sched_deadline);
+                target_scha->sched_period = tswap64(scha.sched_period);
+                if (scha.size > offsetof(struct target_sched_attr, sched_util_min)) {
+                    target_scha->sched_util_min = tswap32(scha.sched_util_min);
+                    target_scha->sched_util_max = tswap32(scha.sched_util_max);
+                }
+                unlock_user(target_scha, arg2, arg3);
+            }
+            return ret;
+        }
+    case TARGET_NR_sched_setattr:
+        {
+            struct target_sched_attr *target_scha;
+            struct target_sched_attr scha;
+            if (arg2 == 0) {
+                return -TARGET_EINVAL;
+            }
+            uint32_t size;
+            if (get_user_u32(size, arg2)) {
+                return -TARGET_EFAULT;
+            }
+            if (!size) {
+                size = offsetof(struct target_sched_attr, sched_util_min);
+            }
+            if (size < offsetof(struct target_sched_attr, sched_util_min)) {
+                if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
+                    return -TARGET_EFAULT;
+                }
+                return -TARGET_E2BIG;
+            }
+
+            if (size > sizeof(scha)) {
+                for (int i = sizeof(scha); i < size; i++) {
+                    uint8_t b;
+                    if (get_user_u8(b, arg2 + i)) {
+                        return -TARGET_EFAULT;
+                    }
+                    if (b != 0) {
+                        if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
+                            return -TARGET_EFAULT;
+                        }
+                        return -TARGET_E2BIG;
+                    }
+                }
+                size = sizeof(scha);
+            }
+
+            target_scha = lock_user(VERIFY_READ, arg2, size, 1);
+            if (!target_scha) {
+                return -TARGET_EFAULT;
+            }
+            scha.size = size;
+            scha.sched_policy = tswap32(target_scha->sched_policy);
+            scha.sched_flags = tswap64(target_scha->sched_flags);
+            scha.sched_nice = tswap32(target_scha->sched_nice);
+            scha.sched_priority = tswap32(target_scha->sched_priority);
+            scha.sched_runtime = tswap64(target_scha->sched_runtime);
+            scha.sched_deadline = tswap64(target_scha->sched_deadline);
+            scha.sched_period = tswap64(target_scha->sched_period);
+            if (size > offsetof(struct target_sched_attr, sched_util_min)) {
+                scha.sched_util_min = tswap32(target_scha->sched_util_min);
+                scha.sched_util_max = tswap32(target_scha->sched_util_max);
+            }
+            unlock_user(target_scha, arg2, 0);
+            return get_errno(sys_sched_setattr(arg1, &scha, arg3));
+        }
     case TARGET_NR_sched_yield:
         return get_errno(sched_yield());
     case TARGET_NR_sched_get_priority_max:
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 0b13975937..310d6ce8ad 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2914,4 +2914,18 @@ struct target_statx {
    /* 0x100 */
 };
 
+/* from kernel's include/linux/sched/types.h */
+struct target_sched_attr {
+    abi_uint size;
+    abi_uint sched_policy;
+    abi_ullong sched_flags;
+    abi_int sched_nice;
+    abi_uint sched_priority;
+    abi_ullong sched_runtime;
+    abi_ullong sched_deadline;
+    abi_ullong sched_period;
+    abi_uint sched_util_min;
+    abi_uint sched_util_max;
+};
+
 #endif
-- 
2.32.0 (Apple Git-132)

[PATCH v3 2/2] linux-user: call set/getscheduler set/getparam directly

[Tonis Tiigi]

There seems to be difference in syscall and libc definition of these
methods and therefore musl does not implement them (1e21e78bf7). Call
syscall directly to ensure the behavior of the libc of user application,
not the libc that was used to build QEMU.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
---
 linux-user/syscall.c      | 40 ++++++++++++++++++++++++++-------------
 linux-user/syscall_defs.h |  4 ++++
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2f5a0fac5a..8c03a52a36 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -345,6 +345,17 @@ _syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
 #define __NR_sys_sched_setattr __NR_sched_setattr
 _syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
           unsigned int, flags);
+#define __NR_sys_sched_getscheduler __NR_sched_getscheduler
+_syscall1(int, sys_sched_getscheduler, pid_t, pid);
+#define __NR_sys_sched_setscheduler __NR_sched_setscheduler
+_syscall3(int, sys_sched_setscheduler, pid_t, pid, int, policy,
+          const struct target_sched_param *, param);
+#define __NR_sys_sched_getparam __NR_sched_getparam
+_syscall2(int, sys_sched_getparam, pid_t, pid,
+          struct target_sched_param *, param);
+#define __NR_sys_sched_setparam __NR_sched_setparam
+_syscall2(int, sys_sched_setparam, pid_t, pid,
+          const struct target_sched_param *, param);
 #define __NR_sys_getcpu __NR_getcpu
 _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
 _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
@@ -10555,30 +10566,32 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
         return ret;
     case TARGET_NR_sched_setparam:
         {
-            struct sched_param *target_schp;
-            struct sched_param schp;
+            struct target_sched_param *target_schp;
+            struct target_sched_param schp;
 
             if (arg2 == 0) {
                 return -TARGET_EINVAL;
             }
-            if (!lock_user_struct(VERIFY_READ, target_schp, arg2, 1))
+            if (!lock_user_struct(VERIFY_READ, target_schp, arg2, 1)) {
                 return -TARGET_EFAULT;
+            }
             schp.sched_priority = tswap32(target_schp->sched_priority);
             unlock_user_struct(target_schp, arg2, 0);
-            return get_errno(sched_setparam(arg1, &schp));
+            return get_errno(sys_sched_setparam(arg1, &schp));
         }
     case TARGET_NR_sched_getparam:
         {
-            struct sched_param *target_schp;
-            struct sched_param schp;
+            struct target_sched_param *target_schp;
+            struct target_sched_param schp;
 
             if (arg2 == 0) {
                 return -TARGET_EINVAL;
             }
-            ret = get_errno(sched_getparam(arg1, &schp));
+            ret = get_errno(sys_sched_getparam(arg1, &schp));
             if (!is_error(ret)) {
-                if (!lock_user_struct(VERIFY_WRITE, target_schp, arg2, 0))
+                if (!lock_user_struct(VERIFY_WRITE, target_schp, arg2, 0)) {
                     return -TARGET_EFAULT;
+                }
                 target_schp->sched_priority = tswap32(schp.sched_priority);
                 unlock_user_struct(target_schp, arg2, 1);
             }
@@ -10586,19 +10599,20 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
         return ret;
     case TARGET_NR_sched_setscheduler:
         {
-            struct sched_param *target_schp;
-            struct sched_param schp;
+            struct target_sched_param *target_schp;
+            struct target_sched_param schp;
             if (arg3 == 0) {
                 return -TARGET_EINVAL;
             }
-            if (!lock_user_struct(VERIFY_READ, target_schp, arg3, 1))
+            if (!lock_user_struct(VERIFY_READ, target_schp, arg3, 1)) {
                 return -TARGET_EFAULT;
+            }
             schp.sched_priority = tswap32(target_schp->sched_priority);
             unlock_user_struct(target_schp, arg3, 0);
-            return get_errno(sched_setscheduler(arg1, arg2, &schp));
+            return get_errno(sys_sched_setscheduler(arg1, arg2, &schp));
         }
     case TARGET_NR_sched_getscheduler:
-        return get_errno(sched_getscheduler(arg1));
+        return get_errno(sys_sched_getscheduler(arg1));
     case TARGET_NR_sched_getattr:
         {
             struct target_sched_attr *target_scha;
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 310d6ce8ad..28b9fe9a47 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2928,4 +2928,8 @@ struct target_sched_attr {
     abi_uint sched_util_max;
 };
 
+struct target_sched_param {
+    abi_int sched_priority;
+};
+
 #endif
-- 
2.32.0 (Apple Git-132)

Re: [PATCH v3 2/2] linux-user: call set/getscheduler set/getparam directly

[Laurent Vivier]

Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :
> There seems to be difference in syscall and libc definition of these
> methods and therefore musl does not implement them (1e21e78bf7). Call
> syscall directly to ensure the behavior of the libc of user application,
> not the libc that was used to build QEMU.
> 
> Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
> ---
>   linux-user/syscall.c      | 40 ++++++++++++++++++++++++++-------------
>   linux-user/syscall_defs.h |  4 ++++
>   2 files changed, 31 insertions(+), 13 deletions(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v3 2/2] linux-user: call set/getscheduler set/getparam directly

[Laurent Vivier]

Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :
> There seems to be difference in syscall and libc definition of these
> methods and therefore musl does not implement them (1e21e78bf7). Call
> syscall directly to ensure the behavior of the libc of user application,
> not the libc that was used to build QEMU.
> 
> Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
> ---
>   linux-user/syscall.c      | 40 ++++++++++++++++++++++++++-------------
>   linux-user/syscall_defs.h |  4 ++++
>   2 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2f5a0fac5a..8c03a52a36 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -345,6 +345,17 @@ _syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
>   #define __NR_sys_sched_setattr __NR_sched_setattr
>   _syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
>             unsigned int, flags);
> +#define __NR_sys_sched_getscheduler __NR_sched_getscheduler
> +_syscall1(int, sys_sched_getscheduler, pid_t, pid);
> +#define __NR_sys_sched_setscheduler __NR_sched_setscheduler
> +_syscall3(int, sys_sched_setscheduler, pid_t, pid, int, policy,
> +          const struct target_sched_param *, param);
> +#define __NR_sys_sched_getparam __NR_sched_getparam
> +_syscall2(int, sys_sched_getparam, pid_t, pid,
> +          struct target_sched_param *, param);
> +#define __NR_sys_sched_setparam __NR_sched_setparam
> +_syscall2(int, sys_sched_setparam, pid_t, pid,
> +          const struct target_sched_param *, param);
>   #define __NR_sys_getcpu __NR_getcpu
>   _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
>   _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
> @@ -10555,30 +10566,32 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>           return ret;
>       case TARGET_NR_sched_setparam:
>           {
> -            struct sched_param *target_schp;
> -            struct sched_param schp;
> +            struct target_sched_param *target_schp;
> +            struct target_sched_param schp;

You need to keep sched_param for schp as it is used with host syscall.

>   
>               if (arg2 == 0) {
>                   return -TARGET_EINVAL;
>               }
> -            if (!lock_user_struct(VERIFY_READ, target_schp, arg2, 1))
> +            if (!lock_user_struct(VERIFY_READ, target_schp, arg2, 1)) {
>                   return -TARGET_EFAULT;
> +            }
>               schp.sched_priority = tswap32(target_schp->sched_priority);
>               unlock_user_struct(target_schp, arg2, 0);
> -            return get_errno(sched_setparam(arg1, &schp));
> +            return get_errno(sys_sched_setparam(arg1, &schp));
>           }
>       case TARGET_NR_sched_getparam:
>           {
> -            struct sched_param *target_schp;
> -            struct sched_param schp;
> +            struct target_sched_param *target_schp;
> +            struct target_sched_param schp;

You need to keep sched_param for schp as it is used with host syscall.

>   
>               if (arg2 == 0) {
>                   return -TARGET_EINVAL;
>               }
> -            ret = get_errno(sched_getparam(arg1, &schp));
> +            ret = get_errno(sys_sched_getparam(arg1, &schp));
>               if (!is_error(ret)) {
> -                if (!lock_user_struct(VERIFY_WRITE, target_schp, arg2, 0))
> +                if (!lock_user_struct(VERIFY_WRITE, target_schp, arg2, 0)) {
>                       return -TARGET_EFAULT;
> +                }
>                   target_schp->sched_priority = tswap32(schp.sched_priority);
>                   unlock_user_struct(target_schp, arg2, 1);
>               }
> @@ -10586,19 +10599,20 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>           return ret;
>       case TARGET_NR_sched_setscheduler:
>           {
> -            struct sched_param *target_schp;
> -            struct sched_param schp;
> +            struct target_sched_param *target_schp;
> +            struct target_sched_param schp;

You need to keep sched_param for schp as it is used with host syscall.

>               if (arg3 == 0) {
>                   return -TARGET_EINVAL;
>               }
> -            if (!lock_user_struct(VERIFY_READ, target_schp, arg3, 1))
> +            if (!lock_user_struct(VERIFY_READ, target_schp, arg3, 1)) {
>                   return -TARGET_EFAULT;
> +            }
>               schp.sched_priority = tswap32(target_schp->sched_priority);
>               unlock_user_struct(target_schp, arg3, 0);
> -            return get_errno(sched_setscheduler(arg1, arg2, &schp));
> +            return get_errno(sys_sched_setscheduler(arg1, arg2, &schp));
>           }
>       case TARGET_NR_sched_getscheduler:
> -        return get_errno(sched_getscheduler(arg1));
> +        return get_errno(sys_sched_getscheduler(arg1));
>       case TARGET_NR_sched_getattr:
>           {
>               struct target_sched_attr *target_scha;
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 310d6ce8ad..28b9fe9a47 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2928,4 +2928,8 @@ struct target_sched_attr {
>       abi_uint sched_util_max;
>   };
>   
> +struct target_sched_param {
> +    abi_int sched_priority;
> +};
> +
>   #endif

Sorry, I missed these problem in my previous review.

Thanks,
Laurent

Re: [PATCH v3 1/2] linux-user: add sched_getattr support

[Laurent Vivier]

Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :

Please copy here what you explain in PATCH 0 regarding this patch.
(do the same for PATCH 1)

> Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
> ---
>   linux-user/syscall.c      | 94 +++++++++++++++++++++++++++++++++++++++
>   linux-user/syscall_defs.h | 14 ++++++
>   2 files changed, 108 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f1cfcc8104..2f5a0fac5a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -339,6 +339,12 @@ _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
>   #define __NR_sys_sched_setaffinity __NR_sched_setaffinity
>   _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned int, len,
>             unsigned long *, user_mask_ptr);
> +#define __NR_sys_sched_getattr __NR_sched_getattr
> +_syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
> +          unsigned int, size, unsigned int, flags);
> +#define __NR_sys_sched_setattr __NR_sched_setattr
> +_syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
> +          unsigned int, flags);
>   #define __NR_sys_getcpu __NR_getcpu
>   _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
>   _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
> @@ -10593,6 +10599,94 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>           }
>       case TARGET_NR_sched_getscheduler:
>           return get_errno(sched_getscheduler(arg1));
> +    case TARGET_NR_sched_getattr:
> +        {
> +            struct target_sched_attr *target_scha;
> +            struct target_sched_attr scha;

In fact, this scha is used with the host syscall, so it must be  sched_attr.

> +            if (arg2 == 0) {
> +                return -TARGET_EINVAL;
> +            }
> +            if (arg3 > sizeof(scha)) {
> +                arg3 = sizeof(scha);
> +            }
> +            ret = get_errno(sys_sched_getattr(arg1, &scha, arg3, arg4));
> +            if (!is_error(ret)) {
> +                target_scha = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> +                if (!target_scha) {
> +                    return -TARGET_EFAULT;
> +                }
> +                target_scha->size = tswap32(scha.size);
> +                target_scha->sched_policy = tswap32(scha.sched_policy);
> +                target_scha->sched_flags = tswap64(scha.sched_flags);
> +                target_scha->sched_nice = tswap32(scha.sched_nice);
> +                target_scha->sched_priority = tswap32(scha.sched_priority);
> +                target_scha->sched_runtime = tswap64(scha.sched_runtime);
> +                target_scha->sched_deadline = tswap64(scha.sched_deadline);
> +                target_scha->sched_period = tswap64(scha.sched_period);
> +                if (scha.size > offsetof(struct target_sched_attr, sched_util_min)) {
> +                    target_scha->sched_util_min = tswap32(scha.sched_util_min);
> +                    target_scha->sched_util_max = tswap32(scha.sched_util_max);
> +                }
> +                unlock_user(target_scha, arg2, arg3);
> +            }
> +            return ret;
> +        }
> +    case TARGET_NR_sched_setattr:
> +        {
> +            struct target_sched_attr *target_scha;
> +            struct target_sched_attr scha;

scha is sched_attr as it is used with the host syscall.


> +            if (arg2 == 0) {
> +                return -TARGET_EINVAL;
> +            }
> +            uint32_t size;

QEMU coding style doesn't allow to mix declarations and statements.

> +            if (get_user_u32(size, arg2)) {
> +                return -TARGET_EFAULT;
> +            }
> +            if (!size) {
> +                size = offsetof(struct target_sched_attr, sched_util_min);
> +            }
> +            if (size < offsetof(struct target_sched_attr, sched_util_min)) {
> +                if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> +                    return -TARGET_EFAULT;
> +                }
> +                return -TARGET_E2BIG;
> +            }
> +
> +            if (size > sizeof(scha)) {
> +                for (int i = sizeof(scha); i < size; i++) {
> +                    uint8_t b;
> +                    if (get_user_u8(b, arg2 + i)) {
> +                        return -TARGET_EFAULT;
> +                    }
> +                    if (b != 0) {
> +                        if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> +                            return -TARGET_EFAULT;
> +                        }
> +                        return -TARGET_E2BIG;
> +                    }
> +                }
> +                size = sizeof(scha);
> +            }

I guess this is the code to mimic kernel copy_struct_from_user(), the part when usize > ksize.

It's a little bit ugly, but I can't disagree because the kernel does the same.

except that the kernel check for unsigned rather than for 8bit. Could you change that?

The best would be to define check_zeroed_user() in Qemu and use it here.

> +
> +            target_scha = lock_user(VERIFY_READ, arg2, size, 1);
> +            if (!target_scha) {
> +                return -TARGET_EFAULT;
> +            }
> +            scha.size = size;
> +            scha.sched_policy = tswap32(target_scha->sched_policy);
> +            scha.sched_flags = tswap64(target_scha->sched_flags);
> +            scha.sched_nice = tswap32(target_scha->sched_nice);
> +            scha.sched_priority = tswap32(target_scha->sched_priority);
> +            scha.sched_runtime = tswap64(target_scha->sched_runtime);
> +            scha.sched_deadline = tswap64(target_scha->sched_deadline);
> +            scha.sched_period = tswap64(target_scha->sched_period);
> +            if (size > offsetof(struct target_sched_attr, sched_util_min)) {
> +                scha.sched_util_min = tswap32(target_scha->sched_util_min);
> +                scha.sched_util_max = tswap32(target_scha->sched_util_max);
> +            }
> +            unlock_user(target_scha, arg2, 0);
> +            return get_errno(sys_sched_setattr(arg1, &scha, arg3));
> +        }
>       case TARGET_NR_sched_yield:
>           return get_errno(sched_yield());
>       case TARGET_NR_sched_get_priority_max:
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 0b13975937..310d6ce8ad 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2914,4 +2914,18 @@ struct target_statx {
>      /* 0x100 */
>   };
>   
> +/* from kernel's include/linux/sched/types.h */
> +struct target_sched_attr {
> +    abi_uint size;
> +    abi_uint sched_policy;
> +    abi_ullong sched_flags;
> +    abi_int sched_nice;
> +    abi_uint sched_priority;
> +    abi_ullong sched_runtime;
> +    abi_ullong sched_deadline;
> +    abi_ullong sched_period;
> +    abi_uint sched_util_min;
> +    abi_uint sched_util_max;
> +};
> +
>   #endif

Thanks,
Laurent

Re: [PATCH v3 1/2] linux-user: add sched_getattr support

[Tõnis Tiigi]

On Thu, Dec 23, 2021 at 1:03 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :
>
> Please copy here what you explain in PATCH 0 regarding this patch.
> (do the same for PATCH 1)
>
> > Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
> > ---
> >   linux-user/syscall.c      | 94 +++++++++++++++++++++++++++++++++++++++
> >   linux-user/syscall_defs.h | 14 ++++++
> >   2 files changed, 108 insertions(+)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index f1cfcc8104..2f5a0fac5a 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -339,6 +339,12 @@ _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
> >   #define __NR_sys_sched_setaffinity __NR_sched_setaffinity
> >   _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned int, len,
> >             unsigned long *, user_mask_ptr);
> > +#define __NR_sys_sched_getattr __NR_sched_getattr
> > +_syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
> > +          unsigned int, size, unsigned int, flags);
> > +#define __NR_sys_sched_setattr __NR_sched_setattr
> > +_syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
> > +          unsigned int, flags);
> >   #define __NR_sys_getcpu __NR_getcpu
> >   _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
> >   _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
> > @@ -10593,6 +10599,94 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
> >           }
> >       case TARGET_NR_sched_getscheduler:
> >           return get_errno(sched_getscheduler(arg1));
> > +    case TARGET_NR_sched_getattr:
> > +        {
> > +            struct target_sched_attr *target_scha;
> > +            struct target_sched_attr scha;
>
> In fact, this scha is used with the host syscall, so it must be  sched_attr.


Where do you want me to define the "host variant" of sched_attr and
with what types for the properties? Or do you want additional
typedef(where?) so the name is less confusing? All properties in this
type are fixed length and identical for all architectures.

>
>
> > +            if (arg2 == 0) {
> > +                return -TARGET_EINVAL;
> > +            }
> > +            if (arg3 > sizeof(scha)) {
> > +                arg3 = sizeof(scha);
> > +            }
> > +            ret = get_errno(sys_sched_getattr(arg1, &scha, arg3, arg4));
> > +            if (!is_error(ret)) {
> > +                target_scha = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> > +                if (!target_scha) {
> > +                    return -TARGET_EFAULT;
> > +                }
> > +                target_scha->size = tswap32(scha.size);
> > +                target_scha->sched_policy = tswap32(scha.sched_policy);
> > +                target_scha->sched_flags = tswap64(scha.sched_flags);
> > +                target_scha->sched_nice = tswap32(scha.sched_nice);
> > +                target_scha->sched_priority = tswap32(scha.sched_priority);
> > +                target_scha->sched_runtime = tswap64(scha.sched_runtime);
> > +                target_scha->sched_deadline = tswap64(scha.sched_deadline);
> > +                target_scha->sched_period = tswap64(scha.sched_period);
> > +                if (scha.size > offsetof(struct target_sched_attr, sched_util_min)) {
> > +                    target_scha->sched_util_min = tswap32(scha.sched_util_min);
> > +                    target_scha->sched_util_max = tswap32(scha.sched_util_max);
> > +                }
> > +                unlock_user(target_scha, arg2, arg3);
> > +            }
> > +            return ret;
> > +        }
> > +    case TARGET_NR_sched_setattr:
> > +        {
> > +            struct target_sched_attr *target_scha;
> > +            struct target_sched_attr scha;
>
> scha is sched_attr as it is used with the host syscall.
>
>
> > +            if (arg2 == 0) {
> > +                return -TARGET_EINVAL;
> > +            }
> > +            uint32_t size;
>
> QEMU coding style doesn't allow to mix declarations and statements.
>
> > +            if (get_user_u32(size, arg2)) {
> > +                return -TARGET_EFAULT;
> > +            }
> > +            if (!size) {
> > +                size = offsetof(struct target_sched_attr, sched_util_min);
> > +            }
> > +            if (size < offsetof(struct target_sched_attr, sched_util_min)) {
> > +                if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> > +                    return -TARGET_EFAULT;
> > +                }
> > +                return -TARGET_E2BIG;
> > +            }
> > +
> > +            if (size > sizeof(scha)) {
> > +                for (int i = sizeof(scha); i < size; i++) {
> > +                    uint8_t b;
> > +                    if (get_user_u8(b, arg2 + i)) {
> > +                        return -TARGET_EFAULT;
> > +                    }
> > +                    if (b != 0) {
> > +                        if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> > +                            return -TARGET_EFAULT;
> > +                        }
> > +                        return -TARGET_E2BIG;
> > +                    }
> > +                }
> > +                size = sizeof(scha);
> > +            }
>
> I guess this is the code to mimic kernel copy_struct_from_user(), the part when usize > ksize.
>
> It's a little bit ugly, but I can't disagree because the kernel does the same.
>
> except that the kernel check for unsigned rather than for 8bit. Could you change that?


You mean "unsigned long" like in
https://github.com/torvalds/linux/blob/76657eaef4a759e695eb1883d4f1d9af1e4ff9a8/lib/usercopy.c#L57
? That would mean that this code needs to be much more complicated to
handle the cases for the unaligned start and end bytes, need
aligned_byte_mask helper etc. Even though kernel seems to have extra
code for these cases iiuc it can still get EFAULT on specific
conditions.

>
>
> The best would be to define check_zeroed_user() in Qemu and use it here.
>
> > +
> > +            target_scha = lock_user(VERIFY_READ, arg2, size, 1);
> > +            if (!target_scha) {
> > +                return -TARGET_EFAULT;
> > +            }
> > +            scha.size = size;
> > +            scha.sched_policy = tswap32(target_scha->sched_policy);
> > +            scha.sched_flags = tswap64(target_scha->sched_flags);
> > +            scha.sched_nice = tswap32(target_scha->sched_nice);
> > +            scha.sched_priority = tswap32(target_scha->sched_priority);
> > +            scha.sched_runtime = tswap64(target_scha->sched_runtime);
> > +            scha.sched_deadline = tswap64(target_scha->sched_deadline);
> > +            scha.sched_period = tswap64(target_scha->sched_period);
> > +            if (size > offsetof(struct target_sched_attr, sched_util_min)) {
> > +                scha.sched_util_min = tswap32(target_scha->sched_util_min);
> > +                scha.sched_util_max = tswap32(target_scha->sched_util_max);
> > +            }
> > +            unlock_user(target_scha, arg2, 0);
> > +            return get_errno(sys_sched_setattr(arg1, &scha, arg3));
> > +        }
> >       case TARGET_NR_sched_yield:
> >           return get_errno(sched_yield());
> >       case TARGET_NR_sched_get_priority_max:
> > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> > index 0b13975937..310d6ce8ad 100644
> > --- a/linux-user/syscall_defs.h
> > +++ b/linux-user/syscall_defs.h
> > @@ -2914,4 +2914,18 @@ struct target_statx {
> >      /* 0x100 */
> >   };
> >
> > +/* from kernel's include/linux/sched/types.h */
> > +struct target_sched_attr {
> > +    abi_uint size;
> > +    abi_uint sched_policy;
> > +    abi_ullong sched_flags;
> > +    abi_int sched_nice;
> > +    abi_uint sched_priority;
> > +    abi_ullong sched_runtime;
> > +    abi_ullong sched_deadline;
> > +    abi_ullong sched_period;
> > +    abi_uint sched_util_min;
> > +    abi_uint sched_util_max;
> > +};
> > +
> >   #endif
>
> Thanks,
> Laurent

Re: [PATCH v3 1/2] linux-user: add sched_getattr support

[Tõnis Tiigi]

Ping Laurent. Any suggestions for the follow-up questions?

On Thu, Dec 23, 2021 at 3:00 PM Tõnis Tiigi <tonistiigi@gmail.com> wrote:
>
> On Thu, Dec 23, 2021 at 1:03 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >
> > Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :
> >
> > Please copy here what you explain in PATCH 0 regarding this patch.
> > (do the same for PATCH 1)
> >
> > > Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
> > > ---
> > >   linux-user/syscall.c      | 94 +++++++++++++++++++++++++++++++++++++++
> > >   linux-user/syscall_defs.h | 14 ++++++
> > >   2 files changed, 108 insertions(+)
> > >
> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > > index f1cfcc8104..2f5a0fac5a 100644
> > > --- a/linux-user/syscall.c
> > > +++ b/linux-user/syscall.c
> > > @@ -339,6 +339,12 @@ _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
> > >   #define __NR_sys_sched_setaffinity __NR_sched_setaffinity
> > >   _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned int, len,
> > >             unsigned long *, user_mask_ptr);
> > > +#define __NR_sys_sched_getattr __NR_sched_getattr
> > > +_syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
> > > +          unsigned int, size, unsigned int, flags);
> > > +#define __NR_sys_sched_setattr __NR_sched_setattr
> > > +_syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
> > > +          unsigned int, flags);
> > >   #define __NR_sys_getcpu __NR_getcpu
> > >   _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
> > >   _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
> > > @@ -10593,6 +10599,94 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
> > >           }
> > >       case TARGET_NR_sched_getscheduler:
> > >           return get_errno(sched_getscheduler(arg1));
> > > +    case TARGET_NR_sched_getattr:
> > > +        {
> > > +            struct target_sched_attr *target_scha;
> > > +            struct target_sched_attr scha;
> >
> > In fact, this scha is used with the host syscall, so it must be  sched_attr.
>
>
> Where do you want me to define the "host variant" of sched_attr and
> with what types for the properties? Or do you want additional
> typedef(where?) so the name is less confusing? All properties in this
> type are fixed length and identical for all architectures.
>
> >
> >
> > > +            if (arg2 == 0) {
> > > +                return -TARGET_EINVAL;
> > > +            }
> > > +            if (arg3 > sizeof(scha)) {
> > > +                arg3 = sizeof(scha);
> > > +            }
> > > +            ret = get_errno(sys_sched_getattr(arg1, &scha, arg3, arg4));
> > > +            if (!is_error(ret)) {
> > > +                target_scha = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> > > +                if (!target_scha) {
> > > +                    return -TARGET_EFAULT;
> > > +                }
> > > +                target_scha->size = tswap32(scha.size);
> > > +                target_scha->sched_policy = tswap32(scha.sched_policy);
> > > +                target_scha->sched_flags = tswap64(scha.sched_flags);
> > > +                target_scha->sched_nice = tswap32(scha.sched_nice);
> > > +                target_scha->sched_priority = tswap32(scha.sched_priority);
> > > +                target_scha->sched_runtime = tswap64(scha.sched_runtime);
> > > +                target_scha->sched_deadline = tswap64(scha.sched_deadline);
> > > +                target_scha->sched_period = tswap64(scha.sched_period);
> > > +                if (scha.size > offsetof(struct target_sched_attr, sched_util_min)) {
> > > +                    target_scha->sched_util_min = tswap32(scha.sched_util_min);
> > > +                    target_scha->sched_util_max = tswap32(scha.sched_util_max);
> > > +                }
> > > +                unlock_user(target_scha, arg2, arg3);
> > > +            }
> > > +            return ret;
> > > +        }
> > > +    case TARGET_NR_sched_setattr:
> > > +        {
> > > +            struct target_sched_attr *target_scha;
> > > +            struct target_sched_attr scha;
> >
> > scha is sched_attr as it is used with the host syscall.
> >
> >
> > > +            if (arg2 == 0) {
> > > +                return -TARGET_EINVAL;
> > > +            }
> > > +            uint32_t size;
> >
> > QEMU coding style doesn't allow to mix declarations and statements.
> >
> > > +            if (get_user_u32(size, arg2)) {
> > > +                return -TARGET_EFAULT;
> > > +            }
> > > +            if (!size) {
> > > +                size = offsetof(struct target_sched_attr, sched_util_min);
> > > +            }
> > > +            if (size < offsetof(struct target_sched_attr, sched_util_min)) {
> > > +                if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> > > +                    return -TARGET_EFAULT;
> > > +                }
> > > +                return -TARGET_E2BIG;
> > > +            }
> > > +
> > > +            if (size > sizeof(scha)) {
> > > +                for (int i = sizeof(scha); i < size; i++) {
> > > +                    uint8_t b;
> > > +                    if (get_user_u8(b, arg2 + i)) {
> > > +                        return -TARGET_EFAULT;
> > > +                    }
> > > +                    if (b != 0) {
> > > +                        if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> > > +                            return -TARGET_EFAULT;
> > > +                        }
> > > +                        return -TARGET_E2BIG;
> > > +                    }
> > > +                }
> > > +                size = sizeof(scha);
> > > +            }
> >
> > I guess this is the code to mimic kernel copy_struct_from_user(), the part when usize > ksize.
> >
> > It's a little bit ugly, but I can't disagree because the kernel does the same.
> >
> > except that the kernel check for unsigned rather than for 8bit. Could you change that?
>
>
> You mean "unsigned long" like in
> https://github.com/torvalds/linux/blob/76657eaef4a759e695eb1883d4f1d9af1e4ff9a8/lib/usercopy.c#L57
> ? That would mean that this code needs to be much more complicated to
> handle the cases for the unaligned start and end bytes, need
> aligned_byte_mask helper etc. Even though kernel seems to have extra
> code for these cases iiuc it can still get EFAULT on specific
> conditions.
>
> >
> >
> > The best would be to define check_zeroed_user() in Qemu and use it here.
> >
> > > +
> > > +            target_scha = lock_user(VERIFY_READ, arg2, size, 1);
> > > +            if (!target_scha) {
> > > +                return -TARGET_EFAULT;
> > > +            }
> > > +            scha.size = size;
> > > +            scha.sched_policy = tswap32(target_scha->sched_policy);
> > > +            scha.sched_flags = tswap64(target_scha->sched_flags);
> > > +            scha.sched_nice = tswap32(target_scha->sched_nice);
> > > +            scha.sched_priority = tswap32(target_scha->sched_priority);
> > > +            scha.sched_runtime = tswap64(target_scha->sched_runtime);
> > > +            scha.sched_deadline = tswap64(target_scha->sched_deadline);
> > > +            scha.sched_period = tswap64(target_scha->sched_period);
> > > +            if (size > offsetof(struct target_sched_attr, sched_util_min)) {
> > > +                scha.sched_util_min = tswap32(target_scha->sched_util_min);
> > > +                scha.sched_util_max = tswap32(target_scha->sched_util_max);
> > > +            }
> > > +            unlock_user(target_scha, arg2, 0);
> > > +            return get_errno(sys_sched_setattr(arg1, &scha, arg3));
> > > +        }
> > >       case TARGET_NR_sched_yield:
> > >           return get_errno(sched_yield());
> > >       case TARGET_NR_sched_get_priority_max:
> > > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> > > index 0b13975937..310d6ce8ad 100644
> > > --- a/linux-user/syscall_defs.h
> > > +++ b/linux-user/syscall_defs.h
> > > @@ -2914,4 +2914,18 @@ struct target_statx {
> > >      /* 0x100 */
> > >   };
> > >
> > > +/* from kernel's include/linux/sched/types.h */
> > > +struct target_sched_attr {
> > > +    abi_uint size;
> > > +    abi_uint sched_policy;
> > > +    abi_ullong sched_flags;
> > > +    abi_int sched_nice;
> > > +    abi_uint sched_priority;
> > > +    abi_ullong sched_runtime;
> > > +    abi_ullong sched_deadline;
> > > +    abi_ullong sched_period;
> > > +    abi_uint sched_util_min;
> > > +    abi_uint sched_util_max;
> > > +};
> > > +
> > >   #endif
> >
> > Thanks,
> > Laurent

Re: [PATCH v3 1/2] linux-user: add sched_getattr support

[Laurent Vivier]

Le 03/01/2022 à 18:07, Tõnis Tiigi a écrit :
> Ping Laurent. Any suggestions for the follow-up questions?
> 
> On Thu, Dec 23, 2021 at 3:00 PM Tõnis Tiigi <tonistiigi@gmail.com> wrote:
>>
>> On Thu, Dec 23, 2021 at 1:03 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>
>>> Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :
>>>
>>> Please copy here what you explain in PATCH 0 regarding this patch.
>>> (do the same for PATCH 1)
>>>
>>>> Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
>>>> ---
>>>>    linux-user/syscall.c      | 94 +++++++++++++++++++++++++++++++++++++++
>>>>    linux-user/syscall_defs.h | 14 ++++++
>>>>    2 files changed, 108 insertions(+)
>>>>
>>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>>> index f1cfcc8104..2f5a0fac5a 100644
>>>> --- a/linux-user/syscall.c
>>>> +++ b/linux-user/syscall.c
>>>> @@ -339,6 +339,12 @@ _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
>>>>    #define __NR_sys_sched_setaffinity __NR_sched_setaffinity
>>>>    _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned int, len,
>>>>              unsigned long *, user_mask_ptr);
>>>> +#define __NR_sys_sched_getattr __NR_sched_getattr
>>>> +_syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
>>>> +          unsigned int, size, unsigned int, flags);
>>>> +#define __NR_sys_sched_setattr __NR_sched_setattr
>>>> +_syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
>>>> +          unsigned int, flags);
>>>>    #define __NR_sys_getcpu __NR_getcpu
>>>>    _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
>>>>    _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
>>>> @@ -10593,6 +10599,94 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>>>>            }
>>>>        case TARGET_NR_sched_getscheduler:
>>>>            return get_errno(sched_getscheduler(arg1));
>>>> +    case TARGET_NR_sched_getattr:
>>>> +        {
>>>> +            struct target_sched_attr *target_scha;
>>>> +            struct target_sched_attr scha;
>>>
>>> In fact, this scha is used with the host syscall, so it must be  sched_attr.
>>
>>
>> Where do you want me to define the "host variant" of sched_attr and
>> with what types for the properties? Or do you want additional
>> typedef(where?) so the name is less confusing? All properties in this
>> type are fixed length and identical for all architectures.

It's better to use the host variant with the host syscall.

Normally sched_attr comes with kernel headers, so it should be available and you should not have to 
define it.

We need to define a target property because alignment also matters as the alignment for type can 
differ from an architecture to another. I agree that in most cases it should not be needed but I 
think it's cleaner like that.

so for this part, only replace "struct target_sched_attr scha;" by "struct sched_att scha;"

>>
>>>
>>>
>>>> +            if (arg2 == 0) {
>>>> +                return -TARGET_EINVAL;
>>>> +            }
>>>> +            if (arg3 > sizeof(scha)) {
>>>> +                arg3 = sizeof(scha);
>>>> +            }
>>>> +            ret = get_errno(sys_sched_getattr(arg1, &scha, arg3, arg4));
>>>> +            if (!is_error(ret)) {
>>>> +                target_scha = lock_user(VERIFY_WRITE, arg2, arg3, 0);
>>>> +                if (!target_scha) {
>>>> +                    return -TARGET_EFAULT;
>>>> +                }
>>>> +                target_scha->size = tswap32(scha.size);
>>>> +                target_scha->sched_policy = tswap32(scha.sched_policy);
>>>> +                target_scha->sched_flags = tswap64(scha.sched_flags);
>>>> +                target_scha->sched_nice = tswap32(scha.sched_nice);
>>>> +                target_scha->sched_priority = tswap32(scha.sched_priority);
>>>> +                target_scha->sched_runtime = tswap64(scha.sched_runtime);
>>>> +                target_scha->sched_deadline = tswap64(scha.sched_deadline);
>>>> +                target_scha->sched_period = tswap64(scha.sched_period);
>>>> +                if (scha.size > offsetof(struct target_sched_attr, sched_util_min)) {
>>>> +                    target_scha->sched_util_min = tswap32(scha.sched_util_min);
>>>> +                    target_scha->sched_util_max = tswap32(scha.sched_util_max);
>>>> +                }
>>>> +                unlock_user(target_scha, arg2, arg3);
>>>> +            }
>>>> +            return ret;
>>>> +        }
>>>> +    case TARGET_NR_sched_setattr:
>>>> +        {
>>>> +            struct target_sched_attr *target_scha;
>>>> +            struct target_sched_attr scha;
>>>
>>> scha is sched_attr as it is used with the host syscall.
>>>
>>>
>>>> +            if (arg2 == 0) {
>>>> +                return -TARGET_EINVAL;
>>>> +            }
>>>> +            uint32_t size;
>>>
>>> QEMU coding style doesn't allow to mix declarations and statements.
>>>
>>>> +            if (get_user_u32(size, arg2)) {
>>>> +                return -TARGET_EFAULT;
>>>> +            }
>>>> +            if (!size) {
>>>> +                size = offsetof(struct target_sched_attr, sched_util_min);
>>>> +            }
>>>> +            if (size < offsetof(struct target_sched_attr, sched_util_min)) {
>>>> +                if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
>>>> +                    return -TARGET_EFAULT;
>>>> +                }
>>>> +                return -TARGET_E2BIG;
>>>> +            }
>>>> +
>>>> +            if (size > sizeof(scha)) {
>>>> +                for (int i = sizeof(scha); i < size; i++) {
>>>> +                    uint8_t b;
>>>> +                    if (get_user_u8(b, arg2 + i)) {
>>>> +                        return -TARGET_EFAULT;
>>>> +                    }
>>>> +                    if (b != 0) {
>>>> +                        if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
>>>> +                            return -TARGET_EFAULT;
>>>> +                        }
>>>> +                        return -TARGET_E2BIG;
>>>> +                    }
>>>> +                }
>>>> +                size = sizeof(scha);
>>>> +            }
>>>
>>> I guess this is the code to mimic kernel copy_struct_from_user(), the part when usize > ksize.
>>>
>>> It's a little bit ugly, but I can't disagree because the kernel does the same.
>>>
>>> except that the kernel check for unsigned rather than for 8bit. Could you change that?
>>
>>
>> You mean "unsigned long" like in
>> https://github.com/torvalds/linux/blob/76657eaef4a759e695eb1883d4f1d9af1e4ff9a8/lib/usercopy.c#L57

yes

>> ? That would mean that this code needs to be much more complicated to
>> handle the cases for the unaligned start and end bytes, need
>> aligned_byte_mask helper etc. Even though kernel seems to have extra
>> code for these cases iiuc it can still get EFAULT on specific
>> conditions.

OK, I don't want too complicated code here, so I think we can keep your version.
But could you move this code to a function?

>>>
>>>
>>> The best would be to define check_zeroed_user() in Qemu and use it here.
>>>
>>>> +
>>>> +            target_scha = lock_user(VERIFY_READ, arg2, size, 1);
>>>> +            if (!target_scha) {
>>>> +                return -TARGET_EFAULT;
>>>> +            }
>>>> +            scha.size = size;
>>>> +            scha.sched_policy = tswap32(target_scha->sched_policy);
>>>> +            scha.sched_flags = tswap64(target_scha->sched_flags);
>>>> +            scha.sched_nice = tswap32(target_scha->sched_nice);
>>>> +            scha.sched_priority = tswap32(target_scha->sched_priority);
>>>> +            scha.sched_runtime = tswap64(target_scha->sched_runtime);
>>>> +            scha.sched_deadline = tswap64(target_scha->sched_deadline);
>>>> +            scha.sched_period = tswap64(target_scha->sched_period);
>>>> +            if (size > offsetof(struct target_sched_attr, sched_util_min)) {
>>>> +                scha.sched_util_min = tswap32(target_scha->sched_util_min);
>>>> +                scha.sched_util_max = tswap32(target_scha->sched_util_max);
>>>> +            }
>>>> +            unlock_user(target_scha, arg2, 0);
>>>> +            return get_errno(sys_sched_setattr(arg1, &scha, arg3));
>>>> +        }
>>>>        case TARGET_NR_sched_yield:
>>>>            return get_errno(sched_yield());
>>>>        case TARGET_NR_sched_get_priority_max:
>>>> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
>>>> index 0b13975937..310d6ce8ad 100644
>>>> --- a/linux-user/syscall_defs.h
>>>> +++ b/linux-user/syscall_defs.h
>>>> @@ -2914,4 +2914,18 @@ struct target_statx {
>>>>       /* 0x100 */
>>>>    };
>>>>
>>>> +/* from kernel's include/linux/sched/types.h */
>>>> +struct target_sched_attr {
>>>> +    abi_uint size;
>>>> +    abi_uint sched_policy;
>>>> +    abi_ullong sched_flags;
>>>> +    abi_int sched_nice;
>>>> +    abi_uint sched_priority;
>>>> +    abi_ullong sched_runtime;
>>>> +    abi_ullong sched_deadline;
>>>> +    abi_ullong sched_period;
>>>> +    abi_uint sched_util_min;
>>>> +    abi_uint sched_util_max;
>>>> +};
>>>> +
>>>>    #endif
>>>
>>> Thanks,
>>> Laurent

Re: [PATCH v3 1/2] linux-user: add sched_getattr support

[Tõnis Tiigi]

On Mon, Jan 3, 2022 at 10:37 AM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 03/01/2022 à 18:07, Tõnis Tiigi a écrit :
> > Ping Laurent. Any suggestions for the follow-up questions?
> >
> > On Thu, Dec 23, 2021 at 3:00 PM Tõnis Tiigi <tonistiigi@gmail.com> wrote:
> >>
> >> On Thu, Dec 23, 2021 at 1:03 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>>
> >>> Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :
> >>>
> >>> Please copy here what you explain in PATCH 0 regarding this patch.
> >>> (do the same for PATCH 1)
> >>>
> >>>> Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
> >>>> ---
> >>>>    linux-user/syscall.c      | 94 +++++++++++++++++++++++++++++++++++++++
> >>>>    linux-user/syscall_defs.h | 14 ++++++
> >>>>    2 files changed, 108 insertions(+)
> >>>>
> >>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> >>>> index f1cfcc8104..2f5a0fac5a 100644
> >>>> --- a/linux-user/syscall.c
> >>>> +++ b/linux-user/syscall.c
> >>>> @@ -339,6 +339,12 @@ _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
> >>>>    #define __NR_sys_sched_setaffinity __NR_sched_setaffinity
> >>>>    _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned int, len,
> >>>>              unsigned long *, user_mask_ptr);
> >>>> +#define __NR_sys_sched_getattr __NR_sched_getattr
> >>>> +_syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
> >>>> +          unsigned int, size, unsigned int, flags);
> >>>> +#define __NR_sys_sched_setattr __NR_sched_setattr
> >>>> +_syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
> >>>> +          unsigned int, flags);
> >>>>    #define __NR_sys_getcpu __NR_getcpu
> >>>>    _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
> >>>>    _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
> >>>> @@ -10593,6 +10599,94 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
> >>>>            }
> >>>>        case TARGET_NR_sched_getscheduler:
> >>>>            return get_errno(sched_getscheduler(arg1));
> >>>> +    case TARGET_NR_sched_getattr:
> >>>> +        {
> >>>> +            struct target_sched_attr *target_scha;
> >>>> +            struct target_sched_attr scha;
> >>>
> >>> In fact, this scha is used with the host syscall, so it must be  sched_attr.
> >>
> >>
> >> Where do you want me to define the "host variant" of sched_attr and
> >> with what types for the properties? Or do you want additional
> >> typedef(where?) so the name is less confusing? All properties in this
> >> type are fixed length and identical for all architectures.
>
> It's better to use the host variant with the host syscall.
>
> Normally sched_attr comes with kernel headers, so it should be available and you should not have to
> define it.
>
> We need to define a target property because alignment also matters as the alignment for type can
> differ from an architecture to another. I agree that in most cases it should not be needed but I
> think it's cleaner like that.
>
> so for this part, only replace "struct target_sched_attr scha;" by "struct sched_att scha;"

sched_attr is defined in linux/sched/types.h . I can't include it
directly as it conflicts with libc headers with "redefinition of
'struct sched_param'". It looks like
https://lkml.org/lkml/2020/5/28/810 attempted to resolve this conflict
but was not merged and seems to be stuck in kernel vs glibc blame
cycle.

>
> >>
> >>>
> >>>
> >>>> +            if (arg2 == 0) {
> >>>> +                return -TARGET_EINVAL;
> >>>> +            }
> >>>> +            if (arg3 > sizeof(scha)) {
> >>>> +                arg3 = sizeof(scha);
> >>>> +            }
> >>>> +            ret = get_errno(sys_sched_getattr(arg1, &scha, arg3, arg4));
> >>>> +            if (!is_error(ret)) {
> >>>> +                target_scha = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> >>>> +                if (!target_scha) {
> >>>> +                    return -TARGET_EFAULT;
> >>>> +                }
> >>>> +                target_scha->size = tswap32(scha.size);
> >>>> +                target_scha->sched_policy = tswap32(scha.sched_policy);
> >>>> +                target_scha->sched_flags = tswap64(scha.sched_flags);
> >>>> +                target_scha->sched_nice = tswap32(scha.sched_nice);
> >>>> +                target_scha->sched_priority = tswap32(scha.sched_priority);
> >>>> +                target_scha->sched_runtime = tswap64(scha.sched_runtime);
> >>>> +                target_scha->sched_deadline = tswap64(scha.sched_deadline);
> >>>> +                target_scha->sched_period = tswap64(scha.sched_period);
> >>>> +                if (scha.size > offsetof(struct target_sched_attr, sched_util_min)) {
> >>>> +                    target_scha->sched_util_min = tswap32(scha.sched_util_min);
> >>>> +                    target_scha->sched_util_max = tswap32(scha.sched_util_max);
> >>>> +                }
> >>>> +                unlock_user(target_scha, arg2, arg3);
> >>>> +            }
> >>>> +            return ret;
> >>>> +        }
> >>>> +    case TARGET_NR_sched_setattr:
> >>>> +        {
> >>>> +            struct target_sched_attr *target_scha;
> >>>> +            struct target_sched_attr scha;
> >>>
> >>> scha is sched_attr as it is used with the host syscall.
> >>>
> >>>
> >>>> +            if (arg2 == 0) {
> >>>> +                return -TARGET_EINVAL;
> >>>> +            }
> >>>> +            uint32_t size;
> >>>
> >>> QEMU coding style doesn't allow to mix declarations and statements.
> >>>
> >>>> +            if (get_user_u32(size, arg2)) {
> >>>> +                return -TARGET_EFAULT;
> >>>> +            }
> >>>> +            if (!size) {
> >>>> +                size = offsetof(struct target_sched_attr, sched_util_min);
> >>>> +            }
> >>>> +            if (size < offsetof(struct target_sched_attr, sched_util_min)) {
> >>>> +                if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> >>>> +                    return -TARGET_EFAULT;
> >>>> +                }
> >>>> +                return -TARGET_E2BIG;
> >>>> +            }
> >>>> +
> >>>> +            if (size > sizeof(scha)) {
> >>>> +                for (int i = sizeof(scha); i < size; i++) {
> >>>> +                    uint8_t b;
> >>>> +                    if (get_user_u8(b, arg2 + i)) {
> >>>> +                        return -TARGET_EFAULT;
> >>>> +                    }
> >>>> +                    if (b != 0) {
> >>>> +                        if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
> >>>> +                            return -TARGET_EFAULT;
> >>>> +                        }
> >>>> +                        return -TARGET_E2BIG;
> >>>> +                    }
> >>>> +                }
> >>>> +                size = sizeof(scha);
> >>>> +            }
> >>>
> >>> I guess this is the code to mimic kernel copy_struct_from_user(), the part when usize > ksize.
> >>>
> >>> It's a little bit ugly, but I can't disagree because the kernel does the same.
> >>>
> >>> except that the kernel check for unsigned rather than for 8bit. Could you change that?
> >>
> >>
> >> You mean "unsigned long" like in
> >> https://github.com/torvalds/linux/blob/76657eaef4a759e695eb1883d4f1d9af1e4ff9a8/lib/usercopy.c#L57
>
> yes
>
> >> ? That would mean that this code needs to be much more complicated to
> >> handle the cases for the unaligned start and end bytes, need
> >> aligned_byte_mask helper etc. Even though kernel seems to have extra
> >> code for these cases iiuc it can still get EFAULT on specific
> >> conditions.
>
> OK, I don't want too complicated code here, so I think we can keep your version.
> But could you move this code to a function?

Sure, but could you tell me where do you want it defined. syscall.c
does not seem to have generic helper functions and current helpers
seem to be macros in qemu.h . Also, are we talking about a function
like check_zeroed_user() or a variant of lock_user() with two size
parameters(or lock_user_struct() with extra size param)?

>
> >>>
> >>>
> >>> The best would be to define check_zeroed_user() in Qemu and use it here.
> >>>
> >>>> +
> >>>> +            target_scha = lock_user(VERIFY_READ, arg2, size, 1);
> >>>> +            if (!target_scha) {
> >>>> +                return -TARGET_EFAULT;
> >>>> +            }
> >>>> +            scha.size = size;
> >>>> +            scha.sched_policy = tswap32(target_scha->sched_policy);
> >>>> +            scha.sched_flags = tswap64(target_scha->sched_flags);
> >>>> +            scha.sched_nice = tswap32(target_scha->sched_nice);
> >>>> +            scha.sched_priority = tswap32(target_scha->sched_priority);
> >>>> +            scha.sched_runtime = tswap64(target_scha->sched_runtime);
> >>>> +            scha.sched_deadline = tswap64(target_scha->sched_deadline);
> >>>> +            scha.sched_period = tswap64(target_scha->sched_period);
> >>>> +            if (size > offsetof(struct target_sched_attr, sched_util_min)) {
> >>>> +                scha.sched_util_min = tswap32(target_scha->sched_util_min);
> >>>> +                scha.sched_util_max = tswap32(target_scha->sched_util_max);
> >>>> +            }
> >>>> +            unlock_user(target_scha, arg2, 0);
> >>>> +            return get_errno(sys_sched_setattr(arg1, &scha, arg3));
> >>>> +        }
> >>>>        case TARGET_NR_sched_yield:
> >>>>            return get_errno(sched_yield());
> >>>>        case TARGET_NR_sched_get_priority_max:
> >>>> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> >>>> index 0b13975937..310d6ce8ad 100644
> >>>> --- a/linux-user/syscall_defs.h
> >>>> +++ b/linux-user/syscall_defs.h
> >>>> @@ -2914,4 +2914,18 @@ struct target_statx {
> >>>>       /* 0x100 */
> >>>>    };
> >>>>
> >>>> +/* from kernel's include/linux/sched/types.h */
> >>>> +struct target_sched_attr {
> >>>> +    abi_uint size;
> >>>> +    abi_uint sched_policy;
> >>>> +    abi_ullong sched_flags;
> >>>> +    abi_int sched_nice;
> >>>> +    abi_uint sched_priority;
> >>>> +    abi_ullong sched_runtime;
> >>>> +    abi_ullong sched_deadline;
> >>>> +    abi_ullong sched_period;
> >>>> +    abi_uint sched_util_min;
> >>>> +    abi_uint sched_util_max;
> >>>> +};
> >>>> +
> >>>>    #endif
> >>>
> >>> Thanks,
> >>> Laurent
>

Re: [PATCH v3 1/2] linux-user: add sched_getattr support

[Laurent Vivier]

Le 03/01/2022 à 20:31, Tõnis Tiigi a écrit :
> On Mon, Jan 3, 2022 at 10:37 AM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 03/01/2022 à 18:07, Tõnis Tiigi a écrit :
>>> Ping Laurent. Any suggestions for the follow-up questions?
>>>
>>> On Thu, Dec 23, 2021 at 3:00 PM Tõnis Tiigi <tonistiigi@gmail.com> wrote:
>>>>
>>>> On Thu, Dec 23, 2021 at 1:03 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>>>
>>>>> Le 23/12/2021 à 07:47, Tonis Tiigi a écrit :
>>>>>
>>>>> Please copy here what you explain in PATCH 0 regarding this patch.
>>>>> (do the same for PATCH 1)
>>>>>
>>>>>> Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
>>>>>> ---
>>>>>>     linux-user/syscall.c      | 94 +++++++++++++++++++++++++++++++++++++++
>>>>>>     linux-user/syscall_defs.h | 14 ++++++
>>>>>>     2 files changed, 108 insertions(+)
>>>>>>
>>>>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>>>>> index f1cfcc8104..2f5a0fac5a 100644
>>>>>> --- a/linux-user/syscall.c
>>>>>> +++ b/linux-user/syscall.c
>>>>>> @@ -339,6 +339,12 @@ _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
>>>>>>     #define __NR_sys_sched_setaffinity __NR_sched_setaffinity
>>>>>>     _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned int, len,
>>>>>>               unsigned long *, user_mask_ptr);
>>>>>> +#define __NR_sys_sched_getattr __NR_sched_getattr
>>>>>> +_syscall4(int, sys_sched_getattr, pid_t, pid, struct target_sched_attr *, attr,
>>>>>> +          unsigned int, size, unsigned int, flags);
>>>>>> +#define __NR_sys_sched_setattr __NR_sched_setattr
>>>>>> +_syscall3(int, sys_sched_setattr, pid_t, pid, struct target_sched_attr *, attr,
>>>>>> +          unsigned int, flags);
>>>>>>     #define __NR_sys_getcpu __NR_getcpu
>>>>>>     _syscall3(int, sys_getcpu, unsigned *, cpu, unsigned *, node, void *, tcache);
>>>>>>     _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
>>>>>> @@ -10593,6 +10599,94 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>>>>>>             }
>>>>>>         case TARGET_NR_sched_getscheduler:
>>>>>>             return get_errno(sched_getscheduler(arg1));
>>>>>> +    case TARGET_NR_sched_getattr:
>>>>>> +        {
>>>>>> +            struct target_sched_attr *target_scha;
>>>>>> +            struct target_sched_attr scha;
>>>>>
>>>>> In fact, this scha is used with the host syscall, so it must be  sched_attr.
>>>>
>>>>
>>>> Where do you want me to define the "host variant" of sched_attr and
>>>> with what types for the properties? Or do you want additional
>>>> typedef(where?) so the name is less confusing? All properties in this
>>>> type are fixed length and identical for all architectures.
>>
>> It's better to use the host variant with the host syscall.
>>
>> Normally sched_attr comes with kernel headers, so it should be available and you should not have to
>> define it.
>>
>> We need to define a target property because alignment also matters as the alignment for type can
>> differ from an architecture to another. I agree that in most cases it should not be needed but I
>> think it's cleaner like that.
>>
>> so for this part, only replace "struct target_sched_attr scha;" by "struct sched_att scha;"
> 
> sched_attr is defined in linux/sched/types.h . I can't include it
> directly as it conflicts with libc headers with "redefinition of
> 'struct sched_param'". It looks like
> https://lkml.org/lkml/2020/5/28/810 attempted to resolve this conflict
> but was not merged and seems to be stuck in kernel vs glibc blame
> cycle.
>

So the best to do is to define a struct host_sched_attr (a strict copy of the kernel one) and use it 
with sched_getattr().

See for instance prlimit64 implementation:

163a05a8398b ("linux-user: Implement prlimit64 syscall")

it's a bit old, but it shows the main idea of a host structure for the host side, a target structure 
for the target side (we have introduced the abi type since).

>>
>>>>
>>>>>
>>>>>
>>>>>> +            if (arg2 == 0) {
>>>>>> +                return -TARGET_EINVAL;
>>>>>> +            }
>>>>>> +            if (arg3 > sizeof(scha)) {
>>>>>> +                arg3 = sizeof(scha);
>>>>>> +            }
>>>>>> +            ret = get_errno(sys_sched_getattr(arg1, &scha, arg3, arg4));
>>>>>> +            if (!is_error(ret)) {
>>>>>> +                target_scha = lock_user(VERIFY_WRITE, arg2, arg3, 0);
>>>>>> +                if (!target_scha) {
>>>>>> +                    return -TARGET_EFAULT;
>>>>>> +                }
>>>>>> +                target_scha->size = tswap32(scha.size);
>>>>>> +                target_scha->sched_policy = tswap32(scha.sched_policy);
>>>>>> +                target_scha->sched_flags = tswap64(scha.sched_flags);
>>>>>> +                target_scha->sched_nice = tswap32(scha.sched_nice);
>>>>>> +                target_scha->sched_priority = tswap32(scha.sched_priority);
>>>>>> +                target_scha->sched_runtime = tswap64(scha.sched_runtime);
>>>>>> +                target_scha->sched_deadline = tswap64(scha.sched_deadline);
>>>>>> +                target_scha->sched_period = tswap64(scha.sched_period);
>>>>>> +                if (scha.size > offsetof(struct target_sched_attr, sched_util_min)) {
>>>>>> +                    target_scha->sched_util_min = tswap32(scha.sched_util_min);
>>>>>> +                    target_scha->sched_util_max = tswap32(scha.sched_util_max);
>>>>>> +                }
>>>>>> +                unlock_user(target_scha, arg2, arg3);
>>>>>> +            }
>>>>>> +            return ret;
>>>>>> +        }
>>>>>> +    case TARGET_NR_sched_setattr:
>>>>>> +        {
>>>>>> +            struct target_sched_attr *target_scha;
>>>>>> +            struct target_sched_attr scha;
>>>>>
>>>>> scha is sched_attr as it is used with the host syscall.
>>>>>
>>>>>
>>>>>> +            if (arg2 == 0) {
>>>>>> +                return -TARGET_EINVAL;
>>>>>> +            }
>>>>>> +            uint32_t size;
>>>>>
>>>>> QEMU coding style doesn't allow to mix declarations and statements.
>>>>>
>>>>>> +            if (get_user_u32(size, arg2)) {
>>>>>> +                return -TARGET_EFAULT;
>>>>>> +            }
>>>>>> +            if (!size) {
>>>>>> +                size = offsetof(struct target_sched_attr, sched_util_min);
>>>>>> +            }
>>>>>> +            if (size < offsetof(struct target_sched_attr, sched_util_min)) {
>>>>>> +                if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
>>>>>> +                    return -TARGET_EFAULT;
>>>>>> +                }
>>>>>> +                return -TARGET_E2BIG;
>>>>>> +            }
>>>>>> +
>>>>>> +            if (size > sizeof(scha)) {
>>>>>> +                for (int i = sizeof(scha); i < size; i++) {
>>>>>> +                    uint8_t b;
>>>>>> +                    if (get_user_u8(b, arg2 + i)) {
>>>>>> +                        return -TARGET_EFAULT;
>>>>>> +                    }
>>>>>> +                    if (b != 0) {
>>>>>> +                        if (put_user_u32(sizeof(struct target_sched_attr), arg2)) {
>>>>>> +                            return -TARGET_EFAULT;
>>>>>> +                        }
>>>>>> +                        return -TARGET_E2BIG;
>>>>>> +                    }
>>>>>> +                }
>>>>>> +                size = sizeof(scha);
>>>>>> +            }
>>>>>
>>>>> I guess this is the code to mimic kernel copy_struct_from_user(), the part when usize > ksize.
>>>>>
>>>>> It's a little bit ugly, but I can't disagree because the kernel does the same.
>>>>>
>>>>> except that the kernel check for unsigned rather than for 8bit. Could you change that?
>>>>
>>>>
>>>> You mean "unsigned long" like in
>>>> https://github.com/torvalds/linux/blob/76657eaef4a759e695eb1883d4f1d9af1e4ff9a8/lib/usercopy.c#L57
>>
>> yes
>>
>>>> ? That would mean that this code needs to be much more complicated to
>>>> handle the cases for the unaligned start and end bytes, need
>>>> aligned_byte_mask helper etc. Even though kernel seems to have extra
>>>> code for these cases iiuc it can still get EFAULT on specific
>>>> conditions.
>>
>> OK, I don't want too complicated code here, so I think we can keep your version.
>> But could you move this code to a function?
> 
> Sure, but could you tell me where do you want it defined. syscall.c
> does not seem to have generic helper functions and current helpers
> seem to be macros in qemu.h . Also, are we talking about a function
> like check_zeroed_user() or a variant of lock_user() with two size
> parameters(or lock_user_struct() with extra size param)?

You can put it in syscall.c, and I'm thinking more about a function like check_zeroed_user().
But feel free to do as you want, and if you think it's not a good idea to have a function you can 
keep the code as is.

Thanks,
Laurent