[RFC v8 06/20] um: add UML library mode

[Hajime Tazaki]

On Mon, 15 Mar 2021 01:49:19 +0900,
Johannes Berg wrote:
> 
> On Wed, 2021-01-20 at 11:27 +0900, Hajime Tazaki wrote:
> > 
> > +++ 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.

> > +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.


> > +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.

> > 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?
>
> > +++ 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.

> > +++ 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