linux-kernel - Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZRyA1jSb_Ok9l0po@google.com>
Date:   Tue, 3 Oct 2023 14:00:06 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Dongli Zhang <dongli.zhang@...cle.com>
Cc:     David Woodhouse <dwmw2@...radead.org>,
        Joe Jin <joe.jin@...cle.com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com
Subject: Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically

On Mon, Oct 02, 2023, Dongli Zhang wrote:
> Hi Sean,
> 
> On 10/2/23 18:49, Sean Christopherson wrote:
> > On Mon, Oct 02, 2023, Dongli Zhang wrote:
> >>> @@ -12185,6 +12203,10 @@ int kvm_arch_hardware_enable(void)
> >>>  	if (ret != 0)
> >>>  		return ret;
> >>>  
> >>> +	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> >>> +		kvm_get_time_scale(NSEC_PER_SEC, tsc_khz * 1000LL,
> >>> +				   &host_tsc_shift, &host_tsc_to_system_mul);
> >>
> >> I agree that to use the kvmclock to calculate the ns elapsed when updating the
> >> master clock.
> >>
> >> Would you take the tsc scaling into consideration?
> >>
> >> While the host_tsc_shift and host_tsc_to_system_mul are pre-computed, how about
> >> the VM using different TSC frequency?
> > 
> > Heh, I'm pretty sure that's completely broken today.  I don't see anything in KVM
> > that takes hardware TSC scaling into account.
> > 
> > This code:
> > 
> > 	if (unlikely(vcpu->hw_tsc_khz != tgt_tsc_khz)) {
> > 		kvm_get_time_scale(NSEC_PER_SEC, tgt_tsc_khz * 1000LL,
> > 				   &vcpu->hv_clock.tsc_shift,
> > 				   &vcpu->hv_clock.tsc_to_system_mul);
> > 		vcpu->hw_tsc_khz = tgt_tsc_khz;
> > 		kvm_xen_update_tsc_info(v);
> > 	}
> > 
> > is recomputing the multipler+shift for the current *physical* CPU, it's not
> > related to the guest's TSC in any way.
> 
> The below is the code.
> 
> line 3175: query freq for current *physical* CPU.
> 
> line 3211: scale the freq if scaling is involved.
> 
> line 3215: compute the view for guest based on new 'tgt_tsc_khz' after scaling.
> 
> 3146 static int kvm_guest_time_update(struct kvm_vcpu *v)
> 3147 {
> 3148         unsigned long flags, tgt_tsc_khz;
> 3149         unsigned seq;
> ... ...
> 3173         /* Keep irq disabled to prevent changes to the clock */
> 3174         local_irq_save(flags);
> 3175         tgt_tsc_khz = get_cpu_tsc_khz();
> ... ...
> 3210         if (kvm_caps.has_tsc_control)
> 3211                 tgt_tsc_khz = kvm_scale_tsc(tgt_tsc_khz,
> 3212                                             v->arch.l1_tsc_scaling_ratio);
> 3213
> 3214         if (unlikely(vcpu->hw_tsc_khz != tgt_tsc_khz)) {
> 3215                 kvm_get_time_scale(NSEC_PER_SEC, tgt_tsc_khz * 1000LL,
> 3216                                    &vcpu->hv_clock.tsc_shift,
> 3217                                    &vcpu->hv_clock.tsc_to_system_mul);
> 3218                 vcpu->hw_tsc_khz = tgt_tsc_khz;
> 3219                 kvm_xen_update_tsc_info(v);
> 3220         }
> 
> 
> Would you please let me know if the above understanding is incorrect?

Ah, yeah, you're correct.  I missed the call to kvm_scale_tsc() at 3211.