Re: [Xen-devel] [PATCH 2/2] x86/HVM: Use fixed TSC value when saving or restoring domain

["Jan Beulich" <JBeulich@xxxxxxxx>]

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -248,19 +248,22 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
>      return 1;
>  }
>  
> -void hvm_set_guest_tsc(struct vcpu *v, u64 guest_tsc)
> +void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc)
>  {
>      uint64_t tsc;
>      uint64_t delta_tsc;
>  
>      if ( v->domain->arch.vtsc )
>      {
> -        tsc = hvm_get_guest_time(v);
> +        tsc = hvm_get_guest_time_fixed(v, at_tsc);
>          tsc = gtime_to_gtsc(v->domain, tsc);
>      }
>      else
>      {
> -        rdtscll(tsc);
> +        if ( at_tsc )
> +            tsc = at_tsc;

Please insert this as an "else if()" above the current "else".

> @@ -279,19 +282,22 @@ void hvm_set_guest_tsc_adjust(struct vcpu *v, u64 
> tsc_adjust)
>      v->arch.hvm_vcpu.msr_tsc_adjust = tsc_adjust;
>  }
>  
> -u64 hvm_get_guest_tsc(struct vcpu *v)
> +u64 hvm_get_guest_tsc_fixed(struct vcpu *v, uint64_t at_tsc)
>  {
>      uint64_t tsc;
>  
>      if ( v->domain->arch.vtsc )
>      {
> -        tsc = hvm_get_guest_time(v);
> +        tsc = hvm_get_guest_time_fixed(v, at_tsc);
>          tsc = gtime_to_gtsc(v->domain, tsc);
>          v->domain->arch.vtsc_kerncount++;
>      }
>      else
>      {
> -        rdtscll(tsc);
> +        if ( at_tsc )
> +            tsc = at_tsc;

Same here.

> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -24,7 +24,7 @@
>  #include <asm/hvm/support.h>
>  #include <public/hvm/save.h>
>  
> -void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
> +void arch_hvm_save(struct domain *dom, struct hvm_save_header *hdr)

The change is unmotivated (afaict) and inconsistent with most other
code - we conventionally use "d" for struct domain * variables. Please
drop the change here and use "d" throughout the rest of the patch,
at once resulting in less churn and hence making it easier to review.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -232,12 +232,15 @@ bool_t hvm_send_assist_req(struct vcpu *v);
>  void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat);
>  int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat);
>  
> -void hvm_set_guest_tsc(struct vcpu *v, u64 guest_tsc);
> -u64 hvm_get_guest_tsc(struct vcpu *v);
> +void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc);
> +#define hvm_set_guest_tsc(v, t) hvm_set_guest_tsc_fixed((v), (t), 0)
> +u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc);
> +#define hvm_get_guest_tsc(v) hvm_get_guest_tsc_fixed((v), 0)
>  
>  void hvm_init_guest_time(struct domain *d);
>  void hvm_set_guest_time(struct vcpu *v, u64 guest_time);
> -u64 hvm_get_guest_time(struct vcpu *v);
> +u64 hvm_get_guest_time_fixed(struct vcpu *v, u64 at_tsc);
> +#define hvm_get_guest_time(v) hvm_get_guest_time_fixed((v), 0)

Pointless parentheses (several instances thereof).

> index 95b4b91..032eb23 100644
> --- a/xen/include/xen/time.h
> +++ b/xen/include/xen/time.h
> @@ -32,7 +32,8 @@ struct vcpu;
>  typedef s64 s_time_t;
>  #define PRI_stime PRId64
>  
> -s_time_t get_s_time(void);
> +s_time_t get_s_time_fixed(u64 at_tick);
> +#define get_s_time() get_s_time_fixed(0)

get_s_time(), through NOW(), has quite many users, so I'm not certain
the code bloat resulting from this is desirable. I'd suggest the function
to remain such; the compiler will be able to make it a mov+jmp.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel