Messages in this thread | | | Date | Thu, 12 May 2022 13:21:12 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v3 2/2] x86: Parse CONFIG_CMDLINE in compressed kernel |
| |
On Thu, May 05, 2022 at 01:32:24PM +0300, Baskov Evgeniy wrote:
Same note on the subject format as for your previous patch.
> CONFIG_CMDLINE, CONFIG_CMDLINE_BOOL, and CONFIG_CMDLINE_OVERRIDE were > ignored during options lookup in compressed kernel. > > Parse CONFIG_CMDLINE-related options correctly in compressed kernel > code. > > cmd_line_ptr_init is explicitly placed in .data section since it is > used and expected to be equal to zero before .bss section is cleared.
What I'm missing in this commit message is the use case which you have in your 0/2 mail.
Also, to the tone of your commit messages, from Documentation/process/submitting-patches.rst:
"Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour."
Also, do not talk about what your patch does - that should hopefully be visible in the diff itself. Rather, talk about *why* you're doing what you're doing.
> Signed-off-by: Baskov Evgeniy <baskov@ispras.ru> > > diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c > index f1add5d85da9..261f53ad395a 100644 > --- a/arch/x86/boot/compressed/cmdline.c > +++ b/arch/x86/boot/compressed/cmdline.c > @@ -1,6 +1,8 @@ > // SPDX-License-Identifier: GPL-2.0 > #include "misc.h" > > +#define COMMAND_LINE_SIZE 2048 > + > static unsigned long fs; > static inline void set_fs(unsigned long seg) > { > @@ -12,12 +14,32 @@ static inline char rdfs8(addr_t addr) > return *((char *)(fs + addr)); > } > #include "../cmdline.c" > + > +#ifdef CONFIG_CMDLINE_BOOL > +static char builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE; > +static bool builtin_cmdline_init __section(".data"); > +#endif > + > unsigned long get_cmd_line_ptr(void) > { > unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr; > - > cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32; > > +#ifdef CONFIG_CMDLINE_BOOL > + if (!builtin_cmdline_init) { > + if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { > + strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > + strlcat(builtin_cmdline, > + (char *)cmd_line_ptr, > + COMMAND_LINE_SIZE); > + } > + > + builtin_cmdline_init = 1; > + } > + > + cmd_line_ptr = (unsigned long)builtin_cmdline; > +#endif
I had asked this already but let me try again: instead of copying this from kernel proper, why don't you add a common helper which you call from both locations?
And it is not like this is going to be a huge function so you can stick it into a shared header in arch/x86/include/asm/shared/ and it'll get inlined into both locations...
Thx.
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |