[PATCH 0/2] qemu: Do not Use canonical path for system memory
Patch 2/2 is where the actual logic is happening. It is also depending on the following QEMU patch: https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg01979.html Once merged I'll replace those two occurrences of 'QEMU commit XXX' with proper commit hash. 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 | 2 ++ 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, 56 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 v4.0.0-rc0~189^2. 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 | 2 ++ 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, 25 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4d132defbd..9850693000 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,7 @@ 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 }, + { "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 Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
experimental or internal to QEMU, the next commit justifies its use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done. We don't want to depend on experimental stuff so we need a strong excuse.
On 1/12/21 12:19 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
experimental or internal to QEMU, the next commit justifies its use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done.
We don't want to depend on experimental stuff so we need a strong excuse.
That's done in the next commit. Do you want me to copy it here too? I figured I'd put the justification where I'm actually setting the internal knob. BTW: I'm linking qemu patch in the cover letter that documents that x-use-canonical-.. is stable and shouldn't be removed/mangled with. Michal
On Tue, Jan 12, 2021 at 12:29:58 +0100, Michal Privoznik wrote:
On 1/12/21 12:19 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
experimental or internal to QEMU, the next commit justifies its use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done.
We don't want to depend on experimental stuff so we need a strong excuse.
That's done in the next commit. Do you want me to copy it here too? I figured I'd put the justification where I'm actually setting the internal knob.
Yes, because this is also mentioning the an 'x-' prefixed property. I want to be absolutely clear in any places (including a comment in the code, which you also should add into the capability code) that this is extraordinary circumstance and that qemu is actually considering that property stable. I want to prevent that this commit will be used as an excuse to depend on experimental properties which are not actually considered non-experimental.
On 1/12/21 12:35 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 12:29:58 +0100, Michal Privoznik wrote:
On 1/12/21 12:19 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
experimental or internal to QEMU, the next commit justifies its use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done.
We don't want to depend on experimental stuff so we need a strong excuse.
That's done in the next commit. Do you want me to copy it here too? I figured I'd put the justification where I'm actually setting the internal knob.
Yes, because this is also mentioning the an 'x-' prefixed property. I want to be absolutely clear in any places (including a comment in the code, which you also should add into the capability code) that this is extraordinary circumstance and that qemu is actually considering that property stable.
I want to prevent that this commit will be used as an excuse to depend on experimental properties which are not actually considered non-experimental.
Alight, fair enough. Let me fix that in v2. Michal
On Tue, 12 Jan 2021 12:35:19 +0100 Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Jan 12, 2021 at 12:29:58 +0100, Michal Privoznik wrote:
On 1/12/21 12:19 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
experimental or internal to QEMU, the next commit justifies its use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done.
We don't want to depend on experimental stuff so we need a strong excuse.
That's done in the next commit. Do you want me to copy it here too? I figured I'd put the justification where I'm actually setting the internal knob.
Yes, because this is also mentioning the an 'x-' prefixed property. I want to be absolutely clear in any places (including a comment in the code, which you also should add into the capability code) that this is extraordinary circumstance and that qemu is actually considering that property stable.
the only reason to keep x- prefix in this case is to cause less issues for downstream QEMUs. Since this compat property is copied to their own machine types. If we keep prefix downstream doesn't have to do anything, if we rename it, then downstreams have to carry a separate patch that does the same for their old machine types.
I want to prevent that this commit will be used as an excuse to depend on experimental properties which are not actually considered non-experimental.
On Tue, Jan 12, 2021 at 19:20:58 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 12:35:19 +0100 Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Jan 12, 2021 at 12:29:58 +0100, Michal Privoznik wrote:
On 1/12/21 12:19 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
experimental or internal to QEMU, the next commit justifies its use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done.
We don't want to depend on experimental stuff so we need a strong excuse.
That's done in the next commit. Do you want me to copy it here too? I figured I'd put the justification where I'm actually setting the internal knob.
Yes, because this is also mentioning the an 'x-' prefixed property. I want to be absolutely clear in any places (including a comment in the code, which you also should add into the capability code) that this is extraordinary circumstance and that qemu is actually considering that property stable.
the only reason to keep x- prefix in this case is to cause less issues for downstream QEMUs. Since this compat property is copied to their own machine types. If we keep prefix downstream doesn't have to do anything, if we rename it, then downstreams have to carry a separate patch that does the same for their old machine types.
That would be okay if it's limited to past versions, but in this instance it is not. Allowing x-prefixed properties for any future release is a dangerous precedent. If we want to allow to detect the capability also for future release, we must declare that it's for a very particular reason and also that qemu will not delete it at will. This is to prevent any future discussions of unwaranted usage of x-prefixed properties in libvirt.
On Tue, Jan 12, 2021 at 07:28:45PM +0100, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 19:20:58 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 12:35:19 +0100 Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Jan 12, 2021 at 12:29:58 +0100, Michal Privoznik wrote:
On 1/12/21 12:19 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
experimental or internal to QEMU, the next commit justifies its use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done.
We don't want to depend on experimental stuff so we need a strong excuse.
That's done in the next commit. Do you want me to copy it here too? I figured I'd put the justification where I'm actually setting the internal knob.
Yes, because this is also mentioning the an 'x-' prefixed property. I want to be absolutely clear in any places (including a comment in the code, which you also should add into the capability code) that this is extraordinary circumstance and that qemu is actually considering that property stable.
the only reason to keep x- prefix in this case is to cause less issues for downstream QEMUs. Since this compat property is copied to their own machine types. If we keep prefix downstream doesn't have to do anything, if we rename it, then downstreams have to carry a separate patch that does the same for their old machine types.
That would be okay if it's limited to past versions, but in this instance it is not. Allowing x-prefixed properties for any future release is a dangerous precedent. If we want to allow to detect the capability also for future release, we must declare that it's for a very particular reason and also that qemu will not delete it at will.
This is to prevent any future discussions of unwaranted usage of x-prefixed properties in libvirt.
Yeah it is pretty dubious on the QEMU side to have used an "x-" prefix here at all, when use of this option is mandatory to make migration work :-( 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 Tue, 12 Jan 2021 18:41:38 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Jan 12, 2021 at 07:28:45PM +0100, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 19:20:58 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 12:35:19 +0100 Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Jan 12, 2021 at 12:29:58 +0100, Michal Privoznik wrote:
On 1/12/21 12:19 PM, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 09:29:49 +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 v4.0.0-rc0~189^2. While "x-" prefix is considered
Please use a commit hash instead of this.
> experimental or internal to QEMU, the next commit justifies its > use.
NACK unless qemu adds a statement to their code and documentation that the this property is considered stable despite the 'x-prefix' and you add a link to the appropriate qemu upstream commit once it's done.
We don't want to depend on experimental stuff so we need a strong excuse.
That's done in the next commit. Do you want me to copy it here too? I figured I'd put the justification where I'm actually setting the internal knob.
Yes, because this is also mentioning the an 'x-' prefixed property. I want to be absolutely clear in any places (including a comment in the code, which you also should add into the capability code) that this is extraordinary circumstance and that qemu is actually considering that property stable.
the only reason to keep x- prefix in this case is to cause less issues for downstream QEMUs. Since this compat property is copied to their own machine types. If we keep prefix downstream doesn't have to do anything, if we rename it, then downstreams have to carry a separate patch that does the same for their old machine types.
That would be okay if it's limited to past versions, but in this instance it is not. Allowing x-prefixed properties for any future release is a dangerous precedent. If we want to allow to detect the capability also for future release, we must declare that it's for a very particular reason and also that qemu will not delete it at will.
This is to prevent any future discussions of unwaranted usage of x-prefixed properties in libvirt.
Yeah it is pretty dubious on the QEMU side to have used an "x-" prefix here at all, when use of this option is mandatory to make migration work :-(
if generic consensus is to drop prefix, I can post a QEMU patch to do so and let downstream(s) to carry burden.
Regards, Daniel
On Tue, Jan 12, 2021 at 20:24:44 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 18:41:38 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Jan 12, 2021 at 07:28:45PM +0100, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 19:20:58 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 12:35:19 +0100 Peter Krempa <pkrempa@redhat.com> wrote
[...]
Yeah it is pretty dubious on the QEMU side to have used an "x-" prefix here at all, when use of this option is mandatory to make migration work :-(
if generic consensus is to drop prefix, I can post a QEMU patch to do so and let downstream(s) to carry burden.
It really depends on the situation, because the commit messages don't seem to describe it satisfactory. Basically we don't want to ever use a qemu property knob, which is qemu free to change arbitrarily. If the property is to be used with any upcoming qemu version we must get a guarantee that it will not change. There are two options basically: 1) 'x-' is dropped 1a) we will use it with qemu-6.0 and later ( everything is clean, but users will have to update qemu to fix it ) 1b) we will carry code which will use 'x-' prefixed version from it's inception until qemu-5.2, when we will hard-mask it out and add plenty comments outlining that this is not what we do normally (it will be okay for past releases, since they will not change) 2) qemu declares the option stable with the 'x-' prefix We'll require that any place even in the code which declares the option has an appropriate comment preventing anybody from changing it. We'll then add also cautionary comments discouraging use of it. 3) qemu fixes the issue without libvirt's involvment For us really 1a) and 3 is acceptable without any comments. Other options will require extraordinary measures to prevent using this as prior art in using any other x-prefixed features from qemu. in 1a) case, downstreams can obviously backport the qemu patch renaming the feature and libvirt will require no change at all Now the question is whether we want to make migration work between the affected releases which will depend what to use.
On Tue, 12 Jan 2021 20:59:11 +0100 Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Jan 12, 2021 at 20:24:44 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 18:41:38 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Jan 12, 2021 at 07:28:45PM +0100, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 19:20:58 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 12:35:19 +0100 Peter Krempa <pkrempa@redhat.com> wrote
[...]
Yeah it is pretty dubious on the QEMU side to have used an "x-" prefix here at all, when use of this option is mandatory to make migration work :-(
if generic consensus is to drop prefix, I can post a QEMU patch to do so and let downstream(s) to carry burden.
It really depends on the situation, because the commit messages don't seem to describe it satisfactory.
Basically we don't want to ever use a qemu property knob, which is qemu free to change arbitrarily.
If the property is to be used with any upcoming qemu version we must get a guarantee that it will not change. There are two options basically:
1) 'x-' is dropped 1a) we will use it with qemu-6.0 and later ( everything is clean, but users will have to update qemu to fix it ) I have thought about it some more, (modulo downstream issue) dropping prefix will effectively exclude old QEMU-(5.0-5.2) even though feature is available there.
1b) we will carry code which will use 'x-' prefixed version from it's inception until qemu-5.2, when we will hard-mask it out and add plenty comments outlining that this is not what we do normally (it will be okay for past releases, since they will not change) 5.2 is not enough, it should be carried till machine type 4.0 exists. On QEMU side we once 4.0 machine type is removed we can deprecate and remove no longer needed option so libvirt (with this patches) would see that it no longer exists and not put it on CLI anymore. Only after that it probably is ok to drop code for it.
2) qemu declares the option stable with the 'x-' prefix We'll require that any place even in the code which declares the option has an appropriate comment preventing anybody from changing it.
We'll then add also cautionary comments discouraging use of it.
I've just resent v2 of QEMU patch that incorporates your suggestions.
3) qemu fixes the issue without libvirt's involvment
if it were possible the option, I'd go for it in the first place. unfortunately, it's too late for it now.
For us really 1a) and 3 is acceptable without any comments. Other options will require extraordinary measures to prevent using this as prior art in using any other x-prefixed features from qemu.
in 1a) case, downstreams can obviously backport the qemu patch renaming the feature and libvirt will require no change at all
Now the question is whether we want to make migration work between the affected releases which will depend what to use. If we can help it, then yes. That's why I resent QEMU patch keeping 'x-' prefix (with your feedback included).
In commit v6.9.0-rc1~450 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 v5.0.0-rc0~75^2~1^2~76 and look at function create_default_memdev(). However, then commit v5.0.0-rc1~11^2~3 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> --- I'll replace both occurrences of 'QEMU commit XXX' once QEMU patch is merged. 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 Tue, 12 Jan 2021 09:29:50 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
In commit v6.9.0-rc1~450 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 v5.0.0-rc0~75^2~1^2~76 and look at function create_default_memdev().
However, then commit v5.0.0-rc1~11^2~3 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> ---
I'll replace both occurrences of 'QEMU commit XXX' once QEMU patch is merged.
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;
is it possible to do it only for old machine types <= 4.0, to limit hack exposure?
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 \
On Tue, Jan 12, 2021 at 20:21:19 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 09:29:50 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
In commit v6.9.0-rc1~450 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 v5.0.0-rc0~75^2~1^2~76 and look at function create_default_memdev().
However, then commit v5.0.0-rc1~11^2~3 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> ---
I'll replace both occurrences of 'QEMU commit XXX' once QEMU patch is merged.
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 ... @@ -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;
is it possible to do it only for old machine types <= 4.0, to limit hack exposure?
We consume machine types as opaque strings, we don't parse them and thus we don't have any ordering on them. Without this telling what <= 4.0 means is impossible. And I don't think we should start doing it, and especially not for limiting this hack as it would be limiting a hack with another one. A reasonable solution would be if we could tell which machine types need (or perhaps don't need) such treatment by probing QEMU for available machine types. Jirka
On Tue, Jan 12, 2021 at 21:13:54 +0100, Jiri Denemark wrote:
On Tue, Jan 12, 2021 at 20:21:19 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 09:29:50 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
[...]
We consume machine types as opaque strings, we don't parse them and thus we don't have any ordering on them. Without this telling what <= 4.0 means is impossible. And I don't think we should start doing it, and especially not for limiting this hack as it would be limiting a hack with another one.
A reasonable solution would be if we could tell which machine types need (or perhaps don't need) such treatment by probing QEMU for available machine types.
Well this has two implications: 1) qemu telling us would be detectable, thus no need to check for x-something 2) if qemu can tell us when to use it, it's very probable that at that point it can do the correct thing itself, not requiring anything from us
On Wed, Jan 13, 2021 at 08:06:30 +0100, Peter Krempa wrote:
On Tue, Jan 12, 2021 at 21:13:54 +0100, Jiri Denemark wrote:
On Tue, Jan 12, 2021 at 20:21:19 +0100, Igor Mammedov wrote:
On Tue, 12 Jan 2021 09:29:50 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
[...]
We consume machine types as opaque strings, we don't parse them and thus we don't have any ordering on them. Without this telling what <= 4.0 means is impossible. And I don't think we should start doing it, and especially not for limiting this hack as it would be limiting a hack with another one.
A reasonable solution would be if we could tell which machine types need (or perhaps don't need) such treatment by probing QEMU for available machine types.
Well this has two implications: 1) qemu telling us would be detectable, thus no need to check for x-something 2) if qemu can tell us when to use it, it's very probable that at that point it can do the correct thing itself, not requiring anything from us
I didn't really study the details, but I guess QEMU cannot do the right thing for old machine types because of backward compatibility. While for new machine types (>4.0) QEMU is automatically doing it right. With this assumption, new QEMU could report some machine types are OK and thus we could skip applying the hack for them. But we would still need it (including the x-... detection) for old machine type and older QEMU releases which didn't mark any machine type as OK. Thus we would not get rid of the x-... hack, but we could properly limit it for old QEMU and old machine types. That is, the additional probing would be an optimization of the hack which we need anyway I'm afraid. You can of course ignore all I said if my assumption is wrong :-) Jirka
participants (5)
-
Daniel P. Berrangé -
Igor Mammedov -
Jiri Denemark -
Michal Privoznik -
Peter Krempa