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

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

[Andreas Färber]

This prepares for switching from OpenHack'Ware to OpenBIOS.

Signed-off-by: Andreas Färber <andreas.faerber@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);
+        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);
     }
     if (filename) {
         g_free(filename);
-- 
1.8.1.4

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

[Hervé Poussineau]

Andreas Färber a écrit :
> This prepares for switching from OpenHack'Ware to OpenBIOS.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  hw/ppc/prep.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
>   
Acked-by: Hervé Poussineau <hpoussin@reactos.org>

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

[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.faerber@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?

> +        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.


Alex

>     }
>     if (filename) {
>         g_free(filename);
> -- 
> 1.8.1.4
> 

Re: [Qemu-devel] [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 <andreas.faerber@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.

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

[Alexander Graf]

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.faerber@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-ppc@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

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

[Andreas Färber]

Am 28.04.2013 12:22, schrieb Alexander Graf:
> 
> On 28.04.2013, at 12:16, Andreas Färber wrote:
> 
>> 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-ppc@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 :).

Well, what I was rather thinking of was removing F: hw/foo/ from all
"Guest CPU cores (TCG)" sections. We already have boards listed
individually under "PowerPC Machines", so this seems duplicate here;
alternatively it would certainly be possible to add a new heading for
hw/foo/ entries. I do not remember seeing any discussion about this
MAINTAINERS change nor us ack'ing it, so reverting would seem the most
natural solution.

Exclusion in the current scheme would mean adding:
X: hw/ppc/prep.c

Andreas

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

[Fabien Chouteau]

On 04/28/2013 12:16 PM, 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.faerber@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...

Thanks Andreas, that will be very useful.

-- 
Fabien Chouteau