Re: [PATCH 4/8] KVM: VMX: Adjust the TSC-related VMCS fields on L2 entry and exit

[Paolo Bonzini <pbonzini@redhat.com>]

On 06/05/21 19:35, Stamatis, Ilias wrote:
> On Thu, 2021-05-06 at 13:32 +0200, Paolo Bonzini wrote:
>> On 06/05/21 12:32, ilstam@mailbox.org wrote:
>>> +     if (vmcs12->cpu_based_vm_exec_control &
>>> CPU_BASED_USE_TSC_OFFSETTING) {
>>> +             if (vmcs12->secondary_vm_exec_control &
>>> SECONDARY_EXEC_TSC_SCALING) {
>>> +                     vcpu->arch.tsc_offset =
>>> kvm_compute_02_tsc_offset(
>>> +                                     vcpu->arch.l1_tsc_offset,
>>> +                                     vmcs12->tsc_multiplier,
>>> +                                     vmcs12->tsc_offset);
>>> +
>>> +                     vcpu->arch.tsc_scaling_ratio =
>>> mul_u64_u64_shr(
>>> +                                     vcpu->arch.tsc_scaling_ratio,
>>> +                                     vmcs12->tsc_multiplier,
>>> +                                     kvm_tsc_scaling_ratio_frac_bit
>>> s);
>>> +             } else {
>>> +                     vcpu->arch.tsc_offset += vmcs12->tsc_offset;
>>> +             }
>>
>> The computation of vcpu->arch.tsc_offset is (not coincidentially) the
>> same that appears in patch 6
>>
>> +           (vmcs12->cpu_based_vm_exec_control &
>> CPU_BASED_USE_TSC_OFFSETTING)) {
>> +               if (vmcs12->secondary_vm_exec_control &
>> SECONDARY_EXEC_TSC_SCALING) {
>> +                       cur_offset = kvm_compute_02_tsc_offset(
>> +                                       l1_offset,
>> +                                       vmcs12->tsc_multiplier,
>> +                                       vmcs12->tsc_offset);
>> +               } else {
>> +                       cur_offset = l1_offset + vmcs12->tsc_offset;
>>
>> So I think you should just pass vmcs12 and the L1 offset to
>> kvm_compute_02_tsc_offset, and let it handle both cases (and possibly
>> even set vcpu->arch.tsc_scaling_ratio in the same function).
> 
> That was my thinking initially too. However, kvm_compute_02_tsc_offset
> is defined in x86.c which is vmx-agnostic and hence 'struct vmcs12' is
> not defined there.

Good point.  Yeah, it's trading one comde duplication for another. 
Right now there's already code duplication in write_l1_tsc_offset, so it 
makes some sense to limit duplication _within_ the file and lose in 
duplicated code across vmx/svm, but either is okay.

Paolo