Re: [Qemu-devel] [PATCH v3 1/2] generic-loader: Add a generic loader

[Peter Maydell]

On 19 February 2016 at 20:40, Alistair Francis
<address@hidden> wrote:
> Add a generic loader to QEMU which can be used to load images or set
> memory values.

I'm not inherently opposed to this (it seems like a nice way
to deal with the desire to load arbitrary images), but it feels
a bit half-baked at the moment. More detailed comments below.

> This only supports ARM architectures at the moment.

This doesn't sound very generic :-)

> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V3:
>  - Pass the ram_size to load_image_targphys()
> V2:
>  - Add maintainers entry
>  - Perform bounds checking
>  - Register and unregister the reset in the realise/unrealise
> Changes since RFC:
>  - Add BE support
>
>  MAINTAINERS                      |   6 ++
>  default-configs/arm-softmmu.mak  |   1 +
>  hw/misc/Makefile.objs            |   2 +
>  hw/misc/generic-loader.c         | 143 
> +++++++++++++++++++++++++++++++++++++++
>  include/hw/misc/generic-loader.h |  50 ++++++++++++++
>  5 files changed, 202 insertions(+)
>  create mode 100644 hw/misc/generic-loader.c
>  create mode 100644 include/hw/misc/generic-loader.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9adeda3..7dae3dd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -963,6 +963,12 @@ F: hw/acpi/nvdimm.c
>  F: hw/mem/nvdimm.c
>  F: include/hw/mem/nvdimm.h
>
> +Generic Loader
> +M: Alistair Francis <address@hidden>
> +S: Maintained
> +F: hw/misc/generic-loader.c
> +F: include/hw/misc/generic-loader.h
> +
>  Subsystems
>  ----------
>  Audio
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index a9f82a1..b246b75 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -110,3 +110,4 @@ CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
>  CONFIG_ACPI=y
>  CONFIG_SMBIOS=y
> +CONFIG_LOADER=y
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index ea6cd3c..9f05dcf 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -46,3 +46,5 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
>  obj-$(CONFIG_PVPANIC) += pvpanic.o
>  obj-$(CONFIG_EDU) += edu.o
>  obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
> +
> +obj-$(CONFIG_LOADER) += generic-loader.o
> diff --git a/hw/misc/generic-loader.c b/hw/misc/generic-loader.c
> new file mode 100644
> index 0000000..20f07a8
> --- /dev/null
> +++ b/hw/misc/generic-loader.c
> @@ -0,0 +1,143 @@
> +/*
> + * Generic Loader
> + *
> + * Copyright (C) 2014 Li Guang
> + * Copyright (C) 2016 Xilinx Inc.
> + * Written by Li Guang <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +#include "hw/loader.h"
> +#include "hw/misc/generic-loader.h"
> +
> +#define CPU_NONE 0xFF

This is a mine waiting to be triggered if we ever raise
MAX_CPUMASK_BITS and allow 256 CPUs.

> +
> +static void generic_loader_reset(void *opaque)
> +{
> +    GenericLoaderState *s = GENERIC_LOADER(opaque);
> +
> +    if (s->cpu) {
> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
> +        cpu_reset(s->cpu);
> +        cc->set_pc(s->cpu, s->addr);

Why is a loader device messing with the CPU state (especially
unconditionally) ?

> +    }
> +
> +    if (s->data_len) {
> +        assert(s->data_len < sizeof(s->data));
> +        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, 
> &s->data,
> +                         s->data_len);

Writing directly to cpu->as is not very generic. In particular,
how should this interact with TrustZone, where you might want to
write the image to the Secure address space?

> +    }
> +}
> +
> +static void generic_loader_realize(DeviceState *dev, Error **errp)
> +{
> +    GenericLoaderState *s = GENERIC_LOADER(dev);
> +    hwaddr entry;
> +    int big_endian;
> +    int size = 0;
> +
> +    qemu_register_reset(generic_loader_reset, dev);
> +
> +    if (s->cpu_nr != CPU_NONE) {
> +        CPUState *cs = first_cpu;
> +        int cpu_num = 0;
> +
> +        CPU_FOREACH(cs) {
> +            if (cpu_num == s->cpu_nr) {
> +                s->cpu = cs;
> +                break;
> +            } else if (!CPU_NEXT(cs)) {
> +                error_setg(errp, "Specified boot CPU#%d is nonexistent",
> +                           s->cpu_nr);
> +                return;
> +            } else {
> +                cpu_num++;
> +            }
> +        }

This appears to be reimplementing qemu_get_cpu(s->cpu_nr).

Is the internal QEMU CPU index really the right thing to expose to
the user? Should we be considering letting the user specify by
MPIDR or some other thing? By path-to-CPU-in-QOM-tree?

(We should be consistent with however the user specifies CPUs
for things like CPU hotplug, anyway.)

> +    }
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    big_endian = 1;
> +#else
> +    big_endian = 0;
> +#endif
> +
> +    if (s->file) {
> +        if (!s->force_raw) {
> +            size = load_elf(s->file, NULL, NULL, &entry, NULL, NULL,
> +                            big_endian, ELF_ARCH, 0);

It would be nice not to add another obstacle to supporting multiple
CPU architectures in one QEMU binary, right? :-)

It looks like load_elf() mostly takes the elf_machine argument in
order to fall over if you pass it an ELF file of the wrong format
(and partly for relocation stuff?) so perhaps we should enhance
it to accept 'any architecture'.

> +
> +            if (size < 0) {
> +                size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL);
> +            }
> +        }
> +
> +        if (size < 0) {
> +            /* Default to the maximum size being the machines ram size */

"machine's".

> +            size = load_image_targphys(s->file, s->addr, ram_size);
> +        } else {
> +            s->addr = entry;
> +        }
> +
> +        if (size < 0) {
> +            error_setg(errp, "Cannot load specified image %s", s->file);
> +            return;
> +        }
> +    }
> +
> +    if (s->data_len && (s->data_len > sizeof(s->data))) {
> +        error_setg(errp, "data-len cannot be more then the data size");

...and the data size can't be more than 8 bytes, but you don't say
that anywhere.

> +        return;
> +    }
> +}
> +
> +static void generic_loader_unrealize(DeviceState *dev, Error **errp)
> +{
> +    qemu_unregister_reset(generic_loader_reset, dev);
> +}
> +
> +static Property generic_loader_props[] = {
> +    DEFINE_PROP_UINT64("addr", GenericLoaderState, addr, 0),
> +    DEFINE_PROP_UINT64("data", GenericLoaderState, data, 0),

Doesn't defining this as a UINT64 property mean that the same qemu
command line will write different data on a big-endian and little-endian
host ?

> +    DEFINE_PROP_UINT8("data-len", GenericLoaderState, data_len, 0),
> +    DEFINE_PROP_UINT8("cpu", GenericLoaderState, cpu_nr, CPU_NONE),
> +    DEFINE_PROP_BOOL("force-raw", GenericLoaderState, force_raw, false),
> +    DEFINE_PROP_STRING("file", GenericLoaderState, file),

At least one of these options didn't make it to the documentation.

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void generic_loader_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = generic_loader_realize;
> +    dc->unrealize = generic_loader_unrealize;
> +    dc->props = generic_loader_props;
> +    dc->desc = "Generic Loader";
> +}
> +
> +static TypeInfo generic_loader_info = {
> +    .name = TYPE_GENERIC_LOADER,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(GenericLoaderState),
> +    .class_init = generic_loader_class_init,
> +};
> +
> +static void generic_loader_register_type(void)
> +{
> +    type_register_static(&generic_loader_info);
> +}
> +
> +type_init(generic_loader_register_type)
> diff --git a/include/hw/misc/generic-loader.h 
> b/include/hw/misc/generic-loader.h
> new file mode 100644
> index 0000000..79b5536
> --- /dev/null
> +++ b/include/hw/misc/generic-loader.h
> @@ -0,0 +1,50 @@
> +/*
> + * Generic Loader
> + *
> + * Copyright (C) 2014 Li Guang
> + * Written by Li Guang <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#ifndef GENERIC_LOADER_H
> +#define GENERIC_LOADER_H
> +
> +#include "elf.h"
> +
> +#if   defined(TARGET_AARCH64)
> +    #define ELF_ARCH        EM_AARCH64
> +#elif defined(TARGET_ARM)
> +    #define ELF_ARCH        EM_ARM
> +#endif

I definitely don't want to add any new #ifdef TARGET_THING ladders,
please. Anything that must be defined per-target should be done by
having a file or define or whatever in target-*/ somewhere.

> +
> +typedef struct GenericLoaderState {
> +    /* <private> */
> +    DeviceState parent_obj;
> +
> +    /* <public> */
> +    CPUState *cpu;
> +
> +    uint64_t addr;
> +    uint64_t data;
> +    uint8_t data_len;
> +    uint8_t cpu_nr;
> +
> +    char *file;
> +
> +    bool force_raw;
> +} GenericLoaderState;
> +
> +#define TYPE_GENERIC_LOADER "loader"
> +#define GENERIC_LOADER(obj) OBJECT_CHECK(GenericLoaderState, (obj), \
> +                                         TYPE_GENERIC_LOADER)
> +
> +#endif

thanks
-- PMM