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 <bala...@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
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
