| Message ID | c69a791c7cad1246f3f34b3993dee4f549b75aa2.1593456926.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Mac Old World ROM experiment | expand |
On 29/06/2020 19:55, BALATON Zoltan wrote: > The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of > the rom region and fall back to loading a binary image with -bios if > loading ELF image failed. This allows testing emulation with a ROM > image from real hardware as well as using an ELF OpenBIOS image. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > v4: use load address from ELF to check if ROM is too big > > hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c > index f8c204ead7..baf3da6f90 100644 > --- a/hw/ppc/mac_oldworld.c > +++ b/hw/ppc/mac_oldworld.c > @@ -59,6 +59,8 @@ > #define NDRV_VGA_FILENAME "qemu_vga.ndrv" > > #define GRACKLE_BASE 0xfec00000 > +#define PROM_BASE 0xffc00000 > +#define PROM_SIZE (4 * MiB) > > static void fw_cfg_boot_set(void *opaque, const char *boot_device, > Error **errp) > @@ -99,6 +101,7 @@ static void ppc_heathrow_init(MachineState *machine) > SysBusDevice *s; > DeviceState *dev, *pic_dev; > BusState *adb_bus; > + uint64_t bios_addr; > int bios_size; > unsigned int smp_cpus = machine->smp.cpus; > uint16_t ppc_boot_device; > @@ -127,24 +130,32 @@ static void ppc_heathrow_init(MachineState *machine) > > memory_region_add_subregion(sysmem, 0, machine->ram); > > - /* allocate and load BIOS */ > - memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, > + /* allocate and load firmware ROM */ > + memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE, > &error_fatal); > + memory_region_add_subregion(sysmem, PROM_BASE, bios); > > - if (bios_name == NULL) > + if (!bios_name) { > bios_name = PROM_FILENAME; > + } > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > - memory_region_add_subregion(sysmem, PROM_ADDR, bios); > - > - /* Load OpenBIOS (ELF) */ > if (filename) { > - bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, NULL, > - 1, PPC_ELF_MACHINE, 0, 0); > + /* Load OpenBIOS (ELF) */ > + bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, > + NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0); > + if (bios_size <= 0) { > + /* or load binary ROM image */ > + bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE); > + bios_addr = PROM_BASE; > + } else { > + /* load_elf sets high 32 bits for some reason, strip those */ > + bios_addr &= 0xffffffffULL; Repeating my earlier comment from v5: something is wrong here if you need to manually strip the high bits. If you compare with SPARC32 which uses the same approach, there is no such strip required - have a look there to try and figure out what's going on here. ATB, Mark.
On Tue, 30 Jun 2020, Mark Cave-Ayland wrote: > On 29/06/2020 19:55, BALATON Zoltan wrote: >> The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of >> the rom region and fall back to loading a binary image with -bios if >> loading ELF image failed. This allows testing emulation with a ROM >> image from real hardware as well as using an ELF OpenBIOS image. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> v4: use load address from ELF to check if ROM is too big >> >> hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++--------- >> 1 file changed, 20 insertions(+), 9 deletions(-) >> >> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c >> index f8c204ead7..baf3da6f90 100644 >> --- a/hw/ppc/mac_oldworld.c >> +++ b/hw/ppc/mac_oldworld.c >> @@ -59,6 +59,8 @@ >> #define NDRV_VGA_FILENAME "qemu_vga.ndrv" >> >> #define GRACKLE_BASE 0xfec00000 >> +#define PROM_BASE 0xffc00000 >> +#define PROM_SIZE (4 * MiB) >> >> static void fw_cfg_boot_set(void *opaque, const char *boot_device, >> Error **errp) >> @@ -99,6 +101,7 @@ static void ppc_heathrow_init(MachineState *machine) >> SysBusDevice *s; >> DeviceState *dev, *pic_dev; >> BusState *adb_bus; >> + uint64_t bios_addr; >> int bios_size; >> unsigned int smp_cpus = machine->smp.cpus; >> uint16_t ppc_boot_device; >> @@ -127,24 +130,32 @@ static void ppc_heathrow_init(MachineState *machine) >> >> memory_region_add_subregion(sysmem, 0, machine->ram); >> >> - /* allocate and load BIOS */ >> - memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, >> + /* allocate and load firmware ROM */ >> + memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE, >> &error_fatal); >> + memory_region_add_subregion(sysmem, PROM_BASE, bios); >> >> - if (bios_name == NULL) >> + if (!bios_name) { >> bios_name = PROM_FILENAME; >> + } >> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> - memory_region_add_subregion(sysmem, PROM_ADDR, bios); >> - >> - /* Load OpenBIOS (ELF) */ >> if (filename) { >> - bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, NULL, >> - 1, PPC_ELF_MACHINE, 0, 0); >> + /* Load OpenBIOS (ELF) */ >> + bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, >> + NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0); >> + if (bios_size <= 0) { >> + /* or load binary ROM image */ >> + bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE); >> + bios_addr = PROM_BASE; >> + } else { >> + /* load_elf sets high 32 bits for some reason, strip those */ >> + bios_addr &= 0xffffffffULL; > > Repeating my earlier comment from v5: something is wrong here if you need to manually > strip the high bits. If you compare with SPARC32 which uses the same approach, there > is no such strip required - have a look there to try and figure out what's going on here. OK, the problem here is this: $ gdb qemu-system-ppc (gdb) b mac_oldworld.c:146 Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146. (gdb) r Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146 146 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); (gdb) n 147 if (filename) { 149 bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, 151 if (bios_size <= 0) { (gdb) p bios_size $1 = 755500 (gdb) p/x bios_addr $2 = 0xfffffffffff00000 this happens within load_elf that I don't feel like wanting to debug but causes problem when we use it to calculate bios size later here: - if (bios_size < 0 || bios_size > BIOS_SIZE) { + if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) { unless we strip it down to expected 32 bits. This could be some unwanted size extension or whatnot but I don't have time to dig deeper. Now lets see what sun4m does: $ gdb qemu-system-sparc (gdb) b sun4m.c:721 Breakpoint 1 at 0x1fc0e6: file hw/sparc/sun4m.c, line 721. (gdb) r Thread 1 "qemu-system-spa" hit Breakpoint 1, prom_init (addr=1879048192, bios_name=0x555555b3c60d "openbios-sparc32") at hw/sparc/sun4m.c:721 721 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); (gdb) p/x addr $1 = 0x70000000 (gdb) n 722 if (filename) { 723 ret = load_elf(filename, NULL, 726 if (ret < 0 || ret > PROM_SIZE_MAX) { (gdb) p ret $2 = 847872 (gdb) p/x addr $3 = 0x70000000 Hmm, does not happen here, the difference is that this calls load_elf with addr already initialised so maybe load_elf only sets low 32 bits? By the way, sun4m does not use the returned addr so even if it was wrong it would not be noticed, Maybe initialising addr before calling load_elf in mac_oldworld,c could fix this so we can get rid of the fix up? Unfortunately not: --- a/hw/ppc/mac_oldworld.c +++ b/hw/ppc/mac_oldworld.c @@ -98,7 +98,7 @@ static void ppc_heathrow_init(MachineState *machine) SysBusDevice *s; DeviceState *dev, *pic_dev; BusState *adb_bus; - uint64_t bios_addr; + uint64_t bios_addr = 0; int bios_size; unsigned int smp_cpus = machine->smp.cpus; uint16_t ppc_boot_device; $ gdb qemu-system-ppc [...] Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146 146 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); (gdb) p bios_addr $1 = 0 (gdb) n 147 if (filename) { 149 bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, 151 if (bios_size <= 0) { (gdb) p/x bios_addr $2 = 0xfffffffffff00000 Could this be something about openbios-ppc? I don't know. I give up investigating further at this point and let someone else find out. Any ideas? Regards, BALATON Zoltan
On Tue, Jun 30, 2020 at 11:45:42PM +0200, BALATON Zoltan wrote: > On Tue, 30 Jun 2020, Mark Cave-Ayland wrote: > > On 29/06/2020 19:55, BALATON Zoltan wrote: > > > The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of > > > the rom region and fall back to loading a binary image with -bios if > > > loading ELF image failed. This allows testing emulation with a ROM > > > image from real hardware as well as using an ELF OpenBIOS image. > > > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > > --- > > > v4: use load address from ELF to check if ROM is too big > > > > > > hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++--------- > > > 1 file changed, 20 insertions(+), 9 deletions(-) > > > > > > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c > > > index f8c204ead7..baf3da6f90 100644 > > > --- a/hw/ppc/mac_oldworld.c > > > +++ b/hw/ppc/mac_oldworld.c > > > @@ -59,6 +59,8 @@ > > > #define NDRV_VGA_FILENAME "qemu_vga.ndrv" > > > > > > #define GRACKLE_BASE 0xfec00000 > > > +#define PROM_BASE 0xffc00000 > > > +#define PROM_SIZE (4 * MiB) > > > > > > static void fw_cfg_boot_set(void *opaque, const char *boot_device, > > > Error **errp) > > > @@ -99,6 +101,7 @@ static void ppc_heathrow_init(MachineState *machine) > > > SysBusDevice *s; > > > DeviceState *dev, *pic_dev; > > > BusState *adb_bus; > > > + uint64_t bios_addr; > > > int bios_size; > > > unsigned int smp_cpus = machine->smp.cpus; > > > uint16_t ppc_boot_device; > > > @@ -127,24 +130,32 @@ static void ppc_heathrow_init(MachineState *machine) > > > > > > memory_region_add_subregion(sysmem, 0, machine->ram); > > > > > > - /* allocate and load BIOS */ > > > - memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, > > > + /* allocate and load firmware ROM */ > > > + memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE, > > > &error_fatal); > > > + memory_region_add_subregion(sysmem, PROM_BASE, bios); > > > > > > - if (bios_name == NULL) > > > + if (!bios_name) { > > > bios_name = PROM_FILENAME; > > > + } > > > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > > > - memory_region_add_subregion(sysmem, PROM_ADDR, bios); > > > - > > > - /* Load OpenBIOS (ELF) */ > > > if (filename) { > > > - bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, NULL, > > > - 1, PPC_ELF_MACHINE, 0, 0); > > > + /* Load OpenBIOS (ELF) */ > > > + bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, > > > + NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0); > > > + if (bios_size <= 0) { > > > + /* or load binary ROM image */ > > > + bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE); > > > + bios_addr = PROM_BASE; > > > + } else { > > > + /* load_elf sets high 32 bits for some reason, strip those */ > > > + bios_addr &= 0xffffffffULL; > > > > Repeating my earlier comment from v5: something is wrong here if you need to manually > > strip the high bits. If you compare with SPARC32 which uses the same approach, there > > is no such strip required - have a look there to try and figure out what's going on here. > > OK, the problem here is this: > > $ gdb qemu-system-ppc > (gdb) b mac_oldworld.c:146 > Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146. > (gdb) r > Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146 > 146 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > (gdb) n > 147 if (filename) { > 149 bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, > 151 if (bios_size <= 0) { > (gdb) p bios_size > $1 = 755500 > (gdb) p/x bios_addr > $2 = 0xfffffffffff00000 > > this happens within load_elf that I don't feel like wanting to debug but > causes problem when we use it to calculate bios size later here: I think the problem is here, in include/hw/elf_ops.h: if (lowaddr) *lowaddr = (uint64_t)(elf_sword)low; "low" is a u64, but for a 32-bit ELF file, which is what I'm guessing you're dealing with here, elf_sword is an int32_t. So the first cast truncates the high bits, but makes it a signed value, so the second cast sign extends, resulting in those high bits. Sign extending rather than zero-extending seems a dubious choice here, so I wonder if that should be (elf_word) instead of (elf_sword). But maybe there's some weird other case where we do want the sign extension here. > > - if (bios_size < 0 || bios_size > BIOS_SIZE) { > + if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) { > > unless we strip it down to expected 32 bits. This could be some unwanted > size extension or whatnot but I don't have time to dig deeper. > > Now lets see what sun4m does: > > $ gdb qemu-system-sparc > (gdb) b sun4m.c:721 > Breakpoint 1 at 0x1fc0e6: file hw/sparc/sun4m.c, line 721. > (gdb) r > Thread 1 "qemu-system-spa" hit Breakpoint 1, prom_init (addr=1879048192, bios_name=0x555555b3c60d "openbios-sparc32") at hw/sparc/sun4m.c:721 > 721 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > (gdb) p/x addr > $1 = 0x70000000 > (gdb) n > 722 if (filename) { > 723 ret = load_elf(filename, NULL, > 726 if (ret < 0 || ret > PROM_SIZE_MAX) { > (gdb) p ret > $2 = 847872 > (gdb) p/x addr > $3 = 0x70000000 > > Hmm, does not happen here, the difference is that this calls load_elf with > addr already initialised so maybe load_elf only sets low 32 bits? By the > way, sun4m does not use the returned addr so even if it was wrong it would > not be noticed, > > Maybe initialising addr before calling load_elf in mac_oldworld,c could fix > this so we can get rid of the fix up? Unfortunately not: > > --- a/hw/ppc/mac_oldworld.c > +++ b/hw/ppc/mac_oldworld.c > @@ -98,7 +98,7 @@ static void ppc_heathrow_init(MachineState *machine) > SysBusDevice *s; > DeviceState *dev, *pic_dev; > BusState *adb_bus; > - uint64_t bios_addr; > + uint64_t bios_addr = 0; > int bios_size; > unsigned int smp_cpus = machine->smp.cpus; > uint16_t ppc_boot_device; > > $ gdb qemu-system-ppc > [...] > Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146 > 146 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > (gdb) p bios_addr > $1 = 0 > (gdb) n > 147 if (filename) { > 149 bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, > 151 if (bios_size <= 0) { > (gdb) p/x bios_addr > $2 = 0xfffffffffff00000 > > Could this be something about openbios-ppc? I don't know. I give up > investigating further at this point and let someone else find out. > Any ideas? > > Regards, > BALATON Zoltan >
On Sun, 5 Jul 2020, David Gibson wrote: > On Tue, Jun 30, 2020 at 11:45:42PM +0200, BALATON Zoltan wrote: >> On Tue, 30 Jun 2020, Mark Cave-Ayland wrote: >>> On 29/06/2020 19:55, BALATON Zoltan wrote: >>>> The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of >>>> the rom region and fall back to loading a binary image with -bios if >>>> loading ELF image failed. This allows testing emulation with a ROM >>>> image from real hardware as well as using an ELF OpenBIOS image. >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> v4: use load address from ELF to check if ROM is too big >>>> >>>> hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++--------- >>>> 1 file changed, 20 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c >>>> index f8c204ead7..baf3da6f90 100644 >>>> --- a/hw/ppc/mac_oldworld.c >>>> +++ b/hw/ppc/mac_oldworld.c >>>> @@ -59,6 +59,8 @@ >>>> #define NDRV_VGA_FILENAME "qemu_vga.ndrv" >>>> >>>> #define GRACKLE_BASE 0xfec00000 >>>> +#define PROM_BASE 0xffc00000 >>>> +#define PROM_SIZE (4 * MiB) >>>> >>>> static void fw_cfg_boot_set(void *opaque, const char *boot_device, >>>> Error **errp) >>>> @@ -99,6 +101,7 @@ static void ppc_heathrow_init(MachineState *machine) >>>> SysBusDevice *s; >>>> DeviceState *dev, *pic_dev; >>>> BusState *adb_bus; >>>> + uint64_t bios_addr; >>>> int bios_size; >>>> unsigned int smp_cpus = machine->smp.cpus; >>>> uint16_t ppc_boot_device; >>>> @@ -127,24 +130,32 @@ static void ppc_heathrow_init(MachineState *machine) >>>> >>>> memory_region_add_subregion(sysmem, 0, machine->ram); >>>> >>>> - /* allocate and load BIOS */ >>>> - memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, >>>> + /* allocate and load firmware ROM */ >>>> + memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE, >>>> &error_fatal); >>>> + memory_region_add_subregion(sysmem, PROM_BASE, bios); >>>> >>>> - if (bios_name == NULL) >>>> + if (!bios_name) { >>>> bios_name = PROM_FILENAME; >>>> + } >>>> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >>>> - memory_region_add_subregion(sysmem, PROM_ADDR, bios); >>>> - >>>> - /* Load OpenBIOS (ELF) */ >>>> if (filename) { >>>> - bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, NULL, >>>> - 1, PPC_ELF_MACHINE, 0, 0); >>>> + /* Load OpenBIOS (ELF) */ >>>> + bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, >>>> + NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0); >>>> + if (bios_size <= 0) { >>>> + /* or load binary ROM image */ >>>> + bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE); >>>> + bios_addr = PROM_BASE; >>>> + } else { >>>> + /* load_elf sets high 32 bits for some reason, strip those */ >>>> + bios_addr &= 0xffffffffULL; >>> >>> Repeating my earlier comment from v5: something is wrong here if you need to manually >>> strip the high bits. If you compare with SPARC32 which uses the same approach, there >>> is no such strip required - have a look there to try and figure out what's going on here. >> >> OK, the problem here is this: >> >> $ gdb qemu-system-ppc >> (gdb) b mac_oldworld.c:146 >> Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146. >> (gdb) r >> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146 >> 146 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> (gdb) n >> 147 if (filename) { >> 149 bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, >> 151 if (bios_size <= 0) { >> (gdb) p bios_size >> $1 = 755500 >> (gdb) p/x bios_addr >> $2 = 0xfffffffffff00000 >> >> this happens within load_elf that I don't feel like wanting to debug but >> causes problem when we use it to calculate bios size later here: > > I think the problem is here, in include/hw/elf_ops.h: > > if (lowaddr) > *lowaddr = (uint64_t)(elf_sword)low; > > "low" is a u64, but for a 32-bit ELF file, which is what I'm guessing > you're dealing with here, elf_sword is an int32_t. So the first cast > truncates the high bits, but makes it a signed value, so the second > cast sign extends, resulting in those high bits. Thanks for finding this, this looks like a likely reason. > Sign extending rather than zero-extending seems a dubious choice here, > so I wonder if that should be (elf_word) instead of (elf_sword). But > maybe there's some weird other case where we do want the sign > extension here. Looks like most callers which get this value don't even use it, I've submitted a patch to clean that up. Of the remaining callers at least hppa does similar truncating but uses a cast instead of explicit masking. I'll update my mac patches to do the same. Regards, BALATON Zoltan >> - if (bios_size < 0 || bios_size > BIOS_SIZE) { >> + if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) { >> >> unless we strip it down to expected 32 bits. This could be some unwanted >> size extension or whatnot but I don't have time to dig deeper. >> >> Now lets see what sun4m does: >> >> $ gdb qemu-system-sparc >> (gdb) b sun4m.c:721 >> Breakpoint 1 at 0x1fc0e6: file hw/sparc/sun4m.c, line 721. >> (gdb) r >> Thread 1 "qemu-system-spa" hit Breakpoint 1, prom_init (addr=1879048192, bios_name=0x555555b3c60d "openbios-sparc32") at hw/sparc/sun4m.c:721 >> 721 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> (gdb) p/x addr >> $1 = 0x70000000 >> (gdb) n >> 722 if (filename) { >> 723 ret = load_elf(filename, NULL, >> 726 if (ret < 0 || ret > PROM_SIZE_MAX) { >> (gdb) p ret >> $2 = 847872 >> (gdb) p/x addr >> $3 = 0x70000000 >> >> Hmm, does not happen here, the difference is that this calls load_elf with >> addr already initialised so maybe load_elf only sets low 32 bits? By the >> way, sun4m does not use the returned addr so even if it was wrong it would >> not be noticed, >> >> Maybe initialising addr before calling load_elf in mac_oldworld,c could fix >> this so we can get rid of the fix up? Unfortunately not: >> >> --- a/hw/ppc/mac_oldworld.c >> +++ b/hw/ppc/mac_oldworld.c >> @@ -98,7 +98,7 @@ static void ppc_heathrow_init(MachineState *machine) >> SysBusDevice *s; >> DeviceState *dev, *pic_dev; >> BusState *adb_bus; >> - uint64_t bios_addr; >> + uint64_t bios_addr = 0; >> int bios_size; >> unsigned int smp_cpus = machine->smp.cpus; >> uint16_t ppc_boot_device; >> >> $ gdb qemu-system-ppc >> [...] >> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146 >> 146 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> (gdb) p bios_addr >> $1 = 0 >> (gdb) n >> 147 if (filename) { >> 149 bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, >> 151 if (bios_size <= 0) { >> (gdb) p/x bios_addr >> $2 = 0xfffffffffff00000 >> >> Could this be something about openbios-ppc? I don't know. I give up >> investigating further at this point and let someone else find out. >> Any ideas? >> >> Regards, >> BALATON Zoltan >> > >
On 05/07/2020 08:31, David Gibson wrote: > On Tue, Jun 30, 2020 at 11:45:42PM +0200, BALATON Zoltan wrote: >> On Tue, 30 Jun 2020, Mark Cave-Ayland wrote: >>> On 29/06/2020 19:55, BALATON Zoltan wrote: >>>> The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of >>>> the rom region and fall back to loading a binary image with -bios if >>>> loading ELF image failed. This allows testing emulation with a ROM >>>> image from real hardware as well as using an ELF OpenBIOS image. >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> v4: use load address from ELF to check if ROM is too big >>>> >>>> hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++--------- >>>> 1 file changed, 20 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c >>>> index f8c204ead7..baf3da6f90 100644 >>>> --- a/hw/ppc/mac_oldworld.c >>>> +++ b/hw/ppc/mac_oldworld.c >>>> @@ -59,6 +59,8 @@ >>>> #define NDRV_VGA_FILENAME "qemu_vga.ndrv" >>>> >>>> #define GRACKLE_BASE 0xfec00000 >>>> +#define PROM_BASE 0xffc00000 >>>> +#define PROM_SIZE (4 * MiB) >>>> >>>> static void fw_cfg_boot_set(void *opaque, const char *boot_device, >>>> Error **errp) >>>> @@ -99,6 +101,7 @@ static void ppc_heathrow_init(MachineState *machine) >>>> SysBusDevice *s; >>>> DeviceState *dev, *pic_dev; >>>> BusState *adb_bus; >>>> + uint64_t bios_addr; >>>> int bios_size; >>>> unsigned int smp_cpus = machine->smp.cpus; >>>> uint16_t ppc_boot_device; >>>> @@ -127,24 +130,32 @@ static void ppc_heathrow_init(MachineState *machine) >>>> >>>> memory_region_add_subregion(sysmem, 0, machine->ram); >>>> >>>> - /* allocate and load BIOS */ >>>> - memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, >>>> + /* allocate and load firmware ROM */ >>>> + memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE, >>>> &error_fatal); >>>> + memory_region_add_subregion(sysmem, PROM_BASE, bios); >>>> >>>> - if (bios_name == NULL) >>>> + if (!bios_name) { >>>> bios_name = PROM_FILENAME; >>>> + } >>>> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >>>> - memory_region_add_subregion(sysmem, PROM_ADDR, bios); >>>> - >>>> - /* Load OpenBIOS (ELF) */ >>>> if (filename) { >>>> - bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, NULL, >>>> - 1, PPC_ELF_MACHINE, 0, 0); >>>> + /* Load OpenBIOS (ELF) */ >>>> + bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, >>>> + NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0); >>>> + if (bios_size <= 0) { >>>> + /* or load binary ROM image */ >>>> + bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE); >>>> + bios_addr = PROM_BASE; >>>> + } else { >>>> + /* load_elf sets high 32 bits for some reason, strip those */ >>>> + bios_addr &= 0xffffffffULL; >>> >>> Repeating my earlier comment from v5: something is wrong here if you need to manually >>> strip the high bits. If you compare with SPARC32 which uses the same approach, there >>> is no such strip required - have a look there to try and figure out what's going on here. >> >> OK, the problem here is this: >> >> $ gdb qemu-system-ppc >> (gdb) b mac_oldworld.c:146 >> Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146. >> (gdb) r >> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146 >> 146 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> (gdb) n >> 147 if (filename) { >> 149 bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, >> 151 if (bios_size <= 0) { >> (gdb) p bios_size >> $1 = 755500 >> (gdb) p/x bios_addr >> $2 = 0xfffffffffff00000 >> >> this happens within load_elf that I don't feel like wanting to debug but >> causes problem when we use it to calculate bios size later here: > > I think the problem is here, in include/hw/elf_ops.h: > > if (lowaddr) > *lowaddr = (uint64_t)(elf_sword)low; > > "low" is a u64, but for a 32-bit ELF file, which is what I'm guessing > you're dealing with here, elf_sword is an int32_t. So the first cast > truncates the high bits, but makes it a signed value, so the second > cast sign extends, resulting in those high bits. > > Sign extending rather than zero-extending seems a dubious choice here, > so I wonder if that should be (elf_word) instead of (elf_sword). But > maybe there's some weird other case where we do want the sign > extension here. I agree that sign-extending here feels odd since it will cause problems with 32-bit values on 64-bit systems like we see here, however I'm not really familiar enough with the QEMU ELF loader to know what the intent was here. The original reference for this was SPARC32 which does a similar thing, but ultimately the resulting address is never used and the ROM is loaded at a fixed address which is why it doesn't matter here. Given that this needs to work with both qemu-system-ppc and qemu-system-ppc64 would casting bios_addr to target_ulong work? ATB, Mark.
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c index f8c204ead7..baf3da6f90 100644 --- a/hw/ppc/mac_oldworld.c +++ b/hw/ppc/mac_oldworld.c @@ -59,6 +59,8 @@ #define NDRV_VGA_FILENAME "qemu_vga.ndrv" #define GRACKLE_BASE 0xfec00000 +#define PROM_BASE 0xffc00000 +#define PROM_SIZE (4 * MiB) static void fw_cfg_boot_set(void *opaque, const char *boot_device, Error **errp) @@ -99,6 +101,7 @@ static void ppc_heathrow_init(MachineState *machine) SysBusDevice *s; DeviceState *dev, *pic_dev; BusState *adb_bus; + uint64_t bios_addr; int bios_size; unsigned int smp_cpus = machine->smp.cpus; uint16_t ppc_boot_device; @@ -127,24 +130,32 @@ static void ppc_heathrow_init(MachineState *machine) memory_region_add_subregion(sysmem, 0, machine->ram); - /* allocate and load BIOS */ - memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, + /* allocate and load firmware ROM */ + memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE, &error_fatal); + memory_region_add_subregion(sysmem, PROM_BASE, bios); - if (bios_name == NULL) + if (!bios_name) { bios_name = PROM_FILENAME; + } filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); - memory_region_add_subregion(sysmem, PROM_ADDR, bios); - - /* Load OpenBIOS (ELF) */ if (filename) { - bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, NULL, - 1, PPC_ELF_MACHINE, 0, 0); + /* Load OpenBIOS (ELF) */ + bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr, + NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0); + if (bios_size <= 0) { + /* or load binary ROM image */ + bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE); + bios_addr = PROM_BASE; + } else { + /* load_elf sets high 32 bits for some reason, strip those */ + bios_addr &= 0xffffffffULL; + } g_free(filename); } else { bios_size = -1; } - if (bios_size < 0 || bios_size > BIOS_SIZE) { + if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) { error_report("could not load PowerPC bios '%s'", bios_name); exit(1); }
The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of the rom region and fall back to loading a binary image with -bios if loading ELF image failed. This allows testing emulation with a ROM image from real hardware as well as using an ELF OpenBIOS image. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- v4: use load address from ELF to check if ROM is too big hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)