Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Tue, 5 Dec 2023 11:25:03 +0100
From:   Petr Malat <oss@malat.biz>
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     linux-kernel@vger.kernel.org, paulmck@kernel.org,
        rostedt@goodmis.org
Subject: Re: [PATCH 2/2] bootconfig: Apply early options from embedded config
Message-ID: <ZW76f0Rt8TM0h56I@ntb.petris.klfree.czf>
References: <20231121231342.193646-1-oss@malat.biz>
 <20231121231342.193646-3-oss@malat.biz>
 <20231123194106.08f5832f558fe806b1fd8098@kernel.org>
 <ZWADKWURCDZXyJTQ@ntb.petris.klfree.czf>
 <20231127074630.993db80be06f03067d8a1aef@kernel.org>
 <ZWSvfgFFu+ILKN4H@ntb.petris.klfree.czf>
 <20231128071355.1fd8b2857f4622bd111d8647@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231128071355.1fd8b2857f4622bd111d8647@kernel.org>
Precedence: bulk

Hi!
Sorry for late reply, I didn't have much time to work on this the last week.

On Tue, Nov 28, 2023 at 07:13:55AM +0900, Masami Hiramatsu wrote:
> On Mon, 27 Nov 2023 16:02:22 +0100
> Petr Malat <oss@malat.biz> wrote:
> 
> > Hi Masami,
> > On Mon, Nov 27, 2023 at 07:46:30AM +0900, Masami Hiramatsu wrote:
> > 
> > Shortened the mail as this seems to be the last open point
> > 
> > > > > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > > > > should be removed.
> > > > 
> > > > I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> > > > compatible change and I didn't want to risk breaking current use cases.
> > > > My change tries to get early options working without affecting how
> > > > other options are handled, but I think appending the config is more
> > > > reasonable behavior and if you do not see it as a problem to not be
> > > > backward compatible here, I will delete the "replace" behavior.
> > > 
> > > That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
> > > it must skip setting early_params to avoid "hidden setting" from the
> > > embedded bootconfig.
> >  
> > That's not a good idea because then disabling BOOT_CONFIG_EMBED_APPEND_INITRD
> > would disable early options handling even if the user doesn't use initrd at
> > all, which we do not want.
> 
> Yes, so the config name and comment needs to be updated. The problematic point
> is that we can not know where there is an initrd (and bootconfig on initrd)
> until parsing/applying the early params. And we have to avoid setting "hidden"
> parameter.
> 
> > 
> > I suggest logging a KERN_NOTICE message if any early option was applied and
> > at the same time embedded bootconfig was replaced.
> 
> No, that's no good because kernel log can be cleared.
> Usually user will check /proc/cmdline to check what parameters are set, so at
> least it needs to be updated, but also, we need to keep the original cmdline
> for make sure the user can reuse it for kexec. To solve this issue without
> contradiction, we need to ask user to set BOOT_CONFIG_EMBED_APPEND_INITRD=y
> to support early params in bootconfig. (Thus it will be something like
> "BOOT_CONFIG_EMBED_EARLY_PARAM")

I made a bit of prototyping and I can add applied early options from embedded
config to /proc/cmdline even when the embedded config is replaced. I don't like
applying these options conditionally, because that leads to confusing behavior
of seeing these options in /proc/cmdline and /proc/bootconfig without actually
applying them. I find that worst than the opposite behavior as if user sets
options in the config and then sees them in both cmdline and bootconfig, he
assumes they have been applied.

We can also introduce BOOT_CONFIG_EMBED_REPLACE_INITRD for backward compatibility
and write that this option will be removed in the future. If nobody complains
for a while, we drop it. Then we are left with the appending only.
  Petr