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

["Tõnis Tiigi" <tonistiigi@gmail.com>]

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