Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7375 bytes --]

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
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]