Re: [Qemu-devel] [PATCH v1 3/3] arm: xilinx_zynq: Use Sysbus Memory device for RAMs

[Peter Crosthwaite]

On Thu, Apr 17, 2014 at 10:50 PM, Igor Mammedov <address@hidden> wrote:
> On Mon, 14 Apr 2014 19:22:11 -0700
> Peter Crosthwaite <address@hidden> wrote:
>
>> For consistency with other devices and completeness of system device
>> tree.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>
>>  hw/arm/xilinx_zynq.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> index 9ee21e7..7a0c951 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -110,9 +110,6 @@ static void zynq_init(QEMUMachineInitArgs *args)
>>      const char *initrd_filename = args->initrd_filename;
>>      ObjectClass *cpu_oc;
>>      ARMCPU *cpu;
>> -    MemoryRegion *address_space_mem = get_system_memory();
>> -    MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
>> -    MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
>>      DeviceState *dev;
>>      SysBusDevice *busdev;
>>      qemu_irq pic[64];
> add following hotplug-handler to board:
>

And we have to do this for every board :S. Cant we solve this
generally for a bus type. I.e. we patch sysbus to be able to holplug,
and you plug to it, not to the machine. And we are making the
transition away from global state WRT to address spaces. Having boards
globally define the hotplug policy (and hotplug address space) seems
like a big step backwards after that.

Plugging to the machine is ill defined especially when you consider
the wide range of machine models from embedded to PC. In your case,
machine level hotplugging means slotting a RAM-stick PCB into a
physical slot on a motherboard. In our case this would be synthesising
soft IP in FPGA reconfigurable logic. We really really need some
context to what and how we are hotplugging to the machine. Especially
if I throw the curveball, that we have boards with both reconfigurable
logic and DIMM slots (and whatever other interfaces we havent thought
through yet). What does it mean to -device "memory"? Does it mean
connect a ram stick to a DIMM slot on a bus behind a memory cntrlr? or
does it mean add an on-chip RAM straight onto the processors bus? The
context of what bus you are hotplugging to in a single machine is
needed to resolve ambiguity.

> zynq_machine_plug_handler_callback(zynq_machine, dev) {
>     if (type(dev) == dimm) {
>         MemoryRegion *mr = DIMM_GET_CLASS(dev)->get_memory_region(dev);
>         // do extra board specific placement checks
>
>         // do actual mapping
>         memory_region_add_subregion(zynq_machine->ram_address_space,
>                                     object_property_get_int(dev, "start", 
> NULL),
>                                     mr);
>         vmstate_register_ram_global(rm)
>     }
> }
>
>> @@ -149,14 +146,18 @@ static void zynq_init(QEMUMachineInitArgs *args)
>>      }
>>
>>      /* DDR remapped to address zero.  */
>> -    memory_region_init_ram(ext_ram, NULL, "zynq.ext_ram", ram_size);
>> -    vmstate_register_ram_global(ext_ram);
>> -    memory_region_add_subregion(address_space_mem, 0, ext_ram);
>> +    dev = qdev_create(NULL, "sysbus-memory");
>> +    qdev_prop_set_string(dev, "device-id", "zynq.ext_ram");
>> +    qdev_prop_set_uint64(dev, "size", ram_size);
>> +    qdev_init_nofail(dev);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0);
>
> could be replaced by hardcoded as above:
>
>        object_add("ext_ram", QDict("memory-ram,size=foo"));
>        dev = DEVICE(object_new("dimm"));

Even with memory hotplug we might not be using DIMMs at all. If we run
with your solution as a generalisation, "DIMM" needs to be replaced as
a name.

We also need to convert a large library of existing sysbus devices the
DIMM abstraction. We want to be able to at least hotplug all our soft
ip (xilinx_dma, xilinx_enet, xilinx_timer etc ...).

But to be honest, it would be nice to hotplug everything.

Regards,
Peter

>        qdev_prop_set_uint64(dev, "start", 0);
>        object_property_set_bool(OBJECT(dev), true, "realized", &error_abort);
>
> or throw away above section and use data driven way:
>
>        -object memory-ram,id=ext_ram,size=ram_size
>        -device dimm,id=zynq.ext_ram,memdev=ext_ram
>
> with it one gets completely functional RAM device on board after
> its realize has been completed.
>
>>
>>      /* 256K of on-chip memory */
>> -    memory_region_init_ram(ocm_ram, NULL, "zynq.ocm_ram", 256 << 10);
>> -    vmstate_register_ram_global(ocm_ram);
>> -    memory_region_add_subregion(address_space_mem, 0xFFFC0000, ocm_ram);
>> +    dev = qdev_create(NULL, "sysbus-memory");
>> +    qdev_prop_set_string(dev, "device-id", "zynq.ocm_ram");
>> +    qdev_prop_set_uint64(dev, "size", 256 << 10);
>> +    qdev_init_nofail(dev);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xFFFC0000);
>>
>>      DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
>>
>
>
>