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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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@intel.com> 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.