From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXG50-0007Mo-3P for qemu-devel@nongnu.org; Tue, 09 Aug 2016 19:05:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXG4y-0004jl-DE for qemu-devel@nongnu.org; Tue, 09 Aug 2016 19:05:01 -0400 Received: from mail-oi0-x244.google.com ([2607:f8b0:4003:c06::244]:34272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXG4y-0004j5-6x for qemu-devel@nongnu.org; Tue, 09 Aug 2016 19:05:00 -0400 Received: by mail-oi0-x244.google.com with SMTP id t127so2471293oie.1 for ; Tue, 09 Aug 2016 16:04:59 -0700 (PDT) MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: References: <4d6e70dc122fafbe81d85beb257ce3e0d6479fb1.1470253246.git.alistair.francis@xilinx.com> From: Alistair Francis Date: Tue, 9 Aug 2016 16:04:28 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v10 7/8] generic-loader: Add a generic loader List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Alistair Francis , Christopher Covington , Peter Crosthwaite , Paolo Bonzini , QEMU Developers , Markus Armbruster On Tue, Aug 9, 2016 at 11:17 AM, Peter Maydell wrote: > On 3 August 2016 at 21:06, Alistair Francis wrote: >> Add a generic loader to QEMU which can be used to load images or set >> memory values. >> >> Signed-off-by: Alistair Francis >> --- >> V10: >> - Split out the PC setting and data loading >> V9: >> - Fix error messages >> - Updated some incorrect logic >> - Add address space loading support for all image types >> - Explain why the reset is manually registered >> V8: >> - Code corrections >> - Rebase >> V7: >> - Rebase >> V6: >> - Add error checking >> V5: >> - Rebase >> V4: >> - Allow the loader to work with every architecture >> - Move the file to hw/core >> - Increase the maximum number of CPUs >> - Make the CPU operations conditional >> - Convert the cpu option to cpu-num >> - Require the user to specify endianess >> 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 ++ >> hw/core/Makefile.objs | 2 + >> hw/core/generic-loader.c | 195 +++++++++++++++++++++++++++++++++++++++ >> include/hw/core/generic-loader.h | 46 +++++++++ >> 4 files changed, 249 insertions(+) >> create mode 100644 hw/core/generic-loader.c >> create mode 100644 include/hw/core/generic-loader.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 9c88c44..cc34ae0 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -984,6 +984,12 @@ M: Dmitry Fleytman >> S: Maintained >> F: hw/net/e1000e* >> >> +Generic Loader >> +M: Alistair Francis >> +S: Maintained >> +F: hw/core/generic-loader.c >> +F: include/hw/core/generic-loader.h >> + >> Subsystems >> ---------- >> Audio >> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs >> index cfd4840..939c94e 100644 >> --- a/hw/core/Makefile.objs >> +++ b/hw/core/Makefile.objs >> @@ -17,3 +17,5 @@ common-obj-$(CONFIG_SOFTMMU) += loader.o >> common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o >> common-obj-$(CONFIG_SOFTMMU) += register.o >> common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o >> + >> +obj-$(CONFIG_SOFTMMU) += generic-loader.o >> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c >> new file mode 100644 >> index 0000000..4ee71cb >> --- /dev/null >> +++ b/hw/core/generic-loader.c >> @@ -0,0 +1,195 @@ >> +/* >> + * Generic Loader >> + * >> + * Copyright (C) 2014 Li Guang >> + * Copyright (C) 2016 Xilinx Inc. >> + * Written by Li Guang >> + * >> + * 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 "qom/cpu.h" >> +#include "hw/sysbus.h" >> +#include "sysemu/dma.h" >> +#include "hw/loader.h" >> +#include "qapi/error.h" >> +#include "hw/core/generic-loader.h" >> + >> +#define CPU_NONE 0xFFFFFFFF >> + >> +static void generic_loader_reset(void *opaque) >> +{ >> + GenericLoaderState *s = GENERIC_LOADER(opaque); >> + >> + if (s->set_pc && s->cpu) { > > This check on s->cpu means that -device loader,file=<...> won't set the > PC to the entry point unless you specifically pass cpu-num=0. > The easiest fix for this is just to remove it (see below). Fixed! > >> + CPUClass *cc = CPU_GET_CLASS(s->cpu); >> + cpu_reset(s->cpu); >> + if (cc) { >> + cc->set_pc(s->cpu, s->addr); >> + } >> + } >> + >> + 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); >> + } >> +} >> + >> +static void generic_loader_realize(DeviceState *dev, Error **errp) >> +{ >> + GenericLoaderState *s = GENERIC_LOADER(dev); >> + hwaddr entry; >> + int big_endian; >> + int size = 0; >> + >> + s->set_pc = false; >> + >> + /* Perform some error checking on the user's options */ >> + if (s->data || s->data_len || s->data_be) { >> + /* User is loading memory values */ >> + if (s->file) { >> + error_setg(errp, "Specifying a file is not supported when loading " >> + "memory values"); >> + return; >> + } else if (s->force_raw) { >> + error_setg(errp, "Specifying force-raw is not supported when " >> + "loading memory values"); >> + return; >> + } else if (!s->data || !s->data_len) { >> + error_setg(errp, "Both data and data-len must be specified"); >> + return; >> + } else if (s->data_len > 8) { >> + error_setg(errp, "data-len cannot be greater then 8 bytes"); >> + return; >> + } else if (s->data_len > sizeof(s->data)) { >> + error_setg(errp, "data-len cannot be more then the data size"); > > This can't ever happen, because sizeof(s->data) is always 8. Ok, removed. > >> + return; >> + } >> + } else if (s->file || s->force_raw) { >> + /* User is loading an image */ >> + if (s->data || s->data_len || s->data_be) { >> + error_setg(errp, "data can not be specified when loading an " >> + "image"); >> + return; >> + } >> + s->set_pc = true; >> + } else if (s->addr) { >> + /* User is setting the PC */ >> + if (s->data || s->data_len || s->data_be) { >> + error_setg(errp, "data can not be specified when setting a " >> + "program counter"); >> + return; >> + } > > Failing to specify cpu_num in this case needs to be an error. Fixed. > >> + s->set_pc = true; >> + } else { >> + /* Did the user specify anything? */ >> + error_setg(errp, "please include valid arguments"); >> + return; >> + } > > >> + >> + qemu_register_reset(generic_loader_reset, dev); >> + >> + if (s->cpu_num != CPU_NONE) { >> + s->cpu = qemu_get_cpu(s->cpu_num); >> + if (!s->cpu) { >> + error_setg(errp, "Specified boot CPU#%d is nonexistent", >> + s->cpu_num); >> + return; >> + } >> + } > > If you always set s->cpu to be first_cpu in the "cpu_num is CPU_NONE" > case you can avoid all the ?: everywhere you use it, and you don't need > to check for 's->cpu' being non-NULL in the function where you set the PC. Fixed. > >> + >> +#ifdef TARGET_WORDS_BIGENDIAN >> + big_endian = 1; >> +#else >> + big_endian = 0; >> +#endif >> + >> + if (s->file) { >> + if (!s->force_raw) { >> + size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL, >> + big_endian, 0, 0, 0, >> + (s->cpu ? s->cpu : first_cpu)->as); >> + >> + if (size < 0) { >> + size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL, >> + (s->cpu ? s->cpu : first_cpu)->as); >> + } >> + } >> + >> + if (size < 0 || s->force_raw) { >> + /* Default to the maximum size being the machine's ram size */ >> + size = load_image_targphys_as(s->file, s->addr, ram_size, >> + (s->cpu ? s->cpu : first_cpu)->as); >> + } else { >> + s->addr = entry; >> + } >> + >> + if (size < 0) { >> + error_setg(errp, "Cannot load specified image %s", s->file); >> + return; >> + } >> + } > > Otherwise: > > Reviewed-by: Peter Maydell Thanks! Alistair > > thanks > -- PMM >