diff mbox series

[PULL,24/32] hw/avr: Add support for loading ELF/raw binaries

Message ID 20200707181710.30950-25-f4bug@amsat.org
State New
Headers show
Series [PULL,01/32] target/avr: Add basic parameters of the new platform | expand

Commit Message

Philippe Mathieu-Daudé July 7, 2020, 6:17 p.m. UTC
Add avr_load_firmware() function to load firmware in ELF or
raw binary format.

[AM: Corrected the type of the variable containing e_flags]
[AM: Moved definition of e_flags conversion function to boot.c]
Suggested-by: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
Reviewed-by: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
Signed-off-by: Thomas Huth <huth@tuxfamily.org>
Message-Id: <20200705140315.260514-24-huth@tuxfamily.org>
[PMD: Replace load_image_targphys() by load_image_mr()]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/boot.h        |  33 +++++++++++++
 include/elf.h        |   4 ++
 hw/avr/boot.c        | 115 +++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS          |   1 +
 hw/avr/Makefile.objs |   1 +
 5 files changed, 154 insertions(+)
 create mode 100644 hw/avr/boot.h
 create mode 100644 hw/avr/boot.c
 create mode 100644 hw/avr/Makefile.objs

Comments

Peter Maydell July 13, 2020, 12:40 p.m. UTC | #1
On Tue, 7 Jul 2020 at 19:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Add avr_load_firmware() function to load firmware in ELF or
> raw binary format.

Hi; Coverity points out a memory leak (CID 1430449) in this function:

> +bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
> +                       MemoryRegion *program_mr, const char *firmware)
> +{
> +    const char *filename;
> +    int bytes_loaded;
> +    uint64_t entry;
> +    uint32_t e_flags;
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);

qemu_find_file() allocates and returns memory, but we don't
pass this to any function that takes ownership of it,
and none of the exit paths from the function (either error-exit
or success-exit cases) call g_free() on it.

thanks
-- PMM
Philippe Mathieu-Daudé July 14, 2020, 3:09 p.m. UTC | #2
On 7/13/20 2:40 PM, Peter Maydell wrote:
> On Tue, 7 Jul 2020 at 19:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Add avr_load_firmware() function to load firmware in ELF or
>> raw binary format.
> 
> Hi; Coverity points out a memory leak (CID 1430449) in this function:
> 
>> +bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
>> +                       MemoryRegion *program_mr, const char *firmware)
>> +{
>> +    const char *filename;
>> +    int bytes_loaded;
>> +    uint64_t entry;
>> +    uint32_t e_flags;
>> +
>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
> 
> qemu_find_file() allocates and returns memory, but we don't
> pass this to any function that takes ownership of it,
> and none of the exit paths from the function (either error-exit
> or success-exit cases) call g_free() on it.

Ah I didn't know it was allocated, I looked at the declaration
in the header then quickly if there was a comment in the source,
but didn't read the implementation (now I see the obvious g_strdup()
call... Neither have I looked at the other callers.

I'll send a patch.

Thanks for following the Coverity reports,

Phil.

> 
> thanks
> -- PMM
>
diff mbox series

Patch

diff --git a/hw/avr/boot.h b/hw/avr/boot.h
new file mode 100644
index 0000000000..684d553322
--- /dev/null
+++ b/hw/avr/boot.h
@@ -0,0 +1,33 @@ 
+/*
+ * AVR loader helpers
+ *
+ * Copyright (c) 2019-2020 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_AVR_BOOT_H
+#define HW_AVR_BOOT_H
+
+#include "hw/boards.h"
+#include "cpu.h"
+
+/**
+ * avr_load_firmware:   load an image into a memory region
+ *
+ * @cpu:        Handle a AVR CPU object
+ * @ms:         A MachineState
+ * @mr:         Memory Region to load into
+ * @firmware:   Path to the firmware file (raw binary or ELF format)
+ *
+ * Load a firmware supplied by the machine or by the user  with the
+ * '-bios' command line option, and put it in target memory.
+ *
+ * Returns: true on success, false on error.
+ */
+bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
+                       MemoryRegion *mr, const char *firmware);
+
+#endif
diff --git a/include/elf.h b/include/elf.h
index 8fbfe60e09..5b06b55f28 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -160,6 +160,8 @@  typedef struct mips_elf_abiflags_v0 {
 
 #define EM_CRIS         76      /* Axis Communications 32-bit embedded processor */
 
+#define EM_AVR          83      /* AVR 8-bit microcontroller */
+
 #define EM_V850		87	/* NEC v850 */
 
 #define EM_H8_300H      47      /* Hitachi H8/300H */
@@ -202,6 +204,8 @@  typedef struct mips_elf_abiflags_v0 {
 #define EM_MOXIE           223     /* Moxie processor family */
 #define EM_MOXIE_OLD       0xFEED
 
+#define EF_AVR_MACH     0x7F       /* Mask for AVR e_flags to get core type */
+
 /* This is the info that is needed to parse the dynamic section of the file */
 #define DT_NULL		0
 #define DT_NEEDED	1
diff --git a/hw/avr/boot.c b/hw/avr/boot.c
new file mode 100644
index 0000000000..6fbcde4061
--- /dev/null
+++ b/hw/avr/boot.c
@@ -0,0 +1,115 @@ 
+/*
+ * AVR loader helpers
+ *
+ * Copyright (c) 2019-2020 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/loader.h"
+#include "elf.h"
+#include "boot.h"
+#include "qemu/error-report.h"
+
+static const char *avr_elf_e_flags_to_cpu_type(uint32_t flags)
+{
+    switch (flags & EF_AVR_MACH) {
+    case bfd_mach_avr1:
+        return AVR_CPU_TYPE_NAME("avr1");
+    case bfd_mach_avr2:
+        return AVR_CPU_TYPE_NAME("avr2");
+    case bfd_mach_avr25:
+        return AVR_CPU_TYPE_NAME("avr25");
+    case bfd_mach_avr3:
+        return AVR_CPU_TYPE_NAME("avr3");
+    case bfd_mach_avr31:
+        return AVR_CPU_TYPE_NAME("avr31");
+    case bfd_mach_avr35:
+        return AVR_CPU_TYPE_NAME("avr35");
+    case bfd_mach_avr4:
+        return AVR_CPU_TYPE_NAME("avr4");
+    case bfd_mach_avr5:
+        return AVR_CPU_TYPE_NAME("avr5");
+    case bfd_mach_avr51:
+        return AVR_CPU_TYPE_NAME("avr51");
+    case bfd_mach_avr6:
+        return AVR_CPU_TYPE_NAME("avr6");
+    case bfd_mach_avrtiny:
+        return AVR_CPU_TYPE_NAME("avrtiny");
+    case bfd_mach_avrxmega2:
+        return AVR_CPU_TYPE_NAME("xmega2");
+    case bfd_mach_avrxmega3:
+        return AVR_CPU_TYPE_NAME("xmega3");
+    case bfd_mach_avrxmega4:
+        return AVR_CPU_TYPE_NAME("xmega4");
+    case bfd_mach_avrxmega5:
+        return AVR_CPU_TYPE_NAME("xmega5");
+    case bfd_mach_avrxmega6:
+        return AVR_CPU_TYPE_NAME("xmega6");
+    case bfd_mach_avrxmega7:
+        return AVR_CPU_TYPE_NAME("xmega7");
+    default:
+        return NULL;
+    }
+}
+
+bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
+                       MemoryRegion *program_mr, const char *firmware)
+{
+    const char *filename;
+    int bytes_loaded;
+    uint64_t entry;
+    uint32_t e_flags;
+
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
+    if (filename == NULL) {
+        error_report("Unable to find %s", firmware);
+        return false;
+    }
+
+    bytes_loaded = load_elf_ram_sym(filename,
+                                    NULL, NULL, NULL,
+                                    &entry, NULL, NULL,
+                                    &e_flags, 0, EM_AVR, 0, 0,
+                                    NULL, true, NULL);
+    if (bytes_loaded >= 0) {
+        /* If ELF file is provided, determine CPU type reading ELF e_flags. */
+        const char *elf_cpu = avr_elf_e_flags_to_cpu_type(e_flags);
+        const char *mcu_cpu_type = object_get_typename(OBJECT(cpu));
+        int cpu_len = strlen(mcu_cpu_type) - strlen(AVR_CPU_TYPE_SUFFIX);
+
+        if (entry) {
+            error_report("BIOS entry_point must be 0x0000 "
+                         "(ELF image '%s' has entry_point 0x%04" PRIx64 ")",
+                         firmware, entry);
+            return false;
+        }
+        if (!elf_cpu) {
+            warn_report("Could not determine CPU type for ELF image '%s', "
+                        "assuming '%.*s' CPU",
+                         firmware, cpu_len, mcu_cpu_type);
+            return true;
+        }
+        if (strcmp(elf_cpu, mcu_cpu_type)) {
+            error_report("Current machine: %s with '%.*s' CPU",
+                         MACHINE_GET_CLASS(ms)->desc, cpu_len, mcu_cpu_type);
+            error_report("ELF image '%s' is for '%.*s' CPU",
+                         firmware,
+                         (int)(strlen(elf_cpu) - strlen(AVR_CPU_TYPE_SUFFIX)),
+                         elf_cpu);
+            return false;
+        }
+    } else {
+        bytes_loaded = load_image_mr(filename, program_mr);
+    }
+    if (bytes_loaded < 0) {
+        error_report("Unable to load firmware image %s as ELF or raw binary",
+                     firmware);
+        return false;
+    }
+    return true;
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 443b377a72..a78d4e56ff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -983,6 +983,7 @@  M: Michael Rolnik <mrolnik@gmail.com>
 R: Sarah Harris <S.E.Harris@kent.ac.uk>
 S: Maintained
 F: default-configs/avr-softmmu.mak
+F: hw/avr/
 F: include/hw/char/avr_usart.h
 F: hw/char/avr_usart.c
 F: include/hw/timer/avr_timer16.h
diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
new file mode 100644
index 0000000000..123f174f0e
--- /dev/null
+++ b/hw/avr/Makefile.objs
@@ -0,0 +1 @@ 
+obj-y += boot.o