Re: [PATCH v2 for-8.2] ppc/amigaone: Allow running AmigaOS without firmware image

[Nicholas Piggin]

On Mon Nov 27, 2023 at 9:43 PM AEST, BALATON Zoltan wrote:
> On Mon, 27 Nov 2023, Nicholas Piggin wrote:
> > On Sun Nov 26, 2023 at 2:34 AM AEST, BALATON Zoltan wrote:
> >> The machine uses a modified U-Boot under GPL license but the sources
> >> of it are lost with only a binary available so it cannot be included
> >> in QEMU. Allow running without the firmware image with -bios none
> >> which can be used when calling a boot loader directly and thus
> >> simplifying booting guests. We need a small routine that AmigaOS calls
> >> from ROM which is added in this case to allow booting AmigaOS without
> >> external firmware image.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >> v2: Unfortunately AmigaOS needs some additional ROM part which is added
> >> Please merge for 8.2 as it allows booting AmigaOS simpler without
> >> having to download separate firmware.
> >
> > How to test this?
>
> You can check with -M amigaone -bios none then from QEMU monitor
> (qemu) xp/10i 0xfff7ff80

Okay, so to fully test it you really need a non-free AmigaOS image?

And, how does a user know how or when to use this? Should it just be
default if there is no bios binary found?

>
> >>  hw/ppc/amigaone.c | 20 +++++++++++++++++---
> >>  1 file changed, 17 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> >> index 992a55e632..a11d2d5556 100644
> >> --- a/hw/ppc/amigaone.c
> >> +++ b/hw/ppc/amigaone.c
> >> @@ -40,6 +40,16 @@
> >>  #define PROM_ADDR 0xfff00000
> >>  #define PROM_SIZE (512 * KiB)
> >>
> >> +/* AmigaOS calls this routine from ROM, use this if -bios none */
> >> +static const char dummy_fw[] = {
> >> +    0x38, 0x00, 0x00, 0x08, /* li      r0,8 */
> >> +    0x7c, 0x09, 0x03, 0xa6, /* mtctr   r0 */
> >> +    0x54, 0x63, 0xf8, 0x7e, /* srwi    r3,r3,1 */
> >> +    0x42, 0x00, 0xff, 0xfc, /* bdnz    0x8 */
> >> +    0x7c, 0x63, 0x18, 0xf8, /* not     r3,r3 */
> >> +    0x4e, 0x80, 0x00, 0x20, /* blr */
> >> +};
> >
> > This is clever, but does anything else create blobs like this?
>
> There are some examples in hw/mips/{loongson3_virt.c, malta.c} at least 
> and maybe others that put code in guest memory. If this was longer than 
> this few instructions I'd consider putting it in a binary but this seems 
> simpler for such small code.

Thanks, compiling blob inline looks fine then.

It's 0x80 bytes from the end of prom AFAIKS. Should you make that
PROM_ADDR + PROM_SIZE - 0x80 instead of hard coding it?

>
> > It could be put into a .S in pc-bios, which might be a bit more
> > consistent.
> >
> > We might make a ppc/ subdirectory under there, but that's for
> > another time.
>
> Maybe later we could reorganise these unless it's really necessary to 
> change this for 8.2 now.

Nah that's fine.

> (The mips boards and some arm and riscv machines 
> seem to use rom_add_blob_fixed() which sould show up in info roms under 
> monitor so maybe I could look at changing to use that now if you think it 
> would be better that way.)

I'm not sure, I don't think it's necessary if your minimal patch works.

I'll do a PR for 8.2 for SLOF and Skiboot updates, so happy to include
this as well.

Thanks,
Nick

>
> Regards,
> BALATON Zoltan
>
> > Thanks,
> > Nick
> >
> >> +
> >>  static void amigaone_cpu_reset(void *opaque)
> >>  {
> >>      PowerPCCPU *cpu = opaque;
> >> @@ -94,17 +104,21 @@ static void amigaone_init(MachineState *machine)
> >>      }
> >>
> >>      /* allocate and load firmware */
> >> +    rom = g_new(MemoryRegion, 1);
> >> +    memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
> >> +    memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
> >>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, fwname);
> >>      if (filename) {
> >> -        rom = g_new(MemoryRegion, 1);
> >> -        memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
> >> -        memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
> >>          sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE);
> >>          if (sz <= 0 || sz > PROM_SIZE) {
> >>              error_report("Could not load firmware '%s'", filename);
> >>              exit(1);
> >>          }
> >>          g_free(filename);
> >> +    } else if (!strcmp(fwname, "none")) {
> >> +        address_space_write_rom(&address_space_memory, 0xfff7ff80,
> >> +                                MEMTXATTRS_UNSPECIFIED, dummy_fw,
> >> +                                ARRAY_SIZE(dummy_fw));
> >>      } else if (!qtest_enabled()) {
> >>          error_report("Could not find firmware '%s'", fwname);
> >>          exit(1);
> >
> >
> >