Re: [Qemu-devel] Re: [RFC] allow multi-core guests: introduce cores= option to -cpu

From: Gleb Natapov <gleb@redhat.com>
To: Andre Przywara <andre.przywara@amd.com>
Cc: Brian Jackson <iggy@theiggy.com>,
	qemu-devel@nongnu.org, kvm@vger.kernel.org
Subject: Re: [Qemu-devel] Re: [RFC] allow multi-core guests: introduce cores= option to -cpu
Date: Sat, 4 Jul 2009 10:18:06 +0300	[thread overview]
Message-ID: <20090704071806.GA24641@redhat.com> (raw)
In-Reply-To: <4A4E8BAE.7090602@amd.com>

On Sat, Jul 04, 2009 at 12:52:30AM +0200, Andre Przywara wrote:
> Brian Jackson wrote:
>> Andre Przywara wrote:
>>> currently SMP guests happen to see <n> vCPUs as <n> different sockets.
>>> Some guests (Windows comes to mind) have license restrictions and refuse
>>> to run on multi-socket machines.
>>> So lets introduce a "cores=" parameter to the -cpu option to let the user
>>> specify the number of _cores_ the guest should see.
> >> ...
>>>
>> Personally, I'd like to see it as an extra arg to the -smp option. 
>> We've seen too many people use -cpu incorrectly in #kvm, so we've 
>> gotten into the habit of telling people not to touch that option unless 
>> they know exactly what they are doing. Plus it seems odd to have to use 
>> -cpu foo when you just want more cpus, not a specific cpu.
>
> Ok, I see your point. I simply used -cpu because of technical reasons  
> (the core topology is reflected in CPUID, which -cpu cares about). But  
> you are right, it does not belong here, -smp looks like a good candidate  
> (IMO better than -numa). Or we use an abstract "-topology" for this, but  
> this seems like overkill.
> So what about: "-smp 4,cores=2,threads=2[,sockets=1]" to inject 4 vCPUs  
> in one package (automatically determined if omitted) with two cores and  
> two threads/core? All parameters except the number of vCPUs would be  
> optional, which would make this new format backwards compatible.
> Only we have to agree on the default topology: multi-socket like the  
> current implementation or multi-core, which would mimic the most common  
> SMP architecture today. It seems that the latter one causes less  
> problems for guests.
>

There is not point in [,sockets=1] option -smp socketnum[,cores=n][,threads=n]
is backwards compatible already if default for cores and threads is 1.
I have a similar patch locally that does that. What I think is missing
for KVM (and in the future for QEMU) is hotplug support. The current
hotplug code can't hotplug the socket, only one vcpu. What is missing is
the ability to specify number of available sockets vs number of
populated sockets and specify base apic id for each available socket.
Cpu hotplug will get base apic id as a parameter and will hotplug all
logical cpus simultaneously.

BTW I don't see that you change cpulevel to 4 in your patch. Guest
will not use cpu topology information without it.

diff --git a/sysemu.h b/sysemu.h
index 5582633..7187920 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -120,6 +120,8 @@ extern int usb_enabled;
 extern int virtio_balloon;
 extern const char *virtio_balloon_devaddr;
 extern int smp_cpus;
+extern int cpu_cores;
+extern int core_threads;
 extern int cursor_hide;
 extern int graphic_rotate;
 extern int no_quit;
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 87c04e5..585af4c 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -24,6 +24,8 @@
 #include <inttypes.h>
 #include <signal.h>
 
+#include "qemu-common.h"
+#include "sysemu.h"
 #include "cpu.h"
 #include "exec-all.h"
 #include "qemu-common.h"
@@ -122,7 +124,7 @@ static x86_def_t x86_defs[] = {
 #ifdef TARGET_X86_64
     {
         .name = "qemu64",
-        .level = 2,
+        .level = 4,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
         .vendor3 = CPUID_VENDOR_AMD_3,
@@ -194,7 +196,7 @@ static x86_def_t x86_defs[] = {
 #endif
     {
         .name = "qemu32",
-        .level = 2,
+        .level = 4,
         .family = 6,
         .model = 3,
         .stepping = 3,
@@ -260,7 +262,7 @@ static x86_def_t x86_defs[] = {
     },
     {
         .name = "athlon",
-        .level = 2,
+        .level = 4,
         .vendor1 = CPUID_VENDOR_AMD_1,
         .vendor2 = CPUID_VENDOR_AMD_2,
         .vendor3 = CPUID_VENDOR_AMD_3,
@@ -483,6 +485,8 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
     env->cpuid_version |= ((def->model & 0xf) << 4) | ((def->model >> 4) << 16);
     env->cpuid_version |= def->stepping;
     env->cpuid_features = def->features;
+    if (cpu_cores * core_threads > 1)
+        env->cpuid_features |= CPUID_HT;
     env->pat = 0x0007040600070406ULL;
     env->cpuid_ext_features = def->ext_features;
     env->cpuid_ext2_features = def->ext2_features;
@@ -1564,7 +1568,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 1:
         *eax = env->cpuid_version;
-        *ebx = (env->cpuid_apic_id << 24) | 8 << 8; /* CLFLUSH size in quad words, Linux wants it. */
+        *ebx = (env->cpuid_apic_id << 24) | (cpu_cores*core_threads) << 16
+            | 8 << 8; /* CLFLUSH size in quad words, Linux wants it. */
         *ecx = env->cpuid_ext_features;
         *edx = env->cpuid_features;
         break;
@@ -1579,7 +1584,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         /* cache info: needed for Core compatibility */
         switch (count) {
             case 0: /* L1 dcache info */
-                *eax = 0x0000121;
+                *eax = 0x0000121 | ((cpu_cores - 1) << 26);
                 *ebx = 0x1c0003f;
                 *ecx = 0x000003f;
                 *edx = 0x0000001;
diff --git a/vl.c b/vl.c
index 5d86e69..9098603 100644
--- a/vl.c
+++ b/vl.c
@@ -244,6 +244,8 @@ int singlestep = 0;
 const char *assigned_devices[MAX_DEV_ASSIGN_CMDLINE];
 int assigned_devices_index;
 int smp_cpus = 1;
+int cpu_cores = 1;
+int core_threads = 1;
 const char *vnc_display;
 int acpi_enabled = 1;
 int no_hpet = 0;
@@ -5696,12 +5698,23 @@ int main(int argc, char **argv, char **envp)
                 usb_devices_index++;
                 break;
             case QEMU_OPTION_smp:
-                smp_cpus = atoi(optarg);
+            {
+                char *p;
+                char option[128];
+                smp_cpus = strtol(optarg, &p, 10);
                 if (smp_cpus < 1) {
-                    fprintf(stderr, "Invalid number of CPUs\n");
+                    fprintf(stderr, "Invalid number of slots\n");
                     exit(1);
                 }
+                if (*p++ != ',')
+                    break;
+                if (get_param_value(option, 128, "cores", p))
+                    cpu_cores = strtol(option, NULL, 0);
+                if (get_param_value(option, 128, "threads", p))
+                    core_threads = strtol(option, NULL, 0);
+                smp_cpus *= (cpu_cores * core_threads);
                 break;
+            }
 	    case QEMU_OPTION_vnc:
                 display_type = DT_VNC;
 		vnc_display = optarg;
--
			Gleb.
