Am 28.04.2013 11:44, schrieb Alexander Graf: > > On 28.04.2013, at 02:32, Andreas Färber wrote: > >> This prepares for switching from OpenHack'Ware to OpenBIOS. >> >> Signed-off-by: Andreas Färber <andreas.faer...@web.de> >> --- >> hw/ppc/prep.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c >> index cceab3e..9bb0119 100644 >> --- a/hw/ppc/prep.c >> +++ b/hw/ppc/prep.c >> @@ -41,6 +41,7 @@ >> #include "sysemu/blockdev.h" >> #include "sysemu/arch_init.h" >> #include "exec/address-spaces.h" >> +#include "elf.h" >> >> //#define HARD_DEBUG_PPC_IO >> //#define DEBUG_PPC_IO >> @@ -502,18 +503,22 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) >> bios_name = BIOS_FILENAME; >> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> if (filename) { >> - bios_size = get_image_size(filename); >> + bios_size = load_elf(filename, NULL, NULL, NULL, >> + NULL, NULL, 1, ELF_MACHINE, 0); > > This leaves bios_addr unset, no?
bios_addr is not yet defined in this scope and doesn't seem needed here? It's been a while that I wrote this (extracted it from a larger patch since Fabien had a need for ELF support), thought I copied this from one of the other ppc machines at the time... >> + if (bios_size < 0) { >> + bios_size = get_image_size(filename); >> + if (bios_size > 0 && bios_size <= BIOS_SIZE) { >> + hwaddr bios_addr; >> + bios_size = (bios_size + 0xfff) & ~0xfff; >> + bios_addr = (uint32_t)(-bios_size); >> + bios_size = load_image_targphys(filename, bios_addr, >> bios_size); >> + } >> + } >> } else { >> bios_size = -1; >> } >> - if (bios_size > 0 && bios_size <= BIOS_SIZE) { >> - hwaddr bios_addr; >> - bios_size = (bios_size + 0xfff) & ~0xfff; >> - bios_addr = (uint32_t)(-bios_size); >> - bios_size = load_image_targphys(filename, bios_addr, bios_size); >> - } >> if (bios_size < 0 || bios_size > BIOS_SIZE) { >> - hw_error("qemu: could not load PPC PREP bios '%s'\n", bios_name); >> + fprintf(stderr, "qemu: could not load PPC PREP bios '%s'\n", >> bios_name); > > You probably want to split this up. Bios_size < 0 means that image loading > failed, which is always a fatal error. Bios_size > BIOS_SIZE should only > really apply to flat image loading, I think. Sure, I might even pull that out of this patch for - I believe - this was fixing a crash since the CPU was not yet properly initialized at this point. IIUC you are suggesting to move the bios_size > BIOS_SIZE check to after get_image_size() and leave only the bios_size < 0 check here for both code paths. Andreas P.S. I am happy about your review comments, but you were not intentionally CC'ed on a PReP patch - this is apparently the result of Paolo having used hw/ppc/ pattern in the ppc TCG guest core section, which used to be target-ppc/ only. Should we exclude prep.c and future PReP files or even hw/ppc/ from that MAINTAINERS section? Similar question for most other architectures - TCG translation maintenance and device maintenance used to be two different responsibilities.