[PATCH] KVM: Add CPUID support for VIA CPU

[PATCH] KVM: Add CPUID support for VIA CPU

[BrillyWu]

The CPUIDs for Centaur are added, and then  the features of PadLock
hardware
engine on VIA CPU, such as "ace", "ace_en" and so on, can be passed into
the 
kvm guest.

Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
Signed-off-by: KaryJin<karyjin@viatech.com.cn>
---
 arch/x86/kvm/x86.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

--- a/arch/x86/kvm/x86.c	2011-04-12 10:16:07.713785938 +0800
+++ b/arch/x86/kvm/x86.c	2011-04-12 10:16:49.903786870 +0800
@@ -2331,6 +2331,12 @@ static void do_cpuid_ent(struct kvm_cpui
 		F(3DNOWPREFETCH) | 0 /* OSVW */ | 0 /* IBS */ | F(XOP) |
 		0 /* SKINIT, WDT, LWP */ | F(FMA4) | F(TBM);
 
+	/* cpuid 0xC0000001.edx */
+	const u32 kvm_supported_word5_x86_features =
+		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
+		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
+		F(PMM) | F(PMM_EN);
+
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 	do_cpuid_1_ent(entry, function, index);
@@ -2440,6 +2446,20 @@ static void do_cpuid_ent(struct kvm_cpui
 		entry->ecx &= kvm_supported_word6_x86_features;
 		cpuid_mask(&entry->ecx, 6);
 		break;
+	/*Add support for Centaur's CPUID instruction*/
+	case 0xC0000000:
+		/*Just support up to 0xC0000004 now*/
+		entry->eax = min(entry->eax, 0xC0000004);
+		break;
+	case 0xC0000001:
+		entry->edx &= kvm_supported_word5_x86_features;
+		cpuid_mask(&entry->edx, 5);
+		break;
+	case 0xC0000002:
+	case 0xC0000003:
+	case 0xC0000004:
+		/*Now nothing to do, reserved for the future*/
+		break;
 	}
 
 	kvm_x86_ops->set_supported_cpuid(function, entry);
@@ -2484,6 +2504,17 @@ static int kvm_dev_ioctl_get_supported_c
 
 	r = -E2BIG;
 	if (nent >= cpuid->nent)
+		goto out_free;
+
+	/* Add support for Centaur's CPUID instruction. */
+	do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0, &nent,
cpuid->nent);
+	limit = cpuid_entries[nent - 1].eax;
+	for (func = 0xC0000001; func <= limit && nent < cpuid->nent;
++func)
+		do_cpuid_ent(&cpuid_entries[nent], func, 0,
+			     &nent, cpuid->nent);
+
+	r = -E2BIG;
+	if (nent >= cpuid->nent)
 		goto out_free;
 
 	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0,
&nent,

Re: [PATCH] KVM: Add CPUID support for VIA CPU

[Avi Kivity]

On 04/13/2011 06:26 AM, BrillyWu@viatech.com.cn wrote:
> The CPUIDs for Centaur are added, and then  the features of PadLock
> hardware
> engine on VIA CPU, such as "ace", "ace_en" and so on, can be passed into
> the
> kvm guest.
>

Nice to see this.  Please post a link to the documentation describing 
these features.

>
> +	/* cpuid 0xC0000001.edx */
> +	const u32 kvm_supported_word5_x86_features =
> +		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
> +		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
> +		F(PMM) | F(PMM_EN);
> +

Are all of these features save wrt save/restore? (do they all act on 
state in standard registers?)  Do they need any control register bits to 
be active or MSRs to configure?

> @@ -2484,6 +2504,17 @@ static int kvm_dev_ioctl_get_supported_c
>
>   	r = -E2BIG;
>   	if (nent>= cpuid->nent)
> +		goto out_free;
> +
> +	/* Add support for Centaur's CPUID instruction. */
> +	do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,&nent,
> cpuid->nent);

nent overflow check missing here.  Also, should probably skip if not a Via.

> +	limit = cpuid_entries[nent - 1].eax;
> +	for (func = 0xC0000001; func<= limit&&  nent<  cpuid->nent;
> ++func)
> +		do_cpuid_ent(&cpuid_entries[nent], func, 0,
> +			&nent, cpuid->nent);
> +
> +	r = -E2BIG;
> +	if (nent>= cpuid->nent)
>   		goto out_free;
>
>   	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0,
> &nent,

-- 
error compiling committee.c: too many arguments to function

RE: [PATCH] KVM: Add CPUID support for VIA CPU

[BrillyWu]

On 04/13/2011 06:26 AM, BrillyWu@viatech.com.cn wrote:
>> The CPUIDs for Centaur are added, and then  the features of PadLock 
>> hardware engine on VIA CPU, such as "ace", "ace_en" and so on, can be 
>> passed into the kvm guest.
>>

> Nice to see this.  Please post a link to the documentation describing 
> these features.

These features are defined in Linux Kernel (arch/x86/include/asm/cpufeature.h),
and the description for these features can be found at the following address:
http://www.via.com.tw/en/initiatives/padlock/hardware.jsp


>>
>> +	/* cpuid 0xC0000001.edx */
>> +	const u32 kvm_supported_word5_x86_features =
>> +		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
>> +		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
>> +		F(PMM) | F(PMM_EN);
>> +

> Are all of these features save wrt save/restore? (do they all act on 
> state in standard registers?)  Do they need any control register bits 
> to be active or MSRs to configure?

These features depend on instructions for the PadLock hardware engine of VIA CPU.
The PadLock instructions just act on standard registers like general X86 instructions, and the features have been enabled when the CPU leave the factory, so there is no need to activate any control register bits or configure MSRs.

>> @@ -2484,6 +2504,17 @@ static int kvm_dev_ioctl_get_supported_c
>>
>>   	r = -E2BIG;
>>   	if (nent>= cpuid->nent)
>> +		goto out_free;
>> +
>> +	/* Add support for Centaur's CPUID instruction. */
>> +	do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,&nent,
>> cpuid->nent);

> nent overflow check missing here.  Also, should probably skip if not a Via.

If not a VIA, the "limit" will be "0", so the following cycle can not run. Moreover, it seems that there is no method to know whther the CPU is a VIA or not in this function.
The nent overflow check is put after the cycle like the "0x8000000" case, and when on a VIA, the returned "limit" is not large (generally it is 0xC0000004), is it neccesary to add a more check here?

> +	limit = cpuid_entries[nent - 1].eax;
> +	for (func = 0xC0000001; func<= limit&&  nent<  cpuid->nent;
> ++func)
> +		do_cpuid_ent(&cpuid_entries[nent], func, 0,
> +			&nent, cpuid->nent);
> +
> +	r = -E2BIG;
> +	if (nent>= cpuid->nent)
>   		goto out_free;
>
>   	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0, &nent,

--
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Add CPUID support for VIA CPU

[Avi Kivity]

On 04/13/2011 02:05 PM, BrillyWu@viatech.com.cn wrote:
> >>
> >>  +	/* cpuid 0xC0000001.edx */
> >>  +	const u32 kvm_supported_word5_x86_features =
> >>  +		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
> >>  +		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
> >>  +		F(PMM) | F(PMM_EN);
> >>  +
>
> >  Are all of these features save wrt save/restore? (do they all act on
> >  state in standard registers?)  Do they need any control register bits
> >  to be active or MSRs to configure?
>
> These features depend on instructions for the PadLock hardware engine of VIA CPU.
> The PadLock instructions just act on standard registers like general X86 instructions, and the features have been enabled when the CPU leave the factory, so there is no need to activate any control register bits or configure MSRs.

I see there is a dependency on EFLAGS[30].  Does a VM entry clear this 
bit?  If not, we have to do it ourselves.

> >>  @@ -2484,6 +2504,17 @@ static int kvm_dev_ioctl_get_supported_c
> >>
> >>    	r = -E2BIG;
> >>    	if (nent>= cpuid->nent)
> >>  +		goto out_free;
> >>  +
> >>  +	/* Add support for Centaur's CPUID instruction. */
> >>  +	do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,&nent,
> >>  cpuid->nent);
>
> >  nent overflow check missing here.  Also, should probably skip if not a Via.
>
> If not a VIA, the "limit" will be "0", so the following cycle can not run.

I think Intel defines CPUID to return the highest standard leaf, so it 
will be equivalent to cpuid(0x1a) or something like that.

> Moreover, it seems that there is no method to know whther the CPU is a VIA or not in this function.

Can't you check the vendor ID?  see boot_cpu_data.

> The nent overflow check is put after the cycle like the "0x8000000" case, and when on a VIA, the returned "limit" is not large (generally it is 0xC0000004), is it neccesary to add a more check here?

Yes, otherwise userspace can supply a buffer that is exactly the wrong 
size and cause an overflow.

-- 
error compiling committee.c: too many arguments to function

RE: [PATCH] KVM: Add CPUID support for VIA CPU

[BrillyWu]

 On 04/13/2011 02:05 PM, BrillyWu@viatech.com.cn wrote:
> > >>
> > >>  +	/* cpuid 0xC0000001.edx */
> > >> +	const u32 kvm_supported_word5_x86_features =
> > >>  +		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
> > >>  +		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
> > >>  +		F(PMM) | F(PMM_EN);
> > >>  +
>
> > >  Are all of these features save wrt save/restore? (do they all act 
> > >on  state in standard registers?)  Do they need any control register 
> > >bits  to be active or MSRs to configure?
>
> > These features depend on instructions for the PadLock hardware engine of VIA CPU.
> > The PadLock instructions just act on standard registers like general X86 instructions, and the features have been enabled when the CPU leave the factory, so there is no need to activate any control register bits or configure MSRs.

> I see there is a dependency on EFLAGS[30].  Does a VM entry clear this bit?  If not, we have to do it ourselves.

Yes, PadLock hardware engine has some association with EFLAGS[30], but it just required that the EFLAGS[30] should be set to "0"
before using PadLock ACE instructions. It is recommanded that execute instruction sequence "pushf;popf" to clear this bit before using
ACE instructions.
AFAIK, the VM entry never sets the EFLAGS[30] bit, so it seems that we do not have to do it ourselves.

> > >>  @@ -2484,6 +2504,17 @@ static int kvm_dev_ioctl_get_supported_c
> > >>
> > >>    	r = -E2BIG;
> > >>    	if (nent>= cpuid->nent)
> > >>  +		goto out_free;
> > >>  +
> > >>  +	/* Add support for Centaur's CPUID instruction. */
> > >> +	do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,&nent,
> > >>  cpuid->nent);
>
> > >  nent overflow check missing here.  Also, should probably skip if not a Via.
>
> > If not a VIA, the "limit" will be "0", so the following cycle can not run.

> I think Intel defines CPUID to return the highest standard leaf, so it will be equivalent to cpuid(0x1a) or something like that.

Yes, you're right.

> > Moreover, it seems that there is no method to know whther the CPU is a VIA or not in this function.

> Can't you check the vendor ID?  see boot_cpu_data.

Good idea, thank you very much.

> > The nent overflow check is put after the cycle like the "0x8000000" case, and when on a VIA, the returned "limit" is not large (generally it is 0xC0000004), is it neccesary to add a more check here?

> Yes, otherwise userspace can supply a buffer that is exactly the wrong size and cause an overflow.

OK, I will add the check.

[PATCH v2] KVM: Add CPUID support for VIA CPU

[BrillyWu]

The CPUIDs for Centaur are added, and then  the features of 
PadLock  hardware engine on VIA CPU, such as "ace", "ace_en" 
and so on, can be passed into the kvm guest.

Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
Signed-off-by: KaryJin<karyjin@viatech.com.cn>
---
 arch/x86/kvm/x86.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

--- a/arch/x86/kvm/x86.c	2011-04-12 10:16:07.713785938 +0800
+++ b/arch/x86/kvm/x86.c	2011-04-14 11:23:34.673820989 +0800
@@ -2331,6 +2331,12 @@ static void do_cpuid_ent(struct kvm_cpui
 		F(3DNOWPREFETCH) | 0 /* OSVW */ | 0 /* IBS */ | F(XOP) |
 		0 /* SKINIT, WDT, LWP */ | F(FMA4) | F(TBM);
 
+	/* cpuid 0xC0000001.edx */
+	const u32 kvm_supported_word5_x86_features =
+		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
+		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
+		F(PMM) | F(PMM_EN);
+
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 	do_cpuid_1_ent(entry, function, index);
@@ -2440,6 +2446,20 @@ static void do_cpuid_ent(struct kvm_cpui
 		entry->ecx &= kvm_supported_word6_x86_features;
 		cpuid_mask(&entry->ecx, 6);
 		break;
+	/*Add support for Centaur's CPUID instruction*/
+	case 0xC0000000:
+		/*Just support up to 0xC0000004 now*/
+		entry->eax = min(entry->eax, 0xC0000004);
+		break;
+	case 0xC0000001:
+		entry->edx &= kvm_supported_word5_x86_features;
+		cpuid_mask(&entry->edx, 5);
+		break;
+	case 0xC0000002:
+	case 0xC0000003:
+	case 0xC0000004:
+		/*Now nothing to do, reserved for the future*/
+		break;
 	}
 
 	kvm_x86_ops->set_supported_cpuid(function, entry);
@@ -2486,6 +2506,26 @@ static int kvm_dev_ioctl_get_supported_c
 	if (nent >= cpuid->nent)
 		goto out_free;
 
+	/* Add support for Centaur's CPUID instruction. */
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR) {
+		do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,
+				&nent, cpuid->nent);
+
+		r = -E2BIG;
+		if (nent >= cpuid->nent)
+			goto out_free;
+
+		limit = cpuid_entries[nent - 1].eax;
+		for (func = 0xC0000001;
+			func <= limit && nent < cpuid->nent; ++func)
+			do_cpuid_ent(&cpuid_entries[nent], func, 0,
+					&nent, cpuid->nent);
+
+		r = -E2BIG;
+		if (nent >= cpuid->nent)
+			goto out_free;
+	}
+
 	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0, &nent,
 		     cpuid->nent);

Re: [PATCH] KVM: Add CPUID support for VIA CPU

[Avi Kivity]

On 04/14/2011 06:14 AM, BrillyWu@viatech.com.cn wrote:
>   On 04/13/2011 02:05 PM, BrillyWu@viatech.com.cn wrote:
> >  >  >>
> >  >  >>   +	/* cpuid 0xC0000001.edx */
> >  >  >>  +	const u32 kvm_supported_word5_x86_features =
> >  >  >>   +		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
> >  >  >>   +		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
> >  >  >>   +		F(PMM) | F(PMM_EN);
> >  >  >>   +
> >
> >  >  >   Are all of these features save wrt save/restore? (do they all act
> >  >  >on  state in standard registers?)  Do they need any control register
> >  >  >bits  to be active or MSRs to configure?
> >
> >  >  These features depend on instructions for the PadLock hardware engine of VIA CPU.
> >  >  The PadLock instructions just act on standard registers like general X86 instructions, and the features have been enabled when the CPU leave the factory, so there is no need to activate any control register bits or configure MSRs.
>
> >  I see there is a dependency on EFLAGS[30].  Does a VM entry clear this bit?  If not, we have to do it ourselves.
>
> Yes, PadLock hardware engine has some association with EFLAGS[30], but it just required that the EFLAGS[30] should be set to "0"
> before using PadLock ACE instructions. It is recommanded that execute instruction sequence "pushf;popf" to clear this bit before using
> ACE instructions.
> AFAIK, the VM entry never sets the EFLAGS[30] bit, so it seems that we do not have to do it ourselves.

I don't think we need to.  kvm kernel code doesn't use padlock; other 
sources which might set EFLAGS[30] are:

- the guest; but VMEXIT clears EFLAGS
- userspace; but syscall/sysenter/int instructions clear EFLAGS[30]
- another kernel thread; there is likely a popf in the context switch 
path somewhere (we should verify this)

-- 
error compiling committee.c: too many arguments to function

RE: [PATCH] KVM: Add CPUID support for VIA CPU

[BrillyWu]

> On 04/14/2011 06:14 AM, BrillyWu@viatech.com.cn wrote:
> >   On 04/13/2011 02:05 PM, BrillyWu@viatech.com.cn wrote:
> > >  >  >>
> > >  >  >>   +	/* cpuid 0xC0000001.edx */
> > >  >  >>  +	const u32 kvm_supported_word5_x86_features =
> > >  >  >>   +		F(XSTORE) | F(XSTORE_EN) | 
> F(XCRYPT) | F(XCRYPT_EN) |
> > >  >  >>   +		F(ACE2) | F(ACE2_EN) | F(PHE) | 
> F(PHE_EN) |
> > >  >  >>   +		F(PMM) | F(PMM_EN);
> > >  >  >>   +
> > >
> > >  >  >   Are all of these features save wrt save/restore? 
> (do they all act
> > >  >  >on  state in standard registers?)  Do they need any control 
> > > register  >  >bits  to be active or MSRs to configure?
> > >
> > >  >  These features depend on instructions for the PadLock
> hardware engine of VIA CPU.
> > >  >  The PadLock instructions just act on standard
> registers like general X86 instructions, and the features have been 
> enabled when the CPU leave the factory, so there is no need to 
> activate any control register bits or configure MSRs.
> >
> > >  I see there is a dependency on EFLAGS[30].  Does a VM
> entry clear this bit?  If not, we have to do it ourselves.
> >
> > Yes, PadLock hardware engine has some association with
> EFLAGS[30], but it just required that the EFLAGS[30] should be set to 
> "0"
> > before using PadLock ACE instructions. It is recommanded
> that execute
> > instruction sequence "pushf;popf" to clear this bit before
> using ACE instructions.
> > AFAIK, the VM entry never sets the EFLAGS[30] bit, so it
> seems that we do not have to do it ourselves.
> 
> I don't think we need to.  kvm kernel code doesn't use padlock; other 
> sources which might set EFLAGS[30] are:
> 
> - the guest; but VMEXIT clears EFLAGS
> - userspace; but syscall/sysenter/int instructions clear EFLAGS[30]
> - another kernel thread; there is likely a popf in the context switch 
> path somewhere (we should verify this)

Thank you very much for telling me where the  EFLAGS[30] might 
be set and cleared.
In fact, adding the CPUID support into kvm 
kernel code is just to provide correct result for the calling of  
the "kvm_arch_get_supported_cpuid" function in Qemu-kvm 
application. 

There is no dependency on EFLAGS when calling VIA CPUID 
instruction. 

Before using padlock engine functions, the application first need detect 
is the features exist through cpuid instruction, then use ACE and other 
instructions such as PHE/RNG/PMM.

And as previously said, only the ACE instructions required 
that the EFLAGS[30] should be set to "0",  It is recommanded 
that execute instruction sequence "pushf;popf" to clear this bit 
before  using ACE instructions.

I have re-submitted this patch, please check it. Thanks!

Re: [PATCH] KVM: Add CPUID support for VIA CPU

[Avi Kivity]

On 04/14/2011 12:54 PM, BrillyWu@viatech.com.cn wrote:
> >  On 04/14/2011 06:14 AM, BrillyWu@viatech.com.cn wrote:
> >  >    On 04/13/2011 02:05 PM, BrillyWu@viatech.com.cn wrote:
> >  >  >   >   >>
> >  >  >   >   >>    +	/* cpuid 0xC0000001.edx */
> >  >  >   >   >>   +	const u32 kvm_supported_word5_x86_features =
> >  >  >   >   >>    +		F(XSTORE) | F(XSTORE_EN) |
> >  F(XCRYPT) | F(XCRYPT_EN) |
> >  >  >   >   >>    +		F(ACE2) | F(ACE2_EN) | F(PHE) |
> >  F(PHE_EN) |
> >  >  >   >   >>    +		F(PMM) | F(PMM_EN);
> >  >  >   >   >>    +
> >  >  >
> >  >  >   >   >    Are all of these features save wrt save/restore?
> >  (do they all act
> >  >  >   >   >on  state in standard registers?)  Do they need any control
> >  >  >  register>   >bits  to be active or MSRs to configure?
> >  >  >
> >  >  >   >   These features depend on instructions for the PadLock
> >  hardware engine of VIA CPU.
> >  >  >   >   The PadLock instructions just act on standard
> >  registers like general X86 instructions, and the features have been
> >  enabled when the CPU leave the factory, so there is no need to
> >  activate any control register bits or configure MSRs.
> >  >
> >  >  >   I see there is a dependency on EFLAGS[30].  Does a VM
> >  entry clear this bit?  If not, we have to do it ourselves.
> >  >
> >  >  Yes, PadLock hardware engine has some association with
> >  EFLAGS[30], but it just required that the EFLAGS[30] should be set to
> >  "0"
> >  >  before using PadLock ACE instructions. It is recommanded
> >  that execute
> >  >  instruction sequence "pushf;popf" to clear this bit before
> >  using ACE instructions.
> >  >  AFAIK, the VM entry never sets the EFLAGS[30] bit, so it
> >  seems that we do not have to do it ourselves.
> >
> >  I don't think we need to.  kvm kernel code doesn't use padlock; other
> >  sources which might set EFLAGS[30] are:
> >
> >  - the guest; but VMEXIT clears EFLAGS
> >  - userspace; but syscall/sysenter/int instructions clear EFLAGS[30]
> >  - another kernel thread; there is likely a popf in the context switch
> >  path somewhere (we should verify this)
>
> Thank you very much for telling me where the  EFLAGS[30] might
> be set and cleared.
> In fact, adding the CPUID support into kvm
> kernel code is just to provide correct result for the calling of
> the "kvm_arch_get_supported_cpuid" function in Qemu-kvm
> application.

That may not be sufficient for correct operation.

Consider:

- guest executes a padlock instruction
- cpu sets EFLAGS[30]
- external interrupt
- VMEXIT (saves EFLAGS in GUEST_RFLAGS with EFLAGS[30] set)
- external interrupt is processed, causes a task switch
- EFLAGS[30] is cleared
- some other process uses padlock instructions, which causes a reload of 
key information
- switch back to kvm
- VM entry (loads EFLAGS from GUEST_RFLAGS; still has EFLAGS[30] set)
- guest executes following padlock instruction, doesn't reload key 
information

so I think the code as is causes data corruption.

> There is no dependency on EFLAGS when calling VIA CPUID
> instruction.
>
> Before using padlock engine functions, the application first need detect
> is the features exist through cpuid instruction, then use ACE and other
> instructions such as PHE/RNG/PMM.
>
> And as previously said, only the ACE instructions required
> that the EFLAGS[30] should be set to "0",  It is recommanded
> that execute instruction sequence "pushf;popf" to clear this bit
> before  using ACE instructions.

The problem is that VM entry reloads eflags from saved state and is not 
affected by popf.

> I have re-submitted this patch, please check it. Thanks!

wrt cpuid it seems reasonable but that's we need to clear the EFLAGS[30] 
issue first.

-- 
error compiling committee.c: too many arguments to function

RE: [PATCH] KVM: Add CPUID support for VIA CPU

[BrillyWu]

Hi, Avi

I'm sorry to reply you a little late.

I spent some time on finding information to fix the EFLAGS[30] issue, and I have got it now.

Here is some comment from Centaur.: 

EFLAGS:30 was used on C7 but not by Nano. EFLAGS:30 is undefined for Intel. C7 used it as a way of improving performance by remembering when keyram was valid but it required users to clear the bit (say by PUSHF/POPF) when changing keys for an encryption.
 
There was a whole lot of ucode to make sure that EFLAGS:30 would always be zero on a task switch, to avoid leaking key material (or simply making an error in calculation).
 
In a rational fit of paranoia, we decided that it was safer to always reset the internal keyram at the start of every XCRYPT instruction, which made use of EFLAGS:30 irrelevant.
 
The current docs make no reference to EFLAGS, and they specifically indicate that they supercede all previous documentation.

So,the conclusion is:

Only VIA C7 CPUs have dependency on EFLAGS[30], but they do not support VT-X technology, so kvm can not run on it and the issue about EFLAGS[30] will not occur. 
VIA Nano CPUs support VT-X technology and can run kvm, but they have no dependency on EFLAGS[30] (EFLAGS[30] is kept cleared), so  the issue about EFLAGS[30] will not appear, too.

I'll send you the current document about nano cpu.


Brilly
2011-04-21


> 
> On 04/14/2011 12:54 PM, BrillyWu@viatech.com.cn wrote:
> > Thank you very much for telling me where the  EFLAGS[30]
> might be set
> > and cleared.
> > In fact, adding the CPUID support into kvm kernel code is just to 
> > provide correct result for the calling of the 
> > "kvm_arch_get_supported_cpuid" function in Qemu-kvm application.
> 
> That may not be sufficient for correct operation.
> 
> Consider:
> 
> - guest executes a padlock instruction
> - cpu sets EFLAGS[30]
> - external interrupt
> - VMEXIT (saves EFLAGS in GUEST_RFLAGS with EFLAGS[30] set)
> - external interrupt is processed, causes a task switch
> - EFLAGS[30] is cleared
> - some other process uses padlock instructions, which causes a reload 
> of key information
> - switch back to kvm
> - VM entry (loads EFLAGS from GUEST_RFLAGS; still has EFLAGS[30] set)
> - guest executes following padlock instruction, doesn't reload key 
> information
> 
> so I think the code as is causes data corruption.

Yes, I agree. But  VIA Nano is no longer associated with EFLAGS[30], so the data corruption will not appear.

> 
> > There is no dependency on EFLAGS when calling VIA CPUID instruction.
> >
> > Before using padlock engine functions, the application first need 
> > detect is the features exist through cpuid instruction,
> then use ACE
> > and other instructions such as PHE/RNG/PMM.
> >
> > And as previously said, only the ACE instructions required that the 
> > EFLAGS[30] should be set to "0",  It is recommanded that execute 
> > instruction sequence "pushf;popf" to clear this bit before
> using ACE
> > instructions.
> 
> The problem is that VM entry reloads eflags from saved state and is 
> not affected by popf.
> 
> > I have re-submitted this patch, please check it. Thanks!
> 
> wrt cpuid it seems reasonable but that's we need to clear the 
> EFLAGS[30] issue first.
> 

I think the  EFLAGS[30] issue is fixed now.

> --
> error compiling committee.c: too many arguments to function
> 
> --
> 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
> 

Re: [PATCH] KVM: Add CPUID support for VIA CPU

[Avi Kivity]

On 04/21/2011 01:06 PM, BrillyWu@viatech.com.cn wrote:
> So,the conclusion is:
>
> Only VIA C7 CPUs have dependency on EFLAGS[30], but they do not support VT-X technology, so kvm can not run on it and the issue about EFLAGS[30] will not occur.
> VIA Nano CPUs support VT-X technology and can run kvm, but they have no dependency on EFLAGS[30] (EFLAGS[30] is kept cleared), so  the issue about EFLAGS[30] will not appear, too.
>

Thanks for the clarification.  The patch however does not apply.  Please 
regenerate it against kvm.git master branch, see 
http://www.linux-kvm.org/page/Code.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

RE: [PATCH] KVM: Add CPUID support for VIA CPU

[BrillyWu]

> On 04/21/2011 01:06 PM, BrillyWu@viatech.com.cn wrote:
> > So,the conclusion is:
> >
> > Only VIA C7 CPUs have dependency on EFLAGS[30], but they do
> not support VT-X technology, so kvm can not run on it and the issue 
> about EFLAGS[30] will not occur.
> > VIA Nano CPUs support VT-X technology and can run kvm, but
> they have no dependency on EFLAGS[30] (EFLAGS[30] is kept cleared), so  
> the issue about EFLAGS[30] will not appear, too.
> >
> 
> Thanks for the clarification.  The patch however does not apply.  
> Please regenerate it against kvm.git master branch, see 
> http://www.linux-kvm.org/page/Code.
> 

OK. I have regenerate it as follows. Please check it, thank you!

Signed-off-by: BrillyWu<brillywu@viatech.com.cn>
Signed-off-by: KaryJin<karyjin@viatech.com.cn>
---
 arch/x86/kvm/x86.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

--- a/arch/x86/kvm/x86.c	2011-04-25 09:28:32.912132000 +0800
+++ b/arch/x86/kvm/x86.c	2011-04-25 09:28:28.232132000 +0800
@@ -2336,6 +2336,12 @@ static void do_cpuid_ent(struct kvm_cpui
 		F(3DNOWPREFETCH) | 0 /* OSVW */ | 0 /* IBS */ | F(XOP) |
 		0 /* SKINIT, WDT, LWP */ | F(FMA4) | F(TBM);
 
+	/* cpuid 0xC0000001.edx */
+	const u32 kvm_supported_word5_x86_features =
+		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
+		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
+		F(PMM) | F(PMM_EN);
+
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 	do_cpuid_1_ent(entry, function, index); @@ -2445,6 +2451,20 @@ static void do_cpuid_ent(struct kvm_cpui
 		entry->ecx &= kvm_supported_word6_x86_features;
 		cpuid_mask(&entry->ecx, 6);
 		break;
+	/*Add support for Centaur's CPUID instruction*/
+	case 0xC0000000:
+		/*Just support up to 0xC0000004 now*/
+		entry->eax = min(entry->eax, 0xC0000004);
+		break;
+	case 0xC0000001:
+		entry->edx &= kvm_supported_word5_x86_features;
+		cpuid_mask(&entry->edx, 5);
+		break;
+	case 0xC0000002:
+	case 0xC0000003:
+	case 0xC0000004:
+		/*Now nothing to do, reserved for the future*/
+		break;
 	}
 
 	kvm_x86_ops->set_supported_cpuid(function, entry); @@ -2491,6 +2511,26 @@ static int kvm_dev_ioctl_get_supported_c
 	if (nent >= cpuid->nent)
 		goto out_free;
 
+	/* Add support for Centaur's CPUID instruction. */
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR) {
+		do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,
+				&nent, cpuid->nent);
+
+		r = -E2BIG;
+		if (nent >= cpuid->nent)
+			goto out_free;
+
+		limit = cpuid_entries[nent - 1].eax;
+		for (func = 0xC0000001;
+			func <= limit && nent < cpuid->nent; ++func)
+			do_cpuid_ent(&cpuid_entries[nent], func, 0,
+					&nent, cpuid->nent);
+
+		r = -E2BIG;
+		if (nent >= cpuid->nent)
+			goto out_free;
+	}
+
 	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0, &nent,
 		     cpuid->nent);

Re: [PATCH] KVM: Add CPUID support for VIA CPU

[Avi Kivity]

On 04/25/2011 08:55 AM, BrillyWu@viatech.com.cn wrote:
> >  On 04/21/2011 01:06 PM, BrillyWu@viatech.com.cn wrote:
> >  >  So,the conclusion is:
> >  >
> >  >  Only VIA C7 CPUs have dependency on EFLAGS[30], but they do
> >  not support VT-X technology, so kvm can not run on it and the issue
> >  about EFLAGS[30] will not occur.
> >  >  VIA Nano CPUs support VT-X technology and can run kvm, but
> >  they have no dependency on EFLAGS[30] (EFLAGS[30] is kept cleared), so
> >  the issue about EFLAGS[30] will not appear, too.
> >  >
> >
> >  Thanks for the clarification.  The patch however does not apply.
> >  Please regenerate it against kvm.git master branch, see
> >  http://www.linux-kvm.org/page/Code.
> >
>
> OK. I have regenerate it as follows. Please check it, thank you!
>

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function

[PATCH] qemu-kvm: Add CPUID support for VIA CPU

[BrillyWu]

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   |    3 +++
 target-i386/cpuid.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
 target-i386/kvm.c   |   15 +++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

--- a/target-i386/cpu.h	2011-04-25 14:31:51.042132014 +0800
+++ b/target-i386/cpu.h	2011-04-25 14:32:10.632132001 +0800
@@ -721,6 +721,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*/
+    uint32_t cpuid_xlevel2;
+    uint32_t cpuid_ext4_features;
 
     /* MTRRs */
     uint64_t mtrr_fixed[11];
--- a/target-i386/cpuid.c	2011-04-25 14:31:54.812132057 +0800
+++ b/target-i386/cpuid.c	2011-04-25 14:32:14.822132001 +0800
@@ -228,6 +228,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) @@ -520,6 +523,13 @@ 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. */
+    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 @@ -853,6 +863,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; @@ -1032,7 +1044,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;
+    } else if (index & 0x80000000) {
         if (index > env->cpuid_xlevel)
             index = env->cpuid_level;
     } else {
@@ -1254,6 +1274,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-04-25 14:31:59.752132003 +0800
+++ b/target-i386/kvm.c	2011-04-25 14:32:19.822132002 +0800
@@ -476,6 +476,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