[PATCH v3 0/3] nolibc: add part2 of support for rv32

[PATCH v3 0/3] nolibc: add part2 of support for rv32

[Zhangjin Wu]

Hi, Willy

This is the v3 part2 of support for rv32, differs from the v2 part2 [1],
we only fix up compile issues in this patchset.

With the v3 generic part1 [2] and this patchset, we can compile nolibc
for rv32 now.

This is based on the idea of suggestions from Arnd [3], instead of
'#error' on the unsupported syscall on a target platform, a 'return
-ENOSYS' allow us to compile it at first and then allow we fix up the
test failures reported by nolibc-test one by one.

The first two patches fix up all of the compile failures with '-ENOSYS'
(and '#ifdef' if required):

  tools/nolibc: fix up #error compile failures with -ENOSYS
  tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS

The last one enables rv32 compile support:
  
  selftests/nolibc: riscv: customize makefile for rv32

The above compile support patch here is only for test currently, as
Thomas suggested, for a full rv32 support, it should wait for the left
parts.

Welcome your feedbacks, will wait for enough discussion on this patchset
and then send the left parts one by one to fix up the test failures
about waitid, llseek and time64 syscalls: ppoll_time64, clock_gettime64,
pselect6_time64.

So, I do recommend to apply this patchset, it allows us to send the left
parts independently, otherwise, all of them should be sent out for
review together. with this patchset, the rv32 users may be able to use
nolibc although some syscalls still missing :-)

Or at least we apply the first two, so, I can manually cherry-pick the
compile support patch to do my local test, and the other platform
developer may also benefit from them.

I'm cleaning up the left parts, but still require some time, I plan to
split them to such parts:

  * part3: waitid, prepared, will send out later
  * part4: llseek, prepared, will send out later
  * part5: time64 syscalls, ppoll_time64 ok, will finish them next week
           (It is a little hard to split them)

Best regards,
Zhangjin
---

[1]: https://lore.kernel.org/linux-riscv/cover.1685387484.git.falcon@tinylab.org/T/#t
[2]: https://lore.kernel.org/linux-riscv/cover.1685777982.git.falcon@tinylab.org/T/#t
[3]: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/

Zhangjin Wu (3):
  tools/nolibc: fix up #error compile failures with -ENOSYS
  tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS
  selftests/nolibc: riscv: customize makefile for rv32

 tools/include/nolibc/sys.h              | 38 ++++++++++++++++---------
 tools/testing/selftests/nolibc/Makefile | 11 +++++--
 2 files changed, 34 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH v3 0/3] nolibc: add part2 of support for rv32

[Zhangjin Wu]

Hi, Willy

This is the v3 part2 of support for rv32, differs from the v2 part2 [1],
we only fix up compile issues in this patchset.

With the v3 generic part1 [2] and this patchset, we can compile nolibc
for rv32 now.

This is based on the idea of suggestions from Arnd [3], instead of
'#error' on the unsupported syscall on a target platform, a 'return
-ENOSYS' allow us to compile it at first and then allow we fix up the
test failures reported by nolibc-test one by one.

The first two patches fix up all of the compile failures with '-ENOSYS'
(and '#ifdef' if required):

  tools/nolibc: fix up #error compile failures with -ENOSYS
  tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS

The last one enables rv32 compile support:
  
  selftests/nolibc: riscv: customize makefile for rv32

The above compile support patch here is only for test currently, as
Thomas suggested, for a full rv32 support, it should wait for the left
parts.

Welcome your feedbacks, will wait for enough discussion on this patchset
and then send the left parts one by one to fix up the test failures
about waitid, llseek and time64 syscalls: ppoll_time64, clock_gettime64,
pselect6_time64.

So, I do recommend to apply this patchset, it allows us to send the left
parts independently, otherwise, all of them should be sent out for
review together. with this patchset, the rv32 users may be able to use
nolibc although some syscalls still missing :-)

Or at least we apply the first two, so, I can manually cherry-pick the
compile support patch to do my local test, and the other platform
developer may also benefit from them.

I'm cleaning up the left parts, but still require some time, I plan to
split them to such parts:

  * part3: waitid, prepared, will send out later
  * part4: llseek, prepared, will send out later
  * part5: time64 syscalls, ppoll_time64 ok, will finish them next week
           (It is a little hard to split them)

Best regards,
Zhangjin
---

[1]: https://lore.kernel.org/linux-riscv/cover.1685387484.git.falcon@tinylab.org/T/#t
[2]: https://lore.kernel.org/linux-riscv/cover.1685777982.git.falcon@tinylab.org/T/#t
[3]: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/

Zhangjin Wu (3):
  tools/nolibc: fix up #error compile failures with -ENOSYS
  tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS
  selftests/nolibc: riscv: customize makefile for rv32

 tools/include/nolibc/sys.h              | 38 ++++++++++++++++---------
 tools/testing/selftests/nolibc/Makefile | 11 +++++--
 2 files changed, 34 insertions(+), 15 deletions(-)

-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

Compiling nolibc for rv32 got such errors:

    In file included from nolibc/sysroot/riscv/include/nolibc.h:99,
                     from nolibc/sysroot/riscv/include/errno.h:26,
                     from nolibc/sysroot/riscv/include/stdio.h:14,
                     from tools/testing/selftests/nolibc/nolibc-test.c:12:
    nolibc/sysroot/riscv/include/sys.h:946:2: error: #error Neither __NR_ppoll nor __NR_poll defined, cannot implement sys_poll()
      946 | #error Neither __NR_ppoll nor __NR_poll defined, cannot implement sys_poll()
          |  ^~~~~
    nolibc/sysroot/riscv/include/sys.h:1062:2: error: #error None of __NR_select, __NR_pselect6, nor __NR__newselect defined, cannot implement sys_select()
     1062 | #error None of __NR_select, __NR_pselect6, nor __NR__newselect defined, cannot implement sys_select()

If a syscall is not supported by a target platform, 'return -ENOSYS' is
better than '#error', which lets the other syscalls work as-is and
allows developers to fix up the test failures reported by nolibc-test
one by one later.

This converts all of the '#error' to 'return -ENOSYS', so, all of the
'#error' failures are fixed.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 856249a11890..78c86f124335 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -124,7 +124,7 @@ int sys_chmod(const char *path, mode_t mode)
 #elif defined(__NR_chmod)
 	return my_syscall2(__NR_chmod, path, mode);
 #else
-#error Neither __NR_fchmodat nor __NR_chmod defined, cannot implement sys_chmod()
+	return -ENOSYS;
 #endif
 }
 
@@ -153,7 +153,7 @@ int sys_chown(const char *path, uid_t owner, gid_t group)
 #elif defined(__NR_chown)
 	return my_syscall3(__NR_chown, path, owner, group);
 #else
-#error Neither __NR_fchownat nor __NR_chown defined, cannot implement sys_chown()
+	return -ENOSYS;
 #endif
 }
 
@@ -251,7 +251,7 @@ int sys_dup2(int old, int new)
 #elif defined(__NR_dup2)
 	return my_syscall2(__NR_dup2, old, new);
 #else
-#error Neither __NR_dup3 nor __NR_dup2 defined, cannot implement sys_dup2()
+	return -ENOSYS;
 #endif
 }
 
@@ -351,7 +351,7 @@ pid_t sys_fork(void)
 #elif defined(__NR_fork)
 	return my_syscall0(__NR_fork);
 #else
-#error Neither __NR_clone nor __NR_fork defined, cannot implement sys_fork()
+	return -ENOSYS;
 #endif
 }
 #endif
@@ -648,7 +648,7 @@ int sys_link(const char *old, const char *new)
 #elif defined(__NR_link)
 	return my_syscall2(__NR_link, old, new);
 #else
-#error Neither __NR_linkat nor __NR_link defined, cannot implement sys_link()
+	return -ENOSYS;
 #endif
 }
 
@@ -700,7 +700,7 @@ int sys_mkdir(const char *path, mode_t mode)
 #elif defined(__NR_mkdir)
 	return my_syscall2(__NR_mkdir, path, mode);
 #else
-#error Neither __NR_mkdirat nor __NR_mkdir defined, cannot implement sys_mkdir()
+	return -ENOSYS;
 #endif
 }
 
@@ -729,7 +729,7 @@ long sys_mknod(const char *path, mode_t mode, dev_t dev)
 #elif defined(__NR_mknod)
 	return my_syscall3(__NR_mknod, path, mode, dev);
 #else
-#error Neither __NR_mknodat nor __NR_mknod defined, cannot implement sys_mknod()
+	return -ENOSYS;
 #endif
 }
 
@@ -848,7 +848,7 @@ int sys_open(const char *path, int flags, mode_t mode)
 #elif defined(__NR_open)
 	return my_syscall3(__NR_open, path, flags, mode);
 #else
-#error Neither __NR_openat nor __NR_open defined, cannot implement sys_open()
+	return -ENOSYS;
 #endif
 }
 
@@ -943,7 +943,7 @@ int sys_poll(struct pollfd *fds, int nfds, int timeout)
 #elif defined(__NR_poll)
 	return my_syscall3(__NR_poll, fds, nfds, timeout);
 #else
-#error Neither __NR_ppoll nor __NR_poll defined, cannot implement sys_poll()
+	return -ENOSYS;
 #endif
 }
 
@@ -1059,7 +1059,7 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva
 #endif
 	return my_syscall5(__NR__newselect, nfds, rfds, wfds, efds, timeout);
 #else
-#error None of __NR_select, __NR_pselect6, nor __NR__newselect defined, cannot implement sys_select()
+	return -ENOSYS;
 #endif
 }
 
@@ -1196,7 +1196,7 @@ int sys_stat(const char *path, struct stat *buf)
 #elif defined(__NR_stat)
 	ret = my_syscall2(__NR_stat, path, &stat);
 #else
-#error Neither __NR_newfstatat nor __NR_stat defined, cannot implement sys_stat()
+	return -ENOSYS;
 #endif
 	buf->st_dev          = stat.st_dev;
 	buf->st_ino          = stat.st_ino;
@@ -1243,7 +1243,7 @@ int sys_symlink(const char *old, const char *new)
 #elif defined(__NR_symlink)
 	return my_syscall2(__NR_symlink, old, new);
 #else
-#error Neither __NR_symlinkat nor __NR_symlink defined, cannot implement sys_symlink()
+	return -ENOSYS;
 #endif
 }
 
@@ -1312,7 +1312,7 @@ int sys_unlink(const char *path)
 #elif defined(__NR_unlink)
 	return my_syscall1(__NR_unlink, path);
 #else
-#error Neither __NR_unlinkat nor __NR_unlink defined, cannot implement sys_unlink()
+	return -ENOSYS;
 #endif
 }
 
-- 
2.25.1

[PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

Compiling nolibc for rv32 got such errors:

    In file included from nolibc/sysroot/riscv/include/nolibc.h:99,
                     from nolibc/sysroot/riscv/include/errno.h:26,
                     from nolibc/sysroot/riscv/include/stdio.h:14,
                     from tools/testing/selftests/nolibc/nolibc-test.c:12:
    nolibc/sysroot/riscv/include/sys.h:946:2: error: #error Neither __NR_ppoll nor __NR_poll defined, cannot implement sys_poll()
      946 | #error Neither __NR_ppoll nor __NR_poll defined, cannot implement sys_poll()
          |  ^~~~~
    nolibc/sysroot/riscv/include/sys.h:1062:2: error: #error None of __NR_select, __NR_pselect6, nor __NR__newselect defined, cannot implement sys_select()
     1062 | #error None of __NR_select, __NR_pselect6, nor __NR__newselect defined, cannot implement sys_select()

If a syscall is not supported by a target platform, 'return -ENOSYS' is
better than '#error', which lets the other syscalls work as-is and
allows developers to fix up the test failures reported by nolibc-test
one by one later.

This converts all of the '#error' to 'return -ENOSYS', so, all of the
'#error' failures are fixed.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 856249a11890..78c86f124335 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -124,7 +124,7 @@ int sys_chmod(const char *path, mode_t mode)
 #elif defined(__NR_chmod)
 	return my_syscall2(__NR_chmod, path, mode);
 #else
-#error Neither __NR_fchmodat nor __NR_chmod defined, cannot implement sys_chmod()
+	return -ENOSYS;
 #endif
 }
 
@@ -153,7 +153,7 @@ int sys_chown(const char *path, uid_t owner, gid_t group)
 #elif defined(__NR_chown)
 	return my_syscall3(__NR_chown, path, owner, group);
 #else
-#error Neither __NR_fchownat nor __NR_chown defined, cannot implement sys_chown()
+	return -ENOSYS;
 #endif
 }
 
@@ -251,7 +251,7 @@ int sys_dup2(int old, int new)
 #elif defined(__NR_dup2)
 	return my_syscall2(__NR_dup2, old, new);
 #else
-#error Neither __NR_dup3 nor __NR_dup2 defined, cannot implement sys_dup2()
+	return -ENOSYS;
 #endif
 }
 
@@ -351,7 +351,7 @@ pid_t sys_fork(void)
 #elif defined(__NR_fork)
 	return my_syscall0(__NR_fork);
 #else
-#error Neither __NR_clone nor __NR_fork defined, cannot implement sys_fork()
+	return -ENOSYS;
 #endif
 }
 #endif
@@ -648,7 +648,7 @@ int sys_link(const char *old, const char *new)
 #elif defined(__NR_link)
 	return my_syscall2(__NR_link, old, new);
 #else
-#error Neither __NR_linkat nor __NR_link defined, cannot implement sys_link()
+	return -ENOSYS;
 #endif
 }
 
@@ -700,7 +700,7 @@ int sys_mkdir(const char *path, mode_t mode)
 #elif defined(__NR_mkdir)
 	return my_syscall2(__NR_mkdir, path, mode);
 #else
-#error Neither __NR_mkdirat nor __NR_mkdir defined, cannot implement sys_mkdir()
+	return -ENOSYS;
 #endif
 }
 
@@ -729,7 +729,7 @@ long sys_mknod(const char *path, mode_t mode, dev_t dev)
 #elif defined(__NR_mknod)
 	return my_syscall3(__NR_mknod, path, mode, dev);
 #else
-#error Neither __NR_mknodat nor __NR_mknod defined, cannot implement sys_mknod()
+	return -ENOSYS;
 #endif
 }
 
@@ -848,7 +848,7 @@ int sys_open(const char *path, int flags, mode_t mode)
 #elif defined(__NR_open)
 	return my_syscall3(__NR_open, path, flags, mode);
 #else
-#error Neither __NR_openat nor __NR_open defined, cannot implement sys_open()
+	return -ENOSYS;
 #endif
 }
 
@@ -943,7 +943,7 @@ int sys_poll(struct pollfd *fds, int nfds, int timeout)
 #elif defined(__NR_poll)
 	return my_syscall3(__NR_poll, fds, nfds, timeout);
 #else
-#error Neither __NR_ppoll nor __NR_poll defined, cannot implement sys_poll()
+	return -ENOSYS;
 #endif
 }
 
@@ -1059,7 +1059,7 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva
 #endif
 	return my_syscall5(__NR__newselect, nfds, rfds, wfds, efds, timeout);
 #else
-#error None of __NR_select, __NR_pselect6, nor __NR__newselect defined, cannot implement sys_select()
+	return -ENOSYS;
 #endif
 }
 
@@ -1196,7 +1196,7 @@ int sys_stat(const char *path, struct stat *buf)
 #elif defined(__NR_stat)
 	ret = my_syscall2(__NR_stat, path, &stat);
 #else
-#error Neither __NR_newfstatat nor __NR_stat defined, cannot implement sys_stat()
+	return -ENOSYS;
 #endif
 	buf->st_dev          = stat.st_dev;
 	buf->st_ino          = stat.st_ino;
@@ -1243,7 +1243,7 @@ int sys_symlink(const char *old, const char *new)
 #elif defined(__NR_symlink)
 	return my_syscall2(__NR_symlink, old, new);
 #else
-#error Neither __NR_symlinkat nor __NR_symlink defined, cannot implement sys_symlink()
+	return -ENOSYS;
 #endif
 }
 
@@ -1312,7 +1312,7 @@ int sys_unlink(const char *path)
 #elif defined(__NR_unlink)
 	return my_syscall1(__NR_unlink, path);
 #else
-#error Neither __NR_unlinkat nor __NR_unlink defined, cannot implement sys_unlink()
+	return -ENOSYS;
 #endif
 }
 
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v3 2/3] tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS

[Zhangjin Wu]

Compiling nolibc for rv32 got such errors:

    nolibc/sysroot/riscv/include/sys.h: In function ‘sys_gettimeofday’:
    nolibc/sysroot/riscv/include/sys.h:557:21: error: ‘__NR_gettimeofday’ undeclared (first use in this function); did you mean ‘sys_gettimeofday’?
      557 |  return my_syscall2(__NR_gettimeofday, tv, tz);
          |                     ^~~~~~~~~~~~~~~~~
    nolibc/sysroot/riscv/include/sys.h: In function ‘sys_lseek’:
    nolibc/sysroot/riscv/include/sys.h:675:21: error: ‘__NR_lseek’ undeclared (first use in this function)
      675 |  return my_syscall3(__NR_lseek, fd, offset, whence);
          |                     ^~~~~~~~~~
    nolibc/sysroot/riscv/include/sys.h: In function ‘sys_wait4’:
    nolibc/sysroot/riscv/include/sys.h:1341:21: error: ‘__NR_wait4’ undeclared (first use in this function)
     1341 |  return my_syscall4(__NR_wait4, pid, status, options, rusage);

If a syscall macro is not supported by a target platform, wrap it with
'#ifdef' and 'return -ENOSYS' for the '#else' branch, which lets the
other syscalls work as-is and allows developers to fix up the test
failures reported by nolibc-test one by one later.

This wraps all of the failed syscall macros with '#ifdef' and 'return
-ENOSYS' for the '#else' branch, so, all of the undeclared failures are
fixed.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 78c86f124335..5464f93e863e 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -554,7 +554,11 @@ long getpagesize(void)
 static __attribute__((unused))
 int sys_gettimeofday(struct timeval *tv, struct timezone *tz)
 {
+#ifdef __NR_gettimeofday
 	return my_syscall2(__NR_gettimeofday, tv, tz);
+#else
+	return -ENOSYS;
+#endif
 }
 
 static __attribute__((unused))
@@ -672,7 +676,11 @@ int link(const char *old, const char *new)
 static __attribute__((unused))
 off_t sys_lseek(int fd, off_t offset, int whence)
 {
+#ifdef __NR_lseek
 	return my_syscall3(__NR_lseek, fd, offset, whence);
+#else
+	return -ENOSYS;
+#endif
 }
 
 static __attribute__((unused))
@@ -1338,7 +1346,11 @@ int unlink(const char *path)
 static __attribute__((unused))
 pid_t sys_wait4(pid_t pid, int *status, int options, struct rusage *rusage)
 {
+#ifdef __NR_wait4
 	return my_syscall4(__NR_wait4, pid, status, options, rusage);
+#else
+	return -ENOSYS;
+#endif
 }
 
 static __attribute__((unused))
-- 
2.25.1

[PATCH v3 2/3] tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS

[Zhangjin Wu]

Compiling nolibc for rv32 got such errors:

    nolibc/sysroot/riscv/include/sys.h: In function ‘sys_gettimeofday’:
    nolibc/sysroot/riscv/include/sys.h:557:21: error: ‘__NR_gettimeofday’ undeclared (first use in this function); did you mean ‘sys_gettimeofday’?
      557 |  return my_syscall2(__NR_gettimeofday, tv, tz);
          |                     ^~~~~~~~~~~~~~~~~
    nolibc/sysroot/riscv/include/sys.h: In function ‘sys_lseek’:
    nolibc/sysroot/riscv/include/sys.h:675:21: error: ‘__NR_lseek’ undeclared (first use in this function)
      675 |  return my_syscall3(__NR_lseek, fd, offset, whence);
          |                     ^~~~~~~~~~
    nolibc/sysroot/riscv/include/sys.h: In function ‘sys_wait4’:
    nolibc/sysroot/riscv/include/sys.h:1341:21: error: ‘__NR_wait4’ undeclared (first use in this function)
     1341 |  return my_syscall4(__NR_wait4, pid, status, options, rusage);

If a syscall macro is not supported by a target platform, wrap it with
'#ifdef' and 'return -ENOSYS' for the '#else' branch, which lets the
other syscalls work as-is and allows developers to fix up the test
failures reported by nolibc-test one by one later.

This wraps all of the failed syscall macros with '#ifdef' and 'return
-ENOSYS' for the '#else' branch, so, all of the undeclared failures are
fixed.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 78c86f124335..5464f93e863e 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -554,7 +554,11 @@ long getpagesize(void)
 static __attribute__((unused))
 int sys_gettimeofday(struct timeval *tv, struct timezone *tz)
 {
+#ifdef __NR_gettimeofday
 	return my_syscall2(__NR_gettimeofday, tv, tz);
+#else
+	return -ENOSYS;
+#endif
 }
 
 static __attribute__((unused))
@@ -672,7 +676,11 @@ int link(const char *old, const char *new)
 static __attribute__((unused))
 off_t sys_lseek(int fd, off_t offset, int whence)
 {
+#ifdef __NR_lseek
 	return my_syscall3(__NR_lseek, fd, offset, whence);
+#else
+	return -ENOSYS;
+#endif
 }
 
 static __attribute__((unused))
@@ -1338,7 +1346,11 @@ int unlink(const char *path)
 static __attribute__((unused))
 pid_t sys_wait4(pid_t pid, int *status, int options, struct rusage *rusage)
 {
+#ifdef __NR_wait4
 	return my_syscall4(__NR_wait4, pid, status, options, rusage);
+#else
+	return -ENOSYS;
+#endif
 }
 
 static __attribute__((unused))
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

Both riscv64 and riscv32 have:

* the same ARCH value, it is riscv
* the same arch/riscv source code tree

The only differences are:

* riscv64 uses defconfig, riscv32 uses rv32_defconfig
* riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32
* riscv32 has different compiler options (-march= and -mabi=)

So, riscv32 can share most of the settings with riscv64, there is no
need to add it as a whole new architecture but just need a flag to
record and reflect the difference.

The 32bit mips and loongarch may be able to use the same method, so,
let's use a meaningful flag: CONFIG_32BIT. If required in the future,
this flag can also be automatically loaded from
include/config/auto.conf.

With this patch, it is able to run nolibc test for rv32 like this:

    $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- ...

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 44088535682e..ea434a0acdc1 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include
 ARCH = $(SUBARCH)
 endif
 
+# Allow pass ARCH=riscv|riscv32|riscv64, riscv implies riscv64
+ifneq ($(findstring xriscv,x$(ARCH)),)
+  CONFIG_32BIT := $(if $(findstring 32x,$(ARCH)x),1)
+  override ARCH := riscv
+endif
+
 # kernel image names by architecture
 IMAGE_i386       = arch/x86/boot/bzImage
 IMAGE_x86_64     = arch/x86/boot/bzImage
@@ -34,7 +40,7 @@ DEFCONFIG_x86        = defconfig
 DEFCONFIG_arm64      = defconfig
 DEFCONFIG_arm        = multi_v7_defconfig
 DEFCONFIG_mips       = malta_defconfig
-DEFCONFIG_riscv      = defconfig
+DEFCONFIG_riscv      = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig)
 DEFCONFIG_s390       = defconfig
 DEFCONFIG_loongarch  = defconfig
 DEFCONFIG            = $(DEFCONFIG_$(ARCH))
@@ -49,7 +55,7 @@ QEMU_ARCH_x86        = x86_64
 QEMU_ARCH_arm64      = aarch64
 QEMU_ARCH_arm        = arm
 QEMU_ARCH_mips       = mipsel  # works with malta_defconfig
-QEMU_ARCH_riscv      = riscv64
+QEMU_ARCH_riscv      = $(if $(CONFIG_32BIT),riscv32,riscv64)
 QEMU_ARCH_s390       = s390x
 QEMU_ARCH_loongarch  = loongarch64
 QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
@@ -76,6 +82,7 @@ else
 Q=@
 endif
 
+CFLAGS_riscv = $(if $(CONFIG_32BIT),-march=rv32i -mabi=ilp32)
 CFLAGS_s390 = -m64
 CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
 CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
-- 
2.25.1

[PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

Both riscv64 and riscv32 have:

* the same ARCH value, it is riscv
* the same arch/riscv source code tree

The only differences are:

* riscv64 uses defconfig, riscv32 uses rv32_defconfig
* riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32
* riscv32 has different compiler options (-march= and -mabi=)

So, riscv32 can share most of the settings with riscv64, there is no
need to add it as a whole new architecture but just need a flag to
record and reflect the difference.

The 32bit mips and loongarch may be able to use the same method, so,
let's use a meaningful flag: CONFIG_32BIT. If required in the future,
this flag can also be automatically loaded from
include/config/auto.conf.

With this patch, it is able to run nolibc test for rv32 like this:

    $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- ...

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 44088535682e..ea434a0acdc1 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include
 ARCH = $(SUBARCH)
 endif
 
+# Allow pass ARCH=riscv|riscv32|riscv64, riscv implies riscv64
+ifneq ($(findstring xriscv,x$(ARCH)),)
+  CONFIG_32BIT := $(if $(findstring 32x,$(ARCH)x),1)
+  override ARCH := riscv
+endif
+
 # kernel image names by architecture
 IMAGE_i386       = arch/x86/boot/bzImage
 IMAGE_x86_64     = arch/x86/boot/bzImage
@@ -34,7 +40,7 @@ DEFCONFIG_x86        = defconfig
 DEFCONFIG_arm64      = defconfig
 DEFCONFIG_arm        = multi_v7_defconfig
 DEFCONFIG_mips       = malta_defconfig
-DEFCONFIG_riscv      = defconfig
+DEFCONFIG_riscv      = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig)
 DEFCONFIG_s390       = defconfig
 DEFCONFIG_loongarch  = defconfig
 DEFCONFIG            = $(DEFCONFIG_$(ARCH))
@@ -49,7 +55,7 @@ QEMU_ARCH_x86        = x86_64
 QEMU_ARCH_arm64      = aarch64
 QEMU_ARCH_arm        = arm
 QEMU_ARCH_mips       = mipsel  # works with malta_defconfig
-QEMU_ARCH_riscv      = riscv64
+QEMU_ARCH_riscv      = $(if $(CONFIG_32BIT),riscv32,riscv64)
 QEMU_ARCH_s390       = s390x
 QEMU_ARCH_loongarch  = loongarch64
 QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
@@ -76,6 +82,7 @@ else
 Q=@
 endif
 
+CFLAGS_riscv = $(if $(CONFIG_32BIT),-march=rv32i -mabi=ilp32)
 CFLAGS_s390 = -m64
 CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
 CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v3 0/3] nolibc: add part2 of support for rv32

[Zhangjin Wu]

Hi, Arnd

Because this patchset is a 'big' change derived from the idea of suggestion
from you [3], I do very welcome your feedback about this change, just like
Thomas suggested, this requires more discussion before Willy plan to determine
merge it or not.

The first two convert all compile failures to a return of -ENOSYS, if you do
like it, welcome your Reviewed-by. These two are required by the coming new
time64 syscalls for rv32, because they depends on how we cope with the
unsupported syscalls, returning -ENOSYS is really better than simply fail the
compiling.

Hi, Thomas, If you are happy with them, welcome your Reviewed-by too, thanks.
If the first two are ok, then, I can focus on preparing the left time64
patchsets.

The third one is not that urgent, because some important syscalls are
still missing for rv32. It is added here only for compile test.

Thanks,
Zhangjin

> Hi, Willy
> 
> This is the v3 part2 of support for rv32, differs from the v2 part2 [1],
> we only fix up compile issues in this patchset.
> 
> With the v3 generic part1 [2] and this patchset, we can compile nolibc
> for rv32 now.
> 
> This is based on the idea of suggestions from Arnd [3], instead of
> '#error' on the unsupported syscall on a target platform, a 'return
> -ENOSYS' allow us to compile it at first and then allow we fix up the
> test failures reported by nolibc-test one by one.
> 
> The first two patches fix up all of the compile failures with '-ENOSYS'
> (and '#ifdef' if required):
> 
>   tools/nolibc: fix up #error compile failures with -ENOSYS
>   tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS
> 
> The last one enables rv32 compile support:
>   
>   selftests/nolibc: riscv: customize makefile for rv32
> 
> The above compile support patch here is only for test currently, as
> Thomas suggested, for a full rv32 support, it should wait for the left
> parts.
> 
> Welcome your feedbacks, will wait for enough discussion on this patchset
> and then send the left parts one by one to fix up the test failures
> about waitid, llseek and time64 syscalls: ppoll_time64, clock_gettime64,
> pselect6_time64.
> 
> So, I do recommend to apply this patchset, it allows us to send the left
> parts independently, otherwise, all of them should be sent out for
> review together. with this patchset, the rv32 users may be able to use
> nolibc although some syscalls still missing :-)
> 
> Or at least we apply the first two, so, I can manually cherry-pick the
> compile support patch to do my local test, and the other platform
> developer may also benefit from them.
> 
> I'm cleaning up the left parts, but still require some time, I plan to
> split them to such parts:
> 
>   * part3: waitid, prepared, will send out later
>   * part4: llseek, prepared, will send out later
>   * part5: time64 syscalls, ppoll_time64 ok, will finish them next week
>            (It is a little hard to split them)
> 
> Best regards,
> Zhangjin
> ---
> 
> [1]: https://lore.kernel.org/linux-riscv/cover.1685387484.git.falcon@tinylab.org/T/#t
> [2]: https://lore.kernel.org/linux-riscv/cover.1685777982.git.falcon@tinylab.org/T/#t
> [3]: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
> 
> Zhangjin Wu (3):
>   tools/nolibc: fix up #error compile failures with -ENOSYS
>   tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS
>   selftests/nolibc: riscv: customize makefile for rv32
> 
>  tools/include/nolibc/sys.h              | 38 ++++++++++++++++---------
>  tools/testing/selftests/nolibc/Makefile | 11 +++++--
>  2 files changed, 34 insertions(+), 15 deletions(-)

[PATCH v3 0/3] nolibc: add part2 of support for rv32

[Zhangjin Wu]

Hi, Arnd

Because this patchset is a 'big' change derived from the idea of suggestion
from you [3], I do very welcome your feedback about this change, just like
Thomas suggested, this requires more discussion before Willy plan to determine
merge it or not.

The first two convert all compile failures to a return of -ENOSYS, if you do
like it, welcome your Reviewed-by. These two are required by the coming new
time64 syscalls for rv32, because they depends on how we cope with the
unsupported syscalls, returning -ENOSYS is really better than simply fail the
compiling.

Hi, Thomas, If you are happy with them, welcome your Reviewed-by too, thanks.
If the first two are ok, then, I can focus on preparing the left time64
patchsets.

The third one is not that urgent, because some important syscalls are
still missing for rv32. It is added here only for compile test.

Thanks,
Zhangjin

> Hi, Willy
> 
> This is the v3 part2 of support for rv32, differs from the v2 part2 [1],
> we only fix up compile issues in this patchset.
> 
> With the v3 generic part1 [2] and this patchset, we can compile nolibc
> for rv32 now.
> 
> This is based on the idea of suggestions from Arnd [3], instead of
> '#error' on the unsupported syscall on a target platform, a 'return
> -ENOSYS' allow us to compile it at first and then allow we fix up the
> test failures reported by nolibc-test one by one.
> 
> The first two patches fix up all of the compile failures with '-ENOSYS'
> (and '#ifdef' if required):
> 
>   tools/nolibc: fix up #error compile failures with -ENOSYS
>   tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS
> 
> The last one enables rv32 compile support:
>   
>   selftests/nolibc: riscv: customize makefile for rv32
> 
> The above compile support patch here is only for test currently, as
> Thomas suggested, for a full rv32 support, it should wait for the left
> parts.
> 
> Welcome your feedbacks, will wait for enough discussion on this patchset
> and then send the left parts one by one to fix up the test failures
> about waitid, llseek and time64 syscalls: ppoll_time64, clock_gettime64,
> pselect6_time64.
> 
> So, I do recommend to apply this patchset, it allows us to send the left
> parts independently, otherwise, all of them should be sent out for
> review together. with this patchset, the rv32 users may be able to use
> nolibc although some syscalls still missing :-)
> 
> Or at least we apply the first two, so, I can manually cherry-pick the
> compile support patch to do my local test, and the other platform
> developer may also benefit from them.
> 
> I'm cleaning up the left parts, but still require some time, I plan to
> split them to such parts:
> 
>   * part3: waitid, prepared, will send out later
>   * part4: llseek, prepared, will send out later
>   * part5: time64 syscalls, ppoll_time64 ok, will finish them next week
>            (It is a little hard to split them)
> 
> Best regards,
> Zhangjin
> ---
> 
> [1]: https://lore.kernel.org/linux-riscv/cover.1685387484.git.falcon@tinylab.org/T/#t
> [2]: https://lore.kernel.org/linux-riscv/cover.1685777982.git.falcon@tinylab.org/T/#t
> [3]: https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
> 
> Zhangjin Wu (3):
>   tools/nolibc: fix up #error compile failures with -ENOSYS
>   tools/nolibc: fix up undeclared syscall macros with #ifdef and -ENOSYS
>   selftests/nolibc: riscv: customize makefile for rv32
> 
>  tools/include/nolibc/sys.h              | 38 ++++++++++++++++---------
>  tools/testing/selftests/nolibc/Makefile | 11 +++++--
>  2 files changed, 34 insertions(+), 15 deletions(-)

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 0/3] nolibc: add part2 of support for rv32

[Willy Tarreau]

Hi Zhangjin,

On Tue, Jun 06, 2023 at 12:25:35PM +0800, Zhangjin Wu wrote:
> The first two convert all compile failures to a return of -ENOSYS, if you do
> like it, welcome your Reviewed-by. These two are required by the coming new
> time64 syscalls for rv32, because they depends on how we cope with the
> unsupported syscalls, returning -ENOSYS is really better than simply fail the
> compiling.

I had a look now and I can sya that I like this. Initially the supported
syscalls were so restricted that it was not even imaginable to accept to
build without any of them, but now that we're completing the list, some
of them are less critical and I don't see why we'd fail to build just
because one is missing. So yeah, a big +1 for -ENOSYS.

> The third one is not that urgent, because some important syscalls are
> still missing for rv32. It is added here only for compile test.

I personally have no opinion on this one. I can't judge whether it will
make things easier or more complicated at this point. It seems to me
that for now it's just avoiding one extra line at the expense of some
$(if) on several lines. Maybe it could help add more such archs, or
maybe it can make them more complicated to debug, I don't know. I'm
interested in others' opinions as well.

Thanks,
Willy

Re: [PATCH v3 0/3] nolibc: add part2 of support for rv32

[Willy Tarreau]

Hi Zhangjin,

On Tue, Jun 06, 2023 at 12:25:35PM +0800, Zhangjin Wu wrote:
> The first two convert all compile failures to a return of -ENOSYS, if you do
> like it, welcome your Reviewed-by. These two are required by the coming new
> time64 syscalls for rv32, because they depends on how we cope with the
> unsupported syscalls, returning -ENOSYS is really better than simply fail the
> compiling.

I had a look now and I can sya that I like this. Initially the supported
syscalls were so restricted that it was not even imaginable to accept to
build without any of them, but now that we're completing the list, some
of them are less critical and I don't see why we'd fail to build just
because one is missing. So yeah, a big +1 for -ENOSYS.

> The third one is not that urgent, because some important syscalls are
> still missing for rv32. It is added here only for compile test.

I personally have no opinion on this one. I can't judge whether it will
make things easier or more complicated at this point. It seems to me
that for now it's just avoiding one extra line at the expense of some
$(if) on several lines. Maybe it could help add more such archs, or
maybe it can make them more complicated to debug, I don't know. I'm
interested in others' opinions as well.

Thanks,
Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 0/3] nolibc: add part2 of support for rv32

[Zhangjin Wu]

Willy, Thomas, Arnd

> Hi Zhangjin,
> 
> On Tue, Jun 06, 2023 at 12:25:35PM +0800, Zhangjin Wu wrote:
> > The first two convert all compile failures to a return of -ENOSYS, if you do
> > like it, welcome your Reviewed-by. These two are required by the coming new
> > time64 syscalls for rv32, because they depends on how we cope with the
> > unsupported syscalls, returning -ENOSYS is really better than simply fail the
> > compiling.
> 
> I had a look now and I can sya that I like this. Initially the supported
> syscalls were so restricted that it was not even imaginable to accept to
> build without any of them, but now that we're completing the list, some
> of them are less critical and I don't see why we'd fail to build just
> because one is missing. So yeah, a big +1 for -ENOSYS.
>

Cool, I will prepare the new patchsets on them, welcome your new branch
with both of them, of course, still weclome Reviewed-by from Arnd and Thomas.

> > The third one is not that urgent, because some important syscalls are
> > still missing for rv32. It is added here only for compile test.
> 
> I personally have no opinion on this one. I can't judge whether it will
> make things easier or more complicated at this point. It seems to me
> that for now it's just avoiding one extra line at the expense of some
> $(if) on several lines. Maybe it could help add more such archs, or
> maybe it can make them more complicated to debug, I don't know. I'm
> interested in others' opinions as well.

As I explained why we did it in current way in this reply [1], Thomas had no
more questions on it, so I think Thomas was happy with it now and I got his
only left suggestion is that may be this patch should be applied after the
missing 64bit syscalls being added for there are several important test
failures currently, for me, it is ok before or after.

Thomas, welcome your Reviewed-by on the makefile patch itself If you are really
happy with it now, thanks very much ;-)

Willy, I will send the v2 syscalls helpers (also required by the coming 64bit
syscalls) and some other patches (mainly for test with faster kernel build)
about selftests/nolibc later, because we have not enough time for v6.5 test,
so, I suggest we can create new branch for v6.6 and my new patchsets will be
for v6.6.

Best regards,
Zhangjin

---

[1]: https://lore.kernel.org/linux-riscv/20230526092029.149351-1-falcon@tinylab.org/

> 
> Thanks,
> Willy

Re: [PATCH v3 0/3] nolibc: add part2 of support for rv32

[Zhangjin Wu]

Willy, Thomas, Arnd

> Hi Zhangjin,
> 
> On Tue, Jun 06, 2023 at 12:25:35PM +0800, Zhangjin Wu wrote:
> > The first two convert all compile failures to a return of -ENOSYS, if you do
> > like it, welcome your Reviewed-by. These two are required by the coming new
> > time64 syscalls for rv32, because they depends on how we cope with the
> > unsupported syscalls, returning -ENOSYS is really better than simply fail the
> > compiling.
> 
> I had a look now and I can sya that I like this. Initially the supported
> syscalls were so restricted that it was not even imaginable to accept to
> build without any of them, but now that we're completing the list, some
> of them are less critical and I don't see why we'd fail to build just
> because one is missing. So yeah, a big +1 for -ENOSYS.
>

Cool, I will prepare the new patchsets on them, welcome your new branch
with both of them, of course, still weclome Reviewed-by from Arnd and Thomas.

> > The third one is not that urgent, because some important syscalls are
> > still missing for rv32. It is added here only for compile test.
> 
> I personally have no opinion on this one. I can't judge whether it will
> make things easier or more complicated at this point. It seems to me
> that for now it's just avoiding one extra line at the expense of some
> $(if) on several lines. Maybe it could help add more such archs, or
> maybe it can make them more complicated to debug, I don't know. I'm
> interested in others' opinions as well.

As I explained why we did it in current way in this reply [1], Thomas had no
more questions on it, so I think Thomas was happy with it now and I got his
only left suggestion is that may be this patch should be applied after the
missing 64bit syscalls being added for there are several important test
failures currently, for me, it is ok before or after.

Thomas, welcome your Reviewed-by on the makefile patch itself If you are really
happy with it now, thanks very much ;-)

Willy, I will send the v2 syscalls helpers (also required by the coming 64bit
syscalls) and some other patches (mainly for test with faster kernel build)
about selftests/nolibc later, because we have not enough time for v6.5 test,
so, I suggest we can create new branch for v6.6 and my new patchsets will be
for v6.6.

Best regards,
Zhangjin

---

[1]: https://lore.kernel.org/linux-riscv/20230526092029.149351-1-falcon@tinylab.org/

> 
> Thanks,
> Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 0/3] nolibc: add part2 of support for rv32

[Willy Tarreau]

On Tue, Jun 06, 2023 at 02:34:21PM +0800, Zhangjin Wu wrote:
> Willy, Thomas, Arnd
> 
> > Hi Zhangjin,
> > 
> > On Tue, Jun 06, 2023 at 12:25:35PM +0800, Zhangjin Wu wrote:
> > > The first two convert all compile failures to a return of -ENOSYS, if you do
> > > like it, welcome your Reviewed-by. These two are required by the coming new
> > > time64 syscalls for rv32, because they depends on how we cope with the
> > > unsupported syscalls, returning -ENOSYS is really better than simply fail the
> > > compiling.
> > 
> > I had a look now and I can sya that I like this. Initially the supported
> > syscalls were so restricted that it was not even imaginable to accept to
> > build without any of them, but now that we're completing the list, some
> > of them are less critical and I don't see why we'd fail to build just
> > because one is missing. So yeah, a big +1 for -ENOSYS.
> >
> 
> Cool, I will prepare the new patchsets on them, welcome your new branch
> with both of them, of course, still weclome Reviewed-by from Arnd and Thomas.

I don't even think a new branch is needed, I can take them as-is it seems.

> > > The third one is not that urgent, because some important syscalls are
> > > still missing for rv32. It is added here only for compile test.
> > 
> > I personally have no opinion on this one. I can't judge whether it will
> > make things easier or more complicated at this point. It seems to me
> > that for now it's just avoiding one extra line at the expense of some
> > $(if) on several lines. Maybe it could help add more such archs, or
> > maybe it can make them more complicated to debug, I don't know. I'm
> > interested in others' opinions as well.
> 
> As I explained why we did it in current way in this reply [1], Thomas had no
> more questions on it, so I think Thomas was happy with it now and I got his
> only left suggestion is that may be this patch should be applied after the
> missing 64bit syscalls being added for there are several important test
> failures currently, for me, it is ok before or after.
> 
> Thomas, welcome your Reviewed-by on the makefile patch itself If you are really
> happy with it now, thanks very much ;-)
> 
> Willy, I will send the v2 syscalls helpers (also required by the coming 64bit
> syscalls) and some other patches (mainly for test with faster kernel build)
> about selftests/nolibc later, because we have not enough time for v6.5 test,
> so, I suggest we can create new branch for v6.6 and my new patchsets will be
> for v6.6.

Agreed, thank you!
Willy

Re: [PATCH v3 0/3] nolibc: add part2 of support for rv32

[Willy Tarreau]

On Tue, Jun 06, 2023 at 02:34:21PM +0800, Zhangjin Wu wrote:
> Willy, Thomas, Arnd
> 
> > Hi Zhangjin,
> > 
> > On Tue, Jun 06, 2023 at 12:25:35PM +0800, Zhangjin Wu wrote:
> > > The first two convert all compile failures to a return of -ENOSYS, if you do
> > > like it, welcome your Reviewed-by. These two are required by the coming new
> > > time64 syscalls for rv32, because they depends on how we cope with the
> > > unsupported syscalls, returning -ENOSYS is really better than simply fail the
> > > compiling.
> > 
> > I had a look now and I can sya that I like this. Initially the supported
> > syscalls were so restricted that it was not even imaginable to accept to
> > build without any of them, but now that we're completing the list, some
> > of them are less critical and I don't see why we'd fail to build just
> > because one is missing. So yeah, a big +1 for -ENOSYS.
> >
> 
> Cool, I will prepare the new patchsets on them, welcome your new branch
> with both of them, of course, still weclome Reviewed-by from Arnd and Thomas.

I don't even think a new branch is needed, I can take them as-is it seems.

> > > The third one is not that urgent, because some important syscalls are
> > > still missing for rv32. It is added here only for compile test.
> > 
> > I personally have no opinion on this one. I can't judge whether it will
> > make things easier or more complicated at this point. It seems to me
> > that for now it's just avoiding one extra line at the expense of some
> > $(if) on several lines. Maybe it could help add more such archs, or
> > maybe it can make them more complicated to debug, I don't know. I'm
> > interested in others' opinions as well.
> 
> As I explained why we did it in current way in this reply [1], Thomas had no
> more questions on it, so I think Thomas was happy with it now and I got his
> only left suggestion is that may be this patch should be applied after the
> missing 64bit syscalls being added for there are several important test
> failures currently, for me, it is ok before or after.
> 
> Thomas, welcome your Reviewed-by on the makefile patch itself If you are really
> happy with it now, thanks very much ;-)
> 
> Willy, I will send the v2 syscalls helpers (also required by the coming 64bit
> syscalls) and some other patches (mainly for test with faster kernel build)
> about selftests/nolibc later, because we have not enough time for v6.5 test,
> so, I suggest we can create new branch for v6.6 and my new patchsets will be
> for v6.6.

Agreed, thank you!
Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Arnd Bergmann]

On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Link: 
> https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/include/nolibc/sys.h | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> index 856249a11890..78c86f124335 100644
> --- a/tools/include/nolibc/sys.h
> +++ b/tools/include/nolibc/sys.h
> @@ -124,7 +124,7 @@ int sys_chmod(const char *path, mode_t mode)
>  #elif defined(__NR_chmod)
>  	return my_syscall2(__NR_chmod, path, mode);
>  #else
> -#error Neither __NR_fchmodat nor __NR_chmod defined, cannot implement 
> sys_chmod()
> +	return -ENOSYS;
>  #endif
>  }

I think the most logical would be to have each syscall (chmod,
fchmodat, ...) have its own function that returns -ENOSYS if
that is not defined, and have the logic that decides which one
to use as a separate function.

This patch is a step in that direction though, so I think that's
totally fine.

     Arnd

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Arnd Bergmann]

On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Link: 
> https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/include/nolibc/sys.h | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> index 856249a11890..78c86f124335 100644
> --- a/tools/include/nolibc/sys.h
> +++ b/tools/include/nolibc/sys.h
> @@ -124,7 +124,7 @@ int sys_chmod(const char *path, mode_t mode)
>  #elif defined(__NR_chmod)
>  	return my_syscall2(__NR_chmod, path, mode);
>  #else
> -#error Neither __NR_fchmodat nor __NR_chmod defined, cannot implement 
> sys_chmod()
> +	return -ENOSYS;
>  #endif
>  }

I think the most logical would be to have each syscall (chmod,
fchmodat, ...) have its own function that returns -ENOSYS if
that is not defined, and have the logic that decides which one
to use as a separate function.

This patch is a step in that direction though, so I think that's
totally fine.

     Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Arnd Bergmann]

On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
> Both riscv64 and riscv32 have:
>
> * the same ARCH value, it is riscv
> * the same arch/riscv source code tree
>
> The only differences are:
>
> * riscv64 uses defconfig, riscv32 uses rv32_defconfig
> * riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32
> * riscv32 has different compiler options (-march= and -mabi=)
>
> So, riscv32 can share most of the settings with riscv64, there is no
> need to add it as a whole new architecture but just need a flag to
> record and reflect the difference.
>
> The 32bit mips and loongarch may be able to use the same method, so,
> let's use a meaningful flag: CONFIG_32BIT. If required in the future,
> this flag can also be automatically loaded from
> include/config/auto.conf.

If we use a CONFIG_* symbol, I think it should be the other way
round, for consistency with the kernel, which uses CONFIG_64BIT
on all architectures, but only uses CONFIG_32BIT on mips, loongarch
powerpc and riscv.


>  # kernel image names by architecture
>  IMAGE_i386       = arch/x86/boot/bzImage
>  IMAGE_x86_64     = arch/x86/boot/bzImage
> @@ -34,7 +40,7 @@ DEFCONFIG_x86        = defconfig
>  DEFCONFIG_arm64      = defconfig
>  DEFCONFIG_arm        = multi_v7_defconfig
>  DEFCONFIG_mips       = malta_defconfig
> -DEFCONFIG_riscv      = defconfig
> +DEFCONFIG_riscv      = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig)
>  DEFCONFIG_s390       = defconfig
>  DEFCONFIG_loongarch  = defconfig
>  DEFCONFIG            = $(DEFCONFIG_$(ARCH))

This feels slightly odd, as we otherwise have a fixed defconfig
per target, so doing

DEFCONFIG_riscv      = defconfig
DEFCONFIG_riscv64    = defconfig
DEFCONFIG_riscv32    = rv32_defconfig

would seem more consistent with how x86 is handled, and would
probably be more easily extensible if we want to also make
this work with other sub-targets like mipseb, armv5 or ppc32
in the future.

     Arnd

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Arnd Bergmann]

On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
> Both riscv64 and riscv32 have:
>
> * the same ARCH value, it is riscv
> * the same arch/riscv source code tree
>
> The only differences are:
>
> * riscv64 uses defconfig, riscv32 uses rv32_defconfig
> * riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32
> * riscv32 has different compiler options (-march= and -mabi=)
>
> So, riscv32 can share most of the settings with riscv64, there is no
> need to add it as a whole new architecture but just need a flag to
> record and reflect the difference.
>
> The 32bit mips and loongarch may be able to use the same method, so,
> let's use a meaningful flag: CONFIG_32BIT. If required in the future,
> this flag can also be automatically loaded from
> include/config/auto.conf.

If we use a CONFIG_* symbol, I think it should be the other way
round, for consistency with the kernel, which uses CONFIG_64BIT
on all architectures, but only uses CONFIG_32BIT on mips, loongarch
powerpc and riscv.


>  # kernel image names by architecture
>  IMAGE_i386       = arch/x86/boot/bzImage
>  IMAGE_x86_64     = arch/x86/boot/bzImage
> @@ -34,7 +40,7 @@ DEFCONFIG_x86        = defconfig
>  DEFCONFIG_arm64      = defconfig
>  DEFCONFIG_arm        = multi_v7_defconfig
>  DEFCONFIG_mips       = malta_defconfig
> -DEFCONFIG_riscv      = defconfig
> +DEFCONFIG_riscv      = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig)
>  DEFCONFIG_s390       = defconfig
>  DEFCONFIG_loongarch  = defconfig
>  DEFCONFIG            = $(DEFCONFIG_$(ARCH))

This feels slightly odd, as we otherwise have a fixed defconfig
per target, so doing

DEFCONFIG_riscv      = defconfig
DEFCONFIG_riscv64    = defconfig
DEFCONFIG_riscv32    = rv32_defconfig

would seem more consistent with how x86 is handled, and would
probably be more easily extensible if we want to also make
this work with other sub-targets like mipseb, armv5 or ppc32
in the future.

     Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

Arnd, Thomas, Willy

> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
> > Both riscv64 and riscv32 have:
> >
> > * the same ARCH value, it is riscv
> > * the same arch/riscv source code tree
> >
> > The only differences are:
> >
> > * riscv64 uses defconfig, riscv32 uses rv32_defconfig
> > * riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32
> > * riscv32 has different compiler options (-march= and -mabi=)
> >
> > So, riscv32 can share most of the settings with riscv64, there is no
> > need to add it as a whole new architecture but just need a flag to
> > record and reflect the difference.
> >
> > The 32bit mips and loongarch may be able to use the same method, so,
> > let's use a meaningful flag: CONFIG_32BIT. If required in the future,
> > this flag can also be automatically loaded from
> > include/config/auto.conf.
>
> If we use a CONFIG_* symbol, I think it should be the other way
> round, for consistency with the kernel, which uses CONFIG_64BIT
> on all architectures, but only uses CONFIG_32BIT on mips, loongarch
> powerpc and riscv.
>
>
> >  # kernel image names by architecture
> >  IMAGE_i386       = arch/x86/boot/bzImage
> >  IMAGE_x86_64     = arch/x86/boot/bzImage
> > @@ -34,7 +40,7 @@ DEFCONFIG_x86        = defconfig
> >  DEFCONFIG_arm64      = defconfig
> >  DEFCONFIG_arm        = multi_v7_defconfig
> >  DEFCONFIG_mips       = malta_defconfig
> > -DEFCONFIG_riscv      = defconfig
> > +DEFCONFIG_riscv      = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig)
> >  DEFCONFIG_s390       = defconfig
> >  DEFCONFIG_loongarch  = defconfig
> >  DEFCONFIG            = $(DEFCONFIG_$(ARCH))
>
> This feels slightly odd, as we otherwise have a fixed defconfig
> per target, so doing
>
> DEFCONFIG_riscv      = defconfig
> DEFCONFIG_riscv64    = defconfig
> DEFCONFIG_riscv32    = rv32_defconfig
>
> would seem more consistent with how x86 is handled, and would
> probably be more easily extensible if we want to also make
> this work with other sub-targets like mipseb, armv5 or ppc32
> in the future.

As Arnd and Thomas suggested to align with x86, I just tried to find a
solution to avoid mixing the use of _ARCH and ARCH in this Makefile.

Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv),
and the kernel side doesn't accept riscv32 or riscv64 currently, we need to
manually convert them to _ARCH=riscv and pass them to the kernel makefile
like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I
used the '$(if' method currently.

The solution is adding something like x86 in the kernel Makefile:

    diff --git a/Makefile b/Makefile
    index 9d765ebcccf1..a442c893d795 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64)
            SRCARCH := parisc
     endif

    +# Additional ARCH settings for riscv
    +ifeq ($(ARCH),riscv32)
    +        SRCARCH := riscv
    +endif
    +ifeq ($(ARCH),riscv64)
    +        SRCARCH := riscv
    +endif
    +
     export cross_compiling :=
     ifneq ($(SRCARCH),$(SUBARCH))
     cross_compiling := 1

And also, we need to let both of them use the arch-riscv.h in
tools/include/nolibc/Makefile:

    diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
    index 64d67b080744..459eaddb230f 100644
    --- a/tools/include/nolibc/Makefile
    +++ b/tools/include/nolibc/Makefile
    @@ -24,6 +24,7 @@ Q=@
     endif

     nolibc_arch := $(patsubst arm64,aarch64,$(ARCH))
    +nolibc_arch := $(patsubst riscv%,riscv,$(ARCH))
     arch_file := arch-$(nolibc_arch).h
     all_files := \
                    compiler.h \

So, the left parts can be added like x86 too:

    diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
    index 4a3a105e1fdf..1b2247a6365d 100644
    --- a/tools/testing/selftests/nolibc/Makefile
    +++ b/tools/testing/selftests/nolibc/Makefile
    @@ -21,6 +21,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
     IMAGE_arm64      = arch/arm64/boot/Image
     IMAGE_arm        = arch/arm/boot/zImage
     IMAGE_mips       = vmlinuz
    +IMAGE_riscv32    = arch/riscv/boot/Image
    +IMAGE_riscv64    = arch/riscv/boot/Image
     IMAGE_riscv      = arch/riscv/boot/Image
     IMAGE_s390       = arch/s390/boot/bzImage
     IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
    @@ -34,6 +36,8 @@ DEFCONFIG_x86        = defconfig
     DEFCONFIG_arm64      = defconfig
     DEFCONFIG_arm        = multi_v7_defconfig
     DEFCONFIG_mips       = malta_defconfig
    +DEFCONFIG_riscv32    = rv32_defconfig
    +DEFCONFIG_riscv64    = defconfig
     DEFCONFIG_riscv      = defconfig
     DEFCONFIG_s390       = defconfig
     DEFCONFIG_loongarch  = defconfig
    @@ -49,6 +53,8 @@ QEMU_ARCH_x86        = x86_64
     QEMU_ARCH_arm64      = aarch64
     QEMU_ARCH_arm        = arm
     QEMU_ARCH_mips       = mipsel  # works with malta_defconfig
    +QEMU_ARCH_riscv32    = riscv32
    +QEMU_ARCH_riscv64    = riscv64
     QEMU_ARCH_riscv      = riscv64
     QEMU_ARCH_s390       = s390x
     QEMU_ARCH_loongarch  = loongarch64
    @@ -61,6 +67,8 @@ QEMU_ARGS_x86        = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(
     QEMU_ARGS_arm64      = -M virt -cpu cortex-a53 -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_arm        = -M virt -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_mips       = -M malta -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv32    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    @@ -76,6 +84,7 @@ else
     Q=@
     endif

    +CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
     CFLAGS_s390 = -m64
     CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
     CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \

And just found the 'm' extension (-march=rv32im) is necessary to avoid linking
libgcc, especially, my local compiler has no rv32 libgcc (target emulation
`elf64-littleriscv' does not match `elf32-littleriscv'), so, added the 'm'
extension back, this is supported by the generic riscv chips. The atomic and
float extensions (include single and double) are not added, keep it as minimal
currently.

Just tested rv32 and rv64 on 20230606-nolibc-rv32+stkp7a with a trivial fixup
of rcu (the problematic code is not in v6.4-rc4, so, this can be ignored, see
below, what about rebase the new branch on a newer rc?), it works as expected.

    diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
    index ce02bb09651b..72bd8fe0cad6 100644
    --- a/kernel/rcu/tasks.h
    +++ b/kernel/rcu/tasks.h
    @@ -1934,11 +1934,13 @@ void show_rcu_tasks_gp_kthreads(void)
     }
     #endif /* #ifndef CONFIG_TINY_RCU */

    +#ifdef CONFIG_TASKS_RCU
     struct task_struct *get_rcu_tasks_gp_kthread(void)
     {
            return rcu_tasks.kthread_ptr;
     }
     EXPORT_SYMBOL_GPL(get_rcu_tasks_gp_kthread);
    +#endif

     #ifdef CONFIG_PROVE_RCU
     struct rcu_tasks_test_desc {

The steps I tested:

    $ history | grep make
     // riscv32 test
     1416  make defconfig ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
     1430  make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin"
     1438  make run-user ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
           113 test(s) passed, 3 skipped, 22 failed. See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

     // riscv64 test (old options)
     1432  make defconfig ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu-
     1433  make run ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu-
     1443  make run-user ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu-
           135 test(s) passed, 3 skipped, 0 failed. See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

     // riscv64 test (new options)
     1441  make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
     1439  make run-user ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-

Thanks Arnd and Thomas for your persistence, If you agree, will send this as a
new revision.

@Willy, I plan to send v4 immediately, since the first two has not been
merged yet, so, I will send them together as v4.

Best regards,
Zhangjin

>
>      Arnd

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

Arnd, Thomas, Willy

> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
> > Both riscv64 and riscv32 have:
> >
> > * the same ARCH value, it is riscv
> > * the same arch/riscv source code tree
> >
> > The only differences are:
> >
> > * riscv64 uses defconfig, riscv32 uses rv32_defconfig
> > * riscv64 uses qemu-system-riscv64, riscv32 uses qemu-system-riscv32
> > * riscv32 has different compiler options (-march= and -mabi=)
> >
> > So, riscv32 can share most of the settings with riscv64, there is no
> > need to add it as a whole new architecture but just need a flag to
> > record and reflect the difference.
> >
> > The 32bit mips and loongarch may be able to use the same method, so,
> > let's use a meaningful flag: CONFIG_32BIT. If required in the future,
> > this flag can also be automatically loaded from
> > include/config/auto.conf.
>
> If we use a CONFIG_* symbol, I think it should be the other way
> round, for consistency with the kernel, which uses CONFIG_64BIT
> on all architectures, but only uses CONFIG_32BIT on mips, loongarch
> powerpc and riscv.
>
>
> >  # kernel image names by architecture
> >  IMAGE_i386       = arch/x86/boot/bzImage
> >  IMAGE_x86_64     = arch/x86/boot/bzImage
> > @@ -34,7 +40,7 @@ DEFCONFIG_x86        = defconfig
> >  DEFCONFIG_arm64      = defconfig
> >  DEFCONFIG_arm        = multi_v7_defconfig
> >  DEFCONFIG_mips       = malta_defconfig
> > -DEFCONFIG_riscv      = defconfig
> > +DEFCONFIG_riscv      = $(if $(CONFIG_32BIT),rv32_defconfig,defconfig)
> >  DEFCONFIG_s390       = defconfig
> >  DEFCONFIG_loongarch  = defconfig
> >  DEFCONFIG            = $(DEFCONFIG_$(ARCH))
>
> This feels slightly odd, as we otherwise have a fixed defconfig
> per target, so doing
>
> DEFCONFIG_riscv      = defconfig
> DEFCONFIG_riscv64    = defconfig
> DEFCONFIG_riscv32    = rv32_defconfig
>
> would seem more consistent with how x86 is handled, and would
> probably be more easily extensible if we want to also make
> this work with other sub-targets like mipseb, armv5 or ppc32
> in the future.

As Arnd and Thomas suggested to align with x86, I just tried to find a
solution to avoid mixing the use of _ARCH and ARCH in this Makefile.

Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv),
and the kernel side doesn't accept riscv32 or riscv64 currently, we need to
manually convert them to _ARCH=riscv and pass them to the kernel makefile
like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I
used the '$(if' method currently.

The solution is adding something like x86 in the kernel Makefile:

    diff --git a/Makefile b/Makefile
    index 9d765ebcccf1..a442c893d795 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64)
            SRCARCH := parisc
     endif

    +# Additional ARCH settings for riscv
    +ifeq ($(ARCH),riscv32)
    +        SRCARCH := riscv
    +endif
    +ifeq ($(ARCH),riscv64)
    +        SRCARCH := riscv
    +endif
    +
     export cross_compiling :=
     ifneq ($(SRCARCH),$(SUBARCH))
     cross_compiling := 1

And also, we need to let both of them use the arch-riscv.h in
tools/include/nolibc/Makefile:

    diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
    index 64d67b080744..459eaddb230f 100644
    --- a/tools/include/nolibc/Makefile
    +++ b/tools/include/nolibc/Makefile
    @@ -24,6 +24,7 @@ Q=@
     endif

     nolibc_arch := $(patsubst arm64,aarch64,$(ARCH))
    +nolibc_arch := $(patsubst riscv%,riscv,$(ARCH))
     arch_file := arch-$(nolibc_arch).h
     all_files := \
                    compiler.h \

So, the left parts can be added like x86 too:

    diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
    index 4a3a105e1fdf..1b2247a6365d 100644
    --- a/tools/testing/selftests/nolibc/Makefile
    +++ b/tools/testing/selftests/nolibc/Makefile
    @@ -21,6 +21,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
     IMAGE_arm64      = arch/arm64/boot/Image
     IMAGE_arm        = arch/arm/boot/zImage
     IMAGE_mips       = vmlinuz
    +IMAGE_riscv32    = arch/riscv/boot/Image
    +IMAGE_riscv64    = arch/riscv/boot/Image
     IMAGE_riscv      = arch/riscv/boot/Image
     IMAGE_s390       = arch/s390/boot/bzImage
     IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
    @@ -34,6 +36,8 @@ DEFCONFIG_x86        = defconfig
     DEFCONFIG_arm64      = defconfig
     DEFCONFIG_arm        = multi_v7_defconfig
     DEFCONFIG_mips       = malta_defconfig
    +DEFCONFIG_riscv32    = rv32_defconfig
    +DEFCONFIG_riscv64    = defconfig
     DEFCONFIG_riscv      = defconfig
     DEFCONFIG_s390       = defconfig
     DEFCONFIG_loongarch  = defconfig
    @@ -49,6 +53,8 @@ QEMU_ARCH_x86        = x86_64
     QEMU_ARCH_arm64      = aarch64
     QEMU_ARCH_arm        = arm
     QEMU_ARCH_mips       = mipsel  # works with malta_defconfig
    +QEMU_ARCH_riscv32    = riscv32
    +QEMU_ARCH_riscv64    = riscv64
     QEMU_ARCH_riscv      = riscv64
     QEMU_ARCH_s390       = s390x
     QEMU_ARCH_loongarch  = loongarch64
    @@ -61,6 +67,8 @@ QEMU_ARGS_x86        = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(
     QEMU_ARGS_arm64      = -M virt -cpu cortex-a53 -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_arm        = -M virt -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_mips       = -M malta -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv32    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    @@ -76,6 +84,7 @@ else
     Q=@
     endif

    +CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
     CFLAGS_s390 = -m64
     CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
     CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \

And just found the 'm' extension (-march=rv32im) is necessary to avoid linking
libgcc, especially, my local compiler has no rv32 libgcc (target emulation
`elf64-littleriscv' does not match `elf32-littleriscv'), so, added the 'm'
extension back, this is supported by the generic riscv chips. The atomic and
float extensions (include single and double) are not added, keep it as minimal
currently.

Just tested rv32 and rv64 on 20230606-nolibc-rv32+stkp7a with a trivial fixup
of rcu (the problematic code is not in v6.4-rc4, so, this can be ignored, see
below, what about rebase the new branch on a newer rc?), it works as expected.

    diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
    index ce02bb09651b..72bd8fe0cad6 100644
    --- a/kernel/rcu/tasks.h
    +++ b/kernel/rcu/tasks.h
    @@ -1934,11 +1934,13 @@ void show_rcu_tasks_gp_kthreads(void)
     }
     #endif /* #ifndef CONFIG_TINY_RCU */

    +#ifdef CONFIG_TASKS_RCU
     struct task_struct *get_rcu_tasks_gp_kthread(void)
     {
            return rcu_tasks.kthread_ptr;
     }
     EXPORT_SYMBOL_GPL(get_rcu_tasks_gp_kthread);
    +#endif

     #ifdef CONFIG_PROVE_RCU
     struct rcu_tasks_test_desc {

The steps I tested:

    $ history | grep make
     // riscv32 test
     1416  make defconfig ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
     1430  make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin"
     1438  make run-user ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
           113 test(s) passed, 3 skipped, 22 failed. See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

     // riscv64 test (old options)
     1432  make defconfig ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu-
     1433  make run ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu-
     1443  make run-user ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu-
           135 test(s) passed, 3 skipped, 0 failed. See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

     // riscv64 test (new options)
     1441  make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
     1439  make run-user ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-

Thanks Arnd and Thomas for your persistence, If you agree, will send this as a
new revision.

@Willy, I plan to send v4 immediately, since the first two has not been
merged yet, so, I will send them together as v4.

Best regards,
Zhangjin

>
>      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Arnd Bergmann]

On Tue, Jun 6, 2023, at 13:12, Zhangjin Wu wrote:
>> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
>> would seem more consistent with how x86 is handled, and would
>> probably be more easily extensible if we want to also make
>> this work with other sub-targets like mipseb, armv5 or ppc32
>> in the future.
>
> As Arnd and Thomas suggested to align with x86, I just tried to find a
> solution to avoid mixing the use of _ARCH and ARCH in this Makefile.
>
> Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv),
> and the kernel side doesn't accept riscv32 or riscv64 currently, we need to
> manually convert them to _ARCH=riscv and pass them to the kernel makefile
> like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I
> used the '$(if' method currently.
>
> The solution is adding something like x86 in the kernel Makefile:
>
>     diff --git a/Makefile b/Makefile
>     index 9d765ebcccf1..a442c893d795 100644
>     --- a/Makefile
>     +++ b/Makefile
>     @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64)
>             SRCARCH := parisc
>      endif
>
>     +# Additional ARCH settings for riscv
>     +ifeq ($(ARCH),riscv32)
>     +        SRCARCH := riscv
>     +endif
>     +ifeq ($(ARCH),riscv64)
>     +        SRCARCH := riscv
>     +endif
>     +
>      export cross_compiling :=
>      ifneq ($(SRCARCH),$(SUBARCH))
>      cross_compiling := 1

I've never been a big fan of the top-level $(ARCH) setting
in the kernel, is there a reason this has to be the same
as the variable in tools/include/nolibc? If not, I'd just
leave the Linux Makefile unchanged.

For userspace we have a lot more target names than
arch/*/ directories in the kernel, and I don't think
I'd want to enumerate all the possibilities in the
build system globally.
> b/tools/testing/selftests/nolibc/Makefile
>     index 4a3a105e1fdf..1b2247a6365d 100644
>     --- a/tools/testing/selftests/nolibc/Makefile
>     +++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -21,6 +21,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
>      IMAGE_arm64      = arch/arm64/boot/Image
>      IMAGE_arm        = arch/arm/boot/zImage
>      IMAGE_mips       = vmlinuz
>     +IMAGE_riscv32    = arch/riscv/boot/Image
>     +IMAGE_riscv64    = arch/riscv/boot/Image
>      IMAGE_riscv      = arch/riscv/boot/Image
>      IMAGE_s390       = arch/s390/boot/bzImage
>      IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
>     @@ -34,6 +36,8 @@ DEFCONFIG_x86        = defconfig
>      DEFCONFIG_arm64      = defconfig
>      DEFCONFIG_arm        = multi_v7_defconfig
>      DEFCONFIG_mips       = malta_defconfig
>     +DEFCONFIG_riscv32    = rv32_defconfig
>     +DEFCONFIG_riscv64    = defconfig
...

Right, that part looks good to me.

      Arnd

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Arnd Bergmann]

On Tue, Jun 6, 2023, at 13:12, Zhangjin Wu wrote:
>> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
>> would seem more consistent with how x86 is handled, and would
>> probably be more easily extensible if we want to also make
>> this work with other sub-targets like mipseb, armv5 or ppc32
>> in the future.
>
> As Arnd and Thomas suggested to align with x86, I just tried to find a
> solution to avoid mixing the use of _ARCH and ARCH in this Makefile.
>
> Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv),
> and the kernel side doesn't accept riscv32 or riscv64 currently, we need to
> manually convert them to _ARCH=riscv and pass them to the kernel makefile
> like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I
> used the '$(if' method currently.
>
> The solution is adding something like x86 in the kernel Makefile:
>
>     diff --git a/Makefile b/Makefile
>     index 9d765ebcccf1..a442c893d795 100644
>     --- a/Makefile
>     +++ b/Makefile
>     @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64)
>             SRCARCH := parisc
>      endif
>
>     +# Additional ARCH settings for riscv
>     +ifeq ($(ARCH),riscv32)
>     +        SRCARCH := riscv
>     +endif
>     +ifeq ($(ARCH),riscv64)
>     +        SRCARCH := riscv
>     +endif
>     +
>      export cross_compiling :=
>      ifneq ($(SRCARCH),$(SUBARCH))
>      cross_compiling := 1

I've never been a big fan of the top-level $(ARCH) setting
in the kernel, is there a reason this has to be the same
as the variable in tools/include/nolibc? If not, I'd just
leave the Linux Makefile unchanged.

For userspace we have a lot more target names than
arch/*/ directories in the kernel, and I don't think
I'd want to enumerate all the possibilities in the
build system globally.
> b/tools/testing/selftests/nolibc/Makefile
>     index 4a3a105e1fdf..1b2247a6365d 100644
>     --- a/tools/testing/selftests/nolibc/Makefile
>     +++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -21,6 +21,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
>      IMAGE_arm64      = arch/arm64/boot/Image
>      IMAGE_arm        = arch/arm/boot/zImage
>      IMAGE_mips       = vmlinuz
>     +IMAGE_riscv32    = arch/riscv/boot/Image
>     +IMAGE_riscv64    = arch/riscv/boot/Image
>      IMAGE_riscv      = arch/riscv/boot/Image
>      IMAGE_s390       = arch/s390/boot/bzImage
>      IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
>     @@ -34,6 +36,8 @@ DEFCONFIG_x86        = defconfig
>      DEFCONFIG_arm64      = defconfig
>      DEFCONFIG_arm        = multi_v7_defconfig
>      DEFCONFIG_mips       = malta_defconfig
>     +DEFCONFIG_riscv32    = rv32_defconfig
>     +DEFCONFIG_riscv64    = defconfig
...

Right, that part looks good to me.

      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

> On Tue, Jun 6, 2023, at 13:12, Zhangjin Wu wrote:
> >> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
> >> would seem more consistent with how x86 is handled, and would
> >> probably be more easily extensible if we want to also make
> >> this work with other sub-targets like mipseb, armv5 or ppc32
> >> in the future.
> >
> > As Arnd and Thomas suggested to align with x86, I just tried to find a
> > solution to avoid mixing the use of _ARCH and ARCH in this Makefile.
> >
> > Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv),
> > and the kernel side doesn't accept riscv32 or riscv64 currently, we need to
> > manually convert them to _ARCH=riscv and pass them to the kernel makefile
> > like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I
> > used the '$(if' method currently.
> >
> > The solution is adding something like x86 in the kernel Makefile:
> >
> >     diff --git a/Makefile b/Makefile
> >     index 9d765ebcccf1..a442c893d795 100644
> >     --- a/Makefile
> >     +++ b/Makefile
> >     @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64)
> >             SRCARCH := parisc
> >      endif
> >
> >     +# Additional ARCH settings for riscv
> >     +ifeq ($(ARCH),riscv32)
> >     +        SRCARCH := riscv
> >     +endif
> >     +ifeq ($(ARCH),riscv64)
> >     +        SRCARCH := riscv
> >     +endif
> >     +
> >      export cross_compiling :=
> >      ifneq ($(SRCARCH),$(SUBARCH))
> >      cross_compiling := 1
>
> I've never been a big fan of the top-level $(ARCH) setting
> in the kernel, is there a reason this has to be the same
> as the variable in tools/include/nolibc? If not, I'd just
> leave the Linux Makefile unchanged.
>
> For userspace we have a lot more target names than
> arch/*/ directories in the kernel, and I don't think
> I'd want to enumerate all the possibilities in the
> build system globally.

Ok, agree very much, it is the root cause why we used the old method
before, because I don't want to touch the top-level Makefile, here
explains the details again just as did for Thomas and Willy [1] ;-)

Without the top-level makefile change, we must add something in
selftests/nolibc/Makefile like this, because the kernel makefile doesn't
accept something like ARCH=riscv32 and ARCH=riscv64 currently, it only
accepts ARCH=riscv (will paste the code later).

    ifneq ($(findstring riscv,$(ARCH)),)
      _ARCH = riscv
    else
      _ARCH = $(ARCH)
    endif

    ...

    sysroot/$(ARCH)/include:
	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
	$(QUIET_MKDIR)mkdir -p sysroot
	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
	$(Q)mv sysroot/sysroot sysroot/$(ARCH)

    defconfig:
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

    kernel: initramfs
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

The above change really works, but it looks not that good, this is the
mixing use of _ARCH and ARCH I mentioned in last reply.

Otherwise, we will get such error:

    $ make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
      MKDIR   sysroot/riscv64/include
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:763: arch/riscv64/Makefile: No such file or directory
    make[2]: *** No rule to make target 'arch/riscv64/Makefile'.  Stop.
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[1]: *** [Makefile:87: headers_standalone] Error 2
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make: *** [Makefile:129: sysroot/riscv64/include] Error 2
    $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
      MKDIR   sysroot/riscv32/include
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:763: arch/riscv32/Makefile: No such file or directory
    make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[1]: *** [Makefile:87: headers_standalone] Error 2
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make: *** [Makefile:129: sysroot/riscv32/include] Error 2

That's because in top-level Makefile, it doesn't accept ARCH=riscv32 and
ARCH=riscv64, but x86 and sparc and even parisc support such variants,
this allows the ARCH variants share the same arch/<SRCARCH>/ source code
tree, otherwise, they will directly find the arch/<ARCH>/ source code,
then fails.

    top-level Makefile:

    ...
    ARCH            ?= $(SUBARCH)

    # Architecture as present in compile.h
    UTS_MACHINE     := $(ARCH)
    SRCARCH         := $(ARCH)   ---> SRCARCH is assigned as ARCH by default

    # Additional ARCH settings for x86
    ifeq ($(ARCH),i386)
            SRCARCH := x86
    endif
    ifeq ($(ARCH),x86_64)
            SRCARCH := x86
    endif

    # Additional ARCH settings for sparc
    ifeq ($(ARCH),sparc32)
           SRCARCH := sparc
    endif
    ifeq ($(ARCH),sparc64)
           SRCARCH := sparc
    endif

    # Additional ARCH settings for parisc
    ifeq ($(ARCH),parisc64)
           SRCARCH := parisc
    endif

So, to really align with x86, we should let the top-level makefile be
able to get the right SRCARCH for riscv32 and riscv64 too ;-)

I even tried to pass SRCARCH=riscv to the top-level Makefile, but it
failed:

    diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
    index 1b2247a6365d..04067776b569 100644
    --- a/tools/testing/selftests/nolibc/Makefile
    +++ b/tools/testing/selftests/nolibc/Makefile
    @@ -14,6 +14,10 @@ include $(srctree)/scripts/subarch.include
     ARCH = $(SUBARCH)
     endif

    +ifneq ($(findstring riscv,$(ARCH)),)
    +SRCARCH := SRCARCH=riscv
    +endif
    +
     # kernel image names by architecture
     IMAGE_i386       = arch/x86/boot/bzImage
     IMAGE_x86_64     = arch/x86/boot/bzImage
    @@ -126,7 +130,7 @@ sysroot: sysroot/$(ARCH)/include
     sysroot/$(ARCH)/include:
            $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
            $(QUIET_MKDIR)mkdir -p sysroot
    -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
    +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) $(SRCARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
            $(Q)mv sysroot/sysroot sysroot/$(ARCH)

     nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
    @@ -150,10 +154,10 @@ initramfs: nolibc-test
            $(Q)cp nolibc-test initramfs/init

     defconfig:
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

     kernel: initramfs
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH )CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

     # run the tests after building the kernel
     run: kernel

    $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin"
      MKDIR   sysroot/riscv32/include
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:397: srcarch: riscv
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:397: srcarch: riscv
      INSTALL /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/sysroot/sysroot/include
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
      CC      nolibc-test
      MKDIR   initramfs
      INSTALL initramfs/init
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:397: srcarch: riscv32
      SYNC    include/config/auto.conf.cmd
    Makefile:397: srcarch: riscv32
    Makefile:687: arch/riscv32/Makefile: No such file or directory
    make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
    make[1]: *** [Makefile:795: include/config/auto.conf.cmd] Error 2
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable'

So, to keep consistent eventually, perhaps we do need to touch the
top-level Makefile.

Best regards,
Zhangjin

[1]: https://lore.kernel.org/linux-riscv/20230526092029.149351-1-falcon@tinylab.org/

>     Arnd

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

> On Tue, Jun 6, 2023, at 13:12, Zhangjin Wu wrote:
> >> On Sat, Jun 3, 2023, at 11:05, Zhangjin Wu wrote:
> >> would seem more consistent with how x86 is handled, and would
> >> probably be more easily extensible if we want to also make
> >> this work with other sub-targets like mipseb, armv5 or ppc32
> >> in the future.
> >
> > As Arnd and Thomas suggested to align with x86, I just tried to find a
> > solution to avoid mixing the use of _ARCH and ARCH in this Makefile.
> >
> > Since both riscv32 and riscv64 share the same SRCARCH=riscv (arch/riscv),
> > and the kernel side doesn't accept riscv32 or riscv64 currently, we need to
> > manually convert them to _ARCH=riscv and pass them to the kernel makefile
> > like this: ARCH=$(_ARCH), it mixes the use of _ARCH and ARCH, this is why I
> > used the '$(if' method currently.
> >
> > The solution is adding something like x86 in the kernel Makefile:
> >
> >     diff --git a/Makefile b/Makefile
> >     index 9d765ebcccf1..a442c893d795 100644
> >     --- a/Makefile
> >     +++ b/Makefile
> >     @@ -415,6 +415,14 @@ ifeq ($(ARCH),parisc64)
> >             SRCARCH := parisc
> >      endif
> >
> >     +# Additional ARCH settings for riscv
> >     +ifeq ($(ARCH),riscv32)
> >     +        SRCARCH := riscv
> >     +endif
> >     +ifeq ($(ARCH),riscv64)
> >     +        SRCARCH := riscv
> >     +endif
> >     +
> >      export cross_compiling :=
> >      ifneq ($(SRCARCH),$(SUBARCH))
> >      cross_compiling := 1
>
> I've never been a big fan of the top-level $(ARCH) setting
> in the kernel, is there a reason this has to be the same
> as the variable in tools/include/nolibc? If not, I'd just
> leave the Linux Makefile unchanged.
>
> For userspace we have a lot more target names than
> arch/*/ directories in the kernel, and I don't think
> I'd want to enumerate all the possibilities in the
> build system globally.

Ok, agree very much, it is the root cause why we used the old method
before, because I don't want to touch the top-level Makefile, here
explains the details again just as did for Thomas and Willy [1] ;-)

Without the top-level makefile change, we must add something in
selftests/nolibc/Makefile like this, because the kernel makefile doesn't
accept something like ARCH=riscv32 and ARCH=riscv64 currently, it only
accepts ARCH=riscv (will paste the code later).

    ifneq ($(findstring riscv,$(ARCH)),)
      _ARCH = riscv
    else
      _ARCH = $(ARCH)
    endif

    ...

    sysroot/$(ARCH)/include:
	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
	$(QUIET_MKDIR)mkdir -p sysroot
	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
	$(Q)mv sysroot/sysroot sysroot/$(ARCH)

    defconfig:
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

    kernel: initramfs
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

The above change really works, but it looks not that good, this is the
mixing use of _ARCH and ARCH I mentioned in last reply.

Otherwise, we will get such error:

    $ make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
      MKDIR   sysroot/riscv64/include
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:763: arch/riscv64/Makefile: No such file or directory
    make[2]: *** No rule to make target 'arch/riscv64/Makefile'.  Stop.
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[1]: *** [Makefile:87: headers_standalone] Error 2
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make: *** [Makefile:129: sysroot/riscv64/include] Error 2
    $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
      MKDIR   sysroot/riscv32/include
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:763: arch/riscv32/Makefile: No such file or directory
    make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[1]: *** [Makefile:87: headers_standalone] Error 2
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make: *** [Makefile:129: sysroot/riscv32/include] Error 2

That's because in top-level Makefile, it doesn't accept ARCH=riscv32 and
ARCH=riscv64, but x86 and sparc and even parisc support such variants,
this allows the ARCH variants share the same arch/<SRCARCH>/ source code
tree, otherwise, they will directly find the arch/<ARCH>/ source code,
then fails.

    top-level Makefile:

    ...
    ARCH            ?= $(SUBARCH)

    # Architecture as present in compile.h
    UTS_MACHINE     := $(ARCH)
    SRCARCH         := $(ARCH)   ---> SRCARCH is assigned as ARCH by default

    # Additional ARCH settings for x86
    ifeq ($(ARCH),i386)
            SRCARCH := x86
    endif
    ifeq ($(ARCH),x86_64)
            SRCARCH := x86
    endif

    # Additional ARCH settings for sparc
    ifeq ($(ARCH),sparc32)
           SRCARCH := sparc
    endif
    ifeq ($(ARCH),sparc64)
           SRCARCH := sparc
    endif

    # Additional ARCH settings for parisc
    ifeq ($(ARCH),parisc64)
           SRCARCH := parisc
    endif

So, to really align with x86, we should let the top-level makefile be
able to get the right SRCARCH for riscv32 and riscv64 too ;-)

I even tried to pass SRCARCH=riscv to the top-level Makefile, but it
failed:

    diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
    index 1b2247a6365d..04067776b569 100644
    --- a/tools/testing/selftests/nolibc/Makefile
    +++ b/tools/testing/selftests/nolibc/Makefile
    @@ -14,6 +14,10 @@ include $(srctree)/scripts/subarch.include
     ARCH = $(SUBARCH)
     endif

    +ifneq ($(findstring riscv,$(ARCH)),)
    +SRCARCH := SRCARCH=riscv
    +endif
    +
     # kernel image names by architecture
     IMAGE_i386       = arch/x86/boot/bzImage
     IMAGE_x86_64     = arch/x86/boot/bzImage
    @@ -126,7 +130,7 @@ sysroot: sysroot/$(ARCH)/include
     sysroot/$(ARCH)/include:
            $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
            $(QUIET_MKDIR)mkdir -p sysroot
    -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
    +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) $(SRCARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
            $(Q)mv sysroot/sysroot sysroot/$(ARCH)

     nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
    @@ -150,10 +154,10 @@ initramfs: nolibc-test
            $(Q)cp nolibc-test initramfs/init

     defconfig:
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

     kernel: initramfs
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH )CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

     # run the tests after building the kernel
     run: kernel

    $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin"
      MKDIR   sysroot/riscv32/include
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:397: srcarch: riscv
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:397: srcarch: riscv
      INSTALL /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/sysroot/sysroot/include
    make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
      CC      nolibc-test
      MKDIR   initramfs
      INSTALL initramfs/init
    make[1]: Entering directory '/labs/linux-lab/src/linux-stable'
    Makefile:397: srcarch: riscv32
      SYNC    include/config/auto.conf.cmd
    Makefile:397: srcarch: riscv32
    Makefile:687: arch/riscv32/Makefile: No such file or directory
    make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
    make[1]: *** [Makefile:795: include/config/auto.conf.cmd] Error 2
    make[1]: Leaving directory '/labs/linux-lab/src/linux-stable'

So, to keep consistent eventually, perhaps we do need to touch the
top-level Makefile.

Best regards,
Zhangjin

[1]: https://lore.kernel.org/linux-riscv/20230526092029.149351-1-falcon@tinylab.org/

>     Arnd


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

Arnd, Thomas, Willy

> > >     +# Additional ARCH settings for riscv
> > >     +ifeq ($(ARCH),riscv32)
> > >     +        SRCARCH := riscv
> > >     +endif
> > >     +ifeq ($(ARCH),riscv64)
> > >     +        SRCARCH := riscv
> > >     +endif
> > >     +
> > >      export cross_compiling :=
> > >      ifneq ($(SRCARCH),$(SUBARCH))
> > >      cross_compiling := 1
> >
> > I've never been a big fan of the top-level $(ARCH) setting
> > in the kernel, is there a reason this has to be the same
> > as the variable in tools/include/nolibc? If not, I'd just
> > leave the Linux Makefile unchanged.
> >
> > For userspace we have a lot more target names than
> > arch/*/ directories in the kernel, and I don't think
> > I'd want to enumerate all the possibilities in the
> > build system globally.
>

Good news, I did find a better solution without touching the top-level
Makefile, that is overriding the ARCH to 'riscv' just before the targets
and after we got the necessary settings with the original ARCH=riscv32
or ARCH=riscv64, but it requires to convert the '=' assignments to ':=',
which is not that hard to do and it is more acceptable, just verified it
and it worked well.

    ...

     LDFLAGS := -s

    +# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy
    +ifneq ($(findstring riscv,$(ARCH)),)
    +override ARCH := riscv
    +endif
    +
     help:
            @echo "Supported targets under selftests/nolibc:"
            @echo "  all          call the \"run\" target below"

This change is not that big, and the left changes can keep consistent with the
other platforms. but I still need to add a standalone patch to convert the '='
to ':=' to avoid the before setting using our new overridded ARCH.

    ++ b/tools/testing/selftests/nolibc/Makefile
    @@ -26,7 +26,7 @@ IMAGE_riscv64    = arch/riscv/boot/Image
     IMAGE_riscv      = arch/riscv/boot/Image
     IMAGE_s390       = arch/s390/boot/bzImage
     IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
    -IMAGE            = $(IMAGE_$(ARCH))
    +IMAGE           := $(IMAGE_$(ARCH))
     IMAGE_NAME       = $(notdir $(IMAGE))

     # default kernel configurations that appear to be usable
    @@ -41,7 +41,7 @@ DEFCONFIG_riscv64    = defconfig
     DEFCONFIG_riscv      = defconfig
     DEFCONFIG_s390       = defconfig
     DEFCONFIG_loongarch  = defconfig
    -DEFCONFIG            = $(DEFCONFIG_$(ARCH))
    +DEFCONFIG           := $(DEFCONFIG_$(ARCH))

     # optional tests to run (default = all)
     TEST =
    @@ -58,7 +58,7 @@ QEMU_ARCH_riscv64    = riscv64
     QEMU_ARCH_riscv      = riscv64
     QEMU_ARCH_s390       = s390x
     QEMU_ARCH_loongarch  = loongarch64
    -QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
    +QEMU_ARCH           := $(QEMU_ARCH_$(ARCH))

     # QEMU_ARGS : some arch-specific args to pass to qemu
     QEMU_ARGS_i386       = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    @@ -72,7 +72,7 @@ QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_T
     QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    -QEMU_ARGS            = $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
    +QEMU_ARGS           := $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)

     # OUTPUT is only set when run from the main makefile, otherwise
     # it defaults to this nolibc directory.
    @@ -87,11 +87,18 @@ endif
     CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
     CFLAGS_s390 = -m64
     CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
    -CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
    +CFLAGS_default := -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
                    $(call cc-option,-fno-stack-protector) \
                    $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
    +
    +CFLAGS  ?= $(CFLAGS_default)

Thanks a lot, will send v4 later.

Best regards,
Zhangjin

> Ok, agree very much, it is the root cause why we used the old method
> before, because I don't want to touch the top-level Makefile, here
> explains the details again just as did for Thomas and Willy [1] ;-)
>
> Without the top-level makefile change, we must add something in
> selftests/nolibc/Makefile like this, because the kernel makefile doesn't
> accept something like ARCH=riscv32 and ARCH=riscv64 currently, it only
> accepts ARCH=riscv (will paste the code later).
>
>     ifneq ($(findstring riscv,$(ARCH)),)
>       _ARCH = riscv
>     else
>       _ARCH = $(ARCH)
>     endif
>
>     ...
>
>     sysroot/$(ARCH)/include:
> 	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
> 	$(QUIET_MKDIR)mkdir -p sysroot
> 	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
> 	$(Q)mv sysroot/sysroot sysroot/$(ARCH)
>
>     defconfig:
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>
>     kernel: initramfs
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>
> The above change really works, but it looks not that good, this is the
> mixing use of _ARCH and ARCH I mentioned in last reply.
>
> Otherwise, we will get such error:
>
>     $ make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
>       MKDIR   sysroot/riscv64/include
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:763: arch/riscv64/Makefile: No such file or directory
>     make[2]: *** No rule to make target 'arch/riscv64/Makefile'.  Stop.
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[1]: *** [Makefile:87: headers_standalone] Error 2
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make: *** [Makefile:129: sysroot/riscv64/include] Error 2
>     $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
>       MKDIR   sysroot/riscv32/include
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:763: arch/riscv32/Makefile: No such file or directory
>     make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[1]: *** [Makefile:87: headers_standalone] Error 2
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make: *** [Makefile:129: sysroot/riscv32/include] Error 2
>
> That's because in top-level Makefile, it doesn't accept ARCH=riscv32 and
> ARCH=riscv64, but x86 and sparc and even parisc support such variants,
> this allows the ARCH variants share the same arch/<SRCARCH>/ source code
> tree, otherwise, they will directly find the arch/<ARCH>/ source code,
> then fails.
>
>     top-level Makefile:
>
>     ...
>     ARCH            ?= $(SUBARCH)
>
>     # Architecture as present in compile.h
>     UTS_MACHINE     := $(ARCH)
>     SRCARCH         := $(ARCH)   ---> SRCARCH is assigned as ARCH by default
>
>     # Additional ARCH settings for x86
>     ifeq ($(ARCH),i386)
>             SRCARCH := x86
>     endif
>     ifeq ($(ARCH),x86_64)
>             SRCARCH := x86
>     endif
>
>     # Additional ARCH settings for sparc
>     ifeq ($(ARCH),sparc32)
>            SRCARCH := sparc
>     endif
>     ifeq ($(ARCH),sparc64)
>            SRCARCH := sparc
>     endif
>
>     # Additional ARCH settings for parisc
>     ifeq ($(ARCH),parisc64)
>            SRCARCH := parisc
>     endif
>
> So, to really align with x86, we should let the top-level makefile be
> able to get the right SRCARCH for riscv32 and riscv64 too ;-)
>
> I even tried to pass SRCARCH=riscv to the top-level Makefile, but it
> failed:
>
>     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
>     index 1b2247a6365d..04067776b569 100644
>     --- a/tools/testing/selftests/nolibc/Makefile
>     +++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -14,6 +14,10 @@ include $(srctree)/scripts/subarch.include
>      ARCH = $(SUBARCH)
>      endif
>
>     +ifneq ($(findstring riscv,$(ARCH)),)
>     +SRCARCH := SRCARCH=riscv
>     +endif
>     +
>      # kernel image names by architecture
>      IMAGE_i386       = arch/x86/boot/bzImage
>      IMAGE_x86_64     = arch/x86/boot/bzImage
>     @@ -126,7 +130,7 @@ sysroot: sysroot/$(ARCH)/include
>      sysroot/$(ARCH)/include:
>             $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
>             $(QUIET_MKDIR)mkdir -p sysroot
>     -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>     +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) $(SRCARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>             $(Q)mv sysroot/sysroot sysroot/$(ARCH)
>
>      nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
>     @@ -150,10 +154,10 @@ initramfs: nolibc-test
>             $(Q)cp nolibc-test initramfs/init
>
>      defconfig:
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>
>      kernel: initramfs
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH )CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>
>      # run the tests after building the kernel
>      run: kernel
>
>     $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin"
>       MKDIR   sysroot/riscv32/include
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:397: srcarch: riscv
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:397: srcarch: riscv
>       INSTALL /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/sysroot/sysroot/include
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>       CC      nolibc-test
>       MKDIR   initramfs
>       INSTALL initramfs/init
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:397: srcarch: riscv32
>       SYNC    include/config/auto.conf.cmd
>     Makefile:397: srcarch: riscv32
>     Makefile:687: arch/riscv32/Makefile: No such file or directory
>     make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
>     make[1]: *** [Makefile:795: include/config/auto.conf.cmd] Error 2
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable'
>
> So, to keep consistent eventually, perhaps we do need to touch the
> top-level Makefile.
>
> Best regards,
> Zhangjin
>
> [1]: https://lore.kernel.org/linux-riscv/20230526092029.149351-1-falcon@tinylab.org/
>
> >     Arnd

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

Arnd, Thomas, Willy

> > >     +# Additional ARCH settings for riscv
> > >     +ifeq ($(ARCH),riscv32)
> > >     +        SRCARCH := riscv
> > >     +endif
> > >     +ifeq ($(ARCH),riscv64)
> > >     +        SRCARCH := riscv
> > >     +endif
> > >     +
> > >      export cross_compiling :=
> > >      ifneq ($(SRCARCH),$(SUBARCH))
> > >      cross_compiling := 1
> >
> > I've never been a big fan of the top-level $(ARCH) setting
> > in the kernel, is there a reason this has to be the same
> > as the variable in tools/include/nolibc? If not, I'd just
> > leave the Linux Makefile unchanged.
> >
> > For userspace we have a lot more target names than
> > arch/*/ directories in the kernel, and I don't think
> > I'd want to enumerate all the possibilities in the
> > build system globally.
>

Good news, I did find a better solution without touching the top-level
Makefile, that is overriding the ARCH to 'riscv' just before the targets
and after we got the necessary settings with the original ARCH=riscv32
or ARCH=riscv64, but it requires to convert the '=' assignments to ':=',
which is not that hard to do and it is more acceptable, just verified it
and it worked well.

    ...

     LDFLAGS := -s

    +# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy
    +ifneq ($(findstring riscv,$(ARCH)),)
    +override ARCH := riscv
    +endif
    +
     help:
            @echo "Supported targets under selftests/nolibc:"
            @echo "  all          call the \"run\" target below"

This change is not that big, and the left changes can keep consistent with the
other platforms. but I still need to add a standalone patch to convert the '='
to ':=' to avoid the before setting using our new overridded ARCH.

    ++ b/tools/testing/selftests/nolibc/Makefile
    @@ -26,7 +26,7 @@ IMAGE_riscv64    = arch/riscv/boot/Image
     IMAGE_riscv      = arch/riscv/boot/Image
     IMAGE_s390       = arch/s390/boot/bzImage
     IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
    -IMAGE            = $(IMAGE_$(ARCH))
    +IMAGE           := $(IMAGE_$(ARCH))
     IMAGE_NAME       = $(notdir $(IMAGE))

     # default kernel configurations that appear to be usable
    @@ -41,7 +41,7 @@ DEFCONFIG_riscv64    = defconfig
     DEFCONFIG_riscv      = defconfig
     DEFCONFIG_s390       = defconfig
     DEFCONFIG_loongarch  = defconfig
    -DEFCONFIG            = $(DEFCONFIG_$(ARCH))
    +DEFCONFIG           := $(DEFCONFIG_$(ARCH))

     # optional tests to run (default = all)
     TEST =
    @@ -58,7 +58,7 @@ QEMU_ARCH_riscv64    = riscv64
     QEMU_ARCH_riscv      = riscv64
     QEMU_ARCH_s390       = s390x
     QEMU_ARCH_loongarch  = loongarch64
    -QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
    +QEMU_ARCH           := $(QEMU_ARCH_$(ARCH))

     # QEMU_ARGS : some arch-specific args to pass to qemu
     QEMU_ARGS_i386       = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    @@ -72,7 +72,7 @@ QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_T
     QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    -QEMU_ARGS            = $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
    +QEMU_ARGS           := $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)

     # OUTPUT is only set when run from the main makefile, otherwise
     # it defaults to this nolibc directory.
    @@ -87,11 +87,18 @@ endif
     CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
     CFLAGS_s390 = -m64
     CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
    -CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
    +CFLAGS_default := -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
                    $(call cc-option,-fno-stack-protector) \
                    $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
    +
    +CFLAGS  ?= $(CFLAGS_default)

Thanks a lot, will send v4 later.

Best regards,
Zhangjin

> Ok, agree very much, it is the root cause why we used the old method
> before, because I don't want to touch the top-level Makefile, here
> explains the details again just as did for Thomas and Willy [1] ;-)
>
> Without the top-level makefile change, we must add something in
> selftests/nolibc/Makefile like this, because the kernel makefile doesn't
> accept something like ARCH=riscv32 and ARCH=riscv64 currently, it only
> accepts ARCH=riscv (will paste the code later).
>
>     ifneq ($(findstring riscv,$(ARCH)),)
>       _ARCH = riscv
>     else
>       _ARCH = $(ARCH)
>     endif
>
>     ...
>
>     sysroot/$(ARCH)/include:
> 	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
> 	$(QUIET_MKDIR)mkdir -p sysroot
> 	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
> 	$(Q)mv sysroot/sysroot sysroot/$(ARCH)
>
>     defconfig:
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>
>     kernel: initramfs
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>
> The above change really works, but it looks not that good, this is the
> mixing use of _ARCH and ARCH I mentioned in last reply.
>
> Otherwise, we will get such error:
>
>     $ make run ARCH=riscv64 CROSS_COMPILE=riscv64-linux-gnu-
>       MKDIR   sysroot/riscv64/include
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:763: arch/riscv64/Makefile: No such file or directory
>     make[2]: *** No rule to make target 'arch/riscv64/Makefile'.  Stop.
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[1]: *** [Makefile:87: headers_standalone] Error 2
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make: *** [Makefile:129: sysroot/riscv64/include] Error 2
>     $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu-
>       MKDIR   sysroot/riscv32/include
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:763: arch/riscv32/Makefile: No such file or directory
>     make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[1]: *** [Makefile:87: headers_standalone] Error 2
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make: *** [Makefile:129: sysroot/riscv32/include] Error 2
>
> That's because in top-level Makefile, it doesn't accept ARCH=riscv32 and
> ARCH=riscv64, but x86 and sparc and even parisc support such variants,
> this allows the ARCH variants share the same arch/<SRCARCH>/ source code
> tree, otherwise, they will directly find the arch/<ARCH>/ source code,
> then fails.
>
>     top-level Makefile:
>
>     ...
>     ARCH            ?= $(SUBARCH)
>
>     # Architecture as present in compile.h
>     UTS_MACHINE     := $(ARCH)
>     SRCARCH         := $(ARCH)   ---> SRCARCH is assigned as ARCH by default
>
>     # Additional ARCH settings for x86
>     ifeq ($(ARCH),i386)
>             SRCARCH := x86
>     endif
>     ifeq ($(ARCH),x86_64)
>             SRCARCH := x86
>     endif
>
>     # Additional ARCH settings for sparc
>     ifeq ($(ARCH),sparc32)
>            SRCARCH := sparc
>     endif
>     ifeq ($(ARCH),sparc64)
>            SRCARCH := sparc
>     endif
>
>     # Additional ARCH settings for parisc
>     ifeq ($(ARCH),parisc64)
>            SRCARCH := parisc
>     endif
>
> So, to really align with x86, we should let the top-level makefile be
> able to get the right SRCARCH for riscv32 and riscv64 too ;-)
>
> I even tried to pass SRCARCH=riscv to the top-level Makefile, but it
> failed:
>
>     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
>     index 1b2247a6365d..04067776b569 100644
>     --- a/tools/testing/selftests/nolibc/Makefile
>     +++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -14,6 +14,10 @@ include $(srctree)/scripts/subarch.include
>      ARCH = $(SUBARCH)
>      endif
>
>     +ifneq ($(findstring riscv,$(ARCH)),)
>     +SRCARCH := SRCARCH=riscv
>     +endif
>     +
>      # kernel image names by architecture
>      IMAGE_i386       = arch/x86/boot/bzImage
>      IMAGE_x86_64     = arch/x86/boot/bzImage
>     @@ -126,7 +130,7 @@ sysroot: sysroot/$(ARCH)/include
>      sysroot/$(ARCH)/include:
>             $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
>             $(QUIET_MKDIR)mkdir -p sysroot
>     -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>     +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) $(SRCARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>             $(Q)mv sysroot/sysroot sysroot/$(ARCH)
>
>      nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
>     @@ -150,10 +154,10 @@ initramfs: nolibc-test
>             $(Q)cp nolibc-test initramfs/init
>
>      defconfig:
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>
>      kernel: initramfs
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) $(SRCARCH )CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>
>      # run the tests after building the kernel
>      run: kernel
>
>     $ make run ARCH=riscv32 CROSS_COMPILE=riscv64-linux-gnu- QEMU_ARGS_EXTRA="-bios /labs/linux-lab/opensbi-riscv32-generic-fw_dynamic.bin"
>       MKDIR   sysroot/riscv32/include
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:397: srcarch: riscv
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[2]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:397: srcarch: riscv
>       INSTALL /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/sysroot/sysroot/include
>     make[2]: Leaving directory '/labs/linux-lab/src/linux-stable'
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable/tools/include/nolibc'
>       CC      nolibc-test
>       MKDIR   initramfs
>       INSTALL initramfs/init
>     make[1]: Entering directory '/labs/linux-lab/src/linux-stable'
>     Makefile:397: srcarch: riscv32
>       SYNC    include/config/auto.conf.cmd
>     Makefile:397: srcarch: riscv32
>     Makefile:687: arch/riscv32/Makefile: No such file or directory
>     make[2]: *** No rule to make target 'arch/riscv32/Makefile'.  Stop.
>     make[1]: *** [Makefile:795: include/config/auto.conf.cmd] Error 2
>     make[1]: Leaving directory '/labs/linux-lab/src/linux-stable'
>
> So, to keep consistent eventually, perhaps we do need to touch the
> top-level Makefile.
>
> Best regards,
> Zhangjin
>
> [1]: https://lore.kernel.org/linux-riscv/20230526092029.149351-1-falcon@tinylab.org/
>
> >     Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Willy Tarreau]

Hi,

On Wed, Jun 07, 2023 at 09:20:32AM +0800, Zhangjin Wu wrote:
> Arnd, Thomas, Willy
> 
> > > >     +# Additional ARCH settings for riscv
> > > >     +ifeq ($(ARCH),riscv32)
> > > >     +        SRCARCH := riscv
> > > >     +endif
> > > >     +ifeq ($(ARCH),riscv64)
> > > >     +        SRCARCH := riscv
> > > >     +endif
> > > >     +
> > > >      export cross_compiling :=
> > > >      ifneq ($(SRCARCH),$(SUBARCH))
> > > >      cross_compiling := 1
> > >
> > > I've never been a big fan of the top-level $(ARCH) setting
> > > in the kernel, is there a reason this has to be the same
> > > as the variable in tools/include/nolibc? If not, I'd just
> > > leave the Linux Makefile unchanged.
> > >
> > > For userspace we have a lot more target names than
> > > arch/*/ directories in the kernel, and I don't think
> > > I'd want to enumerate all the possibilities in the
> > > build system globally.

Actually it's not exactly used by nolibc, except to pass to the kernel
for the install part to extract kernel headers (make headers_install).
It's one of the parts that has required to stick to most of the kernel's
variables very closely (the other one being for nolibc-test which needs
to build a kernel).

> Good news, I did find a better solution without touching the top-level
> Makefile, that is overriding the ARCH to 'riscv' just before the targets
> and after we got the necessary settings with the original ARCH=riscv32
> or ARCH=riscv64, but it requires to convert the '=' assignments to ':=',
> which is not that hard to do and it is more acceptable, just verified it
> and it worked well.
> 
>     ...
> 
>      LDFLAGS := -s
> 
>     +# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy
>     +ifneq ($(findstring riscv,$(ARCH)),)
>     +override ARCH := riscv
>     +endif
>     +

That can be one approach indeed. Another one if we continue to face
difficulties for this one would be to use a distinct KARCH variable
to assign to ARCH in all kernel-specific operations.

>      help:
>             @echo "Supported targets under selftests/nolibc:"
>             @echo "  all          call the \"run\" target below"
> 
> This change is not that big, and the left changes can keep consistent with the
> other platforms. but I still need to add a standalone patch to convert the '='
> to ':=' to avoid the before setting using our new overridded ARCH.

I don't even see why the other ones below are needed, given that as
long as they remain assigned as macros, they will be replaced in-place
where they are used, so they will reference the last known assignment
to ARCH.

>     ++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -26,7 +26,7 @@ IMAGE_riscv64    = arch/riscv/boot/Image
>      IMAGE_riscv      = arch/riscv/boot/Image
>      IMAGE_s390       = arch/s390/boot/bzImage
>      IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
>     -IMAGE            = $(IMAGE_$(ARCH))
>     +IMAGE           := $(IMAGE_$(ARCH))
>      IMAGE_NAME       = $(notdir $(IMAGE))
> 
>      # default kernel configurations that appear to be usable
>     @@ -41,7 +41,7 @@ DEFCONFIG_riscv64    = defconfig
>      DEFCONFIG_riscv      = defconfig
>      DEFCONFIG_s390       = defconfig
>      DEFCONFIG_loongarch  = defconfig
>     -DEFCONFIG            = $(DEFCONFIG_$(ARCH))
>     +DEFCONFIG           := $(DEFCONFIG_$(ARCH))
> 
>      # optional tests to run (default = all)
>      TEST =
>     @@ -58,7 +58,7 @@ QEMU_ARCH_riscv64    = riscv64
>      QEMU_ARCH_riscv      = riscv64
>      QEMU_ARCH_s390       = s390x
>      QEMU_ARCH_loongarch  = loongarch64
>     -QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
>     +QEMU_ARCH           := $(QEMU_ARCH_$(ARCH))
> 
>      # QEMU_ARGS : some arch-specific args to pass to qemu
>      QEMU_ARGS_i386       = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>     @@ -72,7 +72,7 @@ QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_T
>      QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>      QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>      QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>     -QEMU_ARGS            = $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
>     +QEMU_ARGS           := $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
> 
>      # OUTPUT is only set when run from the main makefile, otherwise
>      # it defaults to this nolibc directory.
>     @@ -87,11 +87,18 @@ endif
>      CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
>      CFLAGS_s390 = -m64
>      CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
>     -CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
>     +CFLAGS_default := -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
>                     $(call cc-option,-fno-stack-protector) \
>                     $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
>     +
>     +CFLAGS  ?= $(CFLAGS_default)

Why did you need to split this one like this instead of proceeding
like for the other ones ? Because of the "?=" maybe ? Please
double-check that you really need to turn this from a macro to a
variable, if as I suspect it it's not needed, it would be even
simpler.

Thanks,
Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Willy Tarreau]

Hi,

On Wed, Jun 07, 2023 at 09:20:32AM +0800, Zhangjin Wu wrote:
> Arnd, Thomas, Willy
> 
> > > >     +# Additional ARCH settings for riscv
> > > >     +ifeq ($(ARCH),riscv32)
> > > >     +        SRCARCH := riscv
> > > >     +endif
> > > >     +ifeq ($(ARCH),riscv64)
> > > >     +        SRCARCH := riscv
> > > >     +endif
> > > >     +
> > > >      export cross_compiling :=
> > > >      ifneq ($(SRCARCH),$(SUBARCH))
> > > >      cross_compiling := 1
> > >
> > > I've never been a big fan of the top-level $(ARCH) setting
> > > in the kernel, is there a reason this has to be the same
> > > as the variable in tools/include/nolibc? If not, I'd just
> > > leave the Linux Makefile unchanged.
> > >
> > > For userspace we have a lot more target names than
> > > arch/*/ directories in the kernel, and I don't think
> > > I'd want to enumerate all the possibilities in the
> > > build system globally.

Actually it's not exactly used by nolibc, except to pass to the kernel
for the install part to extract kernel headers (make headers_install).
It's one of the parts that has required to stick to most of the kernel's
variables very closely (the other one being for nolibc-test which needs
to build a kernel).

> Good news, I did find a better solution without touching the top-level
> Makefile, that is overriding the ARCH to 'riscv' just before the targets
> and after we got the necessary settings with the original ARCH=riscv32
> or ARCH=riscv64, but it requires to convert the '=' assignments to ':=',
> which is not that hard to do and it is more acceptable, just verified it
> and it worked well.
> 
>     ...
> 
>      LDFLAGS := -s
> 
>     +# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy
>     +ifneq ($(findstring riscv,$(ARCH)),)
>     +override ARCH := riscv
>     +endif
>     +

That can be one approach indeed. Another one if we continue to face
difficulties for this one would be to use a distinct KARCH variable
to assign to ARCH in all kernel-specific operations.

>      help:
>             @echo "Supported targets under selftests/nolibc:"
>             @echo "  all          call the \"run\" target below"
> 
> This change is not that big, and the left changes can keep consistent with the
> other platforms. but I still need to add a standalone patch to convert the '='
> to ':=' to avoid the before setting using our new overridded ARCH.

I don't even see why the other ones below are needed, given that as
long as they remain assigned as macros, they will be replaced in-place
where they are used, so they will reference the last known assignment
to ARCH.

>     ++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -26,7 +26,7 @@ IMAGE_riscv64    = arch/riscv/boot/Image
>      IMAGE_riscv      = arch/riscv/boot/Image
>      IMAGE_s390       = arch/s390/boot/bzImage
>      IMAGE_loongarch  = arch/loongarch/boot/vmlinuz.efi
>     -IMAGE            = $(IMAGE_$(ARCH))
>     +IMAGE           := $(IMAGE_$(ARCH))
>      IMAGE_NAME       = $(notdir $(IMAGE))
> 
>      # default kernel configurations that appear to be usable
>     @@ -41,7 +41,7 @@ DEFCONFIG_riscv64    = defconfig
>      DEFCONFIG_riscv      = defconfig
>      DEFCONFIG_s390       = defconfig
>      DEFCONFIG_loongarch  = defconfig
>     -DEFCONFIG            = $(DEFCONFIG_$(ARCH))
>     +DEFCONFIG           := $(DEFCONFIG_$(ARCH))
> 
>      # optional tests to run (default = all)
>      TEST =
>     @@ -58,7 +58,7 @@ QEMU_ARCH_riscv64    = riscv64
>      QEMU_ARCH_riscv      = riscv64
>      QEMU_ARCH_s390       = s390x
>      QEMU_ARCH_loongarch  = loongarch64
>     -QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
>     +QEMU_ARCH           := $(QEMU_ARCH_$(ARCH))
> 
>      # QEMU_ARGS : some arch-specific args to pass to qemu
>      QEMU_ARGS_i386       = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>     @@ -72,7 +72,7 @@ QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_T
>      QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>      QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>      QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
>     -QEMU_ARGS            = $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
>     +QEMU_ARGS           := $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
> 
>      # OUTPUT is only set when run from the main makefile, otherwise
>      # it defaults to this nolibc directory.
>     @@ -87,11 +87,18 @@ endif
>      CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
>      CFLAGS_s390 = -m64
>      CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
>     -CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
>     +CFLAGS_default := -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
>                     $(call cc-option,-fno-stack-protector) \
>                     $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
>     +
>     +CFLAGS  ?= $(CFLAGS_default)

Why did you need to split this one like this instead of proceeding
like for the other ones ? Because of the "?=" maybe ? Please
double-check that you really need to turn this from a macro to a
variable, if as I suspect it it's not needed, it would be even
simpler.

Thanks,
Willy

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

> On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Link:
> > https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/include/nolibc/sys.h | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > index 856249a11890..78c86f124335 100644
> > --- a/tools/include/nolibc/sys.h
> > +++ b/tools/include/nolibc/sys.h
> > @@ -124,7 +124,7 @@ int sys_chmod(const char *path, mode_t mode)
> >  #elif defined(__NR_chmod)
> >  	return my_syscall2(__NR_chmod, path, mode);
> >  #else
> > -#error Neither __NR_fchmodat nor __NR_chmod defined, cannot implement
> > sys_chmod()
> > +	return -ENOSYS;
> >  #endif
> >  }
>
> I think the most logical would be to have each syscall (chmod,
> fchmodat, ...) have its own function that returns -ENOSYS if
> that is not defined, and have the logic that decides which one
> to use as a separate function.
>

Yeah, agreed, we can clean up them one by one, If split them to their own
syscalls, I have two questions (for Arnd, and Willy too):

1. do we need to add the corresponding library routines at the same time?

  Use llseek() as an example, there will be llseek() and lsee64(). If off_t
  would be converted to 64bit, then, they can be simply added as follows:

    #define lseek64 lseek
    #define llseek lseek

  Or something like this:

    static __attribute__((unused))
    loff_t lseek(int fd, loff_t offset, int whence)
    {
    	return lseek(fd, offset, whence);
    }

    static __attribute__((unused))
    off64_t lseek(int fd, off64_t offset, int whence)
    {
    	return lseek(fd, offset, whence);
    }

  This one aligns with the other already added library routines.

  Which one do you like more?

2. If so, how to cope with the new types when add the library routines?

  Still use the above llseek() as an example, If not use '#define' method,
  We may need to declare loff_t and off64_t in std.h too:

    #define off64_t off_t
    #define loff_t off_t

  Or align with the other new types, use 'typedef' instead of '#define'.

  And further, use poll() as an example, in its manpage [1], there may be some
  new types, such as 'nfds_t', but 'int' is used in tools/include/nolibc/sys.h
  currently, do we need to add nfds_t?

  The 'idtypes_t' and 'id_t' types used by waitid() [2] is similar, both
  of them can simply use the 'int' type.

The above two questions are important to the coming patches, it may determine
how I should tune the new llseek() and waitid() syscalls and their library
routines. very welcome your suggestions.

> This patch is a step in that direction though, so I think that's
> totally fine.

Thanks, so, can I pick your Reviewed-by for the first two patches? I'm ready to
send v4 now ;-)

Best regards,
Zhangjin

---
[1]: https://linux.die.net/man/2/poll
[2]: https://linux.die.net/man/2/waitid

>
>      Arnd

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

> On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Link:
> > https://lore.kernel.org/linux-riscv/5e7d2adf-e96f-41ca-a4c6-5c87a25d4c9c@app.fastmail.com/
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/include/nolibc/sys.h | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > index 856249a11890..78c86f124335 100644
> > --- a/tools/include/nolibc/sys.h
> > +++ b/tools/include/nolibc/sys.h
> > @@ -124,7 +124,7 @@ int sys_chmod(const char *path, mode_t mode)
> >  #elif defined(__NR_chmod)
> >  	return my_syscall2(__NR_chmod, path, mode);
> >  #else
> > -#error Neither __NR_fchmodat nor __NR_chmod defined, cannot implement
> > sys_chmod()
> > +	return -ENOSYS;
> >  #endif
> >  }
>
> I think the most logical would be to have each syscall (chmod,
> fchmodat, ...) have its own function that returns -ENOSYS if
> that is not defined, and have the logic that decides which one
> to use as a separate function.
>

Yeah, agreed, we can clean up them one by one, If split them to their own
syscalls, I have two questions (for Arnd, and Willy too):

1. do we need to add the corresponding library routines at the same time?

  Use llseek() as an example, there will be llseek() and lsee64(). If off_t
  would be converted to 64bit, then, they can be simply added as follows:

    #define lseek64 lseek
    #define llseek lseek

  Or something like this:

    static __attribute__((unused))
    loff_t lseek(int fd, loff_t offset, int whence)
    {
    	return lseek(fd, offset, whence);
    }

    static __attribute__((unused))
    off64_t lseek(int fd, off64_t offset, int whence)
    {
    	return lseek(fd, offset, whence);
    }

  This one aligns with the other already added library routines.

  Which one do you like more?

2. If so, how to cope with the new types when add the library routines?

  Still use the above llseek() as an example, If not use '#define' method,
  We may need to declare loff_t and off64_t in std.h too:

    #define off64_t off_t
    #define loff_t off_t

  Or align with the other new types, use 'typedef' instead of '#define'.

  And further, use poll() as an example, in its manpage [1], there may be some
  new types, such as 'nfds_t', but 'int' is used in tools/include/nolibc/sys.h
  currently, do we need to add nfds_t?

  The 'idtypes_t' and 'id_t' types used by waitid() [2] is similar, both
  of them can simply use the 'int' type.

The above two questions are important to the coming patches, it may determine
how I should tune the new llseek() and waitid() syscalls and their library
routines. very welcome your suggestions.

> This patch is a step in that direction though, so I think that's
> totally fine.

Thanks, so, can I pick your Reviewed-by for the first two patches? I'm ready to
send v4 now ;-)

Best regards,
Zhangjin

---
[1]: https://linux.die.net/man/2/poll
[2]: https://linux.die.net/man/2/waitid

>
>      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

> On Wed, Jun 07, 2023 at 09:20:32AM +0800, Zhangjin Wu wrote:
> > Arnd, Thomas, Willy
> >     ...
> >
> >      LDFLAGS := -s
> >
> >     +# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy
> >     +ifneq ($(findstring riscv,$(ARCH)),)
> >     +override ARCH := riscv
> >     +endif
> >     +
>
> That can be one approach indeed. Another one if we continue to face
> difficulties for this one would be to use a distinct KARCH variable
> to assign to ARCH in all kernel-specific operations.
>

Yeah, I have replied that method to Arnd and Thomas too, it looks like this:

    ifneq ($(findstring riscv,$(ARCH)),)
      _ARCH = riscv
    else
      _ARCH = $(ARCH)
    endif

    ...

    sysroot/$(ARCH)/include:
	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
	$(QUIET_MKDIR)mkdir -p sysroot
	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
	$(Q)mv sysroot/sysroot sysroot/$(ARCH)

    defconfig:
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

    kernel: initramfs
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

Using KARCH seems better than _ARCH:

    ifneq ($(findstring riscv,$(ARCH)),)
      KARCH = riscv
    else
      KARCH = $(ARCH)
    endif

    ...

    sysroot/$(ARCH)/include:
	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
	$(QUIET_MKDIR)mkdir -p sysroot
	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
	$(Q)mv sysroot/sysroot sysroot/$(ARCH)

    defconfig:
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

    kernel: initramfs
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

but the new method mentioned here differs, it split the whole Makefile
to two 'parts', the before part accept something like ARCH=riscv32,
ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid
touch the targets context:

    ...
    QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
    +QEMU_ARCH           := $(QEMU_ARCH_$(ARCH))

     # QEMU_ARGS : some arch-specific args to pass to qemu
     QEMU_ARGS_i386       = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    @@ -61,10 +67,12 @@ QEMU_ARGS_x86        = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(
     QEMU_ARGS_arm64      = -M virt -cpu cortex-a53 -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_arm        = -M virt -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_mips       = -M malta -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv32    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    -QEMU_ARGS            = $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
    +QEMU_ARGS           := $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)

     # OUTPUT is only set when run from the main makefile, otherwise
     # it defaults to this nolibc directory.
    @@ -76,13 +84,24 @@ else
     Q=@
     endif

    +CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
     CFLAGS_s390 = -m64
     CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
    -CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
    +CFLAGS_default := -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
                    $(call cc-option,-fno-stack-protector) \
                    $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
    +
    +CFLAGS  ?= $(CFLAGS_default)
     LDFLAGS := -s

    ... variable assignments before this line ...

    +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants
    +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy
    +ARCHS := riscv
    +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
    +ifneq ($(_ARCH),)
    +override ARCH := $(_ARCH)
    +endif
    +

    ... targets after this line ...

[1]: https://lore.kernel.org/lkml/20230606120755.548017-1-falcon@tinylab.org/#R

> >      help:
> >             @echo "Supported targets under selftests/nolibc:"
> >             @echo "  all          call the \"run\" target below"
> >
> > This change is not that big, and the left changes can keep consistent with the
> > other platforms. but I still need to add a standalone patch to convert the '='
> > to ':=' to avoid the before setting using our new overridded ARCH.
>
> I don't even see why the other ones below are needed, given that as
> long as they remain assigned as macros, they will be replaced in-place
> where they are used, so they will reference the last known assignment
> to ARCH.

The reason is really:

    "they will reference the last known assignment to ARCH"

If we use something like 'KARCH' or '_ARCH' and not override the ARCH in the
middle, then, no need to touch the ':' and '?='. otherwise, the variable will
accept something like QEMU_ARGS_riscv for riscv32, it breaks our requirement.

>
>        ...
> >      CFLAGS_s390 = -m64 CFLAGS_STACKPROTECTOR ?= $(call
> >      cc-option,-mstack-protector-guard=global $(call
> >      cc-option,-fstack-protector-all)) -CFLAGS  ?= -Os -fno-ident
> >      -fno-asynchronous-unwind-tables -std=c89 \ +CFLAGS_default :=
> >      -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
> >      $(call cc-option,-fno-stack-protector) \ $(CFLAGS_$(ARCH))
> >      $(CFLAGS_STACKPROTECTOR) + +CFLAGS  ?= $(CFLAGS_default)
>
> Why did you need to split this one like this instead of proceeding
> like for the other ones ? Because of the "?=" maybe ? Please
> double-check that you really need to turn this from a macro to a
> variable, if as I suspect it it's not needed, it would be even
> simpler.

It depends on the method we plan to use, just as explained above.

For a standalone KARCH, no need to touch the assignment, otherwise, we
should let the assignment take effect immediately to avoid they use the
one we plan to override.

For the KARCH method, I will tune it to be more scalable like this:

    ARCHS = riscv
    _ARCH = $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
    KARCH = $(or $(_ARCH),$(ARCH))

Willy, Which one do you prefer?

Thanks,
Zhangjin

>
> Thanks, Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

> On Wed, Jun 07, 2023 at 09:20:32AM +0800, Zhangjin Wu wrote:
> > Arnd, Thomas, Willy
> >     ...
> >
> >      LDFLAGS := -s
> >
> >     +# top-level kernel Makefile only accept ARCH=riscv, override ARCH to make kernel happy
> >     +ifneq ($(findstring riscv,$(ARCH)),)
> >     +override ARCH := riscv
> >     +endif
> >     +
>
> That can be one approach indeed. Another one if we continue to face
> difficulties for this one would be to use a distinct KARCH variable
> to assign to ARCH in all kernel-specific operations.
>

Yeah, I have replied that method to Arnd and Thomas too, it looks like this:

    ifneq ($(findstring riscv,$(ARCH)),)
      _ARCH = riscv
    else
      _ARCH = $(ARCH)
    endif

    ...

    sysroot/$(ARCH)/include:
	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
	$(QUIET_MKDIR)mkdir -p sysroot
	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
	$(Q)mv sysroot/sysroot sysroot/$(ARCH)

    defconfig:
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

    kernel: initramfs
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

Using KARCH seems better than _ARCH:

    ifneq ($(findstring riscv,$(ARCH)),)
      KARCH = riscv
    else
      KARCH = $(ARCH)
    endif

    ...

    sysroot/$(ARCH)/include:
	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
	$(QUIET_MKDIR)mkdir -p sysroot
	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
	$(Q)mv sysroot/sysroot sysroot/$(ARCH)

    defconfig:
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

    kernel: initramfs
    	$(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

but the new method mentioned here differs, it split the whole Makefile
to two 'parts', the before part accept something like ARCH=riscv32,
ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid
touch the targets context:

    ...
    QEMU_ARCH            = $(QEMU_ARCH_$(ARCH))
    +QEMU_ARCH           := $(QEMU_ARCH_$(ARCH))

     # QEMU_ARGS : some arch-specific args to pass to qemu
     QEMU_ARGS_i386       = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    @@ -61,10 +67,12 @@ QEMU_ARGS_x86        = -M pc -append "console=ttyS0,9600 i8042.noaux panic=-1 $(
     QEMU_ARGS_arm64      = -M virt -cpu cortex-a53 -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_arm        = -M virt -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_mips       = -M malta -append "panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv32    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    +QEMU_ARGS_riscv64    = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_riscv      = -M virt -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_s390       = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
     QEMU_ARGS_loongarch  = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)"
    -QEMU_ARGS            = $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)
    +QEMU_ARGS           := $(QEMU_ARGS_$(ARCH)) $(QEMU_ARGS_EXTRA)

     # OUTPUT is only set when run from the main makefile, otherwise
     # it defaults to this nolibc directory.
    @@ -76,13 +84,24 @@ else
     Q=@
     endif

    +CFLAGS_riscv32 = -march=rv32im -mabi=ilp32
     CFLAGS_s390 = -m64
     CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
    -CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
    +CFLAGS_default := -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
                    $(call cc-option,-fno-stack-protector) \
                    $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
    +
    +CFLAGS  ?= $(CFLAGS_default)
     LDFLAGS := -s

    ... variable assignments before this line ...

    +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants
    +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy
    +ARCHS := riscv
    +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
    +ifneq ($(_ARCH),)
    +override ARCH := $(_ARCH)
    +endif
    +

    ... targets after this line ...

[1]: https://lore.kernel.org/lkml/20230606120755.548017-1-falcon@tinylab.org/#R

> >      help:
> >             @echo "Supported targets under selftests/nolibc:"
> >             @echo "  all          call the \"run\" target below"
> >
> > This change is not that big, and the left changes can keep consistent with the
> > other platforms. but I still need to add a standalone patch to convert the '='
> > to ':=' to avoid the before setting using our new overridded ARCH.
>
> I don't even see why the other ones below are needed, given that as
> long as they remain assigned as macros, they will be replaced in-place
> where they are used, so they will reference the last known assignment
> to ARCH.

The reason is really:

    "they will reference the last known assignment to ARCH"

If we use something like 'KARCH' or '_ARCH' and not override the ARCH in the
middle, then, no need to touch the ':' and '?='. otherwise, the variable will
accept something like QEMU_ARGS_riscv for riscv32, it breaks our requirement.

>
>        ...
> >      CFLAGS_s390 = -m64 CFLAGS_STACKPROTECTOR ?= $(call
> >      cc-option,-mstack-protector-guard=global $(call
> >      cc-option,-fstack-protector-all)) -CFLAGS  ?= -Os -fno-ident
> >      -fno-asynchronous-unwind-tables -std=c89 \ +CFLAGS_default :=
> >      -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
> >      $(call cc-option,-fno-stack-protector) \ $(CFLAGS_$(ARCH))
> >      $(CFLAGS_STACKPROTECTOR) + +CFLAGS  ?= $(CFLAGS_default)
>
> Why did you need to split this one like this instead of proceeding
> like for the other ones ? Because of the "?=" maybe ? Please
> double-check that you really need to turn this from a macro to a
> variable, if as I suspect it it's not needed, it would be even
> simpler.

It depends on the method we plan to use, just as explained above.

For a standalone KARCH, no need to touch the assignment, otherwise, we
should let the assignment take effect immediately to avoid they use the
one we plan to override.

For the KARCH method, I will tune it to be more scalable like this:

    ARCHS = riscv
    _ARCH = $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
    KARCH = $(or $(_ARCH),$(ARCH))

Willy, Which one do you prefer?

Thanks,
Zhangjin

>
> Thanks, Willy

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Willy Tarreau]

On Wed, Jun 07, 2023 at 02:33:14PM +0800, Zhangjin Wu wrote:
> > That can be one approach indeed. Another one if we continue to face
> > difficulties for this one would be to use a distinct KARCH variable
> > to assign to ARCH in all kernel-specific operations.
> >
> 
> Yeah, I have replied that method to Arnd and Thomas too, it looks like this:
> 
>     ifneq ($(findstring riscv,$(ARCH)),)
>       _ARCH = riscv
>     else
>       _ARCH = $(ARCH)
>     endif
> 
>     ...
> 
>     sysroot/$(ARCH)/include:
> 	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
> 	$(QUIET_MKDIR)mkdir -p sysroot
> 	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
> 	$(Q)mv sysroot/sysroot sysroot/$(ARCH)
> 
>     defconfig:
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> 
>     kernel: initramfs
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
> 
> Using KARCH seems better than _ARCH:
> 
>     ifneq ($(findstring riscv,$(ARCH)),)
>       KARCH = riscv
>     else
>       KARCH = $(ARCH)
>     endif
(...)

At least it suggests what it's going to be used for instead of just
being marked as "special" (something the underscore does).

> but the new method mentioned here differs, it split the whole Makefile
> to two 'parts', the before part accept something like ARCH=riscv32,
> ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid
> touch the targets context:

We don't care about touching *code*. What is important is that it scales
and is understandable, thus maintainable. Code that has many exceptions
or requires a lot of head scratching to figure what's being done is a
pain to maintain and nobody wants to take the risk to touch it. That was
exactly the purpose of the enumeration of per-target args, flags etc in
the makefile: nobody needs to be expert in multiple areas to touch their
own area. If we face a showstopper, we need to address it, and not work
around it for the sake of touching less context.

>     ... variable assignments before this line ...
> 
>     +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants
>     +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy
>     +ARCHS := riscv
>     +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
>     +ifneq ($(_ARCH),)
>     +override ARCH := $(_ARCH)
>     +endif
>     +

So actually this is a perfect example of head scratching for me. I suspect
it would replace x86_64 with x86 if x86 would be placed there for example,
though it would not change anything for i386. Maybe for s390/s390x,
arm/arm64 or ppc/ppc64 etc it would act similarly while these are different
archs. Thus this seems to be trying to generalize a rule around a single
particular case.

Probably that instead this particular case should be addressed explicitly
until we find a generalization (if ever) to other archs:

  ifeq($(ARCH),riscv32)
  override ARCH := riscv
  else ifeq($(ARCH),riscv64)
  override ARCH := riscv
  endif
  endif

Or maybe even better you can decide to remap arch names explicitly:

  # use KARCH_from=to to rename ARCH from $from to $to past this point.
  KARCH_riscv32 := riscv
  KARCH_riscv64 := riscv
  ...
  ifneq($(KARCH_$(ARCH)),)
  override ARCH := $(KARCH_$(ARCH))
  endif

And this does deserve an explicit note in the makefile that anything
using $(ARCH) using a macro will see the renamed arch while anything
using it as a variable before that line will see the original one.

If you want to avoid the '=' vs ':=' mess you can even keep a copy of
the original ARCH at the beginning of the makefile:

  # keep a copy of the arch name requested by the user, for use later
  # when the original form is preferred over the kernel's arch name.
  USER_ARCH = $(ARCH)

> Willy, Which one do you prefer?

The most explicit ones like above. Generally speaking when you try to
add support for your own arch here, you look there for similar ones,
where commands are called, and read in reverse mode till the beginning,
hoping to understand the transformations. I think the current ones and
the proposed ones above are self-explanatory. Anything doing too much
magic renaming or doing too much hard-coded automatic stuff can quickly
obfuscate the principle and make things more complicated. I already
despise "override" because it messes up with macros, but I agree it can
sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH,
and modify the few lines overriding arch in an explicit manner, I think
it would preserve its maintainability.

What do you think ?

thanks,
Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Willy Tarreau]

On Wed, Jun 07, 2023 at 02:33:14PM +0800, Zhangjin Wu wrote:
> > That can be one approach indeed. Another one if we continue to face
> > difficulties for this one would be to use a distinct KARCH variable
> > to assign to ARCH in all kernel-specific operations.
> >
> 
> Yeah, I have replied that method to Arnd and Thomas too, it looks like this:
> 
>     ifneq ($(findstring riscv,$(ARCH)),)
>       _ARCH = riscv
>     else
>       _ARCH = $(ARCH)
>     endif
> 
>     ...
> 
>     sysroot/$(ARCH)/include:
> 	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
> 	$(QUIET_MKDIR)mkdir -p sysroot
> 	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(_ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
> 	$(Q)mv sysroot/sysroot sysroot/$(ARCH)
> 
>     defconfig:
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> 
>     kernel: initramfs
>     	$(Q)$(MAKE) -C $(srctree) ARCH=$(_ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
> 
> Using KARCH seems better than _ARCH:
> 
>     ifneq ($(findstring riscv,$(ARCH)),)
>       KARCH = riscv
>     else
>       KARCH = $(ARCH)
>     endif
(...)

At least it suggests what it's going to be used for instead of just
being marked as "special" (something the underscore does).

> but the new method mentioned here differs, it split the whole Makefile
> to two 'parts', the before part accept something like ARCH=riscv32,
> ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid
> touch the targets context:

We don't care about touching *code*. What is important is that it scales
and is understandable, thus maintainable. Code that has many exceptions
or requires a lot of head scratching to figure what's being done is a
pain to maintain and nobody wants to take the risk to touch it. That was
exactly the purpose of the enumeration of per-target args, flags etc in
the makefile: nobody needs to be expert in multiple areas to touch their
own area. If we face a showstopper, we need to address it, and not work
around it for the sake of touching less context.

>     ... variable assignments before this line ...
> 
>     +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants
>     +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy
>     +ARCHS := riscv
>     +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
>     +ifneq ($(_ARCH),)
>     +override ARCH := $(_ARCH)
>     +endif
>     +

So actually this is a perfect example of head scratching for me. I suspect
it would replace x86_64 with x86 if x86 would be placed there for example,
though it would not change anything for i386. Maybe for s390/s390x,
arm/arm64 or ppc/ppc64 etc it would act similarly while these are different
archs. Thus this seems to be trying to generalize a rule around a single
particular case.

Probably that instead this particular case should be addressed explicitly
until we find a generalization (if ever) to other archs:

  ifeq($(ARCH),riscv32)
  override ARCH := riscv
  else ifeq($(ARCH),riscv64)
  override ARCH := riscv
  endif
  endif

Or maybe even better you can decide to remap arch names explicitly:

  # use KARCH_from=to to rename ARCH from $from to $to past this point.
  KARCH_riscv32 := riscv
  KARCH_riscv64 := riscv
  ...
  ifneq($(KARCH_$(ARCH)),)
  override ARCH := $(KARCH_$(ARCH))
  endif

And this does deserve an explicit note in the makefile that anything
using $(ARCH) using a macro will see the renamed arch while anything
using it as a variable before that line will see the original one.

If you want to avoid the '=' vs ':=' mess you can even keep a copy of
the original ARCH at the beginning of the makefile:

  # keep a copy of the arch name requested by the user, for use later
  # when the original form is preferred over the kernel's arch name.
  USER_ARCH = $(ARCH)

> Willy, Which one do you prefer?

The most explicit ones like above. Generally speaking when you try to
add support for your own arch here, you look there for similar ones,
where commands are called, and read in reverse mode till the beginning,
hoping to understand the transformations. I think the current ones and
the proposed ones above are self-explanatory. Anything doing too much
magic renaming or doing too much hard-coded automatic stuff can quickly
obfuscate the principle and make things more complicated. I already
despise "override" because it messes up with macros, but I agree it can
sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH,
and modify the few lines overriding arch in an explicit manner, I think
it would preserve its maintainability.

What do you think ?

thanks,
Willy

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

> On Wed, Jun 07, 2023 at 02:33:14PM +0800, Zhangjin Wu wrote:
> >
> >     ifneq ($(findstring riscv,$(ARCH)),)
> >       KARCH = riscv
> >     else
> >       KARCH = $(ARCH)
> >     endif
> (...)
>
> At least it suggests what it's going to be used for instead of just
> being marked as "special" (something the underscore does).
>

Yeah.

> > but the new method mentioned here differs, it split the whole Makefile
> > to two 'parts', the before part accept something like ARCH=riscv32,
> > ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid
> > touch the targets context:
>
> We don't care about touching *code*. What is important is that it scales
> and is understandable, thus maintainable. Code that has many exceptions
> or requires a lot of head scratching to figure what's being done is a
> pain to maintain and nobody wants to take the risk to touch it. That was
> exactly the purpose of the enumeration of per-target args, flags etc in
> the makefile: nobody needs to be expert in multiple areas to touch their
> own area. If we face a showstopper, we need to address it, and not work
> around it for the sake of touching less context.
>

Get it clearly.

> >     ... variable assignments before this line ...
> >
> >     +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants
> >     +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy
> >     +ARCHS := riscv
> >     +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
> >     +ifneq ($(_ARCH),)
> >     +override ARCH := $(_ARCH)
> >     +endif
> >     +
>
> So actually this is a perfect example of head scratching for me. I suspect
> it would replace x86_64 with x86 if x86 would be placed there for example,
> though it would not change anything for i386. Maybe for s390/s390x,
> arm/arm64 or ppc/ppc64 etc it would act similarly while these are different
> archs. Thus this seems to be trying to generalize a rule around a single
> particular case.
>

It is true, we did worry about when users wrongly add new ARCH in the
list, a generic way is very hard before we really use them.

> Probably that instead this particular case should be addressed explicitly
> until we find a generalization (if ever) to other archs:
>
>   ifeq($(ARCH),riscv32)
>   override ARCH := riscv
>   else ifeq($(ARCH),riscv64)
>   override ARCH := riscv
>   endif
>   endif
>
> Or maybe even better you can decide to remap arch names explicitly:
>
>   # use KARCH_from=to to rename ARCH from $from to $to past this point.
>   KARCH_riscv32 := riscv
>   KARCH_riscv64 := riscv
>   ...
>   ifneq($(KARCH_$(ARCH)),)
>   override ARCH := $(KARCH_$(ARCH))
>   endif
>

This did inspire me a lot, so, what about simply go back to the KARCH
method without any overriding:

    diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
    index 4a3a105e1fdf..bde635b083f4 100644
    --- a/tools/testing/selftests/nolibc/Makefile
    +++ b/tools/testing/selftests/nolibc/Makefile
    @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include
     ARCH = $(SUBARCH)
     endif

    +# kernel supported ARCH names by architecture
    +KARCH_riscv32    = riscv
    +KARCH_riscv64    = riscv
    +KARCH_riscv      = riscv
    +KARCH            = $(or $(KARCH_$(ARCH)),$(ARCH))
    +
     # kernel image names by architecture
     IMAGE_i386       = arch/x86/boot/bzImage
     IMAGE_x86_64     = arch/x86/boot/bzImage
    @@ -21,6 +27,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
     IMAGE_arm64      = arch/arm64/boot/Image
     IMAGE_arm        = arch/arm/boot/zImage
     IMAGE_mips       = vmlinuz

And this:

    @@ -117,7 +132,7 @@ sysroot: sysroot/$(ARCH)/include
     sysroot/$(ARCH)/include:
            $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
            $(QUIET_MKDIR)mkdir -p sysroot
    -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
    +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
            $(Q)mv sysroot/sysroot sysroot/$(ARCH)

     nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
    @@ -141,10 +156,10 @@ initramfs: nolibc-test
            $(Q)cp nolibc-test initramfs/init

     defconfig:
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

     kernel: initramfs
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

It is almost consistent with the original Makefile now.

I do like this method more than the override method now, the override
method may break the maintainability a lot especially that the
developers may be hard to know which ARCH value it is when he touch a
line of the Makefile.

> And this does deserve an explicit note in the makefile that anything
> using $(ARCH) using a macro will see the renamed arch while anything
> using it as a variable before that line will see the original one.
>
> If you want to avoid the '=' vs ':=' mess you can even keep a copy of
> the original ARCH at the beginning of the makefile:
>
>   # keep a copy of the arch name requested by the user, for use later
>   # when the original form is preferred over the kernel's arch name.
>   USER_ARCH = $(ARCH)
>

Yeah, a copy is good for the override case.

> > Willy, Which one do you prefer?
>
> The most explicit ones like above. Generally speaking when you try to
> add support for your own arch here, you look there for similar ones,
> where commands are called, and read in reverse mode till the beginning,
> hoping to understand the transformations. I think the current ones and
> the proposed ones above are self-explanatory. Anything doing too much
> magic renaming or doing too much hard-coded automatic stuff can quickly
> obfuscate the principle and make things more complicated. I already
> despise "override" because it messes up with macros, but I agree it can
> sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH,
> and modify the few lines overriding arch in an explicit manner, I think
> it would preserve its maintainability.
>

Agree, let's give up the 'override' stuff.

> What do you think ?

So, let's go with the KARCH method if you agree too.

Best regards,
Zhangjin

>
> thanks,
> Willy

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Zhangjin Wu]

> On Wed, Jun 07, 2023 at 02:33:14PM +0800, Zhangjin Wu wrote:
> >
> >     ifneq ($(findstring riscv,$(ARCH)),)
> >       KARCH = riscv
> >     else
> >       KARCH = $(ARCH)
> >     endif
> (...)
>
> At least it suggests what it's going to be used for instead of just
> being marked as "special" (something the underscore does).
>

Yeah.

> > but the new method mentioned here differs, it split the whole Makefile
> > to two 'parts', the before part accept something like ARCH=riscv32,
> > ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid
> > touch the targets context:
>
> We don't care about touching *code*. What is important is that it scales
> and is understandable, thus maintainable. Code that has many exceptions
> or requires a lot of head scratching to figure what's being done is a
> pain to maintain and nobody wants to take the risk to touch it. That was
> exactly the purpose of the enumeration of per-target args, flags etc in
> the makefile: nobody needs to be expert in multiple areas to touch their
> own area. If we face a showstopper, we need to address it, and not work
> around it for the sake of touching less context.
>

Get it clearly.

> >     ... variable assignments before this line ...
> >
> >     +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants
> >     +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy
> >     +ARCHS := riscv
> >     +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch))))
> >     +ifneq ($(_ARCH),)
> >     +override ARCH := $(_ARCH)
> >     +endif
> >     +
>
> So actually this is a perfect example of head scratching for me. I suspect
> it would replace x86_64 with x86 if x86 would be placed there for example,
> though it would not change anything for i386. Maybe for s390/s390x,
> arm/arm64 or ppc/ppc64 etc it would act similarly while these are different
> archs. Thus this seems to be trying to generalize a rule around a single
> particular case.
>

It is true, we did worry about when users wrongly add new ARCH in the
list, a generic way is very hard before we really use them.

> Probably that instead this particular case should be addressed explicitly
> until we find a generalization (if ever) to other archs:
>
>   ifeq($(ARCH),riscv32)
>   override ARCH := riscv
>   else ifeq($(ARCH),riscv64)
>   override ARCH := riscv
>   endif
>   endif
>
> Or maybe even better you can decide to remap arch names explicitly:
>
>   # use KARCH_from=to to rename ARCH from $from to $to past this point.
>   KARCH_riscv32 := riscv
>   KARCH_riscv64 := riscv
>   ...
>   ifneq($(KARCH_$(ARCH)),)
>   override ARCH := $(KARCH_$(ARCH))
>   endif
>

This did inspire me a lot, so, what about simply go back to the KARCH
method without any overriding:

    diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
    index 4a3a105e1fdf..bde635b083f4 100644
    --- a/tools/testing/selftests/nolibc/Makefile
    +++ b/tools/testing/selftests/nolibc/Makefile
    @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include
     ARCH = $(SUBARCH)
     endif

    +# kernel supported ARCH names by architecture
    +KARCH_riscv32    = riscv
    +KARCH_riscv64    = riscv
    +KARCH_riscv      = riscv
    +KARCH            = $(or $(KARCH_$(ARCH)),$(ARCH))
    +
     # kernel image names by architecture
     IMAGE_i386       = arch/x86/boot/bzImage
     IMAGE_x86_64     = arch/x86/boot/bzImage
    @@ -21,6 +27,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
     IMAGE_arm64      = arch/arm64/boot/Image
     IMAGE_arm        = arch/arm/boot/zImage
     IMAGE_mips       = vmlinuz

And this:

    @@ -117,7 +132,7 @@ sysroot: sysroot/$(ARCH)/include
     sysroot/$(ARCH)/include:
            $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
            $(QUIET_MKDIR)mkdir -p sysroot
    -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
    +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
            $(Q)mv sysroot/sysroot sysroot/$(ARCH)

     nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
    @@ -141,10 +156,10 @@ initramfs: nolibc-test
            $(Q)cp nolibc-test initramfs/init

     defconfig:
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare

     kernel: initramfs
    -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
    +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs

It is almost consistent with the original Makefile now.

I do like this method more than the override method now, the override
method may break the maintainability a lot especially that the
developers may be hard to know which ARCH value it is when he touch a
line of the Makefile.

> And this does deserve an explicit note in the makefile that anything
> using $(ARCH) using a macro will see the renamed arch while anything
> using it as a variable before that line will see the original one.
>
> If you want to avoid the '=' vs ':=' mess you can even keep a copy of
> the original ARCH at the beginning of the makefile:
>
>   # keep a copy of the arch name requested by the user, for use later
>   # when the original form is preferred over the kernel's arch name.
>   USER_ARCH = $(ARCH)
>

Yeah, a copy is good for the override case.

> > Willy, Which one do you prefer?
>
> The most explicit ones like above. Generally speaking when you try to
> add support for your own arch here, you look there for similar ones,
> where commands are called, and read in reverse mode till the beginning,
> hoping to understand the transformations. I think the current ones and
> the proposed ones above are self-explanatory. Anything doing too much
> magic renaming or doing too much hard-coded automatic stuff can quickly
> obfuscate the principle and make things more complicated. I already
> despise "override" because it messes up with macros, but I agree it can
> sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH,
> and modify the few lines overriding arch in an explicit manner, I think
> it would preserve its maintainability.
>

Agree, let's give up the 'override' stuff.

> What do you think ?

So, let's go with the KARCH method if you agree too.

Best regards,
Zhangjin

>
> thanks,
> Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Arnd Bergmann]

On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
>> On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
>
> Yeah, agreed, we can clean up them one by one, If split them to their own
> syscalls, I have two questions (for Arnd, and Willy too):
>
> 1. do we need to add the corresponding library routines at the same time?
>
>   Use llseek() as an example, there will be llseek() and lsee64(). If off_t
>   would be converted to 64bit, then, they can be simply added as follows:
>
>     #define lseek64 lseek
>     #define llseek lseek
>
>   Or something like this:
>
>     static __attribute__((unused))
>     loff_t lseek(int fd, loff_t offset, int whence)
>     {
>     	return lseek(fd, offset, whence);
>     }
>
>     static __attribute__((unused))
>     off64_t lseek(int fd, off64_t offset, int whence)
>     {
>     	return lseek(fd, offset, whence);
>     }
>
>   This one aligns with the other already added library routines.
>
>   Which one do you like more?

lseek() is probably not a good example, as the llseek and lseek64
library functions are both nonstandard, and I'd suggest leaving them
out of nolibc altogether.

Are there any examples of functions where we actually want mulitple
versions?

> 2. If so, how to cope with the new types when add the library routines?
>
>   Still use the above llseek() as an example, If not use '#define' method,
>   We may need to declare loff_t and off64_t in std.h too:
>
>     #define off64_t off_t
>     #define loff_t off_t
>
>   Or align with the other new types, use 'typedef' instead of '#define'.

If we do want to include the explicit off64_t interfaces from glibc,
I'd suggest doing it the same way as musl:
https://git.musl-libc.org/cgit/musl/tree/include/unistd.h#n201

>
>> This patch is a step in that direction though, so I think that's
>> totally fine.
>
> Thanks, so, can I pick your Reviewed-by for the first two patches? I'm ready to
> send v4 now ;-)

Yes, please do.

     Arnd

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Arnd Bergmann]

On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
>> On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
>
> Yeah, agreed, we can clean up them one by one, If split them to their own
> syscalls, I have two questions (for Arnd, and Willy too):
>
> 1. do we need to add the corresponding library routines at the same time?
>
>   Use llseek() as an example, there will be llseek() and lsee64(). If off_t
>   would be converted to 64bit, then, they can be simply added as follows:
>
>     #define lseek64 lseek
>     #define llseek lseek
>
>   Or something like this:
>
>     static __attribute__((unused))
>     loff_t lseek(int fd, loff_t offset, int whence)
>     {
>     	return lseek(fd, offset, whence);
>     }
>
>     static __attribute__((unused))
>     off64_t lseek(int fd, off64_t offset, int whence)
>     {
>     	return lseek(fd, offset, whence);
>     }
>
>   This one aligns with the other already added library routines.
>
>   Which one do you like more?

lseek() is probably not a good example, as the llseek and lseek64
library functions are both nonstandard, and I'd suggest leaving them
out of nolibc altogether.

Are there any examples of functions where we actually want mulitple
versions?

> 2. If so, how to cope with the new types when add the library routines?
>
>   Still use the above llseek() as an example, If not use '#define' method,
>   We may need to declare loff_t and off64_t in std.h too:
>
>     #define off64_t off_t
>     #define loff_t off_t
>
>   Or align with the other new types, use 'typedef' instead of '#define'.

If we do want to include the explicit off64_t interfaces from glibc,
I'd suggest doing it the same way as musl:
https://git.musl-libc.org/cgit/musl/tree/include/unistd.h#n201

>
>> This patch is a step in that direction though, so I think that's
>> totally fine.
>
> Thanks, so, can I pick your Reviewed-by for the first two patches? I'm ready to
> send v4 now ;-)

Yes, please do.

     Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

> On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
> >> On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
> >
> > Yeah, agreed, we can clean up them one by one, If split them to their own
> > syscalls, I have two questions (for Arnd, and Willy too):
> >
> > 1. do we need to add the corresponding library routines at the same time?
> >
> >   Use llseek() as an example, there will be llseek() and lsee64(). If off_t
> >   would be converted to 64bit, then, they can be simply added as follows:
> >
> >     #define lseek64 lseek
> >     #define llseek lseek
> >
> >   Or something like this:
> >
> >     static __attribute__((unused))
> >     loff_t lseek(int fd, loff_t offset, int whence)
> >     {
> >     	return lseek(fd, offset, whence);
> >     }
> >
> >     static __attribute__((unused))
> >     off64_t lseek(int fd, off64_t offset, int whence)
> >     {
> >     	return lseek(fd, offset, whence);
> >     }
> >
> >   This one aligns with the other already added library routines.
> >
> >   Which one do you like more?
> 
> lseek() is probably not a good example, as the llseek and lseek64
> library functions are both nonstandard, and I'd suggest leaving them
> out of nolibc altogether.
>

Ok, agree, as the 64bit version of lseek may be enough for nolibc, if a target
application really require, they can add the alias themselves.

> Are there any examples of functions where we actually want mulitple
> versions?
>

For example, the following ones are related to the syscalls being added,
all of them have similar library routines in current sys.h:

* waitid, https://linux.die.net/man/2/waitid
* ppoll, https://linux.die.net/man/2/ppoll
* pselect, https://linux.die.net/man/2/pselect6
* clock_gettime, https://linux.die.net/man/2/clock_gettime

The similar routines are put in right side:

* waitid --> waitpid, wait, wait4
* ppoll --> poll
* pselect --> select
* clock_gettime --> gettimeofday

For the clock_gettime, it may also let us think about if we need to add
its friends (clock_getres, clock_settime) together.

> > 2. If so, how to cope with the new types when add the library routines?
> >
> >   Still use the above llseek() as an example, If not use '#define' method,
> >   We may need to declare loff_t and off64_t in std.h too:
> >
> >     #define off64_t off_t
> >     #define loff_t off_t
> >
> >   Or align with the other new types, use 'typedef' instead of '#define'.
> 
> If we do want to include the explicit off64_t interfaces from glibc,
> I'd suggest doing it the same way as musl:
> https://git.musl-libc.org/cgit/musl/tree/include/unistd.h#n201
>

Thanks.

> >
> >> This patch is a step in that direction though, so I think that's
> >> totally fine.
> >
> > Thanks, so, can I pick your Reviewed-by for the first two patches? I'm ready to
> > send v4 now ;-)
> 
> Yes, please do.
>

Thanks very much, just added the Reviewed-by lines in v4 and have
already sent v4 out, welcome your review on the revision of the 3rd one,
it is almost consistent with the original Makefile now.

Best regards,
Zhangjin

>      Arnd

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

> On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
> >> On Sat, Jun 3, 2023, at 11:01, Zhangjin Wu wrote:
> >
> > Yeah, agreed, we can clean up them one by one, If split them to their own
> > syscalls, I have two questions (for Arnd, and Willy too):
> >
> > 1. do we need to add the corresponding library routines at the same time?
> >
> >   Use llseek() as an example, there will be llseek() and lsee64(). If off_t
> >   would be converted to 64bit, then, they can be simply added as follows:
> >
> >     #define lseek64 lseek
> >     #define llseek lseek
> >
> >   Or something like this:
> >
> >     static __attribute__((unused))
> >     loff_t lseek(int fd, loff_t offset, int whence)
> >     {
> >     	return lseek(fd, offset, whence);
> >     }
> >
> >     static __attribute__((unused))
> >     off64_t lseek(int fd, off64_t offset, int whence)
> >     {
> >     	return lseek(fd, offset, whence);
> >     }
> >
> >   This one aligns with the other already added library routines.
> >
> >   Which one do you like more?
> 
> lseek() is probably not a good example, as the llseek and lseek64
> library functions are both nonstandard, and I'd suggest leaving them
> out of nolibc altogether.
>

Ok, agree, as the 64bit version of lseek may be enough for nolibc, if a target
application really require, they can add the alias themselves.

> Are there any examples of functions where we actually want mulitple
> versions?
>

For example, the following ones are related to the syscalls being added,
all of them have similar library routines in current sys.h:

* waitid, https://linux.die.net/man/2/waitid
* ppoll, https://linux.die.net/man/2/ppoll
* pselect, https://linux.die.net/man/2/pselect6
* clock_gettime, https://linux.die.net/man/2/clock_gettime

The similar routines are put in right side:

* waitid --> waitpid, wait, wait4
* ppoll --> poll
* pselect --> select
* clock_gettime --> gettimeofday

For the clock_gettime, it may also let us think about if we need to add
its friends (clock_getres, clock_settime) together.

> > 2. If so, how to cope with the new types when add the library routines?
> >
> >   Still use the above llseek() as an example, If not use '#define' method,
> >   We may need to declare loff_t and off64_t in std.h too:
> >
> >     #define off64_t off_t
> >     #define loff_t off_t
> >
> >   Or align with the other new types, use 'typedef' instead of '#define'.
> 
> If we do want to include the explicit off64_t interfaces from glibc,
> I'd suggest doing it the same way as musl:
> https://git.musl-libc.org/cgit/musl/tree/include/unistd.h#n201
>

Thanks.

> >
> >> This patch is a step in that direction though, so I think that's
> >> totally fine.
> >
> > Thanks, so, can I pick your Reviewed-by for the first two patches? I'm ready to
> > send v4 now ;-)
> 
> Yes, please do.
>

Thanks very much, just added the Reviewed-by lines in v4 and have
already sent v4 out, welcome your review on the revision of the 3rd one,
it is almost consistent with the original Makefile now.

Best regards,
Zhangjin

>      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Arnd Bergmann]

On Wed, Jun 7, 2023, at 11:46, Zhangjin Wu wrote:
>> On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
>
> Ok, agree, as the 64bit version of lseek may be enough for nolibc, if a target
> application really require, they can add the alias themselves.
>
>> Are there any examples of functions where we actually want mulitple
>> versions?
>>
>
> For example, the following ones are related to the syscalls being added,
> all of them have similar library routines in current sys.h:
>
> * waitid, https://linux.die.net/man/2/waitid
> * ppoll, https://linux.die.net/man/2/ppoll
> * pselect, https://linux.die.net/man/2/pselect6
> * clock_gettime, https://linux.die.net/man/2/clock_gettime
>
> The similar routines are put in right side:
>
> * waitid --> waitpid, wait, wait4
> * ppoll --> poll
> * pselect --> select
> * clock_gettime --> gettimeofday

Ok, I think these are all useful to have in both versions.

All four of these examples are old enough that I think it's
sufficient just expose them to userspace as the bare system
calls, and have the older library calls be implemented using
them without a fallback to the native syscalls of the same
name on architectures that have both, newer architectures
would only have the latest version anyway.

> For the clock_gettime, it may also let us think about if we need to add
> its friends (clock_getres, clock_settime) together.

Yes, I think that makes sense. We also need clock_settime()
to implement settimeofday() on rv32.

Ideally, I'd love to extend the tooling around system calls
in the kernel so we can automatically generate the low-level
wrapper functions from syscall.tbl, but this needs a lot of
other work that you should not need to depend on for what
you are doing right now.

     Arnd

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Arnd Bergmann]

On Wed, Jun 7, 2023, at 11:46, Zhangjin Wu wrote:
>> On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
>
> Ok, agree, as the 64bit version of lseek may be enough for nolibc, if a target
> application really require, they can add the alias themselves.
>
>> Are there any examples of functions where we actually want mulitple
>> versions?
>>
>
> For example, the following ones are related to the syscalls being added,
> all of them have similar library routines in current sys.h:
>
> * waitid, https://linux.die.net/man/2/waitid
> * ppoll, https://linux.die.net/man/2/ppoll
> * pselect, https://linux.die.net/man/2/pselect6
> * clock_gettime, https://linux.die.net/man/2/clock_gettime
>
> The similar routines are put in right side:
>
> * waitid --> waitpid, wait, wait4
> * ppoll --> poll
> * pselect --> select
> * clock_gettime --> gettimeofday

Ok, I think these are all useful to have in both versions.

All four of these examples are old enough that I think it's
sufficient just expose them to userspace as the bare system
calls, and have the older library calls be implemented using
them without a fallback to the native syscalls of the same
name on architectures that have both, newer architectures
would only have the latest version anyway.

> For the clock_gettime, it may also let us think about if we need to add
> its friends (clock_getres, clock_settime) together.

Yes, I think that makes sense. We also need clock_settime()
to implement settimeofday() on rv32.

Ideally, I'd love to extend the tooling around system calls
in the kernel so we can automatically generate the low-level
wrapper functions from syscall.tbl, but this needs a lot of
other work that you should not need to depend on for what
you are doing right now.

     Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Willy Tarreau]

On Wed, Jun 07, 2023 at 04:11:03PM +0800, Zhangjin Wu wrote:
> This did inspire me a lot, so, what about simply go back to the KARCH
> method without any overriding:
> 
>     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
>     index 4a3a105e1fdf..bde635b083f4 100644
>     --- a/tools/testing/selftests/nolibc/Makefile
>     +++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include
>      ARCH = $(SUBARCH)
>      endif
> 
>     +# kernel supported ARCH names by architecture
>     +KARCH_riscv32    = riscv
>     +KARCH_riscv64    = riscv
>     +KARCH_riscv      = riscv
>     +KARCH            = $(or $(KARCH_$(ARCH)),$(ARCH))
>     +
>      # kernel image names by architecture
>      IMAGE_i386       = arch/x86/boot/bzImage
>      IMAGE_x86_64     = arch/x86/boot/bzImage
>     @@ -21,6 +27,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
>      IMAGE_arm64      = arch/arm64/boot/Image
>      IMAGE_arm        = arch/arm/boot/zImage
>      IMAGE_mips       = vmlinuz
> 
> And this:
> 
>     @@ -117,7 +132,7 @@ sysroot: sysroot/$(ARCH)/include
>      sysroot/$(ARCH)/include:
>             $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
>             $(QUIET_MKDIR)mkdir -p sysroot
>     -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>     +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>             $(Q)mv sysroot/sysroot sysroot/$(ARCH)
> 
>      nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
>     @@ -141,10 +156,10 @@ initramfs: nolibc-test
>             $(Q)cp nolibc-test initramfs/init
> 
>      defconfig:
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> 
>      kernel: initramfs
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
> 
> It is almost consistent with the original Makefile now.

If it works, I like it!

> I do like this method more than the override method now, the override
> method may break the maintainability a lot especially that the
> developers may be hard to know which ARCH value it is when he touch a
> line of the Makefile.

Yes definitely, add to this the risk that a patch applies at the wrong
line and only breaks one or two archs, etc.

> > Generally speaking when you try to
> > add support for your own arch here, you look there for similar ones,
> > where commands are called, and read in reverse mode till the beginning,
> > hoping to understand the transformations. I think the current ones and
> > the proposed ones above are self-explanatory. Anything doing too much
> > magic renaming or doing too much hard-coded automatic stuff can quickly
> > obfuscate the principle and make things more complicated. I already
> > despise "override" because it messes up with macros, but I agree it can
> > sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH,
> > and modify the few lines overriding arch in an explicit manner, I think
> > it would preserve its maintainability.
> >
> 
> Agree, let's give up the 'override' stuff.
> 
> > What do you think ?
> 
> So, let's go with the KARCH method if you agree too.

I'm fine with it!

Thanks,
Willy

Re: [PATCH v3 3/3] selftests/nolibc: riscv: customize makefile for rv32

[Willy Tarreau]

On Wed, Jun 07, 2023 at 04:11:03PM +0800, Zhangjin Wu wrote:
> This did inspire me a lot, so, what about simply go back to the KARCH
> method without any overriding:
> 
>     diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
>     index 4a3a105e1fdf..bde635b083f4 100644
>     --- a/tools/testing/selftests/nolibc/Makefile
>     +++ b/tools/testing/selftests/nolibc/Makefile
>     @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include
>      ARCH = $(SUBARCH)
>      endif
> 
>     +# kernel supported ARCH names by architecture
>     +KARCH_riscv32    = riscv
>     +KARCH_riscv64    = riscv
>     +KARCH_riscv      = riscv
>     +KARCH            = $(or $(KARCH_$(ARCH)),$(ARCH))
>     +
>      # kernel image names by architecture
>      IMAGE_i386       = arch/x86/boot/bzImage
>      IMAGE_x86_64     = arch/x86/boot/bzImage
>     @@ -21,6 +27,8 @@ IMAGE_x86        = arch/x86/boot/bzImage
>      IMAGE_arm64      = arch/arm64/boot/Image
>      IMAGE_arm        = arch/arm/boot/zImage
>      IMAGE_mips       = vmlinuz
> 
> And this:
> 
>     @@ -117,7 +132,7 @@ sysroot: sysroot/$(ARCH)/include
>      sysroot/$(ARCH)/include:
>             $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
>             $(QUIET_MKDIR)mkdir -p sysroot
>     -       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>     +       $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>             $(Q)mv sysroot/sysroot sysroot/$(ARCH)
> 
>      nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
>     @@ -141,10 +156,10 @@ initramfs: nolibc-test
>             $(Q)cp nolibc-test initramfs/init
> 
>      defconfig:
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
> 
>      kernel: initramfs
>     -       $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
>     +       $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
> 
> It is almost consistent with the original Makefile now.

If it works, I like it!

> I do like this method more than the override method now, the override
> method may break the maintainability a lot especially that the
> developers may be hard to know which ARCH value it is when he touch a
> line of the Makefile.

Yes definitely, add to this the risk that a patch applies at the wrong
line and only breaks one or two archs, etc.

> > Generally speaking when you try to
> > add support for your own arch here, you look there for similar ones,
> > where commands are called, and read in reverse mode till the beginning,
> > hoping to understand the transformations. I think the current ones and
> > the proposed ones above are self-explanatory. Anything doing too much
> > magic renaming or doing too much hard-coded automatic stuff can quickly
> > obfuscate the principle and make things more complicated. I already
> > despise "override" because it messes up with macros, but I agree it can
> > sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH,
> > and modify the few lines overriding arch in an explicit manner, I think
> > it would preserve its maintainability.
> >
> 
> Agree, let's give up the 'override' stuff.
> 
> > What do you think ?
> 
> So, let's go with the KARCH method if you agree too.

I'm fine with it!

Thanks,
Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

> On Wed, Jun 7, 2023, at 11:46, Zhangjin Wu wrote:
> >> On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
> >
> > Ok, agree, as the 64bit version of lseek may be enough for nolibc, if a target
> > application really require, they can add the alias themselves.
> >
> >> Are there any examples of functions where we actually want mulitple
> >> versions?
> >>
> >
> > For example, the following ones are related to the syscalls being added,
> > all of them have similar library routines in current sys.h:
> >
> > * waitid, https://linux.die.net/man/2/waitid
> > * ppoll, https://linux.die.net/man/2/ppoll
> > * pselect, https://linux.die.net/man/2/pselect6
> > * clock_gettime, https://linux.die.net/man/2/clock_gettime
> >
> > The similar routines are put in right side:
> >
> > * waitid --> waitpid, wait, wait4
> > * ppoll --> poll
> > * pselect --> select
> > * clock_gettime --> gettimeofday
> 
> Ok, I think these are all useful to have in both versions.
> 
> All four of these examples are old enough that I think it's
> sufficient just expose them to userspace as the bare system
> calls, and have the older library calls be implemented using
> them without a fallback to the native syscalls of the same
> name on architectures that have both, newer architectures
> would only have the latest version anyway.
>

Ok, Thanks, I have already added parts of them, will send waitid and
64bit lseek at first.

> > For the clock_gettime, it may also let us think about if we need to add
> > its friends (clock_getres, clock_settime) together.
> 
> Yes, I think that makes sense. We also need clock_settime()
> to implement settimeofday() on rv32.
>

Ok.

> Ideally, I'd love to extend the tooling around system calls
> in the kernel so we can automatically generate the low-level
> wrapper functions from syscall.tbl,

That's cool.

BTW, I did something on dead syscall elimination [1] (DSE, RFC
patchset), a v1 has been prepared locally, but not sent out yet.

It also requires to work with the syscall.tbl or the generic
include/uapi/asm-generic/unistd.h, welcome your feedback on the RFC
patchset [1] and you should be the right reviewer of the coming v1 ;-)

The left issue of RFC version is finding a way to not KEEP the exception
entries (in ld script) added by get_user/put_user() if the corresponding
syscalls are not really used, such KEEPs of exception entries reverts
the ownership from "syscalls -> get_user/put_user" to "get_user/put_user
-> syscalls" and blocks the gc'ing of the sections of such syscalls.

In the coming v1, I used a script trick to drop the wrongly KEPT
exception entries to allow drop all of the unused syscalls at last. Will
clean up them asap. But it is a little slow and looks ugly, it is only
for a further demo of the possibility.

In v2 of DSE, I'm wondering whether it is possible to drop all of the
manually added KEEP operations from ld scripts and use some conditional
attributes (for the sections added by get_user/put_user) to build the
'used' references from "syscalls" to "sections created by
get_user/put_user", this may need support from gcc and ld, welcome your
suggestions too, thanks.

And that RFC patchset added a patch to record the used 'syscalls' in
nolibc automatically ;-)

[1]: https://lore.kernel.org/linux-riscv/cover.1676594211.git.falcon@tinylab.org/
[2]: https://reviews.llvm.org/D96838

> but this needs a lot of
> other work that you should not need to depend on for what
> you are doing right now.

Ok, welcome to share any progress.

Thanks,
Zhangjin

> 
>      Arnd

Re: [PATCH v3 1/3] tools/nolibc: fix up #error compile failures with -ENOSYS

[Zhangjin Wu]

> On Wed, Jun 7, 2023, at 11:46, Zhangjin Wu wrote:
> >> On Wed, Jun 7, 2023, at 07:19, Zhangjin Wu wrote:
> >
> > Ok, agree, as the 64bit version of lseek may be enough for nolibc, if a target
> > application really require, they can add the alias themselves.
> >
> >> Are there any examples of functions where we actually want mulitple
> >> versions?
> >>
> >
> > For example, the following ones are related to the syscalls being added,
> > all of them have similar library routines in current sys.h:
> >
> > * waitid, https://linux.die.net/man/2/waitid
> > * ppoll, https://linux.die.net/man/2/ppoll
> > * pselect, https://linux.die.net/man/2/pselect6
> > * clock_gettime, https://linux.die.net/man/2/clock_gettime
> >
> > The similar routines are put in right side:
> >
> > * waitid --> waitpid, wait, wait4
> > * ppoll --> poll
> > * pselect --> select
> > * clock_gettime --> gettimeofday
> 
> Ok, I think these are all useful to have in both versions.
> 
> All four of these examples are old enough that I think it's
> sufficient just expose them to userspace as the bare system
> calls, and have the older library calls be implemented using
> them without a fallback to the native syscalls of the same
> name on architectures that have both, newer architectures
> would only have the latest version anyway.
>

Ok, Thanks, I have already added parts of them, will send waitid and
64bit lseek at first.

> > For the clock_gettime, it may also let us think about if we need to add
> > its friends (clock_getres, clock_settime) together.
> 
> Yes, I think that makes sense. We also need clock_settime()
> to implement settimeofday() on rv32.
>

Ok.

> Ideally, I'd love to extend the tooling around system calls
> in the kernel so we can automatically generate the low-level
> wrapper functions from syscall.tbl,

That's cool.

BTW, I did something on dead syscall elimination [1] (DSE, RFC
patchset), a v1 has been prepared locally, but not sent out yet.

It also requires to work with the syscall.tbl or the generic
include/uapi/asm-generic/unistd.h, welcome your feedback on the RFC
patchset [1] and you should be the right reviewer of the coming v1 ;-)

The left issue of RFC version is finding a way to not KEEP the exception
entries (in ld script) added by get_user/put_user() if the corresponding
syscalls are not really used, such KEEPs of exception entries reverts
the ownership from "syscalls -> get_user/put_user" to "get_user/put_user
-> syscalls" and blocks the gc'ing of the sections of such syscalls.

In the coming v1, I used a script trick to drop the wrongly KEPT
exception entries to allow drop all of the unused syscalls at last. Will
clean up them asap. But it is a little slow and looks ugly, it is only
for a further demo of the possibility.

In v2 of DSE, I'm wondering whether it is possible to drop all of the
manually added KEEP operations from ld scripts and use some conditional
attributes (for the sections added by get_user/put_user) to build the
'used' references from "syscalls" to "sections created by
get_user/put_user", this may need support from gcc and ld, welcome your
suggestions too, thanks.

And that RFC patchset added a patch to record the used 'syscalls' in
nolibc automatically ;-)

[1]: https://lore.kernel.org/linux-riscv/cover.1676594211.git.falcon@tinylab.org/
[2]: https://reviews.llvm.org/D96838

> but this needs a lot of
> other work that you should not need to depend on for what
> you are doing right now.

Ok, welcome to share any progress.

Thanks,
Zhangjin

> 
>      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv