[Qemu-devel] [RFC PATCH] elf loader: exit if incompatible architecture is detected

[Qemu-devel] [RFC PATCH] elf loader: exit if incompatible architecture is detected

[Alexey Kardashevskiy]

If we know for sure that the image in "-kernel" is an ELF and we know its
architecture and it is not supported by the current QEMU, there is no
point to continue trying booting this image so let's exit once we deteced
this fact.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---


One of our users tried an X86 image with qemu-system-ppc64. Instead of
printing some reasonable message (which is possible in this case as the image
is ELF), QEMU (spapr.c) simply copied the image in RAM as a raw image and
SLOF failed to boot from it.

The patch fixes the issue but there are still questions.

1. Do we need more sophisticated error checking here? Return -2 instead of exit(1)
and do exit(1) few levels up?

2. The patch does not handle x86's vmlinuz case - these images are not ELFs
but "Linux kernel x86 boot executable bzImage" and QEMU does not parse them.
As a result, SLOF crashes with the registers dump. Do we really care to handle this?


---
 include/hw/elf_ops.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index acc701e..6bcc61f 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -212,21 +212,21 @@ static int glue(load_elf, SZ)(const char *name, int fd,
         case EM_PPC64:
             if (EM_PPC64 != ehdr.e_machine)
                 if (EM_PPC != ehdr.e_machine)
-                    goto fail;
+                    goto arch_fail;
             break;
         case EM_X86_64:
             if (EM_X86_64 != ehdr.e_machine)
                 if (EM_386 != ehdr.e_machine)
-                    goto fail;
+                    goto arch_fail;
             break;
         case EM_MICROBLAZE:
             if (EM_MICROBLAZE != ehdr.e_machine)
                 if (EM_MICROBLAZE_OLD != ehdr.e_machine)
-                    goto fail;
+                    goto arch_fail;
             break;
         default:
             if (elf_machine != ehdr.e_machine)
-                goto fail;
+                goto arch_fail;
     }
 
     if (pentry)
@@ -306,4 +306,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     g_free(data);
     g_free(phdr);
     return -1;
+
+arch_fail:
+    fprintf(stderr, "qemu: could not load arch-incompatible kernel '%s'\n",
+            name);
+    exit(1);
 }
-- 
1.8.4.rc4

Re: [Qemu-devel] [RFC PATCH] elf loader: exit if incompatible architecture is detected

[Alexander Graf]

On 07.01.2014, at 05:35, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> If we know for sure that the image in "-kernel" is an ELF and we know its
> architecture and it is not supported by the current QEMU, there is no
> point to continue trying booting this image so let's exit once we deteced
> this fact.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

How about we just remove non-ELF loading from -kernel on -M pseries?


Alex

> ---
> 
> 
> One of our users tried an X86 image with qemu-system-ppc64. Instead of
> printing some reasonable message (which is possible in this case as the image
> is ELF), QEMU (spapr.c) simply copied the image in RAM as a raw image and
> SLOF failed to boot from it.
> 
> The patch fixes the issue but there are still questions.
> 
> 1. Do we need more sophisticated error checking here? Return -2 instead of exit(1)
> and do exit(1) few levels up?
> 
> 2. The patch does not handle x86's vmlinuz case - these images are not ELFs
> but "Linux kernel x86 boot executable bzImage" and QEMU does not parse them.
> As a result, SLOF crashes with the registers dump. Do we really care to handle this?
> 
> 
> ---
> include/hw/elf_ops.h | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index acc701e..6bcc61f 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -212,21 +212,21 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>         case EM_PPC64:
>             if (EM_PPC64 != ehdr.e_machine)
>                 if (EM_PPC != ehdr.e_machine)
> -                    goto fail;
> +                    goto arch_fail;
>             break;
>         case EM_X86_64:
>             if (EM_X86_64 != ehdr.e_machine)
>                 if (EM_386 != ehdr.e_machine)
> -                    goto fail;
> +                    goto arch_fail;
>             break;
>         case EM_MICROBLAZE:
>             if (EM_MICROBLAZE != ehdr.e_machine)
>                 if (EM_MICROBLAZE_OLD != ehdr.e_machine)
> -                    goto fail;
> +                    goto arch_fail;
>             break;
>         default:
>             if (elf_machine != ehdr.e_machine)
> -                goto fail;
> +                goto arch_fail;
>     }
> 
>     if (pentry)
> @@ -306,4 +306,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>     g_free(data);
>     g_free(phdr);
>     return -1;
> +
> +arch_fail:
> +    fprintf(stderr, "qemu: could not load arch-incompatible kernel '%s'\n",
> +            name);
> +    exit(1);
> }
> -- 
> 1.8.4.rc4
> 

Re: [Qemu-devel] [RFC PATCH] elf loader: exit if incompatible architecture is detected

[Peter Maydell]

On 7 January 2014 04:35, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> One of our users tried an X86 image with qemu-system-ppc64. Instead of
> printing some reasonable message (which is possible in this case as the image
> is ELF), QEMU (spapr.c) simply copied the image in RAM as a raw image and
> SLOF failed to boot from it.
>
> The patch fixes the issue but there are still questions.
>
> 1. Do we need more sophisticated error checking here? Return -2 instead of exit(1)
> and do exit(1) few levels up?

I definitely don't think we should be doing printf-and-exit
at this level; we should report something helpful to the
next level up.

-kernel is a mess anyway, the behaviour differs across
architectures regarding what is and isn't supported.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH] elf loader: exit if incompatible architecture is detected

[Alexey Kardashevskiy]

On 01/21/2014 02:11 AM, Alexander Graf wrote:
> 
> On 07.01.2014, at 05:35, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> If we know for sure that the image in "-kernel" is an ELF and we know its
>> architecture and it is not supported by the current QEMU, there is no
>> point to continue trying booting this image so let's exit once we deteced
>> this fact.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> How about we just remove non-ELF loading from -kernel on -M pseries?


We are fine with that, never tried non-elf anyway, I'll cook another patch
for that. I suppose I do exit(), just one level up, in
spapr_machine:init(), correct?


> 
> 
> Alex
> 
>> ---
>>
>>
>> One of our users tried an X86 image with qemu-system-ppc64. Instead of
>> printing some reasonable message (which is possible in this case as the image
>> is ELF), QEMU (spapr.c) simply copied the image in RAM as a raw image and
>> SLOF failed to boot from it.
>>
>> The patch fixes the issue but there are still questions.
>>
>> 1. Do we need more sophisticated error checking here? Return -2 instead of exit(1)
>> and do exit(1) few levels up?
>>
>> 2. The patch does not handle x86's vmlinuz case - these images are not ELFs
>> but "Linux kernel x86 boot executable bzImage" and QEMU does not parse them.
>> As a result, SLOF crashes with the registers dump. Do we really care to handle this?
>>
>>
>> ---
>> include/hw/elf_ops.h | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index acc701e..6bcc61f 100644
>> --- a/include/hw/elf_ops.h
>> +++ b/include/hw/elf_ops.h
>> @@ -212,21 +212,21 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>         case EM_PPC64:
>>             if (EM_PPC64 != ehdr.e_machine)
>>                 if (EM_PPC != ehdr.e_machine)
>> -                    goto fail;
>> +                    goto arch_fail;
>>             break;
>>         case EM_X86_64:
>>             if (EM_X86_64 != ehdr.e_machine)
>>                 if (EM_386 != ehdr.e_machine)
>> -                    goto fail;
>> +                    goto arch_fail;
>>             break;
>>         case EM_MICROBLAZE:
>>             if (EM_MICROBLAZE != ehdr.e_machine)
>>                 if (EM_MICROBLAZE_OLD != ehdr.e_machine)
>> -                    goto fail;
>> +                    goto arch_fail;
>>             break;
>>         default:
>>             if (elf_machine != ehdr.e_machine)
>> -                goto fail;
>> +                goto arch_fail;
>>     }
>>
>>     if (pentry)
>> @@ -306,4 +306,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>     g_free(data);
>>     g_free(phdr);
>>     return -1;
>> +
>> +arch_fail:
>> +    fprintf(stderr, "qemu: could not load arch-incompatible kernel '%s'\n",
>> +            name);
>> +    exit(1);
>> }
>> -- 
>> 1.8.4.rc4
>>
> 


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] elf loader: exit if incompatible architecture is detected

[Alexander Graf]

> Am 21.01.2014 um 05:04 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>:
> 
>> On 01/21/2014 02:11 AM, Alexander Graf wrote:
>> 
>>> On 07.01.2014, at 05:35, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>> 
>>> If we know for sure that the image in "-kernel" is an ELF and we know its
>>> architecture and it is not supported by the current QEMU, there is no
>>> point to continue trying booting this image so let's exit once we deteced
>>> this fact.
>>> 
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> 
>> How about we just remove non-ELF loading from -kernel on -M pseries?
> 
> 
> We are fine with that, never tried non-elf anyway, I'll cook another patch
> for that. I suppose I do exit(), just one level up, in
> spapr_machine:init(), correct?

Yeah :)

Alex