Re: [Qemu-devel] [PATCH 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Paolo Bonzini]

On 16/03/2018 16:40, Vitaly Kuznetsov wrote:
>> On older kernel without re-enlightenment support, you don't want to
>> expose the frequency MSRs unless invtsc is on, right?
>>
> Actually no, I think it's OK to expose frequency MSRs even when we don't
> have invtsc and don't support re-enlightenment. Nested Hyper-V won't
> pass stable TSC pages to its guests unless it sees either invtsc or
> reenlightenment. So as long as we have something to put to these MSRs
> (env->tsc_khz) I *think* we can expose them.
> 
> I may actually be missing the reason why Ladi put
> tsc_is_stable_and_known() here.

Probably because I asked him to. :)  It looks like Hyper-V knows that
you need re-enlightenment in order to really trust the frequency MSRs
(of course the TSC page is special because it has the sequence count).

So the patch is good.  Thanks!

Paolo

> In case we're running Windows (and not
> Hyper-V) as a guest KVM will update TSC page on migration. And genuine
> Hyper-V also exposes these MSRs without exposing INVTSC flag by
> default.

Re: [Qemu-devel] [PATCH 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Vitaly Kuznetsov]

Paolo Bonzini  writes:

> On 16/03/2018 16:05, Vitaly Kuznetsov wrote:
  
 -if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) {
 +if (has_msr_hv_frequencies && env->tsc_khz) {
>>> Should this be
>>>
>>> ((env->tsc_khz && has_msr_hv_reenlightenment) ||
>>>  tsc_is_stable_and_known(env))
>>>
>>> so that you don't regress on older kernels?
>>>
>> I don't actually see where the regression might come from: frequency
>> MSRs are supported regardless or reenlightenment/invtsc and there's
>> nothing wrong with exposing them but I may be missing something..
>
> On older kernel without re-enlightenment support, you don't want to
> expose the frequency MSRs unless invtsc is on, right?
>

Actually no, I think it's OK to expose frequency MSRs even when we don't
have invtsc and don't support re-enlightenment. Nested Hyper-V won't
pass stable TSC pages to its guests unless it sees either invtsc or
reenlightenment. So as long as we have something to put to these MSRs
(env->tsc_khz) I *think* we can expose them.

I may actually be missing the reason why Ladi put
tsc_is_stable_and_known() here. In case we're running Windows (and not
Hyper-V) as a guest KVM will update TSC page on migration. And genuine
Hyper-V also exposes these MSRs without exposing INVTSC flag by
default.

-- 
  Vitaly

Re: [Qemu-devel] [PATCH 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Paolo Bonzini]

On 16/03/2018 16:05, Vitaly Kuznetsov wrote:
>>>  
>>> -if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) {
>>> +if (has_msr_hv_frequencies && env->tsc_khz) {
>> Should this be
>>
>> ((env->tsc_khz && has_msr_hv_reenlightenment) ||
>>  tsc_is_stable_and_known(env))
>>
>> so that you don't regress on older kernels?
>>
> I don't actually see where the regression might come from: frequency
> MSRs are supported regardless or reenlightenment/invtsc and there's
> nothing wrong with exposing them but I may be missing something..

On older kernel without re-enlightenment support, you don't want to
expose the frequency MSRs unless invtsc is on, right?

Paolo

Re: [Qemu-devel] [PATCH 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Vitaly Kuznetsov]

Paolo Bonzini  writes:

> On 12/03/2018 16:12, Vitaly Kuznetsov wrote:
>>  
>> -if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) {
>> +if (has_msr_hv_frequencies && env->tsc_khz) {
>
> Should this be
>
> ((env->tsc_khz && has_msr_hv_reenlightenment) ||
>  tsc_is_stable_and_known(env))
>
> so that you don't regress on older kernels?
>

I don't actually see where the regression might come from: frequency
MSRs are supported regardless or reenlightenment/invtsc and there's
nothing wrong with exposing them but I may be missing something..

-- 
  Vitaly

Re: [Qemu-devel] [PATCH 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Paolo Bonzini]

On 12/03/2018 16:12, Vitaly Kuznetsov wrote:
>  
> -if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) {
> +if (has_msr_hv_frequencies && env->tsc_khz) {

Should this be

((env->tsc_khz && has_msr_hv_reenlightenment) ||
 tsc_is_stable_and_known(env))

so that you don't regress on older kernels?

Paolo

>  env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
>  env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
>  }

[Qemu-devel] [PATCH 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure

[Vitaly Kuznetsov]

Requiring tsc_is_stable_and_known() is too restrictive: even without INVTCS
nested Hyper-V-on-KVM enables TSC pages for its guests e.g. when
Reenlightenment MSRs are present. Presence of frequency MSRs doesn't mean
these frequencies are stable, it just means they're available for reading.

Signed-off-by: Vitaly Kuznetsov 
---
 target/i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 21e06deaf1..43c521f61a 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -646,7 +646,7 @@ static int hyperv_handle_properties(CPUState *cs)
 env->features[FEAT_HYPERV_EAX] |= HV_TIME_REF_COUNT_AVAILABLE;
 env->features[FEAT_HYPERV_EAX] |= HV_REFERENCE_TSC_AVAILABLE;
 
-if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) {
+if (has_msr_hv_frequencies && env->tsc_khz) {
 env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
 env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
 }
-- 
2.14.3