[PATCH kvmtool] arm: Allow command line for firmware

[PATCH kvmtool] arm: Allow command line for firmware

[Andre Przywara]

When loading a firmware instead of a kernel, we can still pass on any
*user-provided* command line, as /chosen/bootargs is a generic device tree
feature. We just need to make sure to not pass our mangled-for-Linux
version.

This allows to run "firmware" images which make use of a command line,
still are not Linux kernels, like kvm-unit-tests.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi Will,

this goes on top of Julien's firmware series (which did not yet appear
on kernel.org?)
This fixes an issue with some kvm-unit-tests support. [1]

Cheers,
Andre.

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2019-January/034251.html

 arm/fdt.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index e3a7a647..a1f8e912 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -163,14 +163,11 @@ static int setup_fdt(struct kvm *kvm)
 	_FDT(fdt_begin_node(fdt, "chosen"));
 	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
 
+	/* Pass on our amended command line to a Linux kernel only. */
 	if (kvm->cfg.firmware_filename) {
-		/*
-		 * When using a firmware, command line is not passed through DT,
-		 * or the firmware can add it itself
-		 */
 		if (kvm->cfg.kernel_cmdline)
-			pr_warning("Ignoring custom bootargs: %s\n",
-				   kvm->cfg.kernel_cmdline);
+			_FDT(fdt_property_string(fdt, "bootargs",
+						 kvm->cfg.kernel_cmdline));
 	} else
 		_FDT(fdt_property_string(fdt, "bootargs",
 					 kvm->cfg.real_cmdline));
-- 
2.17.1

Re: [PATCH kvmtool] arm: Allow command line for firmware

[Will Deacon]

On Fri, Jan 25, 2019 at 03:43:08PM +0000, Andre Przywara wrote:
> When loading a firmware instead of a kernel, we can still pass on any
> *user-provided* command line, as /chosen/bootargs is a generic device tree
> feature. We just need to make sure to not pass our mangled-for-Linux
> version.
> 
> This allows to run "firmware" images which make use of a command line,
> still are not Linux kernels, like kvm-unit-tests.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi Will,
> 
> this goes on top of Julien's firmware series (which did not yet appear
> on kernel.org?)
> This fixes an issue with some kvm-unit-tests support. [1]

Does kvm-unit-tests break if we pass the modified command line? I'm wary of
passing something different depending on whether the payload is firmware or
kernel, because there's a pretty fine line between the two (and the firmware
may even just forward the thing on to the kernel it loads).

Will

Re: [PATCH kvmtool] arm: Allow command line for firmware

[Andrew Jones]

On Wed, Jan 30, 2019 at 06:20:10PM +0000, Will Deacon wrote:
> On Fri, Jan 25, 2019 at 03:43:08PM +0000, Andre Przywara wrote:
> > When loading a firmware instead of a kernel, we can still pass on any
> > *user-provided* command line, as /chosen/bootargs is a generic device tree
> > feature. We just need to make sure to not pass our mangled-for-Linux
> > version.
> > 
> > This allows to run "firmware" images which make use of a command line,
> > still are not Linux kernels, like kvm-unit-tests.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > Hi Will,
> > 
> > this goes on top of Julien's firmware series (which did not yet appear
> > on kernel.org?)
> > This fixes an issue with some kvm-unit-tests support. [1]
> 
> Does kvm-unit-tests break if we pass the modified command line? I'm wary of
> passing something different depending on whether the payload is firmware or
> kernel, because there's a pretty fine line between the two (and the firmware
> may even just forward the thing on to the kernel it loads).
>

kvm-unit-tests assumes the user or unit test run scripts completely
control the kernel command line. The kernel command line is then
turned into the command line of the test's main() function, plus the
addition of the program name at argv[0]. The unit tests then parse
these command line parameters to determine what to do when testing.
If additional options are passed we need to teach the tests to ignore
them, and there's also risk that something passed in might accidentally
match a unit test parameter.

Thanks,
drew

Re: [PATCH kvmtool] arm: Allow command line for firmware

[Andre Przywara]

On Thu, 31 Jan 2019 14:07:15 +0100
Andrew Jones <drjones@redhat.com> wrote:

> On Wed, Jan 30, 2019 at 06:20:10PM +0000, Will Deacon wrote:
> > On Fri, Jan 25, 2019 at 03:43:08PM +0000, Andre Przywara wrote:  
> > > When loading a firmware instead of a kernel, we can still pass on
> > > any *user-provided* command line, as /chosen/bootargs is a
> > > generic device tree feature. We just need to make sure to not
> > > pass our mangled-for-Linux version.
> > > 
> > > This allows to run "firmware" images which make use of a command
> > > line, still are not Linux kernels, like kvm-unit-tests.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > > Hi Will,
> > > 
> > > this goes on top of Julien's firmware series (which did not yet
> > > appear on kernel.org?)
> > > This fixes an issue with some kvm-unit-tests support. [1]  
> > 
> > Does kvm-unit-tests break if we pass the modified command line? I'm
> > wary of passing something different depending on whether the
> > payload is firmware or kernel, because there's a pretty fine line
> > between the two (and the firmware may even just forward the thing
> > on to the kernel it loads). 
> 
> kvm-unit-tests assumes the user or unit test run scripts completely
> control the kernel command line. The kernel command line is then
> turned into the command line of the test's main() function, plus the
> addition of the program name at argv[0]. The unit tests then parse
> these command line parameters to determine what to do when testing.
> If additional options are passed we need to teach the tests to ignore
> them, and there's also risk that something passed in might
> accidentally match a unit test parameter.

Thanks Drew, was about to mention this as well.
In general I find kvmtool a bit presumptuous in assuming that every
guest kernel responses to Linux command line options. I see that
kvmtool originated as a Linux debug tool, but running *BSD or Xen (with
NV) doesn't sound too far off to me.

There was this idea of introducing something like an --expert option,
to tell kvmtool explicitly to omit any generated command line
parameters. I then thought we could just piggy back on --firmware,
which somewhat carries this kind of semantic anyway.
And since we nuke every command line for --firmware right now, passing
on the user provided one seems like the easiest.

Those automated kvmtool features are somewhat dodgy anyway: for instance
it drops the neat 9pfs forwaring when you specify a disk image, and you
can't bring it back explicitly.

Maybe --kernel and --firmware are just misleading names, it should be
--linux and --kernel instead? But I guess we can't change this anymore.

Cheers,
Andre.

Re: [PATCH kvmtool] arm: Allow command line for firmware

[Julien Thierry]

On 30/01/2019 18:20, Will Deacon wrote:
> On Fri, Jan 25, 2019 at 03:43:08PM +0000, Andre Przywara wrote:
>> When loading a firmware instead of a kernel, we can still pass on any
>> *user-provided* command line, as /chosen/bootargs is a generic device tree
>> feature. We just need to make sure to not pass our mangled-for-Linux
>> version.
>>
>> This allows to run "firmware" images which make use of a command line,
>> still are not Linux kernels, like kvm-unit-tests.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> Hi Will,
>>
>> this goes on top of Julien's firmware series (which did not yet appear
>> on kernel.org?)
>> This fixes an issue with some kvm-unit-tests support. [1]
> 
> Does kvm-unit-tests break if we pass the modified command line? I'm wary of
> passing something different depending on whether the payload is firmware or
> kernel, because there's a pretty fine line between the two (and the firmware
> may even just forward the thing on to the kernel it loads).
> 

Yes, this is why I removed it initially for the firmware case.

In the EFI case, the DT is just passed as is to Linux, however the Linux
EFI stub retrieves the command line from EFI, and the command line in
the DT is ignored. So to avoid confusion, I wanted to prevent passing a
command line that just gets ignored.

However the command line property of the chosen node is not linux
specific and some other OS/firmware/bootloader could rely on it. So I'm
not sure what's the best move here.

Cheers,

-- 
Julien Thierry