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

[Alistair Francis]

On Fri, Feb 26, 2016 at 8:22 AM, Peter Maydell <address@hidden> wrote:
> 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 :-)

I have a fix for this :)

>
>> 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.

True. I have changed it to 0xFFFF (and a 16bit value). Is that enough?

>
>> +
>> +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) ?

This is here for when the user loads an image that they would like to
boot. QEMU needs to set the PC to the start of the image. I'll add
some conditions around it.

>
>> +    }
>> +
>> +    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?

Hmmm.... Good point. Do you have any ideas for a solution?

>
>> +    }
>> +}
>> +
>> +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).

Good point, I'll fix this.

>
> 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?

I don't have a problem with numbers. I don't see it as too confusing
as it is generally pretty obvious which CPU you are targeting. It also
matches what the 'info cpus' command prints. The kernel also just uses
numbers when printing out CPU info. Plus I would consider this a more
advanced feature. People who don't know about the CPUs in the system
can still use -kernel.

>
> (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'.

This is actually pretty easy to do, I have a fix!

>
>> +
>> +            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".

Fixed

>
>> +            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.

I'll add that in the documentation.

>
>> +        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 ?

Yeah, I'm not really sure what to do here. All my use cases are both
LE, so I don't have any test cases.

If I am understanding this right I should swap the bytes if the host
and guest endianess are different?

>
>> +    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.

Fixed

>
>> +    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.

Removed.

Thanks,

Alistair

>
>> +
>> +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
>