On Fri, 30 May 2014 20:55:50 +1000 Peter Crosthwaite <peter.crosthwa...@xilinx.com> wrote:
> On Fri, May 30, 2014 at 8:31 PM, Igor Mammedov <imamm...@redhat.com> wrote: > > On Fri, 30 May 2014 19:48:13 +1000 > > Peter Crosthwaite <peter.crosthwa...@xilinx.com> wrote: > > > >> On Fri, May 30, 2014 at 7:01 PM, Igor Mammedov <imamm...@redhat.com> wrote: > >> > On Fri, 30 May 2014 00:21:27 +1000 > >> > Peter Crosthwaite <peter.crosthwa...@xilinx.com> wrote: > >> > > >> >> On Tue, May 27, 2014 at 11:01 PM, Igor Mammedov <imamm...@redhat.com> > >> >> wrote: > >> >> > From: Vasilis Liaskovitis <vasilis.liaskovi...@profitbricks.com> > >> >> > > >> >> > Each hotplug-able memory slot is a DimmDevice. > >> >> > A hot-add operation for a DIMM: > >> >> > - creates a new DimmDevice and makes hotplug controller to map it into > >> >> > guest address space > >> >> > > >> >> > Hotplug operations are done through normal device_add commands. > >> >> > For migration case, all hotplugged DIMMs on source should be specified > >> >> > on target's command line using '-device' option with properties set to > >> >> > the same values as on source. > >> >> > > >> >> > To simplify review, patch introduces only DimmDevice QOM skeleton that > >> >> > will be extended by following patches to implement actual memory > >> >> > hotplug > >> >> > and related functions. > >> >> > > >> >> > Signed-off-by: Vasilis Liaskovitis > >> >> > <vasilis.liaskovi...@profitbricks.com> > >> >> > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > >> >> > --- > >> >> > v4: > >> >> > drop DimmBus in favor of bus-less device hotplug > >> >> > rename property/field 'start' to 'addr' > >> >> > use defines for property names > >> >> > v3: > >> >> > pc: compile memhotplug on i386 target too > >> >> > v2: > >> >> > fix typo s/DimmBus/DimmDevice/ in doc comment > >> >> > s/klass/oc/;s/*/parent_obj/;a/gtk-doc markup/ > >> >> > --- > >> >> > default-configs/i386-softmmu.mak | 1 + > >> >> > default-configs/x86_64-softmmu.mak | 1 + > >> >> > hw/Makefile.objs | 1 + > >> >> > hw/mem/Makefile.objs | 1 + > >> >> > hw/mem/dimm.c | 103 > >> >> > ++++++++++++++++++++++++++++++++++++ > >> >> > include/hw/mem/dimm.h | 73 +++++++++++++++++++++++++ > >> >> > 6 files changed, 180 insertions(+), 0 deletions(-) > >> >> > create mode 100644 hw/mem/Makefile.objs > >> >> > create mode 100644 hw/mem/dimm.c > >> >> > create mode 100644 include/hw/mem/dimm.h > >> >> > > >> >> > diff --git a/default-configs/i386-softmmu.mak > >> >> > b/default-configs/i386-softmmu.mak > >> >> > index 37ef90f..8e08841 100644 > >> >> > --- a/default-configs/i386-softmmu.mak > >> >> > +++ b/default-configs/i386-softmmu.mak > >> >> > @@ -44,3 +44,4 @@ CONFIG_APIC=y > >> >> > CONFIG_IOAPIC=y > >> >> > CONFIG_ICC_BUS=y > >> >> > CONFIG_PVPANIC=y > >> >> > +CONFIG_MEM_HOTPLUG=y > >> >> > diff --git a/default-configs/x86_64-softmmu.mak > >> >> > b/default-configs/x86_64-softmmu.mak > >> >> > index 31bddce..66557ac 100644 > >> >> > --- a/default-configs/x86_64-softmmu.mak > >> >> > +++ b/default-configs/x86_64-softmmu.mak > >> >> > @@ -44,3 +44,4 @@ CONFIG_APIC=y > >> >> > CONFIG_IOAPIC=y > >> >> > CONFIG_ICC_BUS=y > >> >> > CONFIG_PVPANIC=y > >> >> > +CONFIG_MEM_HOTPLUG=y > >> >> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs > >> >> > index d178b65..52a1464 100644 > >> >> > --- a/hw/Makefile.objs > >> >> > +++ b/hw/Makefile.objs > >> >> > @@ -29,6 +29,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += usb/ > >> >> > devices-dirs-$(CONFIG_VIRTIO) += virtio/ > >> >> > devices-dirs-$(CONFIG_SOFTMMU) += watchdog/ > >> >> > devices-dirs-$(CONFIG_SOFTMMU) += xen/ > >> >> > +devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/ > >> >> > devices-dirs-y += core/ > >> >> > common-obj-y += $(devices-dirs-y) > >> >> > obj-y += $(devices-dirs-y) > >> >> > diff --git a/hw/mem/Makefile.objs b/hw/mem/Makefile.objs > >> >> > new file mode 100644 > >> >> > index 0000000..7563ef5 > >> >> > --- /dev/null > >> >> > +++ b/hw/mem/Makefile.objs > >> >> > @@ -0,0 +1 @@ > >> >> > +common-obj-$(CONFIG_MEM_HOTPLUG) += dimm.o > >> >> > diff --git a/hw/mem/dimm.c b/hw/mem/dimm.c > >> >> > new file mode 100644 > >> >> > index 0000000..bb81679 > >> >> > --- /dev/null > >> >> > +++ b/hw/mem/dimm.c > >> >> > @@ -0,0 +1,103 @@ > >> >> > +/* > >> >> > + * Dimm device for Memory Hotplug > >> >> > + * > >> >> > + * Copyright ProfitBricks GmbH 2012 > >> >> > + * Copyright (C) 2014 Red Hat Inc > >> >> > + * > >> >> > + * This library is free software; you can redistribute it and/or > >> >> > + * modify it under the terms of the GNU Lesser General Public > >> >> > + * License as published by the Free Software Foundation; either > >> >> > + * version 2 of the License, or (at your option) any later version. > >> >> > + * > >> >> > + * This library is distributed in the hope that it will be useful, > >> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > >> >> > + * Lesser General Public License for more details. > >> >> > + * > >> >> > + * You should have received a copy of the GNU Lesser General Public > >> >> > + * License along with this library; if not, see > >> >> > <http://www.gnu.org/licenses/> > >> >> > + */ > >> >> > + > >> >> > +#include "hw/mem/dimm.h" > >> >> > +#include "qemu/config-file.h" > >> >> > +#include "qapi/visitor.h" > >> >> > + > >> >> > +static Property dimm_properties[] = { > >> >> > + DEFINE_PROP_UINT64(DIMM_ADDR_PROP, DimmDevice, addr, 0), > >> >> > >> >> One of the long-standing rules of sysbus device design, is devices > >> >> should not have awareness of their memory mappings. Especially in > >> >> cases where that mapping is handled by buses and controllers (which is > >> >> the case in DIMM? - does the actual DIMM card have any awareness of > >> >> its own mapping?). > >> > Actual DIMM probably is not aware of address, and it doesn't use it by > >> > itself for any purpose. > >> > However it's important from interface POV, when user needs to add DIMM > >> > device > >> > at a specific address using CLI, -device_add dimm,addr=foo serves as a > >> > convenient way to carry that information to component that will do actual > >> > mapping and other components that will need it. > >> > In PC it's machine (servers as controller) and ACPI device, which > >> > implements > >> > APCI part of memory hotplug interface. > >> > > >> > Real HW doesn't need it since, it's fixed amount of slots and limited > >> > size of > >> > DIMMs supported by chipset, so it just hard-codes possible addresses > >> > for each slot. For VM it would severely limit flexibility though, > >> > forcing user to decide at QEMU startup time DIMM sizes per slot, > >> > he'd like to hot-add at runtime. So ability to hot-add arbitrary sized > >> > DIMMs, > >> > would require at migration time an interface that allows to say at what > >> > address dimm was mapped. Using dimm.addr for it is logical choice from > >> > user > >> > POV and provides simple and clear interface. > >> > > >> > > >> >> > >> >> That said I'm generally against that policy as sometimes devices > >> >> really do know their absolute address in real HW implementation. So > >> >> despite being generally against an old consensus I think this might be > >> >> ok. > >> >> > >> >> > + DEFINE_PROP_UINT32(DIMM_NODE_PROP, DimmDevice, node, 0), > >> >> > + DEFINE_PROP_INT32(DIMM_SLOT_PROP, DimmDevice, slot, > >> >> > DIMM_UNASSIGNED_SLOT), > >> >> > >> >> I think this slot property may however reduce the re-usability of DIMM > >> >> beyond PC. Is the concept of enumerated DIMM slots a DIMM level > >> >> feature or is it more PC specific? > >> > it's more a shortcut of place where to add device, similarly as we use > >> > 'bus' > >> > for specifying where to add device. > >> > >> So "bus=" args are parsed by the hotplug handling code in that case. > >> Perhaps this can be done the same - addr, slot and friends are parsed > >> by your machine level busless hotpug handling and passes info to > >> PC/ACPI and mapping code where they are actually used. Then you can > >> remove the PCisms from DIMM completely. > > Do you mean to extend current device_add hotplug mechanism, so that > > hotplug controller, would consume its own specific options that define > > how/where device should be added from what was supplied by -device > > dimm,foo... > > ??? > > > > Yes I think that general problem is worth solving. But not needed yet > in this series. I think this is better solved by simply acknowleding > that this API is PC specific and not claiming it to be generic DIMM (I > replied with fuller explanation before I saw this mail). > > > PS: > > Is it ok to do on top of this series? > > I guess so, but is there a commiter out there wanting to take this as > is yet? Theres a few trivials as well as the mr->parent simplificaiton > (plus whatever others come up with). > > > /including renaming DimmDevice -> DIMMDevice/ > > > > Are the conflicts bad on the rebase? > > I notice also you have some patches that are already merged which is > another good reason for a respin. It's cross patches rename but it's not a big problem. I'll rename to suggested TYPE_PC_DIMM, fix other issues you pointed out and then respin. Thanks for suggestions! > > Regards, > Peter > > > > >> > >> Regards, > >> Peter > >> > >> > Slot was a natural choice since DIMMs are > >> > plugged in them. > >> > It's optional from DIMM POV though, so if user doesn't care he can > >> > ignore it. > >> > > >> > It also simplifies internal handling of hot-plugging a device on PC as > >> > it's > >> > serves as simple ID in interface between QEMU and ACPI tables. > >> > > >> > > >> >> > >> >> > + DEFINE_PROP_END_OF_LIST(), > >> >> > +}; > >> >> > + > >> >> > +static void dimm_get_size(Object *obj, Visitor *v, void *opaque, > >> >> > + const char *name, Error **errp) > >> >> > +{ > >> >> > + int64_t value; > >> >> > + MemoryRegion *mr; > >> >> > + DimmDevice *dimm = DIMM(obj); > >> >> > + > >> >> > + mr = host_memory_backend_get_memory(dimm->hostmem, errp); > >> >> > + value = memory_region_size(mr); > >> >> > + > >> >> > + visit_type_int(v, &value, name, errp); > >> >> > +} > >> >> > + > >> >> > +static void dimm_initfn(Object *obj) > >> >> > +{ > >> >> > + DimmDevice *dimm = DIMM(obj); > >> >> > + > >> >> > + object_property_add(obj, DIMM_SIZE_PROP, "int", dimm_get_size, > >> >> > + NULL, NULL, NULL, &error_abort); > >> >> > + object_property_add_link(obj, DIMM_MEMDEV_PROP, > >> >> > TYPE_MEMORY_BACKEND, > >> >> > + (Object **)&dimm->hostmem, > >> >> > + qdev_prop_allow_set_link_before_realize, > >> >> > + OBJ_PROP_LINK_UNREF_ON_RELEASE, > >> >> > + &error_abort); > >> >> > +} > >> >> > + > >> >> > +static void dimm_realize(DeviceState *dev, Error **errp) > >> >> > +{ > >> >> > + DimmDevice *dimm = DIMM(dev); > >> >> > + > >> >> > + if (!dimm->hostmem) { > >> >> > + error_setg(errp, "'" DIMM_MEMDEV_PROP "' property is not > >> >> > set"); > >> >> > + return; > >> >> > + } > >> >> > + > >> >> > + if (!dev->id) { > >> >> > + error_setg(errp, "'id' property is not set"); > >> >> > >> >> Can't implement an "anonymous" (or whatever) default? > >> > This check can be dropped to allow anonymous DIMMs. > >> > > >> > out of scope: > >> > Perhaps we should fix qdev_device_add(), to assign 'id' if it's missing > >> > using the name it auto-generates for child. > >> > > >> >> > >> >> > + return; > >> >> > + } > >> >> > +} > >> >> > + > >> >> > +static MemoryRegion *dimm_get_memory_region(DimmDevice *dimm) > >> >> > +{ > >> >> > + return host_memory_backend_get_memory(dimm->hostmem, > >> >> > &error_abort); > >> >> > +} > >> >> > + > >> >> > +static void dimm_class_init(ObjectClass *oc, void *data) > >> >> > +{ > >> >> > + DeviceClass *dc = DEVICE_CLASS(oc); > >> >> > + DimmDeviceClass *ddc = DIMM_CLASS(oc); > >> >> > + > >> >> > + dc->realize = dimm_realize; > >> >> > + dc->props = dimm_properties; > >> >> > + > >> >> > + ddc->get_memory_region = dimm_get_memory_region; > >> >> > +} > >> >> > + > >> >> > +static TypeInfo dimm_info = { > >> >> > + .name = TYPE_DIMM, > >> >> > + .parent = TYPE_DEVICE, > >> >> > + .instance_size = sizeof(DimmDevice), > >> >> > + .instance_init = dimm_initfn, > >> >> > + .class_init = dimm_class_init, > >> >> > + .class_size = sizeof(DimmDeviceClass), > >> >> > +}; > >> >> > + > >> >> > +static void dimm_register_types(void) > >> >> > +{ > >> >> > + type_register_static(&dimm_info); > >> >> > +} > >> >> > + > >> >> > +type_init(dimm_register_types) > >> >> > diff --git a/include/hw/mem/dimm.h b/include/hw/mem/dimm.h > >> >> > new file mode 100644 > >> >> > index 0000000..8839fb9 > >> >> > --- /dev/null > >> >> > +++ b/include/hw/mem/dimm.h > >> >> > @@ -0,0 +1,73 @@ > >> >> > +/* > >> >> > + * DIMM device > >> >> > + * > >> >> > + * Copyright ProfitBricks GmbH 2012 > >> >> > + * Copyright (C) 2013 Red Hat Inc > >> >> > + * > >> >> > + * Authors: > >> >> > + * Vasilis Liaskovitis <vasilis.liaskovi...@profitbricks.com> > >> >> > + * Igor Mammedov <imamm...@redhat.com> > >> >> > + * > >> >> > + * This work is licensed under the terms of the GNU GPL, version 2 > >> >> > or later. > >> >> > + * See the COPYING file in the top-level directory. > >> >> > + * > >> >> > + */ > >> >> > + > >> >> > +#ifndef QEMU_DIMM_H > >> >> > +#define QEMU_DIMM_H > >> >> > + > >> >> > +#include "exec/memory.h" > >> >> > +#include "sysemu/hostmem.h" > >> >> > +#include "hw/qdev.h" > >> >> > + > >> >> > +#define DEFAULT_DIMMSIZE (1024*1024*1024) > >> >> > + > >> >> > +#define TYPE_DIMM "dimm" > >> >> > +#define DIMM(obj) \ > >> >> > + OBJECT_CHECK(DimmDevice, (obj), TYPE_DIMM) > >> >> > +#define DIMM_CLASS(oc) \ > >> >> > + OBJECT_CLASS_CHECK(DimmDeviceClass, (oc), TYPE_DIMM) > >> >> > +#define DIMM_GET_CLASS(obj) \ > >> >> > + OBJECT_GET_CLASS(DimmDeviceClass, (obj), TYPE_DIMM) > >> >> > + > >> >> > +#define DIMM_ADDR_PROP "addr" > >> >> > +#define DIMM_SLOT_PROP "slot" > >> >> > +#define DIMM_NODE_PROP "node" > >> >> > +#define DIMM_SIZE_PROP "size" > >> >> > +#define DIMM_MEMDEV_PROP "memdev" > >> >> > + > >> >> > +#define DIMM_UNASSIGNED_SLOT -1 > >> >> > + > >> >> > +/** > >> >> > + * DimmDevice: > >> >> > + * @addr: starting guest physical address, where @DimmDevice is > >> >> > mapped. > >> >> > + * Default value: 0, means that address is auto-allocated. > >> >> > + * @node: numa node to which @DimmDevice is attached. > >> >> > + * @slot: slot number into which @DimmDevice is plugged in. > >> >> > + * Default value: -1, means that slot is auto-allocated. > >> >> > + * @hostmem: host memory backend providing memory for @DimmDevice > >> >> > + */ > >> >> > +typedef struct DimmDevice { > >> >> > >> >> DIMMDevice > >> > ok > >> > > >> >> > >> >> > + /* private */ > >> >> > + DeviceState parent_obj; > >> >> > + > >> >> > + /* public */ > >> >> > + ram_addr_t addr; > >> >> > + uint32_t node; > >> >> > + int32_t slot; > >> >> > + HostMemoryBackend *hostmem; > >> >> > +} DimmDevice; > >> >> > + > >> >> > +/** > >> >> > + * DimmDeviceClass: > >> >> > + * @get_memory_region: returns #MemoryRegion associated with @dimm > >> >> > + */ > >> >> > +typedef struct DimmDeviceClass { > >> >> > >> >> DIMMDeviceClass > >> > ok > >> > > >> >> > >> >> > + /* private */ > >> >> > + DeviceClass parent_class; > >> >> > + > >> >> > + /* public */ > >> >> > + MemoryRegion *(*get_memory_region)(DimmDevice *dimm); > >> >> > >> >> This would simply become a link getter if we had QOMified MemoryRegions > >> >> :) > >> >> > >> >> Regards, > >> >> Peter > >> >> > >> >> > +} DimmDeviceClass; > >> >> > + > >> >> > +#endif > >> >> > -- > >> >> > 1.7.1 > >> >> > > >> >> > > >> >> > >> > > >> > > >> > -- > >> > Regards, > >> > Igor > >> > > > > > > > -- > > Regards, > > Igor > > -- Regards, Igor
