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

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

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
>>
>>      
>
>