memory-device: complete refactoring + virtio-pmem
v2 cover memory-device: complete refactoring + virtio-pmem
This series completes refactoring of pre_plug, plug and unplug logic of memory devices. With this as a basis, one can easily have e.g. virtio based memory devices (virtio-mem, virtio-pmem, virtio-fs?) with minor modifications on e.g. x86.
Patch 1-14 are the "real" patches of this series. Patch 15-20 are a modified version of the virtio-pmem prototype that makes use of the new memory-device hotplug model.
I modified Pankajs work to work with this series. virtio-pmem is included as it was requested during the review to showcase a real user, so we can discuss if this is good enough for us or if we have to do further changes.
More details about virtio-pmem can be found at:
https://lkml.org/lkml/2018/7/13/102
Of special interest regarding virtio devices being memory-devices are:
- "virtio-pmem: hotplug support functions"
- "pc: support for virtio-pmem"
Example: defining a simple virtio-pmem device (on /dev/zero for simplicity):
qemu-system-x86_64 \
-machine pc \
-m 8G,maxmem=20G \
-object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=4G \
-device virtio-pmem-pci,id=vp1,memdev=mem1QEMU 3.0.50 monitor - type 'help' for more information (qemu) info memory-devices
Memory device [virtio-pmem]: "vp1"
memaddr: 0x240000000
size: 4294967296
memdev: /objects/mem1
(qemu) info memory_size_summary
base memory: 8589934592
plugged memory: 4294967296
v1 -> v2:
- Include virtio-pmem prototype
- Add "memory-device: fix error message when hinted address is too small"
- Add "memory-device: use memory device terminology in error messages"
- Add "hw/acpi-build: only indicate nvdimm and pc-dimm"
- Add "memory-device: ids of virtio based devices are special"
- Add "memory-device: ids of virtio based devices are special"
- Minor changes/fixes for other patches
David Hildenbrand (18):
memory-device: fix error message when hinted address is too small
memory-device: use memory device terminology in error messages
memory-device: introduce separate config option
memory-device: get_region_size()/get_plugged_size() might fail
memory-device: convert get_region_size() to get_memory_region()
memory-device: document MemoryDeviceClass
memory-device: add device class function set_addr()
pc-dimm: implement memory device class function set_addr()
memory-device: complete factoring out pre_plug handling
memory-device: complete factoring out plug handling
memory-device: complete factoring out unplug handling
memory-device: trace when pre_assigning/assigning/unassigning
addresses
hw/acpi-build: only indicate nvdimm and pc-dimm
memory-device: ids of virtio based devices are special
hmp: handle virtio-pmem when printing memory device infos
numa: handle virtio-pmem in NUMA stats
virtio-pmem: hotplug support functions
pc: support for virtio-pmem
Pankaj Gupta (2):
virtio-pmem: prototype
virtio-pci: proxy for virtio-pmem
default-configs/i386-softmmu.mak | 4 +- default-configs/ppc64-softmmu.mak | 3 +- hmp.c | 29 ++- hw/Makefile.objs | 2 +- hw/i386/acpi-build.c | 15 +- hw/i386/pc.c | 10 +- hw/mem/Makefile.objs | 4 +- hw/mem/memory-device.c | 98 ++++++-- hw/mem/pc-dimm.c | 59 ++--- hw/mem/trace-events | 5 +- hw/virtio/Makefile.objs | 1 + hw/virtio/virtio-pci.c | 41 ++++ hw/virtio/virtio-pci.h | 14 ++ hw/virtio/virtio-pmem.c | 256 ++++++++++++++++++++ include/hw/mem/memory-device.h | 30 ++- include/hw/pci/pci.h | 1 + include/hw/virtio/virtio-pmem.h | 51 ++++ include/standard-headers/linux/virtio_ids.h | 1 + numa.c | 25 +- qapi/misc.json | 28 ++- 20 files changed, 582 insertions(+), 95 deletions(-) create mode 100644 hw/virtio/virtio-pmem.c create mode 100644 include/hw/virtio/virtio-pmem.h
Signature · 1 line
2.17.1
PATCH v2 1/20 memory-device: fix error message when hinted address is too small
The "at" should actually be a "before".
if (new_addr < address_space_start)
-> "can't add memory ... before... $address_space_start"
So it looks similar to the other check
} else if ((new_addr + size) > address_space_end)
-> "can't add memory ... beyond..."--- hw/mem/memory-device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
hw/mem/memory-device.c +2 -1
@@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, new_addr = *hint;if (new_addr < address_space_start) {error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 -"] at 0x%" PRIx64, new_addr, size, address_space_start); +"] before 0x%" PRIx64, new_addr, size, + address_space_start);return 0 ; }else if ((new_addr + size) > address_space_end) { error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 01/20] memory-device: fix error message when hinted address is too small
Quoted 18 lines · * David Hildenbrand ([email protected]) wrote:
The "at" should actually be a "before".
if (new_addr<address_space_start)
-> "can't add memory ... before... $address_space_start"So it looks similar to the other check
} else if ((new_addr + size) > address_space_end) -> "can't add memory ... beyond..."Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)hw/mem/memory-device.c +0
-0
hw/mem/memory-device.c uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, · line 150
Quoted 6 lines
hw/mem/memory-device.c +2
-1@@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, new_addr = *hint;if (new_addr < address_space_start) {error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 -"] at 0x%" PRIx64, new_addr, size, address_space_start); +"] before 0x%" PRIx64, new_addr, size, + address_space_start);
OK, so
hw/mem/memory-device.c uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, · line 152
Quoted 2 lines
hw/mem/memory-device.c +0
-0return 0 ; }else if ((new_addr + size) > address_space_end) {
That comparison (that's already there) doesn't look wrap-safe?
Dave
Quoted 5 lines
hw/mem/memory-device.c +0
-0error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64Signature · 2 lines
-- 2.17.1
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v2 01/20] memory-device: fix error message when hinted address is too small
In reply to David HildenbrandQuoted 9 lines · On Wed, 29 Aug 2018 17:36:05 +0200 David Hildenbrand <[email protected]> wrote:
The "at" should actually be a "before".
if (new_addr<address_space_start)
-> "can't add memory ... before... $address_space_start"So it looks similar to the other check
} else if ((new_addr + size) > address_space_end) -> "can't add memory ... beyond..."Signed-off-by David Hildenbrand <[email protected]>
Quoted 18 lines
--- hw/mem/memory-device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)hw/mem/memory-device.c +2
-1@@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, new_addr = *hint;if (new_addr < address_space_start) {error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 -"] at 0x%" PRIx64, new_addr, size, address_space_start); +"] before 0x%" PRIx64, new_addr, size, + address_space_start);return 0 ; }else if ((new_addr + size) > address_space_end) { error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
PATCH v2 2/20 memory-device: use memory device terminology in error messages
In reply to David HildenbrandWhile we rephrased most error messages, we missed these.
--- hw/mem/memory-device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
hw/mem/memory-device.c +3 -3
@@ -87,7 +87,7 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,memory_device_used_region_size (OBJECT (ms), &used_region_size);if (used_region_size + size > ms->maxram_size - ms->ram_size) {error_setg (errp,"not enough space, currently 0x%" PRIx64 -" in use of total hot pluggable 0x" RAM_ADDR_FMT , +" in use of total space for memory devices 0x" RAM_ADDR_FMT , used_region_size, ms->maxram_size - ms->ram_size);return ; }
@@ -145,12 +145,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,if (hint) { new_addr = *hint;if (new_addr < address_space_start) { -error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 +error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64"] before 0x%" PRIx64, new_addr, size, address_space_start);return 0 ; }else if ((new_addr + size) > address_space_end) { -error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 +error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64"] beyond 0x%" PRIx64, new_addr, size, address_space_end);return 0 ;
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 02/20] memory-device: use memory device terminology in error messages
Quoted 3 lines · * David Hildenbrand ([email protected]) wrote:
While we rephrased most error messages, we missed these.
Signed-off-by David Hildenbrand <[email protected]>
Quoted 35 lines
--- hw/mem/memory-device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)hw/mem/memory-device.c +3
-3@@ -87,7 +87,7 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,memory_device_used_region_size (OBJECT (ms), &used_region_size);if (used_region_size + size > ms->maxram_size - ms->ram_size) {error_setg (errp,"not enough space, currently 0x%" PRIx64 -" in use of total hot pluggable 0x" RAM_ADDR_FMT , +" in use of total space for memory devices 0x" RAM_ADDR_FMT , used_region_size, ms->maxram_size - ms->ram_size);return ; }@@ -145,12 +145,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,if (hint) { new_addr = *hint;if (new_addr < address_space_start) { -error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 +error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64"] before 0x%" PRIx64, new_addr, size, address_space_start);return 0 ; }else if ((new_addr + size) > address_space_end) { -error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 +error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64"] beyond 0x%" PRIx64, new_addr, size, address_space_end);return 0 ;Signature · 1 line
2.17.1
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v2 02/20] memory-device: use memory device terminology in error messages
In reply to David HildenbrandQuoted 3 lines · On Wed, 29 Aug 2018 17:36:06 +0200 David Hildenbrand <[email protected]> wrote:
While we rephrased most error messages, we missed these.
Signed-off-by David Hildenbrand <[email protected]>
Quoted 32 lines
--- hw/mem/memory-device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)hw/mem/memory-device.c +3
-3@@ -87,7 +87,7 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,memory_device_used_region_size (OBJECT (ms), &used_region_size);if (used_region_size + size > ms->maxram_size - ms->ram_size) {error_setg (errp,"not enough space, currently 0x%" PRIx64 -" in use of total hot pluggable 0x" RAM_ADDR_FMT , +" in use of total space for memory devices 0x" RAM_ADDR_FMT , used_region_size, ms->maxram_size - ms->ram_size);return ; }@@ -145,12 +145,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,if (hint) { new_addr = *hint;if (new_addr < address_space_start) { -error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 +error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64"] before 0x%" PRIx64, new_addr, size, address_space_start);return 0 ; }else if ((new_addr + size) > address_space_end) { -error_setg (errp,"can't add memory [0x%" PRIx64":0x%" PRIx64 +error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64"] beyond 0x%" PRIx64, new_addr, size, address_space_end);return 0 ;
PATCH v2 3/20 memory-device: introduce separate config option
In reply to David HildenbrandSome architectures might support memory devices, while they don't support DIMM/NVDIMM. So let's
- Rename CONFIG_MEM_HOTPLUG to CONFIG_MEM_DEVICE
- Introduce CONFIG_DIMM and use it similarly to CONFIG NVDIMM
CONFIG_DIMM and CONFIG_NVDIMM require CONFIG_MEM_DEVICE.
--- default-configs/i386-softmmu.mak | 3 ++- default-configs/ppc64-softmmu.mak | 3 ++- hw/Makefile.objs | 2 +- hw/mem/Makefile.objs | 4 ++-- qapi/misc.json | 2 +- 5 files changed, 8 insertions(+), 6 deletions(-)
default-configs/i386-softmmu.mak +2 -1
@@ -50,7 +50,8 @@ CONFIG_PCI_Q35=y CONFIG_APIC=y CONFIG_IOAPIC=y CONFIG_PVPANIC=y -CONFIG_MEM_HOTPLUG=y +CONFIG_MEM_DEVICE=y +CONFIG_DIMM=y CONFIG_NVDIMM=y CONFIG_ACPI_NVDIMM=y CONFIG_PCIE_PORT=y
default-configs/ppc64-softmmu.mak +2 -1
@@ -16,4 +16,5 @@ CONFIG_VIRTIO_VGA=y CONFIG_XICS=$(CONFIG_PSERIES) CONFIG_XICS_SPAPR=$(CONFIG_PSERIES) CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM)) -CONFIG_MEM_HOTPLUG=y +CONFIG_MEM_DEVICE=y +CONFIG_DIMM=y
hw/Makefile.objs +1 -1
@@ -33,7 +33,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += vfio/ devices-dirs-$(CONFIG_SOFTMMU) += virtio/ devices-dirs-$(CONFIG_SOFTMMU) += watchdog/ devices-dirs-$(CONFIG_SOFTMMU) += xen/ -devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/ +devices-dirs-$(CONFIG_MEM_DEVICE) += mem/ devices-dirs-$(CONFIG_SOFTMMU) += smbios/ devices-dirs-y += core/ common-obj-y += $(devices-dirs-y)
hw/mem/Makefile.objs +2 -2
@@ -1,3 +1,3 @@ -common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o -common-obj-$(CONFIG_MEM_HOTPLUG) += memory-device.o +common-obj-$(CONFIG_DIMM) += pc-dimm.o +common-obj-$(CONFIG_MEM_DEVICE) += memory-device.o common-obj-$(CONFIG_NVDIMM) += nvdimm.o
qapi/misc.json +1 -1
@@ -2061,7 +2061,7 @@ # # @plugged-memory: size of memory that can be hot-unplugged. This field # is omitted if target doesn't support memory hotplug -# (i.e. CONFIG_MEM_HOTPLUG not defined on build time). +# (i.e. CONFIG_MEM_DEVICE not defined at build time). # # Since: 2.11.0 ##
Signature · 1 line
2.17.1
PATCH v2 4/20 memory-device: get_region_size()/get_plugged_size() might fail
In reply to David HildenbrandLet's properly forward the error, so errors from get_region_size() / get_plugged_size(), can be handled.
Users right now call both functions after the device has been realized, which is guaranteed to no fail (we'll document this behavior in a follow-up patch).
--- hw/mem/memory-device.c | 6 +++--- hw/mem/pc-dimm.c | 5 +++-- include/hw/mem/memory-device.h | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-)
hw/mem/memory-device.c +3 -3
@@ -60,7 +60,7 @@ static int memory_device_used_region_size(Object *obj, void *opaque)const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (obj);if (dev->realized) { - *size += mdc->get_region_size (md); + *size += mdc->get_region_size (md, &error_abort); } }
@@ -167,7 +167,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,uint64_t md_size, md_addr; md_addr = mdc->get_addr (md); - md_size = mdc->get_region_size (md); + md_size = mdc->get_region_size (md, &error_abort);if (*errp) { goto out; }
@@ -233,7 +233,7 @@ static int memory_device_plugged_size(Object *obj, void *opaque)const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (obj);if (dev->realized) { - *size += mdc->get_plugged_size (md); + *size += mdc->get_plugged_size (md, &error_abort); } }
hw/mem/pc-dimm.c +3 -2
@@ -236,14 +236,15 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)return dimm->addr; } -static uint64_t pc_dimm_md_get_region_size (const MemoryDeviceState *md) +static uint64_t pc_dimm_md_get_region_size (const MemoryDeviceState *md, +Error **errp) {/* dropping const here is fine as we don't touch the memory region */ PCDIMMDevice *dimm =PC_DIMM (md);const PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (md);MemoryRegion *mr; - mr = ddc->get_memory_region (dimm, &error_abort); + mr = ddc->get_memory_region (dimm, errp);if (!mr) {return 0 ; }
include/hw/mem/memory-device.h +2 -2
@@ -33,8 +33,8 @@ typedef struct MemoryDeviceClass {InterfaceClass parent_class;uint64_t (*get_addr)(const MemoryDeviceState *md); -uint64_t (*get_plugged_size)(const MemoryDeviceState *md); -uint64_t (*get_region_size)(const MemoryDeviceState *md); +uint64_t (*get_plugged_size)(const MemoryDeviceState *md,Error **errp); +uint64_t (*get_region_size)(const MemoryDeviceState *md,Error **errp);void (*fill_device_info)(const MemoryDeviceState *md,MemoryDeviceInfo *info); } MemoryDeviceClass;
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 04/20] memory-device: get_region_size()/get_plugged_size() might fail
Quoted 28 lines · On Wed, 29 Aug 2018 17:36:08 +0200 David Hildenbrand <[email protected]> wrote:
Let's properly forward the error, so errors from
get_region_size()/get_plugged_size(), can be handled.Users right now call both functions after the device has been realized, which is guaranteed to no fail (we'll document this behavior in a follow-up patch).
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 6 +++--- hw/mem/pc-dimm.c | 5 +++-- include/hw/mem/memory-device.h | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-)hw/mem/memory-device.c +1
-1@@ -60,7 +60,7 @@ static int memory_device_used_region_size(Object *obj, void *opaque)const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (obj);if (dev->realized) { - *size += mdc->get_region_size (md); + *size += mdc->get_region_size (md, &error_abort); } }
hw/mem/memory-device.c uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, · line 170
Quoted 5 lines
hw/mem/memory-device.c +1
-1@@ -167,7 +167,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,uint64_t md_size, md_addr; md_addr = mdc->get_addr (md); - md_size = mdc->get_region_size (md); + md_size = mdc->get_region_size (md, &error_abort);
s/error_abort/errp/, so it won't crash on hotplug?
hw/mem/memory-device.c uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, · line 172
Quoted 2 lines
hw/mem/memory-device.c +0
-0if (*errp) { goto out;
is this ever reachable?
Quoted 47 lines
hw/mem/memory-device.c +1
-1}@@ -233,7 +233,7 @@ static int memory_device_plugged_size(Object *obj, void *opaque)const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (obj);if (dev->realized) { - *size += mdc->get_plugged_size (md); + *size += mdc->get_plugged_size (md, &error_abort); } }hw/mem/pc-dimm.c +3
-2@@ -236,14 +236,15 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)return dimm->addr; } -static uint64_t pc_dimm_md_get_region_size (const MemoryDeviceState *md) +static uint64_t pc_dimm_md_get_region_size (const MemoryDeviceState *md, +Error **errp) {/* dropping const here is fine as we don't touch the memory region */ PCDIMMDevice *dimm =PC_DIMM (md);const PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (md);MemoryRegion *mr; - mr = ddc->get_memory_region (dimm, &error_abort); + mr = ddc->get_memory_region (dimm, errp);if (!mr) {return 0 ; }include/hw/mem/memory-device.h +2
-2@@ -33,8 +33,8 @@ typedef struct MemoryDeviceClass {InterfaceClass parent_class;uint64_t (*get_addr)(const MemoryDeviceState *md); -uint64_t (*get_plugged_size)(const MemoryDeviceState *md); -uint64_t (*get_region_size)(const MemoryDeviceState *md); +uint64_t (*get_plugged_size)(const MemoryDeviceState *md,Error **errp); +uint64_t (*get_region_size)(const MemoryDeviceState *md,Error **errp);void (*fill_device_info)(const MemoryDeviceState *md,MemoryDeviceInfo *info); } MemoryDeviceClass;
PATCH v2 5/20 memory-device: convert get_region_size() to get_memory_region()
In reply to David HildenbrandTo factor out plugging and unplugging of memory device we need access to the memory region. So let's replace get_region_size() by get_memory_region().
If any memory device will in the future have multiple memory regions that make up the total region size, it can simply use a wrapping memory region to embed the other ones.
--- hw/mem/memory-device.c | 10 ++++++---- hw/mem/pc-dimm.c | 19 ++++++++++++++----- include/hw/mem/memory-device.h | 2 +- 3 files changed, 21 insertions(+), 10 deletions(-)
hw/mem/memory-device.c +6 -4
@@ -56,11 +56,13 @@ static int memory_device_used_region_size(Object *obj, void *opaque)if (object_dynamic_cast (obj,TYPE_MEMORY_DEVICE )) {const DeviceState *dev =DEVICE (obj); -const MemoryDeviceState *md =MEMORY_DEVICE (obj); +MemoryDeviceState *md =MEMORY_DEVICE (obj);const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (obj);if (dev->realized) { - *size += mdc->get_region_size (md, &error_abort); +MemoryRegion *mr = mdc->get_memory_region (md, &error_abort); + + *size +=memory_region_size (mr); } }
@@ -162,12 +164,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,/* find address range that will fit new memory device */ object_child_foreach (OBJECT (ms), memory_device_build_list, &list);for (item = list; item; item =g_slist_next (item)) { -const MemoryDeviceState *md = item->data; +MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md));uint64_t md_size, md_addr; md_addr = mdc->get_addr (md); - md_size = mdc->get_region_size (md, &error_abort); + md_size =memory_region_size (mdc->get_memory_region (md, &error_abort));if (*errp) { goto out; }
hw/mem/pc-dimm.c +14 -5
@@ -236,8 +236,8 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)return dimm->addr; } -static uint64_t pc_dimm_md_get_region_size (const MemoryDeviceState *md, -Error **errp) +static uint64_t pc_dimm_md_get_plugged_size (const MemoryDeviceState *md, +Error **errp) {/* dropping const here is fine as we don't touch the memory region */ PCDIMMDevice *dimm =PC_DIMM (md);
@@ -249,9 +249,19 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,return 0 ; } +/* for a dimm plugged_size == region_size */ return memory_region_size (mr); } +static MemoryRegion *pc_dimm_md_get_memory_region (MemoryDeviceState *md, +Error **errp) +{ +PCDIMMDevice *dimm =PC_DIMM (md); +const PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (md); + +return ddc->get_memory_region (dimm, errp); +} +static void pc_dimm_md_fill_device_info (const MemoryDeviceState *md,MemoryDeviceInfo *info) {
@@ -297,9 +307,8 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data) ddc->get_vmstate_memory_region = pc_dimm_get_memory_region; mdc->get_addr = pc_dimm_md_get_addr; -/* for a dimm plugged_size == region_size */ - mdc->get_plugged_size = pc_dimm_md_get_region_size; - mdc->get_region_size = pc_dimm_md_get_region_size; + mdc->get_plugged_size = pc_dimm_md_get_plugged_size; + mdc->get_memory_region = pc_dimm_md_get_memory_region; mdc->fill_device_info = pc_dimm_md_fill_device_info; }
include/hw/mem/memory-device.h +1 -1
@@ -34,7 +34,7 @@ typedef struct MemoryDeviceClass {uint64_t (*get_addr)(const MemoryDeviceState *md);uint64_t (*get_plugged_size)(const MemoryDeviceState *md,Error **errp); -uint64_t (*get_region_size)(const MemoryDeviceState *md,Error **errp); + MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);void (*fill_device_info)(const MemoryDeviceState *md,MemoryDeviceInfo *info); } MemoryDeviceClass;
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 05/20] memory-device: convert get_region_size() to get_memory_region()
Quoted 20 lines · On Wed, 29 Aug 2018 17:36:09 +0200 David Hildenbrand <[email protected]> wrote:
To factor out plugging and unplugging of memory device we need access to the memory region. So let's replace
get_region_size()byget_memory_region().If any memory device will in the future have multiple memory regions that make up the total region size, it can simply use a wrapping memory region to embed the other ones.
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 10 ++++++---- hw/mem/pc-dimm.c | 19 ++++++++++++++----- include/hw/mem/memory-device.h | 2 +- 3 files changed, 21 insertions(+), 10 deletions(-)hw/mem/memory-device.c +0
-0
hw/mem/memory-device.c static int memory_device_used_region_size(Object *obj, void *opaque) · line 59
Quoted 5 lines
hw/mem/memory-device.c +1
-1@@ -56,11 +56,13 @@ static int memory_device_used_region_size(Object *obj, void *opaque)if (object_dynamic_cast (obj,TYPE_MEMORY_DEVICE )) {const DeviceState *dev =DEVICE (obj); -const MemoryDeviceState *md =MEMORY_DEVICE (obj); +MemoryDeviceState *md =MEMORY_DEVICE (obj);
it's a getter, why do we loose 'const' here?
Quoted 11 lines
hw/mem/memory-device.c +3
-1const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (obj);if (dev->realized) { - *size += mdc->get_region_size (md, &error_abort); +MemoryRegion *mr = mdc->get_memory_region (md, &error_abort); + + *size +=memory_region_size (mr); } }
hw/mem/memory-device.c uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, · line 167
Quoted 5 lines
hw/mem/memory-device.c +1
-1@@ -162,12 +164,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,/* find address range that will fit new memory device */ object_child_foreach (OBJECT (ms), memory_device_build_list, &list);for (item = list; item; item =g_slist_next (item)) { -const MemoryDeviceState *md = item->data; +MemoryDeviceState *md = item->data;
ditto
Quoted 69 lines
hw/mem/memory-device.c +1
-1const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md));uint64_t md_size, md_addr; md_addr = mdc->get_addr (md); - md_size = mdc->get_region_size (md, &error_abort); + md_size =memory_region_size (mdc->get_memory_region (md, &error_abort));if (*errp) { goto out; }hw/mem/pc-dimm.c +14
-5@@ -236,8 +236,8 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)return dimm->addr; } -static uint64_t pc_dimm_md_get_region_size (const MemoryDeviceState *md, -Error **errp) +static uint64_t pc_dimm_md_get_plugged_size (const MemoryDeviceState *md, +Error **errp) {/* dropping const here is fine as we don't touch the memory region */ PCDIMMDevice *dimm =PC_DIMM (md);@@ -249,9 +249,19 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,return 0 ; } +/* for a dimm plugged_size == region_size */ return memory_region_size (mr); } +static MemoryRegion *pc_dimm_md_get_memory_region (MemoryDeviceState *md, +Error **errp) +{ +PCDIMMDevice *dimm =PC_DIMM (md); +const PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (md); + +return ddc->get_memory_region (dimm, errp); +} +static void pc_dimm_md_fill_device_info (const MemoryDeviceState *md,MemoryDeviceInfo *info) {@@ -297,9 +307,8 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data) ddc->get_vmstate_memory_region = pc_dimm_get_memory_region; mdc->get_addr = pc_dimm_md_get_addr; -/* for a dimm plugged_size == region_size */ - mdc->get_plugged_size = pc_dimm_md_get_region_size; - mdc->get_region_size = pc_dimm_md_get_region_size; + mdc->get_plugged_size = pc_dimm_md_get_plugged_size; + mdc->get_memory_region = pc_dimm_md_get_memory_region; mdc->fill_device_info = pc_dimm_md_fill_device_info; }include/hw/mem/memory-device.h +1
-1@@ -34,7 +34,7 @@ typedef struct MemoryDeviceClass {uint64_t (*get_addr)(const MemoryDeviceState *md);uint64_t (*get_plugged_size)(const MemoryDeviceState *md,Error **errp); -uint64_t (*get_region_size)(const MemoryDeviceState *md,Error **errp); + MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);void (*fill_device_info)(const MemoryDeviceState *md,MemoryDeviceInfo *info); } MemoryDeviceClass;
Re: [Qemu-devel] [PATCH v2 05/20] memory-device: convert get_region_size() to get_memory_region()
Quoted 6 lines · On Thu, 13 Sep 2018 14:20:25 +0200 Igor Mammedov <[email protected]> wrote:
Quoted 3 lines · On Wed, 29 Aug 2018 17:36:09 +0200 David Hildenbrand <[email protected]> wrote:
To factor out plugging and unplugging of memory device we need access to the memory region. So let's replace
get_region_size()byget_memory_region().
this part isn't clear and doesn't really answer "why" patch's been written and how it will really help.
Quoted 4 lines
Quoted 4 lines
If any memory device will in the future have multiple memory regions that make up the total region size, it can simply use a wrapping memory region to embed the other ones.
It might be better to document expectation as part of get_memory_region() doc comment.
I'm not really getting reasoning behind this patch.
* Why get_region_size() doesn't work for you? benefits I see is that's one less get_foo_size() callback, so it becomes less confusing
* I get we might need access to memory region at MemoryDeviceClass level, but why are you keeping PCDIMMDeviceClass::get_memory_region()? I'd expect the later being generalized (moved) to MemoryDeviceClass so we would end up with one level indirection that we have now.
Quoted 108 lines
Quoted 12 lines
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 10 ++++++---- hw/mem/pc-dimm.c | 19 ++++++++++++++----- include/hw/mem/memory-device.h | 2 +- 3 files changed, 21 insertions(+), 10 deletions(-)hw/mem/memory-device.c +0
-0hw/mem/memory-device.c static int memory_device_used_region_size(Object *obj, void *opaque) · line 59
Quoted 5 lines
hw/mem/memory-device.c +1
-1@@ -56,11 +56,13 @@ static int memory_device_used_region_size(Object *obj, void *opaque)if (object_dynamic_cast (obj,TYPE_MEMORY_DEVICE )) {const DeviceState *dev =DEVICE (obj); -const MemoryDeviceState *md =MEMORY_DEVICE (obj); +MemoryDeviceState *md =MEMORY_DEVICE (obj);it's a getter, why do we loose 'const' here?
Quoted 11 lines
hw/mem/memory-device.c +3
-1const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (obj);if (dev->realized) { - *size += mdc->get_region_size (md, &error_abort); +MemoryRegion *mr = mdc->get_memory_region (md, &error_abort); + + *size +=memory_region_size (mr); } }hw/mem/memory-device.c uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, · line 167
Quoted 5 lines
hw/mem/memory-device.c +1
-1@@ -162,12 +164,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,/* find address range that will fit new memory device */ object_child_foreach (OBJECT (ms), memory_device_build_list, &list);for (item = list; item; item =g_slist_next (item)) { -const MemoryDeviceState *md = item->data; +MemoryDeviceState *md = item->data;ditto
Quoted 69 lines
hw/mem/memory-device.c +1
-1const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md));uint64_t md_size, md_addr; md_addr = mdc->get_addr (md); - md_size = mdc->get_region_size (md, &error_abort); + md_size =memory_region_size (mdc->get_memory_region (md, &error_abort));if (*errp) { goto out; }hw/mem/pc-dimm.c +14
-5@@ -236,8 +236,8 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)return dimm->addr; } -static uint64_t pc_dimm_md_get_region_size (const MemoryDeviceState *md, -Error **errp) +static uint64_t pc_dimm_md_get_plugged_size (const MemoryDeviceState *md, +Error **errp) {/* dropping const here is fine as we don't touch the memory region */ PCDIMMDevice *dimm =PC_DIMM (md);@@ -249,9 +249,19 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,return 0 ; } +/* for a dimm plugged_size == region_size */ return memory_region_size (mr); } +static MemoryRegion *pc_dimm_md_get_memory_region (MemoryDeviceState *md, +Error **errp) +{ +PCDIMMDevice *dimm =PC_DIMM (md); +const PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (md); + +return ddc->get_memory_region (dimm, errp); +} +static void pc_dimm_md_fill_device_info (const MemoryDeviceState *md,MemoryDeviceInfo *info) {@@ -297,9 +307,8 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data) ddc->get_vmstate_memory_region = pc_dimm_get_memory_region; mdc->get_addr = pc_dimm_md_get_addr; -/* for a dimm plugged_size == region_size */ - mdc->get_plugged_size = pc_dimm_md_get_region_size; - mdc->get_region_size = pc_dimm_md_get_region_size; + mdc->get_plugged_size = pc_dimm_md_get_plugged_size; + mdc->get_memory_region = pc_dimm_md_get_memory_region; mdc->fill_device_info = pc_dimm_md_fill_device_info; }include/hw/mem/memory-device.h +1
-1@@ -34,7 +34,7 @@ typedef struct MemoryDeviceClass {uint64_t (*get_addr)(const MemoryDeviceState *md);uint64_t (*get_plugged_size)(const MemoryDeviceState *md,Error **errp); -uint64_t (*get_region_size)(const MemoryDeviceState *md,Error **errp); + MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);void (*fill_device_info)(const MemoryDeviceState *md,MemoryDeviceInfo *info); } MemoryDeviceClass;
PATCH v2 6/20 memory-device: document MemoryDeviceClass
In reply to David HildenbrandDocument the functions and when to not expect errors.
--- include/hw/mem/memory-device.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
include/hw/mem/memory-device.h +13 -0
@@ -29,9 +29,22 @@ typedef struct MemoryDeviceState {Object parent_obj; } MemoryDeviceState; +/** +* MemoryDeviceClass: +* @get_addr: The address of the @md in guest physical memory. "0" means that +* no address has been specified by the user and that no address has been +* assigned yet. +* @get_plugged_size: The amount of memory provided by this @md currently +* usable ("plugged") by the guest. Will not fail after the device was realized. +* @get_memory_region: The memory region of the @md to mapped in guest +* physical memory at @get_addr. Will not fail after the device was realized. +* @fill_device_info: Fill out #MemoryDeviceInfo with @md specific information. +*/ typedef struct MemoryDeviceClass { + /* private */ InterfaceClass parent_class; + /* public */ uint64_t (*get_addr)(const MemoryDeviceState *md); uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp); MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 06/20] memory-device: document MemoryDeviceClass
Quoted 12 lines · On Wed, 29 Aug 2018 17:36:10 +0200 David Hildenbrand <[email protected]> wrote:
Document the functions and when to not expect errors.
Signed-off-by David Hildenbrand <[email protected]>--- include/hw/mem/memory-device.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)include/hw/mem/memory-device.h +0
-0
include/hw/mem/memory-device.h typedef struct MemoryDeviceState { · line 38
Quoted 10 lines
include/hw/mem/memory-device.h +7
-0@@ -29,9 +29,22 @@ typedef struct MemoryDeviceState {Object parent_obj; } MemoryDeviceState; +/** +* MemoryDeviceClass: +* @get_addr: The address of the @md in guest physical memory. "0" means that +* no address has been specified by the user and that no address has been +* assigned yet. +* @get_plugged_size: The amount of memory provided by this @md currently +* usable ("plugged") by the guest. Will not fail after the device was realized.
I'd drop 'Will not fail after the device was realized'
include/hw/mem/memory-device.h typedef struct MemoryDeviceState { · line 39
Quoted 1 line
include/hw/mem/memory-device.h +1
-0+* @get_memory_region: The memory region of the @md to mapped in guest
s/to mapped/that's mapped/
include/hw/mem/memory-device.h typedef struct MemoryDeviceState { · line 40
Quoted 1 line
include/hw/mem/memory-device.h +1
-0+* physical memory at @get_addr. Will not fail after the device was realized.
the same
include/hw/mem/memory-device.h typedef struct MemoryDeviceState { · line 41
Quoted 1 line
include/hw/mem/memory-device.h +1
-0+* @fill_device_info: Fill out #MemoryDeviceInfo with @md specific information.
maybe:
translate current @md state into #MemoryDeviceInfo
Quoted 9 lines
include/hw/mem/memory-device.h +3
-0+*/ typedef struct MemoryDeviceClass { + /* private */ InterfaceClass parent_class; + /* public */ uint64_t (*get_addr)(const MemoryDeviceState *md); uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp); MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
PATCH v2 7/20 memory-device: add device class function set_addr()
In reply to David HildenbrandTo be able to factor out address asignment of memory devices, we will have to read (get_addr()) and write (set_addr()) the address.
We can't use properties for this purpose, as properties are device specific. E.g. while the address property for a DIMM is called "addr", it might be called differently (e.g. "memaddr") for other devices.
Especially virtio based memory devices cannot use "addr" as that is already reserved and used for the address on the bus (for the proxy device).
Also, it might be possible to have memory devices without address properties (e.g. internal DIMM-like thingies).
In contrast to get_addr(), we expect that set_addr() can fail.
--- include/hw/mem/memory-device.h | 2 ++ 1 file changed, 2 insertions(+)
include/hw/mem/memory-device.h +2 -0
@@ -34,6 +34,7 @@ typedef struct MemoryDeviceState {* @get_addr: The address of the @md in guest physical memory. "0" means that * no address has been specified by the user and that no address has been * assigned yet. +* @set_addr: Set the address of the @md in guest physical memory. * @get_plugged_size: The amount of memory provided by this @md currently * usable ("plugged") by the guest. Will not fail after the device was realized. * @get_memory_region: The memory region of the @md to mapped in guest
@@ -46,6 +47,7 @@ typedef struct MemoryDeviceClass {/* public */ uint64_t (*get_addr)(const MemoryDeviceState *md); +void (*set_addr)(MemoryDeviceState *md,uint64_t addr,Error **errp);uint64_t (*get_plugged_size)(const MemoryDeviceState *md,Error **errp); MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp); void (*fill_device_info)(const MemoryDeviceState *md,
Signature · 1 line
2.17.1
PATCH v2 8/20 pc-dimm: implement memory device class function set_addr()
In reply to David HildenbrandKeep it simple for now and simply set the static property, that will fail once realized.
--- hw/mem/pc-dimm.c | 7 +++++++ 1 file changed, 7 insertions(+)
hw/mem/pc-dimm.c +7 -0
@@ -236,6 +236,12 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)return dimm->addr; } +static void pc_dimm_md_set_addr (MemoryDeviceState *md,uint64_t addr, +Error **errp) +{ +object_property_set_uint (OBJECT (md), addr,PC_DIMM_ADDR_PROP , errp); +} +static uint64_t pc_dimm_md_get_plugged_size (const MemoryDeviceState *md,Error **errp) {
@@ -307,6 +313,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data) ddc->get_vmstate_memory_region = pc_dimm_get_memory_region; mdc->get_addr = pc_dimm_md_get_addr; + mdc->set_addr = pc_dimm_md_set_addr; mdc->get_plugged_size = pc_dimm_md_get_plugged_size; mdc->get_memory_region = pc_dimm_md_get_memory_region; mdc->fill_device_info = pc_dimm_md_fill_device_info;
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 08/20] pc-dimm: implement memory device class function set_addr()
Quoted 2 lines · On Wed, 29 Aug 2018 17:36:12 +0200 David Hildenbrand <[email protected]> wrote:
Keep it simple for now and simply set the static property, that will fail once realized.
I'd merge this with previous patch and mention that set_addr will replace 'addr' property setting in the next patch where preliminary steps during memory hotplug are factored out into _preplug handler
Quoted 30 lines
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/pc-dimm.c | 7 +++++++ 1 file changed, 7 insertions(+)hw/mem/pc-dimm.c +7
-0@@ -236,6 +236,12 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)return dimm->addr; } +static void pc_dimm_md_set_addr (MemoryDeviceState *md,uint64_t addr, +Error **errp) +{ +object_property_set_uint (OBJECT (md), addr,PC_DIMM_ADDR_PROP , errp); +} +static uint64_t pc_dimm_md_get_plugged_size (const MemoryDeviceState *md,Error **errp) {@@ -307,6 +313,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data) ddc->get_vmstate_memory_region = pc_dimm_get_memory_region; mdc->get_addr = pc_dimm_md_get_addr; + mdc->set_addr = pc_dimm_md_set_addr; mdc->get_plugged_size = pc_dimm_md_get_plugged_size; mdc->get_memory_region = pc_dimm_md_get_memory_region; mdc->fill_device_info = pc_dimm_md_fill_device_info;
PATCH v2 9/20 memory-device: complete factoring out pre_plug handling
In reply to David HildenbrandWith all required memory device class functions in place, we can factor out pre_plug handling of memory devices. Take proper care of errors. We still have to carry along legacy_align required for pc compatibility handling.
--- hw/mem/memory-device.c | 32 +++++++++++++++++++++++++++++--- hw/mem/pc-dimm.c | 16 +++------------- include/hw/mem/memory-device.h | 5 ++--- 3 files changed, 34 insertions(+), 19 deletions(-)
hw/mem/memory-device.c +29 -3
@@ -96,9 +96,10 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size, } -uint64_t memory_device_get_free_addr (MachineState *ms,const uint64_t *hint, -uint64_t align,uint64_t size, -Error **errp) +static uint64_t memory_device_get_free_addr (MachineState *ms, +const uint64_t *hint, +uint64_t align,uint64_t size, +Error **errp) {uint64_t address_space_start, address_space_end;GSList *list =NULL , *item;
@@ -252,6 +253,31 @@ uint64_t get_plugged_memory_size(void)return size; } +void memory_device_pre_plug (MemoryDeviceState *md,MachineState *ms, +const uint64_t *legacy_align,Error **errp) +{ +const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (md); +Error *local_err =NULL ; +uint64_t addr, align; +MemoryRegion *mr; + + mr = mdc->get_memory_region (md, &local_err); +if (local_err) { + goto out; + } + + align = legacy_align ? *legacy_align :memory_region_get_alignment (mr); + addr = mdc->get_addr (md); + addr =memory_device_get_free_addr (ms, !addr ?NULL : &addr, align, +memory_region_size (mr), &local_err); +if (local_err) { + goto out; + } + mdc->set_addr (md, addr, &local_err); +out: +error_propagate (errp, local_err); +} +void memory_device_plug_region (MachineState *ms,MemoryRegion *mr,uint64_t addr) {
hw/mem/pc-dimm.c +3 -13
@@ -32,11 +32,8 @@ static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);void pc_dimm_pre_plug (DeviceState *dev,MachineState *machine,const uint64_t *legacy_align,Error **errp) { -PCDIMMDevice *dimm =PC_DIMM (dev); -PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (dimm);Error *local_err =NULL ; -MemoryRegion *mr; -uint64_t addr, align; +uint64_t addr;int slot; slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
@@ -49,22 +46,15 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,object_property_set_int (OBJECT (dev), slot,PC_DIMM_SLOT_PROP , &error_abort);trace_mhp_pc_dimm_assigned_slot (slot); - mr = ddc->get_memory_region (dimm, &local_err); +memory_device_pre_plug (MEMORY_DEVICE (dev), machine, legacy_align, + &local_err);if (local_err) { goto out; } - align = legacy_align ? *legacy_align :memory_region_get_alignment (mr); addr =object_property_get_uint (OBJECT (dev),PC_DIMM_ADDR_PROP , &error_abort); - addr =memory_device_get_free_addr (machine, !addr ?NULL : &addr, align, -memory_region_size (mr), &local_err); -if (local_err) { - goto out; - }trace_mhp_pc_dimm_assigned_address (addr); -object_property_set_uint (OBJECT (dev), addr,PC_DIMM_ADDR_PROP , - &error_abort); out:error_propagate (errp, local_err); }
include/hw/mem/memory-device.h +2 -3
@@ -56,9 +56,8 @@ typedef struct MemoryDeviceClass {MemoryDeviceInfoList *qmp_memory_device_list (void );uint64_t get_plugged_memory_size (void ); -uint64_t memory_device_get_free_addr (MachineState *ms,const uint64_t *hint, -uint64_t align,uint64_t size, -Error **errp); +void memory_device_pre_plug (MemoryDeviceState *md,MachineState *ms, +const uint64_t *legacy_align,Error **errp);void memory_device_plug_region (MachineState *ms,MemoryRegion *mr,uint64_t addr);void memory_device_unplug_region (MachineState *ms,MemoryRegion *mr);
Signature · 1 line
2.17.1
PATCH v2 10/20 memory-device: complete factoring out plug handling
In reply to David HildenbrandWith the new memory device functions in place, we can factor out plugging of memory devices completely.
--- hw/mem/memory-device.c | 10 +++++++--- hw/mem/pc-dimm.c | 7 +------ include/hw/mem/memory-device.h | 3 +-- 3 files changed, 9 insertions(+), 11 deletions(-)
hw/mem/memory-device.c +7 -3
@@ -278,10 +278,14 @@ out:error_propagate (errp, local_err); } -void memory_device_plug_region (MachineState *ms,MemoryRegion *mr, -uint64_t addr) +void memory_device_plug (MemoryDeviceState *md,MachineState *ms) { -/* we expect a previous call to memory_device_get_free_addr() */ +const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (md); +const uint64_t addr = mdc->get_addr (md); +MemoryRegion *mr; + +/* we expect a previous call to memory_device_pre_plug() */ + mr = mdc->get_memory_region (md, &error_abort);g_assert (ms->device_memory); memory_region_add_subregion(&ms->device_memory->mr,
hw/mem/pc-dimm.c +1 -6
@@ -65,13 +65,8 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp)PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (dimm);MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region (dimm, &error_abort); -MemoryRegion *mr = ddc->get_memory_region (dimm, &error_abort); -uint64_t addr; - - addr =object_property_get_uint (OBJECT (dev),PC_DIMM_ADDR_PROP , - &error_abort); -memory_device_plug_region (machine, mr, addr); +memory_device_plug (MEMORY_DEVICE (dev), machine);vmstate_register_ram (vmstate_mr, dev); }
include/hw/mem/memory-device.h +1 -2
@@ -58,8 +58,7 @@ MemoryDeviceInfoList *qmp_memory_device_list(void);uint64_t get_plugged_memory_size (void );void memory_device_pre_plug (MemoryDeviceState *md,MachineState *ms,const uint64_t *legacy_align,Error **errp); -void memory_device_plug_region (MachineState *ms,MemoryRegion *mr, -uint64_t addr); +void memory_device_plug (MemoryDeviceState *md,MachineState *ms);void memory_device_unplug_region (MachineState *ms,MemoryRegion *mr);#endif
Signature · 1 line
2.17.1
PATCH v2 11/20 memory-device: complete factoring out unplug handling
In reply to David HildenbrandWith the new memory device functions in place, we can factor out unplugging of memory devices completely.
--- hw/mem/memory-device.c | 8 ++++++-- hw/mem/pc-dimm.c | 3 +-- include/hw/mem/memory-device.h | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-)
hw/mem/memory-device.c +6 -2
@@ -292,9 +292,13 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms) addr - ms->device_memory->base, mr); } -void memory_device_unplug_region (MachineState *ms,MemoryRegion *mr) +void memory_device_unplug (MemoryDeviceState *md,MachineState *ms) { -/* we expect a previous call to memory_device_get_free_addr() */ +const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (md); +MemoryRegion *mr; + +/* we expect a previous call to memory_device_pre_plug() */ + mr = mdc->get_memory_region (md, &error_abort);g_assert (ms->device_memory);memory_region_del_subregion (&ms->device_memory->mr, mr);
hw/mem/pc-dimm.c +1 -2
@@ -76,9 +76,8 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine)PCDIMMDeviceClass *ddc =PC_DIMM_GET_CLASS (dimm);MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region (dimm, &error_abort); -MemoryRegion *mr = ddc->get_memory_region (dimm, &error_abort); -memory_device_unplug_region (machine, mr); +memory_device_unplug (MEMORY_DEVICE (dev), machine);vmstate_unregister_ram (vmstate_mr, dev); }
include/hw/mem/memory-device.h +1 -1
@@ -59,6 +59,6 @@ uint64_t get_plugged_memory_size(void);void memory_device_pre_plug (MemoryDeviceState *md,MachineState *ms,const uint64_t *legacy_align,Error **errp);void memory_device_plug (MemoryDeviceState *md,MachineState *ms); -void memory_device_unplug_region (MachineState *ms,MemoryRegion *mr); +void memory_device_unplug (MemoryDeviceState *md,MachineState *ms);#endif
Signature · 1 line
2.17.1
PATCH v2 12/20 memory-device: trace when pre_assigning/assigning/unassigning addresses
In reply to David HildenbrandLet's trace the address when pre_pluggin/plugging/unplugging a memory device.
Trace it when pre_plugging as well as when plugging, so we really know when a specific address is actually used.
--- hw/mem/memory-device.c | 6 ++++++ hw/mem/pc-dimm.c | 8 -------- hw/mem/trace-events | 5 ++++- 3 files changed, 10 insertions(+), 9 deletions(-)
hw/mem/memory-device.c +6 -0
@@ -17,6 +17,7 @@#include "qemu/range.h" #include "hw/virtio/vhost.h" #include "sysemu/kvm.h" +#include "trace.h" static gint memory_device_addr_sort (gconstpointer a,gconstpointer b) {
@@ -274,6 +275,9 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms, goto out; } mdc->set_addr (md, addr, &local_err); +if (!local_err) { +trace_memory_device_pre_assign_address (addr); + } out:error_propagate (errp, local_err); }
@@ -290,6 +294,7 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms)memory_region_add_subregion (&ms->device_memory->mr, addr - ms->device_memory->base, mr); +trace_memory_device_assign_address (addr); }void memory_device_unplug (MemoryDeviceState *md,MachineState *ms)
@@ -302,6 +307,7 @@ void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)g_assert (ms->device_memory);memory_region_del_subregion (&ms->device_memory->mr, mr); +trace_memory_device_unassign_address (mdc->get_addr (md)); }static const TypeInfo memory_device_info = {
hw/mem/pc-dimm.c +0 -8
@@ -33,7 +33,6 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, const uint64_t *legacy_align, Error **errp) { Error *local_err = NULL; -uint64_t addr;int slot; slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
@@ -48,13 +47,6 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,memory_device_pre_plug (MEMORY_DEVICE (dev), machine, legacy_align, &local_err); -if (local_err) { - goto out; - } - - addr =object_property_get_uint (OBJECT (dev),PC_DIMM_ADDR_PROP , - &error_abort); -trace_mhp_pc_dimm_assigned_address (addr); out:error_propagate (errp, local_err); }
hw/mem/trace-events +4 -1
@@ -2,4 +2,7 @@ # hw/mem/pc-dimm.c mhp_pc_dimm_assigned_slot(int slot) "%d" -mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64 +# hw/mem/memory-device.c +memory_device_pre_assign_address(uint64_t addr) "0x%"PRIx64 +memory_device_assign_address(uint64_t addr) "0x%"PRIx64 +memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
Signature · 1 line
2.17.1
PATCH v2 13/20 hw/acpi-build: only indicate nvdimm and pc-dimm
In reply to David HildenbrandOnce we have other memory devices that are not ACPI devices (e.g. virtio based), we cannot indicate them via ACPI. So let's skip these devices.
--- hw/i386/acpi-build.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
hw/i386/acpi-build.c +13 -2
@@ -2266,7 +2266,6 @@ static void build_srat_hotpluggable_memory(GArray *table_data, uint64_t base,for (cur = base, info = info_list; cur < end; cur += size, info = info->next) { - numamem =acpi_data_push (table_data,sizeof *numamem);if (!info) {/*
@@ -2278,19 +2277,30 @@ static void build_srat_hotpluggable_memory(GArray *table_data, uint64_t base,* Memory devices may override proximity set by this entry, * providing _PXM method if necessary. */ + numamem =acpi_data_push (table_data,sizeof *numamem);build_srat_memory (numamem, end -1 ,1 , default_node,MEM_AFFINITY_HOTPLUGGABLE |MEM_AFFINITY_ENABLED );break ; } mi = info->value; +if (mi->type !=MEMORY_DEVICE_INFO_KIND_DIMM && + mi->type !=MEMORY_DEVICE_INFO_KIND_NVDIMM ) { +/* +* Don't indicate memory devices that are not proper ACPI devices, +* merge them with the empty ranges. +*/ + size =0 ; +continue ; + } + is_nvdimm = (mi->type ==MEMORY_DEVICE_INFO_KIND_NVDIMM ); di = !is_nvdimm ? mi->u.dimm.data : mi->u.nvdimm.data;if (cur < di->addr) { + numamem =acpi_data_push (table_data,sizeof *numamem);build_srat_memory (numamem, cur, di->addr - cur, default_node,MEM_AFFINITY_HOTPLUGGABLE |MEM_AFFINITY_ENABLED ); - numamem =acpi_data_push (table_data,sizeof *numamem); } size = di->size;
@@ -2303,6 +2313,7 @@ static void build_srat_hotpluggable_memory(GArray *table_data, uint64_t base, flags |=MEM_AFFINITY_NON_VOLATILE ; } + numamem =acpi_data_push (table_data,sizeof *numamem);build_srat_memory (numamem, di->addr, size, di->node, flags); }
Signature · 1 line
2.17.1
PATCH v2 14/20 memory-device: ids of virtio based devices are special
In reply to David HildenbrandWhen reporting the id of virtio-based memory devices, we always have to take the one of the proxy device (parent).
Expose the function, so especially virtio-based memory devices can reuse the function when filling out the id in MemoryDeviceInfo.
Details:
When the user creates a virtio device (e.g. virtio-balloon-pci), two devices are actually created.
1. Virtio proxy device (e.g. TYPE_VIRTIO_BALLOON_PCI)
2. The "real" virtio device (e.g. TYPE_VIRTIO_BALLOON).
1. aliases all properties of 2, so 2. can be properly configured using 1.
1. gets the device ID set specified by the user. 2. gets no ID set.
If we want to make 2. a MemoryDevice but report errors/information to the user, we always have to report the id of 1. (because that's the device the user instantiated and configured).
--- hw/mem/memory-device.c | 21 +++++++++++++++++++-- include/hw/mem/memory-device.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-)
hw/mem/memory-device.c +19 -2
@@ -19,6 +19,22 @@#include "sysemu/kvm.h" #include "trace.h" +const char *memory_device_id (const MemoryDeviceState *md) +{ +Object *obj =OBJECT (md); + +/* always use the ID of the proxy device for virtio devices */ +if (object_dynamic_cast (obj,TYPE_VIRTIO_DEVICE )) { +if (obj->parent &&object_dynamic_cast (obj->parent,TYPE_DEVICE )) { +const DeviceState *parent_dev =DEVICE (obj->parent); + +return parent_dev->id; + } +return NULL ; + } +return DEVICE (md)->id; +} +static gint memory_device_addr_sort (gconstpointer a,gconstpointer b) {const MemoryDeviceState *md_a =MEMORY_DEVICE (a);
@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);
@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 ); goto out; } new_addr =QEMU_ALIGN_UP (md_addr + md_size, align);
include/hw/mem/memory-device.h +1 -0
@@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass { MemoryDeviceInfo *info); } MemoryDeviceClass; +const char *memory_device_id (const MemoryDeviceState *md);MemoryDeviceInfoList *qmp_memory_device_list (void );uint64_t get_plugged_memory_size (void ); void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 14 lines · * David Hildenbrand ([email protected]) wrote:
When reporting the id of virtio-based memory devices, we always have to take the one of the proxy device (parent).
Expose the function, so especially virtio-based memory devices can reuse the function when filling out the id in MemoryDeviceInfo.
Details:
When the user creates a virtio device (e.g. virtio-balloon-pci), two devices are actually created.
1. Virtio proxy device (e.g.
TYPE_VIRTIO_BALLOON_PCI)
2. The "real" virtio device (e.g.TYPE_VIRTIO_BALLOON).
Quoted 8 lines
1. aliases all properties of 2, so 2. can be properly configured using 1.
1. gets the device ID set specified by the user. 2. gets no ID set.If we want to make 2. a MemoryDevice but report errors/information to the user, we always have to report the id of 1. (because that's the device the user instantiated and configured).
Signed-off-by David Hildenbrand <[email protected]>
Quoted 66 lines
--- hw/mem/memory-device.c | 21 +++++++++++++++++++-- include/hw/mem/memory-device.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-)hw/mem/memory-device.c +19
-2@@ -19,6 +19,22 @@#include "sysemu/kvm.h" #include "trace.h" +const char *memory_device_id (const MemoryDeviceState *md) +{ +Object *obj =OBJECT (md); + +/* always use the ID of the proxy device for virtio devices */ +if (object_dynamic_cast (obj,TYPE_VIRTIO_DEVICE )) { +if (obj->parent &&object_dynamic_cast (obj->parent,TYPE_DEVICE )) { +const DeviceState *parent_dev =DEVICE (obj->parent); + +return parent_dev->id; + } +return NULL ; + } +return DEVICE (md)->id; +} +static gint memory_device_addr_sort (gconstpointer a,gconstpointer b) {const MemoryDeviceState *md_a =MEMORY_DEVICE (a);@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 ); goto out; } new_addr =QEMU_ALIGN_UP (md_addr + md_size, align);include/hw/mem/memory-device.h +1
-0@@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass { MemoryDeviceInfo *info); } MemoryDeviceClass; +const char *memory_device_id (const MemoryDeviceState *md);MemoryDeviceInfoList *qmp_memory_device_list (void );uint64_t get_plugged_memory_size (void ); void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,Signature · 1 line
2.17.1
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
In reply to David HildenbrandActually on second thoughts, a question:
Quoted 63 lines · * David Hildenbrand ([email protected]) wrote:
When reporting the id of virtio-based memory devices, we always have to take the one of the proxy device (parent).
Expose the function, so especially virtio-based memory devices can reuse the function when filling out the id in MemoryDeviceInfo.
Details:
When the user creates a virtio device (e.g. virtio-balloon-pci), two devices are actually created.
1. Virtio proxy device (e.g.
TYPE_VIRTIO_BALLOON_PCI)
2. The "real" virtio device (e.g.TYPE_VIRTIO_BALLOON).1. aliases all properties of 2, so 2. can be properly configured using 1.
1. gets the device ID set specified by the user. 2. gets no ID set.If we want to make 2. a MemoryDevice but report errors/information to the user, we always have to report the id of 1. (because that's the device the user instantiated and configured).
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 21 +++++++++++++++++++-- include/hw/mem/memory-device.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-)hw/mem/memory-device.c +17
-0@@ -19,6 +19,22 @@#include "sysemu/kvm.h" #include "trace.h" +const char *memory_device_id (const MemoryDeviceState *md) +{ +Object *obj =OBJECT (md); + +/* always use the ID of the proxy device for virtio devices */ +if (object_dynamic_cast (obj,TYPE_VIRTIO_DEVICE )) { +if (obj->parent &&object_dynamic_cast (obj->parent,TYPE_DEVICE )) { +const DeviceState *parent_dev =DEVICE (obj->parent); + +return parent_dev->id; + } +return NULL ; + } +return DEVICE (md)->id; +} +static gint memory_device_addr_sort (gconstpointer a,gconstpointer b) {const MemoryDeviceState *md_a =MEMORY_DEVICE (a);@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);
hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 199
Quoted 7 lines
hw/mem/memory-device.c +2
-2@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 );
What's that 'id ? id : 0' trick for?
Dave
Quoted 18 lines
hw/mem/memory-device.c +0
-0goto out; } new_addr =QEMU_ALIGN_UP (md_addr + md_size, align);include/hw/mem/memory-device.h +1
-0@@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass { MemoryDeviceInfo *info); } MemoryDeviceClass; +const char *memory_device_id (const MemoryDeviceState *md);MemoryDeviceInfoList *qmp_memory_device_list (void );uint64_t get_plugged_memory_size (void ); void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,Signature · 1 line
2.17.1
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 14 lines
Quoted 14 lines
static gint
memory_device_addr_sort(gconstpointer a, gconstpointer b){ const MemoryDeviceState *md_a = MEMORY_DEVICE(a);hw/mem/memory-device.c +1
-0@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
Quoted 8 lines
hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 199
Quoted 6 lines
hw/mem/memory-device.c +2
-2if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 );What's that 'id ? id : 0' trick for?
0 -> "", then it actually makes sense :)
I'll fix this up, thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 23 lines · * David Hildenbrand ([email protected]) wrote:
Quoted 14 lines
Quoted 14 lines
static gint
memory_device_addr_sort(gconstpointer a, gconstpointer b){ const MemoryDeviceState *md_a = MEMORY_DEVICE(a);hw/mem/memory-device.c +1
-0@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,Quoted 8 lines
hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 199
Quoted 6 lines
hw/mem/memory-device.c +2
-2if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 );What's that 'id ? id : 0' trick for?
Quoted 3 lines
0 -> "", then it actually makes sense :)
I'll fix this up, thanks!
Except that:
address range conflicts with ''
isn't very helpful.
Why would you get a NULL id ?
Dave
Quoted 6 lines
Signature · 5 lines
-- Thanks, David / dhildenb
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 26 lines · Dr. David Alan Gilbert
Quoted 25 lines · * David Hildenbrand ([email protected]) wrote:
Quoted 14 lines
Quoted 14 lines
static gint
memory_device_addr_sort(gconstpointer a, gconstpointer b){ const MemoryDeviceState *md_a = MEMORY_DEVICE(a);hw/mem/memory-device.c +1
-0@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,Quoted 8 lines
hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 199
Quoted 6 lines
hw/mem/memory-device.c +2
-2if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 );What's that 'id ? id : 0' trick for?
0 -> "", then it actually makes sense :)
Quoted 7 lines
Quoted 1 line
I'll fix this up, thanks!
Except that:
address range conflicts with ''
isn't very helpful.
Why would you get a NULL id ?
This is easy: don't specify an id for a memory device:
Unfortunately, if the user does not give ids to devices, there is no way of telling him what we are talking about.
qemu-system-x86_64 -machine pc -m 4G,maxmem=20G,slots=4 \ -object memory-backend-ram,id=mem0,size=4G \ -object memory-backend-ram,id=mem1,size=4G \ -device pc-dimm,memdev=mem0,addr=0x140000000 \ -device pc-dimm,memdev=mem1,addr=0x140000000
qemu-system-x86_64: -device pc-dimm,memdev=mem1,addr=0x140000000:
address range conflicts with '(null)'
(I thought providing NULL would lead to a crash, but it is actually handled properly)
So while not being able to indicate an id is not nice, I can simply forward the id directly here.
Thanks!
Quoted 12 lines
Dave
Quoted 6 lines
Signature · 5 lines
-- Thanks, David / dhildenb--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 46 lines · * David Hildenbrand ([email protected]) wrote:
Quoted 26 lines · Dr. David Alan Gilbert
Quoted 25 lines · * David Hildenbrand ([email protected]) wrote:
Quoted 14 lines
Quoted 14 lines
static gint
memory_device_addr_sort(gconstpointer a, gconstpointer b){ const MemoryDeviceState *md_a = MEMORY_DEVICE(a);hw/mem/memory-device.c +1
-0@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,Quoted 8 lines
hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 199
Quoted 6 lines
hw/mem/memory-device.c +2
-2if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 );What's that 'id ? id : 0' trick for?
0 -> "", then it actually makes sense :)
Quoted 7 lines
Quoted 1 line
I'll fix this up, thanks!
Except that:
address range conflicts with ''
isn't very helpful.
Why would you get a NULL id ?This is easy: don't specify an id for a memory device:
Unfortunately, if the user does not give ids to devices, there is no way of telling him what we are talking about.
qemu-system-x86_64 -machine pc -m 4G,maxmem=20G,slots=4 \ -object memory-backend-ram,id=mem0,size=4G \ -object memory-backend-ram,id=mem1,size=4G \ -device pc-dimm,memdev=mem0,addr=0x140000000 \ -device pc-dimm,memdev=mem1,addr=0x140000000
Quoted 8 lines
qemu-system-x86_64: -device pc-dimm,memdev=mem1,addr=0x140000000:
address range conflicts with '(null)'(I thought providing NULL would lead to a crash, but it is actually handled properly)
So while not being able to indicate an id is not nice, I can simply forward the id directly here.
OK, or use something like id ? id : "(unnamed)"
Dave
Quoted 6 lines
Thanks!
Quoted 4 lines
Dave
Quoted 1 line
Quoted 15 lines
Quoted 8 lines
Quoted 5 lines
Signature · 3 lines
Thanks, David / dhildenb--
Dr. David Alan Gilbert / [email protected] / Manchester, UKSignature · 3 lines
Thanks, David / dhildenb
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
In reply to David HildenbrandQuoted 10 lines · David Hildenbrand
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 199
Quoted 3 lines
hw/mem/memory-device.c +2
-1-error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 );What's that 'id ? id : 0' trick for?
0 -> "", then it actually makes sense :)
Quoted 5 lines
qemu-system-x86_64: -device pc-dimm,memdev=mem1,addr=0x140000000:
address range conflicts with '(null)'(I thought providing NULL would lead to a crash, but it is actually handled properly)
Well, glibc handles it. But POSIX says it is undefined, and there are other libc where it indeed crashes. It's better to pass an explicit non-null placeholder than to rely on glibc turning NULL into "(null)".
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
In reply to David HildenbrandQuoted 37 lines · David Hildenbrand
When reporting the id of virtio-based memory devices, we always have to take the one of the proxy device (parent).
Expose the function, so especially virtio-based memory devices can reuse the function when filling out the id in MemoryDeviceInfo.
Details:
When the user creates a virtio device (e.g. virtio-balloon-pci), two devices are actually created.
1. Virtio proxy device (e.g.
TYPE_VIRTIO_BALLOON_PCI)
2. The "real" virtio device (e.g.TYPE_VIRTIO_BALLOON).1. aliases all properties of 2, so 2. can be properly configured using 1.
1. gets the device ID set specified by the user. 2. gets no ID set.If we want to make 2. a MemoryDevice but report errors/information to the user, we always have to report the id of 1. (because that's the device the user instantiated and configured).
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 21 +++++++++++++++++++-- include/hw/mem/memory-device.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-)hw/mem/memory-device.c +2
-0@@ -19,6 +19,22 @@#include "sysemu/kvm.h" #include "trace.h" +const char *memory_device_id (const MemoryDeviceState *md) +{
hw/mem/memory-device.c line 33
Quoted 10 lines
hw/mem/memory-device.c +10
-0+Object *obj =OBJECT (md); + +/* always use the ID of the proxy device for virtio devices */ +if (object_dynamic_cast (obj,TYPE_VIRTIO_DEVICE )) { +if (obj->parent &&object_dynamic_cast (obj->parent,TYPE_DEVICE )) { +const DeviceState *parent_dev =DEVICE (obj->parent); + +return parent_dev->id; + } +return NULL ;
I don't like having virtio-specific code on memory-device.c. What about making it generic? Let device 2 register a read-only property for the user-visible ID, and make memory_device_id() use that property if it's present.
Quoted 42 lines
hw/mem/memory-device.c +7
-2+ } +return DEVICE (md)->id; +} +static gint memory_device_addr_sort (gconstpointer a,gconstpointer b) {const MemoryDeviceState *md_a =MEMORY_DEVICE (a);@@ -168,6 +184,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); +const char *id =memory_device_id (md);uint64_t md_size, md_addr; md_addr = mdc->get_addr (md);@@ -178,8 +195,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,if (ranges_overlap (md_addr, md_size, new_addr, size)) {if (hint) { -const DeviceState *d =DEVICE (md); -error_setg (errp,"address range conflicts with '%s'" , d->id); +error_setg (errp,"address range conflicts with '%s'" , + id ? id :0 ); goto out; } new_addr =QEMU_ALIGN_UP (md_addr + md_size, align);include/hw/mem/memory-device.h +1
-0@@ -54,6 +54,7 @@ typedef struct MemoryDeviceClass { MemoryDeviceInfo *info); } MemoryDeviceClass; +const char *memory_device_id (const MemoryDeviceState *md);MemoryDeviceInfoList *qmp_memory_device_list (void );uint64_t get_plugged_memory_size (void ); void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,Signature · 1 line
2.17.1
Signature · 1 line
Eduardo
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 45 lines · Eduardo Habkost
Quoted 44 lines · David Hildenbrand
When reporting the id of virtio-based memory devices, we always have to take the one of the proxy device (parent).
Expose the function, so especially virtio-based memory devices can reuse the function when filling out the id in MemoryDeviceInfo.
Details:
When the user creates a virtio device (e.g. virtio-balloon-pci), two devices are actually created.
1. Virtio proxy device (e.g.
TYPE_VIRTIO_BALLOON_PCI)
2. The "real" virtio device (e.g.TYPE_VIRTIO_BALLOON).1. aliases all properties of 2, so 2. can be properly configured using 1.
1. gets the device ID set specified by the user. 2. gets no ID set.If we want to make 2. a MemoryDevice but report errors/information to the user, we always have to report the id of 1. (because that's the device the user instantiated and configured).
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 21 +++++++++++++++++++-- include/hw/mem/memory-device.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-)hw/mem/memory-device.c +9
-0@@ -19,6 +19,22 @@#include "sysemu/kvm.h" #include "trace.h" +const char *memory_device_id (const MemoryDeviceState *md) +{ +Object *obj =OBJECT (md); + +/* always use the ID of the proxy device for virtio devices */ +if (object_dynamic_cast (obj,TYPE_VIRTIO_DEVICE )) { +if (obj->parent &&object_dynamic_cast (obj->parent,TYPE_DEVICE )) { +const DeviceState *parent_dev =DEVICE (obj->parent); +
Quoted 8 lines
hw/mem/memory-device.c line 33
Quoted 3 lines
hw/mem/memory-device.c +3
-0+return parent_dev->id; + } +return NULL ;I don't like having virtio-specific code on
memory-device.c. What about making it generic? Let device 2 register a read-only property for the user-visible ID, and makememory_device_id()use that property if it's present.
Valid point. Or avoid properties and add a function to the memory-device class?
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 55 lines · David Hildenbrand
Quoted 45 lines · Eduardo Habkost
Quoted 44 lines · David Hildenbrand
When reporting the id of virtio-based memory devices, we always have to take the one of the proxy device (parent).
Expose the function, so especially virtio-based memory devices can reuse the function when filling out the id in MemoryDeviceInfo.
Details:
When the user creates a virtio device (e.g. virtio-balloon-pci), two devices are actually created.
1. Virtio proxy device (e.g.
TYPE_VIRTIO_BALLOON_PCI)
2. The "real" virtio device (e.g.TYPE_VIRTIO_BALLOON).1. aliases all properties of 2, so 2. can be properly configured using 1.
1. gets the device ID set specified by the user. 2. gets no ID set.If we want to make 2. a MemoryDevice but report errors/information to the user, we always have to report the id of 1. (because that's the device the user instantiated and configured).
Signed-off-by David Hildenbrand <[email protected]>--- hw/mem/memory-device.c | 21 +++++++++++++++++++-- include/hw/mem/memory-device.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-)hw/mem/memory-device.c +9
-0@@ -19,6 +19,22 @@#include "sysemu/kvm.h" #include "trace.h" +const char *memory_device_id (const MemoryDeviceState *md) +{ +Object *obj =OBJECT (md); + +/* always use the ID of the proxy device for virtio devices */ +if (object_dynamic_cast (obj,TYPE_VIRTIO_DEVICE )) { +if (obj->parent &&object_dynamic_cast (obj->parent,TYPE_DEVICE )) { +const DeviceState *parent_dev =DEVICE (obj->parent); +Quoted 8 lines
hw/mem/memory-device.c line 33
Quoted 3 lines
hw/mem/memory-device.c +3
-0+return parent_dev->id; + } +return NULL ;I don't like having virtio-specific code on
memory-device.c. What about making it generic? Let device 2 register a read-only property for the user-visible ID, and makememory_device_id()use that property if it's present.
Quoted 2 lines
Valid point. Or avoid properties and add a function to the memory-device class?
That works too, and it was my first thought. But if you want a method whose only purpose is to return a single value without affecting object state, a QOM property seems like a perfect fit.
Either of those options would be good enough for me, though.
Signature · 1 line
Eduardo
Re: [Qemu-devel] [PATCH v2 14/20] memory-device: ids of virtio based devices are special
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
I don't like having virtio-specific code on
memory-device.c. What about making it generic? Let device 2 register a read-only property for the user-visible ID, and makememory_device_id()use that property if it's present.
Quoted 9 lines
Quoted 2 lines
Valid point. Or avoid properties and add a function to the memory-device class?
That works too, and it was my first thought. But if you want a method whose only purpose is to return a single value without affecting object state, a QOM property seems like a perfect fit.
Either of those options would be good enough for me, though.
The function would only have to be defined for those overwriting it. Will have a look. Thanks!
Signature · 3 lines
Thanks, David / dhildenb
PATCH v2 15/20 virtio-pmem: prototype
In reply to David HildenbrandFrom: Pankaj Gupta <[email protected]>
This is the current protoype of virtio-pmem. Support will require machine changes for the architectures that will support it, so it will not yet be compiled.
[ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
split up patches ]
--- hw/virtio/Makefile.objs | 1 + hw/virtio/virtio-pmem.c | 222 ++++++++++++++++++++ include/hw/virtio/virtio-pmem.h | 40 ++++ include/standard-headers/linux/virtio_ids.h | 1 + qapi/misc.json | 26 ++- 5 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 hw/virtio/virtio-pmem.c create mode 100644 include/hw/virtio/virtio-pmem.h
hw/virtio/Makefile.objs +1 -0
@@ -5,6 +5,7 @@ obj-y += virtio.o common-obj-$(CONFIG_VIRTIO_RNG) += virtio-rng.o common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o common-obj-$(CONFIG_VIRTIO_MMIO) += virtio-mmio.o +obj-$(call land,$(CONFIG_VIRTIO_PMEM),$(CONFIG_LINUX)) += virtio-pmem.o obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon.o obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += virtio-crypto-pci.o
hw/virtio/virtio-pmem.c new file +222 -0
222 lines not on this page · raw message
include/hw/virtio/virtio-pmem.h new file +40 -0
@@ -0,0 +1,40 @@ +/* +* Virtio pmem Device +* +* Copyright Red Hat, Inc. 2018 +* Copyright Pankaj Gupta <[email protected]> +* +* This work is licensed under the terms of the GNU GPL, version 2 or +* (at your option) any later version. See the COPYING file in the +* top-level directory. +*/ + +#ifndef QEMU_VIRTIO_PMEM_H +#define QEMU_VIRTIO_PMEM_H + +#include "hw/virtio/virtio.h" +#include "sysemu/hostmem.h" +#include "standard-headers/linux/virtio_ids.h" + +#define TYPE_VIRTIO_PMEM "virtio-pmem" + +#define VIRTIO_PMEM (obj) \ + OBJECT_CHECK(VirtIOPMEM, (obj), TYPE_VIRTIO_PMEM) + +#define VIRTIO_PMEM_ADDR_PROP "memaddr" +#define VIRTIO_PMEM_MEMDEV_PROP "memdev" + +/* VirtIOPMEM device structure */ +typedef struct VirtIOPMEM { +VirtIODevice parent_obj; + +VirtQueue *rq_vq; +uint64_t start; +HostMemoryBackend *memdev; +}VirtIOPMEM ; + +struct virtio_pmem_config { +uint64_t start; +uint64_t size; +}; +#endif
include/standard-headers/linux/virtio_ids.h +1 -0
@@ -43,5 +43,6 @@#define VIRTIO_ID_INPUT 18/* virtio input */ #define VIRTIO_ID_VSOCK 19/* virtio vsock transport */ #define VIRTIO_ID_CRYPTO 20/* virtio crypto */ +#define VIRTIO_ID_PMEM 25/* virtio pmem */ #endif /* _LINUX_VIRTIO_IDS_H */
qapi/misc.json +25 -1
@@ -2907,6 +2907,29 @@ } } +## +# @VirtioPMemDeviceInfo: +# +# VirtioPMem state information +# +# @id: device's ID +# +# @memaddr: physical address in memory, where device is mapped +# +# @size: size of memory that the device provides +# +# @memdev: memory backend linked with device +# +# Since: 3.1 +## +{ 'struct': 'VirtioPMemDeviceInfo', + 'data': { '*id': 'str', + 'memaddr': 'size', + 'size': 'size', + 'memdev': 'str' + } +} + ## # @MemoryDeviceInfo: #
@@ -2916,7 +2939,8 @@ ## { 'union': 'MemoryDeviceInfo', 'data': { 'dimm': 'PCDIMMDeviceInfo', - 'nvdimm': 'PCDIMMDeviceInfo' + 'nvdimm': 'PCDIMMDeviceInfo', + 'virtio-pmem': 'VirtioPMemDeviceInfo' } }
Signature · 1 line
2.17.1
PATCH v2 16/20 virtio-pci: proxy for virtio-pmem
In reply to David HildenbrandFrom: Pankaj Gupta <[email protected]>
We need a proxy device for virtio-pmem.
--- hw/virtio/virtio-pci.c | 41 +++++++++++++++++++++++++++++++++++++++++ hw/virtio/virtio-pci.h | 14 ++++++++++++++ include/hw/pci/pci.h | 1 + 3 files changed, 56 insertions(+)
hw/virtio/virtio-pci.c +41 -0
@@ -2521,6 +2521,46 @@ static const TypeInfo virtio_rng_pci_info = { .class_init = virtio_rng_pci_class_init, }; +/* virtio-pmem-pci */ + +static void virtio_pmem_pci_realize (VirtIOPCIProxy *vpci_dev,Error **errp) +{ +VirtIOPMEMPCI *vpmem =VIRTIO_PMEM_PCI (vpci_dev); +DeviceState *vdev =DEVICE (&vpmem->vdev); + +qdev_set_parent_bus (vdev,BUS (&vpci_dev->bus)); +object_property_set_bool (OBJECT (vdev), true,"realized" , errp); +} + +static void virtio_pmem_pci_class_init (ObjectClass *klass,void *data) +{ +DeviceClass *dc =DEVICE_CLASS (klass); +VirtioPCIClass *k =VIRTIO_PCI_CLASS (klass); +PCIDeviceClass *pcidev_k =PCI_DEVICE_CLASS (klass); + k->realize = virtio_pmem_pci_realize; +set_bit (DEVICE_CATEGORY_MISC , dc->categories); + pcidev_k->vendor_id =PCI_VENDOR_ID_REDHAT_QUMRANET ; + pcidev_k->device_id =PCI_DEVICE_ID_VIRTIO_PMEM ; + pcidev_k->revision =VIRTIO_PCI_ABI_VERSION ; + pcidev_k->class_id =PCI_CLASS_OTHERS ; +} + +static void virtio_pmem_pci_instance_init (Object *obj) +{ +VirtIOPMEMPCI *dev =VIRTIO_PMEM_PCI (obj); + +virtio_instance_init_common (obj, &dev->vdev,sizeof (dev->vdev), +TYPE_VIRTIO_PMEM ); +} + +static const TypeInfo virtio_pmem_pci_info = { + .name =TYPE_VIRTIO_PMEM_PCI , + .parent =TYPE_VIRTIO_PCI , + .instance_size =sizeof (VirtIOPMEMPCI), + .instance_init = virtio_pmem_pci_instance_init, + .class_init = virtio_pmem_pci_class_init, +}; +/* virtio-input-pci */ static Property virtio_input_pci_properties[] = {
@@ -2714,6 +2754,7 @@ static void virtio_pci_register_types(void)type_register_static (&virtio_balloon_pci_info);type_register_static (&virtio_serial_pci_info);type_register_static (&virtio_net_pci_info); +type_register_static (&virtio_pmem_pci_info);#ifdef CONFIG_VHOST_SCSI type_register_static (&vhost_scsi_pci_info);#endif
hw/virtio/virtio-pci.h +14 -0
@@ -19,6 +19,7 @@#include "hw/virtio/virtio-blk.h" #include "hw/virtio/virtio-net.h" #include "hw/virtio/virtio-rng.h" +#include "hw/virtio/virtio-pmem.h" #include "hw/virtio/virtio-serial.h" #include "hw/virtio/virtio-scsi.h" #include "hw/virtio/virtio-balloon.h"
@@ -57,6 +58,7 @@ typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;typedef struct VirtIOGPUPCI VirtIOGPUPCI ;typedef struct VHostVSockPCI VHostVSockPCI ;typedef struct VirtIOCryptoPCI VirtIOCryptoPCI ; +typedef struct VirtIOPMEMPCI VirtIOPMEMPCI ;/* virtio-pci-bus */
@@ -274,6 +276,18 @@ struct VirtIOBlkPCI {VirtIOBlock vdev; }; +/* +* virtio-pmem-pci: This extends VirtioPCIProxy. +*/ +#define TYPE_VIRTIO_PMEM_PCI "virtio-pmem-pci" +#define VIRTIO_PMEM_PCI (obj) \ + OBJECT_CHECK(VirtIOPMEMPCI, (obj), TYPE_VIRTIO_PMEM_PCI) + +struct VirtIOPMEMPCI { +VirtIOPCIProxy parent_obj; +VirtIOPMEM vdev; +}; +/* * virtio-balloon-pci: This extends VirtioPCIProxy. */
include/hw/pci/pci.h +1 -0
@@ -85,6 +85,7 @@ extern bool pci_available;#define PCI_DEVICE_ID_VIRTIO_RNG 0x1005#define PCI_DEVICE_ID_VIRTIO_9P 0x1009#define PCI_DEVICE_ID_VIRTIO_VSOCK 0x1012 +#define PCI_DEVICE_ID_VIRTIO_PMEM 0x1013#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001
Signature · 1 line
2.17.1
PATCH v2 17/20 hmp: handle virtio-pmem when printing memory device infos
In reply to David HildenbrandPrint the memory device info just like for PCDIMM/NVDIMM.
--- hmp.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
hmp.c +17 -12
@@ -2529,27 +2529,21 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)MemoryDeviceInfoList *info_list =qmp_query_memory_devices (&err);MemoryDeviceInfoList *info;MemoryDeviceInfo *value; -PCDIMMDeviceInfo *di; +VirtioPMemDeviceInfo *vpi;for (info = info_list; info; info = info->next) { +PCDIMMDeviceInfo *di =NULL ; + value = info->value;if (value) {switch (value->type) {case MEMORY_DEVICE_INFO_KIND_DIMM : di = value->u.dimm.data; -break ; -case MEMORY_DEVICE_INFO_KIND_NVDIMM : - di = value->u.nvdimm.data; -break ; - -default : - di =NULL ; -break ; - } - -if (di) { +if (!di) { + di = value->u.nvdimm.data; + }monitor_printf (mon,"Memory device [%s]: \"%s\"\n" ,MemoryDeviceInfoKind_str (value->type), di->id ? di->id :"" );
@@ -2562,6 +2556,17 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict) di->hotplugged ? "true" : "false");monitor_printf (mon," hotpluggable: %s\n" , di->hotpluggable ?"true" :"false" ); +break ; +case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM : + vpi = value->u.virtio_pmem.data; +monitor_printf (mon,"Memory device [%s]: \"%s\"\n" , +MemoryDeviceInfoKind_str (value->type), + vpi->id ? vpi->id :"" ); +monitor_printf (mon," memaddr: 0x%" PRIx64"\n" , vpi->memaddr); +monitor_printf (mon," size: %" PRIu64"\n" , vpi->size); +monitor_printf (mon," memdev: %s\n" , vpi->memdev); +default : +break ; } } }
Signature · 1 line
2.17.1
PATCH v2 18/20 numa: handle virtio-pmem in NUMA stats
In reply to David HildenbrandAccount the memory to node 0 for now. Once (if ever) virtio-pmem supports NUMA, we can account it to the right node.
--- numa.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
numa.c +13 -12
@@ -544,30 +544,31 @@ static void numa_stat_memory_devices(NumaNodeMem node_mem[]) {MemoryDeviceInfoList *info_list =qmp_memory_device_list ();MemoryDeviceInfoList *info; -PCDIMMDeviceInfo *pcdimm_info; +VirtioPMemDeviceInfo *vpi;for (info = info_list; info; info = info->next) {MemoryDeviceInfo *value = info->value; +PCDIMMDeviceInfo *pcdimm_info =NULL ;if (value) {switch (value->type) {case MEMORY_DEVICE_INFO_KIND_DIMM : pcdimm_info = value->u.dimm.data; -break ; -case MEMORY_DEVICE_INFO_KIND_NVDIMM : - pcdimm_info = value->u.nvdimm.data; -break ; - -default : - pcdimm_info =NULL ; -break ; - } - -if (pcdimm_info) { +if (!pcdimm_info) { + pcdimm_info = value->u.nvdimm.data; + } node_mem[pcdimm_info->node].node_mem += pcdimm_info->size; node_mem[pcdimm_info->node].node_plugged_mem += pcdimm_info->size; +break ; +case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM : + vpi = value->u.virtio_pmem.data; +/* TODO: once we support numa, assign to right node */ + node_mem[0 ].node_mem += vpi->size; + node_mem[0 ].node_plugged_mem += vpi->size; +default : +break ; } } }
Signature · 1 line
2.17.1
PATCH v2 19/20 virtio-pmem: hotplug support functions
In reply to David Hildenbrandvirtio-pmem devices will have to be hotplugged using the machine hotplug handler just like other memory devices. Therefore, all machines that want to support virtio-pmem will have to modify their machine hotplug handler.
virtio-pmem devices are realized when their parent proxy device (virtio-pmem-pci) is realized. Therefore, they are attached to a bus without a hotplug handler. This makes things a lot easier, because without a hotplug handler, we can directly get control over the device in the machine hotplug handler (otherwise we would have to overwrite control and pass control to the bus hotplug handler from the machine hotplug handler).
As we have to implement support for each machine we want to support, add a safety net ("pre_plugged") that catches if the pre_plug handler was not called - if trying to realize it with a machine that does not support it.
Otherwise creating and realizing virtio-pmem-pci along with virtio-pmem would work, however the memory-device part would not properly get hotplugged.
--- hw/virtio/virtio-pmem.c | 34 +++++++++++++++++++++++++++++++++ include/hw/virtio/virtio-pmem.h | 11 +++++++++++ 2 files changed, 45 insertions(+)
hw/virtio/virtio-pmem.c +34 -0
@@ -112,6 +112,12 @@ static void virtio_pmem_realize(DeviceState *dev, Error **errp)return ; } +/* pre_plug handler wasn't executed (e.g. from machine hotplug handler) */ +if (!pmem->pre_plugged) { +error_setg (errp,"virtio-pmem is not compatible with the machine" ); +return ; + } +host_memory_backend_set_mapped (pmem->memdev, true);virtio_init (vdev,TYPE_VIRTIO_PMEM ,VIRTIO_ID_PMEM ,sizeof (struct virtio_pmem_config ));
@@ -203,6 +209,34 @@ static void virtio_pmem_class_init(ObjectClass *klass, void *data) mdc->fill_device_info = virtio_pmem_md_fill_device_info; } +void virtio_pmem_pre_plug (VirtIOPMEM *pmem,MachineState *ms,Error **errp) +{ +/* +* The proxy device (e.g. virtio-pmem-pci) has an hotplug handler and +* will attach the virtio-pmem device to its bus (parent_bus). This +* device will realize the virtio-mem device from its realize function, +* therefore when it is hotplugged itself. The proxy device bus +* therefore has no hotplug handler and we don't have to forward any +* calls. +*/ +if (!DEVICE (pmem)->parent_bus || +DEVICE (pmem)->parent_bus->hotplug_handler) { +error_setg (errp,"virtio-pmem is not compatible with the proxy." ); + } +memory_device_pre_plug (MEMORY_DEVICE (pmem), ms,NULL , errp); + pmem->pre_plugged = true; +} + +void virtio_pmem_plug (VirtIOPMEM *pmem,MachineState *ms,Error **errp) +{ +memory_device_plug (MEMORY_DEVICE (pmem), ms); +} + +void virtio_pmem_unplug (VirtIOPMEM *pmem,MachineState *ms,Error **errp) +{ +memory_device_unplug (MEMORY_DEVICE (pmem), ms); +} +static TypeInfo virtio_pmem_info = { .name =TYPE_VIRTIO_PMEM , .parent =TYPE_VIRTIO_DEVICE ,
include/hw/virtio/virtio-pmem.h +11 -0
@@ -31,10 +31,21 @@ typedef struct VirtIOPMEM {VirtQueue *rq_vq;uint64_t start;HostMemoryBackend *memdev; + +/* +* Safety net to make sure we can catch trying to be realized on a +* machine that is not prepared to properly hotplug virtio-pmem from +* its machine hotplug handler. +*/ + bool pre_plugged; } VirtIOPMEM;struct virtio_pmem_config {uint64_t start;uint64_t size; }; + +void virtio_pmem_pre_plug (VirtIOPMEM *pmem,MachineState *ms,Error **errp); +void virtio_pmem_plug (VirtIOPMEM *pmem,MachineState *ms,Error **errp); +void virtio_pmem_unplug (VirtIOPMEM *pmem,MachineState *ms,Error **errp);#endif
Signature · 1 line
2.17.1
PATCH v2 20/20 pc: support for virtio-pmem
In reply to David HildenbrandCompile virtio-pmem for x86 and properly hotplug virtio-pmem devices (memory-device part) from our pc machine hotplug handler.
--- default-configs/i386-softmmu.mak | 1 + hw/i386/pc.c | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-)
default-configs/i386-softmmu.mak +1 -0
@@ -51,6 +51,7 @@ CONFIG_APIC=y CONFIG_IOAPIC=y CONFIG_PVPANIC=y CONFIG_MEM_DEVICE=y +CONFIG_VIRTIO_PMEM=y CONFIG_DIMM=y CONFIG_NVDIMM=y CONFIG_ACPI_NVDIMM=y
hw/i386/pc.c +9 -1
@@ -74,6 +74,7 @@#include "hw/nmi.h" #include "hw/i386/intel_iommu.h" #include "hw/net/ne2000-isa.h" +#include "hw/virtio/virtio-pmem.h" /* debug PC/ISA interrupts */ //#define DEBUG_IRQ
@@ -2013,6 +2014,8 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,pc_memory_pre_plug (hotplug_dev, dev, errp); }else if (object_dynamic_cast (OBJECT (dev),TYPE_CPU )) {pc_cpu_pre_plug (hotplug_dev, dev, errp); + }else if (object_dynamic_cast (OBJECT (dev),TYPE_VIRTIO_PMEM )) { +virtio_pmem_pre_plug (VIRTIO_PMEM (dev),MACHINE (hotplug_dev), errp); } }
@@ -2023,6 +2026,8 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,pc_memory_plug (hotplug_dev, dev, errp); }else if (object_dynamic_cast (OBJECT (dev),TYPE_CPU )) {pc_cpu_plug (hotplug_dev, dev, errp); + }else if (object_dynamic_cast (OBJECT (dev),TYPE_VIRTIO_PMEM )) { +virtio_pmem_plug (VIRTIO_PMEM (dev),MACHINE (hotplug_dev), errp); } }
@@ -2046,6 +2051,8 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,pc_memory_unplug (hotplug_dev, dev, errp); }else if (object_dynamic_cast (OBJECT (dev),TYPE_CPU )) {pc_cpu_unplug_cb (hotplug_dev, dev, errp); + }else if (object_dynamic_cast (OBJECT (dev),TYPE_VIRTIO_PMEM )) { +virtio_pmem_unplug (VIRTIO_PMEM (dev),MACHINE (hotplug_dev), errp); }else {error_setg (errp,"acpi: device unplug for not supported device" " type: %s" ,object_get_typename (OBJECT (dev)));
@@ -2056,7 +2063,8 @@ static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, DeviceState *dev) {if (object_dynamic_cast (OBJECT (dev),TYPE_PC_DIMM ) || -object_dynamic_cast (OBJECT (dev),TYPE_CPU )) { +object_dynamic_cast (OBJECT (dev),TYPE_CPU ) || +object_dynamic_cast (OBJECT (dev),TYPE_VIRTIO_PMEM )) {return HOTPLUG_HANDLER (machine); }
Signature · 1 line
2.17.1