|
From: | BALATON Zoltan |
Subject: | Re: [PATCH v2 1/5] mac_oldworld: Allow loading binary ROM image |
Date: | Sun, 14 Jun 2020 16:46:33 +0200 (CEST) |
User-agent: | Alpine 2.22 (BSF 395 2020-01-19) |
On Sun, 14 Jun 2020, Mark Cave-Ayland wrote:
On 13/06/2020 14:36, BALATON Zoltan wrote:The G3 beige machine has a 4MB firmware ROM. Fix the size of the rom region and allow loading a binary image with -bios. This makes it possible to test emulation with a ROM image from real hardware. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/ppc/mac_oldworld.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c index 0b4c1c6373..3812adc441 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) @@ -127,24 +129,28 @@ 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, NULL, 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); + } g_free(filename); } else { bios_size = -1; } - if (bios_size < 0 || bios_size > BIOS_SIZE) { + if (bios_size < 0 || bios_size > PROM_SIZE) { error_report("could not load PowerPC bios '%s'", bios_name); exit(1); }I think the logic could be improved a bit here: load_elf() can return the physical address from the ELF, so it would make sense to use that as the address for load_image_targphys() if present, and otherwise fall back to loading at 0xffc00000.
I don't get this. No need to do it that way because load_elf already loads the image at address specified in ELF file (I guess because it still works with OpenBIOS after this patch) so don't have to call load_image_targphys for that case. The above tries load_elf and only if it did not succeed calls load_image_targphys to load a binary image to fill the ROM. I don't see how this logic could be simpler.
Maybe we need the load address from the ELF to check if an ELF would overflow the region as in elf_addr + bios_size > PROM_ADDR + PROM_SIZE but I'm not sure. Any suggestion?
It may also make sense to split PROM_ADDR to PROM_ADDR_OLDWORLD and PROM_ADDR_NEWWORLD (and similar for BIOS_SIZE) to allow these values to be adjusted separately for each machine.
BIOS_SIZE is not used in this board after this patch any more so that's basically PROM_SIZE_NEWWORLD now which can be defined in mac_newworld and removed from mac.h. Then we have separate PROM_SIZE for each board. I've also defined PROM_ADDR here in mac_oldworld and similar define can be added to mac_newworld if needed. These should not be in mac.h I think as these are board specific. I regard the previous BIOS_* values specific to OpenBIOS not to boards so now that boards can use other ROMs not just OpenBIOS BIOS_SIZE may not be needed, what we need is the size of the ROM chip on board instead.
Regards, BALATON Zoltan
[Prev in Thread] | Current Thread | [Next in Thread] |