Re: [Xen-devel] [PATCH 06/13] x86/hvm: Scale host TSC when setting/getting guest TSC

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

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, <haozhong.zhang@xxxxxxxxx> 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.

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