[PATCH v2 0/2] qemu: Do not Use canonical path for system memory
v2 of: https://www.redhat.com/archives/libvir-list/2021-January/msg00601.html diff to v1: - Added a comment to patch 1/2 per Peter's request. - Replaced 'git describe' commit IDs with just their hashes. The qemu patch is still not merged, so I couldn't replace XXX with actual QEMU commits in 2/2. I'm keeping an eye on the patch and will do that once it's merged. https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg01979.html Michal Prívozník (2): qemu_capabilities: Introduce QEMU_CAPS_X_USE_CANONICAL_PATH_FOR_RAMBLOCK_ID qemu: Do not Use canonical path for system memory src/qemu/qemu_capabilities.c | 5 ++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 30 ++++++++++++++++--- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_hotplug.c | 2 +- .../caps_4.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + .../caps_4.0.0.riscv32.xml | 1 + .../caps_4.0.0.riscv64.xml | 1 + .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + .../caps_4.1.0.x86_64.xml | 1 + .../caps_4.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../qemucapabilitiesdata/caps_5.1.0.sparc.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 + .../caps_5.2.0.riscv64.xml | 1 + .../qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 + .../hugepages-memaccess3.x86_64-latest.args | 4 +-- 28 files changed, 59 insertions(+), 8 deletions(-) -- 2.26.2
This capability tracks whether memory-backend-file has "x-use-canonical-path-for-ramblock-id" attribute. Introduced into QEMU by commit fa0cb34d2210cc749b9a70db99bb41c56ad20831. While "x-" prefix is considered experimental or internal to QEMU, the next commit justifies its use. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 + tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 + 24 files changed, 28 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4d132defbd..e7c9b40378 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -609,6 +609,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "ncr53c90", "dc390", "am53c974", + "memory-backend-file.x-use-canonical-path-for-ramblock-id", ); @@ -1665,6 +1666,10 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendFile[] = { "discard-data", QEMU_CAPS_OBJECT_MEMORY_FILE_DISCARD }, { "align", QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN }, { "pmem", QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM }, + /* While we do NOT want to use QEMU's experimental or internal knobs, this + * one is exception. Without this migration is broken. See justification in + * qemuBuildMemoryBackendProps(). You have been warned. */ + { "x-use-canonical-path-for-ramblock-id", QEMU_CAPS_X_USE_CANONICAL_PATH_FOR_RAMBLOCK_ID }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendMemfd[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 0f90efa459..16639b416e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -589,6 +589,7 @@ 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_X_USE_CANONICAL_PATH_FOR_RAMBLOCK_ID, /* -object memory-backend-file,x-use-canonical-path-for-ramblock-id= */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml index 4722557eaf..e4ce221a1a 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml @@ -180,6 +180,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml index 6f549902ca..02e8a7c921 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml @@ -188,6 +188,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml index b1dc08eb4d..7f2d900752 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml @@ -181,6 +181,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml index babb8fb8ab..0f1df27bbf 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml @@ -181,6 +181,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml index 5a15848f88..f67b5027b0 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml @@ -144,6 +144,7 @@ <flag name='migration-param.downtime'/> <flag name='migration-param.xbzrle-cache-size'/> <flag name='fsdev.createmode'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index 43b70ccc94..5db02c1baf 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -225,6 +225,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml index 58774fddcc..036dfd732e 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='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml index 77fdc73415..35b8bb8904 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml @@ -193,6 +193,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml index 15eaac77a6..0e83bf9ca9 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml @@ -194,6 +194,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml index 42a7cca50a..e7d02d52a6 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml @@ -155,6 +155,7 @@ <flag name='migration-param.xbzrle-cache-size'/> <flag name='blockdev-hostdev-scsi'/> <flag name='fsdev.createmode'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100242</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..b2e1f73494 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='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>4002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml index d584642bff..d297a35e62 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml @@ -203,6 +203,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml index 596bccd70a..fda8ec2c78 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml @@ -212,6 +212,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml index eb760f2911..f3cac396ca 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml @@ -199,6 +199,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</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..aaf1e8c6cc 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='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml b/tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml index a293437850..704ca704e0 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml @@ -115,6 +115,7 @@ <flag name='netdev.vhost-vdpa'/> <flag name='fsdev.createmode'/> <flag name='ncr53c90'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</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..6b2744e2e7 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -249,6 +249,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml index cac9b40528..3cc7412b75 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml @@ -207,6 +207,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700243</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml index e92201ad43..62a25a5608 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml @@ -214,6 +214,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900243</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml index bee7f547c7..a2530874f6 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml @@ -201,6 +201,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml b/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml index 15e7ee84c6..a7ea218456 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml @@ -162,6 +162,7 @@ <flag name='block-export-add'/> <flag name='netdev.vhost-vdpa'/> <flag name='fsdev.createmode'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100243</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index cebacc249d..9810c620a9 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -250,6 +250,7 @@ <flag name='fsdev.createmode'/> <flag name='dc390'/> <flag name='am53c974'/> + <flag name='memory-backend-file.x-use-canonical-path-for-ramblock-id'/> <version>5002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> -- 2.26.2
On Thu, Jan 14, 2021 at 13:37:23 +0100, Michal Privoznik wrote:
This capability tracks whether memory-backend-file has "x-use-canonical-path-for-ramblock-id" attribute. Introduced into QEMU by commit fa0cb34d2210cc749b9a70db99bb41c56ad20831. While "x-" prefix is considered experimental or internal to QEMU, the next commit justifies its use.
Since the detection of this feature is not limited to existing qemus, my requirement that qemu must add acknowledgement that "x-use-canonical-path-for-ramblock-id" will be treated as a stable feature from now on and the qemu commit adding that must be mentioned in this commit. The only other way is to limit the feature detection to any released qemu (thus qemu-5.2 and previous), since we still will not get a guarantee that they will not change the flag in the future.
On Thu, Jan 14, 2021 at 13:52:45 +0100, Peter Krempa wrote:
On Thu, Jan 14, 2021 at 13:37:23 +0100, Michal Privoznik wrote:
This capability tracks whether memory-backend-file has "x-use-canonical-path-for-ramblock-id" attribute. Introduced into QEMU by commit fa0cb34d2210cc749b9a70db99bb41c56ad20831. While "x-" prefix is considered experimental or internal to QEMU, the next commit justifies its use.
Since the detection of this feature is not limited to existing qemus, my requirement that qemu must add acknowledgement that "x-use-canonical-path-for-ramblock-id" will be treated as a stable feature from now on and the qemu commit adding that must be mentioned in this commit.
The only other way is to limit the feature detection to any released qemu (thus qemu-5.2 and previous), since we still will not get a guarantee that they will not change the flag in the future.
Let me reiterate the options when this will be acceptable so that we are in the clear: 1) qemu declares "x-use-canonical-path-for-ramblock-id" stable in code and docs (please cc me on the commit) 2) qemu drops the "x-", and ... 2a) we use "use-canonical-path-for-ramblock-id" only with new qemus which have the stable version (this is what we would usually do) 2b) we also add support for "x-use-canonical-path-for-ramblock-id" but detection will be limited to existing releases Supporting "x-use-canonical-path-for-ramblock-id" for any future release of qemu without a clear declaration that it's considered stable in qemu is not acceptable for libvirt.
On 1/14/21 2:10 PM, Peter Krempa wrote:
On Thu, Jan 14, 2021 at 13:52:45 +0100, Peter Krempa wrote:
On Thu, Jan 14, 2021 at 13:37:23 +0100, Michal Privoznik wrote:
This capability tracks whether memory-backend-file has "x-use-canonical-path-for-ramblock-id" attribute. Introduced into QEMU by commit fa0cb34d2210cc749b9a70db99bb41c56ad20831. While "x-" prefix is considered experimental or internal to QEMU, the next commit justifies its use.
Since the detection of this feature is not limited to existing qemus, my requirement that qemu must add acknowledgement that "x-use-canonical-path-for-ramblock-id" will be treated as a stable feature from now on and the qemu commit adding that must be mentioned in this commit.
Is there something concrete you have on mind that you want me to write there? I thought I added a comment around capability detection that justifies its use.
The only other way is to limit the feature detection to any released qemu (thus qemu-5.2 and previous), since we still will not get a guarantee that they will not change the flag in the future.
Let me reiterate the options when this will be acceptable so that we are in the clear:
1) qemu declares "x-use-canonical-path-for-ramblock-id" stable in code and docs (please cc me on the commit)
I've linked that patch in the cover letter. It was sent already so a little too late to CC anybody, sorry
2) qemu drops the "x-", and ... 2a) we use "use-canonical-path-for-ramblock-id" only with new qemus which have the stable version (this is what we would usually do)
Since I'm failing to document our use of this knob maybe this is the way to go.
2b) we also add support for "x-use-canonical-path-for-ramblock-id" but detection will be limited to existing releases
Supporting "x-use-canonical-path-for-ramblock-id" for any future release of qemu without a clear declaration that it's considered stable in qemu is not acceptable for libvirt.
I think that's what the qemu patch I'm linking in the cover letter does. Michal
On Thu, Jan 14, 2021 at 14:39:42 +0100, Michal Privoznik wrote:
On 1/14/21 2:10 PM, Peter Krempa wrote:
On Thu, Jan 14, 2021 at 13:52:45 +0100, Peter Krempa wrote:
On Thu, Jan 14, 2021 at 13:37:23 +0100, Michal Privoznik wrote:
This capability tracks whether memory-backend-file has "x-use-canonical-path-for-ramblock-id" attribute. Introduced into QEMU by commit fa0cb34d2210cc749b9a70db99bb41c56ad20831. While "x-" prefix is considered experimental or internal to QEMU, the next commit justifies its use.
Since the detection of this feature is not limited to existing qemus, my requirement that qemu must add acknowledgement that "x-use-canonical-path-for-ramblock-id" will be treated as a stable feature from now on and the qemu commit adding that must be mentioned in this commit.
Is there something concrete you have on mind that you want me to write there? I thought I added a comment around capability detection that justifies its use.
I've responded to the qemu patch, since I don't consider the wording binding enough: https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg03288.html Once that change is done: Commit message should be more explicit: This capability tracks whether memory-backend-file has "x-use-canonical-path-for-ramblock-id" attribute. Introduced into QEMU by commit fa0cb34d2210cc749b9a70db99bb41c56ad20831. As of QEMU commit $HASH the property is considered stable by qemu despite the 'x-' prefix to preserve compatibility with released qemu versions. And the comment added to the code should be more factual: /* As of QEMU commit $HASH the "x-use-canonical-path-for-ramblock-id" * property is considered stable and supported. The 'x-' prefix was kept * for compatibility with already released qemu versions. */
In commit 88957116c9d3cb4705380c3702c9d4315fb500bb I've adapted libvirt to QEMU's deprecation of -mem-path and -mem-prealloc and switched to memory-backend-* even for system memory. My claim was that that's what QEMU does under the hood anyway. And indeed it was: see QEMU commit 900c0ba373aada4c13d47d95330aa72ec4067ba5 and look at function create_default_memdev(). However, then commit d96c4d5f193e0e45beec80a6277728b32875bddb was merged into QEMU. While it was fixing a bug, it also changed the create_default_memdev() function in which it started turning off use of canonical path (by setting "x-use-canonical-path-for-ramblock-id" attribute to false). This wasn't documented until QEMU commit XXX. The path affects migration - the same path has to be used on the source and on the destination. Therefore, if there is old guest started with '-m X' it has "pc.ram" block which doesn't use canonical path and thus when migrating to newer QEMU which uses memory-backend-* we have to turn off the canonical path explicitly. Otherwise, "/objects/pc.ram" path would be expected by QEMU which doesn't match the source. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912201 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 30 ++++++++++++++++--- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_hotplug.c | 2 +- .../hugepages-memaccess3.x86_64-latest.args | 4 +-- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6f970a3128..b99d4e5faf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2950,7 +2950,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, qemuDomainObjPrivatePtr priv, const virDomainDef *def, const virDomainMemoryDef *mem, - bool force) + bool force, + bool systemMemory) { const char *backendType = "memory-backend-file"; virDomainNumatuneMemMode mode; @@ -2967,6 +2968,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, bool needHugepage = !!pagesize; bool useHugepage = !!pagesize; int discard = mem->discard; + bool useCanonicalPath = true; /* The difference between @needHugepage and @useHugepage is that the latter * is true whenever huge page is defined for the current memory cell. @@ -3081,6 +3083,9 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) return -1; + if (systemMemory) + useCanonicalPath = false; + } else if (useHugepage || mem->nvdimmPath || memAccess || def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) { @@ -3122,10 +3127,27 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) return -1; + + if (systemMemory) + useCanonicalPath = false; + } else { backendType = "memory-backend-ram"; } + /* This is a terrible hack, but unfortunately there is no better way. + * The replacement for '-m X' argument is not simple '-machine + * memory-backend' and '-object memory-backend-*,size=X' (which was the + * idea). This is because of create_default_memdev() in QEMU sets + * 'x-use-canonical-path-for-ramblock-id' attribute to false and is + * documented in QEMU in qemu-options.hx under 'memory-backend'. + * See QEMU commit XXX. + */ + if (!useCanonicalPath && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_X_USE_CANONICAL_PATH_FOR_RAMBLOCK_ID) && + virJSONValueObjectAdd(props, "b:x-use-canonical-path-for-ramblock-id", false, NULL) < 0) + return -1; + if (!priv->memPrealloc && virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0) return -1; @@ -3237,7 +3259,7 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, mem.info.alias = alias; if ((rc = qemuBuildMemoryBackendProps(&props, alias, cfg, - priv, def, &mem, false)) < 0) + priv, def, &mem, false, false)) < 0) return -1; if (virQEMUBuildObjectCommandlineFromJSON(buf, props) < 0) @@ -3266,7 +3288,7 @@ qemuBuildMemoryDimmBackendStr(virBufferPtr buf, alias = g_strdup_printf("mem%s", mem->info.alias); if (qemuBuildMemoryBackendProps(&props, alias, cfg, - priv, def, mem, true) < 0) + priv, def, mem, true, false) < 0) return -1; if (virQEMUBuildObjectCommandlineFromJSON(buf, props) < 0) @@ -7065,7 +7087,7 @@ qemuBuildMemCommandLineMemoryDefaultBackend(virCommandPtr cmd, mem.info.alias = (char *) defaultRAMid; if (qemuBuildMemoryBackendProps(&props, defaultRAMid, cfg, - priv, def, &mem, false) < 0) + priv, def, &mem, false, true) < 0) return -1; if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 3cfe6ff3e9..32cb013172 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -153,7 +153,8 @@ int qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, qemuDomainObjPrivatePtr priv, const virDomainDef *def, const virDomainMemoryDef *mem, - bool force); + bool force, + bool systemMemory); char * qemuBuildMemoryDeviceStr(const virDomainDef *def, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f336a90c8e..18d8cc06f5 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2426,7 +2426,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, goto cleanup; if (qemuBuildMemoryBackendProps(&props, objalias, cfg, - priv, vm->def, mem, true) < 0) + priv, vm->def, mem, true, false) < 0) goto cleanup; if (qemuProcessBuildDestroyMemoryPaths(driver, vm, mem, true) < 0) diff --git a/tests/qemuxml2argvdata/hugepages-memaccess3.x86_64-latest.args b/tests/qemuxml2argvdata/hugepages-memaccess3.x86_64-latest.args index 6033950eab..9396441b92 100644 --- a/tests/qemuxml2argvdata/hugepages-memaccess3.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hugepages-memaccess3.x86_64-latest.args @@ -19,8 +19,8 @@ arch-capabilities=on,ssbd=on,xsaves=on,cmp-legacy=on,amd-ssbd=on,virt-ssbd=on,\ rdctl-no=on,skip-l1dfl-vmentry=on,mds-no=on,pschange-mc-no=on \ -m 4096 \ -object memory-backend-file,id=pc.ram,\ -mem-path=/dev/hugepages2M/libvirt/qemu/-1-fedora,share=yes,prealloc=yes,\ -size=4294967296 \ +mem-path=/dev/hugepages2M/libvirt/qemu/-1-fedora,share=yes,\ +x-use-canonical-path-for-ramblock-id=no,prealloc=yes,size=4294967296 \ -overcommit mem-lock=off \ -smp 4,sockets=4,cores=1,threads=1 \ -uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ -- 2.26.2
On Thu, Jan 14, 2021 at 13:37:24 +0100, Michal Privoznik wrote:
In commit 88957116c9d3cb4705380c3702c9d4315fb500bb I've adapted libvirt to QEMU's deprecation of -mem-path and -mem-prealloc and switched to memory-backend-* even for system memory. My claim was that that's what QEMU does under the hood anyway. And indeed it was: see QEMU commit 900c0ba373aada4c13d47d95330aa72ec4067ba5 and look at function create_default_memdev().
However, then commit d96c4d5f193e0e45beec80a6277728b32875bddb was merged into QEMU. While it was fixing a bug, it also changed the create_default_memdev() function in which it started turning off use of canonical path (by setting "x-use-canonical-path-for-ramblock-id" attribute to false). This wasn't documented until QEMU commit XXX. The path affects migration - the same path has to be used on the source and on the destination. Therefore, if there is old guest started with '-m X' it has "pc.ram" block which doesn't use canonical path and thus when migrating to newer QEMU which uses memory-backend-* we have to turn off the canonical path explicitly. Otherwise, "/objects/pc.ram" path would be expected by QEMU which doesn't match the source.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912201 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 30 ++++++++++++++++--- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_hotplug.c | 2 +- .../hugepages-memaccess3.x86_64-latest.args | 4 +-- 4 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6f970a3128..b99d4e5faf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2950,7 +2950,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, qemuDomainObjPrivatePtr priv, const virDomainDef *def, const virDomainMemoryDef *mem, - bool force) + bool force, + bool systemMemory) { const char *backendType = "memory-backend-file"; virDomainNumatuneMemMode mode; @@ -2967,6 +2968,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, bool needHugepage = !!pagesize; bool useHugepage = !!pagesize; int discard = mem->discard; + bool useCanonicalPath = true;
Preferably invert this to 'disableCanonicalPath' and add a comment which will point to the explanatiol below, so that you save potential readers a re-read once they find the note about the hack below. /* Disabling canonical path is required for migration compatibility of * system memory objects, see below */
@@ -3122,10 +3127,27 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) return -1; + + if (systemMemory) + useCanonicalPath = false; + } else { backendType = "memory-backend-ram"; }
+ /* This is a terrible hack, but unfortunately there is no better way. + * The replacement for '-m X' argument is not simple '-machine + * memory-backend' and '-object memory-backend-*,size=X' (which was the + * idea). This is because of create_default_memdev() in QEMU sets + * 'x-use-canonical-path-for-ramblock-id' attribute to false and is + * documented in QEMU in qemu-options.hx under 'memory-backend'.
Add: Note that QEMU consideres 'x-use-canonical-path-for-ramblock-id' stable and supported despite the 'x-' prefix.
+ * See QEMU commit XXX. + */ + if (!useCanonicalPath && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_X_USE_CANONICAL_PATH_FOR_RAMBLOCK_ID) && + virJSONValueObjectAdd(props, "b:x-use-canonical-path-for-ramblock-id", false, NULL) < 0)
The qemu documentation calls for enabling this property rather than disabling it as the libvirt code does: + b) for machine types 4.0 and older, user shall + use ``x-use-canonical-path-for-ramblock-id=on`` backend option, + if migration to/from old QEMU (<5.0) is expected. + For example: + :: + -object memory-backend-ram,id=pc.ram,size=512M,x-use-canonical-path-for-ramblock-id=on See qapi_bool_parse, which parses 'on' and 'yes' as identical values. Also the at least the commit message should mention why it's okay to use it also for other machine types and newer qemu despite the documentation in qemu.
participants (2)
-
Michal Privoznik -
Peter Krempa