[2/4] multiboot: load any machine type of ELF

Message ID 20171012235439.19457-3-anatol.pomozov@gmail.com
State New
Headers show
Series
  • [1/4] multiboot: Change multiboot_info from array of bytes to a C struct
Related show

Commit Message

Anatol Pomozov Oct. 12, 2017, 11:54 p.m.
x86 is not the only architecture supported by multiboot.
For example GRUB supports MIPS architecture as well.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 hw/i386/multiboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eduardo Habkost Oct. 13, 2017, 7:25 p.m. | #1
On Thu, Oct 12, 2017 at 04:54:37PM -0700, Anatol Pomozov wrote:
> x86 is not the only architecture supported by multiboot.
> For example GRUB supports MIPS architecture as well.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  hw/i386/multiboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index c9254f313e..7dacd6d827 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -173,7 +173,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>          }
>  
>          kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> -                               &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> +                               &elf_low, &elf_high, 0, EM_NONE,
>                                 0, 0);

I assume we still want PC to reject non-x86 ELF files.  Isn't it
better to add a elf_machine argument to load_multiboot() so each
load_multiboot() caller can specify what's the expected
architecture?


>          if (kernel_size < 0) {
>              fprintf(stderr, "Error while loading elf kernel\n");
> -- 
> 2.15.0.rc0.271.g36b669edcc-goog
>
Anatol Pomozov Oct. 13, 2017, 9:25 p.m. | #2
Hi

On Fri, Oct 13, 2017 at 12:25 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Oct 12, 2017 at 04:54:37PM -0700, Anatol Pomozov wrote:
>> x86 is not the only architecture supported by multiboot.
>> For example GRUB supports MIPS architecture as well.
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>>  hw/i386/multiboot.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>> index c9254f313e..7dacd6d827 100644
>> --- a/hw/i386/multiboot.c
>> +++ b/hw/i386/multiboot.c
>> @@ -173,7 +173,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>>          }
>>
>>          kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
>> -                               &elf_low, &elf_high, 0, I386_ELF_MACHINE,
>> +                               &elf_low, &elf_high, 0, EM_NONE,
>>                                 0, 0);
>
> I assume we still want PC to reject non-x86 ELF files.

Does multiboot spec states this restriction? I've heard that there are
attempts to implement multiboot at ARM [1] [2]. Also multiboot2 spec
mentions MIPS as one of the target architectures.

[1] https://github.com/jncronin/rpi-boot/blob/master/MULTIBOOT-ARM
[2] https://wiki.linaro.org/AndrePrzywara/Multiboot

>  Isn't it
> better to add a elf_machine argument to load_multiboot() so each
> load_multiboot() caller can specify what's the expected
> architecture?
>
>
>>          if (kernel_size < 0) {
>>              fprintf(stderr, "Error while loading elf kernel\n");
>> --
>> 2.15.0.rc0.271.g36b669edcc-goog
>>
>
> --
> Eduardo
Eduardo Habkost Oct. 13, 2017, 11:21 p.m. | #3
On Fri, Oct 13, 2017 at 02:25:43PM -0700, Anatol Pomozov wrote:
> Hi
> 
> On Fri, Oct 13, 2017 at 12:25 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, Oct 12, 2017 at 04:54:37PM -0700, Anatol Pomozov wrote:
> >> x86 is not the only architecture supported by multiboot.
> >> For example GRUB supports MIPS architecture as well.
> >>
> >> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> >> ---
> >>  hw/i386/multiboot.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> >> index c9254f313e..7dacd6d827 100644
> >> --- a/hw/i386/multiboot.c
> >> +++ b/hw/i386/multiboot.c
> >> @@ -173,7 +173,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> >>          }
> >>
> >>          kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> >> -                               &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> >> +                               &elf_low, &elf_high, 0, EM_NONE,
> >>                                 0, 0);
> >
> > I assume we still want PC to reject non-x86 ELF files.
> 
> Does multiboot spec states this restriction? I've heard that there are
> attempts to implement multiboot at ARM [1] [2]. Also multiboot2 spec
> mentions MIPS as one of the target architectures.
> 
> [1] https://github.com/jncronin/rpi-boot/blob/master/MULTIBOOT-ARM
> [2] https://wiki.linaro.org/AndrePrzywara/Multiboot

I don't believe the spec restricts that, but I don't see why it
would be useful to load an ELF file that doesn't match the target
architecture (e.g. loading non-x86 ELF files on a x86 machine
like PC).

> 
> >  Isn't it
> > better to add a elf_machine argument to load_multiboot() so each
> > load_multiboot() caller can specify what's the expected
> > architecture?
> >
> >
> >>          if (kernel_size < 0) {
> >>              fprintf(stderr, "Error while loading elf kernel\n");
> >> --
> >> 2.15.0.rc0.271.g36b669edcc-goog
> >>
> >
> > --
> > Eduardo
Peter Maydell Oct. 14, 2017, 1:41 p.m. | #4
On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
> I don't believe the spec restricts that, but I don't see why it
> would be useful to load an ELF file that doesn't match the target
> architecture (e.g. loading non-x86 ELF files on a x86 machine
> like PC).

Agreed. If we have non i386 boards that want to use multiboot
we should probably move the common code out of hw/i386...

thanks
-- PMM
Kevin Wolf Oct. 16, 2017, 8:27 a.m. | #5
Am 14.10.2017 um 15:41 hat Peter Maydell geschrieben:
> On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > I don't believe the spec restricts that, but I don't see why it
> > would be useful to load an ELF file that doesn't match the target
> > architecture (e.g. loading non-x86 ELF files on a x86 machine
> > like PC).
> 
> Agreed. If we have non i386 boards that want to use multiboot
> we should probably move the common code out of hw/i386...

Impossible with Multiboot 1, it's a spec that is really made for i386.

The spec isn't really explicit about it being a requirement, but it does
say that its target are 32-bit OSes on PCs, and it defines the boot
state in terms of i386 registers, so it doesn't make sense for non-x86.

From my interpretation of the spec, even support for 64-bit ELFs seems
to be (implicitly) out of spec (there is one place where it even says
"refer to the i386 ELF documentation for details"), but if GRUB
implements it...

Kevin
Anatol Pomozov Oct. 16, 2017, 6:38 p.m. | #6
Hi

On Mon, Oct 16, 2017 at 1:27 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 14.10.2017 um 15:41 hat Peter Maydell geschrieben:
>> On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > I don't believe the spec restricts that, but I don't see why it
>> > would be useful to load an ELF file that doesn't match the target
>> > architecture (e.g. loading non-x86 ELF files on a x86 machine
>> > like PC).

I see what do you mean Eduardo. Yes it makes sense to restrict ELF to
the currently emulated platform.

On a second thought adding multiboot support for non x86 needs to go
with other changes, e.g. multiboot.c should be moved out of hw/i386 to
some platform-independent location. It probably worth to postpone this
change until after Qemu gets multiboot2 support that explicitly states
MIPS support, thus it will be easier to test this codepath on multiple
platforms.

So if you don't mind I'll remove this patch from the current patch
series and put it into later one.

>>
>> Agreed. If we have non i386 boards that want to use multiboot
>> we should probably move the common code out of hw/i386...
>
> Impossible with Multiboot 1, it's a spec that is really made for i386.
>
> The spec isn't really explicit about it being a requirement, but it does
> say that its target are 32-bit OSes on PCs, and it defines the boot
> state in terms of i386 registers, so it doesn't make sense for non-x86.
>
> From my interpretation of the spec, even support for 64-bit ELFs seems
> to be (implicitly) out of spec (there is one place where it even says
> "refer to the i386 ELF documentation for details"), but if GRUB
> implements it...

Yes GRUB has support for ELF64 multiboot loading and it successfully
used by many projects in osdev community.

This functionality has been added to GRUB 12 years ago
https://github.com/coreos/grub/commit/ea4097134fbd834d2f688363f9a8208bf6a49ecd

Patch

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index c9254f313e..7dacd6d827 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -173,7 +173,7 @@  int load_multiboot(FWCfgState *fw_cfg,
         }
 
         kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
-                               &elf_low, &elf_high, 0, I386_ELF_MACHINE,
+                               &elf_low, &elf_high, 0, EM_NONE,
                                0, 0);
         if (kernel_size < 0) {
             fprintf(stderr, "Error while loading elf kernel\n");