On Mon, 15 Mar 2021 01:49:19 +0900,
Johannes Berg wrote:

On Wed, 2021-01-20 at 11:27 +0900, Hajime Tazaki wrote:

quoted

+++ b/arch/um/lkl/include/asm/atomic.h

@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __UM_LIBMODE_ATOMIC_H
+#define __UM_LIBMODE_ATOMIC_H
+
+#include <asm-generic/atomic.h>
+
+#ifndef CONFIG_GENERIC_ATOMIC64
+#include "atomic64.h"
+#endif /* !CONFIG_GENERIC_ATOMIC64 */

That's not actually configurable, is it? When is this on or off?

This is off when CONFIG_64BIT is off, because in that case (!64BIT) we
use generic atomic64 facility of in-kernel.

quoted

+static inline void cpu_relax(void)
+{
+	unsigned long flags;
+
+	/* since this is usually called in a tight loop waiting for some
+	 * external condition (e.g. jiffies) lets run interrupts now to allow
+	 * the external condition to propagate
+	 */
+	local_irq_save(flags);
+	local_irq_restore(flags);

Hmm?

If interrupts are enabled, then you'll get them anyway, and if not, then
this does nothing?

You might be right; the original LKL does yield-like operation if this
function is called, but this might not be the case after the
integration with UML.  I will investigate this and fix it.

quoted

+static inline void arch_copy_thread(struct arch_thread *from,
+				    struct arch_thread *to)
+{
+	panic("unimplemented %s: fork isn't supported yet", __func__);
+}

"yet" seems kind of misleading - I mean, it really *won't* be, since
that's basically what UML is, no?

I mean, in principle, nothing actually stops you from building a
linux.so instead of linux binary, and having main() renamed to
linux_main() so the application can start up whenever it needs to, or
something like that?

I commented this in the reply-thread of the [00/20] patch.

quoted

diff --git a/arch/um/lkl/include/uapi/asm/bitsperlong.h b/arch/um/lkl/include/uapi/asm/bitsperlong.h
new file mode 100644
index 000000000000..f6657ba0ff9b
--- /dev/null
+++ b/arch/um/lkl/include/uapi/asm/bitsperlong.h

@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __UM_LIBMODE_UAPI_BITSPERLONG_H
+#define __UM_LIBMODE_UAPI_BITSPERLONG_H
+
+#ifdef CONFIG_64BIT
+#define __BITS_PER_LONG 64
+#else
+#define __BITS_PER_LONG 32
+#endif
+
+#include <asm-generic/bitsperlong.h>

First, that include does nothing.

We followed the way of arch/x86 (and other archs) for this.

This actually includes include/asm-generic/bitsperlong.h, which does
something I think.

Second, the comment there says:

/*
 * There seems to be no way of detecting this automatically from user
 * space, so 64 bit architectures should override this in their
 * bitsperlong.h. In particular, an architecture that supports
 * both 32 and 64 bit user space must not rely on CONFIG_64BIT
 * to decide it, but rather check a compiler provided macro.
 */


So you're doing exactly what it says *not* to?

In fact, that totally makes sense - I mean, this is *uapi* and
applications don't have access to CONFIG_ defines etc.

Sorry, this patch is wrong. There is additional diff in [10/20], which
should be in the same patch here, which eliminates not to rely on
CONFIG_*.

I will fix this.
 

Do you expose that somehow to LKL users that makes this OK-ish? But it's
still very confusing, and you've not made sure the necessary header is
actually included. IIUC, lkl builds a (shared?) library, so it won't
really work this way?

quoted

+++ b/arch/um/lkl/include/uapi/asm/byteorder.h

@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __UM_LIBMODE_UAPI_BYTEORDER_H
+#define __UM_LIBMODE_UAPI_BYTEORDER_H
+
+#if defined(CONFIG_BIG_ENDIAN)
+#include <linux/byteorder/big_endian.h>

Same here.

Same problem here; we should not split this in the patch and [10/20].
Will fix this too.

quoted

+++ b/arch/um/lkl/um/delay.c

@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/jiffies.h>
+#include <linux/delay.h>
+#include <os.h>
+
+void __ndelay(unsigned long nsecs)
+{
+	long long start = os_nsecs();
+
+	while (os_nsecs() < start + nsecs)
+		;

At least do something like cpu_relax()? Although ... you made that do
nothing, so you probably want an arch-specific thing here?

I think this isn't efficient at all, but portable enough, which is
(sub)arch-specific requirement.

-- Hajime

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