With the current control flow the post parse callback returned success right away for fully virtualized VMs. To allow adding additional checks into the post parse callback tweak the conditions so that the function doesn't return early except for error cases. To clarify the original piece of code borrow the wording from the commit message for the patch that introduced the code. --- This patch is new in the series. src/libxl/libxl_domain.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 407a9bd..611ccf4 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -546,10 +546,9 @@ libxlDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - if (STREQ(def->os.type, "hvm")) - return 0; - - if (def->nconsoles == 0) { + /* Xen PV domains always have a PV console, so add one to the domain config + * via post-parse callback if not explicitly specified in the XML. */ + if (STRNEQ(def->os.type, "hvm") && def->nconsoles == 0) { virDomainChrDefPtr chrdef; if (!(chrdef = virDomainChrDefNew())) -- 2.2.2

To enable memory hotplug the maximum memory size and slot count need to be specified. As qemu supports now other units than mebibytes when specifying memory, use the new interface in this case. --- Notes: Version 2: - simplified condition for enabling the max_memory formatting - tweaked test code to use the NONE macro instead of 0 docs/formatdomain.html.in | 2 +- src/qemu/qemu_command.c | 34 ++++++++++++++++++---- src/qemu/qemu_domain.c | 8 ++--- .../qemuxml2argv-memory-hotplug-nonuma.xml | 22 ++++++++++++++ .../qemuxml2argv-memory-hotplug.args | 6 ++++ .../qemuxml2argv-memory-hotplug.xml | 34 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c | 3 ++ 8 files changed, 102 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nonuma.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 57caef0..c167e1c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -725,7 +725,7 @@ Note that due to alignment of the memory chunks added via memory hotplug the full size allocation specified by this element may be impossible to achieve. - <span class='since'>Since 1.2.14</span> + <span class='since'>Since 1.2.14 supported by the QEMU driver.</span> </dd> <dt><code>currentMemory</code></dt> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 02105c3..ef8feeb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8489,13 +8489,35 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuDomainAlignMemorySizes(def) < 0) goto error; - /* Set '-m MB' based on maxmem, because the lower 'memory' limit - * is set post-startup using the balloon driver. If balloon driver - * is not supported, then they're out of luck anyway. Update the - * XML to reflect our rounding. - */ virCommandAddArg(cmd, "-m"); - virCommandAddArgFormat(cmd, "%llu", virDomainDefGetMemoryInitial(def) / 1024); + + if (def->mem.max_memory) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("memory hotplug isn't supported by this QEMU binary")); + goto error; + } + + /* due to guest support, qemu would silently enable NUMA with one node + * once the memory hotplug backend is enabled. To avoid possible + * confusion we will enforce user originated numa configuration along + * with memory hotplug. */ + if (virDomainNumaGetNodeCount(def->numa) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("At least one numa node has to be configured when " + "enabling memory hotplug")); + goto error; + } + + /* Use the 'k' suffix to let qemu handle the units */ + virCommandAddArgFormat(cmd, "size=%lluk,slots=%u,maxmem=%lluk", + virDomainDefGetMemoryInitial(def), + def->mem.memory_slots, + def->mem.max_memory); + + } else { + virCommandAddArgFormat(cmd, "%llu", virDomainDefGetMemoryInitial(def) / 1024); + } if (def->mem.nhugepages && !virDomainNumaGetNodeCount(def->numa)) { const long system_page_size = virGetSystemPageSizeKB(); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6484bb5..3223752 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1052,10 +1052,6 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_INPUT_BUS_USB) < 0) return -1; - /* memory hotplug tunables are not supported by this driver */ - if (virDomainDefCheckUnsupportedMemoryHotplug(def) < 0) - return -1; - return 0; } @@ -2898,5 +2894,9 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) mem = virDomainDefGetMemoryInitial(def); virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, 1024)); + /* Align maximum memory size. QEMU requires rounding to next 4KiB block. + * We'll take the "traditional" path and round it to 1MiB*/ + def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, 1024); + return 0; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nonuma.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nonuma.xml new file mode 100644 index 0000000..5c807ed --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nonuma.xml @@ -0,0 +1,22 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='9' unit='KiB'>1233456789</maxMemory> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.args new file mode 100644 index 0000000..6c26586 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.args @@ -0,0 +1,6 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc \ +-m size=219136k,slots=16,maxmem=1099511627776k \ +-smp 2 -numa node,nodeid=0,cpus=0-1,mem=214 \ +-nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-usb -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.xml new file mode 100644 index 0000000..567a662 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='2' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='219136' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index fcf5218..387b349 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1543,6 +1543,10 @@ mymain(void) DO_TEST_PARSE_ERROR("shmem-msi-only", NONE); DO_TEST("cpu-host-passthrough-features", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST); + DO_TEST_FAILURE("memory-hotplug-nonuma", QEMU_CAPS_DEVICE_PC_DIMM); + DO_TEST_FAILURE("memory-hotplug", NONE); + DO_TEST("memory-hotplug", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA); + virObjectUnref(driver.config); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7755ea3..ed462b8 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -427,6 +427,9 @@ mymain(void) DO_TEST("smbios"); DO_TEST("aarch64-aavmf-virtio-mmio"); + DO_TEST("memory-hotplug"); + DO_TEST("memory-hotplug-nonuma"); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); -- 2.2.2

This patch adds code that parses and formats configuration for memory devices. A simple configuration would be: <memory model='dimm'> <target> <size unit='KiB'>524287</size> <node>0</node> </target> </memory> A complete configuration of a memory device: <memory model='dimm'> <source> <pagesize unit='KiB'>4096</pagesize> <nodemask>1-3</nodemask> </source> <target> <size unit='KiB'>524287</size> <node>1</node> </target> </memory> This patch preemptively forbids use of the <memory> device in individual drivers so the users are warned right away that the device is not supported. --- Notes: Version 3: - target node is stored now as unsigned and all uses are tweaked - fixed documentation wording as suggested - added documentation for behavior of <memory> element once memory devices are used Version 2: - dropped the ACPI prefix - fixed memory leak in the free function - tweaked docs Version 3: - target node is stored now as unsigned and all uses are tweaked - fixed documentation wording as suggested Version 2: - dropped the ACPI prefix - fixed memory leak in the free function - tweaked docs Version 3: - target node is stored now as unsigned and all uses are tweaked - fixed documentation wording as suggested Version 2: - dropped the ACPI prefix - fixed memory leak in the free function - tweaked docs Version 2: - dropped the ACPI prefix - fixed memory leak in the free function - tweaked docs docs/formatdomain.html.in | 92 +++++- docs/schemas/domaincommon.rng | 50 +++ src/bhyve/bhyve_domain.c | 5 +- src/conf/domain_conf.c | 336 ++++++++++++++++++++- src/conf/domain_conf.h | 33 ++ src/libvirt_private.syms | 2 + src/libxl/libxl_domain.c | 3 + src/lxc/lxc_domain.c | 4 + src/openvz/openvz_driver.c | 3 + src/qemu/qemu_domain.c | 3 + src/qemu/qemu_driver.c | 13 + src/qemu/qemu_hotplug.c | 3 + src/uml/uml_driver.c | 3 + src/xen/xen_driver.c | 3 + src/xenapi/xenapi_driver.c | 3 + .../qemuxml2argv-memory-hotplug-dimm.xml | 50 +++ 16 files changed, 601 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c167e1c..e1aa79c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -684,7 +684,9 @@ <dl> <dt><code>memory</code></dt> - <dd>The maximum allocation of memory for the guest at boot time. + <dd>The maximum allocation of memory for the guest at boot time. The + memory allocation includes possible additional memory devices specified + at start or hotplugged later. The units for this value are determined by the optional attribute <code>unit</code>, which defaults to "KiB" (kibibytes, 2¹⁰ or blocks of 1024 bytes). Valid @@ -702,6 +704,9 @@ supported by the hypervisor. Some hypervisors also enforce a minimum, such as 4000KiB. + In case <a href="#elementsCPU">NUMA</a> is configured for the guest the + <code>memory</code> element can be omitted. + In the case of crash, optional attribute <code>dumpCore</code> can be used to control whether the guest memory should be included in the generated coredump or not (values "on", "off"). @@ -5909,6 +5914,91 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> + <h4><a name="elementsMemory">Memory devices</a></h4> + + <p> + In addition to the initial memory assigned to the guest, memory devices + allow additional memory to be assigned to the guest in the form of + memory modules. + + A memory device can be hot-plugged or hot-unplugged depending on the + guests' memory resource needs. + + Some hypervisors may require NUMA configured for the guest. + <span class="since">Since 1.2.14</span> + </p> + + <p> + Example: usage of the memory devices + </p> +<pre> + ... + &lt;devices&gt; + &lt;memory model='dimm'&gt; + &lt;target&gt; + &lt;size unit='KiB'&gt;524287&lt;/size&gt; + &lt;node&gt;0&lt;/node&gt; + &lt;/target&gt; + &lt;/memory&gt; + &lt;memory model='dimm'&gt; + &lt;source&gt; + &lt;pagesize unit='KiB'&gt;4096&lt;/pagesize&gt; + &lt;nodemask&gt;1-3&lt;/nodemask&gt; + &lt;/source&gt; + &lt;target&gt; + &lt;size unit='KiB'&gt;524287&lt;/size&gt; + &lt;node&gt;1&lt;/node&gt; + &lt;/target&gt; + &lt;/memory&gt; + &lt;/devices&gt; + ... +</pre> + <dl> + <dt><code>model</code></dt> + <dd> + <p> + Currently only the <code>dimm</code> model is supported in order to + add a virtual DIMM module to the guest. + </p> + </dd> + + <dt><code>source</code></dt> + <dd> + <p> + The optional source element allows to fine tune the source of the + memory used for the given memory device. If the element is not + provided defaults configured via <code>numatune</code> are used. + </p> + <p> + <code>pagesize</code> can optionally be used to override the default + host page size used for backing the memory device. + + The configured value must correspond to a page size supported by the + host. + </p> + <p> + <code>nodemask</code> can optionally be used to override the default + set of NUMA nodes where the memory would be allocated. + </p> + </dd> + + <dt><code>target</code></dt> + <dd> + <p> + The mandatory <code>target</code> element configures the placement and + sizing of the added memory from the perspective of the guest. + </p> + <p> + The mandatory <code>size</code> subelement configures the size of the + added memory as a scaled integer. + </p> + <p> + The mandatory <code>node</code> subelement configures the guest NUMA + node to attach the memory to. + </p> + </dd> + </dl> + <h3><a name="seclabel">Security label</a></h3> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1f4df8e..e66b467 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4071,6 +4071,7 @@ <ref name="rng"/> <ref name="tpm"/> <ref name="shmem"/> + <ref name="memorydev"/> </choice> </zeroOrMore> <optional> @@ -4487,6 +4488,55 @@ </element> </define> + <define name="memorydev"> + <element name="memory"> + <attribute name="model"> + <choice> + <value>dimm</value> + </choice> + </attribute> + <interleave> + <optional> + <ref name="memorydev-source"/> + </optional> + <ref name="memorydev-target"/> + <optional> + <ref name="address"/> + </optional> + </interleave> + </element> + </define> + + <define name="memorydev-source"> + <element name="source"> + <interleave> + <optional> + <element name="pagesize"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="nodemask"> + <ref name="cpuset"/> + </element> + </optional> + </interleave> + </element> + </define> + + <define name="memorydev-target"> + <element name="target"> + <interleave> + <element name="size"> + <ref name="scaledInteger"/> + </element> + <element name="node"> + <ref name="unsignedInt"/> + </element> + </interleave> + </element> + </define> + <define name="rng"> <element name="rng"> <attribute name="model"> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 25ef852..890963e 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -75,11 +75,14 @@ bhyveDomainDefPostParse(virDomainDefPtr def, } static int -bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, +bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + return 0; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2d765ba..8c2234f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -236,7 +236,8 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "rng", "shmem", "tpm", - "panic") + "panic", + "memory") VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "none", @@ -779,6 +780,9 @@ VIR_ENUM_DECL(virDomainBlockJob) VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, "", "", "copy", "", "active-commit") +VIR_ENUM_IMPL(virDomainMemoryModel, VIR_DOMAIN_MEMORY_MODEL_LAST, + "", "dimm") + static virClassPtr virDomainObjClass; static virClassPtr virDomainObjListClass; static virClassPtr virDomainXMLOptionClass; @@ -1003,6 +1007,27 @@ virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def) } +/** + * virDomainDeviceDefCheckUnsupportedMemoryDevice: + * @dev: device definition + * + * Returns -1 if the device definition describes a memory device and reports an + * error. Otherwise returns 0. + */ +int +virDomainDeviceDefCheckUnsupportedMemoryDevice(virDomainDeviceDefPtr dev) +{ + /* This driver doesn't yet know how to handle memory devices */ + if (dev->type == VIR_DOMAIN_DEVICE_MEMORY) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("memory devices are not supported by this driver")); + return -1; + } + + return 0; +} + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -1936,6 +1961,16 @@ void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def) VIR_FREE(def); } +void virDomainMemoryDefFree(virDomainMemoryDefPtr def) +{ + if (!def) + return; + + virBitmapFree(def->sourceNodes); + virDomainDeviceInfoClear(&def->info); + VIR_FREE(def); +} + void virDomainDeviceDefFree(virDomainDeviceDefPtr def) { if (!def) @@ -2005,6 +2040,9 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_PANIC: virDomainPanicDefFree(def->data.panic); break; + case VIR_DOMAIN_DEVICE_MEMORY: + virDomainMemoryDefFree(def->data.memory); + break; case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_NONE: break; @@ -2202,6 +2240,10 @@ void virDomainDefFree(virDomainDefPtr def) virDomainRNGDefFree(def->rngs[i]); VIR_FREE(def->rngs); + for (i = 0; i < def->nmems; i++) + virDomainMemoryDefFree(def->mems[i]); + VIR_FREE(def->mems); + virDomainTPMDefFree(def->tpm); virDomainPanicDefFree(def->panic); @@ -2743,6 +2785,8 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device) return &device->data.tpm->info; case VIR_DOMAIN_DEVICE_PANIC: return &device->data.panic->info; + case VIR_DOMAIN_DEVICE_MEMORY: + return &device->data.memory->info; /* The following devices do not contain virDomainDeviceInfo */ case VIR_DOMAIN_DEVICE_LEASE: @@ -3063,6 +3107,13 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return -1; } + device.type = VIR_DOMAIN_DEVICE_MEMORY; + for (i = 0; i < def->nmems; i++) { + device.data.memory = def->mems[i]; + if (cb(def, &device, &def->mems[i]->info, opaque) < 0) + return -1; + } + /* Coverity is not very happy with this - all dead_error_condition */ #if !STATIC_ANALYSIS /* This switch statement is here to trigger compiler warning when adding @@ -3095,6 +3146,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_MEMORY: break; } #endif @@ -7082,10 +7134,16 @@ unsigned long long virDomainDefGetMemoryInitial(virDomainDefPtr def) { unsigned long long ret; + size_t i; /* return NUMA memory size total in case numa is enabled */ - if ((ret = virDomainNumaGetMemorySize(def->numa)) > 0) + if ((ret = virDomainNumaGetMemorySize(def->numa)) > 0) { return ret; + } else { + ret = def->mem.max_balloon; + for (i = 0; i < def->nmems; i++) + ret -= def->mems[i]->size; + } return def->mem.max_balloon; } @@ -7117,7 +7175,17 @@ virDomainDefSetMemoryInitial(virDomainDefPtr def, unsigned long long virDomainDefGetMemoryActual(virDomainDefPtr def) { - return virDomainDefGetMemoryInitial(def); + unsigned long long ret; + size_t i; + + if ((ret = virDomainNumaGetMemorySize(def->numa)) > 0) { + for (i = 0; i < def->nmems; i++) + ret += def->mems[i]->size; + } else { + ret = def->mem.max_balloon; + } + + return ret; } @@ -11476,6 +11544,119 @@ virDomainPMStateParseXML(xmlXPathContextPtr ctxt, return ret; } + +static int +virDomainMemorySourceDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainMemoryDefPtr def) +{ + int ret = -1; + char *nodemask = NULL; + xmlNodePtr save = ctxt->node; + ctxt->node = node; + + if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt, + &def->pagesize, false, false) < 0) + goto cleanup; + + if ((nodemask = virXPathString("string(./nodemask)", ctxt))) { + if (virBitmapParse(nodemask, 0, &def->sourceNodes, + VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(nodemask); + ctxt->node = save; + return ret; +} + + +static int +virDomainMemoryTargetDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainMemoryDefPtr def) +{ + int ret = -1; + xmlNodePtr save = ctxt->node; + ctxt->node = node; + + if (virXPathUInt("string(./node)", ctxt, &def->targetNode) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid or missing value of memory device node")); + goto cleanup; + } + + if (virDomainParseMemory("./size", "./size/@unit", ctxt, + &def->size, true, false) < 0) + goto cleanup; + + ret = 0; + + cleanup: + ctxt->node = save; + return ret; +} + + +static virDomainMemoryDefPtr +virDomainMemoryDefParseXML(xmlNodePtr memdevNode, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + char *tmp = NULL; + xmlNodePtr save = ctxt->node; + xmlNodePtr node; + virDomainMemoryDefPtr def; + + ctxt->node = memdevNode; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if (!(tmp = virXMLPropString(memdevNode, "model"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing memory model")); + goto error; + } + + if ((def->model = virDomainMemoryModelTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid memory model '%s'"), tmp); + goto error; + } + VIR_FREE(tmp); + + /* source */ + if ((node = virXPathNode("./source", ctxt)) && + virDomainMemorySourceDefParseXML(node, ctxt, def) < 0) + goto error; + + /* target */ + if (!(node = virXPathNode("./target", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing <target> element for <memory> device")); + goto error; + } + + if (virDomainMemoryTargetDefParseXML(node, ctxt, def) < 0) + goto error; + + if (virDomainDeviceInfoParseXML(memdevNode, NULL, &def->info, flags) < 0) + goto error; + + return def; + + error: + VIR_FREE(tmp); + virDomainMemoryDefFree(def); + ctxt->node = save; + return NULL; +} + + virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, const virDomainDef *def, @@ -11610,6 +11791,10 @@ virDomainDeviceDefParse(const char *xmlStr, if (!(dev->data.panic = virDomainPanicDefParseXML(node))) goto error; break; + case VIR_DOMAIN_DEVICE_MEMORY: + if (!(dev->data.memory = virDomainMemoryDefParseXML(node, ctxt, flags))) + goto error; + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: break; @@ -15009,6 +15194,23 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt->node = node; VIR_FREE(nodes); + /* analysis of memory devices */ + if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0) + goto error; + if (n && VIR_ALLOC_N(def->mems, n) < 0) + goto error; + + for (i = 0; i < n; i++) { + virDomainMemoryDefPtr mem = virDomainMemoryDefParseXML(nodes[i], + ctxt, + flags); + if (!mem) + goto error; + + def->mems[def->nmems++] = mem; + } + VIR_FREE(nodes); + /* analysis of the user namespace mapping */ if ((n = virXPathNodeSet("./idmap/uid", ctxt, &nodes)) < 0) goto error; @@ -16248,6 +16450,39 @@ virDomainPanicDefCheckABIStability(virDomainPanicDefPtr src, return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); } +static bool +virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, + virDomainMemoryDefPtr dst) +{ + if (src->model != dst->model) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target memory device model '%s' " + "doesn't match source model '%s'"), + virDomainMemoryModelTypeToString(dst->model), + virDomainMemoryModelTypeToString(src->model)); + return false; + } + + if (src->targetNode != dst->targetNode) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target memory device targetNode '%u' " + "doesn't match source targetNode '%u'"), + dst->targetNode, src->targetNode); + return false; + } + + if (src->size != dst->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target memory device size '%llu' doesn't match " + "source memory device size '%llu'"), + dst->size, src->size); + return false; + } + + return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); +} + + /* This compares two configurations and looks for any differences * which will affect the guest ABI. This is primarily to allow * validation of custom XML config passed in during migration @@ -16666,6 +16901,18 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; } + if (src->nmems != dst->nmems) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain memory device count %zu " + "does not match source %zu"), dst->nmems, src->nmems); + goto error; + } + + for (i = 0; i < src->nmems; i++) { + if (!virDomainMemoryDefCheckABIStability(src->mems[i], dst->mems[i])) + goto error; + } + /* Coverity is not very happy with this - all dead_error_condition */ #if !STATIC_ANALYSIS /* This switch statement is here to trigger compiler warning when adding @@ -16697,6 +16944,7 @@ virDomainDefCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_MEMORY: break; } #endif @@ -19184,6 +19432,81 @@ virDomainRNGDefFree(virDomainRNGDefPtr def) VIR_FREE(def); } + +static int +virDomainMemorySourceDefFormat(virBufferPtr buf, + virDomainMemoryDefPtr def) +{ + char *bitmap = NULL; + int ret = -1; + + if (!def->pagesize && !def->sourceNodes) + return 0; + + virBufferAddLit(buf, "<source>\n"); + virBufferAdjustIndent(buf, 2); + + if (def->sourceNodes) { + if (!(bitmap = virBitmapFormat(def->sourceNodes))) + goto cleanup; + + virBufferAsprintf(buf, "<nodemask>%s</nodemask>\n", bitmap); + } + + if (def->pagesize) + virBufferAsprintf(buf, "<pagesize unit='KiB'>%llu</pagesize>\n", + def->pagesize); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</source>\n"); + + ret = 0; + + cleanup: + VIR_FREE(bitmap); + return ret; +} + + +static void +virDomainMemoryTargetDefFormat(virBufferPtr buf, + virDomainMemoryDefPtr def) +{ + virBufferAddLit(buf, "<target>\n"); + virBufferAdjustIndent(buf, 2); + + virBufferAsprintf(buf, "<size unit='KiB'>%llu</size>\n", def->size); + virBufferAsprintf(buf, "<node>%u</node>\n", def->targetNode); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</target>\n"); +} + +static int +virDomainMemoryDefFormat(virBufferPtr buf, + virDomainMemoryDefPtr def, + unsigned int flags) +{ + const char *model = virDomainMemoryModelTypeToString(def->model); + + virBufferAsprintf(buf, "<memory model='%s'>\n", model); + virBufferAdjustIndent(buf, 2); + + if (virDomainMemorySourceDefFormat(buf, def) < 0) + return -1; + + virDomainMemoryTargetDefFormat(buf, def); + + if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</memory>\n"); + return 0; +} + static void virDomainVideoAccelDefFormat(virBufferPtr buf, virDomainVideoAccelDefPtr def) @@ -20770,6 +21093,10 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (virDomainShmemDefFormat(buf, def->shmems[n], flags) < 0) goto error; + for (n = 0; n < def->nmems; n++) + if (virDomainMemoryDefFormat(buf, def->mems[n], flags) < 0) + goto error; + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</devices>\n"); @@ -22174,6 +22501,9 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, case VIR_DOMAIN_DEVICE_PANIC: rc = virDomainPanicDefFormat(&buf, src->data.panic); break; + case VIR_DOMAIN_DEVICE_MEMORY: + rc = virDomainMemoryDefFormat(&buf, src->data.memory, flags); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e7376ed..475a174 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -133,6 +133,9 @@ typedef virDomainIdMapDef *virDomainIdMapDefPtr; typedef struct _virDomainPanicDef virDomainPanicDef; typedef virDomainPanicDef *virDomainPanicDefPtr; +typedef struct _virDomainMemoryDef virDomainMemoryDef; +typedef virDomainMemoryDef *virDomainMemoryDefPtr; + /* forward declarations virDomainChrSourceDef, required by * virDomainNetDef */ @@ -169,6 +172,7 @@ typedef enum { VIR_DOMAIN_DEVICE_SHMEM, VIR_DOMAIN_DEVICE_TPM, VIR_DOMAIN_DEVICE_PANIC, + VIR_DOMAIN_DEVICE_MEMORY, VIR_DOMAIN_DEVICE_LAST } virDomainDeviceType; @@ -199,6 +203,7 @@ struct _virDomainDeviceDef { virDomainShmemDefPtr shmem; virDomainTPMDefPtr tpm; virDomainPanicDefPtr panic; + virDomainMemoryDefPtr memory; } data; }; @@ -1970,6 +1975,28 @@ struct _virDomainRNGDef { virDomainDeviceInfo info; }; +typedef enum { + VIR_DOMAIN_MEMORY_MODEL_NONE, + VIR_DOMAIN_MEMORY_MODEL_DIMM, /* dimm hotpluggable memory device */ + + VIR_DOMAIN_MEMORY_MODEL_LAST +} virDomainMemoryModel; + +struct _virDomainMemoryDef { + /* source */ + virBitmapPtr sourceNodes; + unsigned long long pagesize; /* kibibytes */ + + /* target */ + int model; /* virDomainMemoryModel */ + unsigned int targetNode; + unsigned long long size; /* kibibytes */ + + virDomainDeviceInfo info; +}; + +void virDomainMemoryDefFree(virDomainMemoryDefPtr def); + struct _virDomainIdMapEntry { unsigned int start; unsigned int target; @@ -2190,6 +2217,9 @@ struct _virDomainDef { size_t nshmems; virDomainShmemDefPtr *shmems; + size_t nmems; + virDomainMemoryDefPtr *mems; + /* Only 1 */ virDomainWatchdogDefPtr watchdog; virDomainMemballoonDefPtr memballoon; @@ -2352,6 +2382,7 @@ bool virDomainObjTaint(virDomainObjPtr obj, int virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def); +int virDomainDeviceDefCheckUnsupportedMemoryDevice(virDomainDeviceDefPtr dev); void virDomainPanicDefFree(virDomainPanicDefPtr panic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); @@ -2900,6 +2931,8 @@ VIR_ENUM_DECL(virDomainRNGModel) VIR_ENUM_DECL(virDomainRNGBackend) VIR_ENUM_DECL(virDomainTPMModel) VIR_ENUM_DECL(virDomainTPMBackend) +VIR_ENUM_DECL(virDomainMemoryModel) +VIR_ENUM_DECL(virDomainMemoryBackingModel) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0988bc3..f24b449 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -216,6 +216,7 @@ virDomainDefSetMemoryInitial; virDomainDeleteConfig; virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; +virDomainDeviceDefCheckUnsupportedMemoryDevice; virDomainDeviceDefCopy; virDomainDeviceDefFree; virDomainDeviceDefParse; @@ -333,6 +334,7 @@ virDomainLockFailureTypeFromString; virDomainLockFailureTypeToString; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; +virDomainMemoryDefFree; virDomainNetAppendIpAddress; virDomainNetDefFormat; virDomainNetDefFree; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 3843ae0..ff1e4de 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -538,6 +538,9 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + return 0; } diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 1367b0c..c2180cb 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -113,6 +113,10 @@ virLXCDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC; + + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + return 0; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 055670a..d29e35b 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -130,6 +130,9 @@ openvzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3223752..6f328e4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1193,6 +1193,9 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f4b8dab..300bce4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7649,6 +7649,9 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.rng = NULL; break; + /*TODO: implement later */ + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7726,6 +7729,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_RNG: ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng); break; + case VIR_DOMAIN_DEVICE_MEMORY: + /* TODO: Implement later */ case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: @@ -7843,6 +7848,7 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, case VIR_DOMAIN_DEVICE_HOSTDEV: case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: @@ -7985,6 +7991,9 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, dev->data.rng = NULL; break; + case VIR_DOMAIN_DEVICE_MEMORY: + /* TODO: implement later */ + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -8110,6 +8119,9 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainRNGDefFree(virDomainRNGRemove(vmdef, idx)); break; + case VIR_DOMAIN_DEVICE_MEMORY: + /* TODO: implement later */ + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -8224,6 +8236,7 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7845fd1..5c5ad0e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3070,6 +3070,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); break; + /* TODO: implement later */ + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index bdfc12e..2d59126 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -439,6 +439,9 @@ umlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + return 0; } diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index b104243..3b11e9a 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -370,6 +370,9 @@ xenDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + return 0; } diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 3134441..d495f21 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -65,6 +65,9 @@ xenapiDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } + if (virDomainDeviceDefCheckUnsupportedMemoryDevice(dev) < 0) + return -1; + return 0; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml new file mode 100644 index 0000000..78088e2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.xml @@ -0,0 +1,50 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>1267710</memory> + <currentMemory unit='KiB'>1267710</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='2' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='219136' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + <memory model='dimm'> + <target> + <size unit='KiB'>524287</size> + <node>0</node> + </target> + </memory> + <memory model='dimm'> + <source> + <nodemask>1-3</nodemask> + <pagesize unit='KiB'>4096</pagesize> + </source> + <target> + <size unit='KiB'>524287</size> + <node>0</node> + </target> + </memory> + </devices> +</domain> -- 2.2.2

Make sure that libvirt has all vital information needed to reliably represent configuration of guest's memory devices in case of a migration. This patch forbids migration in case the required slot number and module base address are not present (failed to be loaded from qemu via monitor). --- Notes: Version 2: - rebased to the drop of ACPI src/qemu/qemu_migration.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 83be435..f36a36c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2016,6 +2016,20 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, virDomainObjPtr vm, } } + /* Verify that memory device config can be transferred reliably */ + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr mem = def->mems[i]; + + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM && + mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain's dimm info lacks slot ID " + "or base address")); + + return false; + } + } + return true; } -- 2.2.2

On 03/17/2015 10:19 AM, Peter Krempa wrote: Coverity points out that 'props' is being leaked here. So probably should go to cleanup or move this entire hunk above the props = line. In my test I just moved these lines above the props fetch and Coverity was happy John

On 03/17/2015 10:19 PM, Peter Krempa wrote: I have tested your series with our qemu memory hot remove patch series, here would be a possible error. When hotplug a memory device, its size has been aligned. So the compare for size here would fail possiblely. Thanks, Zhu [...]

On 03/20/2015 06:39 AM, John Ferlan wrote: Yes, the stored def->mems[i]->size value was modified. If you assign the size 524287 KiB, the stored value will be 524288. Thanks, Zhu

On Fri, Mar 20, 2015 at 07:40:21 -0400, John Ferlan wrote: Indeed, but the size needs to be aligned only for the active definition as we only align that one, thus it belongs to patch 12/12. I'll be adding the following diff to 12/12: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 40041d5..9b8d11b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4189,6 +4189,8 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, return -1; } + qemuDomainMemoryDeviceAlignSize(memdef); + if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("device not present in domain configuration")); Peter

On 03/17/2015 10:20 AM, Peter Krempa wrote: FWIW: Although there is a note in the AttachMemory code about consumption it may be worth noting here too just so no one in the future "sees" it missing and adds it thinking it's necessary Coverity determines that qemuBuildMemoryBackendStr can return props here with a -1 return and thus leak props That's because qemuBuildMemoryBackendStr sets the returned *backendProps and sets the local props to NULL before the (!hugepages) code which if it fails won't cause 'props' to be free'd properly Adding the virJSONValueFree(props); makes Coverity happy again. John

Add code to hot-remove memory devices from qemu. Unfortunately QEMU doesn't support this right now, so this is just for completenes. --- Notes: Version 3: - moved check for the "-device" capability before any other code Version 2: - no change src/qemu/qemu_driver.c | 4 ++- src/qemu/qemu_hotplug.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cbdf279..9731b5f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7733,7 +7733,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng); break; case VIR_DOMAIN_DEVICE_MEMORY: - /* TODO: Implement later */ + ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory); + break; + case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 88c5e3c..3365982 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2828,6 +2828,44 @@ qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver, } +static int +qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr mem) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event; + char *backendAlias = NULL; + int rc; + int idx; + + VIR_DEBUG("Removing memory device %s from domain %p %s", + mem->info.alias, vm, vm->def->name); + + if ((event = virDomainEventDeviceRemovedNewFromObj(vm, mem->info.alias))) + qemuDomainEventQueue(driver, event); + + if (virAsprintf(&backendAlias, "mem%s", mem->info.alias) < 0) + goto error; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelObject(priv->mon, backendAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + goto error; + + if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0) + virDomainMemoryRemove(vm->def, idx); + + virDomainMemoryDefFree(mem); + VIR_FREE(backendAlias); + return 0; + + error: + VIR_FREE(backendAlias); + return -1; +} + + static void qemuDomainRemovePCIHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3165,8 +3203,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); break; - /* TODO: implement later */ case VIR_DOMAIN_DEVICE_MEMORY: + ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: @@ -4118,3 +4157,53 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + + +int +qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr memdef) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainMemoryDefPtr mem; + int idx; + int rc; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return -1; + } + + if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + mem = vm->def->mems[idx]; + + if (!mem->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias for the memory device was not found")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &mem->info); + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelDevice(priv->mon, mem->info.alias); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + goto cleanup; + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveMemoryDevice(driver, vm, mem); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index ad4ff38..4140da3 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -60,6 +60,9 @@ int qemuDomainFindGraphicsIndex(virDomainDefPtr def, int qemuDomainAttachMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem); +int qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr memdef); int qemuDomainChangeGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainGraphicsDefPtr dev); -- 2.2.2