[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 10/34] dimm: implement dimm device abstractio
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v3 10/34] dimm: implement dimm device abstraction |
Date: |
Fri, 30 May 2014 19:48:13 +1000 |
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.
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
>
- [Qemu-devel] [PATCH v3 06/34] vl.c: extend -m option to support options for memory hotplug, (continued)
- [Qemu-devel] [PATCH v3 06/34] vl.c: extend -m option to support options for memory hotplug, Igor Mammedov, 2014/05/27
- [Qemu-devel] [PATCH v3 01/34] machine: Conversion of QEMUMachineInitArgs to MachineState, Igor Mammedov, 2014/05/27
- [Qemu-devel] [PATCH v3 07/34] pc: create custom generic PC machine type, Igor Mammedov, 2014/05/27
- [Qemu-devel] [PATCH v3 09/34] qdev: expose DeviceState.hotplugged field as a property, Igor Mammedov, 2014/05/27
- [Qemu-devel] [PATCH v3 08/34] qdev: hotplug for buss-less devices, Igor Mammedov, 2014/05/27
- [Qemu-devel] [PATCH v3 10/34] dimm: implement dimm device abstraction, Igor Mammedov, 2014/05/27
- Re: [Qemu-devel] [PATCH v3 10/34] dimm: implement dimm device abstraction, Peter Crosthwaite, 2014/05/30
[Qemu-devel] [PATCH v3 11/34] memory: add memory_region_is_mapped() API, Igor Mammedov, 2014/05/27
[Qemu-devel] [PATCH v3 13/34] pc: initialize memory hotplug address space, Igor Mammedov, 2014/05/27
[Qemu-devel] [PATCH v3 14/34] pc: exit QEMU if number of slots more than supported 256, Igor Mammedov, 2014/05/27