Re: [PATCH 11/21] hw/alpha: Let the machine be the owner of the system memory

[Philippe Mathieu-Daudé]

On 10/21/19 7:19 PM, Richard Henderson wrote:
> On 10/21/19 10:14 AM, Richard Henderson wrote:
> > On 10/20/19 3:56 PM, Philippe Mathieu-Daudé wrote:
> > > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > > ---
> > >   hw/alpha/dp264.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> > > index ddc249261c..5696b1e79b 100644
> > > --- a/hw/alpha/dp264.c
> > > +++ b/hw/alpha/dp264.c
> > > @@ -78,7 +78,7 @@ static void clipper_init(MachineState *machine)
> > >        * 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",
> > > +    memory_region_allocate_system_memory(&ram_region, machine, "ram",
> > >                                            ram_size);
> > >       memory_region_add_subregion(get_system_memory(), 0, &ram_region);
> > >   
> >
> > I take it this squishes into the squish of patches 7+9?
>
> Ah, no, it was the TyphoonState that had owned the ram prior to patch 7.

Indeed, I'll squash them and document in the commit description that I 
dropped Typhoon ownership. I'm not sure what's best here, keep it 
unassigned until patch #11 (this), or reassign to OBJECT(machine).

New squashed patch is:

-- 8< --
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..a93a476531 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 = g_new(MemoryRegion, 1);

     /* Create up to 4 cpus.  */
     memset(cpus, 0, sizeof(cpus));
@@ -73,8 +74,15 @@ 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, OBJECT(machine), "ram", 
ram_size);
+    memory_region_add_subregion(get_system_memory(), 0, ram);
+
     /* 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 179e1f7658..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, OBJECT(s), "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.  */
---