| Message ID | 20180312151233.16565-3-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
Paolo Bonzini <pbonzini@redhat.com> 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..
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
Paolo Bonzini <pbonzini@redhat.com> 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.
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.
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; }
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 <vkuznets@redhat.com> --- target/i386/kvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)