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

[Peter Crosthwaite]

On Fri, May 30, 2014 at 7:48 PM, 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.
>

So it occurs to me if you do this, it would be a deqomification while
also reducing this abstraction to pretty much nothing. It leaves me to
believe that the entire reason for existence for this device
abstraction is PC specific. To that end, I think the whole thing can
be simply clarified by saying this abstraction is PC (or ACPI?)
specific in namings etc. Just call it TYPE_PC_DIMM or something with
mention of its intended use WRT to addresses and slots etc. Then if
someone wants to come along and implement more generic DIMM (with
actual DIMM specific features) they can do that as TYPE_DIMM.
TYPE_PC_DIMM would then be parented to TYPE_DIMM should there be
usable features for actual DIMM that PC platforms can take advantage
of.

The application of this would be some of our existing platforms that
just trivial MRs but really back onto DIMMs.
hw/microblaze/petalogix_ml605_mmu would be an example which does RAM
with a SODIMM slot in real HW:

http://blogs.msdn.com/cfs-filesystemfile.ashx/__key/communityserver-blogs-components-weblogfiles/00-00-01-32-57-metablogapi/2654.image_5F00_8.png

But wouldn't care about any of this slot and address stuffs.

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