qemu-kvm: Add CPUID support for VIA CPU

When KVM is running on VIA CPU with host cpu's model, the feautures of
VIA CPU will be passed into kvm guest by calling the CPUID instruction
for Centaur.

Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
Signed-off-by: KaryJin<karyjin@viatech.com.cn>
---
 target-i386/cpu.h   |    7 +++++++
 target-i386/cpuid.c |   48
+++++++++++++++++++++++++++++++++++++++++++++++-
 target-i386/kvm.c   |   15 +++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

 
 #ifdef KVM_CAP_MCE

Hi,

the subject's tag (qemu-kvm) is misleading. This is actually targeting
the uq/master patch queue, i.e. the upstream kvm staging area.

On 2011-05-05 05:03, BrillyWu@viatech.com.cn wrote:
> When KVM is running on VIA CPU with host cpu's model, the feautures of
> VIA CPU will be passed into kvm guest by calling the CPUID instruction
> for Centaur.
> 
> Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
> Signed-off-by: KaryJin<karyjin@viatech.com.cn>
> ---
>  target-i386/cpu.h   |    7 +++++++
>  target-i386/cpuid.c |   48
> +++++++++++++++++++++++++++++++++++++++++++++++-

You patch is unfortunately line-wrapped.

>  target-i386/kvm.c   |   15 +++++++++++++++
>  3 files changed, 69 insertions(+), 1 deletion(-)
> 
> --- a/target-i386/cpu.h	2011-05-05 09:01:11.742328398 +0800
> +++ b/target-i386/cpu.h	2011-05-05 10:47:32.112329696 +0800
> @@ -441,6 +441,10 @@
>  #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */ 
>  #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
>  
> +#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
> +#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
> +#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
> +
>  #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability
> */
>  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
>  
> @@ -721,6 +725,9 @@ typedef struct CPUX86State {
>      uint32_t cpuid_ext3_features;
>      uint32_t cpuid_apic_id;
>      int cpuid_vendor_override;
> +    /*Store the results of Centaur's CPUID instructions*/

Please format comments like this /* comment text */, ie. with blanks
after/before the /* / */.

> +    uint32_t cpuid_xlevel2;
> +    uint32_t cpuid_ext4_features;
>  
>      /* MTRRs */
>      uint64_t mtrr_fixed[11];
> --- a/target-i386/cpuid.c	2011-05-05 09:01:05.352331142 +0800
> +++ b/target-i386/cpuid.c	2011-05-05 10:47:41.102330705 +0800
> @@ -230,6 +230,9 @@ typedef struct x86_def_t {
>      char model_id[48];
>      int vendor_override;
>      uint32_t flags;
> +    /*Store the results of Centaur's CPUID instructions*/
> +    uint32_t ext4_features;
> +    uint32_t xlevel2;
>  } x86_def_t;
>  
>  #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) @@ -522,6
> +525,17 @@ static int cpu_x86_fill_host(x86_def_t *
>      cpu_x86_fill_model_id(x86_cpu_def->model_id);
>      x86_cpu_def->vendor_override = 0;
>  
> +    /* Call Centaur's CPUID instruction. */
> +    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
> +	x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
> +	x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
> +	    host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> +	    if (eax >= 0xC0000001) {
> +		x86_cpu_def->xlevel2 = eax; /*support VIA max extended
> level*/
> +		host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
> +		x86_cpu_def->ext4_features = edx;
> +	    }
> +    }
>  
>      /*
>       * Every SVM feature requires emulation support in KVM - so we
> can't just @@ -855,6 +869,8 @@ int cpu_x86_register (CPUX86State *env,
>      env->cpuid_xlevel = def->xlevel;
>      env->cpuid_kvm_features = def->kvm_features;
>      env->cpuid_svm_features = def->svm_features;
> +    env->cpuid_ext4_features = def->ext4_features;
> +    env->cpuid_xlevel2 = def->xlevel2;
>      if (!kvm_enabled()) {
>          env->cpuid_features &= TCG_FEATURES;
>          env->cpuid_ext_features &= TCG_EXT_FEATURES; @@ -1034,7
> +1050,15 @@ void cpu_x86_cpuid(CPUX86State *env, uin
>                     uint32_t *ecx, uint32_t *edx)  {
>      /* test if maximum index reached */
> -    if (index & 0x80000000) {
> +    if ((index & 0xC0000000) == 0xC0000000) {
> +	/* Handle the Centaur's CPUID instruction.*
> +	* If cpuid_xlevel2 is "0", then put into the*
> +	* default case. */
> +	if (env->cpuid_xlevel2 == 0)
> +	    index = 0xF0000000;
> +	else if (index > env->cpuid_xlevel2)
> +	    index = env->cpuid_xlevel2;

Please validate your patch before posting with scripts/checkpatch.pl.

> +    } else if (index & 0x80000000) {
>          if (index > env->cpuid_xlevel)
>              index = env->cpuid_level;
>      } else {
> @@ -1256,6 +1280,28 @@ void cpu_x86_cpuid(CPUX86State *env, uin
>  		*edx = 0;
>  	}
>          break;
> +    case 0xC0000000:
> +	*eax = env->cpuid_xlevel2;
> +	*ebx = 0;
> +	*ecx = 0;
> +	*edx = 0;
> +	break;
> +    case 0xC0000001:
> +	/* Support for VIA CPU's CPUID instruction */
> +	*eax = env->cpuid_version;
> +	*ebx = 0;
> +	*ecx = 0;
> +	*edx = env->cpuid_ext4_features;
> +	break;
> +    case 0xC0000002:
> +    case 0xC0000003:
> +    case 0xC0000004:
> +	/*Reserved for the future, and now filled with zero*/
> +	*eax = 0;
> +	*ebx = 0;
> +	*ecx = 0;
> +	*edx = 0;
> +	break;
>      default:
>          /* reserved values: zero */
>          *eax = 0;
> --- a/target-i386/kvm.c	2011-05-05 09:01:17.182326246 +0800
> +++ b/target-i386/kvm.c	2011-05-05 10:47:48.312331989 +0800
> @@ -496,6 +496,21 @@ int kvm_arch_init_vcpu(CPUState *env)
>          cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
>      }
>  
> +    /* Call Centaur's CPUID instructions they are supported. */
> +    if (env->cpuid_xlevel2 > 0) {
> +	env->cpuid_ext4_features &=
> +		kvm_arch_get_supported_cpuid(env, 0xC0000001, 0, R_EDX);
> +	cpu_x86_cpuid(env, 0xC0000000, 0, &limit, &unused, &unused,
> &unused);
> +
> +	for (i = 0xC0000000; i <= limit; i++) {
> +	    c = &cpuid_data.entries[cpuid_i++];
> +
> +	    c->function = i;
> +	    c->flags = 0;
> +	    cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx,
> &c->edx);
> +	}
> +    }
> +
>      cpuid_data.cpuid.nent = cpuid_i;
>  
>  #ifdef KVM_CAP_MCE

Thanks,
Jan

Hi, Jan
    Thank you very much for your advice. That's helpful for me.

> Hi,
> 
> the subject's tag (qemu-kvm) is misleading. This is actually targeting 
> the uq/master patch queue, i.e. the upstream kvm staging area.
> 

If I want to submit a patch for the qemu-kvm-git,  should I use  "[QEMU-DEVEL][Patch]..." as the subject? Or there are other rules for qemu-kvm upstream? If yes, would you like to tell me?. Thanks!

> On 2011-05-05 05:03, BrillyWu@viatech.com.cn wrote:
> > When KVM is running on VIA CPU with host cpu's model, the
> feautures of
> > VIA CPU will be passed into kvm guest by calling the CPUID
> instruction
> > for Centaur.
> > 
> > Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
> > Signed-off-by: KaryJin<karyjin@viatech.com.cn>
> > ---
> >  target-i386/cpu.h   |    7 +++++++
> >  target-i386/cpuid.c |   48
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> 
> You patch is unfortunately line-wrapped.

Yes, I will be careful the next time.

> > @@ -721,6 +725,9 @@ typedef struct CPUX86State {
> >      uint32_t cpuid_ext3_features;
> >      uint32_t cpuid_apic_id;
> >      int cpuid_vendor_override;
> > +    /*Store the results of Centaur's CPUID instructions*/
> 
> Please format comments like this /* comment text */, ie. with blanks 
> after/before the /* / */.
OK, I will check it.
> 
> > +1050,15 @@ void cpu_x86_cpuid(CPUX86State *env, uin
> >                     uint32_t *ecx, uint32_t *edx)  {
> >      /* test if maximum index reached */
> > -    if (index & 0x80000000) {
> > +    if ((index & 0xC0000000) == 0xC0000000) {
> > +	/* Handle the Centaur's CPUID instruction.*
> > +	* If cpuid_xlevel2 is "0", then put into the*
> > +	* default case. */
> > +	if (env->cpuid_xlevel2 == 0)
> > +	    index = 0xF0000000;
> > +	else if (index > env->cpuid_xlevel2)
> > +	    index = env->cpuid_xlevel2;
> 
> Please validate your patch before posting with scripts/checkpatch.pl.

OK, I will do it. 
I found that space is used to code indent other than tab, should I follow it or use tab instead in my patch?
If I use space, there are some warnings when using scripts/checkpatch.pl to validate the patch. Can I ignore them?

On 2011-05-06 03:06, BrillyWu@viatech.com.cn wrote:
> Hi, Jan
>     Thank you very much for your advice. That's helpful for me.
> 
>> Hi,
>>
>> the subject's tag (qemu-kvm) is misleading. This is actually targeting 
>> the uq/master patch queue, i.e. the upstream kvm staging area.
>>
> 
> If I want to submit a patch for the qemu-kvm-git,  should I use  "[QEMU-DEVEL][Patch]..." as the subject? Or there are other rules for qemu-kvm upstream? If yes, would you like to tell me?. Thanks!

If you really have to target qemu-kvm only, then you tagging is fine.
But this patch does not qualify for such exclusiveness. Rather, your
feature should go into upstream's KVM first and will then be merged back
into qemu-kvm on next update.

> 
>> On 2011-05-05 05:03, BrillyWu@viatech.com.cn wrote:
>>> When KVM is running on VIA CPU with host cpu's model, the
>> feautures of
>>> VIA CPU will be passed into kvm guest by calling the CPUID
>> instruction
>>> for Centaur.
>>>
>>> Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
>>> Signed-off-by: KaryJin<karyjin@viatech.com.cn>
>>> ---
>>>  target-i386/cpu.h   |    7 +++++++
>>>  target-i386/cpuid.c |   48
>>> +++++++++++++++++++++++++++++++++++++++++++++++-
>>
>> You patch is unfortunately line-wrapped.
> 
> Yes, I will be careful the next time.
> 
>>> @@ -721,6 +725,9 @@ typedef struct CPUX86State {
>>>      uint32_t cpuid_ext3_features;
>>>      uint32_t cpuid_apic_id;
>>>      int cpuid_vendor_override;
>>> +    /*Store the results of Centaur's CPUID instructions*/
>>
>> Please format comments like this /* comment text */, ie. with blanks 
>> after/before the /* / */.
> OK, I will check it.
>>
>>> +1050,15 @@ void cpu_x86_cpuid(CPUX86State *env, uin
>>>                     uint32_t *ecx, uint32_t *edx)  {
>>>      /* test if maximum index reached */
>>> -    if (index & 0x80000000) {
>>> +    if ((index & 0xC0000000) == 0xC0000000) {
>>> +	/* Handle the Centaur's CPUID instruction.*
>>> +	* If cpuid_xlevel2 is "0", then put into the*
>>> +	* default case. */
>>> +	if (env->cpuid_xlevel2 == 0)
>>> +	    index = 0xF0000000;
>>> +	else if (index > env->cpuid_xlevel2)
>>> +	    index = env->cpuid_xlevel2;
>>
>> Please validate your patch before posting with scripts/checkpatch.pl.
> 
> OK, I will do it. 
> I found that space is used to code indent other than tab, should I follow it or use tab instead in my patch?
> If I use space, there are some warnings when using scripts/checkpatch.pl to validate the patch. Can I ignore them?

Generally, the advices checkpatch provides are valid and shall be
applied. If you feel like you came across a corner case where the script
reports nonsense, please post your findings to qemu-devel.

Jan

> > 
> > If I want to submit a patch for the qemu-kvm-git,  should I
> use  "[QEMU-DEVEL][Patch]..." as the subject? Or there are other rules 
> for qemu-kvm upstream? If yes, would you like to tell me?. Thanks!
> 
> If you really have to target qemu-kvm only, then you tagging is fine.
> But this patch does not qualify for such exclusiveness. 
> Rather, your feature should go into upstream's KVM first and will then 
> be merged back into qemu-kvm on next update.

I have submit a patch into upstream's KVM for supporting these features before, and the patch has been applied in kvm-git.
Do you mean that I should not submit this patch until the KVM's patch is merged back?
 
> > 
> >> On 2011-05-05 05:03, BrillyWu@viatech.com.cn wrote:
> >>> When KVM is running on VIA CPU with host cpu's model, the
> >> feautures of
> >>> VIA CPU will be passed into kvm guest by calling the CPUID
> >> instruction
> >>> for Centaur.
> >>>
> >>> Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
> >>> Signed-off-by: KaryJin<karyjin@viatech.com.cn>
> >>> ---
> >>>  target-i386/cpu.h   |    7 +++++++
> >>>  target-i386/cpuid.c |   48
> >>> +++++++++++++++++++++++++++++++++++++++++++++++-
> >>
> >> You patch is unfortunately line-wrapped.
> > 
> > Yes, I will be careful the next time.
> > 
> >>> @@ -721,6 +725,9 @@ typedef struct CPUX86State {
> >>>      uint32_t cpuid_ext3_features;
> >>>      uint32_t cpuid_apic_id;
> >>>      int cpuid_vendor_override;
> >>> +    /*Store the results of Centaur's CPUID instructions*/
> >>
> >> Please format comments like this /* comment text */, ie. 
> with blanks
> >> after/before the /* / */.
> > OK, I will check it.
> >>
> >>> +1050,15 @@ void cpu_x86_cpuid(CPUX86State *env, uin
> >>>                     uint32_t *ecx, uint32_t *edx)  {
> >>>      /* test if maximum index reached */
> >>> -    if (index & 0x80000000) {
> >>> +    if ((index & 0xC0000000) == 0xC0000000) {
> >>> +	/* Handle the Centaur's CPUID instruction.*
> >>> +	* If cpuid_xlevel2 is "0", then put into the*
> >>> +	* default case. */
> >>> +	if (env->cpuid_xlevel2 == 0)
> >>> +	    index = 0xF0000000;
> >>> +	else if (index > env->cpuid_xlevel2)
> >>> +	    index = env->cpuid_xlevel2;
> >>
> >> Please validate your patch before posting with
> scripts/checkpatch.pl.
> > 
> > OK, I will do it. 
> > I found that space is used to code indent other than tab,
> should I follow it or use tab instead in my patch?
> > If I use space, there are some warnings when using
> scripts/checkpatch.pl to validate the patch. Can I ignore them?
> 
> Generally, the advices checkpatch provides are valid and shall be 
> applied. If you feel like you came across a corner case where the 
> script reports nonsense, please post your findings to qemu-devel.
> 

I use the tab instead of space as code indent, and the checkpatch reports no warnings.
Thanks!

> Jan
> 
> --
> Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence 
> Center Embedded Linux
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in the 
> body of a message to majordomo@vger.kernel.org More majordomo info at  
> http://vger.kernel.org/majordomo-info.html
>

On 2011-05-06 11:37, BrillyWu@viatech.com.cn wrote:
> 
>>>
>>> If I want to submit a patch for the qemu-kvm-git,  should I
>> use  "[QEMU-DEVEL][Patch]..." as the subject? Or there are other rules 
>> for qemu-kvm upstream? If yes, would you like to tell me?. Thanks!
>>
>> If you really have to target qemu-kvm only, then you tagging is fine.
>> But this patch does not qualify for such exclusiveness. 
>> Rather, your feature should go into upstream's KVM first and will then 
>> be merged back into qemu-kvm on next update.
> 
> I have submit a patch into upstream's KVM for supporting these features before, and the patch has been applied in kvm-git.

As far as I can see, nothing has been applied to any tree, neither
qemu.git nor qemu-kvm.git.

> Do you mean that I should not submit this patch until the KVM's patch is merged back?

You still need to submit a fixed version of this patch against
qemu-kvm.git, uq/master branch (which is qemu.git effectively). Then
Marcelo or Avi can pick it up and push it to upstream. Once it's merged
there, qemu-kvm.git will update from upstream, and you will have your
patch applied to both trees.

Jan

> > I have submit a patch into upstream's KVM for supporting
> these features before, and the patch has been applied in kvm-git.
> 
> As far as I can see, nothing has been applied to any tree, neither 
> qemu.git nor qemu-kvm.git.

Sorry, I have submit to kvm.git, not qemu.git or qemu-kvm.git.

> > Do you mean that I should not submit this patch until the
> KVM's patch is merged back?
> 
> You still need to submit a fixed version of this patch against 
> qemu-kvm.git, uq/master branch (which is qemu.git effectively). Then 
> Marcelo or Avi can pick it up and push it to upstream. Once it's 
> merged there, qemu-kvm.git will update from upstream, and you will 
> have your patch applied to both trees.
>
 
Thanks very much for your nice guide. I know what to do now.