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

[Igor Mammedov]

On Thu, 17 Apr 2014 23:31:13 +1000
Peter Crosthwaite <address@hidden> wrote:

> 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
Unfortunatly, making sysbus hotpluggable was banned by Anthony,
Here is probably the first attempt to patch sysbus
http://lists.gnu.org/archive/html/qemu-devel/2010-08/msg00968.html
that was never accepted upstream.

> 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.
policy and address spaces should be defined and initialized somewhere,
in this example address space && hotplug policy && hotplug handler is
a part of machine instance/class. But it's not the only way,
If there is a memory controller device then it probably should be
the hotplug-handler. While at machine level hotplug policy specifies
that for DIMM device hotplug-handler will be a specific memory
controller device.

> 
> 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.
Yep, dimm device doesn't map well as generic IP memory block.
Above mentioned devices could be or maybe should be represented as
different types so that hotplug policy in machine could map type to
a respective hotplug handler. That still lacks address-ability
provided by bus interface.

https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04184.html
was proposing to use links for handler discovery, but links<> were
dropped in current impl. for simplicity and lack of users.
It would be nice if you could throw in there any new ideas,
especially regarding where to hotplug bus-less device
(as the most lacking area).

mst already has a use case for boards with re-configurable logic,
it's guest switchable ACPI PCI HOTPLUG <-> SHPC for PCI bridge.
Currently we hack into PCI bridge from piix4 to change hotplug
handler from default SHPC to ACPI PCIHP handler. That ugly hack
could be avoided if hotplug handler selection routed through
dynamic machine level table, then guest could program PCI-Host
to use another hotplug method.

> 
> > 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);
> >>
> >
> >
> >
>