Re: [PATCH v3 12/20] nubus: move nubus to its own 32-bit address space

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 7965 bytes --]

On Thu, 16 Sep 2021, Mark Cave-Ayland wrote:
> According to "Designing Cards and Drivers for the Macintosh Family" the Nubus
> has its own 32-bit address space based upon physical slot addressing.
>
> Move Nubus to its own 32-bit address space and then use memory region aliases
> to map available slot and super slot ranges into the q800 system address
> space via the Macintosh Nubus bridge.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/m68k/q800.c                      |  8 +++-----
> hw/nubus/mac-nubus-bridge.c         | 15 +++++++++++++--
> hw/nubus/nubus-bus.c                | 18 ++++++++++++++++++
> hw/nubus/nubus-device.c             |  2 +-
> include/hw/nubus/mac-nubus-bridge.h |  2 ++
> include/hw/nubus/nubus.h            | 10 +++++++---
> 6 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 5ba87f789c..0a0051a296 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -67,9 +67,6 @@
> #define ASC_BASE              (IO_BASE + 0x14000)
> #define SWIM_BASE             (IO_BASE + 0x1E000)
>
> -#define NUBUS_SUPER_SLOT_BASE 0x60000000
> -#define NUBUS_SLOT_BASE       0xf0000000
> -
> #define SONIC_PROM_SIZE       0x1000
>
> /*
> @@ -396,8 +393,9 @@ static void q800_init(MachineState *machine)
>
>     dev = qdev_new(TYPE_MAC_NUBUS_BRIDGE);
>     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, NUBUS_SUPER_SLOT_BASE);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 9 * NUBUS_SUPER_SLOT_SIZE);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
> +                                            9 * NUBUS_SLOT_SIZE);
>
>     nubus = MAC_NUBUS_BRIDGE(dev)->bus;
>
> diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
> index c1d77e2bc7..574bc7831e 100644
> --- a/hw/nubus/mac-nubus-bridge.c
> +++ b/hw/nubus/mac-nubus-bridge.c
> @@ -21,8 +21,19 @@ static void mac_nubus_bridge_init(Object *obj)
>     /* Macintosh only has slots 0x9 to 0xe available */
>     s->bus->slot_available_mask = MAKE_64BIT_MASK(9, 6);
>
> -    sysbus_init_mmio(sbd, &s->bus->super_slot_io);
> -    sysbus_init_mmio(sbd, &s->bus->slot_io);
> +    /* Aliases for slots 0x9 to 0xe */
> +    memory_region_init_alias(&s->super_slot_alias, obj, "super-slot-alias",
> +                             &s->bus->nubus_mr,
> +                             9 * NUBUS_SUPER_SLOT_SIZE,
> +                             6 * NUBUS_SUPER_SLOT_SIZE);

Sorry for not spotting it yesterday in v2 but I only had time to have a 
closer look now. Do these 9 and 6 worth a #define? Are these something 
like MAC_FIST_SLOT and MAC_NUM_SLOTS? As they maybe always appear together 
with NUBUS_SUPER_SLOT_SIZE (I haven't checked all but most look like that) 
so those products could have a #define just to make it shorter in these 
calls. (Are those the #defines that you've removed above?) Maybe

#define MAC_FIRST_SLOT 9
#define MAC_NUM_SLOTS  6

then use these to

#define MAC_SLOTS_MASK  MAKE_64BIT_MASK(MAC_FIRST_SLOT, MAC_NUM_SLOTS)

and similarly the memory address and size as

#define MAC_SLOT_BASE  9 * NUBUS_SUPER_SLOT_SIZE 
#define MAC_SLOT_SIZE  6 * NUBUS_SUPER_SLOT_SIZE

or so and then use these latter three where they appear now open coded 
could be shorter and clearer but I don't mind either way so if you want to 
keep the current version that's OK with me as well. (I may have got the 
names for these wrong but hopefully you get the idea, I haven't tried to 
understand in detail what these really are.)

Regards,
BALATON Zoltan

> +
> +    memory_region_init_alias(&s->slot_alias, obj, "slot-alias",
> +                             &s->bus->nubus_mr,
> +                             NUBUS_SLOT_BASE + 9 * NUBUS_SLOT_SIZE,
> +                             6 * NUBUS_SLOT_SIZE);
> +
> +    sysbus_init_mmio(sbd, &s->super_slot_alias);
> +    sysbus_init_mmio(sbd, &s->slot_alias);
> }
>
> static void mac_nubus_bridge_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
> index a617459a4f..ab3515fb75 100644
> --- a/hw/nubus/nubus-bus.c
> +++ b/hw/nubus/nubus-bus.c
> @@ -70,25 +70,42 @@ static const MemoryRegionOps nubus_super_slot_ops = {
>     },
> };
>
> +static void nubus_unrealize(BusState *bus)
> +{
> +    NubusBus *nubus = NUBUS_BUS(bus);
> +
> +    address_space_destroy(&nubus->nubus_as);
> +}
> +
> static void nubus_realize(BusState *bus, Error **errp)
> {
> +    NubusBus *nubus = NUBUS_BUS(bus);
> +
>     if (!nubus_find()) {
>         error_setg(errp, "at most one %s device is permitted", TYPE_NUBUS_BUS);
>         return;
>     }
> +
> +    address_space_init(&nubus->nubus_as, &nubus->nubus_mr, "nubus");
> }
>
> static void nubus_init(Object *obj)
> {
>     NubusBus *nubus = NUBUS_BUS(obj);
>
> +    memory_region_init(&nubus->nubus_mr, obj, "nubus", 0x100000000);
> +
>     memory_region_init_io(&nubus->super_slot_io, obj, &nubus_super_slot_ops,
>                           nubus, "nubus-super-slots",
>                           NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
> +    memory_region_add_subregion(&nubus->nubus_mr, 0x0, &nubus->super_slot_io);
>
>     memory_region_init_io(&nubus->slot_io, obj, &nubus_slot_ops,
>                           nubus, "nubus-slots",
>                           NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
> +    memory_region_add_subregion(&nubus->nubus_mr,
> +                                NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE,
> +                                &nubus->slot_io);
>
>     nubus->slot_available_mask = MAKE_64BIT_MASK(0, 16);
> }
> @@ -149,6 +166,7 @@ static void nubus_class_init(ObjectClass *oc, void *data)
>     BusClass *bc = BUS_CLASS(oc);
>
>     bc->realize = nubus_realize;
> +    bc->unrealize = nubus_unrealize;
>     bc->check_address = nubus_check_address;
>     bc->get_dev_path = nubus_get_dev_path;
> }
> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
> index dbb3bb7efd..bb574f970a 100644
> --- a/hw/nubus/nubus-device.c
> +++ b/hw/nubus/nubus-device.c
> @@ -26,7 +26,7 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>     int ret;
>
>     /* Super */
> -    slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE;
> +    slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
>
>     name = g_strdup_printf("nubus-super-slot-%x", nd->slot);
>     memory_region_init(&nd->super_slot_mem, OBJECT(dev), name,
> diff --git a/include/hw/nubus/mac-nubus-bridge.h b/include/hw/nubus/mac-nubus-bridge.h
> index 36aa098dd4..650fd24eab 100644
> --- a/include/hw/nubus/mac-nubus-bridge.h
> +++ b/include/hw/nubus/mac-nubus-bridge.h
> @@ -19,6 +19,8 @@ struct MacNubusState {
>     SysBusDevice sysbus_dev;
>
>     NubusBus *bus;
> +    MemoryRegion super_slot_alias;
> +    MemoryRegion slot_alias;
> };
>
> #endif
> diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
> index 0c9f50c32e..0b9f74d4ac 100644
> --- a/include/hw/nubus/nubus.h
> +++ b/include/hw/nubus/nubus.h
> @@ -15,13 +15,14 @@
> #include "qemu/units.h"
>
> #define NUBUS_SUPER_SLOT_SIZE 0x10000000U
> -#define NUBUS_SUPER_SLOT_NB   0x9
> +#define NUBUS_SUPER_SLOT_NB   0xf
>
> +#define NUBUS_SLOT_BASE       (NUBUS_SUPER_SLOT_SIZE * NUBUS_SUPER_SLOT_NB)
> #define NUBUS_SLOT_SIZE       0x01000000
> -#define NUBUS_SLOT_NB         0xF
> +#define NUBUS_SLOT_NB         0xf
>
> #define NUBUS_FIRST_SLOT      0x0
> -#define NUBUS_LAST_SLOT       0xF
> +#define NUBUS_LAST_SLOT       0xf
>
> #define TYPE_NUBUS_DEVICE "nubus-device"
> OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE)
> @@ -34,6 +35,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(NubusBus, NUBUS_BUS)
> struct NubusBus {
>     BusState qbus;
>
> +    AddressSpace nubus_as;
> +    MemoryRegion nubus_mr;
> +
>     MemoryRegion super_slot_io;
>     MemoryRegion slot_io;
>
>