From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36141) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWOf8-0007Vx-QJ for qemu-devel@nongnu.org; Sun, 28 Apr 2013 06:16:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWOf7-0003OA-JI for qemu-devel@nongnu.org; Sun, 28 Apr 2013 06:16:54 -0400 Message-ID: <517CF710.3000900@web.de> Date: Sun, 28 Apr 2013 12:16:48 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1367109168-3673-1-git-send-email-andreas.faerber@web.de> <7F54D756-8594-4181-A2FC-75E0DA568770@suse.de> In-Reply-To: <7F54D756-8594-4181-A2FC-75E0DA568770@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH prep for-1.5] prep: Add ELF support for -bios List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , Paolo Bonzini Cc: PReP , mark.cave-ayland@ilande.co.uk, =?ISO-8859-1?Q?Herv=E9_?= =?ISO-8859-1?Q?Poussineau?= , qemu-devel@nongnu.org, chouteau@adacore.com 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 >> --- >> 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.