Re: [PATCH 2/6] kvm: x86: drop read_tsc_offset()

[Jim Mattson]

vmx_read_tsc_offset has a bug when running nested VMs.  It should really be:

       if (is_guest_mode(vcpu))
               return to_vmx(vcpu)->nested.vmcs01_tsc_offset;
       else
               return vmcs_read64(TSC_OFFSET);

Perhaps a better name woulf be "vmx_get_l1_tsc_offset."

In any case, this does not seem consistent with vcpu->arch.tsc_offset.

On Fri, Sep 16, 2016 at 7:27 AM, Luiz Capitulino <lcapitulino@xxxxxxxxxx> wrote:
> The TSC offset can now be read directly from struct kvm_arch_vcpu.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 -
>  arch/x86/kvm/svm.c              | 8 --------
>  arch/x86/kvm/vmx.c              | 6 ------
>  arch/x86/kvm/x86.c              | 2 +-
>  4 files changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 9b36a31..c4c5ac5 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -954,7 +954,6 @@ struct kvm_x86_ops {
>
>         bool (*has_wbinvd_exit)(void);
>
> -       u64 (*read_tsc_offset)(struct kvm_vcpu *vcpu);
>         void (*write_tsc_offset)(struct kvm_vcpu *vcpu, u64 offset);
>
>         u64 (*read_l1_tsc)(struct kvm_vcpu *vcpu, u64 host_tsc);
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index db77c1c..8023d53 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1119,13 +1119,6 @@ static void init_sys_seg(struct vmcb_seg *seg, uint32_t type)
>         seg->base = 0;
>  }
>
> -static u64 svm_read_tsc_offset(struct kvm_vcpu *vcpu)
> -{
> -       struct vcpu_svm *svm = to_svm(vcpu);
> -
> -       return svm->vmcb->control.tsc_offset;
> -}
> -
>  static void svm_write_tsc_offset(struct kvm_vcpu *vcpu, u64 offset)
>  {
>         struct vcpu_svm *svm = to_svm(vcpu);
> @@ -5427,7 +5420,6 @@ static struct kvm_x86_ops svm_x86_ops = {
>
>         .has_wbinvd_exit = svm_has_wbinvd_exit,
>
> -       .read_tsc_offset = svm_read_tsc_offset,
>         .write_tsc_offset = svm_write_tsc_offset,
>         .adjust_tsc_offset_guest = svm_adjust_tsc_offset_guest,
>         .read_l1_tsc = svm_read_l1_tsc,
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 5cede40..29cbd4b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2603,11 +2603,6 @@ static u64 vmx_read_l1_tsc(struct kvm_vcpu *vcpu, u64 host_tsc)
>         return host_tsc + tsc_offset;
>  }
>
> -static u64 vmx_read_tsc_offset(struct kvm_vcpu *vcpu)
> -{
> -       return vmcs_read64(TSC_OFFSET);
> -}
> -
>  /*
>   * writes 'offset' into guest's timestamp counter offset register
>   */
> @@ -11274,7 +11269,6 @@ static struct kvm_x86_ops vmx_x86_ops = {
>
>         .has_wbinvd_exit = cpu_has_vmx_wbinvd_exit,
>
> -       .read_tsc_offset = vmx_read_tsc_offset,
>         .write_tsc_offset = vmx_write_tsc_offset,
>         .adjust_tsc_offset_guest = vmx_adjust_tsc_offset_guest,
>         .read_l1_tsc = vmx_read_l1_tsc,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cda4ca5..1651668 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1367,7 +1367,7 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
>
>  static void update_ia32_tsc_adjust_msr(struct kvm_vcpu *vcpu, s64 offset)
>  {
> -       u64 curr_offset = kvm_x86_ops->read_tsc_offset(vcpu);
> +       u64 curr_offset = vcpu->arch.tsc_offset;
>         vcpu->arch.ia32_tsc_adjust_msr += offset - curr_offset;
>  }
>
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html