diff mbox

[RFC,V1,1/2] arm_boot: added linux switch

Message ID f7256b2be069a80037b977e3f66f4d51418e35d0.1338512893.git.peter.crosthwaite@petalogix.com
State New
Headers show

Commit Message

Peter A. G. Crosthwaite June 1, 2012, 1:16 a.m. UTC
Added a switch to tell the bootloader that the image is linux and should be
bootstrapped as such. This is needed to boot an elf that is linux.

Syntax would be:

qemu-system-arm ... -kernel linux.elf -machine linux=on

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/arm_boot.c |    1 +
 qemu-config.c |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

Comments

Peter Maydell June 1, 2012, 1:41 a.m. UTC | #1
On 1 June 2012 02:16, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> Added a switch to tell the bootloader that the image is linux and should be
> bootstrapped as such. This is needed to boot an elf that is linux.
>
> Syntax would be:
>
> qemu-system-arm ... -kernel linux.elf -machine linux=on

This whole area is a mess. Personally I think we should have a
standard way to say "load this ELF file into memory" which isn't
target-dependent, and then we could make -kernel actually load
a kernel rather than its current "load a kernel unless it's an
ELF file in which case do something else".

There are some back-compatibility issues there, though.

-- PMM
Peter A. G. Crosthwaite June 1, 2012, 1:49 a.m. UTC | #2
On Fri, Jun 1, 2012 at 11:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2012 02:16, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> Added a switch to tell the bootloader that the image is linux and should be
>> bootstrapped as such. This is needed to boot an elf that is linux.
>>
>> Syntax would be:
>>
>> qemu-system-arm ... -kernel linux.elf -machine linux=on
>
> This whole area is a mess. Personally I think we should have a
> standard way to say "load this ELF file into memory" which isn't
> target-dependent, and then we could make -kernel actually load
> a kernel rather than its current "load a kernel unless it's an
> ELF file in which case do something else".
>

Ok,

So this sounds like -kernel should behave like is_linux=1. Then you
add -elf as the command line option to boot your random non-linux
elfs?

> There are some back-compatibility issues there, though.

Yes there are definitely back compat issues in that people are using
-kernel already for their random elfs. But if you want to keep it
clean aren't we just going to have to take the hit on the command line
syntax change?

>
> -- PMM
Andreas Färber June 1, 2012, 9:26 a.m. UTC | #3
Am 01.06.2012 03:16, schrieb Peter A. G. Crosthwaite:
> Added a switch to tell the bootloader that the image is linux and should be
> bootstrapped as such. This is needed to boot an elf that is linux.
> 
> Syntax would be:
> 
> qemu-system-arm ... -kernel linux.elf -machine linux=on
> 
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  hw/arm_boot.c |    1 +
>  qemu-config.c |    4 ++++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 7447f5c..8e25873b 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -320,6 +320,7 @@ void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)

Beware that this signature has changed on qom-next branch, but I don't
see a conflict here.

Andreas

>      machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>      if (machine_opts) {
>          info->dtb_filename = qemu_opt_get(machine_opts, "dtb");
> +        is_linux = qemu_opt_get_bool(machine_opts, "linux", 0) ? 1 : 0;
>      } else {
>          info->dtb_filename = NULL;
>      }
> diff --git a/qemu-config.c b/qemu-config.c
> index be84a03..0ee781c 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -582,6 +582,10 @@ static QemuOptsList qemu_machine_opts = {
>              .name = "dtb",
>              .type = QEMU_OPT_STRING,
>              .help = "Linux kernel device tree file",
> +        }, {
> +            .name = "linux",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Bootstrap Linux",
>          },
>          { /* End of list */ }
>      },
Peter A. G. Crosthwaite June 25, 2012, 1:51 a.m. UTC | #4
Ping! @PMM

You rejected my workaround in V2 (i.e. using -dtb to force is_linux)
on the grounds that youd "accept some reasonable way of saying "this
ELF file is a Linux kernel"," That brings us back to V1 - this patch.
Any objections?

Regards,
Peter

On Fri, Jun 1, 2012 at 11:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2012 02:16, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> Added a switch to tell the bootloader that the image is linux and should be
>> bootstrapped as such. This is needed to boot an elf that is linux.
>>
>> Syntax would be:
>>
>> qemu-system-arm ... -kernel linux.elf -machine linux=on
>
> This whole area is a mess. Personally I think we should have a
> standard way to say "load this ELF file into memory" which isn't
> target-dependent, and then we could make -kernel actually load
> a kernel rather than its current "load a kernel unless it's an
> ELF file in which case do something else".
>
> There are some back-compatibility issues there, though.
>
> -- PMM
Andreas Färber June 25, 2012, 7:31 a.m. UTC | #5
Am 25.06.2012 03:51, schrieb Peter Crosthwaite:
> Ping! @PMM
> 
> You rejected my workaround in V2 (i.e. using -dtb to force is_linux)
> on the grounds that youd "accept some reasonable way of saying "this
> ELF file is a Linux kernel"," That brings us back to V1 - this patch.
> Any objections?

I won't object to it, but I don't like it: If it's really needed, it
should be os=linux or something (enum?) for extensibility or at some
point we'll need windows=on, vxworks=on, aix=on, etc.

Andreas

> 
> Regards,
> Peter
> 
> On Fri, Jun 1, 2012 at 11:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 June 2012 02:16, Peter A. G. Crosthwaite
>> <peter.crosthwaite@petalogix.com> wrote:
>>> Added a switch to tell the bootloader that the image is linux and should be
>>> bootstrapped as such. This is needed to boot an elf that is linux.
>>>
>>> Syntax would be:
>>>
>>> qemu-system-arm ... -kernel linux.elf -machine linux=on
>>
>> This whole area is a mess. Personally I think we should have a
>> standard way to say "load this ELF file into memory" which isn't
>> target-dependent, and then we could make -kernel actually load
>> a kernel rather than its current "load a kernel unless it's an
>> ELF file in which case do something else".
>>
>> There are some back-compatibility issues there, though.
>>
>> -- PMM
>
Peter A. G. Crosthwaite June 25, 2012, 7:42 a.m. UTC | #6
On Mon, Jun 25, 2012 at 5:31 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 25.06.2012 03:51, schrieb Peter Crosthwaite:
>> Ping! @PMM
>>
>> You rejected my workaround in V2 (i.e. using -dtb to force is_linux)
>> on the grounds that youd "accept some reasonable way of saying "this
>> ELF file is a Linux kernel"," That brings us back to V1 - this patch.
>> Any objections?
>
> I won't object to it, but I don't like it: If it's really needed, it
> should be os=linux

String machine opt os=foo work?

 or something (enum?) for extensibility or at some
> point we'll need windows=on, vxworks=on, aix=on, etc.
>
> Andreas
>
Peter Maydell June 25, 2012, 7:59 a.m. UTC | #7
On 25 June 2012 02:51, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> You rejected my workaround in V2 (i.e. using -dtb to force is_linux)
> on the grounds that youd "accept some reasonable way of saying "this
> ELF file is a Linux kernel"," That brings us back to V1 - this patch.
> Any objections?

I'd really like to see some input from people who deal with the
other QEMU architectures on whether we can come up with something
coherent as a plan for handling these loader command line options.
I don't really like "here's a switch but it happens to only
work on ARM targets" if we can avoid it.

-- PMM
Peter A. G. Crosthwaite June 25, 2012, 8:55 a.m. UTC | #8
On Mon, Jun 25, 2012 at 5:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 June 2012 02:51, Peter Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> You rejected my workaround in V2 (i.e. using -dtb to force is_linux)
>> on the grounds that youd "accept some reasonable way of saying "this
>> ELF file is a Linux kernel"," That brings us back to V1 - this patch.
>> Any objections?
>
> I'd really like to see some input from people who deal with the
> other QEMU architectures on whether we can come up with something
> coherent as a plan for handling these loader command line options.
> I don't really like "here's a switch but it happens to only
> work on ARM targets" if we can avoid it.
>
Well, this switch itself is generic. Theres no armisms in the machine
opt itself.

As for architectures, I can speak for Microblaze and the PPC
bootloader for virtex_ml507. The DTB will blob in regardless of
whether its an elf or not, so no is_linux switch is needed there for
my Linux elfs. There is no functional difference between linux and not
linux in those bootloaders.

> -- PMM
Peter Maydell June 25, 2012, 9:03 a.m. UTC | #9
On 25 June 2012 09:55, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> As for architectures, I can speak for Microblaze and the PPC
> bootloader for virtex_ml507. The DTB will blob in regardless of
> whether its an elf or not, so no is_linux switch is needed there for
> my Linux elfs. There is no functional difference between linux and not
> linux in those bootloaders.

Yes, it is exactly the "on architecture X we behave like this and
on architecture Y we do something else" that I dislike, and this is
why I'm not happy with any patch which is just trying to band-aid
a situation on architecture X rather than trying to address the
more general situation.

-- PMM
Peter A. G. Crosthwaite June 25, 2012, 9:19 a.m. UTC | #10
On Mon, Jun 25, 2012 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 June 2012 09:55, Peter Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> As for architectures, I can speak for Microblaze and the PPC
>> bootloader for virtex_ml507. The DTB will blob in regardless of
>> whether its an elf or not, so no is_linux switch is needed there for
>> my Linux elfs. There is no functional difference between linux and not
>> linux in those bootloaders.
>
> Yes, it is exactly the "on architecture X we behave like this and
> on architecture Y we do something else" that I dislike, and this is
> why I'm not happy with any patch which is just trying to band-aid
> a situation on architecture X rather than trying to address the
> more general situation.
>

Well if X and Y behave differently and you want to unify them, then
either X or Y loses backwards compatibility. Someone is going to have
their command line change on them.

Looking at some of the other bootloaders, I dont see others that
"assume elfs are not linux", so it seems to me that ARM is the
exception, not the rule. So from that unification would involve
getting rid of this ARM assumption that elfs are different. Just boot
the elf with the linux style bootloader like most other platforms.

> -- PMM
Peter A. G. Crosthwaite June 27, 2012, 4:38 a.m. UTC | #11
Ping!

Whats the action item here? Put out an RFC about unifying bootloaders
or some such?

Regards,
Peter

On Mon, Jun 25, 2012 at 7:19 PM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Mon, Jun 25, 2012 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 June 2012 09:55, Peter Crosthwaite
>> <peter.crosthwaite@petalogix.com> wrote:
>>> As for architectures, I can speak for Microblaze and the PPC
>>> bootloader for virtex_ml507. The DTB will blob in regardless of
>>> whether its an elf or not, so no is_linux switch is needed there for
>>> my Linux elfs. There is no functional difference between linux and not
>>> linux in those bootloaders.
>>
>> Yes, it is exactly the "on architecture X we behave like this and
>> on architecture Y we do something else" that I dislike, and this is
>> why I'm not happy with any patch which is just trying to band-aid
>> a situation on architecture X rather than trying to address the
>> more general situation.
>>
>
> Well if X and Y behave differently and you want to unify them, then
> either X or Y loses backwards compatibility. Someone is going to have
> their command line change on them.
>
> Looking at some of the other bootloaders, I dont see others that
> "assume elfs are not linux", so it seems to me that ARM is the
> exception, not the rule. So from that unification would involve
> getting rid of this ARM assumption that elfs are different. Just boot
> the elf with the linux style bootloader like most other platforms.
>
>> -- PMM
Peter Maydell June 27, 2012, 7:23 a.m. UTC | #12
On 27 June 2012 05:38, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> Ping!
>
> Whats the action item here? Put out an RFC about unifying bootloaders
> or some such?

Something like that -- identify what all the architectures currently
do with these command line arguments and propose something to bring
the outliers into line, I guess.

(We do still need the "just load an ELF" functionality but ideally
as something consistently provided for all archs.)

-- PMM
diff mbox

Patch

diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index 7447f5c..8e25873b 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -320,6 +320,7 @@  void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
     machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
     if (machine_opts) {
         info->dtb_filename = qemu_opt_get(machine_opts, "dtb");
+        is_linux = qemu_opt_get_bool(machine_opts, "linux", 0) ? 1 : 0;
     } else {
         info->dtb_filename = NULL;
     }
diff --git a/qemu-config.c b/qemu-config.c
index be84a03..0ee781c 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -582,6 +582,10 @@  static QemuOptsList qemu_machine_opts = {
             .name = "dtb",
             .type = QEMU_OPT_STRING,
             .help = "Linux kernel device tree file",
+        }, {
+            .name = "linux",
+            .type = QEMU_OPT_BOOL,
+            .help = "Bootstrap Linux",
         },
         { /* End of list */ }
     },