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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.04.14 at 19:29, <boris.ostrovsky@xxxxxxxxxx> wrote:
> When a domain is saved each VCPU's TSC value needs to be preserved. To get it 
> we
> use hvm_get_guest_tsc(). This routine (either itself or via get_s_time() which
> it may call) calculates VCPU's TSC based on current host's TSC value (by 
> doing a
> rdtscll()). Since this is performed for each VCPU separately we end up with
> un-synchronized TSCs.
> 
> Similarly, during a restore each VCPU is assigned its TSC based on host's 
> current
> tick, causing virtual TSCs to diverge further.
> 
> With this, we can easily get into situation where a guest may see time going
> backwards.
> 
> Instead of reading new TSC value for each VCPU when saving/restoring it we 
> should
> use the same value across all VCPUs.
> 
> (As part of the patch, update rdtscll's definition so that its variables don't
> clash with outer code)
> 
> Reported-by: Philippe Coquard <philippe.coquard@xxxxxxxx>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

I don't think you can retain this with non-trivial changes done
compared to the version that it was offered for.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -255,16 +255,20 @@ 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)

What I now started wondering is (namely with the struct
hvm_function_table pointer also gaining the extra argument): Is
this ever being called with a zero at_tsc when
v->domain->arch.chkpt_tsc is non-zero? If not, rather than passing
around the value I guess the function could simply read it itself. And
yes, this is meant only for the "set" version, I'm relatively convinced
that the "get" ones would (or could easily become) different.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -308,6 +308,8 @@ struct arch_domain
>                                  (possibly other cases in the future */
>      uint64_t vtsc_kerncount; /* for hvm, counts all vtsc */
>      uint64_t vtsc_usercount; /* not used for hvm */
> +    uint64_t chkpt_tsc;      /* TSC value that VCPUs use to calculate their
> +                                tsc_offset value. Used during save/restore */

I'm sorry for not asking earlier - why does this get put in struct
arch_domain rather than struct hvm_domain?

Jan


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