Le 03/01/2022 à 20:31, Tõnis Tiigi a écrit :
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.
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#L57yes? 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
