On Mon, Jan 3, 2022 at 10:37 AM Laurent Vivier <laur...@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 <tonisti...@gmail.com> wrote:
> >>
> >> On Thu, Dec 23, 2021 at 1:03 PM Laurent Vivier <laur...@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 <tonisti...@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
>
