[PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Jan Kiszka]

We must not report the TSC deadline timer feature on our own when user
space provides the APIC as we have no clue about its features.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - reorganized code to make the flow clearer

 arch/x86/kvm/cpuid.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 230f713..b7cdd3b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -27,7 +27,6 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
 	struct kvm_lapic *apic = vcpu->arch.apic;
-	u32 timer_mode_mask;
 
 	best = kvm_find_cpuid_entry(vcpu, 1, 0);
 	if (!best)
@@ -40,15 +39,14 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			best->ecx |= bit(X86_FEATURE_OSXSAVE);
 	}
 
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
-		best->function == 0x1) {
-		best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
-		timer_mode_mask = 3 << 17;
-	} else
-		timer_mode_mask = 1 << 17;
-
-	if (apic)
-		apic->lapic_timer.timer_mode_mask = timer_mode_mask;
+	if (apic) {
+		if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+		    best->function == 0x1) {
+			best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
+			apic->lapic_timer.timer_mode_mask = 3 << 17;
+		} else
+			apic->lapic_timer.timer_mode_mask = 1 << 17;
+	}
 
 	kvm_pmu_cpuid_update(vcpu);
 }
-- 
1.7.3.4

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Avi Kivity]

On 12/21/2011 12:25 PM, Jan Kiszka wrote:
> We must not report the TSC deadline timer feature on our own when user
> space provides the APIC as we have no clue about its features.

We must not report the TSC deadline timer feature on our own, period. 
We should just update the timer mode mask there.  Don't know how this
slipped through review.

I think your original idea was correct.  Add a new KVM_CAP for the tsc
deadline timer.  Userspace can add the bit to cpuid if either it
implements the feature in a userspace apic, or if it finds the new
capability and uses the kernel apic.

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

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Jan Kiszka]

On 2011-12-21 11:35, Avi Kivity wrote:
> On 12/21/2011 12:25 PM, Jan Kiszka wrote:
>> We must not report the TSC deadline timer feature on our own when user
>> space provides the APIC as we have no clue about its features.
> 
> We must not report the TSC deadline timer feature on our own, period. 
> We should just update the timer mode mask there.  Don't know how this
> slipped through review.
>
> I think your original idea was correct.  Add a new KVM_CAP for the tsc
> deadline timer.  Userspace can add the bit to cpuid if either it
> implements the feature in a userspace apic, or if it finds the new
> capability and uses the kernel apic.

Right, we do need some control for user space to keep the feature
disabled when migrating from an older host.

However, there is also the timer_mode_mask which requires tuning in
addition to the cpuid flag.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Alexey Zaytsev]

On Wed, Dec 21, 2011 at 12:25, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> We must not report the TSC deadline timer feature on our own when user
> space provides the APIC as we have no clue about its features.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> Changes in v2:
>  - reorganized code to make the flow clearer

I've seen Avi's comment below, but just for the note, it works fine
when applied to arch/x86/kvm/x86.c.

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Avi Kivity]

On 12/21/2011 12:41 PM, Jan Kiszka wrote:
> On 2011-12-21 11:35, Avi Kivity wrote:
> > On 12/21/2011 12:25 PM, Jan Kiszka wrote:
> >> We must not report the TSC deadline timer feature on our own when user
> >> space provides the APIC as we have no clue about its features.
> > 
> > We must not report the TSC deadline timer feature on our own, period. 
> > We should just update the timer mode mask there.  Don't know how this
> > slipped through review.
> >
> > I think your original idea was correct.  Add a new KVM_CAP for the tsc
> > deadline timer.  Userspace can add the bit to cpuid if either it
> > implements the feature in a userspace apic, or if it finds the new
> > capability and uses the kernel apic.
>
> Right, we do need some control for user space to keep the feature
> disabled when migrating from an older host.
>
> However, there is also the timer_mode_mask which requires tuning in
> addition to the cpuid flag.
>

timer_mode_mask can just be slaved to the bit (as received by
KVM_SET_CPUID); that's exactly kvm_update_cpuid()'s role.

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

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Jan Kiszka]

On 2011-12-21 11:44, Avi Kivity wrote:
> On 12/21/2011 12:41 PM, Jan Kiszka wrote:
>> On 2011-12-21 11:35, Avi Kivity wrote:
>>> On 12/21/2011 12:25 PM, Jan Kiszka wrote:
>>>> We must not report the TSC deadline timer feature on our own when user
>>>> space provides the APIC as we have no clue about its features.
>>>
>>> We must not report the TSC deadline timer feature on our own, period. 
>>> We should just update the timer mode mask there.  Don't know how this
>>> slipped through review.
>>>
>>> I think your original idea was correct.  Add a new KVM_CAP for the tsc
>>> deadline timer.  Userspace can add the bit to cpuid if either it
>>> implements the feature in a userspace apic, or if it finds the new
>>> capability and uses the kernel apic.
>>
>> Right, we do need some control for user space to keep the feature
>> disabled when migrating from an older host.
>>
>> However, there is also the timer_mode_mask which requires tuning in
>> addition to the cpuid flag.
>>
> 
> timer_mode_mask can just be slaved to the bit (as received by
> KVM_SET_CPUID); that's exactly kvm_update_cpuid()'s role.

Like

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 230f713..89b02bf 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -27,7 +27,6 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
 	struct kvm_lapic *apic = vcpu->arch.apic;
-	u32 timer_mode_mask;

 	best = kvm_find_cpuid_entry(vcpu, 1, 0);
 	if (!best)
@@ -40,15 +39,12 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			best->ecx |= bit(X86_FEATURE_OSXSAVE);
 	}

-	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
-		best->function == 0x1) {
-		best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
-		timer_mode_mask = 3 << 17;
-	} else
-		timer_mode_mask = 1 << 17;
-
-	if (apic)
-		apic->lapic_timer.timer_mode_mask = timer_mode_mask;
+	if (apic) {
+		if (best->ecx & bit(X86_FEATURE_TSC_DEADLINE_TIMER))
+			apic->lapic_timer.timer_mode_mask = 3 << 17;
+		else
+			apic->lapic_timer.timer_mode_mask = 1 << 17;
+	}

 	kvm_pmu_cpuid_update(vcpu);
 }


+ the KVM_CAP thing?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Avi Kivity]

On 12/21/2011 01:28 PM, Jan Kiszka wrote:
> > 
> > timer_mode_mask can just be slaved to the bit (as received by
> > KVM_SET_CPUID); that's exactly kvm_update_cpuid()'s role.
>
> Like

Ugh, the facebook generation invades lkml.

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 230f713..89b02bf 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -27,7 +27,6 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best;
>  	struct kvm_lapic *apic = vcpu->arch.apic;
> -	u32 timer_mode_mask;
>
>  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>  	if (!best)
> @@ -40,15 +39,12 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>  			best->ecx |= bit(X86_FEATURE_OSXSAVE);
>  	}
>
> -	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> -		best->function == 0x1) {
> -		best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
> -		timer_mode_mask = 3 << 17;
> -	} else
> -		timer_mode_mask = 1 << 17;
> -
> -	if (apic)
> -		apic->lapic_timer.timer_mode_mask = timer_mode_mask;
> +	if (apic) {
> +		if (best->ecx & bit(X86_FEATURE_TSC_DEADLINE_TIMER))
> +			apic->lapic_timer.timer_mode_mask = 3 << 17;
> +		else
> +			apic->lapic_timer.timer_mode_mask = 1 << 17;
> +	}
>
>  	kvm_pmu_cpuid_update(vcpu);
>  }
>
>
> + the KVM_CAP thing?
>

+1

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

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Jan Kiszka]

On 2011-12-21 12:45, Avi Kivity wrote:
> On 12/21/2011 01:28 PM, Jan Kiszka wrote:
>>>
>>> timer_mode_mask can just be slaved to the bit (as received by
>>> KVM_SET_CPUID); that's exactly kvm_update_cpuid()'s role.
>>
>> Like
> 
> Ugh, the facebook generation invades lkml.

I'm not on facebook. Does anyone sell shirts with that?

> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 230f713..89b02bf 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -27,7 +27,6 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>  {
>>  	struct kvm_cpuid_entry2 *best;
>>  	struct kvm_lapic *apic = vcpu->arch.apic;
>> -	u32 timer_mode_mask;
>>
>>  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>>  	if (!best)
>> @@ -40,15 +39,12 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>  			best->ecx |= bit(X86_FEATURE_OSXSAVE);
>>  	}
>>
>> -	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
>> -		best->function == 0x1) {
>> -		best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
>> -		timer_mode_mask = 3 << 17;
>> -	} else
>> -		timer_mode_mask = 1 << 17;
>> -
>> -	if (apic)
>> -		apic->lapic_timer.timer_mode_mask = timer_mode_mask;
>> +	if (apic) {
>> +		if (best->ecx & bit(X86_FEATURE_TSC_DEADLINE_TIMER))
>> +			apic->lapic_timer.timer_mode_mask = 3 << 17;
>> +		else
>> +			apic->lapic_timer.timer_mode_mask = 1 << 17;
>> +	}
>>
>>  	kvm_pmu_cpuid_update(vcpu);
>>  }
>>
>>
>> + the KVM_CAP thing?
>>
> 
> +1

Ah, the adult crowd that trusts the other side.

OK, will see that I can cook up a proper patch later.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Avi Kivity]

On 12/21/2011 01:58 PM, Jan Kiszka wrote:
> > +1
>
> Ah, the adult crowd that trusts the other side.

It's 80% gnome-shell rants and similar, don't bother.

> OK, will see that I can cook up a proper patch later.
>

Thanks.

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

RE: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Liu, Jinsong]

Avi Kivity wrote:
> On 12/21/2011 12:25 PM, Jan Kiszka wrote:
>> We must not report the TSC deadline timer feature on our own when
>> user space provides the APIC as we have no clue about its features.
> 
> We must not report the TSC deadline timer feature on our own, period.
> We should just update the timer mode mask there.  Don't know how this
> slipped through review.
> 
> I think your original idea was correct.  Add a new KVM_CAP for the tsc
> deadline timer.  Userspace can add the bit to cpuid if either it
> implements the feature in a userspace apic, or if it finds the new
> capability and uses the kernel apic.

Is it necessary to use KVM_CAP? If I didn't misunderstand, the KVM_CAP sulotion would be:
1. qemu get kvm tsc deadline timer capability by KVM_CAP_...;
2. qemu add cpuid bit
        if ((guest use qemu apic && qemu emualte tsc deadline timer) || 
           (guest use kvm apic && kvm emulate tsc deadline timer (KVM_CAP)))
3. qemu ioctl KVM_SET_CPUID2
4. kvm expose the feature to guest by saving it at vcpu->arch.cpuid_entries, 
seems it's logically redundant.

Jan's patch v2 is a straight forward and simple fix. in the patch
    if (apic) { ... }
means apic (and then its sub-logic tsc deadline timer) emulated by kvm, that's enough:
if quest use kvm apic, it's OK to add cpuid bit and expose to guest;
if guest don't use kvm apic, it will not touch cpuid bit;

Thanks,
Jinsong

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Avi Kivity]

On 12/22/2011 05:41 PM, Liu, Jinsong wrote:
> Avi Kivity wrote:
> > On 12/21/2011 12:25 PM, Jan Kiszka wrote:
> >> We must not report the TSC deadline timer feature on our own when
> >> user space provides the APIC as we have no clue about its features.
> > 
> > We must not report the TSC deadline timer feature on our own, period.
> > We should just update the timer mode mask there.  Don't know how this
> > slipped through review.
> > 
> > I think your original idea was correct.  Add a new KVM_CAP for the tsc
> > deadline timer.  Userspace can add the bit to cpuid if either it
> > implements the feature in a userspace apic, or if it finds the new
> > capability and uses the kernel apic.
>
> Is it necessary to use KVM_CAP? If I didn't misunderstand, the KVM_CAP sulotion would be:
> 1. qemu get kvm tsc deadline timer capability by KVM_CAP_...;
> 2. qemu add cpuid bit
>         if ((guest use qemu apic && qemu emualte tsc deadline timer) || 
>            (guest use kvm apic && kvm emulate tsc deadline timer (KVM_CAP)))
> 3. qemu ioctl KVM_SET_CPUID2
> 4. kvm expose the feature to guest by saving it at vcpu->arch.cpuid_entries, 

Correct.

> seems it's logically redundant.

What's logically redundant?

> Jan's patch v2 is a straight forward and simple fix. in the patch
>     if (apic) { ... }
> means apic (and then its sub-logic tsc deadline timer) emulated by kvm, that's enough:
> if quest use kvm apic, it's OK to add cpuid bit and expose to guest;
> if guest don't use kvm apic, it will not touch cpuid bit;
>

It breaks live migration: if you start a guest on a TSC-deadline capable
host kernel, and migrate it to a TSC-deadline incapable host kernel, you
end up with a broken guest.

More broadly, kvm never exposes features transparently to the guest, it
always passes them to userspace first, so userspace controls the ABI
exposed to the guest.  This prevents the following scenario:

- a guest is started on some hardware, which doesn't support some cpuid
feature (say AVX for example)
- the guest or one of its applications are broken wrt AVX, but because
the feature is not exposed, it works correctly
- the host hardware is upgraded to one which supports AVX
- the guest is now broken

(the downside is that guests run slower than they would with automatic
feature exposure, but that's better than breaking them)

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

RE: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Liu, Jinsong]

Avi Kivity wrote:
> On 12/22/2011 05:41 PM, Liu, Jinsong wrote:
>> Avi Kivity wrote:
>>> On 12/21/2011 12:25 PM, Jan Kiszka wrote:
>>>> We must not report the TSC deadline timer feature on our own when
>>>> user space provides the APIC as we have no clue about its features.
>>> 
>>> We must not report the TSC deadline timer feature on our own,
>>> period. We should just update the timer mode mask there.  Don't
>>> know how this slipped through review. 
>>> 
>>> I think your original idea was correct.  Add a new KVM_CAP for the
>>> tsc deadline timer.  Userspace can add the bit to cpuid if either it
>>> implements the feature in a userspace apic, or if it finds the new
>>> capability and uses the kernel apic.
>> 
>> Is it necessary to use KVM_CAP? If I didn't misunderstand, the
>> KVM_CAP sulotion would be: 
>> 1. qemu get kvm tsc deadline timer capability by KVM_CAP_...;
>> 2. qemu add cpuid bit
>>         if ((guest use qemu apic && qemu emualte tsc deadline timer)
>>            || (guest use kvm apic && kvm emulate tsc deadline timer
>> (KVM_CAP))) 
>> 3. qemu ioctl KVM_SET_CPUID2
>> 4. kvm expose the feature to guest by saving it at
>> vcpu->arch.cpuid_entries, 
> 
> Correct.
> 
>> seems it's logically redundant.
> 
> What's logically redundant?
> 
>> Jan's patch v2 is a straight forward and simple fix. in the patch   
>> if (apic) { ... } means apic (and then its sub-logic tsc deadline
>> timer) emulated by kvm, that's enough: if quest use kvm apic, it's
>> OK to add cpuid bit and expose to guest; 
>> if guest don't use kvm apic, it will not touch cpuid bit;
>> 
> 
> It breaks live migration: if you start a guest on a TSC-deadline
> capable host kernel, and migrate it to a TSC-deadline incapable host
> kernel, you end up with a broken guest.
> 
> More broadly, kvm never exposes features transparently to the guest,
> it always passes them to userspace first, so userspace controls the
> ABI exposed to the guest.  This prevents the following scenario:

Do you mean, by the method qemu control cpuid exposing, it can avoid live migration broken issue by
1. user probe the lowest ability host of whole pool where vm may live migrate;
2. only if the lowest ablility host support the feature can user enable the feature when boot a vm;
3. if the lowest ability host didn't support the feature (say tsc deadline timer as example), user disable the feature when boot a vm;
In this way, live migration wouldn't be broken. Right?

or, do you mean qemu-kvm solve live migration broken issue by some other method?

> 
> - a guest is started on some hardware, which doesn't support some
> cpuid feature (say AVX for example)
> - the guest or one of its applications are broken wrt AVX, but because
> the feature is not exposed, it works correctly
> - the host hardware is upgraded to one which supports AVX
> - the guest is now broken

You mean, live migrate from 'old' (which doesn't support the feature) platform to 'new' platform would broken?

Thanks,
Jinsong

Re: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Avi Kivity]

On 12/26/2011 10:11 AM, Liu, Jinsong wrote:
> > 
> > It breaks live migration: if you start a guest on a TSC-deadline
> > capable host kernel, and migrate it to a TSC-deadline incapable host
> > kernel, you end up with a broken guest.
> > 
> > More broadly, kvm never exposes features transparently to the guest,
> > it always passes them to userspace first, so userspace controls the
> > ABI exposed to the guest.  This prevents the following scenario:
>
> Do you mean, by the method qemu control cpuid exposing, it can avoid live migration broken issue by
> 1. user probe the lowest ability host of whole pool where vm may live migrate;
> 2. only if the lowest ablility host support the feature can user enable the feature when boot a vm;
> 3. if the lowest ability host didn't support the feature (say tsc deadline timer as example), user disable the feature when boot a vm;
> In this way, live migration wouldn't be broken. Right?

Right.

> or, do you mean qemu-kvm solve live migration broken issue by some other method?

The method you outlined, or any other method, such as partitioning the
cluster according to hardware capabilities.

>
> > 
> > - a guest is started on some hardware, which doesn't support some
> > cpuid feature (say AVX for example)
> > - the guest or one of its applications are broken wrt AVX, but because
> > the feature is not exposed, it works correctly
> > - the host hardware is upgraded to one which supports AVX
> > - the guest is now broken
>
> You mean, live migrate from 'old' (which doesn't support the feature) platform to 'new' platform would broken?

Live migration, or even just a guest restart on updated hardware.

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

RE: [PATCH v2] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC

[Liu, Jinsong]

Avi Kivity wrote:
> On 12/26/2011 10:11 AM, Liu, Jinsong wrote:
>>> 
>>> It breaks live migration: if you start a guest on a TSC-deadline
>>> capable host kernel, and migrate it to a TSC-deadline incapable host
>>> kernel, you end up with a broken guest.
>>> 
>>> More broadly, kvm never exposes features transparently to the guest,
>>> it always passes them to userspace first, so userspace controls the
>>> ABI exposed to the guest.  This prevents the following scenario:
>> 
>> Do you mean, by the method qemu control cpuid exposing, it can avoid
>> live migration broken issue by 
>> 1. user probe the lowest ability host of whole pool where vm may
>> live migrate; 
>> 2. only if the lowest ablility host support the feature can user
>> enable the feature when boot a vm; 
>> 3. if the lowest ability host didn't support the feature (say tsc
>> deadline timer as example), user disable the feature when boot a vm;
>> In this way, live migration wouldn't be broken. Right? 
> 
> Right.

Thanks Avi, for your detailed explanation, fix a long misunderstanding I had for live migration.

Best Regards,
Jinsong

> 
>> or, do you mean qemu-kvm solve live migration broken issue by some
>> other method? 
> 
> The method you outlined, or any other method, such as partitioning the
> cluster according to hardware capabilities.
> 
>> 
>>> 
>>> - a guest is started on some hardware, which doesn't support some
>>> cpuid feature (say AVX for example)
>>> - the guest or one of its applications are broken wrt AVX, but
>>> because the feature is not exposed, it works correctly
>>> - the host hardware is upgraded to one which supports AVX
>>> - the guest is now broken
>> 
>> You mean, live migrate from 'old' (which doesn't support the
>> feature) platform to 'new' platform would broken? 
> 
> Live migration, or even just a guest restart on updated hardware.