| Message ID | 1451531020-29964-5-git-send-email-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/30/2015 10:03 PM, Haozhong Zhang wrote: > This patch makes the pvclock return the scaled host TSC and > corresponding scaling parameters to HVM domains if guest TSC is not > emulated and TSC scaling is enabled. > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > --- > Changes in v3: > (addressing Boris Ostrovsky's comments) > * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0 > if host_tsc_is_safe() is not satisfied, so it is safe to use > d->arch.vtsc_to_ns here. I don't think I understand your argument here. I think last time we decided that vtsc_to_ns can be used only if TSC is constant. -boris > > xen/arch/x86/time.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > index d83f068..b31634a 100644 > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) > } > else > { > - tsc_stamp = t->local_tsc_stamp; > - > - _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > - _u.tsc_shift = (s8)t->tsc_scale.shift; > + if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) > + { > + tsc_stamp = hvm_funcs.scale_tsc(v, t->local_tsc_stamp); > + _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; > + _u.tsc_shift = d->arch.vtsc_to_ns.shift; > + } > + else > + { > + tsc_stamp = t->local_tsc_stamp; > + _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > + _u.tsc_shift = (s8)t->tsc_scale.shift; > + } > } > > _u.tsc_timestamp = tsc_stamp;
On 01/04/16 13:09, Boris Ostrovsky wrote: > On 12/30/2015 10:03 PM, Haozhong Zhang wrote: > >This patch makes the pvclock return the scaled host TSC and > >corresponding scaling parameters to HVM domains if guest TSC is not > >emulated and TSC scaling is enabled. > > > >Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > >--- > >Changes in v3: > > (addressing Boris Ostrovsky's comments) > > * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0 > > if host_tsc_is_safe() is not satisfied, so it is safe to use > > d->arch.vtsc_to_ns here. > > I don't think I understand your argument here. I think last time we decided > that vtsc_to_ns can be used only if TSC is constant. > > -boris > In tsc_set_info(), if TSC scaling is available and host has X86_FEATURE_TSC_RELIABLE (checked by host_tsc_is_safe()), vtsc is left to 0. And looking at init_amd() and init_intel(), X86_FEATURE_CONSTANT_TSC is available whenever X86_FEATURE_TSC_RELIABLE is available as well. Therefore, when vtsc_to_ns is used in __update_vcpu_system_time(), we can always ensure that host has X86_FEATURE_CONSTANT_TSC and do not need to recheck it there. Haozhong > > > > xen/arch/x86/time.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > >diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > >index d83f068..b31634a 100644 > >--- a/xen/arch/x86/time.c > >+++ b/xen/arch/x86/time.c > >@@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) > > } > > else > > { > >- tsc_stamp = t->local_tsc_stamp; > >- > >- _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > >- _u.tsc_shift = (s8)t->tsc_scale.shift; > >+ if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) > >+ { > >+ tsc_stamp = hvm_funcs.scale_tsc(v, t->local_tsc_stamp); > >+ _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; > >+ _u.tsc_shift = d->arch.vtsc_to_ns.shift; > >+ } > >+ else > >+ { > >+ tsc_stamp = t->local_tsc_stamp; > >+ _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > >+ _u.tsc_shift = (s8)t->tsc_scale.shift; > >+ } > > } > > _u.tsc_timestamp = tsc_stamp; >
On 01/04/2016 07:59 PM, Haozhong Zhang wrote: > On 01/04/16 13:09, Boris Ostrovsky wrote: >> On 12/30/2015 10:03 PM, Haozhong Zhang wrote: >>> This patch makes the pvclock return the scaled host TSC and >>> corresponding scaling parameters to HVM domains if guest TSC is not >>> emulated and TSC scaling is enabled. >>> >>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> >>> --- >>> Changes in v3: >>> (addressing Boris Ostrovsky's comments) >>> * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0 >>> if host_tsc_is_safe() is not satisfied, so it is safe to use >>> d->arch.vtsc_to_ns here. >> I don't think I understand your argument here. I think last time we decided >> that vtsc_to_ns can be used only if TSC is constant. >> >> -boris >> > In tsc_set_info(), if TSC scaling is available and host has > X86_FEATURE_TSC_RELIABLE (checked by host_tsc_is_safe()), vtsc is left > to 0. And looking at init_amd() and init_intel(), > X86_FEATURE_CONSTANT_TSC is available whenever > X86_FEATURE_TSC_RELIABLE is available as well. Therefore, when > vtsc_to_ns is used in __update_vcpu_system_time(), we can always > ensure that host has X86_FEATURE_CONSTANT_TSC and do not need to > recheck it there. OK --- I was looking at tsc_set_info()'s TSC_MODE_NEVER_EMULATE path but now I realize that we don't use scaling in that mode (right?). Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Haozhong > >>> xen/arch/x86/time.c | 16 ++++++++++++---- >>> 1 file changed, 12 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c >>> index d83f068..b31634a 100644 >>> --- a/xen/arch/x86/time.c >>> +++ b/xen/arch/x86/time.c >>> @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) >>> } >>> else >>> { >>> - tsc_stamp = t->local_tsc_stamp; >>> - >>> - _u.tsc_to_system_mul = t->tsc_scale.mul_frac; >>> - _u.tsc_shift = (s8)t->tsc_scale.shift; >>> + if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) >>> + { >>> + tsc_stamp = hvm_funcs.scale_tsc(v, t->local_tsc_stamp); >>> + _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; >>> + _u.tsc_shift = d->arch.vtsc_to_ns.shift; >>> + } >>> + else >>> + { >>> + tsc_stamp = t->local_tsc_stamp; >>> + _u.tsc_to_system_mul = t->tsc_scale.mul_frac; >>> + _u.tsc_shift = (s8)t->tsc_scale.shift; >>> + } >>> } >>> _u.tsc_timestamp = tsc_stamp;
On 01/05/16 11:15, Boris Ostrovsky wrote: > On 01/04/2016 07:59 PM, Haozhong Zhang wrote: > >On 01/04/16 13:09, Boris Ostrovsky wrote: > >>On 12/30/2015 10:03 PM, Haozhong Zhang wrote: > >>>This patch makes the pvclock return the scaled host TSC and > >>>corresponding scaling parameters to HVM domains if guest TSC is not > >>>emulated and TSC scaling is enabled. > >>> > >>>Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > >>>--- > >>>Changes in v3: > >>> (addressing Boris Ostrovsky's comments) > >>> * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0 > >>> if host_tsc_is_safe() is not satisfied, so it is safe to use > >>> d->arch.vtsc_to_ns here. > >>I don't think I understand your argument here. I think last time we decided > >>that vtsc_to_ns can be used only if TSC is constant. > >> > >>-boris > >> > >In tsc_set_info(), if TSC scaling is available and host has > >X86_FEATURE_TSC_RELIABLE (checked by host_tsc_is_safe()), vtsc is left > >to 0. And looking at init_amd() and init_intel(), > >X86_FEATURE_CONSTANT_TSC is available whenever > >X86_FEATURE_TSC_RELIABLE is available as well. Therefore, when > >vtsc_to_ns is used in __update_vcpu_system_time(), we can always > >ensure that host has X86_FEATURE_CONSTANT_TSC and do not need to > >recheck it there. > > OK --- I was looking at tsc_set_info()'s TSC_MODE_NEVER_EMULATE path but now > I realize that we don't use scaling in that mode (right?). > Right. Haozhong > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > > >Haozhong > > > >>> xen/arch/x86/time.c | 16 ++++++++++++---- > >>> 1 file changed, 12 insertions(+), 4 deletions(-) > >>> > >>>diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > >>>index d83f068..b31634a 100644 > >>>--- a/xen/arch/x86/time.c > >>>+++ b/xen/arch/x86/time.c > >>>@@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) > >>> } > >>> else > >>> { > >>>- tsc_stamp = t->local_tsc_stamp; > >>>- > >>>- _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > >>>- _u.tsc_shift = (s8)t->tsc_scale.shift; > >>>+ if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) > >>>+ { > >>>+ tsc_stamp = hvm_funcs.scale_tsc(v, t->local_tsc_stamp); > >>>+ _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; > >>>+ _u.tsc_shift = d->arch.vtsc_to_ns.shift; > >>>+ } > >>>+ else > >>>+ { > >>>+ tsc_stamp = t->local_tsc_stamp; > >>>+ _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > >>>+ _u.tsc_shift = (s8)t->tsc_scale.shift; > >>>+ } > >>> } > >>> _u.tsc_timestamp = tsc_stamp; > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 31.12.15 at 04:03, <haozhong.zhang@intel.com> wrote: > This patch makes the pvclock return the scaled host TSC and > corresponding scaling parameters to HVM domains if guest TSC is not > emulated and TSC scaling is enabled. > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > --- > Changes in v3: > (addressing Boris Ostrovsky's comments) > * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0 > if host_tsc_is_safe() is not satisfied, so it is safe to use > d->arch.vtsc_to_ns here. > > xen/arch/x86/time.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > index d83f068..b31634a 100644 > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) > } > else > { > - tsc_stamp = t->local_tsc_stamp; > - > - _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > - _u.tsc_shift = (s8)t->tsc_scale.shift; > + if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) For symmetry with patches 1 and 2 I'd expect !d->arch.vtsc to be added here too. > + { > + tsc_stamp = hvm_funcs.scale_tsc(v, t->local_tsc_stamp); > + _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; > + _u.tsc_shift = d->arch.vtsc_to_ns.shift; > + } > + else > + { > + tsc_stamp = t->local_tsc_stamp; > + _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > + _u.tsc_shift = (s8)t->tsc_scale.shift; The case has been pointless anyway, and you don't add a similar one in the if() branch - please delete it. Jan
On 01/08/16 02:20, Jan Beulich wrote: > >>> On 31.12.15 at 04:03, <haozhong.zhang@intel.com> wrote: > > This patch makes the pvclock return the scaled host TSC and > > corresponding scaling parameters to HVM domains if guest TSC is not > > emulated and TSC scaling is enabled. > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > > --- > > Changes in v3: > > (addressing Boris Ostrovsky's comments) > > * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0 > > if host_tsc_is_safe() is not satisfied, so it is safe to use > > d->arch.vtsc_to_ns here. > > > > xen/arch/x86/time.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > > index d83f068..b31634a 100644 > > --- a/xen/arch/x86/time.c > > +++ b/xen/arch/x86/time.c > > @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) > > } > > else > > { > > - tsc_stamp = t->local_tsc_stamp; > > - > > - _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > > - _u.tsc_shift = (s8)t->tsc_scale.shift; > > + if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) > > For symmetry with patches 1 and 2 I'd expect !d->arch.vtsc to be > added here too. > even though it's already in the !d->arch.vtsc branch? But I'm fine to add it for symmetric. > > + { > > + tsc_stamp = hvm_funcs.scale_tsc(v, t->local_tsc_stamp); > > + _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; > > + _u.tsc_shift = d->arch.vtsc_to_ns.shift; > > + } > > + else > > + { > > + tsc_stamp = t->local_tsc_stamp; > > + _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > > + _u.tsc_shift = (s8)t->tsc_scale.shift; > > The case has been pointless anyway, and you don't add a similar > one in the if() branch - please delete it. > I don't understand why it's pointless. The inner if branch is to ensure pvclock provides the scaled TSC information when TSC scaling is used, and the inner else branch is to keep the existing behavior when TSC scaling is not used. For the outer if branch, TSC scaling is never used, so no change is needed. Haozhong
>>> On 08.01.16 at 14:22, <haozhong.zhang@intel.com> wrote: > On 01/08/16 02:20, Jan Beulich wrote: >> >>> On 31.12.15 at 04:03, <haozhong.zhang@intel.com> wrote: >> > --- a/xen/arch/x86/time.c >> > +++ b/xen/arch/x86/time.c >> > @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) >> > } >> > else >> > { >> > - tsc_stamp = t->local_tsc_stamp; >> > - >> > - _u.tsc_to_system_mul = t->tsc_scale.mul_frac; >> > - _u.tsc_shift = (s8)t->tsc_scale.shift; >> > + if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) >> >> For symmetry with patches 1 and 2 I'd expect !d->arch.vtsc to be >> added here too. > > even though it's already in the !d->arch.vtsc branch? But I'm fine to > add it for symmetric. Ah, no, I didn't realize (only looking at the patch) that it's in the else to a respective if(). No need to change it then. >> > + { >> > + tsc_stamp = hvm_funcs.scale_tsc(v, > t->local_tsc_stamp); >> > + _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; >> > + _u.tsc_shift = d->arch.vtsc_to_ns.shift; >> > + } >> > + else >> > + { >> > + tsc_stamp = t->local_tsc_stamp; >> > + _u.tsc_to_system_mul = t->tsc_scale.mul_frac; >> > + _u.tsc_shift = (s8)t->tsc_scale.shift; >> >> The case has been pointless anyway, and you don't add a similar >> one in the if() branch - please delete it. > > I don't understand why it's pointless. The inner if branch is to > ensure pvclock provides the scaled TSC information when TSC scaling is > used, and the inner else branch is to keep the existing behavior when > TSC scaling is not used. For the outer if branch, TSC scaling is never > used, so no change is needed. Sorry, I see I typo-ed my reply. It was meant to start "The cast has been ..." Jan
On 01/08/16 06:43, Jan Beulich wrote: > >>> On 08.01.16 at 14:22, <haozhong.zhang@intel.com> wrote: > > On 01/08/16 02:20, Jan Beulich wrote: > >> >>> On 31.12.15 at 04:03, <haozhong.zhang@intel.com> wrote: > >> > --- a/xen/arch/x86/time.c > >> > +++ b/xen/arch/x86/time.c > >> > @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) > >> > } > >> > else > >> > { > >> > - tsc_stamp = t->local_tsc_stamp; > >> > - > >> > - _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > >> > - _u.tsc_shift = (s8)t->tsc_scale.shift; > >> > + if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) > >> > >> For symmetry with patches 1 and 2 I'd expect !d->arch.vtsc to be > >> added here too. > > > > even though it's already in the !d->arch.vtsc branch? But I'm fine to > > add it for symmetric. > > Ah, no, I didn't realize (only looking at the patch) that it's in the > else to a respective if(). No need to change it then. > > >> > + { > >> > + tsc_stamp = hvm_funcs.scale_tsc(v, > > t->local_tsc_stamp); > >> > + _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; > >> > + _u.tsc_shift = d->arch.vtsc_to_ns.shift; > >> > + } > >> > + else > >> > + { > >> > + tsc_stamp = t->local_tsc_stamp; > >> > + _u.tsc_to_system_mul = t->tsc_scale.mul_frac; > >> > + _u.tsc_shift = (s8)t->tsc_scale.shift; > >> > >> The case has been pointless anyway, and you don't add a similar > >> one in the if() branch - please delete it. > > > > I don't understand why it's pointless. The inner if branch is to > > ensure pvclock provides the scaled TSC information when TSC scaling is > > used, and the inner else branch is to keep the existing behavior when > > TSC scaling is not used. For the outer if branch, TSC scaling is never > > used, so no change is needed. > > Sorry, I see I typo-ed my reply. It was meant to start "The cast > has been ..." > OK, I'll remove it. Haozhong
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index d83f068..b31634a 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, int force) } else { - tsc_stamp = t->local_tsc_stamp; - - _u.tsc_to_system_mul = t->tsc_scale.mul_frac; - _u.tsc_shift = (s8)t->tsc_scale.shift; + if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio ) + { + tsc_stamp = hvm_funcs.scale_tsc(v, t->local_tsc_stamp); + _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac; + _u.tsc_shift = d->arch.vtsc_to_ns.shift; + } + else + { + tsc_stamp = t->local_tsc_stamp; + _u.tsc_to_system_mul = t->tsc_scale.mul_frac; + _u.tsc_shift = (s8)t->tsc_scale.shift; + } } _u.tsc_timestamp = tsc_stamp;
This patch makes the pvclock return the scaled host TSC and corresponding scaling parameters to HVM domains if guest TSC is not emulated and TSC scaling is enabled. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- Changes in v3: (addressing Boris Ostrovsky's comments) * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0 if host_tsc_is_safe() is not satisfied, so it is safe to use d->arch.vtsc_to_ns here. xen/arch/x86/time.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)