Re: [Qemu-devel] [PATCH v3 10/34] dimm: implement dimm device abstraction

[Igor Mammedov]

On Fri, 30 May 2014 20:55:50 +1000
Peter Crosthwaite <address@hidden> wrote:

> On Fri, May 30, 2014 at 8:31 PM, Igor Mammedov <address@hidden> wrote:
> > On Fri, 30 May 2014 19:48:13 +1000
> > Peter Crosthwaite <address@hidden> wrote:
> >
> >> On Fri, May 30, 2014 at 7:01 PM, Igor Mammedov <address@hidden> wrote:
> >> > On Fri, 30 May 2014 00:21:27 +1000
> >> > Peter Crosthwaite <address@hidden> wrote:
> >> >
> >> >> On Tue, May 27, 2014 at 11:01 PM, Igor Mammedov <address@hidden> wrote:
> >> >> > From: Vasilis Liaskovitis <address@hidden>
> >> >> >
> >> >> > 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 <address@hidden>
> >> >> > Signed-off-by: Igor Mammedov <address@hidden>
> >> >> > ---
> >> >> > 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 <address@hidden>
> >> >> > + *  Igor Mammedov <address@hidden>
> >> >> > + *
> >> >> > + * 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