[PATCH for 7.0.0 v1 00/26] Introduce virtio memory support
Available also here: https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virtio_mem/ There are new virtio variants of pc-dimm and nvdimm devices. This is the first attempt to impalement support for them in libvirt. Thanks to David Hildenbrand for his valuable input! Michal Prívozník (26): 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 docs/formatdomain.rst | 70 +++- 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 | 198 +++++++++- 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 | 52 ++- 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 ++++++- 47 files changed, 1703 insertions(+), 384 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
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 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
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 b1534dcc1e..631165c3fa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24238,6 +24238,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 11/27/20 12:02 PM, Michal Privoznik wrote:
The UUID is guest visible and thus shouldn't change if we want to not break guest ABI.
Fixes: 08ed673901bb5b4f419b37bcce9b11d31ce370e6
Thanks for fixing this up. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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 b1534dcc1e..631165c3fa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24238,6 +24238,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);
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 27/11/2020 16.02, 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);
Looks fine to me, but docs/coding-style.rst still suggest to format code with "indent -l75", so is this really the right thing to do here?
+ qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def))) goto cleanup;
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
Not related to your patch, but an idea for a future clean-up: That QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without ccw) machine that has been removed in QEMU v2.6 already: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d IIRC, that machine was already considered as deprecated since a couple of earlier QEMU releases, so I really doubt that anybody is still using that in production today. Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be removed from libvirt nowadays. Thomas
On 11/30/20 10:38 AM, Thomas Huth wrote:
On 27/11/2020 16.02, 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);
Looks fine to me, but docs/coding-style.rst still suggest to format code with "indent -l75", so is this really the right thing to do here?
It's true that we have 80 characters limit, but that is more of a soft limit and common sense should be used. Personally, I find func( arg1, arg2 ); worse than exceeding that 80 char rule. My common sense tells me that the rule tries to avoid the following pattern (among others): func(arg1, arg2, ...., very_long_list_of_arguments, which, could, easily, go_on_multiple_lines, but, didnt);
+ qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def))) goto cleanup;
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
Not related to your patch, but an idea for a future clean-up: That QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without ccw) machine that has been removed in QEMU v2.6 already:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
IIRC, that machine was already considered as deprecated since a couple of earlier QEMU releases, so I really doubt that anybody is still using that in production today.
Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be removed from libvirt nowadays.
That is even better idea. But currently libvirt supports QEMU-1.5.0 and newer. So I think we shouldn't remove that until the minimum version is bumped even though we think feature has no users. https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4 Although, it might be about time to look again what is the oldest QEMU we need to support. Michal
On Mon, Nov 30, 2020 at 11:18:20AM +0100, Michal Privoznik wrote:
On 11/30/20 10:38 AM, Thomas Huth wrote:
On 27/11/2020 16.02, 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);
Looks fine to me, but docs/coding-style.rst still suggest to format code with "indent -l75", so is this really the right thing to do here?
It's true that we have 80 characters limit, but that is more of a soft limit and common sense should be used. Personally, I find
func( arg1, arg2 );
worse than exceeding that 80 char rule. My common sense tells me that the rule tries to avoid the following pattern (among others):
func(arg1, arg2, ...., very_long_list_of_arguments, which, could, easily, go_on_multiple_lines, but, didnt);
+ qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def))) goto cleanup; } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
Not related to your patch, but an idea for a future clean-up: That QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without ccw) machine that has been removed in QEMU v2.6 already:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
IIRC, that machine was already considered as deprecated since a couple of earlier QEMU releases, so I really doubt that anybody is still using that in production today.
Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be removed from libvirt nowadays.
That is even better idea. But currently libvirt supports QEMU-1.5.0 and newer. So I think we shouldn't remove that until the minimum version is bumped even though we think feature has no users.
https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4
Although, it might be about time to look again what is the oldest QEMU we need to support.
It was set due to the base RHEL-7 QEMU version. RHEL-7 will fall out of our support matrix at start of May 2021, so in a few months time we'll have a massive QEMU version bump we can do, along with a more general cleanup of RHEL-7 vintage cruft. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, 30 Nov 2020 11:18:20 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
On 11/30/20 10:38 AM, Thomas Huth wrote:
On 27/11/2020 16.02, 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(-)
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
Not related to your patch, but an idea for a future clean-up: That QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without ccw) machine that has been removed in QEMU v2.6 already:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82 https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
IIRC, that machine was already considered as deprecated since a couple of earlier QEMU releases, so I really doubt that anybody is still using that in production today.
Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be removed from libvirt nowadays.
That is even better idea. But currently libvirt supports QEMU-1.5.0 and newer. So I think we shouldn't remove that until the minimum version is bumped even though we think feature has no users.
https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4
Although, it might be about time to look again what is the oldest QEMU we need to support.
Would be great if you could bump it enough to get rid of the old virtio-s390 transport :) FWIW, virtio-ccw was introduced in QEMU 1.4, and became the default with QEMU 2.4, although it had supplanted virtio-s390 well before that. What are the criteria for possibly removing support for a feature in libvirt: that nobody would use it in practice, or that nobody would be able to use it?
On 30/11/2020 11.18, Michal Privoznik wrote:
On 11/30/20 10:38 AM, Thomas Huth wrote:
On 27/11/2020 16.02, 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);
Looks fine to me, but docs/coding-style.rst still suggest to format code with "indent -l75", so is this really the right thing to do here?
It's true that we have 80 characters limit, but that is more of a soft limit and common sense should be used. Personally, I find
func( arg1, arg2 );
worse than exceeding that 80 char rule. My common sense tells me that the rule tries to avoid the following pattern (among others):
func(arg1, arg2, ...., very_long_list_of_arguments, which, could, easily, go_on_multiple_lines, but, didnt);
Fair point, but then this should IMHO be reflected in the coding-style doc first. Otherwise the next contributor to this file might simply undo your change to fit everything again into the 80 (or even 75) columns limit...? Thomas
On 11/30/20 12:48 PM, Thomas Huth wrote:
On 30/11/2020 11.18, Michal Privoznik wrote:
On 11/30/20 10:38 AM, Thomas Huth wrote:
On 27/11/2020 16.02, 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);
Looks fine to me, but docs/coding-style.rst still suggest to format code with "indent -l75", so is this really the right thing to do here?
It's true that we have 80 characters limit, but that is more of a soft limit and common sense should be used. Personally, I find
func( arg1, arg2 );
worse than exceeding that 80 char rule. My common sense tells me that the rule tries to avoid the following pattern (among others):
func(arg1, arg2, ...., very_long_list_of_arguments, which, could, easily, go_on_multiple_lines, but, didnt);
Fair point, but then this should IMHO be reflected in the coding-style doc first. Otherwise the next contributor to this file might simply undo your change to fit everything again into the 80 (or even 75) columns limit...?
I can try. But these coding style rules are always hard to get right. There is always some counter example. Well, let me try. Michal
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 631165c3fa..6c1fa93b0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6699,6 +6699,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")); @@ -16713,11 +16719,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
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 6c1fa93b0a..38ddbacd66 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16699,7 +16699,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) @@ -16920,12 +16920,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) { @@ -18598,7 +18599,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) @@ -27884,7 +27885,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 96e6c34553..8ac23fd7d9 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 479bcc0b0c..568c0f6054 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3263,7 +3263,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
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 38ddbacd66..8f10fe14d3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27877,37 +27877,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: @@ -27915,8 +27910,7 @@ virDomainMemorySourceDefFormat(virBufferPtr buf, break; } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</source>\n"); + virXMLFormatElement(buf, "source", NULL, &childBuf); return 0; } -- 2.26.2
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 8f10fe14d3..c4da972f1a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27920,24 +27920,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
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
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 c4da972f1a..60a0fda27e 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); } @@ -6699,7 +6709,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; @@ -16702,15 +16712,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; @@ -16719,14 +16729,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; @@ -18602,15 +18612,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; @@ -24220,15 +24230,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")); @@ -27882,26 +27892,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 8ac23fd7d9..611dac8bc4 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 10fdc7444d..519158eccd 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 568c0f6054..d3321b30e9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2941,10 +2941,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. @@ -2954,6 +2957,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)) { @@ -3059,11 +3079,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) @@ -3081,7 +3101,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", @@ -3108,18 +3128,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 " @@ -3130,13 +3150,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)) @@ -3149,8 +3166,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 1030309b5a..ba1e6365f1 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 579b3c3713..e68df5abe7 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
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 60a0fda27e..d80ab014bb 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; } @@ -6708,7 +6728,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'")); @@ -6726,6 +6747,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; @@ -16740,6 +16794,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; @@ -18624,6 +18686,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; @@ -24221,7 +24289,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 " @@ -24257,6 +24326,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); @@ -27916,6 +28000,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 611dac8bc4..5c37ba6513 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 d3321b30e9..649403d638 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2969,6 +2969,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; @@ -3322,6 +3323,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
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 538551e772..fe6412db5f 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
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
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 519158eccd..b746963b0b 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
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 ba1e6365f1..0800c5d585 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
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 649403d638..ae1b7bc81b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -376,6 +376,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; @@ -3273,7 +3277,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; @@ -3315,12 +3321,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: @@ -7465,7 +7466,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
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 ae1b7bc81b..1ee5110b44 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2940,7 +2940,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; @@ -2974,6 +2975,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; @@ -3003,7 +3009,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) @@ -3078,7 +3084,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, return -1; } - prealloc = true; + wantPrealloc = true; } if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) @@ -3089,11 +3095,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. */ @@ -3126,8 +3132,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) @@ -3282,7 +3288,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", @@ -3291,46 +3297,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 42d147243e..ba904149e8 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
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 d80ab014bb..0a402f1a51 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6769,10 +6769,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; @@ -16797,9 +16810,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: @@ -16835,7 +16864,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; @@ -16854,6 +16884,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; @@ -18668,7 +18719,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) { @@ -24289,6 +24342,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) { @@ -28005,6 +28074,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: @@ -28036,6 +28117,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 5c37ba6513..3f111e994b 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
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 1ee5110b44..a94b81ce15 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2975,9 +2975,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: @@ -3305,7 +3310,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: @@ -3320,6 +3328,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 ba904149e8..86a039eddd 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 0a402f1a51..5db1fee16b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18781,6 +18781,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 3f111e994b..31892c4941 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3580,6 +3580,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 f581676227..1bed019aac 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 391596ba11..677f921920 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7109,6 +7109,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, @@ -7150,6 +7302,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: @@ -7165,7 +7329,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 f2ed165b22..ace7c889d4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4769,3 +4769,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 d301568e40..c792c95c46 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1527,3 +1527,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 723bdb4426..3d94181afb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9635,3 +9635,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 b588722d90..dcf101a165 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -710,3 +710,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 11/27/20 12:03 PM, Michal Privoznik wrote:
As advertised in the previous commit, we want' to be able to
Extra ' after "want". Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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 0a402f1a51..5db1fee16b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18781,6 +18781,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 3f111e994b..31892c4941 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3580,6 +3580,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 f581676227..1bed019aac 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 391596ba11..677f921920 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7109,6 +7109,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, @@ -7150,6 +7302,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: @@ -7165,7 +7329,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 f2ed165b22..ace7c889d4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4769,3 +4769,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 d301568e40..c792c95c46 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1527,3 +1527,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 723bdb4426..3d94181afb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9635,3 +9635,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 b588722d90..dcf101a165 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -710,3 +710,8 @@ int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, int qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, bool *migratable); + +int +qemuMonitorJSONChangeMemoryRequestedSize(qemuMonitorPtr mon, + const char *alias, + unsigned long long requestedsize);
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. TODO: Fix up total domain memory calculation. 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 | 33 ++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 24 ++++++++++++++++++++ src/qemu/qemu_monitor.h | 20 +++++++++++++++++ src/qemu/qemu_monitor_json.c | 24 ++++++++++++++++++++ src/qemu/qemu_process.c | 41 +++++++++++++++++++++++++++++++++++ 12 files changed, 188 insertions(+), 2 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 5db1fee16b..05f5d70cee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18804,6 +18804,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: * @@ -28124,7 +28139,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); @@ -28146,6 +28162,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); @@ -28180,7 +28200,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 31892c4941..633c07b59c 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 */ @@ -3584,6 +3587,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 1bed019aac..0fdec594ba 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 677f921920..0c5db18dff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4289,6 +4289,36 @@ 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 = info->size / 1024; + + endjob: + qemuDomainObjEndJob(driver, vm); +} + + static void qemuProcessEventHandler(void *data, void *opaque) { struct qemuProcessEvent *processEvent = data; @@ -4338,6 +4368,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 ace7c889d4..5f1cae9f48 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1435,6 +1435,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) @@ -4456,6 +4470,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 c792c95c46..63c52ce6e8 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, @@ -511,6 +527,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 3d94181afb..0c050b27b7 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 e68df5abe7..64576377b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1932,6 +1932,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 +2005,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainRdmaGidStatusChanged = qemuProcessHandleRdmaGidStatusChanged, .domainGuestCrashloaded = qemuProcessHandleGuestCrashloaded, .domainMemoryFailure = qemuProcessHandleMemoryFailure, + .domainMemoryDeviceSizeChange = qemuProcessHandleMemoryDeviceSizeChange, }; static void -- 2.26.2
On 11/27/20 12:03 PM, 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.
TODO: Fix up total domain memory calculation.
I don't mind the 'TODO' here, but it would be good to clarify what we can/can't expect while this isn't looked at. E.g. I took the series for a spin in my x86 dev box (apparently pSeries does no support virtio-pmem and virtio-mem, so here I am doing x86 work hehe) and I saw that 'virsh setmem --virtio' does not update the 'maxMemory' of the live XML. Is that the intended effect of this pending 'TODO' the commit is referring to? Code LGTM. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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 | 33 ++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 24 ++++++++++++++++++++ src/qemu/qemu_monitor.h | 20 +++++++++++++++++ src/qemu/qemu_monitor_json.c | 24 ++++++++++++++++++++ src/qemu/qemu_process.c | 41 +++++++++++++++++++++++++++++++++++ 12 files changed, 188 insertions(+), 2 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 5db1fee16b..05f5d70cee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18804,6 +18804,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: * @@ -28124,7 +28139,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);
@@ -28146,6 +28162,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); @@ -28180,7 +28200,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 31892c4941..633c07b59c 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 */ @@ -3584,6 +3587,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 1bed019aac..0fdec594ba 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 677f921920..0c5db18dff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4289,6 +4289,36 @@ 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 = info->size / 1024; + + endjob: + qemuDomainObjEndJob(driver, vm); +} + + static void qemuProcessEventHandler(void *data, void *opaque) { struct qemuProcessEvent *processEvent = data; @@ -4338,6 +4368,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 ace7c889d4..5f1cae9f48 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1435,6 +1435,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) @@ -4456,6 +4470,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 c792c95c46..63c52ce6e8 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, @@ -511,6 +527,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 3d94181afb..0c050b27b7 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 e68df5abe7..64576377b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1932,6 +1932,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 +2005,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainRdmaGidStatusChanged = qemuProcessHandleRdmaGidStatusChanged, .domainGuestCrashloaded = qemuProcessHandleGuestCrashloaded, .domainMemoryFailure = qemuProcessHandleMemoryFailure, + .domainMemoryDeviceSizeChange = qemuProcessHandleMemoryDeviceSizeChange, };
static void
On 11/30/20 10:44 PM, Daniel Henrique Barboza wrote:
On 11/27/20 12:03 PM, 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.
TODO: Fix up total domain memory calculation.
I don't mind the 'TODO' here, but it would be good to clarify what we can/can't expect while this isn't looked at.
E.g. I took the series for a spin in my x86 dev box (apparently pSeries does no support virtio-pmem and virtio-mem, so here I am doing x86 work hehe) and I saw that 'virsh setmem --virtio' does not update the 'maxMemory' of the live XML. Is that the intended effect of this pending 'TODO' the commit is referring to?
That is the part that's missing, yes, that's what the TODO is refering to. Let me see if I can get my head around it this time. Michal
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..3cd5708548 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 = 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 63c52ce6e8..3a0bd6b79d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1402,10 +1402,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 0c050b27b7..39089fe3e6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8409,7 +8409,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))) @@ -8430,6 +8429,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"))) { @@ -8439,26 +8441,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", @@ -8489,17 +8488,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 64576377b8..fb77c6b9d9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8478,6 +8478,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 7203403b31..2784e278b0 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
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> --- tools/virsh-domain.c | 126 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 116 insertions(+), 10 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2784e278b0..b8ded114b3 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 11/27/20 12:02 PM, Michal Privoznik wrote:
Available also here:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virtio_mem/
There are new virtio variants of pc-dimm and nvdimm devices. This is the first attempt to impalement support for them in libvirt.
Thanks to David Hildenbrand for his valuable input!
Series LGTM: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> I did a field test in my x86_64 (since pSeries does not support neither virtio-mem nor virtio-pmem), played around with 'virsh setmem --virtio' and, as a non-x86 expert, looks good to me as well: Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Michal Prívozník (26): 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
docs/formatdomain.rst | 70 +++- 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 | 198 +++++++++- 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 | 52 ++- 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 ++++++- 47 files changed, 1703 insertions(+), 384 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
Works for me on qemu-5.2.0-0.7.rc2.fc34.x86_64, libvirt v6.10.0-61-gd8c9cc3bd0 with this series patched: guest kernel: kernel-5.10.0-0.rc5.20201125git127c501a03d5.85.fc33.x86_64 recompiled with VIRTIO_PMEM Steps: 1. Start VM with <maxMemory> and <numa>: Domain xml: <domain type='kvm' id='5'> <name>pc</name> <uuid>bb508b28-d57b-44bd-9e9c-a134cef24b60</uuid> <maxMemory slots='16' unit='KiB'>20972544</maxMemory> <memory unit='KiB'>1048576</memory> <cpu mode='custom' match='exact' check='full'> <numa> <cell id='0' cpus='0-3' memory='1048576' unit='KiB'/> </numa> </cpu> ... </domain> 2. Attach the virtio memory device: ➜ ~ cat /tmp/virtio-mem.xml <memory model='virtio'> <target> <size unit='KiB'>4194304</size> <node>0</node> <block unit='KiB'>2048</block> <requested unit='KiB'>524288</requested> </target> </memory> ➜ ~ virsh attach-device pc /tmp/virtio-mem.xml Device attached successfully Before memory attached, the VM memory is: [root@localhost ~]# free -m total used free shared buff/cache available Mem: 879 124 601 2 152 615 Swap: 0 0 0 After: [root@localhost ~]# free -m total used free shared buff/cache available Mem: 1391 132 1106 2 153 1074 Swap: 0 0 0 3. Attach the virtio pmem device: ➜ ~ cat /tmp/virtio-pmem.xml <memory model='virtio' access='shared'> <source> <path>/tmp/virtio_pmem</path> <pmem/> </source> <target> <size unit='KiB'>524288</size> </target> </memory> ➜ ~ virsh attach-device pc /tmp/virtio-pmem.xml Device attached successfully Check it in VM: [root@localhost ~]# lsblk NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT vda 252:0 0 10G 0 disk └─vda1 252:1 0 10G 0 part / pmem0 259:0 0 512M 0 disk 3. Try to detach them: ➜ ~ virsh detach-device pc /tmp/virtio-mem.xml error: Failed to detach device from /tmp/virtio-mem.xml error: internal error: unable to execute QEMU command 'device_del': virtio based memory devices cannot be unplugged. ➜ ~ virsh detach-device pc /tmp/virtio-pmem.xml error: Failed to detach device from /tmp/virtio-pmem.xml error: internal error: unable to execute QEMU command 'device_del': virtio based memory devices cannot be unplugged. On Fri, Nov 27, 2020 at 11:05 PM Michal Privoznik <mprivozn@redhat.com> wrote:
Available also here:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virtio_mem/
There are new virtio variants of pc-dimm and nvdimm devices. This is the first attempt to impalement support for them in libvirt.
Thanks to David Hildenbrand for his valuable input!
Michal Prívozník (26): 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
docs/formatdomain.rst | 70 +++- 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 | 198 +++++++++- 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 | 52 ++- 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 ++++++- 47 files changed, 1703 insertions(+), 384 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
-- Tested-by: Han Han <hhan@redhat.com>
participants (6)
-
Cornelia Huck -
Daniel Henrique Barboza -
Daniel P. Berrangé -
Han Han -
Michal Privoznik -
Thomas Huth