qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/4 v2] elf loader: exit if incompatible architecture is detected
@ 2014-01-22  5:20 Alexey Kardashevskiy
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 1/4] spapr: support only ELF kernel images Alexey Kardashevskiy
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-22  5:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, Alexander Graf

The first 2 patches are real fixes, the last two are for discussion 
not sure we really need them). Thanks.


Alexey Kardashevskiy (4):
  spapr: support only ELF kernel images
  moxie: fix load_elf() usage
  elf-loader: add more return codes
  spapr: add more details error description of why load_elf() failed

 hw/core/loader.c     | 12 ++++++------
 hw/moxie/moxiesim.c  |  2 +-
 hw/ppc/spapr.c       | 22 ++++++++++++++--------
 hw/s390x/ipl.c       |  4 ++--
 include/hw/elf_ops.h | 19 ++++++++++++++-----
 include/hw/loader.h  |  5 +++++
 6 files changed, 42 insertions(+), 22 deletions(-)

-- 
1.8.4.rc4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/4] spapr: support only ELF kernel images
  2014-01-22  5:20 [Qemu-devel] [PATCH 0/4 v2] elf loader: exit if incompatible architecture is detected Alexey Kardashevskiy
@ 2014-01-22  5:20 ` Alexey Kardashevskiy
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 2/4] moxie: fix load_elf() usage Alexey Kardashevskiy
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-22  5:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, Alexander Graf

Currently everybody uses ELF kernel images with "-kernel" option on
pseries machine but QEMU still tries to boot from an image even it
fails to recognize it is ELF. This produces undefined behaviour if
the user tries a kernel image compiled for another architecture.

This removes support of raw kernel images.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5b21562..851ce4b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1313,11 +1313,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
             kernel_le = kernel_size > 0;
         }
         if (kernel_size < 0) {
-            kernel_size = load_image_targphys(kernel_filename,
-                                              KERNEL_LOAD_ADDR,
-                                              load_limit - KERNEL_LOAD_ADDR);
-        }
-        if (kernel_size < 0) {
             fprintf(stderr, "qemu: could not load kernel '%s'\n",
                     kernel_filename);
             exit(1);
-- 
1.8.4.rc4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/4] moxie: fix load_elf() usage
  2014-01-22  5:20 [Qemu-devel] [PATCH 0/4 v2] elf loader: exit if incompatible architecture is detected Alexey Kardashevskiy
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 1/4] spapr: support only ELF kernel images Alexey Kardashevskiy
@ 2014-01-22  5:20 ` Alexey Kardashevskiy
  2014-01-31  7:18   ` Alexander Graf
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 3/4] elf-loader: add more return codes Alexey Kardashevskiy
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 4/4] spapr: add more details error description of why load_elf() failed Alexey Kardashevskiy
  3 siblings, 1 reply; 9+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-22  5:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, Alexander Graf

At the moment in the case of error, load_elf() returns -1 so load_kernel()
will not signal error at all.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/moxie/moxiesim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/moxie/moxiesim.c b/hw/moxie/moxiesim.c
index ef4f3a8..a87ca6d 100644
--- a/hw/moxie/moxiesim.c
+++ b/hw/moxie/moxiesim.c
@@ -55,7 +55,7 @@ static void load_kernel(MoxieCPU *cpu, LoaderParams *loader_params)
                            &entry, &kernel_low, &kernel_high, 1,
                            ELF_MACHINE, 0);
 
-    if (!kernel_size) {
+    if (kernel_size <= 0) {
         fprintf(stderr, "qemu: could not load kernel '%s'\n",
                 loader_params->kernel_filename);
         exit(1);
-- 
1.8.4.rc4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 3/4] elf-loader: add more return codes
  2014-01-22  5:20 [Qemu-devel] [PATCH 0/4 v2] elf loader: exit if incompatible architecture is detected Alexey Kardashevskiy
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 1/4] spapr: support only ELF kernel images Alexey Kardashevskiy
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 2/4] moxie: fix load_elf() usage Alexey Kardashevskiy
@ 2014-01-22  5:20 ` Alexey Kardashevskiy
  2014-01-22  7:27   ` Alexey Kardashevskiy
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 4/4] spapr: add more details error description of why load_elf() failed Alexey Kardashevskiy
  3 siblings, 1 reply; 9+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-22  5:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, Alexander Graf

The existing load_elf() just returns -1 if it fails to load ELF. However
it could be smarter than this and tell more about the failure such as
wrong endianness or incompatible platform.

This adds additional return codes for wrong architecture, wrong
endianness and if the image is not ELF at all.

This fixes handling of what load_elf() returns for s390x and moxie
architectures, other callers just check the return value for <0 and
this remains unchanged.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/core/loader.c     | 12 ++++++------
 hw/s390x/ipl.c       |  4 ++--
 include/hw/elf_ops.h | 19 ++++++++++++++-----
 include/hw/loader.h  |  5 +++++
 4 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 0634bee..f510260 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -289,7 +289,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
              void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
              uint64_t *highaddr, int big_endian, int elf_machine, int clear_lsb)
 {
-    int fd, data_order, target_data_order, must_swab, ret;
+    int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
     uint8_t e_ident[EI_NIDENT];
 
     fd = open(filename, O_RDONLY | O_BINARY);
@@ -302,8 +302,10 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
     if (e_ident[0] != ELFMAG0 ||
         e_ident[1] != ELFMAG1 ||
         e_ident[2] != ELFMAG2 ||
-        e_ident[3] != ELFMAG3)
+        e_ident[3] != ELFMAG3) {
+        ret = ELF_LOAD_NOT_ELF;
         goto fail;
+    }
 #ifdef HOST_WORDS_BIGENDIAN
     data_order = ELFDATA2MSB;
 #else
@@ -317,6 +319,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
     }
 
     if (target_data_order != e_ident[EI_DATA]) {
+        ret = ELF_LOAD_WRONG_ENDIAN;
         goto fail;
     }
 
@@ -329,12 +332,9 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
                          pentry, lowaddr, highaddr, elf_machine, clear_lsb);
     }
 
-    close(fd);
-    return ret;
-
  fail:
     close(fd);
-    return -1;
+    return ret;
 }
 
 static void bswap_uboot_header(uboot_image_header_t *hdr)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 1a6397b..cff77ad 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -97,10 +97,10 @@ static int s390_ipl_init(SysBusDevice *dev)
     } else {
         kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL,
                                NULL, 1, ELF_MACHINE, 0);
-        if (kernel_size == -1) {
+        if (kernel_size < 0) {
             kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
         }
-        if (kernel_size == -1) {
+        if (kernel_size < 0) {
             fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel);
             return -1;
         }
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index acc701e..b7e7b36 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -201,6 +201,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     uint64_t addr, low = (uint64_t)-1, high = 0;
     uint8_t *data = NULL;
     char label[128];
+    int ret = ELF_LOAD_FAILED;
 
     if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
         goto fail;
@@ -210,23 +211,31 @@ static int glue(load_elf, SZ)(const char *name, int fd,
 
     switch (elf_machine) {
         case EM_PPC64:
-            if (EM_PPC64 != ehdr.e_machine)
+            if (EM_PPC64 != ehdr.e_machine) {
                 if (EM_PPC != ehdr.e_machine)
+                    ret = ELF_LOAD_WRONG_ARCH;
                     goto fail;
+                }
             break;
         case EM_X86_64:
             if (EM_X86_64 != ehdr.e_machine)
-                if (EM_386 != ehdr.e_machine)
+                if (EM_386 != ehdr.e_machine) {
+                    ret = ELF_LOAD_WRONG_ARCH;
                     goto fail;
+                }
             break;
         case EM_MICROBLAZE:
             if (EM_MICROBLAZE != ehdr.e_machine)
-                if (EM_MICROBLAZE_OLD != ehdr.e_machine)
+                if (EM_MICROBLAZE_OLD != ehdr.e_machine) {
+                    ret = ELF_LOAD_WRONG_ARCH;
                     goto fail;
+                }
             break;
         default:
-            if (elf_machine != ehdr.e_machine)
+            if (elf_machine != ehdr.e_machine) {
+                ret = ELF_LOAD_WRONG_ARCH;
                 goto fail;
+            }
     }
 
     if (pentry)
@@ -305,5 +314,5 @@ static int glue(load_elf, SZ)(const char *name, int fd,
  fail:
     g_free(data);
     g_free(phdr);
-    return -1;
+    return ret;
 }
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 7a23d6b..6c9d762 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -8,6 +8,11 @@ int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
 int load_image_targphys(const char *filename, hwaddr,
                         uint64_t max_sz);
+
+#define ELF_LOAD_FAILED       -1
+#define ELF_LOAD_NOT_ELF      -2
+#define ELF_LOAD_WRONG_ARCH   -3
+#define ELF_LOAD_WRONG_ENDIAN -4
 int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
              void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
              uint64_t *highaddr, int big_endian, int elf_machine,
-- 
1.8.4.rc4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 4/4] spapr: add more details error description of why load_elf() failed
  2014-01-22  5:20 [Qemu-devel] [PATCH 0/4 v2] elf loader: exit if incompatible architecture is detected Alexey Kardashevskiy
                   ` (2 preceding siblings ...)
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 3/4] elf-loader: add more return codes Alexey Kardashevskiy
@ 2014-01-22  5:20 ` Alexey Kardashevskiy
  2014-01-31  7:20   ` Alexander Graf
  3 siblings, 1 reply; 9+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-22  5:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, Alexander Graf

This makes use of new error codes which load_elf() can return.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 851ce4b..18a4872 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1306,15 +1306,26 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
         kernel_size = load_elf(kernel_filename, translate_kernel_address, NULL,
                                NULL, &lowaddr, NULL, 1, ELF_MACHINE, 0);
-        if (kernel_size < 0) {
+        if (kernel_size == ELF_LOAD_WRONG_ENDIAN) {
             kernel_size = load_elf(kernel_filename,
                                    translate_kernel_address, NULL,
                                    NULL, &lowaddr, NULL, 0, ELF_MACHINE, 0);
             kernel_le = kernel_size > 0;
         }
         if (kernel_size < 0) {
-            fprintf(stderr, "qemu: could not load kernel '%s'\n",
-                    kernel_filename);
+            switch (kernel_size) {
+            case ELF_LOAD_WRONG_ARCH:
+                fprintf(stderr, "qemu: could not load kernel '%s' from incompatible arhitecture\n",
+                        kernel_filename);
+                break;
+            case ELF_LOAD_NOT_ELF:
+                fprintf(stderr, "qemu: '%s' is not ELF\n", kernel_filename);
+                break;
+            default:
+                fprintf(stderr, "qemu: could not load kernel '%s'\n",
+                        kernel_filename);
+                break;
+            }
             exit(1);
         }
 
-- 
1.8.4.rc4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 3/4] elf-loader: add more return codes
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 3/4] elf-loader: add more return codes Alexey Kardashevskiy
@ 2014-01-22  7:27   ` Alexey Kardashevskiy
  2014-01-31  2:15     ` Alexey Kardashevskiy
  0 siblings, 1 reply; 9+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-22  7:27 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, Alexander Graf

On 01/22/2014 04:20 PM, Alexey Kardashevskiy wrote:
> The existing load_elf() just returns -1 if it fails to load ELF. However
> it could be smarter than this and tell more about the failure such as
> wrong endianness or incompatible platform.
> 
> This adds additional return codes for wrong architecture, wrong
> endianness and if the image is not ELF at all.
> 
> This fixes handling of what load_elf() returns for s390x and moxie
> architectures, other callers just check the return value for <0 and
> this remains unchanged.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/core/loader.c     | 12 ++++++------
>  hw/s390x/ipl.c       |  4 ++--
>  include/hw/elf_ops.h | 19 ++++++++++++++-----
>  include/hw/loader.h  |  5 +++++
>  4 files changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 0634bee..f510260 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -289,7 +289,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>               void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>               uint64_t *highaddr, int big_endian, int elf_machine, int clear_lsb)
>  {
> -    int fd, data_order, target_data_order, must_swab, ret;
> +    int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>      uint8_t e_ident[EI_NIDENT];
>  
>      fd = open(filename, O_RDONLY | O_BINARY);
> @@ -302,8 +302,10 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>      if (e_ident[0] != ELFMAG0 ||
>          e_ident[1] != ELFMAG1 ||
>          e_ident[2] != ELFMAG2 ||
> -        e_ident[3] != ELFMAG3)
> +        e_ident[3] != ELFMAG3) {
> +        ret = ELF_LOAD_NOT_ELF;
>          goto fail;
> +    }
>  #ifdef HOST_WORDS_BIGENDIAN
>      data_order = ELFDATA2MSB;
>  #else
> @@ -317,6 +319,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>      }
>  
>      if (target_data_order != e_ident[EI_DATA]) {
> +        ret = ELF_LOAD_WRONG_ENDIAN;
>          goto fail;
>      }
>  
> @@ -329,12 +332,9 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb);
>      }
>  
> -    close(fd);
> -    return ret;
> -
>   fail:
>      close(fd);
> -    return -1;
> +    return ret;
>  }
>  
>  static void bswap_uboot_header(uboot_image_header_t *hdr)
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 1a6397b..cff77ad 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -97,10 +97,10 @@ static int s390_ipl_init(SysBusDevice *dev)
>      } else {
>          kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL,
>                                 NULL, 1, ELF_MACHINE, 0);
> -        if (kernel_size == -1) {
> +        if (kernel_size < 0) {
>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
>          }
> -        if (kernel_size == -1) {
> +        if (kernel_size < 0) {
>              fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel);
>              return -1;
>          }
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index acc701e..b7e7b36 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -201,6 +201,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>      uint64_t addr, low = (uint64_t)-1, high = 0;
>      uint8_t *data = NULL;
>      char label[128];
> +    int ret = ELF_LOAD_FAILED;
>  
>      if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
>          goto fail;
> @@ -210,23 +211,31 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>  
>      switch (elf_machine) {
>          case EM_PPC64:
> -            if (EM_PPC64 != ehdr.e_machine)
> +            if (EM_PPC64 != ehdr.e_machine) {
>                  if (EM_PPC != ehdr.e_machine)

A stupid bug here, "{" should go one line down :)

I'll repost if someone tells me that the whole idea makes any sense. Thanks.


-- 
Alexey

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 3/4] elf-loader: add more return codes
  2014-01-22  7:27   ` Alexey Kardashevskiy
@ 2014-01-31  2:15     ` Alexey Kardashevskiy
  0 siblings, 0 replies; 9+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-31  2:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, Alexander Graf

On 01/22/2014 06:27 PM, Alexey Kardashevskiy wrote:
> On 01/22/2014 04:20 PM, Alexey Kardashevskiy wrote:
>> The existing load_elf() just returns -1 if it fails to load ELF. However
>> it could be smarter than this and tell more about the failure such as
>> wrong endianness or incompatible platform.
>>
>> This adds additional return codes for wrong architecture, wrong
>> endianness and if the image is not ELF at all.
>>
>> This fixes handling of what load_elf() returns for s390x and moxie
>> architectures, other callers just check the return value for <0 and
>> this remains unchanged.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  hw/core/loader.c     | 12 ++++++------
>>  hw/s390x/ipl.c       |  4 ++--
>>  include/hw/elf_ops.h | 19 ++++++++++++++-----
>>  include/hw/loader.h  |  5 +++++
>>  4 files changed, 27 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 0634bee..f510260 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -289,7 +289,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>>               void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>>               uint64_t *highaddr, int big_endian, int elf_machine, int clear_lsb)
>>  {
>> -    int fd, data_order, target_data_order, must_swab, ret;
>> +    int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>>      uint8_t e_ident[EI_NIDENT];
>>  
>>      fd = open(filename, O_RDONLY | O_BINARY);
>> @@ -302,8 +302,10 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>>      if (e_ident[0] != ELFMAG0 ||
>>          e_ident[1] != ELFMAG1 ||
>>          e_ident[2] != ELFMAG2 ||
>> -        e_ident[3] != ELFMAG3)
>> +        e_ident[3] != ELFMAG3) {
>> +        ret = ELF_LOAD_NOT_ELF;
>>          goto fail;
>> +    }
>>  #ifdef HOST_WORDS_BIGENDIAN
>>      data_order = ELFDATA2MSB;
>>  #else
>> @@ -317,6 +319,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>>      }
>>  
>>      if (target_data_order != e_ident[EI_DATA]) {
>> +        ret = ELF_LOAD_WRONG_ENDIAN;
>>          goto fail;
>>      }
>>  
>> @@ -329,12 +332,9 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb);
>>      }
>>  
>> -    close(fd);
>> -    return ret;
>> -
>>   fail:
>>      close(fd);
>> -    return -1;
>> +    return ret;
>>  }
>>  
>>  static void bswap_uboot_header(uboot_image_header_t *hdr)
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 1a6397b..cff77ad 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -97,10 +97,10 @@ static int s390_ipl_init(SysBusDevice *dev)
>>      } else {
>>          kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL,
>>                                 NULL, 1, ELF_MACHINE, 0);
>> -        if (kernel_size == -1) {
>> +        if (kernel_size < 0) {
>>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
>>          }
>> -        if (kernel_size == -1) {
>> +        if (kernel_size < 0) {
>>              fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel);
>>              return -1;
>>          }
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index acc701e..b7e7b36 100644
>> --- a/include/hw/elf_ops.h
>> +++ b/include/hw/elf_ops.h
>> @@ -201,6 +201,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>      uint64_t addr, low = (uint64_t)-1, high = 0;
>>      uint8_t *data = NULL;
>>      char label[128];
>> +    int ret = ELF_LOAD_FAILED;
>>  
>>      if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
>>          goto fail;
>> @@ -210,23 +211,31 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>  
>>      switch (elf_machine) {
>>          case EM_PPC64:
>> -            if (EM_PPC64 != ehdr.e_machine)
>> +            if (EM_PPC64 != ehdr.e_machine) {
>>                  if (EM_PPC != ehdr.e_machine)
> 
> A stupid bug here, "{" should go one line down :)
> 
> I'll repost if someone tells me that the whole idea makes any sense. Thanks.


Ping?





-- 
Alexey

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 2/4] moxie: fix load_elf() usage
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 2/4] moxie: fix load_elf() usage Alexey Kardashevskiy
@ 2014-01-31  7:18   ` Alexander Graf
  0 siblings, 0 replies; 9+ messages in thread
From: Alexander Graf @ 2014-01-31  7:18 UTC (permalink / raw)
  To: Alexey Kardashevskiy
  Cc: Peter Maydell, green, list@suse.de:PowerPC, qemu-devel


On 22.01.2014, at 06:20, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> At the moment in the case of error, load_elf() returns -1 so load_kernel()
> will not signal error at all.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Anthony (Green), could you please ack this patch?


Alex

> ---
> hw/moxie/moxiesim.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/moxie/moxiesim.c b/hw/moxie/moxiesim.c
> index ef4f3a8..a87ca6d 100644
> --- a/hw/moxie/moxiesim.c
> +++ b/hw/moxie/moxiesim.c
> @@ -55,7 +55,7 @@ static void load_kernel(MoxieCPU *cpu, LoaderParams *loader_params)
>                            &entry, &kernel_low, &kernel_high, 1,
>                            ELF_MACHINE, 0);
> 
> -    if (!kernel_size) {
> +    if (kernel_size <= 0) {
>         fprintf(stderr, "qemu: could not load kernel '%s'\n",
>                 loader_params->kernel_filename);
>         exit(1);
> -- 
> 1.8.4.rc4
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 4/4] spapr: add more details error description of why load_elf() failed
  2014-01-22  5:20 ` [Qemu-devel] [PATCH 4/4] spapr: add more details error description of why load_elf() failed Alexey Kardashevskiy
@ 2014-01-31  7:20   ` Alexander Graf
  0 siblings, 0 replies; 9+ messages in thread
From: Alexander Graf @ 2014-01-31  7:20 UTC (permalink / raw)
  To: Alexey Kardashevskiy; +Cc: Peter Maydell, list@suse.de:PowerPC, qemu-devel


On 22.01.2014, at 06:20, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> This makes use of new error codes which load_elf() can return.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> hw/ppc/spapr.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 851ce4b..18a4872 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1306,15 +1306,26 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> 
>         kernel_size = load_elf(kernel_filename, translate_kernel_address, NULL,
>                                NULL, &lowaddr, NULL, 1, ELF_MACHINE, 0);
> -        if (kernel_size < 0) {
> +        if (kernel_size == ELF_LOAD_WRONG_ENDIAN) {
>             kernel_size = load_elf(kernel_filename,
>                                    translate_kernel_address, NULL,
>                                    NULL, &lowaddr, NULL, 0, ELF_MACHINE, 0);
>             kernel_le = kernel_size > 0;
>         }
>         if (kernel_size < 0) {
> -            fprintf(stderr, "qemu: could not load kernel '%s'\n",
> -                    kernel_filename);
> +            switch (kernel_size) {
> +            case ELF_LOAD_WRONG_ARCH:
> +                fprintf(stderr, "qemu: could not load kernel '%s' from incompatible arhitecture\n",
> +                        kernel_filename);
> +                break;
> +            case ELF_LOAD_NOT_ELF:
> +                fprintf(stderr, "qemu: '%s' is not ELF\n", kernel_filename);

These exit code to string translation should be a common helper, similar to strerror(). Apart from that I like the patch set.


Alex

> +                break;
> +            default:
> +                fprintf(stderr, "qemu: could not load kernel '%s'\n",
> +                        kernel_filename);
> +                break;
> +            }
>             exit(1);
>         }
> 
> -- 
> 1.8.4.rc4
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-01-31  7:21 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-22  5:20 [Qemu-devel] [PATCH 0/4 v2] elf loader: exit if incompatible architecture is detected Alexey Kardashevskiy
2014-01-22  5:20 ` [Qemu-devel] [PATCH 1/4] spapr: support only ELF kernel images Alexey Kardashevskiy
2014-01-22  5:20 ` [Qemu-devel] [PATCH 2/4] moxie: fix load_elf() usage Alexey Kardashevskiy
2014-01-31  7:18   ` Alexander Graf
2014-01-22  5:20 ` [Qemu-devel] [PATCH 3/4] elf-loader: add more return codes Alexey Kardashevskiy
2014-01-22  7:27   ` Alexey Kardashevskiy
2014-01-31  2:15     ` Alexey Kardashevskiy
2014-01-22  5:20 ` [Qemu-devel] [PATCH 4/4] spapr: add more details error description of why load_elf() failed Alexey Kardashevskiy
2014-01-31  7:20   ` Alexander Graf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).