Patchwork Re: [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

login
register
mail settings
Submitter Anthony Liguori
Date Oct. 7, 2010, 12:48 p.m.
Message ID <4CADC186.3050309@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/67050/
State New
Headers show

Comments

Anthony Liguori - Oct. 7, 2010, 12:48 p.m.
On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
>    
>>>> +    qemu_compat_version = machine->compat_version;
>>>> +
>>>>        if (display_type == DT_NOGRAPHIC) {
>>>>            if (default_parallel)
>>>>                add_device_config(DEV_PARALLEL, "null");
>>>> -- 
>>>> 1.7.0.4
>>>>
>>>>          
>>> Looks fine to me, given CPUs are not in qdev. Anthony?
>>>
>>>        
>> The idea is fine, but why not just add the default CPU to the machine
>> description?
>>      
> If I remember correctly the reason was that the machine description was
> not accessible in the cpuid initialization path because it is a function
> local variable.

Not tested at all but I think the attached patch addresses it in a 
pretty nice way.

There's a couple ways you could support your patch on top of this.  You 
could add a kvm_cpu_model to the machine structure that gets defaulted 
too if kvm_enabled().  You could also introduce a new KVM machine type 
that gets defaulted to if no explicit machine is specified.

>   I could have made it a global variable but considered
> the compat_version approach simpler. The qemu_compat_version might also
> be useful at other places.
>    

The reason we've avoided having a builtin notion of versions is that we 
have many downstreams where versioning would get very complicated.  If 
we stick to features it makes it much easier for downstreams.

Regards,

Anthony Liguori

> 	Joerg
>
>
Joerg Roedel - Oct. 18, 2010, 8:22 a.m.
(Sorry for the late reply)

On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote:
> On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
> > On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
> >    
> >>>> +    qemu_compat_version = machine->compat_version;
> >>>> +
> >>>>        if (display_type == DT_NOGRAPHIC) {
> >>>>            if (default_parallel)
> >>>>                add_device_config(DEV_PARALLEL, "null");
> >>>> -- 
> >>>> 1.7.0.4
> >>>>
> >>>>          
> >>> Looks fine to me, given CPUs are not in qdev. Anthony?
> >>>
> >>>        
> >> The idea is fine, but why not just add the default CPU to the machine
> >> description?
> >>      
> > If I remember correctly the reason was that the machine description was
> > not accessible in the cpuid initialization path because it is a function
> > local variable.
> 
> Not tested at all but I think the attached patch addresses it in a 
> pretty nice way.
> 
> There's a couple ways you could support your patch on top of this.  You 
> could add a kvm_cpu_model to the machine structure that gets defaulted 
> too if kvm_enabled().  You could also introduce a new KVM machine type 
> that gets defaulted to if no explicit machine is specified.

I had something similar in mind but then I realized that we need at
least a cpu_model and a cpu_model_kvm to distinguish between the TCG and
the KVM case.
Further the QEMUMachine data structure is used for all architectures in
QEMU and the model-names only make sense for x86. So I decided for the
comapt-version way (which doesn't mean I object against this one ;-) )

	Joerg

> From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori <aliguori@us.ibm.com>
> Date: Thu, 7 Oct 2010 07:43:42 -0500
> Subject: [PATCH] machine: make default cpu model part of machine structure
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/hw/boards.h b/hw/boards.h
> index 6f0f0d7..8c6ef27 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -16,6 +16,7 @@ typedef struct QEMUMachine {
>      const char *name;
>      const char *alias;
>      const char *desc;
> +    const char *cpu_model;
>      QEMUMachineInitFunc *init;
>      int use_scsi;
>      int max_cpus;
> diff --git a/hw/pc.c b/hw/pc.c
> index 69b13bf..0826107 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model)
>      int i;
>  
>      /* init CPUs */
> -    if (cpu_model == NULL) {
> -#ifdef TARGET_X86_64
> -        cpu_model = "qemu64";
> -#else
> -        cpu_model = "qemu32";
> -#endif
> -    }
> -
>      for(i = 0; i < smp_cpus; i++) {
>          pc_new_cpu(cpu_model);
>      }
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 12359a7..919b4d6 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size,
>                          const char *initrd_filename,
>                          const char *cpu_model)
>  {
> -    if (cpu_model == NULL)
> -        cpu_model = "486";
>      pc_init1(ram_size, boot_device,
>               kernel_filename, kernel_cmdline,
>               initrd_filename, cpu_model, 0);
>  }
>  
> +#ifdef TARGET_X86_64
> +#define DEF_CPU_MODEL "qemu64"
> +#else
> +#define DEF_CPU_MODEL "qemu32"
> +#endif
> +
>  static QEMUMachine pc_machine = {
>      .name = "pc-0.13",
>      .alias = "pc",
>      .desc = "Standard PC",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .is_default = 1,
> @@ -223,6 +228,7 @@ static QEMUMachine pc_machine = {
>  static QEMUMachine pc_machine_v0_12 = {
>      .name = "pc-0.12",
>      .desc = "Standard PC",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> @@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = {
>  static QEMUMachine pc_machine_v0_11 = {
>      .name = "pc-0.11",
>      .desc = "Standard PC, qemu 0.11",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> @@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = {
>  static QEMUMachine pc_machine_v0_10 = {
>      .name = "pc-0.10",
>      .desc = "Standard PC, qemu 0.10",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> @@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = {
>  static QEMUMachine isapc_machine = {
>      .name = "isapc",
>      .desc = "ISA-only PC",
> +    .cpu_model = "486",
>      .init = pc_init_isa,
>      .max_cpus = 1,
>  };
> diff --git a/vl.c b/vl.c
> index df414ef..3a55cc8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp)
>      }
>      qemu_add_globals();
>  
> +    if (cpu_model == NULL) {
> +        cpu_model = machine->cpu_model;
> +    }
> +
>      machine->init(ram_size, boot_devices,
>                    kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
>  
> -- 
> 1.7.0.4
>
Anthony Liguori - Oct. 18, 2010, 2:16 p.m.
On 10/18/2010 03:22 AM, Roedel, Joerg wrote:
> (Sorry for the late reply)
>
> On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote:
>    
>> On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
>>      
>>> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
>>>
>>>        
>>>>>> +    qemu_compat_version = machine->compat_version;
>>>>>> +
>>>>>>         if (display_type == DT_NOGRAPHIC) {
>>>>>>             if (default_parallel)
>>>>>>                 add_device_config(DEV_PARALLEL, "null");
>>>>>> -- 
>>>>>> 1.7.0.4
>>>>>>
>>>>>>
>>>>>>              
>>>>> Looks fine to me, given CPUs are not in qdev. Anthony?
>>>>>
>>>>>
>>>>>            
>>>> The idea is fine, but why not just add the default CPU to the machine
>>>> description?
>>>>
>>>>          
>>> If I remember correctly the reason was that the machine description was
>>> not accessible in the cpuid initialization path because it is a function
>>> local variable.
>>>        
>> Not tested at all but I think the attached patch addresses it in a
>> pretty nice way.
>>
>> There's a couple ways you could support your patch on top of this.  You
>> could add a kvm_cpu_model to the machine structure that gets defaulted
>> too if kvm_enabled().  You could also introduce a new KVM machine type
>> that gets defaulted to if no explicit machine is specified.
>>      
> I had something similar in mind but then I realized that we need at
> least a cpu_model and a cpu_model_kvm to distinguish between the TCG and
> the KVM case.
>    

I would think that having different default machines for KVM and TCG 
would be a better solution.

> Further the QEMUMachine data structure is used for all architectures in
> QEMU and the model-names only make sense for x86.

SPARC uses cpu_model too FWIW.  I believe Blue Swirl has even discussed 
using a feature-format similar to how x86 does it for SPARC CPUs.

Regards,

Anthony Liguori

>   So I decided for the
> comapt-version way (which doesn't mean I object against this one ;-) )
>
> 	Joerg
>
>    
>>  From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001
>> From: Anthony Liguori<aliguori@us.ibm.com>
>> Date: Thu, 7 Oct 2010 07:43:42 -0500
>> Subject: [PATCH] machine: make default cpu model part of machine structure
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/hw/boards.h b/hw/boards.h
>> index 6f0f0d7..8c6ef27 100644
>> --- a/hw/boards.h
>> +++ b/hw/boards.h
>> @@ -16,6 +16,7 @@ typedef struct QEMUMachine {
>>       const char *name;
>>       const char *alias;
>>       const char *desc;
>> +    const char *cpu_model;
>>       QEMUMachineInitFunc *init;
>>       int use_scsi;
>>       int max_cpus;
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 69b13bf..0826107 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model)
>>       int i;
>>
>>       /* init CPUs */
>> -    if (cpu_model == NULL) {
>> -#ifdef TARGET_X86_64
>> -        cpu_model = "qemu64";
>> -#else
>> -        cpu_model = "qemu32";
>> -#endif
>> -    }
>> -
>>       for(i = 0; i<  smp_cpus; i++) {
>>           pc_new_cpu(cpu_model);
>>       }
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 12359a7..919b4d6 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size,
>>                           const char *initrd_filename,
>>                           const char *cpu_model)
>>   {
>> -    if (cpu_model == NULL)
>> -        cpu_model = "486";
>>       pc_init1(ram_size, boot_device,
>>                kernel_filename, kernel_cmdline,
>>                initrd_filename, cpu_model, 0);
>>   }
>>
>> +#ifdef TARGET_X86_64
>> +#define DEF_CPU_MODEL "qemu64"
>> +#else
>> +#define DEF_CPU_MODEL "qemu32"
>> +#endif
>> +
>>   static QEMUMachine pc_machine = {
>>       .name = "pc-0.13",
>>       .alias = "pc",
>>       .desc = "Standard PC",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .is_default = 1,
>> @@ -223,6 +228,7 @@ static QEMUMachine pc_machine = {
>>   static QEMUMachine pc_machine_v0_12 = {
>>       .name = "pc-0.12",
>>       .desc = "Standard PC",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .compat_props = (GlobalProperty[]) {
>> @@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = {
>>   static QEMUMachine pc_machine_v0_11 = {
>>       .name = "pc-0.11",
>>       .desc = "Standard PC, qemu 0.11",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .compat_props = (GlobalProperty[]) {
>> @@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = {
>>   static QEMUMachine pc_machine_v0_10 = {
>>       .name = "pc-0.10",
>>       .desc = "Standard PC, qemu 0.10",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .compat_props = (GlobalProperty[]) {
>> @@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = {
>>   static QEMUMachine isapc_machine = {
>>       .name = "isapc",
>>       .desc = "ISA-only PC",
>> +    .cpu_model = "486",
>>       .init = pc_init_isa,
>>       .max_cpus = 1,
>>   };
>> diff --git a/vl.c b/vl.c
>> index df414ef..3a55cc8 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp)
>>       }
>>       qemu_add_globals();
>>
>> +    if (cpu_model == NULL) {
>> +        cpu_model = machine->cpu_model;
>> +    }
>> +
>>       machine->init(ram_size, boot_devices,
>>                     kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
>>
>> -- 
>> 1.7.0.4
>>
>>      
>
>
Blue Swirl - Oct. 20, 2010, 3:53 p.m.
On Mon, Oct 18, 2010 at 2:16 PM, Anthony Liguori
<aliguori@linux.vnet.ibm.com> wrote:
> On 10/18/2010 03:22 AM, Roedel, Joerg wrote:
>>
>> (Sorry for the late reply)
>>
>> On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote:
>>
>>>
>>> On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
>>>
>>>>
>>>> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
>>>>
>>>>
>>>>>>>
>>>>>>> +    qemu_compat_version = machine->compat_version;
>>>>>>> +
>>>>>>>        if (display_type == DT_NOGRAPHIC) {
>>>>>>>            if (default_parallel)
>>>>>>>                add_device_config(DEV_PARALLEL, "null");
>>>>>>> --
>>>>>>> 1.7.0.4
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Looks fine to me, given CPUs are not in qdev. Anthony?
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> The idea is fine, but why not just add the default CPU to the machine
>>>>> description?
>>>>>
>>>>>
>>>>
>>>> If I remember correctly the reason was that the machine description was
>>>> not accessible in the cpuid initialization path because it is a function
>>>> local variable.
>>>>
>>>
>>> Not tested at all but I think the attached patch addresses it in a
>>> pretty nice way.
>>>
>>> There's a couple ways you could support your patch on top of this.  You
>>> could add a kvm_cpu_model to the machine structure that gets defaulted
>>> too if kvm_enabled().  You could also introduce a new KVM machine type
>>> that gets defaulted to if no explicit machine is specified.
>>>
>>
>> I had something similar in mind but then I realized that we need at
>> least a cpu_model and a cpu_model_kvm to distinguish between the TCG and
>> the KVM case.
>>
>
> I would think that having different default machines for KVM and TCG would
> be a better solution.
>
>> Further the QEMUMachine data structure is used for all architectures in
>> QEMU and the model-names only make sense for x86.
>
> SPARC uses cpu_model too FWIW.  I believe Blue Swirl has even discussed
> using a feature-format similar to how x86 does it for SPARC CPUs.

Actually I copied Sparc feature support from x86. Generic feature
support would be nice.

Patch

From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Thu, 7 Oct 2010 07:43:42 -0500
Subject: [PATCH] machine: make default cpu model part of machine structure

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/hw/boards.h b/hw/boards.h
index 6f0f0d7..8c6ef27 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -16,6 +16,7 @@  typedef struct QEMUMachine {
     const char *name;
     const char *alias;
     const char *desc;
+    const char *cpu_model;
     QEMUMachineInitFunc *init;
     int use_scsi;
     int max_cpus;
diff --git a/hw/pc.c b/hw/pc.c
index 69b13bf..0826107 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -866,14 +866,6 @@  void pc_cpus_init(const char *cpu_model)
     int i;
 
     /* init CPUs */
-    if (cpu_model == NULL) {
-#ifdef TARGET_X86_64
-        cpu_model = "qemu64";
-#else
-        cpu_model = "qemu32";
-#endif
-    }
-
     for(i = 0; i < smp_cpus; i++) {
         pc_new_cpu(cpu_model);
     }
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 12359a7..919b4d6 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -204,17 +204,22 @@  static void pc_init_isa(ram_addr_t ram_size,
                         const char *initrd_filename,
                         const char *cpu_model)
 {
-    if (cpu_model == NULL)
-        cpu_model = "486";
     pc_init1(ram_size, boot_device,
              kernel_filename, kernel_cmdline,
              initrd_filename, cpu_model, 0);
 }
 
+#ifdef TARGET_X86_64
+#define DEF_CPU_MODEL "qemu64"
+#else
+#define DEF_CPU_MODEL "qemu32"
+#endif
+
 static QEMUMachine pc_machine = {
     .name = "pc-0.13",
     .alias = "pc",
     .desc = "Standard PC",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .is_default = 1,
@@ -223,6 +228,7 @@  static QEMUMachine pc_machine = {
 static QEMUMachine pc_machine_v0_12 = {
     .name = "pc-0.12",
     .desc = "Standard PC",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
@@ -242,6 +248,7 @@  static QEMUMachine pc_machine_v0_12 = {
 static QEMUMachine pc_machine_v0_11 = {
     .name = "pc-0.11",
     .desc = "Standard PC, qemu 0.11",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
@@ -277,6 +284,7 @@  static QEMUMachine pc_machine_v0_11 = {
 static QEMUMachine pc_machine_v0_10 = {
     .name = "pc-0.10",
     .desc = "Standard PC, qemu 0.10",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
@@ -324,6 +332,7 @@  static QEMUMachine pc_machine_v0_10 = {
 static QEMUMachine isapc_machine = {
     .name = "isapc",
     .desc = "ISA-only PC",
+    .cpu_model = "486",
     .init = pc_init_isa,
     .max_cpus = 1,
 };
diff --git a/vl.c b/vl.c
index df414ef..3a55cc8 100644
--- a/vl.c
+++ b/vl.c
@@ -2904,6 +2904,10 @@  int main(int argc, char **argv, char **envp)
     }
     qemu_add_globals();
 
+    if (cpu_model == NULL) {
+        cpu_model = machine->cpu_model;
+    }
+
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
-- 
1.7.0.4