Re: [PATCH v2 03/10] KVM: X86: Add kvm_scale_tsc_l1() and kvm_compute_tsc_offset_l1()

["Stamatis, Ilias" <ilstam@amazon.com>]

On Wed, 2021-05-19 at 15:40 +0000, Sean Christopherson wrote:
> On Wed, May 19, 2021, Stamatis, Ilias wrote:
> > On Tue, 2021-05-18 at 23:04 +0000, Sean Christopherson wrote:
> > > On Wed, May 12, 2021, Ilias Stamatis wrote:
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index 07cf5d7ece38..84af1af7a2cc 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -2319,18 +2319,30 @@ u64 kvm_scale_tsc(struct kvm_vcpu *vcpu, u64 tsc)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(kvm_scale_tsc);
> > > > 
> > > > -static u64 kvm_compute_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc)
> > > > +u64 kvm_scale_tsc_l1(struct kvm_vcpu *vcpu, u64 tsc)
> > > > +{
> > > > +     u64 _tsc = tsc;
> > > > +     u64 ratio = vcpu->arch.l1_tsc_scaling_ratio;
> > > > +
> > > > +     if (ratio != kvm_default_tsc_scaling_ratio)
> > > > +             _tsc = __scale_tsc(ratio, tsc);
> > > > +
> > > > +     return _tsc;
> > > > +}
> > > 
> > > Just make the ratio a param.  This is complete copy+paste of kvm_scale_tsc(),
> > > with 3 characters added.  And all of the callers are already in an L1-specific
> > > function or have L1 vs. L2 awareness.  IMO, that makes the code less magical, too,
> > > as I don't have to dive into a helper to see that it reads l1_tsc_scaling_ratio
> > > versus tsc_scaling_ratio.
> > > 
> > 
> > That's how I did it initially but changed it into a separate function after
> > receiving feedback on v1. I'm neutral, I don't mind changing it back.
> 
> Ah, I see the conundrum.  The vendor code isn't straightforward because of all
> the enabling checks against vmcs12 controls.
> 
> Given that, I don't terribly mind the callbacks, but I do think the connection
> between the computation and the VMWRITE needs to be more explicit.
> 
> Poking around the code, the other thing that would help would be to get rid of
> the awful decache_tsc_multiplier().  That helper was added to paper over the
> completely broken logic of commit ff2c3a180377 ("KVM: VMX: Setup TSC scaling
> ratio when a vcpu is loaded").  Its use in vmx_vcpu_load_vmcs() is basically
> "write the VMCS if we forgot to earlier", which is all kinds of wrong.
> 

I am going to add a patch that removes decache_tsc_multiplier() and 
vmx->current_tsc_ratio as the latter is useless since vcpu->arch.tsc_scaling_ratio 
is already the current ratio. And without it decache_tsc_multiplier() becomes
an one-liner that is pointless to have; we can do vmcs_write64() directly.

Nevertheless, I am not going to move the code outside of vmx_vcpu_load_vmcs().
Granted, a better place for setting the multiplier in hardware would be
set_tsc_khz(). But this function is inside x86.c so it would require yet
another vendor callback to be added, move the svm code too, etc, etc.

Much more refactoring can be done in KVM code in general but I don't think it
has to be part of this series. I am going to send the v3 patches tomorrow. 

Ilias