[3/3] qemu: command: explicitly error for non-x86 default CPU

Message ID 6a8b5ec69544b9f2582b8b38e3bab1127da6358d.1500075702.git.crobinso@redhat.com
State Accepted
Commit 64665fa82f605a5e9aaafd4327fd7cf53751b1bd
Headers show
Series
  • qemu: command: clean up default cpu formatting
Related show

Commit Message

Cole Robinson July 14, 2017, 11:43 p.m.
The code only currently handles writing an x86 default -cpu
argument, and doesn't know anything about other architectures.
Let's make this explicit rather than leaving ex. qemu ppc64 to
throw an error about -cpu qemu64

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/qemu/qemu_command.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Andrea Bolognani July 15, 2017, 4:40 p.m. | #1
On Fri, 2017-07-14 at 19:43 -0400, Cole Robinson wrote:
> The code only currently handles writing an x86 default -cpu
> argument, and doesn't know anything about other architectures.
> Let's make this explicit rather than leaving ex. qemu ppc64 to
> throw an error about -cpu qemu64
> 
> Signed-off-by: Cole Robinson <crobinso@redhat.com>

This seems to also fix

  https://bugzilla.redhat.com/show_bug.cgi?id=1235511

I'll review the series early next week.
Cole Robinson July 16, 2017, 9:32 p.m. | #2
On 07/15/2017 12:40 PM, Andrea Bolognani wrote:
> On Fri, 2017-07-14 at 19:43 -0400, Cole Robinson wrote:

>> The code only currently handles writing an x86 default -cpu

>> argument, and doesn't know anything about other architectures.

>> Let's make this explicit rather than leaving ex. qemu ppc64 to

>> throw an error about -cpu qemu64

>>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> 

> This seems to also fix

> 

>   https://bugzilla.redhat.com/show_bug.cgi?id=1235511

> 

> I'll review the series early next week.

> 


Thanks, but I don't think it will fix pmu=on for ppc64, just improve the error
scenario. We will still need to extend the code to hardcode the default CPU
model for PPC64 (would be nice if qemu had a generic 'default' value to use
here, or some way to get around specifying a cpu model)

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Ján Tomko July 21, 2017, 2:25 p.m. | #3
On Fri, Jul 14, 2017 at 07:43:06PM -0400, Cole Robinson wrote:
>The code only currently handles writing an x86 default -cpu

>argument, and doesn't know anything about other architectures.

>Let's make this explicit rather than leaving ex. qemu ppc64 to

>throw an error about -cpu qemu64

>

>Signed-off-by: Cole Robinson <crobinso@redhat.com>

>---

> src/qemu/qemu_command.c | 25 ++++++++++++++++++-------

> 1 file changed, 18 insertions(+), 7 deletions(-)

>


ACK

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Aug. 3, 2017, 3:56 p.m. | #4
On 07/21/2017 10:25 AM, Ján Tomko wrote:
> On Fri, Jul 14, 2017 at 07:43:06PM -0400, Cole Robinson wrote:

>> The code only currently handles writing an x86 default -cpu

>> argument, and doesn't know anything about other architectures.

>> Let's make this explicit rather than leaving ex. qemu ppc64 to

>> throw an error about -cpu qemu64

>>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>> src/qemu/qemu_command.c | 25 ++++++++++++++++++-------

>> 1 file changed, 18 insertions(+), 7 deletions(-)

>>

> 

> ACK

> 

> Jan


Thanks, I pushed patch 2 + 3 now which you reviewed, patch 1 was independent
and has some comments.

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aa12479f7..f727e3d30 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6716,17 +6716,11 @@  qemuBuildCpuCommandLine(virCommandPtr cmd,
     virArch hostarch = virArchFromHost();
     char *cpu = NULL, *cpu_flags = NULL;
     bool hasHwVirt = false;
-    const char *default_model;
     int ret = -1;
     virBuffer cpu_buf = VIR_BUFFER_INITIALIZER;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     size_t i;
 
-    if (def->os.arch == VIR_ARCH_I686)
-        default_model = "qemu32";
-    else
-        default_model = "qemu64";
-
     if (def->cpu &&
         (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) {
         if (qemuBuildCpuModelArgStr(driver, def, &cpu_buf, qemuCaps) < 0)
@@ -6768,7 +6762,7 @@  qemuBuildCpuCommandLine(virCommandPtr cmd,
             ((hostarch == VIR_ARCH_X86_64 &&
               strstr(def->emulator, "kvm")) ||
              strstr(def->emulator, "x86_64"))) {
-            virBufferAdd(&cpu_buf, default_model, -1);
+            virBufferAddLit(&cpu_buf, "qemu32");
         }
     }
 
@@ -6915,6 +6909,23 @@  qemuBuildCpuCommandLine(virCommandPtr cmd,
     cpu_flags = virBufferContentAndReset(&buf);
 
     if (cpu_flags && !cpu) {
+        const char *default_model;
+
+        switch (def->os.arch) {
+        case VIR_ARCH_I686:
+            default_model = "qemu32";
+            break;
+        case VIR_ARCH_X86_64:
+            default_model = "qemu64";
+            break;
+        default:
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("CPU flags requested but can't determine "
+                             "default CPU for arch %s"),
+                           virArchToString(def->os.arch));
+            goto cleanup;
+        }
+
         if (VIR_STRDUP(cpu, default_model) < 0)
             goto cleanup;
     }