| Message ID | 1367109168-3673-1-git-send-email-andreas.faerber@web.de |
|---|---|
| State | New |
| Headers | show |
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>
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 >
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.
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
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
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.
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);
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(-)