From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?Um9nZXIgUGF1IE1vbm7DqQ==?= Subject: Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time Date: Tue, 4 Jun 2013 11:58:24 +0200 Message-ID: <51ADBA40.6040700@citrix.com> References: <1370337050-2444-1-git-send-email-roger.pau@citrix.com> <51ADB7A4.2000503@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51ADB7A4.2000503@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Keir Fraser , Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 04/06/13 11:47, George Dunlap wrote: > On 06/04/2013 10:10 AM, Roger Pau Monne 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. > > Would it make more sense to actually do this in hvm_set_guest_time() > instead, so that this window where the vcpu system_time is closed for > all callers, not just hvm_do_resume? > > You could gate calling update_vcpu_system_time on: > 1. Whether stime_offset actually changed > 2. Whether v is currently running This was my first approach, but I thought it was better to fix the actual call to update_vcpu_system_time to happen in the right place (before the context switch), rather than adding more calls to update_vcpu_system_time all over the place. If later we decide for whatever reason to add more offsets to the vtsc, we will also have to add more calls to update_vcpu_system_time in every place that we modify those offsets, which doesn't seem right.