Re: [Qemu-devel] [PATCH v2 3/4] m68k: Add NeXTcube machine

[Thomas Huth]

On 29/06/2019 14.26, Philippe Mathieu-Daudé wrote:
> On 6/28/19 8:15 PM, Thomas Huth wrote:
>> It is still quite incomplete (no SCSI, no floppy emulation, no network,
>> etc.), but the firmware already shows up the debug monitor prompt in the
>> framebuffer display, so at least the very basics are already working.
>>
>> This code has been taken from Bryce Lanham's GSoC 2011 NeXT branch at
>>
>>  https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-cube.c
>>
>> and altered quite a bit to fit the latest interface and coding conventions
>> of the current QEMU.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  hw/m68k/Makefile.objs       |   2 +-
>>  hw/m68k/next-cube.c         | 988 ++++++++++++++++++++++++++++++++++++
>>  include/hw/m68k/next-cube.h |  38 ++
>>  3 files changed, 1027 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/m68k/next-cube.c
>>
>> diff --git a/hw/m68k/Makefile.objs b/hw/m68k/Makefile.objs
>> index 688002cac1..f25854730d 100644
>> --- a/hw/m68k/Makefile.objs
>> +++ b/hw/m68k/Makefile.objs
>> @@ -1,3 +1,3 @@
>>  obj-$(CONFIG_AN5206) += an5206.o mcf5206.o
>>  obj-$(CONFIG_MCF5208) += mcf5208.o mcf_intc.o
>> -obj-$(CONFIG_NEXTCUBE) += next-kbd.o
>> +obj-$(CONFIG_NEXTCUBE) += next-kbd.o next-cube.o
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> new file mode 100644
>> index 0000000000..700d386fb9
>> --- /dev/null
>> +++ b/hw/m68k/next-cube.c
>> @@ -0,0 +1,988 @@
>> +/*
>> + * NeXT Cube System Driver
>> + *
>> + * Copyright (c) 2011 Bryce Lanham
>> + *
>> + * This code is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published
>> + * by the Free Software Foundation; either version 2 of the License,
>> + * or (at your option) any later version.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "exec/hwaddr.h"
>> +#include "exec/address-spaces.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/qtest.h"
>> +#include "hw/hw.h"
>> +#include "hw/m68k/next-cube.h"
>> +#include "hw/boards.h"
>> +#include "hw/loader.h"
>> +#include "hw/scsi/esp.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/char/escc.h" /* ZILOG 8530 Serial Emulation */
>> +#include "hw/block/fdc.h"
>> +#include "qapi/error.h"
>> +#include "ui/console.h"
>> +#include "target/m68k/cpu.h"
>> +
>> +/* #define DEBUG_NEXT */
>> +#ifdef DEBUG_NEXT
>> +#define DPRINTF(fmt, ...) \
>> +    do { printf("NeXT: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do { } while (0)
>> +#endif
>> +
>> +#define TYPE_NEXT_MACHINE MACHINE_TYPE_NAME("next-cube")
>> +#define NEXT_MACHINE(obj) OBJECT_CHECK(NeXTState, (obj), TYPE_NEXT_MACHINE)
>> +
>> +#define ENTRY       0x0100001e
>> +#define RAM_SIZE    0x4000000
>> +#define ROM_FILE    "rom66.bin"
> 
> Where can we find this file to test your work?

Bryce added that file to his repository ... but the one from the
location that you've mentioned in your other mail seems to work fine, too.

[...]
>> +static uint32_t mmio_readb(NeXTState *s, hwaddr addr)
>> +{
>> +    switch (addr) {
>> +    case 0xc000:
>> +        return (s->scr1 >> 24) & 0xFF;
>> +    case 0xc001:
>> +        return (s->scr1 >> 16) & 0xFF;
>> +    case 0xc002:
>> +        return (s->scr1 >> 8)  & 0xFF;
>> +    case 0xc003:
>> +        return (s->scr1 >> 0)  & 0xFF;
> 
> So you have a 32-bit implementation (DMA accessed device?).
> 
> memory::access_with_adjusted_size() already does this work
> for you if you use:
> 
>    .impl.min_access_size = 4,
>    .valid.min_access_size = 1,
>    .valid.max_access_size = 4,

Yeah, it's old code from 2011 ... I'll try to rework it as you suggested.

>> +
>> +    case 0xd000:
>> +        return (s->scr2 >> 24) & 0xFF;
>> +    case 0xd001:
>> +        return (s->scr2 >> 16) & 0xFF;
>> +    case 0xd002:
>> +        return (s->scr2 >> 8)  & 0xFF;
>> +    case 0xd003:
>> +        return (s->scr2 >> 0)  & 0xFF;
>> +    case 0x14020:
>> +        DPRINTF("MMIO Read 0x4020\n");
>> +        return 0x7f;
>> +
>> +    default:
>> +        DPRINTF("MMIO Read B @ %"HWADDR_PRIx"\n", addr);
>> +        return 0x0;
>> +    }
[...]
>> +static uint64_t dma_readfn(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    NeXTState *ns = NEXT_MACHINE(opaque);
>> +
>> +    switch (size) {
>> +    case 4:
>> +        return dma_readl(ns, addr);
> 
> Well, maybe you can directly use dma_readfn prototype for dma_readl, and
> remove this function and dma_writefn (you know we'll ever get 32-bit
> accesses here due to .valid.min/max_access_size = 4).

Funny, bit this function is still triggered with size = 1 when you use
the Rev_2.1_v59.bin firmware file... looks like I need an additional
.impl.min_access_size = 4 to get it right.

>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
[...]
>> +static void next_cube_init(MachineState *machine)
>> +{
>> +    M68kCPU *cpu;
>> +    CPUM68KState *env;
>> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
>> +    MemoryRegion *rom = g_new(MemoryRegion, 1);
>> +    MemoryRegion *mmiomem = g_new(MemoryRegion, 1);
>> +    MemoryRegion *scrmem = g_new(MemoryRegion, 1);
>> +    MemoryRegion *dmamem = g_new(MemoryRegion, 1);
>> +    MemoryRegion *unknownmem = g_new(MemoryRegion, 1);
>> +    MemoryRegion *sysmem = get_system_memory();
>> +    NeXTState *ns = NEXT_MACHINE(machine);
>> +
>> +    /* Initialize the cpu core */
>> +    cpu = M68K_CPU(cpu_create(machine->cpu_type));
>> +    if (!cpu) {
>> +        error_report("Unable to find m68k CPU definition");
>> +        exit(1);
>> +    }
>> +    env = &cpu->env;
>> +
>> +    /* Initialize CPU registers.  */
>> +    env->vbr = 0;
>> +    env->pc  = 0x100001e; /* technically should read vector */
>> +    env->sr  = 0x2700;
>> +
>> +    /* Set internal registers to initial values */
>> +    /*     0x0000XX00 << vital bits */
>> +    ns->scr1 = 0x00011102;
>> +    ns->scr2 = 0x00ff0c80;
>> +
>> +    ns->int_mask = 0x0; /* 88027640; */
>> +    ns->int_status = 0x0; /* 200; */
> 
> What mean those comments?

No clue, they were part of Bryce code. Maybe I should simply remove them.

>> +
>> +    /* Load RTC RAM - TODO: provide possibility to load contents from file 
>> */
>> +    memcpy(ns->rtc_ram, rtc_ram2, 32);
>> +
>> +    /* 64MB RAM starting at 0x4000000  */
>> +    memory_region_allocate_system_memory(ram, NULL, "next.ram", ram_size);
>> +    memory_region_add_subregion(sysmem, 0x4000000, ram);
> 
> 0x04000000
> 
>> +
>> +    /* Framebuffer */
>> +    nextfb_init();
> 
> Todays QEMU style seems to create device in place (here).
> 
>> +
>> +    /* MMIO */
>> +    memory_region_init_io(mmiomem, NULL, &mmio_ops, machine, "next.mmio",
>> +                          0xD0000);
>> +    memory_region_add_subregion(sysmem, 0x2000000, mmiomem);
> 
> 0x02000000

Ok.

>> +
>> +    /* BMAP - acts as a catch-all for now */
>> +    memory_region_init_io(scrmem, NULL, &scr_ops, machine, "next.scr",
>> +                          0x3A7FF);
> 
> 0x3A7FF? 0x3a800 at least, but why not use a 256 * KiB full range?

Ok.

> Wait... Isn't this I/O range of 128KB?

Hmm, yes, according to the Previous sources, it's 128kB only... I'll
have a try with 0x20000, and if there are no regressions, I'll use that
value instead.

>> +    /* TODO: */
>> +    /* Serial */
>> +    /* Network */
>> +    /* SCSI */
> 
> Can you use create_unimplemented_device() here?

There are already patches for these devices available (just not working
yet), so I don't think it's worth the effort to take the detour via the
unimplemented device here.

>> +    /* FIXME: Why does the bios access this memory area? */
>> +    memory_region_allocate_system_memory(unknownmem, NULL, "next.unknown", 
>> 16);
>> +    memory_region_add_subregion(sysmem, 0x820c0020, unknownmem);
> 
> Isn't this uncached access to 0x020c0000?

No clue, but looking at the Previous sources, this seems to be a mirror
of 0x020c0000 indeed. I'll change that accordingly.

[...]
>> diff --git a/include/hw/m68k/next-cube.h b/include/hw/m68k/next-cube.h
>> index 88e94f6595..d7df6a223b 100644
>> --- a/include/hw/m68k/next-cube.h
>> +++ b/include/hw/m68k/next-cube.h
>> @@ -2,6 +2,44 @@
>>  #ifndef NEXT_CUBE_H
>>  #define NEXT_CUBE_H
>>  
>> +enum next_dma_chan {
>> +    NEXTDMA_FD,
>> +    NEXTDMA_ENRX,
>> +    NEXTDMA_ENTX,
>> +    NEXTDMA_SCSI,
>> +    NEXTDMA_SCC,
>> +    NEXTDMA_SND
>> +};
>> +
>> +#define DMA_ENABLE      0x01000000
>> +#define DMA_SUPDATE     0x02000000
>> +#define DMA_COMPLETE    0x08000000
>> +
>> +#define DMA_M2DEV       0x0
>> +#define DMA_SETENABLE   0x00010000
>> +#define DMA_SETSUPDATE  0x00020000
>> +#define DMA_DEV2M       0x00040000
>> +#define DMA_CLRCOMPLETE 0x00080000
>> +#define DMA_RESET       0x00100000
> 
> The DMA code is consequent enough to deserve its own file IMO.

Maybe later ... I'd like to keep the initial patches close to the code
from Bryce if possible.

 Thanks for the review,
  Thomas