qemu: Don't ignore failure when building default memory backend

[PATCH] qemu: Don't ignore failure when building default memory backend

Posted by Michal Privoznik 2 years, 2 months ago

When building the default memory backend (which has id='pc.ram')
and no guest NUMA is configured then
qemuBuildMemCommandLineMemoryDefaultBackend() is called. However,
its return value is ignored which means that on invalid
configuration (e.g. when non-existent hugepage size was
requested) an error is reported into the logs but QEMU is started
anyway. And while QEMU does error out its error message doesn't
give much clue what's going on:

  qemu-system-x86_64: Memory backend 'pc.ram' not found

While at it, introduce a test case. While I could chose a nice
looking value (e.g. 4MiB) that's exactly what I wanted to avoid,
because while such value might not be possible on x84_64 it may
be possible on other arches (e.g. ppc is notoriously known for
supporting wide range of HP sizes). Let's stick with obviously
wrong value of 5MiB.

Reported-by: Charles Polisher <chas@chasmo.org>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c                       |  5 ++--
 .../hugepages-default-5M.x86_64-latest.err    |  1 +
 .../qemuxml2argvdata/hugepages-default-5M.xml | 27 +++++++++++++++++++
 tests/qemuxml2argvtest.c                      |  1 +
 4 files changed, 32 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/hugepages-default-5M.x86_64-latest.err
 create mode 100644 tests/qemuxml2argvdata/hugepages-default-5M.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2c963a7297..c836799888 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7369,8 +7369,9 @@ qemuBuildMemCommandLine(virCommand *cmd,
          * regular memory because -mem-path and -mem-prealloc are obsolete.
          * However, if domain has one or more NUMA nodes then there is no
          * default RAM and we mustn't generate the memory object. */
-        if (!virDomainNumaGetNodeCount(def->numa))
-            qemuBuildMemCommandLineMemoryDefaultBackend(cmd, def, priv, defaultRAMid);
+        if (!virDomainNumaGetNodeCount(def->numa) &&
+            qemuBuildMemCommandLineMemoryDefaultBackend(cmd, def, priv, defaultRAMid) < 0)
+            return -1;
     } else {
         /*
          * Add '-mem-path' (and '-mem-prealloc') parameter here if
diff --git a/tests/qemuxml2argvdata/hugepages-default-5M.x86_64-latest.err b/tests/qemuxml2argvdata/hugepages-default-5M.x86_64-latest.err
new file mode 100644
index 0000000000..bf5e54c9e4
--- /dev/null
+++ b/tests/qemuxml2argvdata/hugepages-default-5M.x86_64-latest.err
@@ -0,0 +1 @@
+internal error: Unable to find any usable hugetlbfs mount for 5120 KiB
diff --git a/tests/qemuxml2argvdata/hugepages-default-5M.xml b/tests/qemuxml2argvdata/hugepages-default-5M.xml
new file mode 100644
index 0000000000..280ea4bb71
--- /dev/null
+++ b/tests/qemuxml2argvdata/hugepages-default-5M.xml
@@ -0,0 +1,27 @@
+<domain type="kvm">
+  <name>NonExistentPageSize</name>
+  <uuid>21433e10-aea8-434a-8f81-55781c2e9035</uuid>
+  <memory unit="KiB">4194304</memory>
+  <currentMemory unit="KiB">4194304</currentMemory>
+  <memoryBacking>
+    <hugepages>
+      <page size="5" unit="MiB"/>
+    </hugepages>
+  </memoryBacking>
+  <vcpu placement="static">2</vcpu>
+  <os>
+    <type arch="x86_64" machine="pc">hvm</type>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset="utc"/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>restart</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 9c5c394e03..a32c5a8250 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1270,6 +1270,7 @@ mymain(void)
     DO_TEST("hugepages-default", QEMU_CAPS_OBJECT_MEMORY_FILE);
     DO_TEST("hugepages-default-2M", QEMU_CAPS_OBJECT_MEMORY_FILE);
     DO_TEST("hugepages-default-system-size", QEMU_CAPS_OBJECT_MEMORY_FILE);
+    DO_TEST_CAPS_LATEST_FAILURE("hugepages-default-5M");
     DO_TEST_PARSE_ERROR_NOCAPS("hugepages-default-1G-nodeset-2M");
     DO_TEST("hugepages-nodeset", QEMU_CAPS_OBJECT_MEMORY_FILE);
     DO_TEST_PARSE_ERROR("hugepages-nodeset-nonexist",
-- 
2.34.1

Re: [PATCH] qemu: Don't ignore failure when building default memory backend

Posted by Ján Tomko 2 years, 2 months ago

On a Tuesday in 2022, Michal Privoznik wrote:
>When building the default memory backend (which has id='pc.ram')
>and no guest NUMA is configured then
>qemuBuildMemCommandLineMemoryDefaultBackend() is called. However,
>its return value is ignored which means that on invalid
>configuration (e.g. when non-existent hugepage size was
>requested) an error is reported into the logs but QEMU is started
>anyway. And while QEMU does error out its error message doesn't
>give much clue what's going on:
>
>  qemu-system-x86_64: Memory backend 'pc.ram' not found
>
>While at it, introduce a test case. While I could chose a nice
>looking value (e.g. 4MiB) that's exactly what I wanted to avoid,
>because while such value might not be possible on x84_64 it may
>be possible on other arches (e.g. ppc is notoriously known for
>supporting wide range of HP sizes). Let's stick with obviously
>wrong value of 5MiB.
>
>Reported-by: Charles Polisher <chas@chasmo.org>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_command.c                       |  5 ++--
> .../hugepages-default-5M.x86_64-latest.err    |  1 +
> .../qemuxml2argvdata/hugepages-default-5M.xml | 27 +++++++++++++++++++
> tests/qemuxml2argvtest.c                      |  1 +
> 4 files changed, 32 insertions(+), 2 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/hugepages-default-5M.x86_64-latest.err
> create mode 100644 tests/qemuxml2argvdata/hugepages-default-5M.xml
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

Re: [PATCH] qemu: Don't ignore failure when building default memory backend

Posted by Michal Prívozník 2 years, 2 months ago

On 2/22/22 17:21, Ján Tomko wrote:
> On a Tuesday in 2022, Michal Privoznik wrote:
>> When building the default memory backend (which has id='pc.ram')
>> and no guest NUMA is configured then
>> qemuBuildMemCommandLineMemoryDefaultBackend() is called. However,
>> its return value is ignored which means that on invalid
>> configuration (e.g. when non-existent hugepage size was
>> requested) an error is reported into the logs but QEMU is started
>> anyway. And while QEMU does error out its error message doesn't
>> give much clue what's going on:
>>
>>  qemu-system-x86_64: Memory backend 'pc.ram' not found
>>
>> While at it, introduce a test case. While I could chose a nice
>> looking value (e.g. 4MiB) that's exactly what I wanted to avoid,
>> because while such value might not be possible on x84_64 it may
>> be possible on other arches (e.g. ppc is notoriously known for
>> supporting wide range of HP sizes). Let's stick with obviously
>> wrong value of 5MiB.
>>
>> Reported-by: Charles Polisher <chas@chasmo.org>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_command.c                       |  5 ++--
>> .../hugepages-default-5M.x86_64-latest.err    |  1 +
>> .../qemuxml2argvdata/hugepages-default-5M.xml | 27 +++++++++++++++++++
>> tests/qemuxml2argvtest.c                      |  1 +
>> 4 files changed, 32 insertions(+), 2 deletions(-)
>> create mode 100644
>> tests/qemuxml2argvdata/hugepages-default-5M.x86_64-latest.err
>> create mode 100644 tests/qemuxml2argvdata/hugepages-default-5M.xml
>>
> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> 

Thanks, pushed now.

Michal