| Message ID | 20191031100341.3827-1-laurent@vivier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] q800: fix I/O memory map | expand |
On 10/31/19 11:03 AM, Laurent Vivier wrote: > Linux kernel 5.4 will introduce a new memory map for SWIM device. > (aee6bff1c325 ("m68k: mac: Revisit floppy disc controller base addresses")) > > Until this release all MMIO are mapped between 0x50f00000 and 0x50f40000, > but it appears that for real hardware 0x50f00000 is not the base address: > the MMIO region spans 0x50000000 through 0x60000000, and 0x50040000 through > 0x54000000 is repeated images of 0x50000000 to 0x50040000. > > Fixed: 04e7ca8d0f ("hw/m68k: define Macintosh Quadra 800") > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > hw/m68k/q800.c | 33 +++++++++++++++++++++++++-------- > 1 file changed, 25 insertions(+), 8 deletions(-) > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c > index 2b4842f8c6..8122e7c612 100644 > --- a/hw/m68k/q800.c > +++ b/hw/m68k/q800.c > @@ -60,14 +60,14 @@ > #define MACH_MAC 3 > #define Q800_MAC_CPU_ID 2 > > -#define VIA_BASE 0x50f00000 > -#define SONIC_PROM_BASE 0x50f08000 > -#define SONIC_BASE 0x50f0a000 > -#define SCC_BASE 0x50f0c020 > -#define ESP_BASE 0x50f10000 > -#define ESP_PDMA 0x50f10100 > -#define ASC_BASE 0x50F14000 > -#define SWIM_BASE 0x50F1E000 > +#define VIA_BASE 0x50000000 > +#define SONIC_PROM_BASE 0x50008000 > +#define SONIC_BASE 0x5000a000 > +#define SCC_BASE 0x5000c020 > +#define ESP_BASE 0x50010000 > +#define ESP_PDMA 0x50010100 > +#define ASC_BASE 0x50014000 > +#define SWIM_BASE 0x5001E000 > #define NUBUS_SUPER_SLOT_BASE 0x60000000 > #define NUBUS_SLOT_BASE 0xf0000000 > > @@ -135,6 +135,7 @@ static void q800_init(MachineState *machine) > int32_t initrd_size; > MemoryRegion *rom; > MemoryRegion *ram; > + int i; > ram_addr_t ram_size = machine->ram_size; > const char *kernel_filename = machine->kernel_filename; > const char *initrd_filename = machine->initrd_filename; > @@ -163,10 +164,26 @@ static void q800_init(MachineState *machine) > cpu = M68K_CPU(cpu_create(machine->cpu_type)); > qemu_register_reset(main_cpu_reset, cpu); > > + /* RAM */ > ram = g_malloc(sizeof(*ram)); > memory_region_init_ram(ram, NULL, "m68k_mac.ram", ram_size, &error_abort); > memory_region_add_subregion(get_system_memory(), 0, ram); > > + /* > + * Memory from VIA_BASE to VIA_BASE + 0x40000 is repeated > + * from VIA_BASE + 0x40000 to VIA_BASE + 0x4000000 > + */ Maybe: const size_t via_aliases_count = (0x4000000 / 0x40000) - 1; MemoryRegion *via_alias = g_new(MemoryRegion, via_aliases_count); for (size_t i = 0; i < via_aliases_count; i++) { ... memory_region_add_subregion(get_system_memory(), VIA_BASE + (i + 1) * 0x40000, via_alias[i]); ... } > + for (i = 1; i < 256; i++) { > + MemoryRegion *io = g_malloc(sizeof(*io)); > + char *name = g_strdup_printf("mac_m68k.io[%d]", i); > + > + memory_region_init_alias(io, NULL, name, get_system_memory(), > + VIA_BASE, 0x40000); > + memory_region_add_subregion(get_system_memory(), > + VIA_BASE + i * 0x40000, io); > + g_free(name); > + } I'm trying to get ride of this pattern, so I plan to refactor this later (and will use 256*KiB). Anyway not this patch problem. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > + > /* IRQ Glue */ > > irq = g_new0(GLUEState, 1); >
Le 01/11/2019 à 01:00, Philippe Mathieu-Daudé a écrit : > On 10/31/19 11:03 AM, Laurent Vivier wrote: >> Linux kernel 5.4 will introduce a new memory map for SWIM device. >> (aee6bff1c325 ("m68k: mac: Revisit floppy disc controller base >> addresses")) >> >> Until this release all MMIO are mapped between 0x50f00000 and 0x50f40000, >> but it appears that for real hardware 0x50f00000 is not the base address: >> the MMIO region spans 0x50000000 through 0x60000000, and 0x50040000 >> through >> 0x54000000 is repeated images of 0x50000000 to 0x50040000. >> >> Fixed: 04e7ca8d0f ("hw/m68k: define Macintosh Quadra 800") >> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >> --- >> hw/m68k/q800.c | 33 +++++++++++++++++++++++++-------- >> 1 file changed, 25 insertions(+), 8 deletions(-) >> >> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c >> index 2b4842f8c6..8122e7c612 100644 >> --- a/hw/m68k/q800.c >> +++ b/hw/m68k/q800.c >> @@ -60,14 +60,14 @@ >> #define MACH_MAC 3 >> #define Q800_MAC_CPU_ID 2 >> -#define VIA_BASE 0x50f00000 >> -#define SONIC_PROM_BASE 0x50f08000 >> -#define SONIC_BASE 0x50f0a000 >> -#define SCC_BASE 0x50f0c020 >> -#define ESP_BASE 0x50f10000 >> -#define ESP_PDMA 0x50f10100 >> -#define ASC_BASE 0x50F14000 >> -#define SWIM_BASE 0x50F1E000 >> +#define VIA_BASE 0x50000000 >> +#define SONIC_PROM_BASE 0x50008000 >> +#define SONIC_BASE 0x5000a000 >> +#define SCC_BASE 0x5000c020 >> +#define ESP_BASE 0x50010000 >> +#define ESP_PDMA 0x50010100 >> +#define ASC_BASE 0x50014000 >> +#define SWIM_BASE 0x5001E000 >> #define NUBUS_SUPER_SLOT_BASE 0x60000000 >> #define NUBUS_SLOT_BASE 0xf0000000 >> @@ -135,6 +135,7 @@ static void q800_init(MachineState *machine) >> int32_t initrd_size; >> MemoryRegion *rom; >> MemoryRegion *ram; >> + int i; >> ram_addr_t ram_size = machine->ram_size; >> const char *kernel_filename = machine->kernel_filename; >> const char *initrd_filename = machine->initrd_filename; >> @@ -163,10 +164,26 @@ static void q800_init(MachineState *machine) >> cpu = M68K_CPU(cpu_create(machine->cpu_type)); >> qemu_register_reset(main_cpu_reset, cpu); >> + /* RAM */ >> ram = g_malloc(sizeof(*ram)); >> memory_region_init_ram(ram, NULL, "m68k_mac.ram", ram_size, >> &error_abort); >> memory_region_add_subregion(get_system_memory(), 0, ram); >> + /* >> + * Memory from VIA_BASE to VIA_BASE + 0x40000 is repeated >> + * from VIA_BASE + 0x40000 to VIA_BASE + 0x4000000 >> + */ > > Maybe: > > const size_t via_aliases_count = (0x4000000 / 0x40000) - 1; good idea. > MemoryRegion *via_alias = g_new(MemoryRegion, via_aliases_count); do we really need to store them? We will not reuse them or free them. > for (size_t i = 0; i < via_aliases_count; i++) { > > ... > > memory_region_add_subregion(get_system_memory(), > VIA_BASE + (i + 1) * 0x40000, > via_alias[i]); > ... > } > >> + for (i = 1; i < 256; i++) { >> + MemoryRegion *io = g_malloc(sizeof(*io)); >> + char *name = g_strdup_printf("mac_m68k.io[%d]", i); >> + >> + memory_region_init_alias(io, NULL, name, get_system_memory(), >> + VIA_BASE, 0x40000); >> + memory_region_add_subregion(get_system_memory(), >> + VIA_BASE + i * 0x40000, io); >> + g_free(name); >> + } > > I'm trying to get ride of this pattern, so I plan to refactor this later > (and will use 256*KiB). Anyway not this patch problem. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> + >> /* IRQ Glue */ >> irq = g_new0(GLUEState, 1); >> Thanks, Laurent
Paolo, the RFC was mainly for you: Is this the good way to replicate 256 times a memory chunk containing a bunch of different MMIO spaces? ... > + /* > + * Memory from VIA_BASE to VIA_BASE + 0x40000 is repeated > + * from VIA_BASE + 0x40000 to VIA_BASE + 0x4000000 > + */ > + for (i = 1; i < 256; i++) { > + MemoryRegion *io = g_malloc(sizeof(*io)); > + char *name = g_strdup_printf("mac_m68k.io[%d]", i); > + > + memory_region_init_alias(io, NULL, name, get_system_memory(), > + VIA_BASE, 0x40000); > + memory_region_add_subregion(get_system_memory(), > + VIA_BASE + i * 0x40000, io); > + g_free(name); > + } > + ... Thanks, Laurent
On 11/2/19 7:10 PM, Laurent Vivier wrote: > Paolo, > > the RFC was mainly for you: > > Is this the good way to replicate 256 times a memory chunk containing a > bunch of different MMIO spaces? I asked Richard about this and he said this is OK because this is the FlatView memory tree. It is then optimized in AddressSpace (see memory_region_transaction_commit). What matters in term of performance are the AddressSpaces. > ... >> + /* >> + * Memory from VIA_BASE to VIA_BASE + 0x40000 is repeated >> + * from VIA_BASE + 0x40000 to VIA_BASE + 0x4000000 >> + */ >> + for (i = 1; i < 256; i++) { >> + MemoryRegion *io = g_malloc(sizeof(*io)); >> + char *name = g_strdup_printf("mac_m68k.io[%d]", i); >> + >> + memory_region_init_alias(io, NULL, name, get_system_memory(), >> + VIA_BASE, 0x40000); >> + memory_region_add_subregion(get_system_memory(), >> + VIA_BASE + i * 0x40000, io); >> + g_free(name); >> + } >> + > ... > Thanks, > Laurent >
Le 02/11/2019 à 20:54, Philippe Mathieu-Daudé a écrit : > On 11/2/19 7:10 PM, Laurent Vivier wrote: >> Paolo, >> >> the RFC was mainly for you: >> >> Is this the good way to replicate 256 times a memory chunk containing a >> bunch of different MMIO spaces? > > I asked Richard about this and he said this is OK because this is > the FlatView memory tree. It is then optimized in AddressSpace > (see memory_region_transaction_commit). What matters in term of > performance are the AddressSpaces. Thank you Philippe. Laurent
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c index 2b4842f8c6..8122e7c612 100644 --- a/hw/m68k/q800.c +++ b/hw/m68k/q800.c @@ -60,14 +60,14 @@ #define MACH_MAC 3 #define Q800_MAC_CPU_ID 2 -#define VIA_BASE 0x50f00000 -#define SONIC_PROM_BASE 0x50f08000 -#define SONIC_BASE 0x50f0a000 -#define SCC_BASE 0x50f0c020 -#define ESP_BASE 0x50f10000 -#define ESP_PDMA 0x50f10100 -#define ASC_BASE 0x50F14000 -#define SWIM_BASE 0x50F1E000 +#define VIA_BASE 0x50000000 +#define SONIC_PROM_BASE 0x50008000 +#define SONIC_BASE 0x5000a000 +#define SCC_BASE 0x5000c020 +#define ESP_BASE 0x50010000 +#define ESP_PDMA 0x50010100 +#define ASC_BASE 0x50014000 +#define SWIM_BASE 0x5001E000 #define NUBUS_SUPER_SLOT_BASE 0x60000000 #define NUBUS_SLOT_BASE 0xf0000000 @@ -135,6 +135,7 @@ static void q800_init(MachineState *machine) int32_t initrd_size; MemoryRegion *rom; MemoryRegion *ram; + int i; ram_addr_t ram_size = machine->ram_size; const char *kernel_filename = machine->kernel_filename; const char *initrd_filename = machine->initrd_filename; @@ -163,10 +164,26 @@ static void q800_init(MachineState *machine) cpu = M68K_CPU(cpu_create(machine->cpu_type)); qemu_register_reset(main_cpu_reset, cpu); + /* RAM */ ram = g_malloc(sizeof(*ram)); memory_region_init_ram(ram, NULL, "m68k_mac.ram", ram_size, &error_abort); memory_region_add_subregion(get_system_memory(), 0, ram); + /* + * Memory from VIA_BASE to VIA_BASE + 0x40000 is repeated + * from VIA_BASE + 0x40000 to VIA_BASE + 0x4000000 + */ + for (i = 1; i < 256; i++) { + MemoryRegion *io = g_malloc(sizeof(*io)); + char *name = g_strdup_printf("mac_m68k.io[%d]", i); + + memory_region_init_alias(io, NULL, name, get_system_memory(), + VIA_BASE, 0x40000); + memory_region_add_subregion(get_system_memory(), + VIA_BASE + i * 0x40000, io); + g_free(name); + } + /* IRQ Glue */ irq = g_new0(GLUEState, 1);
Linux kernel 5.4 will introduce a new memory map for SWIM device. (aee6bff1c325 ("m68k: mac: Revisit floppy disc controller base addresses")) Until this release all MMIO are mapped between 0x50f00000 and 0x50f40000, but it appears that for real hardware 0x50f00000 is not the base address: the MMIO region spans 0x50000000 through 0x60000000, and 0x50040000 through 0x54000000 is repeated images of 0x50000000 to 0x50040000. Fixed: 04e7ca8d0f ("hw/m68k: define Macintosh Quadra 800") Signed-off-by: Laurent Vivier <laurent@vivier.eu> --- hw/m68k/q800.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)