Re: [PATCH 08/30] q800: reimplement mac-io region aliasing using IO memory region

[Laurent Vivier <laurent@vivier.eu>]

Le 24/05/2023 à 23:10, Mark Cave-Ayland a écrit :
> The current use of aliased memory regions causes us 2 problems: firstly the
> output of "info qom-tree" is absolutely huge and difficult to read, and
> secondly we have already reached the internal limit for memory regions as
> adding any new memory region into the mac-io region causes QEMU to assert
> with "phys_section_add: Assertion `map->sections_nb < TARGET_PAGE_SIZE'
> failed".
> 
> Implement the mac-io region aliasing using a single IO memory region that
> applies IO_SLICE_MASK representing the maximum size of the aliased region and
> then forwarding the access to the existing mac-io memory region using the
> address space API.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/q800.c         | 100 +++++++++++++++++++++++++++++++++--------
>   include/hw/m68k/q800.h |   1 +
>   2 files changed, 82 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 6399631ed0..f15f1eaff9 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -59,6 +59,7 @@
>   
>   #define IO_BASE               0x50000000
>   #define IO_SLICE              0x00040000
> +#define IO_SLICE_MASK         (IO_SLICE - 1)
>   #define IO_SIZE               0x04000000
>   
>   #define VIA_BASE              (IO_BASE + 0x00000)
> @@ -361,6 +362,68 @@ static uint8_t fake_mac_rom[] = {
>       0x60, 0xFE                          /* bras [self] */
>   };
>   
> +static MemTxResult macio_alias_read(void *opaque, hwaddr addr, uint64_t *data,
> +                                    unsigned size, MemTxAttrs attrs)
> +{
> +    MemTxResult r;
> +    uint32_t val;
> +
> +    addr &= IO_SLICE_MASK;
> +    addr |= IO_BASE;
> +
> +    switch (size) {
> +    case 4:
> +        val = address_space_ldl_be(&address_space_memory, addr, attrs, &r);
> +        break;
> +    case 2:
> +        val = address_space_lduw_be(&address_space_memory, addr, attrs, &r);
> +        break;
> +    case 1:
> +        val = address_space_ldub(&address_space_memory, addr, attrs, &r);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    *data = val;
> +    return r;
> +}
> +
> +static MemTxResult macio_alias_write(void *opaque, hwaddr addr, uint64_t value,
> +                                     unsigned size, MemTxAttrs attrs)
> +{
> +    MemTxResult r;
> +
> +    addr &= IO_SLICE_MASK;
> +    addr |= IO_BASE;
> +
> +    switch (size) {
> +    case 4:
> +        address_space_stl_be(&address_space_memory, addr, value, attrs, &r);
> +        break;
> +    case 2:
> +        address_space_stw_be(&address_space_memory, addr, value, attrs, &r);
> +        break;
> +    case 1:
> +        address_space_stb(&address_space_memory, addr, value, attrs, &r);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    return r;
> +}
> +
> +static const MemoryRegionOps macio_alias_ops = {
> +    .read_with_attrs = macio_alias_read,
> +    .write_with_attrs = macio_alias_write,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
> +};
> +
>   static void q800_machine_init(MachineState *machine)
>   {
>       Q800MachineState *m = Q800_MACHINE(machine);
> @@ -371,10 +434,8 @@ static void q800_machine_init(MachineState *machine)
>       int bios_size;
>       ram_addr_t initrd_base;
>       int32_t initrd_size;
> -    MemoryRegion *io;
>       MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
>       uint8_t *prom;
> -    const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
>       int i, checksum;
>       MacFbMode *macfb_mode;
>       ram_addr_t ram_size = machine->ram_size;
> @@ -420,16 +481,10 @@ static void q800_machine_init(MachineState *machine)
>        * Memory from IO_BASE to IO_BASE + IO_SLICE is repeated
>        * from IO_BASE + IO_SLICE to IO_BASE + IO_SIZE
>        */
> -    io = g_new(MemoryRegion, io_slice_nb);
> -    for (i = 0; i < io_slice_nb; i++) {
> -        char *name = g_strdup_printf("mac_m68k.io[%d]", i + 1);
> -
> -        memory_region_init_alias(&io[i], NULL, name, get_system_memory(),
> -                                 IO_BASE, IO_SLICE);
> -        memory_region_add_subregion(get_system_memory(),
> -                                    IO_BASE + (i + 1) * IO_SLICE, &io[i]);
> -        g_free(name);
> -    }
> +    memory_region_init_io(&m->macio_alias, NULL, &macio_alias_ops, &m->macio,
> +                          "mac-io.alias", IO_SIZE - IO_SLICE);
> +    memory_region_add_subregion(get_system_memory(), IO_BASE + IO_SLICE,
> +                                &m->macio_alias);
>   
>       /* IRQ Glue */
>       m->glue = qdev_new(TYPE_GLUE);
> @@ -445,7 +500,8 @@ static void q800_machine_init(MachineState *machine)
>       }
>       sysbus = SYS_BUS_DEVICE(via1_dev);
>       sysbus_realize_and_unref(sysbus, &error_fatal);
> -    sysbus_mmio_map(sysbus, 1, VIA_BASE);
> +    memory_region_add_subregion(&m->macio, VIA_BASE - IO_BASE,
> +                                sysbus_mmio_get_region(sysbus, 1));
>       sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(m->glue, GLUE_IRQ_IN_VIA1));
>       /* A/UX mode */
>       qdev_connect_gpio_out(via1_dev, 0,
> @@ -461,7 +517,8 @@ static void q800_machine_init(MachineState *machine)
>       via2_dev = qdev_new(TYPE_MOS6522_Q800_VIA2);
>       sysbus = SYS_BUS_DEVICE(via2_dev);
>       sysbus_realize_and_unref(sysbus, &error_fatal);
> -    sysbus_mmio_map(sysbus, 1, VIA_BASE + VIA_SIZE);
> +    memory_region_add_subregion(&m->macio, VIA_BASE - IO_BASE + VIA_SIZE,
> +                                sysbus_mmio_get_region(sysbus, 1));
>       sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(m->glue, GLUE_IRQ_IN_VIA2));
>   
>       /* MACSONIC */
> @@ -494,7 +551,8 @@ static void q800_machine_init(MachineState *machine)
>                                OBJECT(get_system_memory()), &error_abort);
>       sysbus = SYS_BUS_DEVICE(dev);
>       sysbus_realize_and_unref(sysbus, &error_fatal);
> -    sysbus_mmio_map(sysbus, 0, SONIC_BASE);
> +    memory_region_add_subregion(&m->macio, SONIC_BASE - IO_BASE,
> +                                sysbus_mmio_get_region(sysbus, 0));
>       sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(m->glue, GLUE_IRQ_IN_SONIC));
>   
>       memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
> @@ -533,7 +591,8 @@ static void q800_machine_init(MachineState *machine)
>       sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1));
>       qdev_connect_gpio_out(DEVICE(escc_orgate), 0,
>                             qdev_get_gpio_in(m->glue, GLUE_IRQ_IN_ESCC));
> -    sysbus_mmio_map(sysbus, 0, SCC_BASE);
> +    memory_region_add_subregion(&m->macio, SCC_BASE - IO_BASE,
> +                                sysbus_mmio_get_region(sysbus, 0));
>   
>       /* SCSI */
>   
> @@ -553,8 +612,10 @@ static void q800_machine_init(MachineState *machine)
>                                                     VIA2_IRQ_SCSI_BIT)));
>       sysbus_connect_irq(sysbus, 1, qemu_irq_invert(qdev_get_gpio_in(via2_dev,
>                                                     VIA2_IRQ_SCSI_DATA_BIT)));
> -    sysbus_mmio_map(sysbus, 0, ESP_BASE);
> -    sysbus_mmio_map(sysbus, 1, ESP_PDMA);
> +    memory_region_add_subregion(&m->macio, ESP_BASE - IO_BASE,
> +                                sysbus_mmio_get_region(sysbus, 0));
> +    memory_region_add_subregion(&m->macio, ESP_PDMA - IO_BASE,
> +                                sysbus_mmio_get_region(sysbus, 1));
>   
>       scsi_bus_legacy_handle_cmdline(&esp->bus);
>   
> @@ -562,7 +623,8 @@ static void q800_machine_init(MachineState *machine)
>   
>       dev = qdev_new(TYPE_SWIM);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, SWIM_BASE);
> +    memory_region_add_subregion(&m->macio, SWIM_BASE - IO_BASE,
> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
>   
>       /* NuBus */
>   
> diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h
> index 156872a124..8d788a7072 100644
> --- a/include/hw/m68k/q800.h
> +++ b/include/hw/m68k/q800.h
> @@ -34,6 +34,7 @@ struct Q800MachineState {
>       MemoryRegion rom;
>       DeviceState *glue;
>       MemoryRegion macio;
> +    MemoryRegion macio_alias;
>   };
>   
>   #define TYPE_Q800_MACHINE MACHINE_TYPE_NAME("q800")

Reviewed-by: Laurent Vivier <laurent@vivier.eu>