Re: [PATCH v4 06/10] x86/hvm: Setup TSC scaling ratio

From: "Zhang, Haozhong" <haozhong.zhang@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v4 06/10] x86/hvm: Setup TSC scaling ratio
Date: Tue, 16 Feb 2016 17:44:06 +0800	[thread overview]
Message-ID: <20160216094406.GB6519@hz-desktop.sh.intel.com> (raw)
In-Reply-To: <56B4B7AD02000078000CF10B@prv-mh.provo.novell.com>

On 02/05/16 21:54, Jan Beulich wrote:
> >>> On 17.01.16 at 22:58, <haozhong.zhang@intel.com> wrote:
> > +u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz)
> > +{
> > +    u64 ratio;
> > +
> > +    if ( !hvm_tsc_scaling_supported )
> > +        return 0;
> > +
> > +    /*
> > +     * The multiplication of the first two terms may overflow a 64-bit
> > +     * integer, so use mul_u64_u32_div() instead to keep precision.
> > +     */
> > +    ratio = mul_u64_u32_div(1ULL << hvm_funcs.tsc_scaling_ratio_frac_bits,
> > +                            gtsc_khz, cpu_khz);
> 
> Is this the only use for this new math64 function? If so, I don't
> see the point of adding that function, because (leaving limited
> significant bits aside) the above simply is
> 
> (gtsc_khz << hvm_funcs.tsc_scaling_ratio_frac_bits) / cpu_khz
> 
> which can be had without any multiplication. Personally, if indeed
> the only use I'd favor converting the above to inline assembly
> here instead of adding that helper function (just like we have a
> number of asm()-s in x86/time.c for similar reasons).
>

OK, I'll rewrite it as asm(). mul_u64_u32_div() will not be used any
more and will be removed.

I'll also inline another math64 function mul_u64_u64_shr() in its single
caller hvm_scale_tsc(). Then the math64 patch will be dropped in the
next version.

> > +void hvm_setup_tsc_scaling(struct vcpu *v)
> > +{
> > +    v->arch.hvm_vcpu.tsc_scaling_ratio =
> > +        hvm_get_tsc_scaling_ratio(v->domain->arch.tsc_khz);
> > +}
> 
> So why again is this per-vCPU setup of per-vCPU state when it
> only depends on a per-domain input? If this was per-domain, its
> setup could be where it belongs - in arch_hvm_load().
>

It's a per-domain state. I'll the state to x86's struct arch_domain
where other TSC fields are (or struct hvm_domain, because this is only
used for HVM?).

Then it will be setup in tsc_set_info() after guest tsc frequency is
determined.

> > @@ -5504,6 +5536,9 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
> >      hvm_set_segment_register(v, x86_seg_gdtr, &reg);
> >      hvm_set_segment_register(v, x86_seg_idtr, &reg);
> >  
> > +    if ( hvm_tsc_scaling_supported && !d->arch.vtsc )
> > +        hvm_setup_tsc_scaling(v);
> 
> Could you remind me why this is needed? What state of the guest
> would have changed making this necessary? Is this perhaps just
> because it's per-vCPU instead of per-domain?
> 
> Jan
> 

Yes, just because I mistakenly made it per-vcpu. So it will be not
necessary in this patch after tsc_scaling_ratio become per-domain.

Haozhong