From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 06/13] x86/hvm: Scale host TSC when setting/getting guest TSC Date: Tue, 27 Oct 2015 09:10:19 -0400 Message-ID: <562F77BB.2080204@oracle.com> References: <1443424438-13404-1-git-send-email-haozhong.zhang@intel.com> <1443424438-13404-7-git-send-email-haozhong.zhang@intel.com> <56290C1902000078000AD930@prv-mh.provo.novell.com> <20151027084448.GC15208@hzzhang-OptiPlex-9020.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151027084448.GC15208@hzzhang-OptiPlex-9020.sh.intel.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: Suravee Suthikulpanit , Aravind Gopalakrishnan , Jan Beulich , Andrew Cooper , Ian Campbell , Wei Liu , Ian Jackson , Stefano Stabellini , Jun Nakajima , Kevin Tian , xen-devel@lists.xen.org, Keir Fraser List-Id: xen-devel@lists.xenproject.org On 10/27/2015 04:44 AM, Haozhong Zhang wrote: > On Thu, Oct 22, 2015 at 08:17:29AM -0600, Jan Beulich wrote: >>>>> On 28.09.15 at 09:13, wrote: >>> The existing hvm_set_guest_tsc_fixed() and hvm_get_guest_tsc_fixed() >>> calculate the guest TSC by adding the TSC offset to the host TSC. When >>> the TSC scaling is enabled, the host TSC should be scaled first. This >>> patch adds the scaling logic to those two functions. >> Just like mentioned for the first twp patches - I'd first of all like to >> understand why the lack of scaling this wasn't an issue for SVM so >> far. What you reads plausible, but assuming that SVM TSC scaling >> code was tested, I'm hesitant to apply changes to it without >> understanding the details (or at least without SVM maintainers' >> consent). >> > Hi SVM maintainers, > > Could you help to review this patch 6 as well as patch 2? They intend > to fix bugs in SVM TSC ratio code (or code that affects SVM TSC ratio > code). > > The detailed explanations of patch 2 and patch 6 can be found at > http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg01490.html > and > http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg02843.html > respectively. I agree with patch 2 (so you can add my Reviewed-by). but I am not so sure about patch 6 (and 11, together with existing SVM handlers). I don't have latest Intel's manual handy but for SVM the guest TSC value is calculated as scaled host TSC plus *unscaled* VMCB's TSC offset. Is Intel's implementation similar? Both svm_set_tsc_offset and vmx_set_tsc_offset (as proposed in patch 11) write VMCB/VMCS with guest (i.s. scaled) offset, and that doesn't seem right. If I am right then I think (1) SVM is broken now and (2) patches 6 and 11 don't fix this brokenness and instead propagate it to VMX. (I should have thought about this when I last replied to you asking to move scaling out of vmx_set_tsc_offset(). But I re-read this code now and it doesn't make sense to me anymore). -boris > > Thanks, > Haozhong > >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -388,13 +388,12 @@ void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc) >>> tsc = hvm_get_guest_time_fixed(v, at_tsc); >>> tsc = gtime_to_gtsc(v->domain, tsc); >>> } >>> - else if ( at_tsc ) >>> - { >>> - tsc = at_tsc; >>> - } >>> else >>> { >>> - tsc = rdtsc(); >>> + tsc = at_tsc ? at_tsc : rdtsc(); >> In cases like this please prefer the gcc extension allowing the middle >> operand of the ?: to be omitted. >> >> Jan >>