[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
Kevin Wolf Jan. 15, 2018, 2:52 p.m. | #7
Am 16.10.2017 um 20:38 hat Anatol Pomozov geschrieben:
> 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.

I can't find a new version of this patch series with this patch dropped.
Are you still planning to send one?

Kevin
Anatol Pomozov Jan. 29, 2018, 6:35 p.m. | #8
Hi

On Mon, Jan 15, 2018 at 6:52 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 16.10.2017 um 20:38 hat Anatol Pomozov geschrieben:
>> 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.
>
> I can't find a new version of this patch series with this patch dropped.
> Are you still planning to send one?

Yes I dropped it. It belongs to future multiboot refactoring patch series.

> 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 reading of multiboot specification it says that boot state is
32-bit. Thus OS image should make sure its entry point is 32bit code.
But specification does not say that ELF should not contain 64bit code.
GRUB project, the reference multiboot implementation, supports 64bit
ELF.

> 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 supports it from its early days. One reason why I sent this
patch is to make QEMU multiboot functionality compatible with GRUB. So
the same binary can work in emulator and at real hardware.

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");