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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Thu, Oct 22, 2015 at 11:44:52AM -0400, 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?
> 
> -boris
>

Thank Boris and any guys who can help to test this patch on AMD systems!

On both specifications, VMX TSC scaling and SVM TSC ratio intend to
address the same problem and take quite similar approach. Thus, when I
originally started to prepare this patchset and before I lift the
common code, I simply copied corresponding SVM TSC ratio code to VMX
and only made necessary adaptions and assumed all other
hardware-independent code was correct.

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
> >
> 

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