[PATCH/resend/bypass] um: Preinclude include/linux/kern_levels.h

[PATCH/resend/bypass] um: Preinclude include/linux/kern_levels.h

[Geert Uytterhoeven]

The userspace part of UML uses the asm-offsets.h generator mechanism to
create definitions for UM_KERN_<LEVEL> that match the in-kernel
KERN_<LEVEL> constant definitions.

As of commit 04d2c8c83d0e3ac5f78aeede51babb3236200112 ("printk: convert
the format for KERN_<LEVEL> to a 2 byte pattern"), KERN_<LEVEL> is no
longer expanded to the literal '"<LEVEL>"', but to '"\001" "LEVEL"', i.e.
it contains two parts.

However, the combo of DEFINE_STR() in
arch/x86/um/shared/sysdep/kernel-offsets.h and sed-y in Kbuild doesn't
support string literals consisting of multiple parts. Hence for all
UM_KERN_<LEVEL> definitions, only the SOH character is retained in the actual
definition, while the remainder ends up in the comment. E.g. in
include/generated/asm-offsets.h we get

    #define UM_KERN_INFO "\001" /* "6" KERN_INFO */

instead of

    #define UM_KERN_INFO "\001" "6" /* KERN_INFO */

This causes spurious '^A' output in some kernel messages:

    Calibrating delay loop... 4640.76 BogoMIPS (lpj=23203840)
    pid_max: default: 32768 minimum: 301
    Mount-cache hash table entries: 256
    ^AChecking that host ptys support output SIGIO...Yes
    ^AChecking that host ptys support SIGIO on close...No, enabling workaround
    ^AUsing 2.6 host AIO
    NET: Registered protocol family 16
    bio: create slab <bio-0> at 0
    Switching to clocksource itimer

To fix this:
  - Move the mapping from UM_KERN_<LEVEL> to KERN_<LEVEL> from
    arch/um/include/shared/common-offsets.h to
    arch/um/include/shared/user.h, which is preincluded for all userspace
    parts,
  - Preinclude include/linux/kern_levels.h for all userspace parts, to
    obtain the in-kernel KERN_<LEVEL> constant definitions. This doesn't
    violate the kernel/userspace separation, as include/linux/kern_levels.h
    is self-contained and doesn't expose any other kernel internals.
  - Remove the now unused STR() and DEFINE_STR() macros.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
This fixes a regression from the KERN_<LEVEL> conversion

 arch/um/include/shared/common-offsets.h    |   10 ----------
 arch/um/include/shared/user.h              |   11 +++++++++++
 arch/um/scripts/Makefile.rules             |    2 +-
 arch/x86/um/shared/sysdep/kernel-offsets.h |    3 ---
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/um/include/shared/common-offsets.h b/arch/um/include/shared/common-offsets.h
index 40db8f7..2df313b 100644
--- a/arch/um/include/shared/common-offsets.h
+++ b/arch/um/include/shared/common-offsets.h
@@ -7,16 +7,6 @@ DEFINE(UM_KERN_PAGE_MASK, PAGE_MASK);
 DEFINE(UM_KERN_PAGE_SHIFT, PAGE_SHIFT);
 DEFINE(UM_NSEC_PER_SEC, NSEC_PER_SEC);
 
-DEFINE_STR(UM_KERN_EMERG, KERN_EMERG);
-DEFINE_STR(UM_KERN_ALERT, KERN_ALERT);
-DEFINE_STR(UM_KERN_CRIT, KERN_CRIT);
-DEFINE_STR(UM_KERN_ERR, KERN_ERR);
-DEFINE_STR(UM_KERN_WARNING, KERN_WARNING);
-DEFINE_STR(UM_KERN_NOTICE, KERN_NOTICE);
-DEFINE_STR(UM_KERN_INFO, KERN_INFO);
-DEFINE_STR(UM_KERN_DEBUG, KERN_DEBUG);
-DEFINE_STR(UM_KERN_CONT, KERN_CONT);
-
 DEFINE(UM_ELF_CLASS, ELF_CLASS);
 DEFINE(UM_ELFCLASS32, ELFCLASS32);
 DEFINE(UM_ELFCLASS64, ELFCLASS64);
diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
index 4fa82c0..cef0685 100644
--- a/arch/um/include/shared/user.h
+++ b/arch/um/include/shared/user.h
@@ -26,6 +26,17 @@
 extern void panic(const char *fmt, ...)
 	__attribute__ ((format (printf, 1, 2)));
 
+/* Requires preincluding include/linux/kern_levels.h */
+#define UM_KERN_EMERG	KERN_EMERG
+#define UM_KERN_ALERT	KERN_ALERT
+#define UM_KERN_CRIT	KERN_CRIT
+#define UM_KERN_ERR	KERN_ERR
+#define UM_KERN_WARNING	KERN_WARNING
+#define UM_KERN_NOTICE	KERN_NOTICE
+#define UM_KERN_INFO	KERN_INFO
+#define UM_KERN_DEBUG	KERN_DEBUG
+#define UM_KERN_CONT	KERN_CONT
+
 #ifdef UML_CONFIG_PRINTK
 extern int printk(const char *fmt, ...)
 	__attribute__ ((format (printf, 1, 2)));
diff --git a/arch/um/scripts/Makefile.rules b/arch/um/scripts/Makefile.rules
index d50270d..15889df 100644
--- a/arch/um/scripts/Makefile.rules
+++ b/arch/um/scripts/Makefile.rules
@@ -8,7 +8,7 @@ USER_OBJS += $(filter %_user.o,$(obj-y) $(obj-m)  $(USER_SINGLE_OBJS))
 USER_OBJS := $(foreach file,$(USER_OBJS),$(obj)/$(file))
 
 $(USER_OBJS:.o=.%): \
-	c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include user.h $(CFLAGS_$(basetarget).o)
+	c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include $(srctree)/include/linux/kern_levels.h -include user.h $(CFLAGS_$(basetarget).o)
 
 # These are like USER_OBJS but filter USER_CFLAGS through unprofile instead of
 # using it directly.
diff --git a/arch/x86/um/shared/sysdep/kernel-offsets.h b/arch/x86/um/shared/sysdep/kernel-offsets.h
index 5868526..46a9df9 100644
--- a/arch/x86/um/shared/sysdep/kernel-offsets.h
+++ b/arch/x86/um/shared/sysdep/kernel-offsets.h
@@ -7,9 +7,6 @@
 #define DEFINE(sym, val) \
 	asm volatile("\n->" #sym " %0 " #val : : "i" (val))
 
-#define STR(x) #x
-#define DEFINE_STR(sym, val) asm volatile("\n->" #sym " " STR(val) " " #val: : )
-
 #define BLANK() asm volatile("\n->" : : )
 
 #define OFFSET(sym, str, mem) \
-- 
1.7.0.4

Re: [PATCH/resend/bypass] um: Preinclude include/linux/kern_levels.h

[Linus Torvalds]

On Tue, Sep 25, 2012 at 12:11 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> To fix this:
>   - Move the mapping from UM_KERN_<LEVEL> to KERN_<LEVEL> from
>     arch/um/include/shared/common-offsets.h to
>     arch/um/include/shared/user.h, which is preincluded for all userspace
>     parts,
>   - Preinclude include/linux/kern_levels.h for all userspace parts, to
>     obtain the in-kernel KERN_<LEVEL> constant definitions. This doesn't
>     violate the kernel/userspace separation, as include/linux/kern_levels.h
>     is self-contained and doesn't expose any other kernel internals.
>   - Remove the now unused STR() and DEFINE_STR() macros.

Ugh.

Why do you preinclude kern_levels.h instead of just having a
"#include" in user.h?

IOW, this part of the patch:

-       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include user.h
$(CFLAGS_$(basetarget).o)
+       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include
$(srctree)/include/linux/kern_levels.h -include user.h
$(CFLAGS_$(basetarget).o)

just makes me go want to puke. The user.h file already has other
#include's in it, so I really don't see why you create this insane
special case.

And why does UM have those "UM_KERN_XYZ" defines in the first place?
Why isn't it just using KERN_XYZ directly? Is it because kern_levels.h
didn't use to exist, so it was some kind of "let's create our own that
we can hide in our special headers".

IOW, I really thinks this patch makes things uglier. At the very least
it could be done more prettily, but preferably we'd get rid of the odd
and useless UM_ prefix from these things entirely.

             Linus

Re: [PATCH/resend/bypass] um: Preinclude include/linux/kern_levels.h

[Al Viro]

On Tue, Sep 25, 2012 at 12:20:55PM -0700, Linus Torvalds wrote:
> IOW, this part of the patch:
> 
> -       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include user.h
> $(CFLAGS_$(basetarget).o)
> +       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include
> $(srctree)/include/linux/kern_levels.h -include user.h
> $(CFLAGS_$(basetarget).o)
> 
> just makes me go want to puke. The user.h file already has other
> #include's in it, so I really don't see why you create this insane
> special case.
> 
> And why does UM have those "UM_KERN_XYZ" defines in the first place?
> Why isn't it just using KERN_XYZ directly? Is it because kern_levels.h
> didn't use to exist, so it was some kind of "let's create our own that
> we can hide in our special headers".

Because user.h is included *without* kernel headers in include path.
It's for the stuff that is compiled with host libc headers.  Keep in
mind that UML talks to libc like normal architecture would talk to
hardware.  IOW, analogs of asm glue are in (host) userland C.  And
they need libc headers instead of the kernel ones.  That's what that
USER_OBJ thing is about.  Kernel-side constants, etc. are delivered
to that sucker using the same mechanism we normally use to give them
to assembler - asm-offsets.c.  And here, of course, slapping ifndef
__ASSEMBLER__ around the tricky bits will not work - the header itself
is just fine, but getting kernel headers in the search path really
isn't.

	I agree that proposed solution is ugly.  What we can do is copy
the damn header into include/generated and #include <generated/kern_levels.h>
from user.h.  And kill UM_KERN_... stuff.  Objections?

Re: [PATCH/resend/bypass] um: Preinclude include/linux/kern_levels.h

[Geert Uytterhoeven]

On Tue, Sep 25, 2012 at 9:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Sep 25, 2012 at 12:20:55PM -0700, Linus Torvalds wrote:
>> IOW, this part of the patch:
>>
>> -       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include user.h
>> $(CFLAGS_$(basetarget).o)
>> +       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include
>> $(srctree)/include/linux/kern_levels.h -include user.h
>> $(CFLAGS_$(basetarget).o)
>>
>> just makes me go want to puke. The user.h file already has other
>> #include's in it, so I really don't see why you create this insane
>> special case.
>>
>> And why does UM have those "UM_KERN_XYZ" defines in the first place?
>> Why isn't it just using KERN_XYZ directly? Is it because kern_levels.h
>> didn't use to exist, so it was some kind of "let's create our own that
>> we can hide in our special headers".
>
> Because user.h is included *without* kernel headers in include path.

Indeed.

> It's for the stuff that is compiled with host libc headers.  Keep in
> mind that UML talks to libc like normal architecture would talk to
> hardware.  IOW, analogs of asm glue are in (host) userland C.  And
> they need libc headers instead of the kernel ones.  That's what that
> USER_OBJ thing is about.  Kernel-side constants, etc. are delivered
> to that sucker using the same mechanism we normally use to give them
> to assembler - asm-offsets.c.  And here, of course, slapping ifndef
> __ASSEMBLER__ around the tricky bits will not work - the header itself
> is just fine, but getting kernel headers in the search path really
> isn't.
>
>         I agree that proposed solution is ugly.  What we can do is copy
> the damn header into include/generated and #include <generated/kern_levels.h>
> from user.h.  And kill UM_KERN_... stuff.  Objections?

My first submission had "We may convert all UM_KERN_* users to KERN_*
and drop the extra defines?" as a suggestion, but so far I haven't found time
to implement that...

Still, no one came up with a better patch, and this is a regression.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/resend/bypass] um: Preinclude include/linux/kern_levels.h

[Richard Weinberger]

Am Tue, 25 Sep 2012 22:37:13 +0200
schrieb Geert Uytterhoeven <geert@linux-m68k.org>:

> On Tue, Sep 25, 2012 at 9:43 PM, Al Viro <viro@zeniv.linux.org.uk>
> wrote:
> > On Tue, Sep 25, 2012 at 12:20:55PM -0700, Linus Torvalds wrote:
> >> IOW, this part of the patch:
> >>
> >> -       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include user.h
> >> $(CFLAGS_$(basetarget).o)
> >> +       c_flags = -Wp,-MD,$(depfile) $(USER_CFLAGS) -include
> >> $(srctree)/include/linux/kern_levels.h -include user.h
> >> $(CFLAGS_$(basetarget).o)
> >>
> >> just makes me go want to puke. The user.h file already has other
> >> #include's in it, so I really don't see why you create this insane
> >> special case.
> >>
> >> And why does UM have those "UM_KERN_XYZ" defines in the first
> >> place? Why isn't it just using KERN_XYZ directly? Is it because
> >> kern_levels.h didn't use to exist, so it was some kind of "let's
> >> create our own that we can hide in our special headers".
> >
> > Because user.h is included *without* kernel headers in include path.
> 
> Indeed.
> 
> > It's for the stuff that is compiled with host libc headers.  Keep in
> > mind that UML talks to libc like normal architecture would talk to
> > hardware.  IOW, analogs of asm glue are in (host) userland C.  And
> > they need libc headers instead of the kernel ones.  That's what that
> > USER_OBJ thing is about.  Kernel-side constants, etc. are delivered
> > to that sucker using the same mechanism we normally use to give them
> > to assembler - asm-offsets.c.  And here, of course, slapping ifndef
> > __ASSEMBLER__ around the tricky bits will not work - the header
> > itself is just fine, but getting kernel headers in the search path
> > really isn't.
> >
> >         I agree that proposed solution is ugly.  What we can do is
> > copy the damn header into include/generated and #include
> > <generated/kern_levels.h> from user.h.  And kill UM_KERN_...
> > stuff.  Objections?
> 
> My first submission had "We may convert all UM_KERN_* users to KERN_*
> and drop the extra defines?" as a suggestion, but so far I haven't
> found time to implement that...
> 
> Still, no one came up with a better patch, and this is a regression.

Yeah, I'd like to take the "ugly" patch to get rid of the regresion.
Later we can get rid of UM_KERN_*, which is IMHO also very ugly.

Thanks,
//richard