[PATCH v2 00/27] Introduce virtio memory support
v2 of: https://www.redhat.com/archives/libvir-list/2020-November/msg01525.html diff to v1: - I've made change to 23/26 which now updates <currentMemory/> on MEMORY_DEVICE_SIZE_CHANGE event - virsh manpage was written in 25/26 - 26/26 is new, it's NEWS.rst addition Original patchset was: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> Tested-by: Han Han <hhan@redhat.com> and I will add those tags before pushing. However, I'd like have review for this new version, because of the changes I made. Michal Prívozník (27): viruuid: Rework virUUIDIsValid() internal.h: Introduce VIR_IS_POW2() docs: Fix nvdimm example wrt to <uuid/> domain_conf: Check NVDIMM UUID in ABI stability qemu_domain_address: Reformat qemuDomainAssignS390Addresses() conf: Require nvdimm path in validate step domain_conf: Fix virDomainMemoryModel type virDomainMemorySourceDefFormat: Utilize virXMLFormatElement() virDomainMemoryTargetDefFormat: Utilize virXMLFormatElement() qemu: Move mem validation into post parse validator conf: Move some of virDomainMemoryDef members into a union conf: Introduce virtio-pmem <memory/> model qemu_capabilities: Introduce QEMU_CAPS_DEVICE_VIRTIO_{P}MEM_PCI qemu_validate: Require virtio-mem device for mem model virtio security: Relabel virtio mem qemu: Allow virtio-pmem in CGroups qemu: Create virtio-pmem in domain namespace qemu_command: Move dimm into qemuBuildDeviceAddressStr() qemu: Build command line for virtio-pmem conf: Introduce virtio-mem <memory/> model qemu: Build command line for virtio-mem qemu: Wire up <memory/> live update qemu: Wire up MEMORY_DEVICE_SIZE_CHANGE event qemu: Refresh the actual size of virtio-mem on monitor reconnect virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem() virsh: Introduce --virtio to setmem news: document recent virtio memory addition NEWS.rst | 9 + docs/formatdomain.rst | 70 +++- docs/manpages/virsh.rst | 6 + docs/schemas/domaincommon.rng | 16 + src/conf/domain_conf.c | 372 ++++++++++++++---- src/conf/domain_conf.h | 38 +- src/internal.h | 10 + src/libvirt_private.syms | 2 + src/qemu/qemu_alias.c | 59 ++- src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_cgroup.c | 43 +- src/qemu/qemu_command.c | 172 +++++--- src/qemu/qemu_command.h | 5 +- src/qemu/qemu_domain.c | 99 +++-- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_domain_address.c | 98 +++-- src/qemu/qemu_domain_address.h | 3 +- src/qemu/qemu_driver.c | 201 +++++++++- src/qemu/qemu_hotplug.c | 22 +- src/qemu/qemu_hotplug.h | 5 + src/qemu/qemu_monitor.c | 37 ++ src/qemu/qemu_monitor.h | 27 ++ src/qemu/qemu_monitor_json.c | 94 +++-- src/qemu/qemu_monitor_json.h | 5 + src/qemu/qemu_namespace.c | 19 +- src/qemu/qemu_process.c | 107 ++++- src/qemu/qemu_validate.c | 78 ++-- src/security/security_apparmor.c | 35 +- src/security/security_dac.c | 48 ++- src/security/security_selinux.c | 48 ++- src/security/virt-aa-helper.c | 22 +- src/util/virrandom.c | 2 +- src/util/viruuid.c | 17 +- src/util/viruuid.h | 2 +- .../caps_4.1.0.x86_64.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 2 + .../caps_5.2.0.x86_64.xml | 2 + ...mory-hotplug-virtio-mem.x86_64-latest.args | 53 +++ .../memory-hotplug-virtio-mem.xml | 78 ++++ ...ory-hotplug-virtio-pmem.x86_64-latest.args | 45 +++ .../memory-hotplug-virtio-pmem.xml | 54 +++ tests/qemuxml2argvtest.c | 2 + ...emory-hotplug-virtio-mem.x86_64-latest.xml | 1 + ...mory-hotplug-virtio-pmem.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 3 + tools/virsh-domain.c | 138 ++++++- 49 files changed, 1766 insertions(+), 394 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.xml create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-virtio-mem.x86_64-latest.xml create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-virtio-pmem.x86_64-latest.xml -- 2.26.2
The only test we do when checking for UUID validity is that whether all bytes are the same (invalid UUID) or not (valid UUID). The algorithm we use is needlessly complicated. Also, the checked UUID is not modified and hence the argument can be of 'const' type. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viruuid.c | 17 +++++++---------- src/util/viruuid.h | 2 +- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/util/viruuid.c b/src/util/viruuid.c index 908b09945d..558fbb9c0d 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -170,25 +170,22 @@ virUUIDFormat(const unsigned char *uuid, char *uuidstr) * Basic tests: * - Not all of the digits may be equal */ -int -virUUIDIsValid(unsigned char *uuid) +bool +virUUIDIsValid(const unsigned char *uuid) { size_t i; - unsigned int ctr = 1; - unsigned char c; if (!uuid) - return 0; - - c = uuid[0]; + return false; for (i = 1; i < VIR_UUID_BUFLEN; i++) - if (uuid[i] == c) - ctr++; + if (uuid[i] != uuid[0]) + return true; - return ctr != VIR_UUID_BUFLEN; + return false; } + static int getDMISystemUUID(char *uuid, int len) { diff --git a/src/util/viruuid.h b/src/util/viruuid.h index 5d64e58405..b403b1906a 100644 --- a/src/util/viruuid.h +++ b/src/util/viruuid.h @@ -43,7 +43,7 @@ int virSetHostUUIDStr(const char *host_uuid); int virGetHostUUID(unsigned char *host_uuid) ATTRIBUTE_NONNULL(1) G_GNUC_NO_INLINE; -int virUUIDIsValid(unsigned char *uuid); +bool virUUIDIsValid(const unsigned char *uuid); int virUUIDGenerate(unsigned char *uuid) G_GNUC_NO_INLINE; -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:04 +0100, Michal Privoznik wrote:
The only test we do when checking for UUID validity is that whether all bytes are the same (invalid UUID) or not (valid UUID). The algorithm we use is needlessly complicated.
Also, the checked UUID is not modified and hence the argument can be of 'const' type.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viruuid.c | 17 +++++++---------- src/util/viruuid.h | 2 +- 2 files changed, 8 insertions(+), 11 deletions(-)
I've briefly thought whether we could replace the internals by a memset of a temp array and then memcmp, but that doesn't seem to be worth. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This macro checks whether given number is an integer power of two. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/internal.h | 10 +++++++++ src/qemu/qemu_validate.c | 46 +++++++++++++++++++++------------------- src/util/virrandom.c | 2 +- 3 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/internal.h b/src/internal.h index ff94e7e53e..0a03dfc46f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -237,6 +237,16 @@ (a) = (a) ^ (b); \ } while (0) + +/** + * VIR_IS_POW2: + * + * Returns true if given number is a power of two + */ +#define VIR_IS_POW2(x) \ + ((x) && !((x) & ((x) - 1))) + + /** * virCheckFlags: * @supported: an OR'ed set of supported flags diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index e60d39a22f..6f4662b25a 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1460,20 +1460,32 @@ qemuValidateDomainDeviceDefNetwork(const virDomainNetDef *net, return -1; } - if (net->driver.virtio.rx_queue_size && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio rx_queue_size option is not supported " - "with this QEMU binary")); - return -1; + if (net->driver.virtio.rx_queue_size) { + if (!VIR_IS_POW2(net->driver.virtio.rx_queue_size)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("rx_queue_size has to be a power of two")); + return -1; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio rx_queue_size option is not supported " + "with this QEMU binary")); + return -1; + } } - if (net->driver.virtio.tx_queue_size && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio tx_queue_size option is not supported " - "with this QEMU binary")); - return -1; + if (net->driver.virtio.tx_queue_size) { + if (!VIR_IS_POW2(net->driver.virtio.tx_queue_size)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("tx_queue_size has to be a power of two")); + return -1; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio tx_queue_size option is not supported " + "with this QEMU binary")); + return -1; + } } if (net->mtu && @@ -1484,16 +1496,6 @@ qemuValidateDomainDeviceDefNetwork(const virDomainNetDef *net, return -1; } - if (net->driver.virtio.rx_queue_size & (net->driver.virtio.rx_queue_size - 1)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("rx_queue_size has to be a power of two")); - return -1; - } - if (net->driver.virtio.tx_queue_size & (net->driver.virtio.tx_queue_size - 1)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("tx_queue_size has to be a power of two")); - return -1; - } if (qemuValidateDomainVirtioOptions(net->virtio, qemuCaps) < 0) return -1; } diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 6417232e3a..3ae1297e6b 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -89,7 +89,7 @@ double virRandom(void) */ uint32_t virRandomInt(uint32_t max) { - if ((max & (max - 1)) == 0) + if (VIR_IS_POW2(max)) return virRandomBits(__builtin_ffs(max) - 1); return virRandom() * max; -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:05 +0100, Michal Privoznik wrote:
This macro checks whether given number is an integer power of two.
Mention that you are also refactoring code to use it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/internal.h | 10 +++++++++ src/qemu/qemu_validate.c | 46 +++++++++++++++++++++------------------- src/util/virrandom.c | 2 +- 3 files changed, 35 insertions(+), 23 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On a Friday in 2020, Peter Krempa wrote:
On Thu, Dec 03, 2020 at 13:36:05 +0100, Michal Privoznik wrote:
This macro checks whether given number is an integer power of two.
Mention that you are also refactoring code to use it.
IIUC those are new checks - not a refactor. Jano
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/internal.h | 10 +++++++++ src/qemu/qemu_validate.c | 46 +++++++++++++++++++++------------------- src/util/virrandom.c | 2 +- 3 files changed, 35 insertions(+), 23 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Mon, Dec 07, 2020 at 09:55:31 +0100, Ján Tomko wrote:
On a Friday in 2020, Peter Krempa wrote:
On Thu, Dec 03, 2020 at 13:36:05 +0100, Michal Privoznik wrote:
This macro checks whether given number is an integer power of two.
Mention that you are also refactoring code to use it.
IIUC those are new checks - not a refactor.
It is in fact a bit more involved refactor. The checks are removed in a different hunk. In fact this question shows that the patch split is wrong.
On 12/7/20 9:55 AM, Ján Tomko wrote:
On a Friday in 2020, Peter Krempa wrote:
On Thu, Dec 03, 2020 at 13:36:05 +0100, Michal Privoznik wrote:
This macro checks whether given number is an integer power of two.
Mention that you are also refactoring code to use it.
IIUC those are new checks - not a refactor.
What is new? These checks for pow2 were introduced in v2.3.0-rc1~186 and v3.7.0-rc1~236. I'm not sure such old checks can be viewed as new. Michal
On a Monday in 2020, Michal Privoznik wrote:
On 12/7/20 9:55 AM, Ján Tomko wrote:
On a Friday in 2020, Peter Krempa wrote:
On Thu, Dec 03, 2020 at 13:36:05 +0100, Michal Privoznik wrote:
This macro checks whether given number is an integer power of two.
Mention that you are also refactoring code to use it.
IIUC those are new checks - not a refactor.
What is new? These checks for pow2 were introduced in v2.3.0-rc1~186 and v3.7.0-rc1~236. I'm not sure such old checks can be viewed as new.
Right, guess I shouldn't look at code so early in the morning. Jano
Michal
On PPC platform it is required that a NVDIMM has an UUID. If none is provided then libvirt generates one during parsing (see v6.2.0-rc1~96 and friends). However, the example provided in our documentation is not valid XML. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ff64996af2..512939679b 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -7159,7 +7159,7 @@ Example: usage of the memory devices </target> </memory> <memory model='nvdimm'> - <uuid> + <uuid>9066901e-c90a-46ad-8b55-c18868cf92ae</uuid> <source> <path>/tmp/nvdimm</path> </source> @@ -7173,7 +7173,7 @@ Example: usage of the memory devices </target> </memory> <memory model='nvdimm' access='shared'> - <uuid> + <uuid>e39080c8-7f99-4b12-9c43-d80014e977b8</uuid> <source> <path>/dev/dax0.0</path> <alignsize unit='KiB'>2048</alignsize> @@ -7211,7 +7211,7 @@ Example: usage of the memory devices ``uuid`` For pSeries guests, an uuid can be set to identify the nvdimm module. If - absent, libvirt will generate an uuid. automatically. This attribute is + absent, libvirt will generate an uuid automatically. This attribute is allowed only for ``model='nvdimm'`` for pSeries guests. :since:`Since 6.2.0` ``source`` -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:06 +0100, Michal Privoznik wrote:
On PPC platform it is required that a NVDIMM has an UUID. If none is provided then libvirt generates one during parsing (see v6.2.0-rc1~96 and friends). However, the example provided in our documentation is not valid XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The UUID is guest visible and thus shouldn't change if we want to not break guest ABI. Fixes: 08ed673901bb5b4f419b37bcce9b11d31ce370e6 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 425e3c3710..d8df18b542 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24204,6 +24204,12 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, "source NVDIMM readonly flag")); return false; } + + if (memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target NVDIMM UUID doesn't match source NVDIMM")); + return false; + } } return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:07 +0100, Michal Privoznik wrote:
The UUID is guest visible and thus shouldn't change if we want to not break guest ABI.
Fixes: 08ed673901bb5b4f419b37bcce9b11d31ce370e6 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
Neither the libvirt commit nor the qemu commit mention this fact clearly. Libvirt doesn't store it under <target> either, which doesn't entirely substantiate the claim, but it makes sense that it's the same since it's an uuid and also it's formatted as part of the frontend definiton on the command line. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain_address.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2788dc7fb3..d872f75b38 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, if (qemuDomainIsS390CCW(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) - qemuDomainPrimeVfioDeviceAddresses( - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); - qemuDomainPrimeVirtioDeviceAddresses( - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); + qemuDomainPrimeVfioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); + + qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def))) goto cleanup; } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) { /* deal with legacy virtio-s390 */ - qemuDomainPrimeVirtioDeviceAddresses( - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390); + qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390); } ret = 0; -- 2.26.2
On 03/12/2020 13.36, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain_address.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2788dc7fb3..d872f75b38 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, if (qemuDomainIsS390CCW(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) - qemuDomainPrimeVfioDeviceAddresses( - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); - qemuDomainPrimeVirtioDeviceAddresses( - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); + qemuDomainPrimeVfioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); + + qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def))) goto cleanup;
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) { /* deal with legacy virtio-s390 */ - qemuDomainPrimeVirtioDeviceAddresses( - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390); + qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390); }
ret = 0;
Reviewed-by: Thomas Huth <thuth@redhat.com>
Our code expects that a nvdimm has a path defined always. And the parser does check for that. Well, not fully - only when parsing <source/> (which is an optional element). So if the element is not in the XML then the check is not performed and the assumption is broken. Verify in the memory def validator that a path was set. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 +++++++----- src/security/security_apparmor.c | 6 ------ 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8df18b542..da14760e2d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6694,6 +6694,12 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, const virDomainDef *def) { if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (!mem->nvdimmPath) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("path is required for model 'nvdimm'")); + return -1; + } + if (mem->discard == VIR_TRISTATE_BOOL_YES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("discard is not supported for nvdimms")); @@ -16690,11 +16696,7 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - if (!(def->nvdimmPath = virXPathString("string(./path)", ctxt))) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("path is required for model 'nvdimm'")); - return -1; - } + def->nvdimmPath = virXPathString("string(./path)", ctxt); if (virDomainParseMemory("./alignsize", "./alignsize/@unit", ctxt, &def->alignsize, false, false) < 0) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index c2d86c6940..f306af8dd3 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -686,12 +686,6 @@ AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, switch ((virDomainMemoryModel) mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - if (mem->nvdimmPath == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: nvdimm without a path"), - __func__); - return -1; - } if (!virFileExists(mem->nvdimmPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: \'%s\' does not exist"), -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:09 +0100, Michal Privoznik wrote:
Our code expects that a nvdimm has a path defined always. And the parser does check for that. Well, not fully - only when parsing <source/> (which is an optional element). So if the element is not in the XML then the check is not performed and the assumption is broken. Verify in the memory def validator that a path was set.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 +++++++----- src/security/security_apparmor.c | 6 ------ 2 files changed, 7 insertions(+), 11 deletions(-)
We don't validate existing configs, but removing the apparmor check is okay since you can't have an running VM with broken config due to it and the check will be done when starting the VM. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The virDomainMemoryModel structure has a @type member which is really type of virDomainMemoryModel but we store it as int because the virDomainMemoryModelTypeFromString() call stores its retval right into it. Then, to have compiler do compile time check for us, every switch() typecasts the @type. This is needlessly verbose because the parses already has @val - a variable to store temporary values. Switch @type in the struct to virDomainMemoryModel and drop all typecasts. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 9 +++++---- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/security/security_apparmor.c | 2 +- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 7 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index da14760e2d..2a2cfd24e0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16676,7 +16676,7 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, ctxt->node = node; - switch ((virDomainMemoryModel) def->model) { + switch (def->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt, &def->pagesize, false, false) < 0) @@ -16897,12 +16897,13 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if ((def->model = virDomainMemoryModelTypeFromString(tmp)) <= 0) { + if ((val = virDomainMemoryModelTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid memory model '%s'"), tmp); goto error; } VIR_FREE(tmp); + def->model = val; if ((tmp = virXMLPropString(memdevNode, "access"))) { if ((val = virDomainMemoryAccessTypeFromString(tmp)) <= 0) { @@ -18579,7 +18580,7 @@ virDomainMemoryFindByDefInternal(virDomainDefPtr def, tmp->size != mem->size) continue; - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: /* source stuff -> match with device */ if (tmp->pagesize != mem->pagesize) @@ -27846,7 +27847,7 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, virBufferAddLit(buf, "<source>\n"); virBufferAdjustIndent(buf, 2); - switch ((virDomainMemoryModel) def->model) { + switch (def->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: if (def->sourceNodes) { if (!(bitmap = virBitmapFormat(def->sourceNodes))) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 34cde22965..5853e3b290 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2322,7 +2322,7 @@ struct _virDomainMemoryDef { bool nvdimmPmem; /* valid only for NVDIMM */ /* target */ - int model; /* virDomainMemoryModel */ + virDomainMemoryModel model; int targetNode; unsigned long long size; /* kibibytes */ unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 33f9b96bf8..eb64ce84da 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3280,7 +3280,7 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem) return NULL; } - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: case VIR_DOMAIN_MEMORY_MODEL_DIMM: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 663c0af867..19a699540f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8485,7 +8485,7 @@ static int qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, const virDomainDef *def) { - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && @@ -8598,7 +8598,7 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, for (i = 0; i < def->nmems; i++) { hotplugMemory += def->mems[i]->size; - switch ((virDomainMemoryModel) def->mems[i]->model) { + switch (def->mems[i]->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: needPCDimmCap = true; break; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index f306af8dd3..eed66e460f 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -684,7 +684,7 @@ AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, if (mem == NULL) return 0; - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (!virFileExists(mem->nvdimmPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 258d246659..4f4a0a069e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1887,7 +1887,7 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr, { int ret = -1; - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: ret = virSecurityDACRestoreFileLabel(mgr, mem->nvdimmPath); break; @@ -2060,7 +2060,7 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, uid_t user; gid_t group; - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel && !seclabel->relabel) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c0e76b2222..e9cd95916e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1571,7 +1571,7 @@ virSecuritySELinuxSetMemoryLabel(virSecurityManagerPtr mgr, { virSecurityLabelDefPtr seclabel; - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (!seclabel || !seclabel->relabel) @@ -1600,7 +1600,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr, int ret = -1; virSecurityLabelDefPtr seclabel; - switch ((virDomainMemoryModel) mem->model) { + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (!seclabel || !seclabel->relabel) -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:10 +0100, Michal Privoznik wrote:
The virDomainMemoryModel structure has a @type member which is really type of virDomainMemoryModel but we store it as int because the virDomainMemoryModelTypeFromString() call stores its retval right into it. Then, to have compiler do compile time check for us, every switch() typecasts the @type. This is needlessly verbose because the parses already has @val - a variable to store temporary values. Switch @type in the struct to
I actually wanted to ramble about reuse of @val, but it's there multiple times ...
virDomainMemoryModel and drop all typecasts.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 9 +++++---- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/security/security_apparmor.c | 2 +- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 7 files changed, 14 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The virDomainMemorySourceDefFormat() uses good old style of formatting child buffer (virBufferAdjustIndent()). When switched to virXMLFormatElement() we can save a couple of lines. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2a2cfd24e0..bb0d9f4501 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27839,37 +27839,32 @@ static int virDomainMemorySourceDefFormat(virBufferPtr buf, virDomainMemoryDefPtr def) { + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); g_autofree char *bitmap = NULL; - if (!def->pagesize && !def->sourceNodes && !def->nvdimmPath) - return 0; - - virBufferAddLit(buf, "<source>\n"); - virBufferAdjustIndent(buf, 2); - switch (def->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: if (def->sourceNodes) { if (!(bitmap = virBitmapFormat(def->sourceNodes))) return -1; - virBufferAsprintf(buf, "<nodemask>%s</nodemask>\n", bitmap); + virBufferAsprintf(&childBuf, "<nodemask>%s</nodemask>\n", bitmap); } if (def->pagesize) - virBufferAsprintf(buf, "<pagesize unit='KiB'>%llu</pagesize>\n", + virBufferAsprintf(&childBuf, "<pagesize unit='KiB'>%llu</pagesize>\n", def->pagesize); break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - virBufferEscapeString(buf, "<path>%s</path>\n", def->nvdimmPath); + virBufferEscapeString(&childBuf, "<path>%s</path>\n", def->nvdimmPath); if (def->alignsize) - virBufferAsprintf(buf, "<alignsize unit='KiB'>%llu</alignsize>\n", + virBufferAsprintf(&childBuf, "<alignsize unit='KiB'>%llu</alignsize>\n", def->alignsize); if (def->nvdimmPmem) - virBufferAddLit(buf, "<pmem/>\n"); + virBufferAddLit(&childBuf, "<pmem/>\n"); break; case VIR_DOMAIN_MEMORY_MODEL_NONE: @@ -27877,8 +27872,7 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, break; } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</source>\n"); + virXMLFormatElement(buf, "source", NULL, &childBuf); return 0; } -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:11 +0100, Michal Privoznik wrote:
The virDomainMemorySourceDefFormat() uses good old style of formatting child buffer (virBufferAdjustIndent()). When switched to virXMLFormatElement() we can save a couple of lines.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The virDomainMemoryTargetDefFormat() uses good old style of formatting child buffer (virBufferAdjustIndent()). When switched to virXMLFormatElement() we can save a couple of lines Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bb0d9f4501..7d9e5d14ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27882,24 +27882,21 @@ static void virDomainMemoryTargetDefFormat(virBufferPtr buf, virDomainMemoryDefPtr def) { - virBufferAddLit(buf, "<target>\n"); - virBufferAdjustIndent(buf, 2); + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); - virBufferAsprintf(buf, "<size unit='KiB'>%llu</size>\n", def->size); + virBufferAsprintf(&childBuf, "<size unit='KiB'>%llu</size>\n", def->size); if (def->targetNode >= 0) - virBufferAsprintf(buf, "<node>%d</node>\n", def->targetNode); + virBufferAsprintf(&childBuf, "<node>%d</node>\n", def->targetNode); if (def->labelsize) { - virBufferAddLit(buf, "<label>\n"); - virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<size unit='KiB'>%llu</size>\n", def->labelsize); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</label>\n"); + g_auto(virBuffer) labelChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); + + virBufferAsprintf(&labelChildBuf, "<size unit='KiB'>%llu</size>\n", def->labelsize); + virXMLFormatElement(&childBuf, "label", NULL, &labelChildBuf); } if (def->readonly) - virBufferAddLit(buf, "<readonly/>\n"); + virBufferAddLit(&childBuf, "<readonly/>\n"); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</target>\n"); + virXMLFormatElement(buf, "target", NULL, &childBuf); } static int -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:12 +0100, Michal Privoznik wrote:
The virDomainMemoryTargetDefFormat() uses good old style of formatting child buffer (virBufferAdjustIndent()). When switched to virXMLFormatElement() we can save a couple of lines
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There is this function qemuDomainDefValidateMemoryHotplug() which is called explicitly from hotplug path and the qemu's domain def validator. This is not really necessary because we can move the part that validates feature against qemuCaps into device validator which is called implicitly (from qemu driver's POV). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 32 +------------------------------- src/qemu/qemu_validate.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 19a699540f..e705e8d8d5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8544,14 +8544,12 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, */ int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, - virQEMUCapsPtr qemuCaps, + virQEMUCapsPtr qemuCaps G_GNUC_UNUSED, const virDomainMemoryDef *mem) { unsigned int nmems = def->nmems; unsigned long long hotplugSpace; unsigned long long hotplugMemory = 0; - bool needPCDimmCap = false; - bool needNvdimmCap = false; size_t i; hotplugSpace = def->mem.max_memory - virDomainDefGetMemoryInitial(def); @@ -8598,40 +8596,12 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, for (i = 0; i < def->nmems; i++) { hotplugMemory += def->mems[i]->size; - switch (def->mems[i]->model) { - case VIR_DOMAIN_MEMORY_MODEL_DIMM: - needPCDimmCap = true; - break; - - case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - needNvdimmCap = true; - break; - - case VIR_DOMAIN_MEMORY_MODEL_NONE: - case VIR_DOMAIN_MEMORY_MODEL_LAST: - break; - } - /* already existing devices don't need to be checked on hotplug */ if (!mem && qemuDomainDefValidateMemoryHotplugDevice(def->mems[i], def) < 0) return -1; } - if (needPCDimmCap && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("memory hotplug isn't supported by this QEMU binary")); - return -1; - } - - if (needNvdimmCap && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("nvdimm isn't supported by this QEMU binary")); - return -1; - } - if (hotplugMemory > hotplugSpace) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("memory device total size exceeds hotplug space")); diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 6f4662b25a..8ceea022d7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4595,7 +4595,16 @@ static int qemuValidateDomainDeviceDefMemory(virDomainMemoryDefPtr mem, virQEMUCapsPtr qemuCaps) { - if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("memory hotplug isn't supported by this QEMU binary")); + return -1; + } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("nvdimm isn't supported by this QEMU binary")); @@ -4609,6 +4618,11 @@ qemuValidateDomainDeviceDefMemory(virDomainMemoryDefPtr mem, "with this QEMU binary")); return -1; } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; } return 0; -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:13 +0100, Michal Privoznik wrote:
There is this function qemuDomainDefValidateMemoryHotplug() which is called explicitly from hotplug path and the qemu's domain def validator. This is not really necessary because we can move the part that validates feature against qemuCaps into device validator which is called implicitly (from qemu driver's POV).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 32 +------------------------------- src/qemu/qemu_validate.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 32 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Thu, Dec 03, 2020 at 13:36:13 +0100, Michal Privoznik wrote:
There is this function qemuDomainDefValidateMemoryHotplug() which is called explicitly from hotplug path and the qemu's domain def validator. This is not really necessary because we can move the part that validates feature against qemuCaps into device validator which is called implicitly (from qemu driver's POV).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 32 +------------------------------- src/qemu/qemu_validate.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 19a699540f..e705e8d8d5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8544,14 +8544,12 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, */ int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, - virQEMUCapsPtr qemuCaps, + virQEMUCapsPtr qemuCaps G_GNUC_UNUSED,
Actually, I'm missing a patch removing this now-unused argument.
The structure has two sets of members: some for the target and some for the source. The latter is model dependant (e.g. path to a nvdimm device applies only to NVDIMM model). Move the members into an union so that it is obvious which members apply to which model. This way it's easier to maintain code cleanliness when introducing a new model. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 57 ++++++++++++++++++------------ src/conf/domain_conf.h | 16 ++++++--- src/qemu/qemu_alias.c | 13 +++++-- src/qemu/qemu_cgroup.c | 10 +++--- src/qemu/qemu_command.c | 60 +++++++++++++++++++++----------- src/qemu/qemu_namespace.c | 2 +- src/qemu/qemu_process.c | 8 ++--- src/security/security_apparmor.c | 6 ++-- src/security/security_dac.c | 4 +-- src/security/security_selinux.c | 4 +-- src/security/virt-aa-helper.c | 2 +- 11 files changed, 113 insertions(+), 69 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7d9e5d14ad..4d462b0627 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3136,8 +3136,18 @@ void virDomainMemoryDefFree(virDomainMemoryDefPtr def) if (!def) return; - VIR_FREE(def->nvdimmPath); - virBitmapFree(def->sourceNodes); + switch (def->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + virBitmapFree(def->s.dimm.sourceNodes); + break; + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + VIR_FREE(def->s.nvdimm.path); + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } + virDomainDeviceInfoClear(&def->info); VIR_FREE(def); } @@ -6694,7 +6704,7 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, const virDomainDef *def) { if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { - if (!mem->nvdimmPath) { + if (!mem->s.nvdimm.path) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("path is required for model 'nvdimm'")); return -1; @@ -16679,15 +16689,15 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, switch (def->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt, - &def->pagesize, false, false) < 0) + &def->s.dimm.pagesize, false, false) < 0) return -1; if ((nodemask = virXPathString("string(./nodemask)", ctxt))) { - if (virBitmapParse(nodemask, &def->sourceNodes, + if (virBitmapParse(nodemask, &def->s.dimm.sourceNodes, VIR_DOMAIN_CPUMASK_LEN) < 0) return -1; - if (virBitmapIsAllClear(def->sourceNodes)) { + if (virBitmapIsAllClear(def->s.dimm.sourceNodes)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'nodemask': %s"), nodemask); return -1; @@ -16696,14 +16706,14 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - def->nvdimmPath = virXPathString("string(./path)", ctxt); + def->s.nvdimm.path = virXPathString("string(./path)", ctxt); if (virDomainParseMemory("./alignsize", "./alignsize/@unit", ctxt, - &def->alignsize, false, false) < 0) + &def->s.nvdimm.alignsize, false, false) < 0) return -1; if (virXPathBoolean("boolean(./pmem)", ctxt)) - def->nvdimmPmem = true; + def->s.nvdimm.pmem = true; break; @@ -18583,15 +18593,15 @@ virDomainMemoryFindByDefInternal(virDomainDefPtr def, switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: /* source stuff -> match with device */ - if (tmp->pagesize != mem->pagesize) + if (tmp->s.dimm.pagesize != mem->s.dimm.pagesize) continue; - if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes)) + if (!virBitmapEqual(tmp->s.dimm.sourceNodes, mem->s.dimm.sourceNodes)) continue; break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - if (STRNEQ(tmp->nvdimmPath, mem->nvdimmPath)) + if (STRNEQ(tmp->s.nvdimm.path, mem->s.nvdimm.path)) continue; break; @@ -24186,15 +24196,15 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, return false; } - if (src->alignsize != dst->alignsize) { + if (src->s.nvdimm.alignsize != dst->s.nvdimm.alignsize) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target NVDIMM alignment '%llu' doesn't match " "source NVDIMM alignment '%llu'"), - src->alignsize, dst->alignsize); + src->s.nvdimm.alignsize, dst->s.nvdimm.alignsize); return false; } - if (src->nvdimmPmem != dst->nvdimmPmem) { + if (src->s.nvdimm.pmem != dst->s.nvdimm.pmem) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target NVDIMM pmem flag doesn't match " "source NVDIMM pmem flag")); @@ -27844,26 +27854,27 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, switch (def->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: - if (def->sourceNodes) { - if (!(bitmap = virBitmapFormat(def->sourceNodes))) + if (def->s.dimm.sourceNodes) { + if (!(bitmap = virBitmapFormat(def->s.dimm.sourceNodes))) return -1; virBufferAsprintf(&childBuf, "<nodemask>%s</nodemask>\n", bitmap); } - if (def->pagesize) + if (def->s.dimm.pagesize) { virBufferAsprintf(&childBuf, "<pagesize unit='KiB'>%llu</pagesize>\n", - def->pagesize); + def->s.dimm.pagesize); + } break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - virBufferEscapeString(&childBuf, "<path>%s</path>\n", def->nvdimmPath); + virBufferEscapeString(&childBuf, "<path>%s</path>\n", def->s.nvdimm.path); - if (def->alignsize) + if (def->s.nvdimm.alignsize) virBufferAsprintf(&childBuf, "<alignsize unit='KiB'>%llu</alignsize>\n", - def->alignsize); + def->s.nvdimm.alignsize); - if (def->nvdimmPmem) + if (def->s.nvdimm.pmem) virBufferAddLit(&childBuf, "<pmem/>\n"); break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5853e3b290..e7f8fc156f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2315,11 +2315,17 @@ struct _virDomainMemoryDef { virTristateBool discard; /* source */ - virBitmapPtr sourceNodes; - unsigned long long pagesize; /* kibibytes */ - char *nvdimmPath; - unsigned long long alignsize; /* kibibytes; valid only for NVDIMM */ - bool nvdimmPmem; /* valid only for NVDIMM */ + union { + struct { + virBitmapPtr sourceNodes; + unsigned long long pagesize; /* kibibytes */ + } dimm; /* VIR_DOMAIN_MEMORY_MODEL_DIMM */ + struct { + char *path; + unsigned long long alignsize; /* kibibytes */ + bool pmem; + } nvdimm; /* VIR_DOMAIN_MEMORY_MODEL_NVDIMM */ + } s; /* target */ virDomainMemoryModel model; diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index dcb6c7156d..5ebcd766a9 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -486,15 +486,22 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, size_t i; int maxidx = 0; int idx; - const char *prefix; + const char *prefix = NULL; if (mem->info.alias) return 0; - if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM) + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: prefix = "dimm"; - else + break; + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: prefix = "nvdimm"; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } if (oldAlias) { for (i = 0; i < def->nmems; i++) { diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f7146a71c9..92caadf840 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -508,11 +508,11 @@ qemuSetupMemoryDevicesCgroup(virDomainObjPtr vm, if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - VIR_DEBUG("Setting devices Cgroup for NVDIMM device: %s", mem->nvdimmPath); - rv = virCgroupAllowDevicePath(priv->cgroup, mem->nvdimmPath, + VIR_DEBUG("Setting devices Cgroup for NVDIMM device: %s", mem->s.nvdimm.path); + rv = virCgroupAllowDevicePath(priv->cgroup, mem->s.nvdimm.path, VIR_CGROUP_DEVICE_RW, false); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", - mem->nvdimmPath, "rw", rv); + mem->s.nvdimm.path, "rw", rv); return rv; } @@ -531,10 +531,10 @@ qemuTeardownMemoryDevicesCgroup(virDomainObjPtr vm, if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - rv = virCgroupDenyDevicePath(priv->cgroup, mem->nvdimmPath, + rv = virCgroupDenyDevicePath(priv->cgroup, mem->s.nvdimm.path, VIR_CGROUP_DEVICE_RWM, false); virDomainAuditCgroupPath(vm, priv->cgroup, - "deny", mem->nvdimmPath, "rwm", rv); + "deny", mem->s.nvdimm.path, "rwm", rv); return rv; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eb64ce84da..4bd45e0638 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2958,10 +2958,13 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, int rc; g_autoptr(virJSONValue) props = NULL; bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, mem->targetNode); - unsigned long long pagesize = mem->pagesize; - bool needHugepage = !!pagesize; - bool useHugepage = !!pagesize; + unsigned long long pagesize = 0; + bool needHugepage = false; + bool useHugepage = false; int discard = mem->discard; + const char *nvdimmPath = NULL; + unsigned long long alignsize = 0; + bool nvdimmPmem = false; /* The difference between @needHugepage and @useHugepage is that the latter * is true whenever huge page is defined for the current memory cell. @@ -2971,6 +2974,23 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, *backendProps = NULL; + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + pagesize = mem->s.dimm.pagesize; + needHugepage = !!pagesize; + useHugepage = !!pagesize; + nodemask = mem->s.dimm.sourceNodes; + break; + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + nvdimmPath = mem->s.nvdimm.path; + alignsize = mem->s.nvdimm.alignsize; + nvdimmPmem = mem->s.nvdimm.pmem; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } + if (mem->targetNode >= 0) { /* memory devices could provide a invalid guest node */ if (mem->targetNode >= virDomainNumaGetNodeCount(def->numa)) { @@ -3076,11 +3096,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) return -1; - } else if (useHugepage || mem->nvdimmPath || memAccess || - def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) { + } else if (useHugepage || nvdimmPath || memAccess || + def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) { - if (mem->nvdimmPath) { - memPath = g_strdup(mem->nvdimmPath); + if (nvdimmPath) { + memPath = g_strdup(nvdimmPath); prealloc = true; } else if (useHugepage) { if (qemuGetDomainHupageMemPath(priv->driver, def, pagesize, &memPath) < 0) @@ -3098,7 +3118,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, NULL) < 0) return -1; - if (!mem->nvdimmPath && + if (!nvdimmPath && discard == VIR_TRISTATE_BOOL_YES) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_DISCARD)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -3125,18 +3145,18 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0) return -1; - if (mem->alignsize) { + if (alignsize) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("nvdimm align property is not available " "with this QEMU binary")); return -1; } - if (virJSONValueObjectAdd(props, "U:align", mem->alignsize * 1024, NULL) < 0) + if (virJSONValueObjectAdd(props, "U:align", alignsize * 1024, NULL) < 0) return -1; } - if (mem->nvdimmPmem) { + if (nvdimmPmem) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("nvdimm pmem property is not available " @@ -3147,13 +3167,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, return -1; } - if (mem->sourceNodes) { - nodemask = mem->sourceNodes; - } else { - if (virDomainNumatuneMaybeGetNodeset(def->numa, priv->autoNodeset, - &nodemask, mem->targetNode) < 0) - return -1; - } + if (!nodemask && + virDomainNumatuneMaybeGetNodeset(def->numa, priv->autoNodeset, + &nodemask, mem->targetNode) < 0) + return -1; if (nodemask) { if (!virNumaNodesetIsAvailable(nodemask)) @@ -3166,8 +3183,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, } /* If none of the following is requested... */ - if (!needHugepage && !mem->sourceNodes && !nodeSpecified && - !mem->nvdimmPath && + if (!needHugepage && + !(mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM && + mem->s.dimm.sourceNodes) && + !nodeSpecified && + !nvdimmPath && memAccess == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE && def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_MEMFD && diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 1002455ddf..b8aebd1e61 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -354,7 +354,7 @@ qemuDomainSetupMemory(virDomainMemoryDefPtr mem, if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) return 0; - return virStringListAdd(paths, mem->nvdimmPath); + return virStringListAdd(paths, mem->s.nvdimm.path); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 20e90026e1..8ea7e0df05 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3885,15 +3885,15 @@ qemuProcessNeedHugepagesPath(virDomainDefPtr def, for (i = 0; i < def->nmems; i++) { if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_DIMM && - def->mems[i]->pagesize && - def->mems[i]->pagesize != system_pagesize) + def->mems[i]->s.dimm.pagesize && + def->mems[i]->s.dimm.pagesize != system_pagesize) return true; } if (mem && mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM && - mem->pagesize && - mem->pagesize != system_pagesize) + mem->s.dimm.pagesize && + mem->s.dimm.pagesize != system_pagesize) return true; return false; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index eed66e460f..78136e751e 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -686,13 +686,13 @@ AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - if (!virFileExists(mem->nvdimmPath)) { + if (!virFileExists(mem->s.nvdimm.path)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: \'%s\' does not exist"), - __func__, mem->nvdimmPath); + __func__, mem->s.nvdimm.path); return -1; } - return reload_profile(mgr, def, mem->nvdimmPath, true); + return reload_profile(mgr, def, mem->s.nvdimm.path, true); case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4f4a0a069e..44ee42e1bd 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1889,7 +1889,7 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr, switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - ret = virSecurityDACRestoreFileLabel(mgr, mem->nvdimmPath); + ret = virSecurityDACRestoreFileLabel(mgr, mem->s.nvdimm.path); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -2070,7 +2070,7 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, return -1; ret = virSecurityDACSetOwnership(mgr, NULL, - mem->nvdimmPath, + mem->s.nvdimm.path, user, group, true); break; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e9cd95916e..294c9f1db5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1577,7 +1577,7 @@ virSecuritySELinuxSetMemoryLabel(virSecurityManagerPtr mgr, if (!seclabel || !seclabel->relabel) return 0; - if (virSecuritySELinuxSetFilecon(mgr, mem->nvdimmPath, + if (virSecuritySELinuxSetFilecon(mgr, mem->s.nvdimm.path, seclabel->imagelabel, true) < 0) return -1; break; @@ -1606,7 +1606,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr, if (!seclabel || !seclabel->relabel) return 0; - ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath, true); + ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->s.nvdimm.path, true); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5a6f4a5f7d..a8a05a0a90 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1170,7 +1170,7 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->nmems; i++) { if (ctl->def->mems[i] && ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { - if (vah_add_file(&buf, ctl->def->mems[i]->nvdimmPath, "rw") != 0) + if (vah_add_file(&buf, ctl->def->mems[i]->s.nvdimm.path, "rw") != 0) goto cleanup; } } -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:14 +0100, Michal Privoznik wrote:
The structure has two sets of members: some for the target and some for the source. The latter is model dependant (e.g. path to a nvdimm device applies only to NVDIMM model). Move the members into an union so that it is obvious which members apply to which model. This way it's easier to maintain code cleanliness when introducing a new model.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 57 ++++++++++++++++++------------ src/conf/domain_conf.h | 16 ++++++--- src/qemu/qemu_alias.c | 13 +++++-- src/qemu/qemu_cgroup.c | 10 +++--- src/qemu/qemu_command.c | 60 +++++++++++++++++++++----------- src/qemu/qemu_namespace.c | 2 +- src/qemu/qemu_process.c | 8 ++--- src/security/security_apparmor.c | 6 ++-- src/security/security_dac.c | 4 +-- src/security/security_selinux.c | 4 +-- src/security/virt-aa-helper.c | 2 +- 11 files changed, 113 insertions(+), 69 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7d9e5d14ad..4d462b0627 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index dcb6c7156d..5ebcd766a9 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -486,15 +486,22 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, size_t i; int maxidx = 0; int idx; - const char *prefix; + const char *prefix = NULL;
if (mem->info.alias) return 0;
- if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM) + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: prefix = "dimm"; - else + break; + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: prefix = "nvdimm"; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + }
Previously 'prefix' was guaranteed non-null in this function. This is no longer true with this patch and 'prefix' will be used later without a check. You need to add a 'default:' case and report enum error from the switch to prevent that.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eb64ce84da..4bd45e0638 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2958,10 +2958,13 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, int rc; g_autoptr(virJSONValue) props = NULL; bool nodeSpecified = virDomainNumatuneNodeSpecified(def->numa, mem->targetNode); - unsigned long long pagesize = mem->pagesize; - bool needHugepage = !!pagesize; - bool useHugepage = !!pagesize; + unsigned long long pagesize = 0; + bool needHugepage = false; + bool useHugepage = false; int discard = mem->discard; + const char *nvdimmPath = NULL; + unsigned long long alignsize = 0; + bool nvdimmPmem = false;
/* The difference between @needHugepage and @useHugepage is that the latter * is true whenever huge page is defined for the current memory cell. @@ -2971,6 +2974,23 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
*backendProps = NULL;
+ switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + pagesize = mem->s.dimm.pagesize; + needHugepage = !!pagesize; + useHugepage = !!pagesize; + nodemask = mem->s.dimm.sourceNodes; + break; + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + nvdimmPath = mem->s.nvdimm.path; + alignsize = mem->s.nvdimm.alignsize; + nvdimmPmem = mem->s.nvdimm.pmem; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + }
Add default/error as above. Also this is not straight conversion to enum here. The scope of changes was fairly limited in previous hunks, but here we are modifying multiple variables and adding new ones. Peferrably, do such conversion prior to the refactor next time.
if (mem->targetNode >= 0) { /* memory devices could provide a invalid guest node */ if (mem->targetNode >= virDomainNumaGetNodeCount(def->numa)) {
[...]
@@ -3125,18 +3145,18 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0) return -1;
- if (mem->alignsize) { + if (alignsize) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("nvdimm align property is not available " "with this QEMU binary")); return -1; } - if (virJSONValueObjectAdd(props, "U:align", mem->alignsize * 1024, NULL) < 0) + if (virJSONValueObjectAdd(props, "U:align", alignsize * 1024, NULL) < 0) return -1; }
- if (mem->nvdimmPmem) { + if (nvdimmPmem) {
This variable is read only once.
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("nvdimm pmem property is not available " @@ -3147,13 +3167,10 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, return -1; }
- if (mem->sourceNodes) { - nodemask = mem->sourceNodes; - } else { - if (virDomainNumatuneMaybeGetNodeset(def->numa, priv->autoNodeset, - &nodemask, mem->targetNode) < 0) - return -1; - } + if (!nodemask && + virDomainNumatuneMaybeGetNodeset(def->numa, priv->autoNodeset, + &nodemask, mem->targetNode) < 0) + return -1;
if (nodemask) { if (!virNumaNodesetIsAvailable(nodemask)) @@ -3166,8 +3183,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, }
/* If none of the following is requested... */ - if (!needHugepage && !mem->sourceNodes && !nodeSpecified && - !mem->nvdimmPath && + if (!needHugepage && + !(mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM && + mem->s.dimm.sourceNodes) && + !nodeSpecified && + !nvdimmPath && memAccess == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE && def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_MEMFD &&
Mixing of two refactors made this patch unnecessarily complex to review. The patch itself is correct and I don't really want to see it again. Thus you can commit it as-is provided that you follow up with a patch which properly refactors qemuBuildMemoryBackendProps now that we have a switch statement and aggregate code which is relevant for only one of the memory models under the appropriate cases. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The virtio-pmem is a virtio variant of NVDIMM and just like NVDIMM virtio-pmem also allows accessing host pages bypassing guest page cache. The difference is that if a regular file is used to back guest's NVDIMM (model='nvdimm') the persistence of guest writes might not be guaranteed while with virtio-pmem it is. To express this new model at domain XML level, I've chose the following: <memory model='virtio' access='shared'> <source> <path>/tmp/virtio_pmem</path> <pmem/> </source> <target> <size unit='KiB'>524288</size> </target> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </memory> The <source/> looks like for model='nvdimm', except <pmem/> is required - this is to allow for future expansion when the model='virtio' without <pmem/> will describe virtio-mem (which is a different device). Another difference between NVDIMM and virtio-pmem is that while the former supports NUMA node locality the latter doesn't. And also, the latter goes onto PCI bus and not into a DIMM module. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 35 +++++- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 113 ++++++++++++++++-- src/conf/domain_conf.h | 5 + src/qemu/qemu_alias.c | 1 + src/qemu/qemu_command.c | 2 + src/qemu/qemu_domain.c | 26 ++++ src/qemu/qemu_domain_address.c | 88 +++++++++++--- src/qemu/qemu_domain_address.h | 3 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_validate.c | 1 + src/security/security_apparmor.c | 1 + src/security/security_dac.c | 2 + src/security/security_selinux.c | 2 + .../memory-hotplug-virtio-pmem.xml | 54 +++++++++ ...mory-hotplug-virtio-pmem.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 2 + 17 files changed, 304 insertions(+), 35 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.xml create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-virtio-pmem.x86_64-latest.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 512939679b..ca6bc0432e 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -7187,20 +7187,31 @@ Example: usage of the memory devices </label> </target> </memory> + <memory model='virtio' access='shared'> + <source> + <path>/tmp/virtio_pmem</path> + <pmem/> + </source> + <target> + <size unit='KiB'>524288</size> + </target> + </memory> </devices> ... ``model`` Provide ``dimm`` to add a virtual DIMM module to the guest. :since:`Since - 1.2.14` Provide ``nvdimm`` model adds a Non-Volatile DIMM module. - :since:`Since 3.2.0` + 1.2.14` Provide ``nvdimm`` model that adds a Non-Volatile DIMM module. + :since:`Since 3.2.0` Provide ``virtio`` model to add a paravirtualized + (persistent) memory device. :since:`Since 7.0.0` ``access`` An optional attribute ``access`` ( :since:`since 3.2.0` ) that provides capability to fine tune mapping of the memory on per module basis. Values are the same as `Memory Backing <#elementsMemoryBacking>`__: ``shared`` and ``private``. For ``nvdimm`` model, if using real NVDIMM DAX device as - backend, ``shared`` is required. + backend, ``shared`` is required. For ``virtio`` model with ``<pmem/>`` + ``shared`` is required. ``discard`` An optional attribute ``discard`` ( :since:`since 4.4.0` ) that provides @@ -7229,9 +7240,9 @@ Example: usage of the memory devices This element can be used to override the default set of NUMA nodes where the memory would be allocated. - For model ``nvdimm`` this element is mandatory. The mandatory child element - ``path`` represents a path in the host that backs the nvdimm module in the - guest. The following optional elements may be used: + For model ``nvdimm`` the ``source`` element is mandatory. The mandatory + child element ``path`` represents a path in the host that backs the nvdimm + module in the guest. The following optional elements may be used: ``alignsize`` The ``alignsize`` element defines the page size alignment used to mmap the @@ -7245,6 +7256,18 @@ Example: usage of the memory devices to guarantee the persistence of writes to the vNVDIMM backend, then use the ``pmem`` element in order to utilize the feature. :since:`Since 5.0.0` + For model ``virtio`` the ``source`` element is mandatory. The following + optional elements may be used: + + ``path`` + Represents a path in the host that backs the virtio memory module in the + guest. It is mandatory for persistent memory (i.e. when ``pmem`` is also + set). + + ``pmem`` + If persistence of data is required then this element must be provided. + + ``target`` The mandatory ``target`` element configures the placement and sizing of the added memory from the perspective of the guest. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 795b654feb..b385bae84c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5973,6 +5973,7 @@ <choice> <value>dimm</value> <value>nvdimm</value> + <value>virtio</value> </choice> </attribute> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4d462b0627..935bea1804 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1332,6 +1332,7 @@ VIR_ENUM_IMPL(virDomainMemoryModel, "", "dimm", "nvdimm", + "virtio", ); VIR_ENUM_IMPL(virDomainShmemModel, @@ -3143,6 +3144,9 @@ void virDomainMemoryDefFree(virDomainMemoryDefPtr def) case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: VIR_FREE(def->s.nvdimm.path); break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + VIR_FREE(def->s.virtio.path); + break; case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; @@ -5373,15 +5377,31 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + /* Virtio-pmem mandates shared access so that guest writes get + * reflected in the underlying file. */ + if (mem->s.virtio.pmem && + mem->access == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT) + mem->access = VIR_DOMAIN_MEMORY_ACCESS_SHARED; + break; + + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } return 0; } @@ -6703,7 +6723,8 @@ static int virDomainMemoryDefValidate(const virDomainMemoryDef *mem, const virDomainDef *def) { - if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (!mem->s.nvdimm.path) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("path is required for model 'nvdimm'")); @@ -6721,6 +6742,39 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, _("label size is required for NVDIMM device")); return -1; } + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (mem->s.virtio.pmem) { + if (!mem->s.virtio.path) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("path is required for model 'virtio'")); + return -1; + } + + if (mem->access != VIR_DOMAIN_MEMORY_ACCESS_SHARED) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("shared access mode required for virtio-pmem device")); + return -1; + } + + if (mem->targetNode != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio-pmem does not support NUMA nodes")); + return -1; + } + } else { + /* TODO: plain virtio-mem behaves differently then virtio-pmem */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio-mem is not supported yet. <pmem/> is required")); + return -1; + } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; } return 0; @@ -16717,6 +16771,14 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + def->s.virtio.path = virXPathString("string(./path)", ctxt); + + if (virXPathBoolean("boolean(./pmem)", ctxt)) + def->s.virtio.pmem = true; + + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; @@ -18605,6 +18667,12 @@ virDomainMemoryFindByDefInternal(virDomainDefPtr def, continue; break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (tmp->s.virtio.pmem != mem->s.virtio.pmem || + STRNEQ_NULLABLE(tmp->s.virtio.path, mem->s.virtio.path)) + continue; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; @@ -24187,7 +24255,8 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, return false; } - if (src->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + switch (src->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (src->labelsize != dst->labelsize) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target NVDIMM label size '%llu' doesn't match " @@ -24223,6 +24292,21 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, _("Target NVDIMM UUID doesn't match source NVDIMM")); return false; } + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (src->s.virtio.pmem != dst->s.virtio.pmem) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target NVDIMM pmem flag doesn't match " + "source NVDIMM pmem flag")); + return false; + } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; } return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); @@ -27878,6 +27962,13 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, virBufferAddLit(&childBuf, "<pmem/>\n"); break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + virBufferEscapeString(&childBuf, "<path>%s</path>\n", def->s.virtio.path); + + if (def->s.virtio.pmem) + virBufferAddLit(&childBuf, "<pmem/>\n"); + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e7f8fc156f..efaa4c5473 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2306,6 +2306,7 @@ typedef enum { VIR_DOMAIN_MEMORY_MODEL_NONE, VIR_DOMAIN_MEMORY_MODEL_DIMM, /* dimm hotpluggable memory device */ VIR_DOMAIN_MEMORY_MODEL_NVDIMM, /* nvdimm memory device */ + VIR_DOMAIN_MEMORY_MODEL_VIRTIO, /* virtio-{p}mem memory device */ VIR_DOMAIN_MEMORY_MODEL_LAST } virDomainMemoryModel; @@ -2325,6 +2326,10 @@ struct _virDomainMemoryDef { unsigned long long alignsize; /* kibibytes */ bool pmem; } nvdimm; /* VIR_DOMAIN_MEMORY_MODEL_NVDIMM */ + struct { + char *path; /* Required for pmem, otherwise optional */ + bool pmem; + } virtio; /* VIR_DOMAIN_MEMORY_MODEL_VIRTIO */ } s; /* target */ diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 5ebcd766a9..931dbd4e84 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -498,6 +498,7 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: prefix = "nvdimm"; break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4bd45e0638..9c50778180 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2986,6 +2986,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, alignsize = mem->s.nvdimm.alignsize; nvdimmPmem = mem->s.nvdimm.pmem; break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; @@ -3339,6 +3340,7 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem) break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e705e8d8d5..ab7938a355 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8485,6 +8485,8 @@ static int qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, const virDomainDef *def) { + bool needsNuma = true; + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: @@ -8520,11 +8522,35 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, } break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only 'pci' addresses are supported for the " + "virtio-pmem device")); + return -1; + } + + /* virtio-pmem doesn't have .node attribute. */ + if (mem->s.virtio.pmem) + needsNuma = false; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: return -1; } + if (needsNuma && + virDomainNumaGetNodeCount(def->numa) != 0) { + if (mem->targetNode == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("target NUMA node needs to be specified for " + "memory device")); + return -1; + } + } + return 0; } diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index d872f75b38..bcf6724da3 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -313,11 +313,11 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, virDomainDeviceAddressType type) { /* - declare address-less virtio devices to be of address type 'type' - disks, networks, videos, consoles, controllers, memballoon and rng - in this order - if type is ccw filesystem and vsock devices are declared to be of - address type ccw + Declare address-less virtio devices to be of address type 'type' + disks, networks, videos, consoles, controllers, hostdevs, memballoon, + rngs and memories in this order. + If type is ccw filesystem and vsock devices are declared to be of + address type ccw. */ size_t i; @@ -379,6 +379,12 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, def->rngs[i]->info.type = type; } + for (i = 0; i < def->nmems; i++) { + if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO && + def->mems[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->mems[i]->info.type = type; + } + if (type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { for (i = 0; i < def->nfss; i++) { if (def->fss[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) @@ -1040,11 +1046,23 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, } break; + case VIR_DOMAIN_DEVICE_MEMORY: + switch (dev->data.memory->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + return virtioFlags; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + return 0; + } + break; + /* These devices don't ever connect with PCI */ case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_SMARTCARD: @@ -2455,6 +2473,17 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, return -1; } + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr mem = def->mems[i]; + + if (mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO || + !virDeviceInfoPCIAddressIsWanted(&mem->info)) + continue; + + if (qemuDomainPCIAddressReserveNextAddr(addrs, &mem->info) < 0) + return -1; + } + return 0; } @@ -3102,19 +3131,32 @@ qemuAssignMemoryDeviceSlot(virDomainMemoryDefPtr mem, int -qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def, +qemuDomainAssignMemoryDeviceSlot(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainMemoryDefPtr mem) { - virBitmapPtr slotmap = NULL; - int ret; + g_autoptr(virBitmap) slotmap = NULL; + virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_MEMORY, .data.memory = mem}; - if (!(slotmap = qemuDomainGetMemorySlotMap(def))) - return -1; + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (!(slotmap = qemuDomainGetMemorySlotMap(vm->def))) + return -1; - ret = qemuAssignMemoryDeviceSlot(mem, slotmap); + return qemuAssignMemoryDeviceSlot(mem, slotmap); + break; - virBitmapFree(slotmap); - return ret; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + return qemuDomainEnsurePCIAddress(vm, &dev, driver); + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } + + return 0; } @@ -3132,8 +3174,22 @@ qemuDomainAssignMemorySlots(virDomainDefPtr def) return -1; for (i = 0; i < def->nmems; i++) { - if (qemuAssignMemoryDeviceSlot(def->mems[i], slotmap) < 0) - goto cleanup; + virDomainMemoryDefPtr mem = def->mems[i]; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (qemuAssignMemoryDeviceSlot(def->mems[i], slotmap) < 0) + goto cleanup; + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + /* handled in qemuDomainAssignPCIAddresses() */ + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } } ret = 0; diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index 7ef3308246..20a46160d5 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -56,7 +56,8 @@ void qemuDomainFillDeviceIsolationGroup(virDomainDefPtr def, void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, virDomainDeviceInfoPtr info); -int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def, +int qemuDomainAssignMemoryDeviceSlot(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainMemoryDefPtr mem); int qemuDomainEnsureVirtioAddress(bool *releaseAddr, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2c12ef60af..1f9df182bf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2402,7 +2402,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0) goto cleanup; - if (qemuDomainAssignMemoryDeviceSlot(vm->def, mem) < 0) + if (qemuDomainAssignMemoryDeviceSlot(driver, vm, mem) < 0) goto cleanup; /* in cases where we are using a VM with aliases generated according to the diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 8ceea022d7..fc9bc7a5b4 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4620,6 +4620,7 @@ qemuValidateDomainDeviceDefMemory(virDomainMemoryDefPtr mem, } break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 78136e751e..b22ee739d8 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -693,6 +693,7 @@ AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, return -1; } return reload_profile(mgr, def, mem->s.nvdimm.path, true); + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 44ee42e1bd..6b681c4021 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1892,6 +1892,7 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr, ret = virSecurityDACRestoreFileLabel(mgr, mem->s.nvdimm.path); break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: case VIR_DOMAIN_MEMORY_MODEL_NONE: @@ -2074,6 +2075,7 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, user, group, true); break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: case VIR_DOMAIN_MEMORY_MODEL_NONE: diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 294c9f1db5..77b69447da 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1582,6 +1582,7 @@ virSecuritySELinuxSetMemoryLabel(virSecurityManagerPtr mgr, return -1; break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: @@ -1609,6 +1610,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr, ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->s.nvdimm.path, true); break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.xml b/tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.xml new file mode 100644 index 0000000000..3570b7d1b1 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.xml @@ -0,0 +1,54 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>2095104</memory> + <currentMemory unit='KiB'>2095104</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + <topology sockets='2' dies='1' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='2095104' 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-system-i386</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <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'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + <memory model='virtio' access='shared'> + <source> + <path>/tmp/virtio_pmem</path> + <pmem/> + </source> + <target> + <size unit='KiB'>524288</size> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-virtio-pmem.x86_64-latest.xml b/tests/qemuxml2xmloutdata/memory-hotplug-virtio-pmem.x86_64-latest.xml new file mode 120000 index 0000000000..904425abe4 --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-virtio-pmem.x86_64-latest.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/memory-hotplug-virtio-pmem.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c006719dee..e804db8aee 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1238,6 +1238,8 @@ mymain(void) QEMU_CAPS_DEVICE_NVDIMM_UNARMED); DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_DEVICE_NVDIMM); + DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem"); + DO_TEST("net-udp", NONE); DO_TEST("video-virtio-gpu-device", QEMU_CAPS_DEVICE_VIRTIO_GPU); -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:15 +0100, Michal Privoznik wrote:
The virtio-pmem is a virtio variant of NVDIMM and just like NVDIMM virtio-pmem also allows accessing host pages bypassing guest page cache. The difference is that if a regular file is used to back guest's NVDIMM (model='nvdimm') the persistence of guest writes might not be guaranteed while with virtio-pmem it is.
To express this new model at domain XML level, I've chose the following:
<memory model='virtio' access='shared'> <source> <path>/tmp/virtio_pmem</path> <pmem/> </source> <target> <size unit='KiB'>524288</size> </target> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </memory>
The <source/> looks like for model='nvdimm', except <pmem/> is required - this is to allow for future expansion when the model='virtio' without <pmem/> will describe virtio-mem (which is a different device).
This is not a good design. You use a property of <source> which describes the backend to influence the model of the device, which is definitely a frontend property as witnessed also by the ABI stability check: [I'm moving some hunks around for locality of my point]
@@ -24223,6 +24292,21 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, _("Target NVDIMM UUID doesn't match source NVDIMM")); return false; } + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (src->s.virtio.pmem != dst->s.virtio.pmem) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target NVDIMM pmem flag doesn't match " + "source NVDIMM pmem flag"));
(also please error messages with no line breaks)
+ return false; + } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; }
return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
@@ -6721,6 +6742,39 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, _("label size is required for NVDIMM device")); return -1; } + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (mem->s.virtio.pmem) { + if (!mem->s.virtio.path) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("path is required for model 'virtio'")); + return -1; + } + + if (mem->access != VIR_DOMAIN_MEMORY_ACCESS_SHARED) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("shared access mode required for virtio-pmem device")); + return -1; + } + + if (mem->targetNode != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio-pmem does not support NUMA nodes")); + return -1; + } + } else { + /* TODO: plain virtio-mem behaves differently then virtio-pmem */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio-mem is not supported yet. <pmem/> is required")); + return -1;
This also doesn't seem to inspire confidence in the design. I think we'll need to use 'virtio-mem' and 'virtio-pmem' for the models.
+ } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; }
return 0;
[...]
Another difference between NVDIMM and virtio-pmem is that while the former supports NUMA node locality the latter doesn't. And also, the latter goes onto PCI bus and not into a DIMM module.
I think you've used enough arguments to avoid starting the commit message that it's a variant of nvdimm :). It's similar in usage, but definitely very dissimilar in implementation. [...]
@@ -5373,15 +5377,31 @@ static int virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, const virDomainDef *def) { - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + /* Although only the QEMU driver implements PPC64 support, this + * code is related to the platform specification (PAPR), i.e. it + * is hypervisor agnostic, and any future PPC64 hypervisor driver + * will have the same restriction. + */ + if (ARCH_IS_PPC64(def->os.arch) && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + /* Virtio-pmem mandates shared access so that guest writes get + * reflected in the underlying file. */
I'd expect a check that 'access' is not 'VIR_DOMAIN_MEMORY_ACCESS_PRIVATE' explicitly if it's mandatory to use shared access.
+ if (mem->s.virtio.pmem && + mem->access == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT) + mem->access = VIR_DOMAIN_MEMORY_ACCESS_SHARED; + break; + + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + }
return 0; }
[...] [...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e705e8d8d5..ab7938a355 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8485,6 +8485,8 @@ static int qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, const virDomainDef *def) { + bool needsNuma = true; + switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: @@ -8520,11 +8522,35 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, } break;
+ case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only 'pci' addresses are supported for the " + "virtio-pmem device")); + return -1; + } + + /* virtio-pmem doesn't have .node attribute. */ + if (mem->s.virtio.pmem) + needsNuma = false; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: return -1; }
+ if (needsNuma && + virDomainNumaGetNodeCount(def->numa) != 0) { + if (mem->targetNode == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("target NUMA node needs to be specified for " + "memory device")); + return -1; + } + } + return 0; }
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index d872f75b38..bcf6724da3 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -313,11 +313,11 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, virDomainDeviceAddressType type) { /* - declare address-less virtio devices to be of address type 'type' - disks, networks, videos, consoles, controllers, memballoon and rng - in this order - if type is ccw filesystem and vsock devices are declared to be of - address type ccw + Declare address-less virtio devices to be of address type 'type' + disks, networks, videos, consoles, controllers, hostdevs, memballoon, + rngs and memories in this order. + If type is ccw filesystem and vsock devices are declared to be of + address type ccw. */ size_t i;
@@ -379,6 +379,12 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, def->rngs[i]->info.type = type; }
+ for (i = 0; i < def->nmems; i++) { + if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO && + def->mems[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->mems[i]->info.type = type; + } + if (type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { for (i = 0; i < def->nfss; i++) { if (def->fss[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) @@ -1040,11 +1046,23 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, } break;
+ case VIR_DOMAIN_DEVICE_MEMORY: + switch (dev->data.memory->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + return virtioFlags; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + return 0; + } + break; + /* These devices don't ever connect with PCI */ case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_SMARTCARD: @@ -2455,6 +2473,17 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, return -1; }
+ for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr mem = def->mems[i]; + + if (mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO || + !virDeviceInfoPCIAddressIsWanted(&mem->info)) + continue; + + if (qemuDomainPCIAddressReserveNextAddr(addrs, &mem->info) < 0) + return -1; + } + return 0; }
@@ -3102,19 +3131,32 @@ qemuAssignMemoryDeviceSlot(virDomainMemoryDefPtr mem,
int -qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def, +qemuDomainAssignMemoryDeviceSlot(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainMemoryDefPtr mem) { - virBitmapPtr slotmap = NULL; - int ret; + g_autoptr(virBitmap) slotmap = NULL; + virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_MEMORY, .data.memory = mem};
- if (!(slotmap = qemuDomainGetMemorySlotMap(def))) - return -1; + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (!(slotmap = qemuDomainGetMemorySlotMap(vm->def))) + return -1;
- ret = qemuAssignMemoryDeviceSlot(mem, slotmap); + return qemuAssignMemoryDeviceSlot(mem, slotmap); + break;
- virBitmapFree(slotmap); - return ret; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + return qemuDomainEnsurePCIAddress(vm, &dev, driver); + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } + + return 0; }
@@ -3132,8 +3174,22 @@ qemuDomainAssignMemorySlots(virDomainDefPtr def) return -1;
for (i = 0; i < def->nmems; i++) { - if (qemuAssignMemoryDeviceSlot(def->mems[i], slotmap) < 0) - goto cleanup; + virDomainMemoryDefPtr mem = def->mems[i]; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + if (qemuAssignMemoryDeviceSlot(def->mems[i], slotmap) < 0) + goto cleanup; + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + /* handled in qemuDomainAssignPCIAddresses() */ + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; + } }
Preferably this commit will not introduce actual code to the qemu driver. Please do this refactor separately, before introduction of the new model. This applies to all of the above hunks.
In this commit two new capabilities are introduced that reflect device support by give qemu: QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI, /* -device virtio-mem-pci */ QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI, /* -device virtio-pmem-pci */ Please note, virtio-pmem device was introduced sooner (QEMU 4.1) than virtio-mem (QEMU 5.1). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 2 ++ tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 ++ tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 2 ++ 7 files changed, 13 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4d132defbd..14e6892fb2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -609,6 +609,8 @@ VIR_ENUM_IMPL(virQEMUCaps, "ncr53c90", "dc390", "am53c974", + "virtio-mem-pci", + "virtio-pmem-pci", ); @@ -1325,6 +1327,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { */ { "dc390", QEMU_CAPS_SCSI_DC390 }, { "am53c974", QEMU_CAPS_SCSI_AM53C974 }, + { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI }, + { "virtio-pmem-pci", QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI }, }; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 0f90efa459..b21e56b1db 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -589,6 +589,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_SCSI_NCR53C90, /* built-in SCSI */ QEMU_CAPS_SCSI_DC390, /* -device dc-390 */ QEMU_CAPS_SCSI_AM53C974, /* -device am53c974 */ + QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI, /* -device virtio-mem-pci */ + QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI, /* -device virtio-pmem-pci */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml index 58774fddcc..28a4b0ede0 100644 --- a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml @@ -231,6 +231,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='virtio-pmem-pci'/> <version>4001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml index 1ba8c09374..e150741f11 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml @@ -240,6 +240,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='virtio-pmem-pci'/> <version>4002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml index 849727eb40..7c56d110f4 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml @@ -247,6 +247,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='virtio-pmem-pci'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml index ff5f42a563..ae8787981a 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -249,6 +249,8 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='virtio-mem-pci'/> + <flag name='virtio-pmem-pci'/> <version>5001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index 5e9fa8575a..f0892cdbe5 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -251,6 +251,8 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='virtio-mem-pci'/> + <flag name='virtio-pmem-pci'/> <version>5001091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:16 +0100, Michal Privoznik wrote:
In this commit two new capabilities are introduced that reflect device support by give qemu:
QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI, /* -device virtio-mem-pci */ QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI, /* -device virtio-pmem-pci */
Please note, virtio-pmem device was introduced sooner (QEMU 4.1) than virtio-mem (QEMU 5.1).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 2 ++ tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 ++ tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 2 ++ 7 files changed, 13 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_validate.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index fc9bc7a5b4..e4d12db3bd 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4621,6 +4621,21 @@ qemuValidateDomainDeviceDefMemory(virDomainMemoryDefPtr mem, break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (mem->s.virtio.pmem) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio-pmem isn't supported by this QEMU binary")); + return -1; + } + } else { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio-mem isn't supported by this QEMU binary")); + return -1; + } + } + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; -- 2.26.2
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_apparmor.c | 26 ++++++++++++-------- src/security/security_dac.c | 42 +++++++++++++++++--------------- src/security/security_selinux.c | 42 ++++++++++++++++++-------------- src/security/virt-aa-helper.c | 22 ++++++++++++++--- 4 files changed, 81 insertions(+), 51 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index b22ee739d8..8bf7570d4a 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -681,26 +681,32 @@ AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem) { - if (mem == NULL) - return 0; + const char *path = NULL; switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - if (!virFileExists(mem->s.nvdimm.path)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: \'%s\' does not exist"), - __func__, mem->s.nvdimm.path); - return -1; - } - return reload_profile(mgr, def, mem->s.nvdimm.path, true); + path = mem->s.nvdimm.path; + break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; } - return 0; + if (!path) + return 0; + + if (!virFileExists(path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: \'%s\' does not exist"), + __func__, path); + return -1; + } + + return reload_profile(mgr, def, path, true); } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 6b681c4021..24daa41898 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1885,22 +1885,25 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr def G_GNUC_UNUSED, virDomainMemoryDefPtr mem) { - int ret = -1; + const char *path = NULL; switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - ret = virSecurityDACRestoreFileLabel(mgr, mem->s.nvdimm.path); + path = mem->s.nvdimm.path; break; - case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: case VIR_DOMAIN_MEMORY_MODEL_NONE: - ret = 0; break; } - return ret; + if (!path) + return 0; + + return virSecurityDACRestoreFileLabel(mgr, path); } @@ -2057,33 +2060,34 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr seclabel; - int ret = -1; + const char *path = NULL; uid_t user; gid_t group; switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (seclabel && !seclabel->relabel) - return 0; - - if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) - return -1; - - ret = virSecurityDACSetOwnership(mgr, NULL, - mem->s.nvdimm.path, - user, group, true); + path = mem->s.nvdimm.path; break; - case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: case VIR_DOMAIN_MEMORY_MODEL_NONE: - ret = 0; break; } - return ret; + if (!path) + return 0; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + if (seclabel && !seclabel->relabel) + return 0; + + if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) + return -1; + + return virSecurityDACSetOwnership(mgr, NULL, path, user, group, true); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 77b69447da..c0f78f8a46 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1570,26 +1570,29 @@ virSecuritySELinuxSetMemoryLabel(virSecurityManagerPtr mgr, virDomainMemoryDefPtr mem) { virSecurityLabelDefPtr seclabel; + const char *path = NULL; switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!seclabel || !seclabel->relabel) - return 0; - - if (virSecuritySELinuxSetFilecon(mgr, mem->s.nvdimm.path, - seclabel->imagelabel, true) < 0) - return -1; + path = mem->s.nvdimm.path; break; - case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; } - return 0; + if (!path) + return 0; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!seclabel || !seclabel->relabel) + return 0; + + return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel, true); } @@ -1598,27 +1601,30 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem) { - int ret = -1; virSecurityLabelDefPtr seclabel; + const char *path = NULL; switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!seclabel || !seclabel->relabel) - return 0; - - ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->s.nvdimm.path, true); + path = mem->s.nvdimm.path; break; - case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: - ret = 0; break; } - return ret; + if (!path) + return 0; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!seclabel || !seclabel->relabel) + return 0; + + return virSecuritySELinuxRestoreFileLabel(mgr, path, true); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a8a05a0a90..f895fecea4 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1168,11 +1168,25 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->nmems; i++) { - if (ctl->def->mems[i] && - ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { - if (vah_add_file(&buf, ctl->def->mems[i]->s.nvdimm.path, "rw") != 0) - goto cleanup; + virDomainMemoryDefPtr mem = ctl->def->mems[i]; + const char *path = NULL; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + path = mem->s.nvdimm.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + break; } + + if (path && + vah_add_file(&buf, path, "rw") != 0) + goto cleanup; } for (i = 0; i < ctl->def->nsysinfo; i++) { -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:18 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_apparmor.c | 26 ++++++++++++-------- src/security/security_dac.c | 42 +++++++++++++++++--------------- src/security/security_selinux.c | 42 ++++++++++++++++++-------------- src/security/virt-aa-helper.c | 22 ++++++++++++++--- 4 files changed, 81 insertions(+), 51 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index b22ee739d8..8bf7570d4a 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -681,26 +681,32 @@ AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem) { - if (mem == NULL) - return 0;
Removal of this NULL check is not justified in the commit message and also really seems to be a separate cleanup if it is actually justified.
+ const char *path = NULL;
switch (mem->model) {
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Some users might wanto have virtio-pmem backed by a block device in which case we have to allow the device in CGroups. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 43 ++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 92caadf840..b5639de93e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -500,19 +500,32 @@ qemuSetupMemoryDevicesCgroup(virDomainObjPtr vm, virDomainMemoryDefPtr mem) { qemuDomainObjPrivatePtr priv = vm->privateData; + const char *path = NULL; int rv; - if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + path = mem->s.nvdimm.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + break; + } + + if (!path) return 0; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - VIR_DEBUG("Setting devices Cgroup for NVDIMM device: %s", mem->s.nvdimm.path); - rv = virCgroupAllowDevicePath(priv->cgroup, mem->s.nvdimm.path, + VIR_DEBUG("Setting devices Cgroup for memory device: %s", path); + rv = virCgroupAllowDevicePath(priv->cgroup, path, VIR_CGROUP_DEVICE_RW, false); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", - mem->s.nvdimm.path, "rw", rv); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rv); return rv; } @@ -523,18 +536,28 @@ qemuTeardownMemoryDevicesCgroup(virDomainObjPtr vm, virDomainMemoryDefPtr mem) { qemuDomainObjPrivatePtr priv = vm->privateData; + const char *path = NULL; int rv; - if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) - return 0; + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + path = mem->s.nvdimm.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + break; + } if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - rv = virCgroupDenyDevicePath(priv->cgroup, mem->s.nvdimm.path, + rv = virCgroupDenyDevicePath(priv->cgroup, path, VIR_CGROUP_DEVICE_RWM, false); - virDomainAuditCgroupPath(vm, priv->cgroup, - "deny", mem->s.nvdimm.path, "rwm", rv); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rv); return rv; } -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:19 +0100, Michal Privoznik wrote:
Some users might wanto have virtio-pmem backed by a block device in which case we have to allow the device in CGroups.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 43 ++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Some users might wanto have virtio-pmem backed by a block device in which case we have to create the device in domain namespace. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_namespace.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index b8aebd1e61..360f48a2fc 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -351,10 +351,25 @@ static int qemuDomainSetupMemory(virDomainMemoryDefPtr mem, char ***paths) { - if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM) + const char *path = NULL; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + path = mem->s.nvdimm.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + path = mem->s.virtio.path; + break; + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + break; + } + + if (!path) return 0; - return virStringListAdd(paths, mem->s.nvdimm.path); + return virStringListAdd(paths, path); } -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:20 +0100, Michal Privoznik wrote:
Some users might wanto have virtio-pmem backed by a block device in which case we have to create the device in domain namespace.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_namespace.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far our memory modules could go only into DIMM slots. But with virtio model this assumption is no longer true - virtio-pmem goes onto PCI bus. But for formatting PCI address onto command line we already have a function - qemuBuildDeviceAddressStr(). Therefore, mode DIMM address generation into it so that we don't have to special case address building later on. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 17 +++++++++-------- src/qemu/qemu_command.h | 5 ++++- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9c50778180..49241fc507 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -393,6 +393,10 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x", info->addr.isa.iobase, info->addr.isa.irq); + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + virBufferAsprintf(buf, ",slot=%d", info->addr.dimm.slot); + if (info->addr.dimm.base) + virBufferAsprintf(buf, ",addr=%llu", info->addr.dimm.base); } return 0; @@ -3290,7 +3294,9 @@ qemuBuildMemoryDimmBackendStr(virBufferPtr buf, char * -qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem) +qemuBuildMemoryDeviceStr(const virDomainDef *def, + virDomainMemoryDefPtr mem, + virQEMUCapsPtr qemuCaps) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const char *device; @@ -3332,12 +3338,7 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem) virBufferAsprintf(&buf, "memdev=mem%s,id=%s", mem->info.alias, mem->info.alias); - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { - virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot); - if (mem->info.addr.dimm.base) - virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base); - } - + qemuBuildDeviceAddressStr(&buf, def, &mem->info, qemuCaps); break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: @@ -7478,7 +7479,7 @@ qemuBuildMemoryDeviceCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, "-object"); virCommandAddArgBuffer(cmd, &buf); - if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i]))) + if (!(dimmStr = qemuBuildMemoryDeviceStr(def, def->mems[i], priv->qemuCaps))) return -1; virCommandAddArgList(cmd, "-device", dimmStr, NULL); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 3da07e25a1..3cfe6ff3e9 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -155,7 +155,10 @@ int qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, const virDomainMemoryDef *mem, bool force); -char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem); +char * +qemuBuildMemoryDeviceStr(const virDomainDef *def, + virDomainMemoryDefPtr mem, + virQEMUCapsPtr qemuCaps); /* Current, best practice */ char *qemuBuildPCIHostdevDevStr(const virDomainDef *def, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1f9df182bf..bc5dc02a2f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2412,7 +2412,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, objalias = g_strdup_printf("mem%s", mem->info.alias); - if (!(devstr = qemuBuildMemoryDeviceStr(mem))) + if (!(devstr = qemuBuildMemoryDeviceStr(vm->def, mem, priv->qemuCaps))) goto cleanup; if (qemuBuildMemoryBackendProps(&props, objalias, cfg, -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:21 +0100, Michal Privoznik wrote:
So far our memory modules could go only into DIMM slots. But with virtio model this assumption is no longer true - virtio-pmem goes onto PCI bus. But for formatting PCI address onto command line we already have a function - qemuBuildDeviceAddressStr(). Therefore, mode DIMM address generation into it so that we don't have to special case address building later on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 17 +++++++++-------- src/qemu/qemu_command.h | 5 ++++- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9c50778180..49241fc507 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -393,6 +393,10 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x", info->addr.isa.iobase, info->addr.isa.irq); + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + virBufferAsprintf(buf, ",slot=%d", info->addr.dimm.slot); + if (info->addr.dimm.base) + virBufferAsprintf(buf, ",addr=%llu", info->addr.dimm.base); }
This function would really benefit from a refactor to a switch. Reviewed-by: Peter Krempa <pkrempa@redhat.com> This cleanup should be applicable even without the virtio-mem patches.
Now we have everything prepared for generating the command line. The device alias prefix was chosen to be 'virtiopmem'. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1735375 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 44 ++++++---- src/qemu/qemu_command.c | 82 ++++++++++--------- ...ory-hotplug-virtio-pmem.x86_64-latest.args | 45 ++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 120 insertions(+), 52 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 931dbd4e84..68d4a7b480 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -466,6 +466,28 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def, } +static int +qemuDeviceMemoryGetAliasID(virDomainDefPtr def, + virDomainMemoryDefPtr mem, + bool oldAlias, + const char *prefix) +{ + size_t i; + int maxidx = 0; + + if (!oldAlias) + return mem->info.addr.dimm.slot; + + for (i = 0; i < def->nmems; i++) { + int idx; + if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, prefix)) >= maxidx) + maxidx = idx + 1; + } + + return maxidx; +} + + /** * qemuAssignDeviceMemoryAlias: * @def: domain definition. Necessary only if @oldAlias is true. @@ -483,10 +505,8 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, virDomainMemoryDefPtr mem, bool oldAlias) { - size_t i; - int maxidx = 0; - int idx; const char *prefix = NULL; + int idx = 0; if (mem->info.alias) return 0; @@ -494,26 +514,22 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: prefix = "dimm"; + idx = qemuDeviceMemoryGetAliasID(def, mem, oldAlias, prefix); break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: prefix = "nvdimm"; + idx = qemuDeviceMemoryGetAliasID(def, mem, oldAlias, prefix); break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + prefix = "virtiopmem"; + idx = qemuDeviceMemoryGetAliasID(def, mem, true, prefix); + break; case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; } - if (oldAlias) { - for (i = 0; i < def->nmems; i++) { - if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, prefix)) >= maxidx) - maxidx = idx + 1; - } - } else { - maxidx = mem->info.addr.dimm.slot; - } - - mem->info.alias = g_strdup_printf("%s%d", prefix, maxidx); + mem->info.alias = g_strdup_printf("%s%d", prefix, idx); return 0; } @@ -688,7 +704,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nmems; i++) { - if (qemuAssignDeviceMemoryAlias(NULL, def->mems[i], false) < 0) + if (qemuAssignDeviceMemoryAlias(def, def->mems[i], false) < 0) return -1; } if (def->vsock) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 49241fc507..501deff1ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2957,7 +2957,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, virDomainMemoryAccess memAccess = mem->access; size_t i; g_autofree char *memPath = NULL; - bool prealloc = false; + bool wantPrealloc = false; + bool allowPrealloc = !priv->memPrealloc; virBitmapPtr nodemask = NULL; int rc; g_autoptr(virJSONValue) props = NULL; @@ -2991,6 +2992,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, nvdimmPmem = mem->s.nvdimm.pmem; break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + nvdimmPath = mem->s.virtio.path; + /* virtio-pmem doesn't need prealloc, it's very likely exposing a real + * device and thus there's nothing to prealloc */ + allowPrealloc = false; + break; case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; @@ -3020,7 +3026,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, discard = def->mem.discard; if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) - prealloc = true; + wantPrealloc = true; if (virDomainNumatuneGetMode(def->numa, mem->targetNode, &mode) < 0 && virDomainNumatuneGetMode(def->numa, -1, &mode) < 0) @@ -3095,7 +3101,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, return -1; } - prealloc = true; + wantPrealloc = true; } if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) @@ -3106,11 +3112,11 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, if (nvdimmPath) { memPath = g_strdup(nvdimmPath); - prealloc = true; + wantPrealloc = true; } else if (useHugepage) { if (qemuGetDomainHupageMemPath(priv->driver, def, pagesize, &memPath) < 0) return -1; - prealloc = true; + wantPrealloc = true; } else { /* We can have both pagesize and mem source. If that's the case, * prefer hugepages as those are more specific. */ @@ -3143,8 +3149,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, backendType = "memory-backend-ram"; } - if (!priv->memPrealloc && - virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0) + if (allowPrealloc && + virJSONValueObjectAdd(props, "B:prealloc", wantPrealloc, NULL) < 0) return -1; if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0) @@ -3299,7 +3305,7 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - const char *device; + const char *device = NULL; if (!mem->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -3308,46 +3314,46 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def, } switch (mem->model) { - case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: case VIR_DOMAIN_MEMORY_MODEL_DIMM: - - if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM) - device = "pc-dimm"; - else - device = "nvdimm"; - - virBufferAsprintf(&buf, "%s,", device); - - if (mem->targetNode >= 0) - virBufferAsprintf(&buf, "node=%d,", mem->targetNode); - - if (mem->labelsize) - virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024); - - if (virUUIDIsValid(mem->uuid)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(mem->uuid, uuidstr); - virBufferAsprintf(&buf, "uuid=%s,", uuidstr); - } - - if (mem->readonly) { - virBufferAddLit(&buf, "unarmed=on,"); - } - - virBufferAsprintf(&buf, "memdev=mem%s,id=%s", - mem->info.alias, mem->info.alias); - - qemuBuildDeviceAddressStr(&buf, def, &mem->info, qemuCaps); + device = "pc-dimm"; + break; + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + device = "nvdimm"; break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + device = "virtio-pmem-pci"; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; + } + + virBufferAsprintf(&buf, "%s,", device); + + if (mem->targetNode >= 0) + virBufferAsprintf(&buf, "node=%d,", mem->targetNode); + + if (mem->labelsize) + virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024); + + if (virUUIDIsValid(mem->uuid)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(mem->uuid, uuidstr); + virBufferAsprintf(&buf, "uuid=%s,", uuidstr); } + if (mem->readonly) { + virBufferAddLit(&buf, "unarmed=on,"); + } + + virBufferAsprintf(&buf, "memdev=mem%s,id=%s", + mem->info.alias, mem->info.alias); + + qemuBuildDeviceAddressStr(&buf, def, &mem->info, qemuCaps); + + return virBufferContentAndReset(&buf); } diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args b/tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args new file mode 100644 index 0000000000..e2f5097424 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args @@ -0,0 +1,45 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i386 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=kvm,usb=off,dump-guest-core=off \ +-cpu qemu64 \ +-m size=2095104k,slots=16,maxmem=1099511627776k \ +-overcommit mem-lock=off \ +-smp 2,sockets=2,dies=1,cores=1,threads=1 \ +-object memory-backend-ram,id=ram-node0,size=2145386496 \ +-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-object memory-backend-file,id=memvirtiopmem0,mem-path=/tmp/virtio_pmem,\ +share=yes,size=536870912 \ +-device virtio-pmem-pci,memdev=memvirtiopmem0,id=virtiopmem0,bus=pci.0,\ +addr=0x5 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1",\ +"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\ +"file":"libvirt-1-storage"}' \ +-device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 409680c84e..c79246e0bb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3012,6 +3012,7 @@ mymain(void) DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-pmem"); DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-readonly"); DO_TEST_CAPS_ARCH_LATEST("memory-hotplug-nvdimm-ppc64", "ppc64"); + DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem"); DO_TEST("machine-aeskeywrap-on-caps", QEMU_CAPS_AES_KEY_WRAP, -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:22 +0100, Michal Privoznik wrote:
Now we have everything prepared for generating the command line. The device alias prefix was chosen to be 'virtiopmem'.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1735375 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 44 ++++++---- src/qemu/qemu_command.c | 82 ++++++++++--------- ...ory-hotplug-virtio-pmem.x86_64-latest.args | 45 ++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 120 insertions(+), 52 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 931dbd4e84..68d4a7b480 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -466,6 +466,28 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def, }
+static int +qemuDeviceMemoryGetAliasID(virDomainDefPtr def, + virDomainMemoryDefPtr mem, + bool oldAlias, + const char *prefix) +{ + size_t i; + int maxidx = 0; + + if (!oldAlias) + return mem->info.addr.dimm.slot;
If oldAlias is false this returns 'info.addr.dimm.slot' ...
+ + for (i = 0; i < def->nmems; i++) { + int idx; + if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, prefix)) >= maxidx) + maxidx = idx + 1; + } + + return maxidx; +} + + /** * qemuAssignDeviceMemoryAlias: * @def: domain definition. Necessary only if @oldAlias is true. @@ -483,10 +505,8 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, virDomainMemoryDefPtr mem, bool oldAlias) { - size_t i; - int maxidx = 0; - int idx; const char *prefix = NULL; + int idx = 0;
if (mem->info.alias) return 0; @@ -494,26 +514,22 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: prefix = "dimm"; + idx = qemuDeviceMemoryGetAliasID(def, mem, oldAlias, prefix); break; case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: prefix = "nvdimm"; + idx = qemuDeviceMemoryGetAliasID(def, mem, oldAlias, prefix); break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + prefix = "virtiopmem"; + idx = qemuDeviceMemoryGetAliasID(def, mem, true, prefix);
... and you use it like that here, but virtio-pmem is a PCI device.
+ break; case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST:
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 49241fc507..501deff1ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c
[...]
@@ -3143,8 +3149,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, backendType = "memory-backend-ram"; }
- if (!priv->memPrealloc && - virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0) + if (allowPrealloc && + virJSONValueObjectAdd(props, "B:prealloc", wantPrealloc, NULL) < 0) return -1;
We can theoretically use a virTristate* and use 'T' type here instead of having two variables. [...]
@@ -3308,46 +3314,46 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def, }
switch (mem->model) { - case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: case VIR_DOMAIN_MEMORY_MODEL_DIMM: - - if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM) - device = "pc-dimm"; - else - device = "nvdimm"; - - virBufferAsprintf(&buf, "%s,", device); - - if (mem->targetNode >= 0) - virBufferAsprintf(&buf, "node=%d,", mem->targetNode); - - if (mem->labelsize) - virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024); - - if (virUUIDIsValid(mem->uuid)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(mem->uuid, uuidstr); - virBufferAsprintf(&buf, "uuid=%s,", uuidstr); - } - - if (mem->readonly) { - virBufferAddLit(&buf, "unarmed=on,"); - } - - virBufferAsprintf(&buf, "memdev=mem%s,id=%s", - mem->info.alias, mem->info.alias); - - qemuBuildDeviceAddressStr(&buf, def, &mem->info, qemuCaps); + device = "pc-dimm"; + break; + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + device = "nvdimm"; break;
case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + device = "virtio-pmem-pci"; + case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: break; + } + + virBufferAsprintf(&buf, "%s,", device); + + if (mem->targetNode >= 0) + virBufferAsprintf(&buf, "node=%d,", mem->targetNode); + + if (mem->labelsize) + virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024); + + if (virUUIDIsValid(mem->uuid)) { + char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(mem->uuid, uuidstr); + virBufferAsprintf(&buf, "uuid=%s,", uuidstr); }
+ if (mem->readonly) { + virBufferAddLit(&buf, "unarmed=on,"); + } + + virBufferAsprintf(&buf, "memdev=mem%s,id=%s", + mem->info.alias, mem->info.alias); + + qemuBuildDeviceAddressStr(&buf, def, &mem->info, qemuCaps); + + return virBufferContentAndReset(&buf); }
Some of these seem to be specific for some devices (such as unarmed is relevant only for nvdimm)
QEMU gained this new virtio-mem model. It's similar to pc-dimm in a sense that guest uses it as memory, but in a sense very different from it as it can dynamically change allocation, without need for hotplug. More specifically, the device has two attributes more (it has more of course, but these two are important here): 1) block-size - the granularity of the device. You can imagine the device being divided into blocks, each 'block-size' long. 2) requested-size - the portion of the device that is in use by the guest. And it all works like this: at guest startup/hotplug both block-size and requested-size are specified. When sysadmin wants to give some more memory to the guest, or take some back, they change the 'requested-size' attribute which is propagated to the guest where virtio-mem module takes corresponding action. This means, that 'requested-size' must be a whole number product of 'block-size' and of course has to be in rage [0, max-size] (including). The 'max-size' is yet another attribute but if not set it's "inherited" from corresponding memory-backend-* object. Therefore, two new elements are introduced under <target/>, to reflect these attributes: <memory model='virtio'> <target> <size unit='KiB'>4194304</size> <node>0</node> <block unit='KiB'>2048</block> <requested unit='KiB'>524288</requested> </target> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </memory> The intent here is that <requested/> will be allowed to change via virDomainUpdateDeviceFlags() API. Note, QEMU does inform us about success of allocation via an event - this is covered in next patches. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 22 ++++ docs/schemas/domaincommon.rng | 10 ++ src/conf/domain_conf.c | 103 ++++++++++++++++-- src/conf/domain_conf.h | 5 + .../memory-hotplug-virtio-mem.xml | 78 +++++++++++++ ...emory-hotplug-virtio-mem.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-virtio-mem.x86_64-latest.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ca6bc0432e..3990728939 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -7187,6 +7187,17 @@ Example: usage of the memory devices </label> </target> </memory> + <memory model='virtio'> + <source> + <path>/tmp/virtio_mem</path> + </source> + <target> + <size unit='KiB'>1048576</size> + <node>0</node> + <block unit='KiB'>2048</block> + <requested unit='KiB'>524288</requested> + </target> + </memory> <memory model='virtio' access='shared'> <source> <path>/tmp/virtio_pmem</path> @@ -7300,6 +7311,17 @@ Example: usage of the memory devices so other backend types should use the ``readonly`` element. :since:`Since 5.0.0` + ``block`` + The size of an individual block, granularity of division of memory module. + Must be power of two and at least equal to size of a transparent hugepage + (2MiB on x84_64). The default is hypervisor dependant. This is valid for + ``virtio`` model only and mutually exclusive with ``pmem``. + + ``requested`` + The total size of blocks exposed to the guest. Must respect ``block`` + granularity. This is valid for ``virtio`` model only and mutually + exclusive with ``pmem``. + :anchor:`<a id="elementsIommu"/>` IOMMU devices diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b385bae84c..d478b639fa 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -6053,6 +6053,16 @@ <element name="size"> <ref name="scaledInteger"/> </element> + <optional> + <element name="block"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="requested"> + <ref name="scaledInteger"/> + </element> + </optional> <optional> <element name="node"> <ref name="unsignedInt"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 935bea1804..0551f6f266 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6764,10 +6764,23 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, return -1; } } else { - /* TODO: plain virtio-mem behaves differently then virtio-pmem */ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-mem is not supported yet. <pmem/> is required")); - return -1; + if (mem->requestedsize > mem->size) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("requested size must be smaller than @size")); + return -1; + } + + if (!VIR_IS_POW2(mem->blocksize)) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("block size must be a power of two")); + return -1; + } + + if (mem->requestedsize % mem->blocksize != 0) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("requested size must be an integer multiple of block size")); + return -1; + } } break; @@ -16774,9 +16787,25 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: def->s.virtio.path = virXPathString("string(./path)", ctxt); - if (virXPathBoolean("boolean(./pmem)", ctxt)) + if (virXPathBoolean("boolean(./pmem)", ctxt)) { def->s.virtio.pmem = true; + } else { + if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt, + &def->s.virtio.pagesize, false, false) < 0) + return -1; + if ((nodemask = virXPathString("string(./nodemask)", ctxt))) { + if (virBitmapParse(nodemask, &def->s.virtio.sourceNodes, + VIR_DOMAIN_CPUMASK_LEN) < 0) + return -1; + + if (virBitmapIsAllClear(def->s.virtio.sourceNodes)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'nodemask': %s"), nodemask); + return -1; + } + } + } break; case VIR_DOMAIN_MEMORY_MODEL_NONE: @@ -16812,7 +16841,8 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, &def->size, true, false) < 0) return -1; - if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + switch (def->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (virDomainParseMemory("./label/size", "./label/size/@unit", ctxt, &def->labelsize, false, false) < 0) return -1; @@ -16831,6 +16861,27 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, if (virXPathBoolean("boolean(./readonly)", ctxt)) def->readonly = true; + + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (!def->s.virtio.pmem) { + if (virDomainParseMemory("./block", "./block/@unit", ctxt, + &def->blocksize, false, false) < 0) + return -1; + + if (virDomainParseMemory("./requested", "./requested/@unit", ctxt, + &def->requestedsize, false, false) < 0) + return -1; + } + + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + /* nada */ + break; } return 0; @@ -18649,7 +18700,9 @@ virDomainMemoryFindByDefInternal(virDomainDefPtr def, /* target info -> always present */ if (tmp->model != mem->model || tmp->targetNode != mem->targetNode || - tmp->size != mem->size) + tmp->size != mem->size || + tmp->blocksize != mem->blocksize || + tmp->requestedsize != mem->requestedsize) continue; switch (mem->model) { @@ -24255,6 +24308,22 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, return false; } + if (src->blocksize != dst->blocksize) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target memory device block size '%llu' doesn't match " + "source memory device block size '%llu'"), + dst->blocksize, src->blocksize); + return false; + } + + if (src->requestedsize != dst->requestedsize) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target memory device requested size '%llu' doesn't match " + "source memory device requested size '%llu'"), + dst->requestedsize, src->requestedsize); + return false; + } + switch (src->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (src->labelsize != dst->labelsize) { @@ -27967,6 +28036,18 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, if (def->s.virtio.pmem) virBufferAddLit(&childBuf, "<pmem/>\n"); + + if (def->s.virtio.sourceNodes) { + if (!(bitmap = virBitmapFormat(def->s.virtio.sourceNodes))) + return -1; + + virBufferAsprintf(&childBuf, "<nodemask>%s</nodemask>\n", bitmap); + } + + if (def->s.virtio.pagesize) { + virBufferAsprintf(&childBuf, "<pagesize unit='KiB'>%llu</pagesize>\n", + def->s.virtio.pagesize); + } break; case VIR_DOMAIN_MEMORY_MODEL_NONE: @@ -27998,6 +28079,14 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf, if (def->readonly) virBufferAddLit(&childBuf, "<readonly/>\n"); + if (def->blocksize) { + virBufferAsprintf(&childBuf, "<block unit='KiB'>%llu</block>\n", + def->blocksize); + + virBufferAsprintf(&childBuf, "<requested unit='KiB'>%llu</requested>\n", + def->requestedsize); + } + virXMLFormatElement(buf, "target", NULL, &childBuf); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index efaa4c5473..f16dc0a029 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2327,8 +2327,11 @@ struct _virDomainMemoryDef { bool pmem; } nvdimm; /* VIR_DOMAIN_MEMORY_MODEL_NVDIMM */ struct { + // nodemask + hugepages + no prealloc char *path; /* Required for pmem, otherwise optional */ bool pmem; + virBitmapPtr sourceNodes; + unsigned long long pagesize; /* kibibytes */ } virtio; /* VIR_DOMAIN_MEMORY_MODEL_VIRTIO */ } s; @@ -2337,6 +2340,8 @@ struct _virDomainMemoryDef { int targetNode; unsigned long long size; /* kibibytes */ unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */ + unsigned long long blocksize; /* kibibytes, valid for virtio-mem only */ + unsigned long long requestedsize; /* kibibytes, valid for virtio-mem only */ bool readonly; /* valid only for NVDIMM */ /* required for QEMU NVDIMM ppc64 support */ diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml new file mode 100644 index 0000000000..0c8d1d970e --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml @@ -0,0 +1,78 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>8388608</memory> + <currentMemory unit='KiB'>8388608</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + <topology sockets='2' dies='1' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='2095104' 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-system-i386</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <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'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + <memory model='virtio'> + <target> + <size unit='KiB'>1048576</size> + <node>0</node> + <block unit='KiB'>2048</block> + <requested unit='KiB'>524288</requested> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memory> + <memory model='virtio'> + <source> + <nodemask>1-3</nodemask> + <pagesize unit='KiB'>2048</pagesize> + </source> + <target> + <size unit='KiB'>2097152</size> + <node>0</node> + <block unit='KiB'>2048</block> + <requested unit='KiB'>1048576</requested> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </memory> + <memory model='virtio'> + <source> + <path>/tmp/virtio_mem</path> + </source> + <target> + <size unit='KiB'>4194304</size> + <node>0</node> + <block unit='KiB'>2048</block> + <requested unit='KiB'>524288</requested> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-virtio-mem.x86_64-latest.xml b/tests/qemuxml2xmloutdata/memory-hotplug-virtio-mem.x86_64-latest.xml new file mode 120000 index 0000000000..a9d298129c --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-virtio-mem.x86_64-latest.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/memory-hotplug-virtio-mem.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index e804db8aee..00d8f171bc 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1239,6 +1239,7 @@ mymain(void) DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_DEVICE_NVDIMM); DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem"); + DO_TEST_CAPS_LATEST("memory-hotplug-virtio-mem"); DO_TEST("net-udp", NONE); -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:23 +0100, Michal Privoznik wrote:
QEMU gained this new virtio-mem model. It's similar to pc-dimm in a sense that guest uses it as memory, but in a sense very different from it as it can dynamically change allocation, without need for hotplug. More specifically, the device has two attributes more (it has more of course, but these two are important here):
1) block-size - the granularity of the device. You can imagine the device being divided into blocks, each 'block-size' long.
2) requested-size - the portion of the device that is in use by the guest.
And it all works like this: at guest startup/hotplug both block-size and requested-size are specified. When sysadmin wants to give some more memory to the guest, or take some back, they change the 'requested-size' attribute which is propagated to the guest where virtio-mem module takes corresponding action. This means, that 'requested-size' must be a whole number product of 'block-size' and of course has to be in rage [0, max-size] (including). The 'max-size' is yet another attribute but if not set it's "inherited" from corresponding memory-backend-* object.
Therefore, two new elements are introduced under <target/>, to reflect these attributes:
<memory model='virtio'> <target> <size unit='KiB'>4194304</size> <node>0</node> <block unit='KiB'>2048</block> <requested unit='KiB'>524288</requested> </target> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </memory>
The intent here is that <requested/> will be allowed to change via virDomainUpdateDeviceFlags() API.
Note, QEMU does inform us about success of allocation via an event - this is covered in next patches.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 22 ++++ docs/schemas/domaincommon.rng | 10 ++ src/conf/domain_conf.c | 103 ++++++++++++++++-- src/conf/domain_conf.h | 5 + .../memory-hotplug-virtio-mem.xml | 78 +++++++++++++ ...emory-hotplug-virtio-mem.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-virtio-mem.x86_64-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ca6bc0432e..3990728939 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -7187,6 +7187,17 @@ Example: usage of the memory devices </label> </target> </memory> + <memory model='virtio'> + <source> + <path>/tmp/virtio_mem</path> + </source> + <target> + <size unit='KiB'>1048576</size> + <node>0</node> + <block unit='KiB'>2048</block> + <requested unit='KiB'>524288</requested> + </target> + </memory> <memory model='virtio' access='shared'> <source> <path>/tmp/virtio_pmem</path> @@ -7300,6 +7311,17 @@ Example: usage of the memory devices so other backend types should use the ``readonly`` element. :since:`Since 5.0.0`
+ ``block`` + The size of an individual block, granularity of division of memory module. + Must be power of two and at least equal to size of a transparent hugepage + (2MiB on x84_64). The default is hypervisor dependant. This is valid for + ``virtio`` model only and mutually exclusive with ``pmem``. + + ``requested`` + The total size of blocks exposed to the guest. Must respect ``block`` + granularity. This is valid for ``virtio`` model only and mutually + exclusive with ``pmem``.
Docs don't mention interactions of <size> and <requested>. Is size the actual size? In such case you'll need to clear it on startup and populate it with the actual size later. The issue with <pmem> was mentioned earlier. [...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 935bea1804..0551f6f266 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6764,10 +6764,23 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem, return -1; } } else { - /* TODO: plain virtio-mem behaves differently then virtio-pmem */ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("virtio-mem is not supported yet. <pmem/> is required")); - return -1; + if (mem->requestedsize > mem->size) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("requested size must be smaller than @size")); + return -1; + } + + if (!VIR_IS_POW2(mem->blocksize)) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("block size must be a power of two")); + return -1; + }
Docs state that blocksize must be also a multiple of page size.
+ + if (mem->requestedsize % mem->blocksize != 0) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("requested size must be an integer multiple of block size")); + return -1; + } } break;
@@ -16774,9 +16787,25 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: def->s.virtio.path = virXPathString("string(./path)", ctxt);
- if (virXPathBoolean("boolean(./pmem)", ctxt)) + if (virXPathBoolean("boolean(./pmem)", ctxt)) { def->s.virtio.pmem = true; + } else { + if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt, + &def->s.virtio.pagesize, false, false) < 0) + return -1;
+ if ((nodemask = virXPathString("string(./nodemask)", ctxt))) { + if (virBitmapParse(nodemask, &def->s.virtio.sourceNodes, + VIR_DOMAIN_CPUMASK_LEN) < 0) + return -1; + + if (virBitmapIsAllClear(def->s.virtio.sourceNodes)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'nodemask': %s"), nodemask); + return -1;
Move this to virDomainMemoryDefValidate too.
+ } + } + } break;
case VIR_DOMAIN_MEMORY_MODEL_NONE:
[...]
@@ -16831,6 +16861,27 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
if (virXPathBoolean("boolean(./readonly)", ctxt)) def->readonly = true; + + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (!def->s.virtio.pmem) { + if (virDomainParseMemory("./block", "./block/@unit", ctxt, + &def->blocksize, false, false) < 0) + return -1; + + if (virDomainParseMemory("./requested", "./requested/@unit", ctxt, + &def->requestedsize, false, false) < 0) + return -1; + }
If size is current size it should be optional.
+ + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + /* nada */ + break; }
return 0; @@ -18649,7 +18700,9 @@ virDomainMemoryFindByDefInternal(virDomainDefPtr def, /* target info -> always present */ if (tmp->model != mem->model || tmp->targetNode != mem->targetNode || - tmp->size != mem->size) + tmp->size != mem->size ||
If size is the current size and being actively updated, this lookup might not actually work if it's updated between checking and updating.
+ tmp->blocksize != mem->blocksize || + tmp->requestedsize != mem->requestedsize) continue;
switch (mem->model) {
[...]
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index efaa4c5473..f16dc0a029 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2327,8 +2327,11 @@ struct _virDomainMemoryDef { bool pmem; } nvdimm; /* VIR_DOMAIN_MEMORY_MODEL_NVDIMM */ struct { + // nodemask + hugepages + no prealloc
Leftovers?
char *path; /* Required for pmem, otherwise optional */ bool pmem; + virBitmapPtr sourceNodes; + unsigned long long pagesize; /* kibibytes */ } virtio; /* VIR_DOMAIN_MEMORY_MODEL_VIRTIO */ } s;
On Fri, Dec 04, 2020 at 13:17:52 +0100, Peter Krempa wrote:
On Thu, Dec 03, 2020 at 13:36:23 +0100, Michal Privoznik wrote:
QEMU gained this new virtio-mem model. It's similar to pc-dimm in a sense that guest uses it as memory, but in a sense very different from it as it can dynamically change allocation, without need for hotplug. More specifically, the device has two attributes more (it has more of course, but these two are important here):
1) block-size - the granularity of the device. You can imagine the device being divided into blocks, each 'block-size' long.
2) requested-size - the portion of the device that is in use by the guest.
And it all works like this: at guest startup/hotplug both block-size and requested-size are specified. When sysadmin wants to give some more memory to the guest, or take some back, they change the 'requested-size' attribute which is propagated to the guest where virtio-mem module takes corresponding action. This means, that 'requested-size' must be a whole number product of 'block-size' and of course has to be in rage [0, max-size] (including). The 'max-size' is yet another attribute but if not set it's "inherited" from corresponding memory-backend-* object.
Therefore, two new elements are introduced under <target/>, to reflect these attributes:
<memory model='virtio'> <target> <size unit='KiB'>4194304</size> <node>0</node> <block unit='KiB'>2048</block> <requested unit='KiB'>524288</requested> </target> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </memory>
The intent here is that <requested/> will be allowed to change via virDomainUpdateDeviceFlags() API.
Note, QEMU does inform us about success of allocation via an event - this is covered in next patches.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 22 ++++ docs/schemas/domaincommon.rng | 10 ++ src/conf/domain_conf.c | 103 ++++++++++++++++-- src/conf/domain_conf.h | 5 + .../memory-hotplug-virtio-mem.xml | 78 +++++++++++++ ...emory-hotplug-virtio-mem.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.xml create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-virtio-mem.x86_64-latest.xml
Disregard any comments about size being actual size ...
The majority of the code is prepared since virtio-pmem. The alias for virtio-mem is new ("virtiomem"). And again, prealloc doesn't make much sense - the whole point is to dynamically allocate memory for guest and possibly return it to the host (e.g. if the host is running low on memory) and this wouldn't happen with prealloc. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 5 +- src/qemu/qemu_command.c | 19 +++++-- ...mory-hotplug-virtio-mem.x86_64-latest.args | 53 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 68d4a7b480..29c26f0b72 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -521,7 +521,10 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, idx = qemuDeviceMemoryGetAliasID(def, mem, oldAlias, prefix); break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: - prefix = "virtiopmem"; + if (mem->s.virtio.pmem) + prefix = "virtiopmem"; + else + prefix = "virtiomem"; idx = qemuDeviceMemoryGetAliasID(def, mem, true, prefix); break; case VIR_DOMAIN_MEMORY_MODEL_NONE: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 501deff1ee..ccbc55e376 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2992,9 +2992,14 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, nvdimmPmem = mem->s.nvdimm.pmem; break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + pagesize = mem->s.virtio.pagesize; + needHugepage = !!pagesize; + useHugepage = !!pagesize; + nodemask = mem->s.virtio.sourceNodes; nvdimmPath = mem->s.virtio.path; - /* virtio-pmem doesn't need prealloc, it's very likely exposing a real - * device and thus there's nothing to prealloc */ + /* virtio-pmem doesn't need prealloc. Either it's pmem and thus very + * likely exposing a real device where is nothing to prealloc, OR it's + * virtio-mem where we want to unmap pages on the fly. */ allowPrealloc = false; break; case VIR_DOMAIN_MEMORY_MODEL_NONE: @@ -3322,7 +3327,10 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def, break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: - device = "virtio-pmem-pci"; + if (mem->s.virtio.pmem) + device = "virtio-pmem-pci"; + else + device = "virtio-mem-pci"; case VIR_DOMAIN_MEMORY_MODEL_NONE: case VIR_DOMAIN_MEMORY_MODEL_LAST: @@ -3337,6 +3345,11 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def, if (mem->labelsize) virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024); + if (mem->blocksize) { + virBufferAsprintf(&buf, "block-size=%llu,", mem->blocksize * 1024); + virBufferAsprintf(&buf, "requested-size=%llu,", mem->requestedsize * 1024); + } + if (virUUIDIsValid(mem->uuid)) { char uuidstr[VIR_UUID_STRING_BUFLEN]; diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args new file mode 100644 index 0000000000..c12248518d --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args @@ -0,0 +1,53 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i386 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=kvm,usb=off,dump-guest-core=off \ +-cpu qemu64 \ +-m size=2095104k,slots=16,maxmem=1099511627776k \ +-overcommit mem-lock=off \ +-smp 2,sockets=2,dies=1,cores=1,threads=1 \ +-object memory-backend-ram,id=ram-node0,size=2145386496 \ +-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \ +-object memory-backend-ram,id=memvirtiomem0,size=1073741824 \ +-device virtio-mem-pci,node=0,block-size=2097152,requested-size=536870912,\ +memdev=memvirtiomem0,id=virtiomem0,bus=pci.0,addr=0x2 \ +-object memory-backend-file,id=memvirtiomem1,\ +mem-path=/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1,size=2147483648,\ +host-nodes=1-3,policy=bind \ +-device virtio-mem-pci,node=0,block-size=2097152,requested-size=1073741824,\ +memdev=memvirtiomem1,id=virtiomem1,bus=pci.0,addr=0x4 \ +-object memory-backend-file,id=memvirtiomem2,mem-path=/tmp/virtio_mem,\ +size=4294967296 \ +-device virtio-mem-pci,node=0,block-size=2097152,requested-size=536870912,\ +memdev=memvirtiomem2,id=virtiomem2,bus=pci.0,addr=0x5 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1",\ +"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\ +"file":"libvirt-1-storage"}' \ +-device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c79246e0bb..13678fc891 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3013,6 +3013,7 @@ mymain(void) DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-readonly"); DO_TEST_CAPS_ARCH_LATEST("memory-hotplug-nvdimm-ppc64", "ppc64"); DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem"); + DO_TEST_CAPS_LATEST("memory-hotplug-virtio-mem"); DO_TEST("machine-aeskeywrap-on-caps", QEMU_CAPS_AES_KEY_WRAP, -- 2.26.2
As advertised in the previous commit, we want' to be able to change 'requested-size' attribute of virtio-mem on the fly. This commit does exactly that. Changing anything else is checked for and forbidden. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 23 +++++ src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 165 ++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.c | 18 ++++ src/qemu/qemu_hotplug.h | 5 ++ src/qemu/qemu_monitor.c | 13 +++ src/qemu/qemu_monitor.h | 4 + src/qemu/qemu_monitor_json.c | 15 ++++ src/qemu/qemu_monitor_json.h | 5 ++ 10 files changed, 251 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0551f6f266..a4293f1749 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18762,6 +18762,29 @@ virDomainMemoryFindInactiveByDef(virDomainDefPtr def, } +ssize_t +virDomainMemoryFindByDeviceInfo(virDomainDefPtr def, + virDomainDeviceInfoPtr info) +{ + size_t i; + + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr tmp = def->mems[i]; + + if (!virDomainDeviceInfoAddressIsEqual(&tmp->info, info)) + continue; + + /* alias, if present */ + if (STRNEQ_NULLABLE(tmp->info.alias, info->alias)) + continue; + + return i; + } + + return -1; +} + + /** * virDomainMemoryInsert: * diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f16dc0a029..d0814d7639 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3584,6 +3584,9 @@ int virDomainMemoryFindByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem) int virDomainMemoryFindInactiveByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +ssize_t virDomainMemoryFindByDeviceInfo(virDomainDefPtr dev, + virDomainDeviceInfoPtr info) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; int virDomainShmemDefInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2f640ef1c4..7014d602b6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -497,6 +497,7 @@ virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemoryDefFree; virDomainMemoryFindByDef; +virDomainMemoryFindByDeviceInfo; virDomainMemoryFindInactiveByDef; virDomainMemoryInsert; virDomainMemoryModelTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bca1c84630..edc109cb28 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7104,6 +7104,158 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, return 0; } + +static bool +qemuDomainChangeMemoryLiveValidateChange(const virDomainMemoryDef *oldDef, + const virDomainMemoryDef *newDef) +{ + /* The only thing that is allowed to change is 'requestedsize' for virtio + * model. */ + if (oldDef->model != newDef->model) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory model from '%s' to '%s'"), + virDomainMemoryModelTypeToString(oldDef->model), + virDomainMemoryModelTypeToString(newDef->model)); + return false; + } + + if (oldDef->access != newDef->access) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory access from '%s' to '%s'"), + virDomainMemoryAccessTypeToString(oldDef->access), + virDomainMemoryAccessTypeToString(newDef->access)); + return false; + } + + if (oldDef->discard != newDef->discard) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory discard from '%s' to '%s'"), + virTristateBoolTypeToString(oldDef->discard), + virTristateBoolTypeToString(newDef->discard)); + return false; + } + + if (oldDef->targetNode != newDef->targetNode) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory targetNode from '%d' to '%d'"), + oldDef->targetNode, newDef->targetNode); + return false; + } + + if (oldDef->size != newDef->size) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory size from '%llu' to '%llu'"), + oldDef->size, newDef->size); + return false; + } + + if (oldDef->blocksize != newDef->blocksize) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory block size from '%llu' to '%llu'"), + oldDef->blocksize, newDef->blocksize); + return false; + } + + /* requestedsize can change */ + + if (oldDef->readonly != newDef->readonly) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot modify memory pmem flag")); + return false; + } + + if (virUUIDIsValid(oldDef->uuid) && + memcmp(oldDef->uuid, newDef, VIR_UUID_BUFLEN) != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot modify memory UUID")); + return false; + } + + switch (oldDef->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + if (STRNEQ_NULLABLE(oldDef->s.virtio.path, newDef->s.virtio.path)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory path from '%s' to '%s'"), + NULLSTR(oldDef->s.virtio.path), + NULLSTR(newDef->s.virtio.path)); + return false; + } + + if (oldDef->s.virtio.pmem != newDef->s.virtio.pmem) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot modify memory pmem flag")); + return false; + } + + if (!virBitmapEqual(oldDef->s.virtio.sourceNodes, + newDef->s.virtio.sourceNodes)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot modify memory source nodes")); + return false; + } + + if (oldDef->s.virtio.pagesize != newDef->s.virtio.pagesize) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory pagesize from '%llu' to '%llu'"), + oldDef->s.virtio.pagesize, + newDef->s.virtio.pagesize); + return false; + } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot modify memory of model '%s'"), + virDomainMemoryModelTypeToString(oldDef->model)); + return false; + break; + } + + return true; +} + + +static int +qemuDomainChangeMemoryLive(virQEMUDriverPtr driver G_GNUC_UNUSED, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + virDomainMemoryDefPtr newDef = dev->data.memory; + virDomainMemoryDefPtr oldDef = NULL; + ssize_t idx; + + idx = virDomainMemoryFindByDeviceInfo(vm->def, &dev->data.memory->info); + if (idx < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory '%s' not found"), dev->data.memory->info.alias); + return -1; + } + + oldDef = vm->def->mems[idx]; + + if (newDef->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO || + newDef->s.virtio.pmem) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("changing anything but <requested/> size for virtio-mem is not implemented yet")); + return -1; + } + + if (!qemuDomainChangeMemoryLiveValidateChange(oldDef, newDef)) + return -1; + + if (qemuDomainChangeMemoryRequestedSize(driver, vm, + newDef->info.alias, + newDef->requestedsize) < 0) + return -1; + + oldDef->requestedsize = newDef->requestedsize; + return 0; +} + + static int qemuDomainUpdateDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev, @@ -7145,6 +7297,18 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm, ret = qemuDomainChangeNet(driver, vm, dev); break; + case VIR_DOMAIN_DEVICE_MEMORY: + if ((idx = virDomainMemoryFindByDeviceInfo(vm->def, &dev->data.memory->info)) >= 0) { + oldDev.data.memory = vm->def->mems[idx]; + if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev, + VIR_DOMAIN_DEVICE_ACTION_UPDATE, + true) < 0) + return -1; + } + + ret = qemuDomainChangeMemoryLive(driver, vm, dev); + break; + case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: @@ -7160,7 +7324,6 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm, 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: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bc5dc02a2f..a18fa6d362 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6672,3 +6672,21 @@ qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, virBitmapFree(livevcpus); return ret; } + + +int +qemuDomainChangeMemoryRequestedSize(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *alias, + unsigned long long requestedsize) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorChangeMemoryRequestedSize(priv->mon, alias, requestedsize); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + return rc; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 6287c5b5e8..9e551a1f82 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -160,3 +160,8 @@ int qemuHotplugAttachDBusVMState(virQEMUDriverPtr driver, int qemuHotplugRemoveDBusVMState(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob); + +int qemuDomainChangeMemoryRequestedSize(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *alias, + unsigned long long requestedsize); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ce1a06c4c8..0e2cc0b76c 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4733,3 +4733,16 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, return qemuMonitorJSONTransactionBackup(actions, device, jobname, target, bitmap, syncmode); } + + +int +qemuMonitorChangeMemoryRequestedSize(qemuMonitorPtr mon, + const char *alias, + unsigned long long requestedsize) +{ + VIR_DEBUG("alias=%s requestedsize=%llu", alias, requestedsize); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONChangeMemoryRequestedSize(mon, alias, requestedsize); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8bc092870b..3005adc1e0 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1518,3 +1518,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, const char *target, const char *bitmap, qemuMonitorTransactionBackupSyncMode syncmode); + +int qemuMonitorChangeMemoryRequestedSize(qemuMonitorPtr mon, + const char *alias, + unsigned long long requestedsize); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e82d762925..4046d0b3b0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9467,3 +9467,18 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, return virJSONValueGetBoolean(virJSONValueObjectGet(reply, "return"), migratable); } + + +int +qemuMonitorJSONChangeMemoryRequestedSize(qemuMonitorPtr mon, + const char *alias, + unsigned long long requestedsize) +{ + g_autofree char *path = g_strdup_printf("/machine/peripheral/%s", alias); + qemuMonitorJSONObjectProperty prop = { + .type = QEMU_MONITOR_OBJECT_PROPERTY_ULONG, + .val.ul = requestedsize * 1024, /* monitor needs bytes */ + }; + + return qemuMonitorJSONSetObjectProperty(mon, path, "requested-size", &prop); +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d2928b0ffc..d4b80b5f27 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -704,3 +704,8 @@ int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, int qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, bool *migratable); + +int +qemuMonitorJSONChangeMemoryRequestedSize(qemuMonitorPtr mon, + const char *alias, + unsigned long long requestedsize); -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:25 +0100, Michal Privoznik wrote:
As advertised in the previous commit, we want' to be able to change 'requested-size' attribute of virtio-mem on the fly. This commit does exactly that. Changing anything else is checked for and forbidden.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 23 +++++ src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 165 ++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.c | 18 ++++ src/qemu/qemu_hotplug.h | 5 ++ src/qemu/qemu_monitor.c | 13 +++ src/qemu/qemu_monitor.h | 4 + src/qemu/qemu_monitor_json.c | 15 ++++ src/qemu/qemu_monitor_json.h | 5 ++ 10 files changed, 251 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0551f6f266..a4293f1749 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18762,6 +18762,29 @@ virDomainMemoryFindInactiveByDef(virDomainDefPtr def, }
+ssize_t +virDomainMemoryFindByDeviceInfo(virDomainDefPtr def, + virDomainDeviceInfoPtr info) +{ + size_t i; + + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr tmp = def->mems[i]; + + if (!virDomainDeviceInfoAddressIsEqual(&tmp->info, info)) + continue; + + /* alias, if present */ + if (STRNEQ_NULLABLE(tmp->info.alias, info->alias))
This doesn't work as the comment expects it to: STRNEQ_NULLABLE(NULL, NULL) == false STRNEQ_NULLABLE("blah", NULL) == true STRNEQ_NULLABLE(NULL, "blah") == true STRNEQ_NULLABLE("blah", "blah") == false Since info->alias is always set, it would not skip the condition if the definition used for lookup didn't specify it.
+ continue; + + return i; + } + + return -1; +} + + /** * virDomainMemoryInsert: *
[...]
As advertised in previous commit, this event is delivered to us when virtio-mem module changes the allocation inside the guest. It comes with one attribute - size - which holds the new size of the virtio-mem (well, allocated size), in bytes. Mind you, this is not necessarily the same number as 'requested size'. It almost certainly will be when sizing the memory up, but it might not be when sizing the memory down - the guest kernel might be unable to free some blocks. This actual size is reported in the domain XML as an output element only. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 7 +++ docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 24 ++++++++- src/conf/domain_conf.h | 7 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 3 ++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 36 +++++++++++++ src/qemu/qemu_monitor.c | 24 +++++++++ src/qemu/qemu_monitor.h | 20 ++++++++ src/qemu/qemu_monitor_json.c | 24 +++++++++ src/qemu/qemu_process.c | 96 +++++++++++++++++++++++++++++++---- 12 files changed, 236 insertions(+), 12 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 3990728939..ac87d03b33 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -7196,6 +7196,7 @@ Example: usage of the memory devices <node>0</node> <block unit='KiB'>2048</block> <requested unit='KiB'>524288</requested> + <actual unit='KiB'>524288</requested> </target> </memory> <memory model='virtio' access='shared'> @@ -7322,6 +7323,12 @@ Example: usage of the memory devices granularity. This is valid for ``virtio`` model only and mutually exclusive with ``pmem``. + ``actual`` + The active XML for a ``virtio`` model may contain ``actual`` element that + reflects the actual size of the corresponding virtio memory device. The + element is formatted into live XML and never parsed, i.e. it is + output-only element. + :anchor:`<a id="elementsIommu"/>` IOMMU devices diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d478b639fa..3b12902e04 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -6063,6 +6063,11 @@ <ref name="scaledInteger"/> </element> </optional> + <optional> + <element name="actual"> + <ref name="scaledInteger"/> + </element> + </optional> <optional> <element name="node"> <ref name="unsignedInt"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a4293f1749..8adb3e99e2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18785,6 +18785,21 @@ virDomainMemoryFindByDeviceInfo(virDomainDefPtr def, } +ssize_t +virDomainMemoryFindByDeviceAlias(virDomainDefPtr def, + const char *alias) +{ + size_t i; + + for (i = 0; i < def->nmems; i++) { + if (STREQ_NULLABLE(def->mems[i]->info.alias, alias)) + return i; + } + + return -1; +} + + /** * virDomainMemoryInsert: * @@ -28086,7 +28101,8 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, static void virDomainMemoryTargetDefFormat(virBufferPtr buf, - virDomainMemoryDefPtr def) + virDomainMemoryDefPtr def, + unsigned int flags) { g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -28108,6 +28124,10 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf, virBufferAsprintf(&childBuf, "<requested unit='KiB'>%llu</requested>\n", def->requestedsize); + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { + virBufferAsprintf(&childBuf, "<actual unit='KiB'>%llu</actual>\n", + def->actualsize); + } } virXMLFormatElement(buf, "target", NULL, &childBuf); @@ -28142,7 +28162,7 @@ virDomainMemoryDefFormat(virBufferPtr buf, if (virDomainMemorySourceDefFormat(buf, def) < 0) return -1; - virDomainMemoryTargetDefFormat(buf, def); + virDomainMemoryTargetDefFormat(buf, def, flags); virDomainDeviceInfoFormat(buf, &def->info, flags); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d0814d7639..f8d86d704e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2342,6 +2342,9 @@ struct _virDomainMemoryDef { unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */ unsigned long long blocksize; /* kibibytes, valid for virtio-mem only */ unsigned long long requestedsize; /* kibibytes, valid for virtio-mem only */ + unsigned long long actualsize; /* kibibytes, valid for virtio-mem and + active domain only, only to report never + parse */ bool readonly; /* valid only for NVDIMM */ /* required for QEMU NVDIMM ppc64 support */ @@ -3588,6 +3591,10 @@ ssize_t virDomainMemoryFindByDeviceInfo(virDomainDefPtr dev, virDomainDeviceInfoPtr info) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +ssize_t virDomainMemoryFindByDeviceAlias(virDomainDefPtr def, + const char *alias) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; + int virDomainShmemDefInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; bool virDomainShmemDefEquals(virDomainShmemDefPtr src, virDomainShmemDefPtr dst) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7014d602b6..1d800087fc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -497,6 +497,7 @@ virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemoryDefFree; virDomainMemoryFindByDef; +virDomainMemoryFindByDeviceAlias; virDomainMemoryFindByDeviceInfo; virDomainMemoryFindInactiveByDef; virDomainMemoryInsert; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ab7938a355..fc994ec282 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10554,6 +10554,9 @@ qemuProcessEventFree(struct qemuProcessEvent *event) case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE: virObjectUnref(event->data); break; + case QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE: + qemuMonitorMemoryDeviceSizeChangeFree(event->data); + break; case QEMU_PROCESS_EVENT_PR_DISCONNECT: case QEMU_PROCESS_EVENT_LAST: break; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 010bae285d..376679da77 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -441,6 +441,7 @@ typedef enum { QEMU_PROCESS_EVENT_PR_DISCONNECT, QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED, QEMU_PROCESS_EVENT_GUEST_CRASHLOADED, + QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE, QEMU_PROCESS_EVENT_LAST } qemuProcessEventType; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index edc109cb28..3d2d6bc1eb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4284,6 +4284,39 @@ processGuestCrashloadedEvent(virQEMUDriverPtr driver, } +static void +processMemoryDeviceSizeChange(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuMonitorMemoryDeviceSizeChangePtr info) +{ + virDomainMemoryDefPtr mem = NULL; + ssize_t idx; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + return; + + if (!virDomainObjIsActive(vm)) { + VIR_DEBUG("Domain is not running"); + goto endjob; + } + + idx = virDomainMemoryFindByDeviceAlias(vm->def, info->devAlias); + if (idx < 0) { + VIR_DEBUG("Memory device '%s' not found", info->devAlias); + goto endjob; + } + + mem = vm->def->mems[idx]; + mem->actualsize = VIR_DIV_UP(info->size, 1024); + + /* fix the balloon size */ + ignore_value(qemuProcessRefreshBalloonState(driver, vm, QEMU_ASYNC_JOB_NONE)); + + endjob: + qemuDomainObjEndJob(driver, vm); +} + + static void qemuProcessEventHandler(void *data, void *opaque) { struct qemuProcessEvent *processEvent = data; @@ -4333,6 +4366,9 @@ static void qemuProcessEventHandler(void *data, void *opaque) case QEMU_PROCESS_EVENT_GUEST_CRASHLOADED: processGuestCrashloadedEvent(driver, vm); break; + case QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE: + processMemoryDeviceSizeChange(driver, vm, processEvent->data); + break; case QEMU_PROCESS_EVENT_LAST: break; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0e2cc0b76c..e8a9788011 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1417,6 +1417,20 @@ qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon) } +int +qemuMonitorEmitMemoryDeviceSizeChange(qemuMonitorPtr mon, + const char *devAlias, + unsigned long long size) +{ + int ret = -1; + VIR_DEBUG("mon=%p, devAlias='%s', size=%llu", mon, devAlias, size); + + QEMU_MONITOR_CALLBACK(mon, ret, domainMemoryDeviceSizeChange, mon->vm, devAlias, size); + + return ret; +} + + int qemuMonitorEmitMemoryFailure(qemuMonitorPtr mon, qemuMonitorEventMemoryFailurePtr mfp) @@ -4420,6 +4434,16 @@ qemuMonitorEventRdmaGidStatusFree(qemuMonitorRdmaGidStatusPtr info) } +void +qemuMonitorMemoryDeviceSizeChangeFree(qemuMonitorMemoryDeviceSizeChangePtr info) +{ + if (!info) + return; + + VIR_FREE(info->devAlias); +} + + int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, const char *action) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 3005adc1e0..2f9184c122 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -108,6 +108,14 @@ struct _qemuMonitorRdmaGidStatus { }; +typedef struct _qemuMonitorMemoryDeviceSizeChange qemuMonitorMemoryDeviceSizeChange; +typedef qemuMonitorMemoryDeviceSizeChange *qemuMonitorMemoryDeviceSizeChangePtr; +struct _qemuMonitorMemoryDeviceSizeChange { + char *devAlias; + unsigned long long size; +}; + + typedef enum { QEMU_MONITOR_JOB_TYPE_UNKNOWN, /* internal value, not exposed by qemu */ QEMU_MONITOR_JOB_TYPE_COMMIT, @@ -153,6 +161,7 @@ struct _qemuMonitorJobInfo { char *qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info); void qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info); void qemuMonitorEventRdmaGidStatusFree(qemuMonitorRdmaGidStatusPtr info); +void qemuMonitorMemoryDeviceSizeChangeFree(qemuMonitorMemoryDeviceSizeChangePtr info); typedef void (*qemuMonitorDestroyCallback)(qemuMonitorPtr mon, virDomainObjPtr vm, @@ -374,6 +383,12 @@ typedef int (*qemuMonitorDomainMemoryFailureCallback)(qemuMonitorPtr mon, qemuMonitorEventMemoryFailurePtr mfp, void *opaque); +typedef int (*qemuMonitorDomainMemoryDeviceSizeChange)(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *alias, + unsigned long long size, + void *opaque); + typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks; typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr; struct _qemuMonitorCallbacks { @@ -411,6 +426,7 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainRdmaGidStatusChangedCallback domainRdmaGidStatusChanged; qemuMonitorDomainGuestCrashloadedCallback domainGuestCrashloaded; qemuMonitorDomainMemoryFailureCallback domainMemoryFailure; + qemuMonitorDomainMemoryDeviceSizeChange domainMemoryDeviceSizeChange; }; qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, @@ -507,6 +523,10 @@ int qemuMonitorEmitSerialChange(qemuMonitorPtr mon, bool connected); int qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon); +int qemuMonitorEmitMemoryDeviceSizeChange(qemuMonitorPtr mon, + const char *devAlias, + unsigned long long size); + int qemuMonitorEmitMemoryFailure(qemuMonitorPtr mon, qemuMonitorEventMemoryFailurePtr mfp); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4046d0b3b0..3fd925eae1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -113,6 +113,7 @@ static void qemuMonitorJSONHandleDumpCompleted(qemuMonitorPtr mon, virJSONValueP static void qemuMonitorJSONHandlePRManagerStatusChanged(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleRdmaGidStatusChanged(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleMemoryFailure(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandleMemoryDeviceSizeChange(qemuMonitorPtr mon, virJSONValuePtr data); typedef struct { const char *type; @@ -133,6 +134,7 @@ static qemuEventHandler eventHandlers[] = { { "GUEST_CRASHLOADED", qemuMonitorJSONHandleGuestCrashloaded, }, { "GUEST_PANICKED", qemuMonitorJSONHandleGuestPanic, }, { "JOB_STATUS_CHANGE", qemuMonitorJSONHandleJobStatusChange, }, + { "MEMORY_DEVICE_SIZE_CHANGE", qemuMonitorJSONHandleMemoryDeviceSizeChange, }, { "MEMORY_FAILURE", qemuMonitorJSONHandleMemoryFailure, }, { "MIGRATION", qemuMonitorJSONHandleMigrationStatus, }, { "MIGRATION_PASS", qemuMonitorJSONHandleMigrationPass, }, @@ -1335,6 +1337,28 @@ qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, } +static void +qemuMonitorJSONHandleMemoryDeviceSizeChange(qemuMonitorPtr mon, + virJSONValuePtr data) +{ + const char *name; + unsigned long long size; + + if (!(name = virJSONValueObjectGetString(data, "id"))) { + VIR_WARN("missing device alias in MEMORY_DEVICE_SIZE_CHANGE event"); + return; + } + + if (virJSONValueObjectGetNumberUlong(data, "size", &size) < 0) { + VIR_WARN("missing new size for '%s' in MEMORY_DEVICE_SIZE_CHANGE event", name); + return; + } + + + qemuMonitorEmitMemoryDeviceSizeChange(mon, name, size); +} + + static void qemuMonitorJSONHandleMemoryFailure(qemuMonitorPtr mon, virJSONValuePtr data) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8ea7e0df05..1fba9d8302 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1248,10 +1248,30 @@ qemuProcessHandleBalloonChange(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + size_t i; virObjectLock(vm); event = virDomainEventBalloonChangeNewFromObj(vm, actual); + VIR_DEBUG("New balloon size before fixup: %lld", actual); + + for (i = 0; i < vm->def->nmems; i++) { + virDomainMemoryDefPtr mem = vm->def->mems[i]; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + actual += mem->actualsize; + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + /* nada */ + break; + } + } + VIR_DEBUG("Updating balloon from %lld to %lld kb", vm->def->mem.cur_balloon, actual); vm->def->mem.cur_balloon = actual; @@ -1932,6 +1952,46 @@ qemuProcessHandleMemoryFailure(qemuMonitorPtr mon G_GNUC_UNUSED, } +static int +qemuProcessHandleMemoryDeviceSizeChange(qemuMonitorPtr mon G_GNUC_UNUSED, + virDomainObjPtr vm, + const char *devAlias, + unsigned long long size, + void *opaque) +{ + virQEMUDriverPtr driver = opaque; + struct qemuProcessEvent *processEvent = NULL; + qemuMonitorMemoryDeviceSizeChangePtr info = NULL; + int ret = -1; + + virObjectLock(vm); + + VIR_DEBUG("Memory device '%s' changed size to '%llu' in domain '%s'", + devAlias, size, vm->def->name); + + info = g_new0(qemuMonitorMemoryDeviceSizeChange, 1); + info->devAlias = g_strdup(devAlias); + info->size = size; + + processEvent = g_new0(struct qemuProcessEvent, 1); + processEvent->eventType = QEMU_PROCESS_EVENT_MEMORY_DEVICE_SIZE_CHANGE; + processEvent->vm = virObjectRef(vm); + processEvent->data = g_steal_pointer(&info); + + if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { + qemuProcessEventFree(processEvent); + virObjectUnref(vm); + goto cleanup; + } + + ret = 0; + cleanup: + qemuMonitorMemoryDeviceSizeChangeFree(info); + virObjectUnlock(vm); + return ret; +} + + static qemuMonitorCallbacks monitorCallbacks = { .eofNotify = qemuProcessHandleMonitorEOF, .errorNotify = qemuProcessHandleMonitorError, @@ -1965,6 +2025,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainRdmaGidStatusChanged = qemuProcessHandleRdmaGidStatusChanged, .domainGuestCrashloaded = qemuProcessHandleGuestCrashloaded, .domainMemoryFailure = qemuProcessHandleMemoryFailure, + .domainMemoryDeviceSizeChange = qemuProcessHandleMemoryDeviceSizeChange, }; static void @@ -2405,21 +2466,36 @@ qemuProcessRefreshBalloonState(virQEMUDriverPtr driver, int asyncJob) { unsigned long long balloon; + size_t i; int rc; - /* if no ballooning is available, the current size equals to the current - * full memory size */ - if (!virDomainDefHasMemballoon(vm->def)) { - vm->def->mem.cur_balloon = virDomainDefGetMemoryTotal(vm->def); - return 0; + if (virDomainDefHasMemballoon(vm->def)) { + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + rc = qemuMonitorGetBalloonInfo(qemuDomainGetMonitor(vm), &balloon); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + return -1; + } else { + balloon = virDomainDefGetMemoryTotal(vm->def); } - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - return -1; + for (i = 0; i < vm->def->nmems; i++) { + virDomainMemoryDefPtr mem = vm->def->mems[i]; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + balloon += mem->actualsize; + break; - rc = qemuMonitorGetBalloonInfo(qemuDomainGetMonitor(vm), &balloon); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - return -1; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + /* nada */ + break; + } + } vm->def->mem.cur_balloon = balloon; -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:26 +0100, Michal Privoznik wrote:
As advertised in previous commit, this event is delivered to us when virtio-mem module changes the allocation inside the guest. It comes with one attribute - size - which holds the new size of the virtio-mem (well, allocated size), in bytes. Mind you, this is not necessarily the same number as 'requested size'. It almost certainly will be when sizing the memory up, but it might not be when sizing the memory down - the guest kernel might be unable to free some blocks.
This actual size is reported in the domain XML as an output element only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 7 +++ docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 24 ++++++++- src/conf/domain_conf.h | 7 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 3 ++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 36 +++++++++++++ src/qemu/qemu_monitor.c | 24 +++++++++ src/qemu/qemu_monitor.h | 20 ++++++++ src/qemu/qemu_monitor_json.c | 24 +++++++++ src/qemu/qemu_process.c | 96 +++++++++++++++++++++++++++++++---- 12 files changed, 236 insertions(+), 12 deletions(-)
[...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8ea7e0df05..1fba9d8302 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1248,10 +1248,30 @@ qemuProcessHandleBalloonChange(qemuMonitorPtr mon G_GNUC_UNUSED, virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + size_t i;
virObjectLock(vm); event = virDomainEventBalloonChangeNewFromObj(vm, actual);
+ VIR_DEBUG("New balloon size before fixup: %lld", actual); + + for (i = 0; i < vm->def->nmems; i++) { + virDomainMemoryDefPtr mem = vm->def->mems[i]; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + actual += mem->actualsize; + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + /* nada */ + break; + } + } + VIR_DEBUG("Updating balloon from %lld to %lld kb", vm->def->mem.cur_balloon, actual); vm->def->mem.cur_balloon = actual;
@@ -2405,21 +2466,36 @@ qemuProcessRefreshBalloonState(virQEMUDriverPtr driver, int asyncJob) { unsigned long long balloon; + size_t i; int rc;
- /* if no ballooning is available, the current size equals to the current - * full memory size */ - if (!virDomainDefHasMemballoon(vm->def)) { - vm->def->mem.cur_balloon = virDomainDefGetMemoryTotal(vm->def); - return 0; + if (virDomainDefHasMemballoon(vm->def)) { + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + rc = qemuMonitorGetBalloonInfo(qemuDomainGetMonitor(vm), &balloon); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + return -1; + } else { + balloon = virDomainDefGetMemoryTotal(vm->def); }
- if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - return -1; + for (i = 0; i < vm->def->nmems; i++) { + virDomainMemoryDefPtr mem = vm->def->mems[i]; + + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + balloon += mem->actualsize; + break;
- rc = qemuMonitorGetBalloonInfo(qemuDomainGetMonitor(vm), &balloon); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - return -1; + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + /* nada */ + break; + } + }
vm->def->mem.cur_balloon = balloon;
I don't think we should count the size of virtio-pmem, virtio-mem against the total VM memory size. The mechanism this is provided to the VM is different compared to "traditional" dimms. The OS needs driver to access the memory. The open question is whether we should add a similar event to what we have for memballoon change. For now I'd probably not do it since a guest OS restart (driver unload) can't actually result in more memory consumed than before.
If the QEMU driver restarts it loses the track of the actual size of virtio-mem (because it's runtime type of information and thus not stored in XML) and therefore, we have to refresh it when reconnecting to the domain monitor. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 36 +++++++++++++++++++---- src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 55 +++++++++++++++++++++--------------- src/qemu/qemu_process.c | 3 ++ 4 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fc994ec282..ea05498b78 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7845,9 +7845,21 @@ qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver, return -1; } - /* if qemu doesn't support the info request, just carry on */ - if (rc == -2) + /* If qemu doesn't support the info request, just carry on, unless we + * really need it. */ + if (rc == -2) { + for (i = 0; i < vm->def->nmems; i++) { + virDomainMemoryDefPtr mem = vm->def->mems[i]; + + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu did not return info on vitio-mem device")); + return -1; + } + } + return 0; + } if (rc < 0) return -1; @@ -7862,9 +7874,23 @@ qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver, if (!(dimm = virHashLookup(meminfo, mem->info.alias))) continue; - mem->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM; - mem->info.addr.dimm.slot = dimm->slot; - mem->info.addr.dimm.base = dimm->address; + switch (mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO: + mem->actualsize = VIR_DIV_UP(dimm->size, 1024); + break; + + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + mem->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM; + mem->info.addr.dimm.slot = dimm->slot; + mem->info.addr.dimm.base = dimm->address; + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + /* nada */ + break; + } } virHashFree(meminfo); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2f9184c122..94bef3d5d5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1393,10 +1393,13 @@ typedef struct _qemuMonitorMemoryDeviceInfo qemuMonitorMemoryDeviceInfo; typedef qemuMonitorMemoryDeviceInfo *qemuMonitorMemoryDeviceInfoPtr; struct _qemuMonitorMemoryDeviceInfo { + /* For pc-dimm */ unsigned long long address; unsigned int slot; bool hotplugged; bool hotpluggable; + /* For virtio-mem */ + unsigned long long size; }; int qemuMonitorGetMemoryDeviceInfo(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3fd925eae1..80f159618a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8241,7 +8241,6 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon, virJSONValuePtr cmd; virJSONValuePtr reply = NULL; virJSONValuePtr data = NULL; - qemuMonitorMemoryDeviceInfoPtr meminfo = NULL; size_t i; if (!(cmd = qemuMonitorJSONMakeCommand("query-memory-devices", NULL))) @@ -8262,6 +8261,9 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon, for (i = 0; i < virJSONValueArraySize(data); i++) { virJSONValuePtr elem = virJSONValueArrayGet(data, i); + g_autofree qemuMonitorMemoryDeviceInfoPtr meminfo = NULL; + virJSONValuePtr dimminfo; + const char *devalias; const char *type; if (!(type = virJSONValueObjectGetString(elem, "type"))) { @@ -8271,26 +8273,23 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon, goto cleanup; } + if (!(dimminfo = virJSONValueObjectGetObject(elem, "data"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-memory-devices reply data doesn't " + "contain enum data")); + goto cleanup; + } + + if (!(devalias = virJSONValueObjectGetString(dimminfo, "id"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("dimm memory info data is missing 'id'")); + goto cleanup; + } + + meminfo = g_new0(qemuMonitorMemoryDeviceInfo, 1); + /* dimm memory devices */ if (STREQ(type, "dimm")) { - virJSONValuePtr dimminfo; - const char *devalias; - - if (!(dimminfo = virJSONValueObjectGetObject(elem, "data"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-memory-devices reply data doesn't " - "contain enum data")); - goto cleanup; - } - - if (!(devalias = virJSONValueObjectGetString(dimminfo, "id"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("dimm memory info data is missing 'id'")); - goto cleanup; - } - - meminfo = g_new0(qemuMonitorMemoryDeviceInfo, 1); - if (virJSONValueObjectGetNumberUlong(dimminfo, "addr", &meminfo->address) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -8321,17 +8320,27 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon, } - if (virHashAddEntry(info, devalias, meminfo) < 0) + } else if (STREQ(type, "virtio-mem")) { + if (virJSONValueObjectGetNumberUlong(dimminfo, "size", + &meminfo->size) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing slot in dimm memory info")); goto cleanup; - - meminfo = NULL; + } + } else { + /* type not handled yet */ + continue; } + + if (virHashAddEntry(info, devalias, meminfo) < 0) + goto cleanup; + + meminfo = NULL; } ret = 0; cleanup: - VIR_FREE(meminfo); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1fba9d8302..286818fa83 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8513,6 +8513,9 @@ qemuProcessReconnect(void *opaque) qemuDomainVcpuPersistOrder(obj->def); + if (qemuDomainUpdateMemoryDeviceInfo(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) + goto error; + if (qemuProcessDetectIOThreadPIDs(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) goto error; -- 2.26.2
What code tries to achieve is that if no flags were provided to either 'setmem' or 'setmaxmem' commands then the old (no flags) API is called to be able to communicate with older daemons. Well, the code can be simplified a bit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 51a9fd90d1..eeeeaa8639 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9018,9 +9018,6 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; - /* none of the options were specified */ - if (!current && !live && !config) - flags = -1; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9037,7 +9034,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) } kibibytes = VIR_DIV_UP(bytes, 1024); - if (flags == -1) { + if (flags == 0) { if (virDomainSetMemory(dom, kibibytes) != 0) ret = false; } else { @@ -9090,7 +9087,7 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); - unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT | VIR_DOMAIN_MEM_MAXIMUM; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); @@ -9099,9 +9096,6 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; - /* none of the options were specified */ - if (!current && !live && !config) - flags = -1; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9118,13 +9112,13 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) } kibibytes = VIR_DIV_UP(bytes, 1024); - if (flags == -1) { + if (flags == 0) { if (virDomainSetMaxMemory(dom, kibibytes) != 0) { vshError(ctl, "%s", _("Unable to change MaxMemorySize")); ret = false; } } else { - if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) { + if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) { vshError(ctl, "%s", _("Unable to change MaxMemorySize")); ret = false; } -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:28 +0100, Michal Privoznik wrote:
What code tries to achieve is that if no flags were provided to either 'setmem' or 'setmaxmem' commands then the old (no flags) API is called to be able to communicate with older daemons. Well, the code can be simplified a bit.
It's not quite the same though.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 51a9fd90d1..eeeeaa8639 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
It's better visible in the second hunk ...
@@ -9090,7 +9087,7 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); - unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT | VIR_DOMAIN_MEM_MAXIMUM; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
VIR_DOMAIN_AFFECT_CURRENT is 0
VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); @@ -9099,9 +9096,6 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; - /* none of the options were specified */ - if (!current && !live && !config) - flags = -1;
In the old code, if --current is used, this condition would be false ...
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9118,13 +9112,13 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) } kibibytes = VIR_DIV_UP(bytes, 1024);
- if (flags == -1) { + if (flags == 0) { if (virDomainSetMaxMemory(dom, kibibytes) != 0) { vshError(ctl, "%s", _("Unable to change MaxMemorySize")); ret = false; } } else { - if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) {
... and the new API is used.
+ if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) { vshError(ctl, "%s", _("Unable to change MaxMemorySize")); ret = false; }
With your change --current does nothing actually. Semantically it should be the same though. Perhaps justify it better in the commit message.
Provide an user friendly way of modifying 'requested-size' of a virtio-mem device. New --node and --alias arguments are introduced but they are needed only if it is not clear which virtio-mem does user mean. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/manpages/virsh.rst | 6 ++ tools/virsh-domain.c | 126 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 122 insertions(+), 10 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9ef6b68422..c3c8c27a18 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4098,6 +4098,7 @@ setmem :: setmem domain size [[--config] [--live] | [--current]] + [--virtio [--node NODE] | [--alias ALIAS]] Change the memory allocation for a guest domain. If *--live* is specified, perform a memory balloon of a running guest. @@ -4107,6 +4108,11 @@ If *--current* is specified, it is equivalent to either *--live* or Both *--live* and *--config* flags may be given, but *--current* is exclusive. If no flag is specified, behavior is different depending on hypervisor. +If *--virtio* is specified then instead of changing the memory allocation for +whole domain the individual memory device with virtio model is changed. If +there is more than one such memory device then *--node* or *--alias* must be +used to specify the device uniquely. *NODE* refers to the guest NUMA node to +which the memory device is attached to and *ALIAS* is the device alias. *size* is a scaled integer (see ``NOTES`` above); it defaults to kibibytes (blocks of 1024 bytes) unless you provide a suffix (and the older option diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index eeeeaa8639..7d8546d806 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8992,12 +8992,101 @@ static const vshCmdOptDef opts_setmem[] = { .flags = VSH_OFLAG_REQ, .help = N_("new memory size, as scaled integer (default KiB)") }, + {.name = "virtio", + .type = VSH_OT_BOOL, + .help = N_("modify virtio memory instead of top level domain memory") + }, + {.name = "node", + .type = VSH_OT_INT, + .help = N_("guest numa node"), + }, + {.name = "alias", + .type = VSH_OT_STRING, + .help = N_("memory device alias"), + }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; + +static int +virshDomainSetmemVirtio(vshControl *ctl, + virDomainPtr dom, + unsigned long kibibytes, + int numaNode, + const char *alias, + unsigned int flags) +{ + g_autoptr(xmlDoc) doc = NULL; + g_autoptr(xmlXPathContext) ctxt = NULL; + g_autofree char *xpath = NULL; + int nmems; + g_autofree xmlNodePtr *mems = NULL; + xmlNodePtr requestedSizeNode = NULL; + g_autofree char *kibibytesStr = NULL; + g_autoptr(xmlBuffer) xmlbuf = NULL; + unsigned int domainXMLFlags = 0; + const char *updatedMemoryXML = NULL; + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + domainXMLFlags |= VIR_DOMAIN_XML_INACTIVE; + + if (virshDomainGetXMLFromDom(ctl, dom, domainXMLFlags, &doc, &ctxt) < 0) + return -1; + + if (alias) { + xpath = g_strdup_printf("/domain/devices/memory[./alias/@name='%s']", alias); + } else if (numaNode >= 0) { + xpath = g_strdup_printf("/domain/devices/memory[./target/node/text()=%d]", numaNode); + } else { + xpath = g_strdup("/domain/devices/memory[@model='virtio' and not(./source/pmem)]"); + } + + nmems = virXPathNodeSet(xpath, ctxt, &mems); + if (nmems < 0) { + vshSaveLibvirtError(); + return -1; + } else if (nmems == 0) { + vshError(ctl, _("no virtio-mem device found")); + return -1; + } else if (nmems > 1) { + vshError(ctl, _("multiple virtio-mem devices found")); + return -1; + } + + ctxt->node = mems[0]; + + requestedSizeNode = virXPathNode("./target/requested", ctxt); + + if (!requestedSizeNode) { + vshError(ctl, _("virtio-mem device is missing <requested/>")); + return -1; + } + + kibibytesStr = g_strdup_printf("%lu", kibibytes); + xmlNodeSetContent(requestedSizeNode, BAD_CAST kibibytesStr); + + if (!(xmlbuf = xmlBufferCreate())) { + vshError(ctl, _("unable to allocate XML buffer")); + return -1; + } + + if (xmlNodeDump(xmlbuf, doc, mems[0], 0, 1) < 0) { + vshError(ctl, _("unable to format new <memory/> node")); + return -1; + } + + updatedMemoryXML = (const char *)xmlBufferContent(xmlbuf); + + if (virDomainUpdateDeviceFlags(dom, updatedMemoryXML, flags) < 0) + return -1; + + return 0; +} + + static bool cmdSetmem(vshControl *ctl, const vshCmd *cmd) { @@ -9005,7 +9094,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) unsigned long long bytes = 0; unsigned long long max; unsigned long kibibytes = 0; - bool ret = true; + bool ret = false; bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); @@ -9013,6 +9102,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + VSH_EXCLUSIVE_OPTIONS("node", "alias"); if (config) flags |= VIR_DOMAIN_AFFECT_CONFIG; @@ -9028,20 +9118,36 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) max = 1024ull * ULONG_MAX; else max = ULONG_MAX; - if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) { - virshDomainFree(dom); - return false; - } + if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) + goto cleanup; + kibibytes = VIR_DIV_UP(bytes, 1024); - if (flags == 0) { - if (virDomainSetMemory(dom, kibibytes) != 0) - ret = false; + if (vshCommandOptBool(cmd, "virtio")) { + int numaNode = -1; + const char *alias = NULL; + + if (vshCommandOptInt(ctl, cmd, "node", &numaNode) < 0) + goto cleanup; + + if (vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0) + goto cleanup; + + if (virshDomainSetmemVirtio(ctl, dom, kibibytes, + numaNode, alias, flags) < 0) + goto cleanup; } else { - if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) - ret = false; + if (flags == 0) { + if (virDomainSetMemory(dom, kibibytes) != 0) + goto cleanup; + } else { + if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) + goto cleanup; + } } + ret = true; + cleanup: virshDomainFree(dom); return ret; } -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:29 +0100, Michal Privoznik wrote:
Provide an user friendly way of modifying 'requested-size' of a virtio-mem device. New --node and --alias arguments are introduced but they are needed only if it is not clear which virtio-mem does user mean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/manpages/virsh.rst | 6 ++ tools/virsh-domain.c | 126 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 122 insertions(+), 10 deletions(-)
NACK to overloading 'setmem'. You have two almost completely distinct code paths with a completely new set of arguments. You can directly add a new command instead.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- NEWS.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 135c4e2fe0..adedf15d2b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -13,6 +13,15 @@ v7.0.0 (unreleased) * **New features** + * qemu: Introduce virtio memory support + + New ``virtio`` model is introduced for ``<memory/>`` device which + translates to ``virtio-pmem`` or ``virtio-mem`` on QEMU command + line. The former servers as a NVDIMM and the latter is a + paravirtualized mechanism of adding/removing memory to/from a VM. + It is exposed via ``virDomainUpdateDeviceFlags()`` API and ``virsh + setmem --virtio`` for user convenience. + * **Improvements** * **Bug fixes** -- 2.26.2
On Thu, Dec 03, 2020 at 13:36:30 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- NEWS.rst | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst index 135c4e2fe0..adedf15d2b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -13,6 +13,15 @@ v7.0.0 (unreleased)
* **New features**
+ * qemu: Introduce virtio memory support + + New ``virtio`` model is introduced for ``<memory/>`` device which + translates to ``virtio-pmem`` or ``virtio-mem`` on QEMU command + line. The former servers as a NVDIMM and the latter is a
IIUC the guest visible portion is rather significantly different from an NVDIMM, so you should not equate them.
+ paravirtualized mechanism of adding/removing memory to/from a VM. + It is exposed via ``virDomainUpdateDeviceFlags()`` API and ``virsh + setmem --virtio`` for user convenience. + * **Improvements**
* **Bug fixes** -- 2.26.2
participants (4)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa -
Thomas Huth