From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH 06/13] x86/hvm: Scale host TSC when setting/getting guest TSC Date: Tue, 27 Oct 2015 15:16:15 -0500 Message-ID: <562FDB8F.7010301@amd.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> <56290474.8020404@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56290474.8020404@oracle.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: Boris Ostrovsky , Jan Beulich , Suravee Suthikulpanit , Haozhong Zhang Cc: Kevin Tian , Wei Liu , Ian Campbell , Stefano Stabellini , Andrew Cooper , Ian Jackson , xen-devel@lists.xen.org, Jun Nakajima , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 10/22/2015 10:44 AM, Boris Ostrovsky wrote: > On 10/22/2015 10:17 AM, 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). > > I don't see that this series will create any regressions in SVM . Most > of the changes move SVM-specific code into HVM I didn't see any > obvious problems there. I do have concern about patch 5 since I am > sure I fully understand whether the new algorithm (in __scale_tsc()) > is equivalent to current SVM code. I think you also had questions > about that. > > Having said this, the fact that this patch (and patch 9) fix bugs > leads me to believe this feature may not have been thoroughly tested. > > I don't have a pair of appropriate AMD systems to test this series > with migration (which is where this can be verified). Aravind, can you > find something and see how this works? > Haozhong, Boris- I am planning to use a Fam10h system (older processor) and Fam15h Model 60h (newer processor) for the test case. Shall try to run the test on a single system as Haozhong mentioned on a different reply. I ran into a problem with xl right now which I am trying to solve. So, shall keep you posted on how testing goes. Btw, I had issues with applying the patches to my local xen.git branch. Patches 9 and 10 did not apply cleanly. Here is the log from git apply- Patch 9: Checking patch xen/arch/x86/time.c... error: while searching for: } else { _u.tsc_timestamp = t->local_tsc_stamp; _u.system_time = t->stime_local_stamp; _u.tsc_to_system_mul = t->tsc_scale.mul_frac; _u.tsc_shift = (s8)t->tsc_scale.shift; } if ( is_hvm_domain(d) ) _u.tsc_timestamp += v->arch.hvm_vcpu.cache_tsc_offset; error: patch failed: xen/arch/x86/time.c:832 I think the complaint is about "_u.tsc_timestamp = t->local_tsc_stamp;". I checked current master (http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/time.c;h=5d7452a2bf8b8fb830c14f8897cfca65cb1ad39e;hb=refs/heads/master) and the line there is "tsc_stamp = t->local_tsc_stamp" inside the else block and outside it, we have "_u.tsc_timestamp = tsc_stamp" The rejected hunk for Patch 10: +#define VMX_TSC_MULTIPLIER_DEFAULT 0x0001000000000000ULL +#define VMX_TSC_MULTIPLIER_MAX 0xffffffffffffffffULL + #define cpu_has_wbinvd_exiting \ This seems to be because we have the #defines ordered like so on current master- #define VMX_MISC_CR3_TARGET 0x01ff0000 #define VMX_MISC_VMWRITE_ALL 0x20000000 #define cpu_has_wbinvd_exiting \ (vmx_secondary_exec_control & SECONDARY_EXEC_WBINVD_EXITING) but the *_VMWRITE_ALL define is missing on your diff for Patch 10.. Maybe I am missing something? Thanks, -Aravind.