[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 06/04/2013 10:46 AM, Roger Pau Monné wrote:
On 04/06/13 11:24, Jan Beulich wrote:
On 04.06.13 at 11:10, 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.

This was not done correctly, since in context_switch
update_vcpu_system_time was called before vmx_do_resume, which caused
the vcpu_info time structure to be updated with the wrong values. This
patch fixes this by calling update_vcpu_system_time after the call to
hvm_set_guest_time has happened.

So at the first glance I was thinking this would be fixing a regression
from commit ae5092f420e87a4a6b541bf581378c8cc0ee3a99, but
after a closer look it looks like this was done even earlier before.
Can you confirm this (not the least because this would have
implications on the need to backport this change)?

I've took a look at the commit, and I don't think it introduced a
regression, a call to update_vcpu_system_time was removed, but this call
was also made before calling context_switch, which wouldn't fix the
problem at hand. This should be backported to all the versions that
expose the XENFEAT_hvm_safe_pvclock feature.

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -343,6 +343,12 @@ void hvm_do_resume(struct vcpu *v)
      ioreq_t *p;

+    /*
+     * Update vcpu_info, since the call to pt_restore_timer can change
+     * the value in v->arch.hvm_vcpu.stime_offset that is used
+     * to calculate the TSC in vcpu_info->time.
+     */
+    update_vcpu_system_time(v);

Adding it here means, unless I'm mistaken, the one in
context_switch() is now pointless, so I'd encourage you to
gate that one on !is_hvm_vcpu() (with a comment saying that
in this case it's being done in hvm_do_resume()).

Yes, the call in context_switch is now superseded by the one in
hvm_do_resume for the HVM case. I will change it and resend the patch,
thanks for the review.

I agree that it's worth trying to avoid calling the same function twice; but since the common case is for pt_restore_timer() to not actually make any substantial changes to hvm_vcpu.stime_offset, I think it would be better if we had the basic "update the system time" call shared between HVM and PV codepaths, and have the uncommon case where hvm_vcpu.stime_offset does change just call it twice.

Updating it at hvm_set_guest_time() will also make sure that there wont' be any problems between the update at pt_intr_post() and the next time the vcpu is scheduled.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.