Re: [PATCH v4 04/10] x86/hvm: Collect information of TSC scaling ratio

["Zhang, Haozhong" <haozhong.zhang@intel.com>]

On 02/05/16 19:41, Jan Beulich wrote:
> >>> On 17.01.16 at 22:58, <haozhong.zhang@intel.com> wrote:
> > Both VMX TSC scaling and SVM TSC ratio use the 64-bit TSC scaling ratio,
> > but the number of fractional bits of the ratio is different between VMX
> > and SVM. This patch adds the architecture code to collect the number of
> > fractional bits and other related information into fields of struct
> > hvm_function_table so that they can be used in the common code.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > ---
> > Changes in v4:
> >  (addressing Jan Beulich's comments in v3 patch 12)
> >  * Set TSC scaling parameters in hvm_funcs conditionally.
> >  * Remove TSC scaling parameter tsc_scaling_supported in hvm_funcs which
> >    can be derived from other parameters.
> >  (code cleanup)
> >  * Merge with v3 patch 11 "x86/hvm: Detect TSC scaling through hvm_funcs"
> >    whose work can be done early in this patch.
> 
> I really think this the scope of these changes should have invalidated
> all earlier tags.
>

I'll remove all R-b tags.

> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -1450,6 +1450,14 @@ const struct hvm_function_table * __init start_svm(void)
> >      if ( !cpu_has_svm_nrips )
> >          clear_bit(SVM_FEATURE_DECODEASSISTS, &svm_feature_flags);
> >  
> > +    if ( cpu_has_tsc_ratio )
> > +    {
> > +        svm_function_table.default_tsc_scaling_ratio = DEFAULT_TSC_RATIO;
> > +        svm_function_table.max_tsc_scaling_ratio = ~TSC_RATIO_RSVD_BITS;
> > +        svm_function_table.tsc_scaling_ratio_frac_bits = 32;
> > +        svm_function_table.scale_tsc = svm_scale_tsc;
> > +    }
> > +
> >  #define P(p,s) if ( p ) { printk(" - %s\n", s); printed = 1; }
> >      P(cpu_has_svm_npt, "Nested Page Tables (NPT)");
> >      P(cpu_has_svm_lbrv, "Last Branch Record (LBR) Virtualisation");
> > @@ -2269,8 +2277,6 @@ static struct hvm_function_table __initdata svm_function_table = {
> >      .nhvm_vmcx_hap_enabled = nsvm_vmcb_hap_enabled,
> >      .nhvm_intr_blocked = nsvm_intr_blocked,
> >      .nhvm_hap_walk_L1_p2m = nsvm_hap_walk_L1_p2m,
> > -
> > -    .scale_tsc            = svm_scale_tsc,
> >  };
> 
> From at the first glance purely mechanical POV this change was
> unnecessary with ...
> 
> > @@ -249,6 +261,8 @@ void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc);
> >  u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc);
> >  #define hvm_get_guest_tsc(v) hvm_get_guest_tsc_fixed(v, 0)
> >  
> > +#define hvm_tsc_scaling_supported (!!hvm_funcs.default_tsc_scaling_ratio)
> 
> ... this, but considering our general aim to avoid having NULL
> callback pointers wherever possible, I think this is more than just
> a mechanical concern: I'd prefer if at least the callback pointer
> always be statically initialized, and ideally also two of the other
> fields. Only one field should be dynamically initialized (unless -
> considering the VMX code to come - static initialization is
> impossible), and ideally one which, if zero, would not have any
> bad consequences if used by mistake (frac_bits maybe). And
> perhaps an ASSERT() should be placed inside svm_scale_tsc()
> making sure the dynamically initialized field actually is initialized.
>

Combined with your comments for patch 9, I'll leave only
tsc_scaling_ratio_frac_bits to be dynamically initialized.

> The conditional here would then check _all_ fields which either
> vendor's code leaves uninitialized (i.e. the VMX patch may then
> add to the above).
>

so it would be
    #define hvm_tsc_scaling_supported (!!hvm_funcs.tsc_scaling_ratio_frac_bits)


Thanks,
Haozhong