Re: [Xen-devel] [PATCH v2] x86/vtsc: update vcpu_time in hvm_set_guest_time

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.06.13 at 14:49, Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
> When using a vtsc, hvm_set_guest_time changes hvm_vcpu.stime_offset,
> which is used in the vcpu time structure to calculate the
> tsc_timestamp, so after updating stime_offset we need to propagate the
> change to vcpu_time in order for the guest to get the right time if
> using the PV clock.
> 
> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> ---
> Changes since v1:
>  * Perform the call to update_vcpu_system_time in hvm_set_guest_time
>    if the offset has changed and the vCPU is running.
> ---
>  xen/arch/x86/hvm/vpt.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> index 8dee662..d37d214 100644
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -57,7 +57,18 @@ u64 hvm_get_guest_time(struct vcpu *v)
>  
>  void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>  {
> -    v->arch.hvm_vcpu.stime_offset += guest_time - hvm_get_guest_time(v);
> +    u64 offset = guest_time - hvm_get_guest_time(v);
> +
> +    if ( offset ) {
> +        v->arch.hvm_vcpu.stime_offset += offset;
> +        /*
> +         * If hvm_vcpu.stime_offset is updated make sure to
> +         * also update vcpu time, since this value is used to
> +         * calculate the TSC.
> +         */
> +        if ( v->is_running )

I'm afraid this is at least a latent bug: "v->is_running" gets set
before "current" gets switched, and in the intermediate time
__hvm_copy() would copy to the wrong VM (or crash, if "current"
isn't a HVM vCPU).

> +            update_vcpu_system_time(v);
> +    }

Right now, both call paths

{svm,vmx}_intr_assist() -> pt_intr_post() -> hvm_set_guest_time()

and

{svm,vmx}_do_resume() -> hvm_do_resume() -> pt_restore_timer()
-> pt_thaw_time() -> hvm_set_guest_time()

appear to be safe, but it doesn't feel well to rely on this. I'd
therefore prefer to switch the condition above to "v == current"
or, considering that there are no other call paths (easier to prove
if hvm_set_guest_time() was made static, perhaps simply
ASSERT(v == current) before the call.

Jan

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