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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Tue, Oct 27, 2015 at 03:16:15PM -0500, Aravind Gopalakrishnan wrote:
> 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.

Hi Aravind,

This patchset has been sent out for quite a while and is based on
commit 9cc1346. Something has changed in master and broken the
structure of patch 9 and patch 10. You can either try the old commit
9cc1346, or my rebased patch 9 and patch 10 in the attachment (on
commit e08f383).

Thanks,
Haozhong

Attachment: 0009-x86-time.c-Scale-host-TSC-in-pvclock-properly-rebased.patch
Description: Text document

Attachment: 0010-vmx-Detect-and-initialize-VMX-RDTSC-P-scaling-rebased.patch
Description: Text document

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