Re: [Xen-devel] [PATCH v7 2/2] x86/viridian: Add partition time reference counter MSR support

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 28 August 2014 08:34
> To: Paul Durrant
> Cc: Christoph Egger; Ian Campbell; Ian Jackson; Stefano Stabellini; xen-
> devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v7 2/2] x86/viridian: Add partition time reference
> counter MSR support
> 
> >>> On 27.08.14 at 18:09, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 27 August 2014 17:02
> >> To: Paul Durrant
> >> Cc: Christoph Egger; Ian Campbell; Ian Jackson; Stefano Stabellini; xen-
> >> devel@xxxxxxxxxxxxx; Keir (Xen.org)
> >> Subject: Re: [PATCH v7 2/2] x86/viridian: Add partition time reference
> >> counter MSR support
> >>
> >> >>> On 27.08.14 at 17:44, <paul.durrant@xxxxxxxxxx> wrote:
> >> > This patch optionally re-instates support for the partition time 
> >> > reference
> >> > counter that was previously introduced by commit
> >> > e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b and reverted by commit
> >> > 1cd4fab14ce25859efa4a2af13475e6650a5506c. The previous
> >> implementation was
> >> > non-optional and flawed.
> >> >
> >> > This implementation uses the tsc of vcpu0, which is preserved across
> >> > save/restore as part of the architectural state, and then converts that
> >> > to a 100ns tick using the domain's tsc_khz.
> >> >
> >> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >> > Cc: Keir Fraser <keir@xxxxxxx>
> >> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >> > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> >> > Cc: Christoph Egger <chegger@xxxxxxxxx>
> >> > ---
> >> >  docs/man/xl.cfg.pod.5            |    7 +++++++
> >> >  tools/libxl/libxl_dom.c          |    3 +++
> >> >  tools/libxl/libxl_types.idl      |    1 +
> >> >  xen/arch/x86/hvm/viridian.c      |   28 +++++++++++++++++++++++-----
> >> >  xen/arch/x86/time.c              |    4 ++--
> >> >  xen/include/asm-x86/perfc_defn.h |    1 +
> >> >  xen/include/asm-x86/time.h       |    4 ++++
> >> >  xen/include/public/hvm/params.h  |    9 ++++++++-
> >> >  8 files changed, 49 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >> > index d91f101..ac3798a 100644
> >> > --- a/docs/man/xl.cfg.pod.5
> >> > +++ b/docs/man/xl.cfg.pod.5
> >> > @@ -1131,6 +1131,13 @@ This set incorporates the TSC and APIC
> >> frequency MSRs.
> >> >  This enlightenment can improve performance of Windows 7 and
> Windows
> >> >  Server 2008 R2 onwards.
> >> >
> >> > +=item B<time_ref_count>
> >> > +
> >> > +This set incorporates Partition Time Reference Counter MSR.
> >> > +
> >> > +This enlightenment can improve performance of Windows 8 and
> Windows
> >> > +Server 2012 onwards.
> >> > +
> >> >  =back
> >> >
> >> >  See the latest version of Microsoft's Hypervisor Top-Level Functional
> >> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> >> > index e854984..579188a 100644
> >> > --- a/tools/libxl/libxl_dom.c
> >> > +++ b/tools/libxl/libxl_dom.c
> >> > @@ -241,6 +241,9 @@ static void hvm_set_viridian_features(libxl__gc
> *gc,
> >> > uint32_t domid,
> >> >              case LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ:
> >> >                  feature_mask &= ~HVMPV_no_freq;
> >> >                  break;
> >> > +            case LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT:
> >> > +                feature_mask |= HVMPV_time_ref_count;
> >> > +                break;
> >> >              default:
> >> >                  LIBXL__LOG(CTX, LIBXL__LOG_WARNING,
> >> >                             "Unknown viridian enlightenment %d\n",
> >> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> > index 3b0059c..a2a523e 100644
> >> > --- a/tools/libxl/libxl_types.idl
> >> > +++ b/tools/libxl/libxl_types.idl
> >> > @@ -183,6 +183,7 @@ libxl_viridian_enlightenment =
> >> > Enumeration("viridian_enlightenment", [
> >> >      (0, "unknown"),
> >> >      (1, "base"),
> >> >      (2, "freq"),
> >> > +    (3, "time_ref_count"),
> >> >      ])
> >> >
> >> >  #
> >> > diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> >> > index 28c4479..04cd158 100644
> >> > --- a/xen/arch/x86/hvm/viridian.c
> >> > +++ b/xen/arch/x86/hvm/viridian.c
> >> > @@ -36,11 +36,11 @@
> >> >  #define HvNotifyLongSpinWait    8
> >> >
> >> >  /* Viridian CPUID 4000003, Viridian MSR availability. */
> >> > -#define CPUID3A_MSR_REF_COUNT   (1 << 1)
> >> > -#define CPUID3A_MSR_APIC_ACCESS (1 << 4)
> >> > -#define CPUID3A_MSR_HYPERCALL   (1 << 5)
> >> > -#define CPUID3A_MSR_VP_INDEX    (1 << 6)
> >> > -#define CPUID3A_MSR_FREQ        (1 << 11)
> >> > +#define CPUID3A_MSR_TIME_REF_COUNT (1 << 1)
> >> > +#define CPUID3A_MSR_APIC_ACCESS    (1 << 4)
> >> > +#define CPUID3A_MSR_HYPERCALL      (1 << 5)
> >> > +#define CPUID3A_MSR_VP_INDEX       (1 << 6)
> >> > +#define CPUID3A_MSR_FREQ           (1 << 11)
> >> >
> >> >  /* Viridian CPUID 4000004, Implementation Recommendations. */
> >> >  #define CPUID4A_MSR_BASED_APIC  (1 << 3)
> >> > @@ -93,6 +93,8 @@ int cpuid_viridian_leaves(unsigned int leaf,
> unsigned
> >> int
> >> > *eax,
> >> >                  CPUID3A_MSR_VP_INDEX);
> >> >          if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> >> >              *eax |= CPUID3A_MSR_FREQ;
> >> > +        if ( viridian_feature_mask(d) & HVMPV_time_ref_count )
> >> > +            *eax |= CPUID3A_MSR_TIME_REF_COUNT;
> >> >          break;
> >> >      case 4:
> >> >          /* Recommended hypercall usage. */
> >> > @@ -344,6 +346,22 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t
> *val)
> >> >          *val = v->arch.hvm_vcpu.viridian.apic_assist.raw;
> >> >          break;
> >> >
> >> > +    case VIRIDIAN_MSR_TIME_REF_COUNT:
> >> > +    {
> >> > +        uint64_t tsc;
> >> > +        struct time_scale tsc_to_ns;
> >> > +
> >> > +        if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> >> > +            return 0;
> >> > +
> >> > +        perfc_incr(mshv_rdmsr_time_ref_count);
> >> > +        tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
> >> > +
> >> > +        /* convert tsc to count of 100ns periods */
> >> > +        set_time_scale(&tsc_to_ns, d->arch.tsc_khz * 1000);
> >>
> >> While the second parameter of the function has type u64, nothing
> >> forces the multiplication to be done at 64-bit width (and 4GHz or
> >> beyond clock speed isn't plain impossible).
> >>
> >
> > True. The promotion will probably done afterwards; I'll add a cast to make
> > sure it's done before.
> 
> Or perhaps rather than an ugly cast, just use 1000ULL?
> 

Yes, that would be sufficient but I thought a cast would make it more obvious. 
I don't particularly care though. Are you ok with the cast as it is in the v8 
patch?

  Paul

> Jan

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