Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lkl: 32-bit timespec fixes #556

Merged
merged 5 commits into from
Feb 6, 2025
Merged

lkl: 32-bit timespec fixes #556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef3fb8c
lkl tools: use 64-bit lkl_sys_nanosleep on 32-bit builds
ddiss Jan 21, 2025
4348d61
lkl tools: fix some -Wincompatible-pointer-types errors
ddiss Jan 28, 2025
75fd3b6
lkl cptofs: handle stat errors earlier
ddiss Feb 3, 2025
ab1c2d0
lkl: rework broken lkl_sys_select timeval conversion
ddiss Feb 4, 2025
0692c1e
lkl: fix 32-bit timespec casts
ddiss Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 28 additions & 16 deletions tools/lkl/cptofs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ static int stat_src(const char *path, unsigned int *type, unsigned int *mode,

if (cptofs) {
ret = lstat(path, &stat);
if (ret)
goto err_out;

if (type)
*type = stat.st_mode & S_IFMT;
if (mode)
Expand All @@ -279,6 +282,9 @@ static int stat_src(const char *path, unsigned int *type, unsigned int *mode,
}
} else {
ret = lkl_sys_lstat(path, &lkl_stat);
if (ret)
goto err_out;

if (type)
*type = lkl_stat.st_mode & S_IFMT;
if (mode)
Expand All @@ -295,6 +301,7 @@ static int stat_src(const char *path, unsigned int *type, unsigned int *mode,
}
}

err_out:
if (ret)
fprintf(stderr, "fsimg lstat(%s) error: %s\n",
path, cptofs ? strerror(errno) : lkl_strerror(ret));
Expand Down Expand Up @@ -430,6 +437,8 @@ static int do_entry(const char *_src, const char *_dst, const char *name, uid_t
snprintf(dst, sizeof(dst), "%s/%s", _dst, name);

ret = stat_src(src, &type, &mode, NULL, &mtime, &atime);
if (ret)
goto err_out;

switch (type) {
case S_IFREG:
Expand All @@ -454,24 +463,27 @@ static int do_entry(const char *_src, const char *_dst, const char *name, uid_t
printf("skipping %s: unsupported entry type %d\n", src, type);
}

if (!ret) {
if (cptofs) {
struct lkl_timespec lkl_ts[] = { atime, mtime };
if (ret)
goto err_out;

ret = lkl_sys_utimensat(LKL_AT_FDCWD, dst,
(struct __lkl__kernel_timespec
*)lkl_ts,
LKL_AT_SYMLINK_NOFOLLOW);
} else {
struct timespec ts[] = {
{ .tv_sec = atime.tv_sec, .tv_nsec = atime.tv_nsec, },
{ .tv_sec = mtime.tv_sec, .tv_nsec = mtime.tv_nsec, },
};
if (cptofs) {
struct __lkl__kernel_timespec lkl_ts[] = {
{ .tv_sec = atime.tv_sec, .tv_nsec = atime.tv_nsec, },
{ .tv_sec = mtime.tv_sec, .tv_nsec = mtime.tv_nsec, },
};

ret = utimensat(AT_FDCWD, dst, ts, AT_SYMLINK_NOFOLLOW);
}
ret = lkl_sys_utimensat(LKL_AT_FDCWD, dst, lkl_ts,
LKL_AT_SYMLINK_NOFOLLOW);
} else {
struct timespec ts[] = {
{ .tv_sec = atime.tv_sec, .tv_nsec = atime.tv_nsec, },
{ .tv_sec = mtime.tv_sec, .tv_nsec = mtime.tv_nsec, },
};

ret = utimensat(AT_FDCWD, dst, ts, AT_SYMLINK_NOFOLLOW);
}

err_out:
if (ret)
printf("error processing entry %s, aborting\n", src);

Expand Down Expand Up @@ -684,11 +696,11 @@ int main(int argc, char **argv)
if (ret == 0)
break;
if (ret == -EBUSY) {
struct lkl_timespec ts = {
struct __lkl__kernel_timespec ts = {
.tv_sec = 1,
.tv_nsec = 0,
};
lkl_sys_nanosleep((struct __lkl__kernel_timespec *)&ts, NULL);
lkl_sys_nanosleep(&ts, NULL);
continue;
} else if (ret < 0) {
fprintf(stderr, "cannot remount mount disk read-only: %s\n", lkl_strerror(ret));
Expand Down
36 changes: 17 additions & 19 deletions tools/lkl/include/lkl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,12 @@ static inline int lkl_sys_fstatfs(unsigned int fd, struct lkl_statfs *buf)
return lkl_sys_fstatfs64(fd, sizeof(*buf), buf);
}

#define lkl_sys_nanosleep lkl_sys_nanosleep_time32
static inline int lkl_sys_nanosleep_time32(struct lkl_timespec *rqtp,
struct lkl_timespec *rmtp)
static inline int lkl_sys_nanosleep(struct __lkl__kernel_timespec *rqtp,
struct __lkl__kernel_timespec *rmtp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should follow what Linux does and provide timespec for LKL APIs, even if for 32 bit machines it means that we loose range. Per timespec(3) man:

SYNOPSIS
       #include <time.h>

       struct timespec {
           time_t     tv_sec;   /* Seconds */
           /* ... */  tv_nsec;  /* Nanoseconds [0, 999'999'999] */
       };

DESCRIPTION
       Describes times in seconds and nanoseconds.

       tv_nsec  is  of  an implementation-defined signed type capable of holding the specified range.  Under glibc, this is usually long,
       and long long on X32.  It can be safely down-cast to any concrete 32-bit integer type for processing.

So basically use a local __lkl_kernel_timespec, copy the data from lkl_timespec argument, and pass the local to the system call.

Then we can remove __lkl_kernel_timespec from LKL libs and apps which we introduced in 3d4047a. @thehajime does this sound reasonable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you just want this wrapper for nanosleep, or should I also add lkl_timespec wrappers for lkl_sys_utimensat()? I was hoping to avoid going too far down the generic-libc path and just stick to the modern kernel API.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a962b15 to this branch (with two extra changes) to show where other wrappers would be needed if we switch callers to lkl_timespec.
On the generic-libc side, I wonder whether lkl could hook into nolibc via e.g. tools/include/nolibc/arch-lkl.h. It's already in-tree and relatively exhaustive in terms of syscalls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can remove __lkl_kernel_timespec from LKL libs and apps which we introduced in 3d4047a. @thehajime does this sound reasonable?

sorry, I don't quite remember what/why my followup fixes (3d4047a) did.. my best guess is to silence warnings from compilation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you just want this wrapper for nanosleep, or should I also add lkl_timespec wrappers for lkl_sys_utimensat()? I was hoping to avoid going too far down the generic-libc path and just stick to the modern kernel API.

I thought more about this and I think you are right, we should stick with modern kernel APIs.

We should be able to override struct timespec defined in include/uapi/linux/time.h with a custom one arch/lkl/include/uapi/asm/types.h. If you prefer to merge as is (use __lkl_kernel_timespec) for now we can cleanup later.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...

We should be able to override struct timespec defined in include/uapi/linux/time.h with a custom one arch/lkl/include/uapi/asm/types.h. If you prefer to merge as is (use __lkl_kernel_timespec) for now we can cleanup later.

I'm not sure I follow; would you like some sort of helper macro to ease conversion from timespec to __lkl_kernel_timespec? Thanks a lot for the review btw

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets merge this as it is and I will followup with a proposal.

{
long p[6] = {(long)rqtp, (long)rmtp, 0, 0, 0, 0};
long p[6] = {LKL_CLOCK_MONOTONIC, 0, (long)rqtp, (long)rmtp, 0};

return lkl_syscall(__lkl__NR_nanosleep, p);
return lkl_syscall(__lkl__NR_clock_nanosleep_time64, p);
}

#endif
Expand Down Expand Up @@ -277,21 +276,16 @@ static inline long lkl_sys_select(int n, lkl_fd_set *rfds, lkl_fd_set *wfds,
lkl_fd_set *efds, struct lkl_timeval *tv)
{
long data[2] = { 0, _LKL_NSIG/8 };
struct lkl_timespec ts;
lkl_time_t extra_secs;
const lkl_time_t max_time = ((1ULL<<8)*sizeof(time_t)-1)-1;
struct __lkl__kernel_timespec ts;

if (tv) {
if (tv->tv_sec < 0 || tv->tv_usec < 0)
return -LKL_EINVAL;

extra_secs = tv->tv_usec / 1000000;
ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
max_time : tv->tv_sec + extra_secs;
ts.tv_sec = tv->tv_sec;
ts.tv_nsec = tv->tv_usec * 1000;
}
return lkl_sys_pselect6(n, rfds, wfds, efds, tv ?
(struct __lkl__kernel_timespec *)&ts : 0, data);
return lkl_sys_pselect6(n, rfds, wfds, efds, tv ? &ts : 0, data);
}
#endif

Expand All @@ -301,11 +295,15 @@ static inline long lkl_sys_select(int n, lkl_fd_set *rfds, lkl_fd_set *wfds,
*/
static inline long lkl_sys_poll(struct lkl_pollfd *fds, int n, int timeout)
{
return lkl_sys_ppoll(fds, n, timeout >= 0 ?
(struct __lkl__kernel_timespec *)
&((struct lkl_timespec){ .tv_sec = timeout/1000,
.tv_nsec = timeout%1000*1000000 }) : 0,
0, _LKL_NSIG/8);
struct __lkl__kernel_timespec ts;

if (timeout >= 0)
ts = (struct __lkl__kernel_timespec){
.tv_sec = timeout / 1000,
.tv_nsec = timeout % 1000 * 1000000,
};

return lkl_sys_ppoll(fds, n, timeout >= 0 ? &ts : 0, 0, _LKL_NSIG/8);
}
#endif

Expand Down
5 changes: 2 additions & 3 deletions tools/lkl/lib/fs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ long lkl_mount_dev(unsigned int disk_id, unsigned int part,
long lkl_umount_timeout(char *path, int flags, long timeout_ms)
{
long incr = 10000000; /* 10 ms */
struct lkl_timespec ts = {
struct __lkl__kernel_timespec ts = {
.tv_sec = 0,
.tv_nsec = incr,
};
Expand All @@ -284,8 +284,7 @@ long lkl_umount_timeout(char *path, int flags, long timeout_ms)
do {
err = lkl_sys_umount(path, flags);
if (err == -LKL_EBUSY) {
lkl_sys_nanosleep((struct __lkl__kernel_timespec *)&ts,
NULL);
lkl_sys_nanosleep(&ts, NULL);
timeout_ms -= incr / 1000000;
}
} while (err == -LKL_EBUSY && timeout_ms > 0);
Expand Down
21 changes: 18 additions & 3 deletions tools/lkl/lib/posix-host.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,21 @@ static void *lkl_shmem_mmap(void *addr, unsigned long pg_off,
}
#endif // LKL_HOST_CONFIG_MMU

static void *posix_malloc(unsigned long size)
{
return malloc((size_t)size);
}

static void *posix_memcpy(void *dest, const void *src, unsigned long n)
{
return memcpy(dest, src, (size_t)n);
}

static void *posix_memset(void *s, int c, unsigned long n)
{
return memset(s, c, (size_t)n);
}

struct lkl_host_operations lkl_host_ops = {
.panic = panic,
.thread_create = thread_create,
Expand All @@ -547,7 +562,7 @@ struct lkl_host_operations lkl_host_ops = {
.timer_set_oneshot = timer_set_oneshot,
.timer_free = timer_free,
.print = print,
.mem_alloc = malloc,
.mem_alloc = posix_malloc,
.mem_free = free,
.page_alloc = page_alloc,
.page_free = page_free,
Expand All @@ -557,8 +572,8 @@ struct lkl_host_operations lkl_host_ops = {
.gettid = _gettid,
.jmp_buf_set = jmp_buf_set,
.jmp_buf_longjmp = jmp_buf_longjmp,
.memcpy = memcpy,
.memset = memset,
.memcpy = posix_memcpy,
.memset = posix_memset,
.mmap = lkl_mmap,
.munmap = lkl_munmap,
#ifdef LKL_HOST_CONFIG_MMU
Expand Down
20 changes: 8 additions & 12 deletions tools/lkl/lklfuse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,19 +503,16 @@ static int lklfuse_utimens(const char *path, const struct timespec tv[2],
struct fuse_file_info *fi)
{
int ret;
struct lkl_timespec ts[2] = {
struct __lkl__kernel_timespec ts[2] = {
{ .tv_sec = tv[0].tv_sec, .tv_nsec = tv[0].tv_nsec },
{ .tv_sec = tv[1].tv_sec, .tv_nsec = tv[1].tv_nsec },
};

if (fi)
ret = lkl_sys_utimensat(fi->fh, NULL,
(struct __lkl__kernel_timespec *)ts,
0);
ret = lkl_sys_utimensat(fi->fh, NULL, ts, 0);
else
ret = lkl_sys_utimensat(-1, path,
(struct __lkl__kernel_timespec *)ts,
LKL_AT_SYMLINK_NOFOLLOW);
ret = lkl_sys_utimensat(-1, path, ts, LKL_AT_SYMLINK_NOFOLLOW);

return ret;
}

Expand Down Expand Up @@ -686,7 +683,7 @@ static int start_lkl(void)
long ret;
char mpoint[32];
struct timespec walltime;
struct lkl_timespec ts;
struct __lkl__kernel_timespec ts;
int mount_flags = 0;
char remaining_mopts[4096] = { 0 };

Expand All @@ -709,10 +706,9 @@ static int start_lkl(void)
if (ret < 0)
goto out_halt;

ts = (struct lkl_timespec){ .tv_sec = walltime.tv_sec,
.tv_nsec = walltime.tv_nsec };
ret = lkl_sys_clock_settime(LKL_CLOCK_REALTIME,
(struct __lkl__kernel_timespec *)&ts);
ts = (struct __lkl__kernel_timespec){ .tv_sec = walltime.tv_sec,
.tv_nsec = walltime.tv_nsec };
ret = lkl_sys_clock_settime(LKL_CLOCK_REALTIME, &ts);
if (ret < 0) {
fprintf(stderr, "lkl_sys_clock_settime() failed: %s\n",
lkl_strerror(ret));
Expand Down
7 changes: 4 additions & 3 deletions tools/lkl/tests/boot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#define sleep_ns 87654321
int lkl_test_nanosleep(void)
{
struct lkl_timespec ts = {
struct __lkl__kernel_timespec ts = {
.tv_sec = 0,
.tv_nsec = sleep_ns,
};
Expand All @@ -36,13 +36,14 @@ int lkl_test_nanosleep(void)
long ret;

clock_gettime(CLOCK_MONOTONIC, &start);
ret = lkl_sys_nanosleep((struct __lkl__kernel_timespec *)&ts, NULL);
ret = lkl_sys_nanosleep(&ts, NULL);
clock_gettime(CLOCK_MONOTONIC, &stop);

delta = 1e9*(stop.tv_sec - start.tv_sec) +
(stop.tv_nsec - start.tv_nsec);

lkl_test_logf("sleep %ld, expected sleep %d\n", delta, sleep_ns);
lkl_test_logf("sleep %ld (ret=%ld), expected sleep %d\n",
delta, ret, sleep_ns);

if (ret == 0 && delta > sleep_ns * 0.9)
return TEST_SUCCESS;
Expand Down
4 changes: 2 additions & 2 deletions tools/lkl/tests/net-test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,12 @@ in_cksum(const u_short *addr, register int len, u_short csum)

static int lkl_test_sleep(void)
{
struct lkl_timespec ts = {
struct __lkl__kernel_timespec ts = {
.tv_sec = cla.sleep,
};
int ret;

ret = lkl_sys_nanosleep((struct __lkl__kernel_timespec *)&ts, NULL);
ret = lkl_sys_nanosleep(&ts, NULL);
if (ret < 0) {
lkl_test_logf("nanosleep error: %s\n", lkl_strerror(ret));
return TEST_FAILURE;
Expand Down
Loading