On 28.04.2013, at 12:16, Andreas Färber wrote:
> 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...
Ah, sorry, my bad :). I didn't see that bios_addr was only defined inside the
branch to specify where to load the image too. Interesting code ;).
>
>>> + 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.
Yes :).
>
> 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
I think having hw/ppc at least listed as CC to qemu-...@nongnu.org is a good
idea. I'm not sure how much hassle it'll be to split maintainership inside of a
directory. Does it mean we have to list all files individually? That'd be
annoying :).
Alex
