| 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 | expand |
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 >
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
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
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
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
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
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
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.
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");
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(-)