Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel

[Peter Maydell]

On 7 October 2016 at 15:09, Stefan Hajnoczi  wrote:
> On Thu, Sep 29, 2016 at 06:58:37PM -0700, Peter Maydell wrote:
>> I think we have two choices:
>> (1) just go ahead and remove the error-check, on the basis that:
>>  * for some boards -dtb is useful even without -kernel
>>  * -dtb might be ignored even with -kernel if the specified
>>kernel isn't a DTB-aware kernel, but we ignore that
>>  * -dtb is ignored even with -kernel for target archs/boards
>>which don't support or use DTB, and we don't warn about that
>>  * we don't warn about -kernel being useless for target boards
>>that don't pay any attention to it
>> (2) add some kind of field to MachineClass indicating whether
>>the machine can handle dtb files with/without a kernel
>>(and perhaps also whether the machine supports -kernel at all),
>>use that to gate the warning messages, and update all the
>>machines to correctly indicate what they can or can't handle.
>>This would let us give warning messages when the user asks
>>for something we're going to ignore (including letting us
>>fix up some of the cases we don't currently deal with as
>>enumerated above), but it would be a fair chunk of effort
>>for a fairly small user-friendliness gain
>>
>> Thinking about it more, I'm inclining towards the simpler
>> option (1). Paolo, do you have an opinion here ?
>
> The error check doesn't seem worth the effort.  It's a convenience
> message to notify users that their configuration is broken but we can't
> detect all the cases where it's broken.  It doesn't seem like a good
> business to be in :).

I agree, so let's apply this patch as-is; added to target-arm.next.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel

[Stefan Hajnoczi]

On Thu, Sep 29, 2016 at 06:58:37PM -0700, Peter Maydell wrote:
> On 22 September 2016 at 23:33, Michael Olbrich  
> wrote:
> > On Thu, Sep 22, 2016 at 05:23:17PM +0100, Peter Maydell wrote:
> >> On 10 September 2016 at 16:07, Michael Olbrich  
> >> wrote:
> >> > diff --git a/vl.c b/vl.c
> >> > index ee557a1d3f8a..bbea51e0ce7d 100644
> >> > --- a/vl.c
> >> > +++ b/vl.c
> >> > @@ -4335,11 +4335,6 @@ int main(int argc, char **argv, char **envp)
> >> >  exit(1);
> >> >  }
> >> >
> >> > -if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
> >> > -error_report("-dtb only allowed with -kernel option");
> >> > -exit(1);
> >> > -}
> >> > -
> >>
> >> I can see why you want this change, but what worries me a little
> >> is that this is changing the behaviour of -dtb for all QEMU
> >> target architectures, not just ARM (they no longer get a helpful
> >> message on user error). I'm not sure how to address that, though.
> >
> > Would a 'if !arm' be possible or useful here?
> 
> It's not quite that simple :-)
> 
> I think we have two choices:
> (1) just go ahead and remove the error-check, on the basis that:
>  * for some boards -dtb is useful even without -kernel
>  * -dtb might be ignored even with -kernel if the specified
>kernel isn't a DTB-aware kernel, but we ignore that
>  * -dtb is ignored even with -kernel for target archs/boards
>which don't support or use DTB, and we don't warn about that
>  * we don't warn about -kernel being useless for target boards
>that don't pay any attention to it
> (2) add some kind of field to MachineClass indicating whether
>the machine can handle dtb files with/without a kernel
>(and perhaps also whether the machine supports -kernel at all),
>use that to gate the warning messages, and update all the
>machines to correctly indicate what they can or can't handle.
>This would let us give warning messages when the user asks
>for something we're going to ignore (including letting us
>fix up some of the cases we don't currently deal with as
>enumerated above), but it would be a fair chunk of effort
>for a fairly small user-friendliness gain
> 
> Thinking about it more, I'm inclining towards the simpler
> option (1). Paolo, do you have an opinion here ?

The error check doesn't seem worth the effort.  It's a convenience
message to notify users that their configuration is broken but we can't
detect all the cases where it's broken.  It doesn't seem like a good
business to be in :).

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel

[Peter Maydell]

On 22 September 2016 at 23:33, Michael Olbrich  wrote:
> On Thu, Sep 22, 2016 at 05:23:17PM +0100, Peter Maydell wrote:
>> On 10 September 2016 at 16:07, Michael Olbrich  
>> wrote:
>> > diff --git a/vl.c b/vl.c
>> > index ee557a1d3f8a..bbea51e0ce7d 100644
>> > --- a/vl.c
>> > +++ b/vl.c
>> > @@ -4335,11 +4335,6 @@ int main(int argc, char **argv, char **envp)
>> >  exit(1);
>> >  }
>> >
>> > -if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
>> > -error_report("-dtb only allowed with -kernel option");
>> > -exit(1);
>> > -}
>> > -
>>
>> I can see why you want this change, but what worries me a little
>> is that this is changing the behaviour of -dtb for all QEMU
>> target architectures, not just ARM (they no longer get a helpful
>> message on user error). I'm not sure how to address that, though.
>
> Would a 'if !arm' be possible or useful here?

It's not quite that simple :-)

I think we have two choices:
(1) just go ahead and remove the error-check, on the basis that:
 * for some boards -dtb is useful even without -kernel
 * -dtb might be ignored even with -kernel if the specified
   kernel isn't a DTB-aware kernel, but we ignore that
 * -dtb is ignored even with -kernel for target archs/boards
   which don't support or use DTB, and we don't warn about that
 * we don't warn about -kernel being useless for target boards
   that don't pay any attention to it
(2) add some kind of field to MachineClass indicating whether
   the machine can handle dtb files with/without a kernel
   (and perhaps also whether the machine supports -kernel at all),
   use that to gate the warning messages, and update all the
   machines to correctly indicate what they can or can't handle.
   This would let us give warning messages when the user asks
   for something we're going to ignore (including letting us
   fix up some of the cases we don't currently deal with as
   enumerated above), but it would be a fair chunk of effort
   for a fairly small user-friendliness gain

Thinking about it more, I'm inclining towards the simpler
option (1). Paolo, do you have an opinion here ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel

[Michael Olbrich]

On Thu, Sep 22, 2016 at 05:23:17PM +0100, Peter Maydell wrote:
> On 10 September 2016 at 16:07, Michael Olbrich  
> wrote:
> > When kernel and device tree are specified in the QEMU commandline, then
> > this device tree may be modified e.g. to add virtio_mmio devices.
> > With a bootloader e.g. on a flash device these extra devices are not
> > available.
> > With this change, the device tree can be specified at the QEMU commandline.
> > The modified device tree made available to the bootloader with the same
> > mechanism already supported by device trees fully generated by QEMU.
> 
> Would you mind explaining your usecase in a little more detail
> (for instance which machine model are you using) ?

I'm using qemu for system integration testing. Including some bootloader /
Linux kernel and userspace interaction. I'm currently using vexpress-a9 but
I plan to investigate if the new imx6 machine is suitable for my use-case.

In a setup without bootloader I already use a 9p rootfs. It's great for
debugging and a lot easier to set up than networking and a NFS server.

With this patch (and some bootloader patches to handle the device tree) I
can do the same when a bootloader is involved.

> > Signed-off-by: Michael Olbrich 
> > ---
> >  hw/arm/boot.c | 4 ++--
> >  vl.c  | 5 -
> >  2 files changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index 1b913a43ca65..942416d95a6f 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -773,6 +773,8 @@ static void arm_load_kernel_notify(Notifier *notifier, 
> > void *data)
> >   */
> >  assert(!(info->secure_board_setup && kvm_enabled()));
> >
> > +info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> > +
> >  /* Load the kernel.  */
> >  if (!info->kernel_filename || info->firmware_loaded) {
> >
> > @@ -833,8 +835,6 @@ static void arm_load_kernel_notify(Notifier *notifier, 
> > void *data)
> >  elf_machine = EM_ARM;
> >  }
> >
> > -info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> > -
> >  if (!info->secondary_cpu_reset_hook) {
> >  info->secondary_cpu_reset_hook = default_reset_secondary;
> >  }
> 
> This change definitely makes sense -- we check info->dtb_filename
> in have_dtb() so we need to set it before we call that function,
> not afterwards.
> 
> > diff --git a/vl.c b/vl.c
> > index ee557a1d3f8a..bbea51e0ce7d 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4335,11 +4335,6 @@ int main(int argc, char **argv, char **envp)
> >  exit(1);
> >  }
> >
> > -if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
> > -error_report("-dtb only allowed with -kernel option");
> > -exit(1);
> > -}
> > -
> 
> I can see why you want this change, but what worries me a little
> is that this is changing the behaviour of -dtb for all QEMU
> target architectures, not just ARM (they no longer get a helpful
> message on user error). I'm not sure how to address that, though.

Would a 'if !arm' be possible or useful here?

Michael

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel

[Peter Maydell]

On 10 September 2016 at 16:07, Michael Olbrich  wrote:
> When kernel and device tree are specified in the QEMU commandline, then
> this device tree may be modified e.g. to add virtio_mmio devices.
> With a bootloader e.g. on a flash device these extra devices are not
> available.
> With this change, the device tree can be specified at the QEMU commandline.
> The modified device tree made available to the bootloader with the same
> mechanism already supported by device trees fully generated by QEMU.

Would you mind explaining your usecase in a little more detail
(for instance which machine model are you using) ?

> Signed-off-by: Michael Olbrich 
> ---
>  hw/arm/boot.c | 4 ++--
>  vl.c  | 5 -
>  2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1b913a43ca65..942416d95a6f 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -773,6 +773,8 @@ static void arm_load_kernel_notify(Notifier *notifier, 
> void *data)
>   */
>  assert(!(info->secure_board_setup && kvm_enabled()));
>
> +info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> +
>  /* Load the kernel.  */
>  if (!info->kernel_filename || info->firmware_loaded) {
>
> @@ -833,8 +835,6 @@ static void arm_load_kernel_notify(Notifier *notifier, 
> void *data)
>  elf_machine = EM_ARM;
>  }
>
> -info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> -
>  if (!info->secondary_cpu_reset_hook) {
>  info->secondary_cpu_reset_hook = default_reset_secondary;
>  }

This change definitely makes sense -- we check info->dtb_filename
in have_dtb() so we need to set it before we call that function,
not afterwards.

> diff --git a/vl.c b/vl.c
> index ee557a1d3f8a..bbea51e0ce7d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4335,11 +4335,6 @@ int main(int argc, char **argv, char **envp)
>  exit(1);
>  }
>
> -if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
> -error_report("-dtb only allowed with -kernel option");
> -exit(1);
> -}
> -

I can see why you want this change, but what worries me a little
is that this is changing the behaviour of -dtb for all QEMU
target architectures, not just ARM (they no longer get a helpful
message on user error). I'm not sure how to address that, though.

thanks
-- PMM