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

[BALATON Zoltan]

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

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

> 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. (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.)

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