| Message ID | 20191020225650.3671-9-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: Let the machine be the owner of the system memory | expand |
On 10/21/19 12:56 AM, Philippe Mathieu-Daudé wrote: > The SDRAM is incorrectly created in the Tyohoon northbridge ASIC. > Move its creation in the board code, this will later allow the > board to have the QOM ownership of the RAM. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > checkpatch complains: > > ERROR: spaces required around that '*' (ctx:WxV) > #10: FILE: hw/alpha/alpha_sys.h:13: > +PCIBus *typhoon_init(ISABus **, qemu_irq *, AlphaCPU *[4], > ^ > --- > hw/alpha/alpha_sys.h | 2 +- > hw/alpha/dp264.c | 11 ++++++++++- > hw/alpha/typhoon.c | 9 +-------- > 3 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h > index 4e127a6de8..17f97e1bfe 100644 > --- a/hw/alpha/alpha_sys.h > +++ b/hw/alpha/alpha_sys.h > @@ -10,7 +10,7 @@ > #include "hw/i386/pc.h" > > > -PCIBus *typhoon_init(ram_addr_t, ISABus **, qemu_irq *, AlphaCPU *[4], > +PCIBus *typhoon_init(ISABus **, qemu_irq *, AlphaCPU *[4], > pci_map_irq_fn); > > /* alpha_pci.c. */ > diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c > index 51feee8558..ddc249261c 100644 > --- a/hw/alpha/dp264.c > +++ b/hw/alpha/dp264.c > @@ -62,6 +62,7 @@ static void clipper_init(MachineState *machine) > uint64_t palcode_entry, palcode_low, palcode_high; > uint64_t kernel_entry, kernel_low, kernel_high; > unsigned int smp_cpus = machine->smp.cpus; > + MemoryRegion ram_region; This is incorrect, this variable should be in .heap, not stack. > > /* Create up to 4 cpus. */ > memset(cpus, 0, sizeof(cpus)); > @@ -73,8 +74,16 @@ static void clipper_init(MachineState *machine) > cpus[0]->env.trap_arg1 = 0; > cpus[0]->env.trap_arg2 = smp_cpus; > > + /* > + * Main memory region, 0x00.0000.0000. Real hardware supports 32GB, > + * but the address space hole reserved at this point is 8TB. > + */ > + memory_region_allocate_system_memory(&ram_region, NULL, "ram", > + ram_size); > + memory_region_add_subregion(get_system_memory(), 0, &ram_region); > + > /* Init the chipset. */ > - pci_bus = typhoon_init(ram_size, &isa_bus, &rtc_irq, cpus, > + pci_bus = typhoon_init(&isa_bus, &rtc_irq, cpus, > clipper_pci_map_irq); > > /* Since we have an SRM-compatible PALcode, use the SRM epoch. */ > diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c > index 8489ec335c..12589ef331 100644 > --- a/hw/alpha/typhoon.c > +++ b/hw/alpha/typhoon.c > @@ -58,7 +58,6 @@ typedef struct TyphoonState { > TyphoonCchip cchip; > TyphoonPchip pchip; > MemoryRegion dchip_region; > - MemoryRegion ram_region; > } TyphoonState; > > /* Called when one of DRIR or DIM changes. */ > @@ -817,7 +816,7 @@ static void typhoon_alarm_timer(void *opaque) > cpu_interrupt(CPU(s->cchip.cpu[cpu]), CPU_INTERRUPT_TIMER); > } > > -PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, > +PCIBus *typhoon_init(ISABus **isa_bus, > qemu_irq *p_rtc_irq, > AlphaCPU *cpus[4], pci_map_irq_fn sys_map_irq) > { > @@ -849,12 +848,6 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, > > *p_rtc_irq = qemu_allocate_irq(typhoon_set_timer_irq, s, 0); > > - /* Main memory region, 0x00.0000.0000. Real hardware supports 32GB, > - but the address space hole reserved at this point is 8TB. */ > - memory_region_allocate_system_memory(&s->ram_region, NULL, "ram", > - ram_size); > - memory_region_add_subregion(addr_space, 0, &s->ram_region); > - > /* TIGbus, 0x801.0000.0000, 1GB. */ > /* ??? The TIGbus is used for delivering interrupts, and access to > the flash ROM. I'm not sure that we need to implement it at all. */ >
On 10/20/19 3:56 PM, Philippe Mathieu-Daudé wrote: > @@ -73,8 +74,16 @@ static void clipper_init(MachineState *machine) > cpus[0]->env.trap_arg1 = 0; > cpus[0]->env.trap_arg2 = smp_cpus; > > + /* > + * Main memory region, 0x00.0000.0000. Real hardware supports 32GB, > + * but the address space hole reserved at this point is 8TB. > + */ > + memory_region_allocate_system_memory(&ram_region, NULL, "ram", > + ram_size); > + memory_region_add_subregion(get_system_memory(), 0, &ram_region); The thing that I don't like about this is that ... > @@ -849,12 +848,6 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, > > *p_rtc_irq = qemu_allocate_irq(typhoon_set_timer_irq, s, 0); > > - /* Main memory region, 0x00.0000.0000. Real hardware supports 32GB, > - but the address space hole reserved at this point is 8TB. */ > - memory_region_allocate_system_memory(&s->ram_region, NULL, "ram", > - ram_size); > - memory_region_add_subregion(addr_space, 0, &s->ram_region); > - > /* TIGbus, 0x801.0000.0000, 1GB. */ > /* ??? The TIGbus is used for delivering interrupts, and access to > the flash ROM. I'm not sure that we need to implement it at all. */ ... previously, it was clear that Typhoon owns the entire address space layout. With this moved to dp264.c, the "address space hole" comment is out of context and makes no sense. Would it be too weird to pass in a MemoryRegion* to typhoon_init and leave the memory_region_add_subregion() call where it is? r~
On 10/21/19 7:27 PM, Richard Henderson wrote: > On 10/20/19 3:56 PM, Philippe Mathieu-Daudé wrote: >> @@ -73,8 +74,16 @@ static void clipper_init(MachineState *machine) >> cpus[0]->env.trap_arg1 = 0; >> cpus[0]->env.trap_arg2 = smp_cpus; >> >> + /* >> + * Main memory region, 0x00.0000.0000. Real hardware supports 32GB, >> + * but the address space hole reserved at this point is 8TB. >> + */ >> + memory_region_allocate_system_memory(&ram_region, NULL, "ram", >> + ram_size); >> + memory_region_add_subregion(get_system_memory(), 0, &ram_region); > > The thing that I don't like about this is that ... > >> @@ -849,12 +848,6 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, >> >> *p_rtc_irq = qemu_allocate_irq(typhoon_set_timer_irq, s, 0); >> >> - /* Main memory region, 0x00.0000.0000. Real hardware supports 32GB, >> - but the address space hole reserved at this point is 8TB. */ >> - memory_region_allocate_system_memory(&s->ram_region, NULL, "ram", >> - ram_size); >> - memory_region_add_subregion(addr_space, 0, &s->ram_region); >> - >> /* TIGbus, 0x801.0000.0000, 1GB. */ >> /* ??? The TIGbus is used for delivering interrupts, and access to >> the flash ROM. I'm not sure that we need to implement it at all. */ > > ... previously, it was clear that Typhoon owns the entire address space layout. > With this moved to dp264.c, the "address space hole" comment is out of context > and makes no sense. > > Would it be too weird to pass in a MemoryRegion* to typhoon_init and leave the > memory_region_add_subregion() call where it is? Yes, this will also better match the hardware. I have a WiP series that first QOM'ify the CY82C693UB southbridge then the Typhoon northbridge and pass the DRAM MemoryRegion* as a link, so in the non-QOM form your suggestion is the same. I'll do your change.
diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h index 4e127a6de8..17f97e1bfe 100644 --- a/hw/alpha/alpha_sys.h +++ b/hw/alpha/alpha_sys.h @@ -10,7 +10,7 @@ #include "hw/i386/pc.h" -PCIBus *typhoon_init(ram_addr_t, ISABus **, qemu_irq *, AlphaCPU *[4], +PCIBus *typhoon_init(ISABus **, qemu_irq *, AlphaCPU *[4], pci_map_irq_fn); /* alpha_pci.c. */ diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c index 51feee8558..ddc249261c 100644 --- a/hw/alpha/dp264.c +++ b/hw/alpha/dp264.c @@ -62,6 +62,7 @@ static void clipper_init(MachineState *machine) uint64_t palcode_entry, palcode_low, palcode_high; uint64_t kernel_entry, kernel_low, kernel_high; unsigned int smp_cpus = machine->smp.cpus; + MemoryRegion ram_region; /* Create up to 4 cpus. */ memset(cpus, 0, sizeof(cpus)); @@ -73,8 +74,16 @@ static void clipper_init(MachineState *machine) cpus[0]->env.trap_arg1 = 0; cpus[0]->env.trap_arg2 = smp_cpus; + /* + * Main memory region, 0x00.0000.0000. Real hardware supports 32GB, + * but the address space hole reserved at this point is 8TB. + */ + memory_region_allocate_system_memory(&ram_region, NULL, "ram", + ram_size); + memory_region_add_subregion(get_system_memory(), 0, &ram_region); + /* Init the chipset. */ - pci_bus = typhoon_init(ram_size, &isa_bus, &rtc_irq, cpus, + pci_bus = typhoon_init(&isa_bus, &rtc_irq, cpus, clipper_pci_map_irq); /* Since we have an SRM-compatible PALcode, use the SRM epoch. */ diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c index 8489ec335c..12589ef331 100644 --- a/hw/alpha/typhoon.c +++ b/hw/alpha/typhoon.c @@ -58,7 +58,6 @@ typedef struct TyphoonState { TyphoonCchip cchip; TyphoonPchip pchip; MemoryRegion dchip_region; - MemoryRegion ram_region; } TyphoonState; /* Called when one of DRIR or DIM changes. */ @@ -817,7 +816,7 @@ static void typhoon_alarm_timer(void *opaque) cpu_interrupt(CPU(s->cchip.cpu[cpu]), CPU_INTERRUPT_TIMER); } -PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, +PCIBus *typhoon_init(ISABus **isa_bus, qemu_irq *p_rtc_irq, AlphaCPU *cpus[4], pci_map_irq_fn sys_map_irq) { @@ -849,12 +848,6 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, *p_rtc_irq = qemu_allocate_irq(typhoon_set_timer_irq, s, 0); - /* Main memory region, 0x00.0000.0000. Real hardware supports 32GB, - but the address space hole reserved at this point is 8TB. */ - memory_region_allocate_system_memory(&s->ram_region, NULL, "ram", - ram_size); - memory_region_add_subregion(addr_space, 0, &s->ram_region); - /* TIGbus, 0x801.0000.0000, 1GB. */ /* ??? The TIGbus is used for delivering interrupts, and access to the flash ROM. I'm not sure that we need to implement it at all. */
The SDRAM is incorrectly created in the Tyohoon northbridge ASIC. Move its creation in the board code, this will later allow the board to have the QOM ownership of the RAM. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- checkpatch complains: ERROR: spaces required around that '*' (ctx:WxV) #10: FILE: hw/alpha/alpha_sys.h:13: +PCIBus *typhoon_init(ISABus **, qemu_irq *, AlphaCPU *[4], ^ --- hw/alpha/alpha_sys.h | 2 +- hw/alpha/dp264.c | 11 ++++++++++- hw/alpha/typhoon.c | 9 +-------- 3 files changed, 12 insertions(+), 10 deletions(-)