[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH prep for-1.5] prep: Add ELF support for -bios
From: |
Andreas Färber |
Subject: |
Re: [Qemu-ppc] [PATCH prep for-1.5] prep: Add ELF support for -bios |
Date: |
Sun, 28 Apr 2013 12:16:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 |
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 <address@hidden>
>> ---
>> 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.