On 13.04.2018 05:25, David Gibson wrote:
> On Thu, Apr 12, 2018 at 04:36:33PM +0200, David Hildenbrand wrote:
>> On the qmp level, we already have the concept of memory devices:
>> "query-memory-devices"
>> Right now, we only support NVDIMM and PCDIMM.
>>
>> We want to map other devices later into the address space of the guest.
>> Such device could e.g. be virtio devices. These devices will have a
>> guest memory range assigned but won't be exposed via e.g. ACPI. We want
>> to make them look like memory device, but not glued to pc-dimm.
>>
>> Especially, it will not always be possible to have TYPE_PC_DIMM as a parent
>> class (e.g. virtio devices). Let's use an interface instead. As a first
>> part, convert handling of
>> - qmp_pc_dimm_device_list
>> - get_plugged_memory_size
>> to our new model. plug/unplug stuff etc. will follow later.
>>
>> A memory device will have to provide the following functions:
>> - get_addr(): Necessary, as the property "addr" can e.g. not be used for
>> virtio devices (already defined).
>
> Is the assumption here that a memory device has a fixed RAM address
> (GPA) from when it is realized? It seems to me plausible that you
> could have a memory device that has a specific size, but has a BAR of
> some sort that's programmed by the guest as part of the "plug"
> process.
Yes, the assumption is that the
a) address in physical memory
b) region size
Are defined at realize time. Which holds true for DIMM/NVDIMM and will
hold true for the two virtio device types we have in mind
(virtio-mem/virtio-pmem).
Both basically allow to find free address ranges.
>
>> - get_plugged_size(): The amount this device offers to the guest as of
>> now.
>> - get_region_size(): Because this can later on be bigger than the
>> plugged size.
>> - fill_device_info(): Fill MemoryDeviceInfo, e.g. for qmp.
>>
>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>> ---
>> hw/i386/acpi-build.c | 3 +-
>> hw/mem/Makefile.objs | 1 +
>> hw/mem/memory-device.c | 119
>> +++++++++++++++++++++++++++
>> hw/mem/pc-dimm.c | 119
>> ++++++++++++++-------------
>> hw/ppc/spapr.c | 3 +-
>> hw/ppc/spapr_hcall.c | 1 +
>> include/hw/mem/memory-device.h | 44 ++++++++++
>> include/hw/mem/pc-dimm.h | 2 -
>> numa.c | 3 +-
>> qmp.c | 4 +-
>> stubs/Makefile.objs | 2 +-
>> stubs/{qmp_pc_dimm.c => qmp_memory_device.c} | 4 +-
>> 12 files changed, 239 insertions(+), 66 deletions(-)
>> create mode 100644 hw/mem/memory-device.c
>> create mode 100644 include/hw/mem/memory-device.h
>> rename stubs/{qmp_pc_dimm.c => qmp_memory_device.c} (61%)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 3cf2a1679c..ca3645d57b 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -46,6 +46,7 @@
>> #include "hw/acpi/vmgenid.h"
>> #include "sysemu/tpm_backend.h"
>> #include "hw/timer/mc146818rtc_regs.h"
>> +#include "hw/mem/memory-device.h"
>> #include "sysemu/numa.h"
>>
>> /* Supported chipsets: */
>> @@ -2253,7 +2254,7 @@ build_tpm2(GArray *table_data, BIOSLinker *linker,
>> GArray *tcpalog)
>> static void build_srat_hotpluggable_memory(GArray *table_data, uint64_t
>> base,
>> uint64_t len, int default_node)
>> {
>> - MemoryDeviceInfoList *info_list = qmp_pc_dimm_device_list();
>> + MemoryDeviceInfoList *info_list = qmp_memory_device_list();
>> MemoryDeviceInfoList *info;
>> MemoryDeviceInfo *mi;
>> PCDIMMDeviceInfo *di;
>> diff --git a/hw/mem/Makefile.objs b/hw/mem/Makefile.objs
>> index f12f8b97a2..10be4df2a2 100644
>> --- a/hw/mem/Makefile.objs
>> +++ b/hw/mem/Makefile.objs
>> @@ -1,2 +1,3 @@
>> common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o
>> +common-obj-$(CONFIG_MEM_HOTPLUG) += memory-device.o
>> common-obj-$(CONFIG_NVDIMM) += nvdimm.o
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> new file mode 100644
>> index 0000000000..0e0d6b539a
>> --- /dev/null
>> +++ b/hw/mem/memory-device.c
>> @@ -0,0 +1,119 @@
>> +/*
>> + * Memory Device Interface
>> + *
>> + * Copyright ProfitBricks GmbH 2012
>> + * Copyright (C) 2014 Red Hat Inc
>> + * Copyright (c) 2018 Red Hat Inc
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/mem/memory-device.h"
>> +#include "hw/qdev.h"
>> +#include "qapi/error.h"
>> +#include "hw/boards.h"
>> +#include "qemu/range.h"
>> +
>> +static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
>> +{
>> + MemoryDeviceState *md_a = MEMORY_DEVICE(a);
>> + MemoryDeviceState *md_b = MEMORY_DEVICE(b);
>> + MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(a);
>
> AFAICT from the code below, the two devices don't necessarily have to
> have the same class, so I think you need to get the class pointers for
> each of them separately. (Or if they do have to be the same by
> design, I think there should at least be an assert).
Haven't considered that, my assumption was that any memory device class
would be sufficient to find/use the pointer (as that layout should not
change). But I might be wrong, so I'll lookup both pointers, thanks!
>
>> + const uint64_t addr_a = mdc->get_addr(md_a);
>> + const uint64_t addr_b = mdc->get_addr(md_b);
>> +
>> + if (addr_a > addr_b) {
>> + return 1;
>> + } else if (addr_a < addr_b) {
>> + return -1;
>> + }
>> + return 0;
>> +}
--
Thanks,
David / dhildenb
