Re: [Qemu-ppc] [PATCH prep for-1.5] prep: Add ELF support for -bios

[Andreas Färber]

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.