Re: [PATCH rc4 23/29] hw/avr: Add helper to load raw/ELF firmware binaries

[Aleksandar Markovic]

On Fri, Jan 31, 2020 at 1:28 AM Philippe Mathieu-Daudé
<address@hidden> wrote:
>
> On 1/31/20 1:26 AM, Aleksandar Markovic wrote:
> > On Fri, Jan 31, 2020 at 1:20 AM Philippe Mathieu-Daudé
> > <address@hidden> wrote:
> >>
> >> On 1/31/20 1:03 AM, Aleksandar Markovic wrote:
> >>> From: Philippe Mathieu-Daudé <address@hidden>
> >>>
> >>> Add avr_load_firmware() function to load firmware in ELF or
> >>> raw binary format.
> >>>
> >>> [AM: Corrected the type of the variable containing e_flags]
> >>>
> >>> Suggested-by: Aleksandar Markovic <address@hidden>
> >>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> >>> Signed-off-by: Aleksandar Markovic <address@hidden>
> >>> ---
> >>>    hw/avr/Makefile.objs |  1 +
> >>>    hw/avr/boot.c        | 74 
> >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>    hw/avr/boot.h        | 33 +++++++++++++++++++++++
> >>>    include/elf.h        |  2 ++
> >>>    4 files changed, 110 insertions(+)
> >>>    create mode 100644 hw/avr/Makefile.objs
> >>>    create mode 100644 hw/avr/boot.c
> >>>    create mode 100644 hw/avr/boot.h
> >>>
> >>> diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
> >>> new file mode 100644
> >>> index 0000000..123f174
> >>> --- /dev/null
> >>> +++ b/hw/avr/Makefile.objs
> >>> @@ -0,0 +1 @@
> >>> +obj-y += boot.o
> >>> diff --git a/hw/avr/boot.c b/hw/avr/boot.c
> >>> new file mode 100644
> >>> index 0000000..9ac2c88
> >>> --- /dev/null
> >>> +++ b/hw/avr/boot.c
> >>> @@ -0,0 +1,74 @@
> >>> +/*
> >>> + * AVR loader helpers
> >>> + *
> >>> + * Copyright (c) 2019 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"
> >>> +
> >>> +bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
> >>> +                       MemoryRegion *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_flags_to_cpu_type(e_flags, NULL);
> >>> +        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_targphys(filename, OFFSET_CODE,
> >>> +                                           memory_region_size(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/hw/avr/boot.h b/hw/avr/boot.h
> >>> new file mode 100644
> >>> index 0000000..62bc10c
> >>> --- /dev/null
> >>> +++ b/hw/avr/boot.h
> >>> @@ -0,0 +1,33 @@
> >>> +/*
> >>> + * AVR loader helpers
> >>> + *
> >>> + * Copyright (c) 2019 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 8fbfe60..3f08f68 100644
> >>> --- a/include/elf.h
> >>> +++ b/include/elf.h
> >>> @@ -202,6 +202,8 @@ typedef struct mips_elf_abiflags_v0 {
> >>>    #define EM_MOXIE           223     /* Moxie processor family */
> >>>    #define EM_MOXIE_OLD       0xFEED
> >>>
> >>> +#define EM_AVR 83 /* AVR 8-bit microcontroller */
> >>
> >> IIRC from previous version you suggested to move this, I suppose between:
> >>
> >> #define EM_CRIS         76
> >>
> >> #define EM_V850         87
> >>
> >
> > I forgot about that, thinking about other larger changes.
>
> Anyway looking closer, the definitions are not ordered correctly, so I
> think we don't care much. Your call :)
>

Let's not add more mess to the mess. :)

> >
> >>> +
> >>>    /* This is the info that is needed to parse the dynamic section of the 
> >>> file */
> >>>    #define DT_NULL             0
> >>>    #define DT_NEEDED   1
> >>>
> >>
> >
>