Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: factor out MemoryDevice interface

[David Hildenbrand]

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 <address@hidden>
>> ---
>>  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