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

[BALATON Zoltan]

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