* [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel
@ 2016-09-10 15:07 Michael Olbrich
2016-09-22 16:23 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Michael Olbrich @ 2016-09-10 15:07 UTC (permalink / raw)
To: peter.maydell, pbonzini; +Cc: qemu-arm, qemu-devel, Michael Olbrich
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.
Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
---
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;
}
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);
- }
-
if (semihosting_enabled() && !semihosting_get_argc() && kernel_filename) {
/* fall back to the -kernel/-append */
semihosting_arg_fallback(kernel_filename, kernel_cmdline);
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel
2016-09-10 15:07 [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel Michael Olbrich
@ 2016-09-22 16:23 ` Peter Maydell
2016-09-23 6:33 ` Michael Olbrich
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2016-09-22 16:23 UTC (permalink / raw)
To: Michael Olbrich; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers
On 10 September 2016 at 16:07, Michael Olbrich <m.olbrich@pengutronix.de> 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 <m.olbrich@pengutronix.de>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel
2016-09-22 16:23 ` Peter Maydell
@ 2016-09-23 6:33 ` Michael Olbrich
2016-09-30 1:58 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Michael Olbrich @ 2016-09-23 6:33 UTC (permalink / raw)
To: Peter Maydell; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers
On Thu, Sep 22, 2016 at 05:23:17PM +0100, Peter Maydell wrote:
> On 10 September 2016 at 16:07, Michael Olbrich <m.olbrich@pengutronix.de> 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 <m.olbrich@pengutronix.de>
> > ---
> > 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-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel
2016-09-23 6:33 ` Michael Olbrich
@ 2016-09-30 1:58 ` Peter Maydell
2016-10-07 14:09 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2016-09-30 1:58 UTC (permalink / raw)
To: Michael Olbrich; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers
On 22 September 2016 at 23:33, Michael Olbrich <m.olbrich@pengutronix.de> wrote:
> On Thu, Sep 22, 2016 at 05:23:17PM +0100, Peter Maydell wrote:
>> On 10 September 2016 at 16:07, Michael Olbrich <m.olbrich@pengutronix.de> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel
2016-09-30 1:58 ` Peter Maydell
@ 2016-10-07 14:09 ` Stefan Hajnoczi
2016-10-07 14:33 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2016-10-07 14:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: Michael Olbrich, Paolo Bonzini, qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]
On Thu, Sep 29, 2016 at 06:58:37PM -0700, Peter Maydell wrote:
> On 22 September 2016 at 23:33, Michael Olbrich <m.olbrich@pengutronix.de> wrote:
> > On Thu, Sep 22, 2016 at 05:23:17PM +0100, Peter Maydell wrote:
> >> On 10 September 2016 at 16:07, Michael Olbrich <m.olbrich@pengutronix.de> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel
2016-10-07 14:09 ` Stefan Hajnoczi
@ 2016-10-07 14:33 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2016-10-07 14:33 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Michael Olbrich, Paolo Bonzini, qemu-arm, QEMU Developers
On 7 October 2016 at 15:09, Stefan Hajnoczi <stefanha@gmail.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-07 14:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-10 15:07 [Qemu-devel] [PATCH] hw/arm/boot: allow using a command line specified dtb without a kernel Michael Olbrich
2016-09-22 16:23 ` Peter Maydell
2016-09-23 6:33 ` Michael Olbrich
2016-09-30 1:58 ` Peter Maydell
2016-10-07 14:09 ` Stefan Hajnoczi
2016-10-07 14:33 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.