[PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Boris Ostrovsky]

# HG changeset patch
# User Boris Ostrovsky <boris.ostrovsky@amd.com>
# Date 1334875170 14400
# Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
# Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
Acked-by: Wei Huang <wei.huang2@amd.com>
Tested-by: Wei Huang <wei.huang2@amd.com>

diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c	Wed Apr 18 16:49:55 2012 +0100
+++ b/xen/arch/x86/hvm/svm/svm.c	Thu Apr 19 18:39:30 2012 -0400
@@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
+    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
 
     general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
-    if ( enable )
+    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
+
+    if ( enable && !cpu_has_tsc_ratio ) {
         general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
+        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
+    }
 
     vmcb_set_general1_intercepts(vmcb, general1_intercepts);
+    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
 }
 
 static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Huang2, Wei]

Please also backport this patch to xen-4.1. Thanks.

-Wei

-----Original Message-----
From: Boris Ostrovsky [mailto:boris.ostrovsky@amd.com] 
Sent: Thursday, April 19, 2012 9:21 PM
To: JBeulich@suse.com; keir@xen.org
Cc: Ostrovsky, Boris; Huang2, Wei; xen-devel@lists.xensource.com
Subject: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

# HG changeset patch
# User Boris Ostrovsky <boris.ostrovsky@amd.com>
# Date 1334875170 14400
# Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
# Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
Acked-by: Wei Huang <wei.huang2@amd.com>
Tested-by: Wei Huang <wei.huang2@amd.com>

diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c	Wed Apr 18 16:49:55 2012 +0100
+++ b/xen/arch/x86/hvm/svm/svm.c	Thu Apr 19 18:39:30 2012 -0400
@@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
+    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
 
     general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
-    if ( enable )
+    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
+
+    if ( enable && !cpu_has_tsc_ratio ) {
         general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
+        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
+    }
 
     vmcb_set_general1_intercepts(vmcb, general1_intercepts);
+    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
 }
 
 static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Keir Fraser]

On 20/04/2012 03:21, "Boris Ostrovsky" <boris.ostrovsky@amd.com> wrote:

> # HG changeset patch
> # User Boris Ostrovsky <boris.ostrovsky@amd.com>
> # Date 1334875170 14400
> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> 
> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> Acked-by: Wei Huang <wei.huang2@amd.com>
> Tested-by: Wei Huang <wei.huang2@amd.com>

Worth an ack/nack from Dan M I'd say. He'll probably have some comment about
possible cross-CPU TSC skew.

 -- Keir

> diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
> --- a/xen/arch/x86/hvm/svm/svm.c Wed Apr 18 16:49:55 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/svm.c Thu Apr 19 18:39:30 2012 -0400
> @@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> +    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
>  
>      general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
> -    if ( enable )
> +    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
> +
> +    if ( enable && !cpu_has_tsc_ratio ) {
>          general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
> +        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
> +    }
>  
>      vmcb_set_general1_intercepts(vmcb, general1_intercepts);
> +    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
>  }
>  
>  static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
> 

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Keir Fraser]

On 20/04/2012 09:05, "Keir Fraser" <keir.xen@gmail.com> wrote:

> On 20/04/2012 03:21, "Boris Ostrovsky" <boris.ostrovsky@amd.com> wrote:
> 
>> # HG changeset patch
>> # User Boris Ostrovsky <boris.ostrovsky@amd.com>
>> # Date 1334875170 14400
>> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
>> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
>> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>> 
>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
>> 
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
>> Acked-by: Wei Huang <wei.huang2@amd.com>
>> Tested-by: Wei Huang <wei.huang2@amd.com>
> 
> Worth an ack/nack from Dan M I'd say. He'll probably have some comment about
> possible cross-CPU TSC skew.

Oh, and apart from that, we're also in feature freeze for 4.2, and this
isn't a bug fix. Similarly, it's not really a candidate for the stable 4.1
branch either, at any time.

 -- Keir

>  -- Keir
> 
>> diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
>> --- a/xen/arch/x86/hvm/svm/svm.c Wed Apr 18 16:49:55 2012 +0100
>> +++ b/xen/arch/x86/hvm/svm/svm.c Thu Apr 19 18:39:30 2012 -0400
>> @@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
>>  {
>>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>> +    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
>>  
>>      general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
>> -    if ( enable )
>> +    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
>> +
>> +    if ( enable && !cpu_has_tsc_ratio ) {
>>          general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
>> +        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
>> +    }
>>  
>>      vmcb_set_general1_intercepts(vmcb, general1_intercepts);
>> +    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
>>  }
>>  
>>  static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
>> 
> 
> 

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Huang2, Wei]

Hi Keir,

This patch is a bug fix for 23437:d7c755c25bb9 than new feature. It slipped my hand and Boris fixed it for me. The same applies to Xen-4.1.

===== Changeset 23437 =====

HVM/SVM: enable tsc scaling ratio for SVM

Future AMD CPUs support TSC scaling. It allows guests to have a
different TSC frequency from host system using this formula: guest_tsc
= host_tsc * tsc_ratio + vmcb_offset. The tsc_ratio is a 64bit MSR
contains a fixed-point number in 8.32 format (8 bits for integer part
and 32bits for fractional part). For instance 0x00000003_80000000
means tsc_ratio=3.5.

This patch enables TSC scaling ratio for SVM. With it, guest VMs don't
need take #VMEXIT to calculate a translated TSC value when it is
running under TSC emulation mode. This can substancially reduce the
rdtsc overhead.


-Wei

-----Original Message-----
From: Keir Fraser [mailto:keir.xen@gmail.com] On Behalf Of Keir Fraser
Sent: Friday, April 20, 2012 3:15 AM
To: Ostrovsky, Boris; JBeulich@suse.com; Dan Magenheimer
Cc: Huang2, Wei; xen-devel@lists.xensource.com
Subject: Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

On 20/04/2012 09:05, "Keir Fraser" <keir.xen@gmail.com> wrote:

> On 20/04/2012 03:21, "Boris Ostrovsky" <boris.ostrovsky@amd.com> wrote:
> 
>> # HG changeset patch
>> # User Boris Ostrovsky <boris.ostrovsky@amd.com>
>> # Date 1334875170 14400
>> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
>> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
>> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>> 
>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
>> 
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
>> Acked-by: Wei Huang <wei.huang2@amd.com>
>> Tested-by: Wei Huang <wei.huang2@amd.com>
> 
> Worth an ack/nack from Dan M I'd say. He'll probably have some comment about
> possible cross-CPU TSC skew.

Oh, and apart from that, we're also in feature freeze for 4.2, and this
isn't a bug fix. Similarly, it's not really a candidate for the stable 4.1
branch either, at any time.

 -- Keir

>  -- Keir
> 
>> diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
>> --- a/xen/arch/x86/hvm/svm/svm.c Wed Apr 18 16:49:55 2012 +0100
>> +++ b/xen/arch/x86/hvm/svm/svm.c Thu Apr 19 18:39:30 2012 -0400
>> @@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
>>  {
>>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>> +    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
>>  
>>      general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
>> -    if ( enable )
>> +    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
>> +
>> +    if ( enable && !cpu_has_tsc_ratio ) {
>>          general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
>> +        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
>> +    }
>>  
>>      vmcb_set_general1_intercepts(vmcb, general1_intercepts);
>> +    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
>>  }
>>  
>>  static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
>> 
> 
> 

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Keir Fraser]

Ah, okay. Well, depending on review feedback on list perhaps it can slip
into the schedule then. :-)


On 20/04/2012 16:06, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:

> Hi Keir,
> 
> This patch is a bug fix for 23437:d7c755c25bb9 than new feature. It slipped my
> hand and Boris fixed it for me. The same applies to Xen-4.1.
> 
> ===== Changeset 23437 =====
> 
> HVM/SVM: enable tsc scaling ratio for SVM
> 
> Future AMD CPUs support TSC scaling. It allows guests to have a
> different TSC frequency from host system using this formula: guest_tsc
> = host_tsc * tsc_ratio + vmcb_offset. The tsc_ratio is a 64bit MSR
> contains a fixed-point number in 8.32 format (8 bits for integer part
> and 32bits for fractional part). For instance 0x00000003_80000000
> means tsc_ratio=3.5.
> 
> This patch enables TSC scaling ratio for SVM. With it, guest VMs don't
> need take #VMEXIT to calculate a translated TSC value when it is
> running under TSC emulation mode. This can substancially reduce the
> rdtsc overhead.
> 
> 
> -Wei
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com] On Behalf Of Keir Fraser
> Sent: Friday, April 20, 2012 3:15 AM
> To: Ostrovsky, Boris; JBeulich@suse.com; Dan Magenheimer
> Cc: Huang2, Wei; xen-devel@lists.xensource.com
> Subject: Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is
> supported by hardware
> 
> On 20/04/2012 09:05, "Keir Fraser" <keir.xen@gmail.com> wrote:
> 
>> On 20/04/2012 03:21, "Boris Ostrovsky" <boris.ostrovsky@amd.com> wrote:
>> 
>>> # HG changeset patch
>>> # User Boris Ostrovsky <boris.ostrovsky@amd.com>
>>> # Date 1334875170 14400
>>> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
>>> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
>>> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>>> 
>>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
>>> 
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
>>> Acked-by: Wei Huang <wei.huang2@amd.com>
>>> Tested-by: Wei Huang <wei.huang2@amd.com>
>> 
>> Worth an ack/nack from Dan M I'd say. He'll probably have some comment about
>> possible cross-CPU TSC skew.
> 
> Oh, and apart from that, we're also in feature freeze for 4.2, and this
> isn't a bug fix. Similarly, it's not really a candidate for the stable 4.1
> branch either, at any time.
> 
>  -- Keir
> 
>>  -- Keir
>> 
>>> diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
>>> --- a/xen/arch/x86/hvm/svm/svm.c Wed Apr 18 16:49:55 2012 +0100
>>> +++ b/xen/arch/x86/hvm/svm/svm.c Thu Apr 19 18:39:30 2012 -0400
>>> @@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
>>>  {
>>>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
>>> +    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
>>>  
>>>      general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
>>> -    if ( enable )
>>> +    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
>>> +
>>> +    if ( enable && !cpu_has_tsc_ratio ) {
>>>          general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
>>> +        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
>>> +    }
>>>  
>>>      vmcb_set_general1_intercepts(vmcb, general1_intercepts);
>>> +    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
>>>  }
>>>  
>>>  static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
>>> 
>> 
>> 
> 
> 
> 
> 

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Dan Magenheimer]

> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Subject: Re: [Xen-devel] [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by
> hardware
> 
> On 20/04/2012 03:21, "Boris Ostrovsky" <boris.ostrovsky@amd.com> wrote:
> 
> > # HG changeset patch
> > # User Boris Ostrovsky <boris.ostrovsky@amd.com>
> > # Date 1334875170 14400
> > # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
> > # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
> > svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> >
> > When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
> > TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
> >
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> > Acked-by: Wei Huang <wei.huang2@amd.com>
> > Tested-by: Wei Huang <wei.huang2@amd.com>
> 
> Worth an ack/nack from Dan M I'd say. He'll probably have some comment about
> possible cross-CPU TSC skew.

Provided the hypervisor writes the "TSC-scale-register" with the same
value when any vcpu for any guest is scheduled, I don't think
there is any cross-CPU TSC skew.

But my recollection is that I had a concern that, to work properly
after migration, TSC scaling might need to implement:

	((B + cur_tsc) * scale ) + A

and AFAIK the feature only implements this for B==0.
Without the rest of the implementation in the hypervisor
and tools, it's hard to determine whether my concern is valid
or not.

Also, I don't recall the exact scaling process but
was also concerned that a guest kernel or userland
process approximating the passage of time by counting
TSC cycles, they might just estimate the value once at
boot (or application startup) and, due to the scaling,
post-migration ticks/second might later change enough
to be a problem.  However, this
is a generic problem with emulation as well, so the
concern is really: Does the TSC scaling use the same
multiplication precision as is available to emulation
in the hypervisor?

Dan

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Wei Huang]

On 04/20/2012 10:02 AM, Dan Magenheimer wrote:
>> From: Keir Fraser [mailto:keir.xen@gmail.com]
>> Subject: Re: [Xen-devel] [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by
>> hardware
>>
>> On 20/04/2012 03:21, "Boris Ostrovsky"<boris.ostrovsky@amd.com>  wrote:
>>
>>> # HG changeset patch
>>> # User Boris Ostrovsky<boris.ostrovsky@amd.com>
>>> # Date 1334875170 14400
>>> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
>>> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
>>> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>>>
>>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
>>>
>>> Signed-off-by: Boris Ostrovsky<boris.ostrovsky@amd.com>
>>> Acked-by: Wei Huang<wei.huang2@amd.com>
>>> Tested-by: Wei Huang<wei.huang2@amd.com>
>> Worth an ack/nack from Dan M I'd say. He'll probably have some comment about
>> possible cross-CPU TSC skew.
> Provided the hypervisor writes the "TSC-scale-register" with the same
> value when any vcpu for any guest is scheduled, I don't think
> there is any cross-CPU TSC skew.
>
> But my recollection is that I had a concern that, to work properly
> after migration, TSC scaling might need to implement:
>
> 	((B + cur_tsc) * scale ) + A
Hi Dan,

Thanks for your review. I tried to interpret this formula as the following:

cur_tsc: Guest TSC before migration
B: time elapsed (overhead) during migration
scale: ratio of guest frequency/target host frequency
A: target host TSC

> and AFAIK the feature only implements this for B==0.
> Without the rest of the implementation in the hypervisor
> and tools, it's hard to determine whether my concern is valid
> or not.
If my interpretation above is correct, doesn't emulated TSC have the 
same problem of B == 0?
> Also, I don't recall the exact scaling process but
> was also concerned that a guest kernel or userland
> process approximating the passage of time by counting
> TSC cycles, they might just estimate the value once at
> boot (or application startup) and, due to the scaling,
> post-migration ticks/second might later change enough
> to be a problem.  However, this
> is a generic problem with emulation as well, so the
> concern is really: Does the TSC scaling use the same
> multiplication precision as is available to emulation
> in the hypervisor?
Hardware TSC scaling uses 8.32 format: 8 bits for integer part and 32 
bits for fraction. Is it enough for the precision from your experience? 
The details of TSC scaling spec can be found on page 554 of 
http://support.amd.com/us/Processor_TechDocs/24593_APM_v2.pdf.
> Dan
>
>

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Dan Magenheimer]

> From: Wei Huang [mailto:wei.huang2@amd.com]
> Subject: Re: [Xen-devel] [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by
> hardware
> 
> >>>
> >>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
> >>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
> >>>
> >>> Signed-off-by: Boris Ostrovsky<boris.ostrovsky@amd.com>
> >>> Acked-by: Wei Huang<wei.huang2@amd.com>
> >>> Tested-by: Wei Huang<wei.huang2@amd.com>
> >> Worth an ack/nack from Dan M I'd say. He'll probably have some comment about
> >> possible cross-CPU TSC skew.
> > Provided the hypervisor writes the "TSC-scale-register" with the same
> > value when any vcpu for any guest is scheduled, I don't think
> > there is any cross-CPU TSC skew.
> >
> > But my recollection is that I had a concern that, to work properly
> > after migration, TSC scaling might need to implement:
> >
> > 	((B + cur_tsc) * scale ) + A
> Hi Dan,
> 
> Thanks for your review. I tried to interpret this formula as the following:
> 
> cur_tsc: Guest TSC before migration
> B: time elapsed (overhead) during migration
> scale: ratio of guest frequency/target host frequency
> A: target host TSC

Hi Wei --

I have to admit I don't remember the details of the problem anymore
and can't mentally reproduce it right now.  If TSC scaling is deployed
(on Xen... I imagine it was designed to accommodate VMware)
and works as well as emulation but faster, great!  I just would hope
that there is a great deal of testing done before it becomes
the default because if problems do occur, they will be extremely
difficult to track down.

>> Without the rest of the implementation in the hypervisor
>> and tools, it's hard to determine whether my concern is valid
>> or not.
> If my interpretation above is correct, doesn't emulated TSC have the 
> same problem of B == 0?

That may be true.  But if the emulation code for HVM is broken and
must be fixed, that's a much easier change than a change to a
hardware instruction.

> Hardware TSC scaling uses 8.32 format: 8 bits for integer part and 32
> bits for fraction. Is it enough for the precision from your experience?
> The details of TSC scaling spec can be found on page 554 of
> http://support.amd.com/us/Processor_TechDocs/24593_APM_v2.pdf.

I didn't write the emulation scaling code for HVM... if it uses
a 32-bit scale factor, your 8+32 should be more precise.  If
it uses a 64-bit multiplier, it will be less precise.  OTOH,
the "how many TSC ticks per second" code in Xen may not be
precise anyway.

Keir/Jan, I still think it would be a good idea to implement a
global boot-time "never trust the hardware TSC" option (default off)
which would result in all guests always emulating TSC.
At least this would be a "baseline" and any problems with
it are due to hypervisor bugs.

Dan

P.S. About to be away from email for a few days.

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Jan Beulich]

>>> On 20.04.12 at 04:21, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> # HG changeset patch
> # User Boris Ostrovsky <boris.ostrovsky@amd.com>
> # Date 1334875170 14400
> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> 
> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.

While the patch itself looks fine, I'm having difficulty to connect the
mentioning of TSC_MODE_ALWAYS_EMULATE to it - afaics all modes
are affected as long as they result in d->arch.vtsc to be set.

Jan

> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> Acked-by: Wei Huang <wei.huang2@amd.com>
> Tested-by: Wei Huang <wei.huang2@amd.com>
> 
> diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
> --- a/xen/arch/x86/hvm/svm/svm.c	Wed Apr 18 16:49:55 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/svm.c	Thu Apr 19 18:39:30 2012 -0400
> @@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> +    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
>  
>      general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
> -    if ( enable )
> +    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
> +
> +    if ( enable && !cpu_has_tsc_ratio ) {
>          general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
> +        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
> +    }
>  
>      vmcb_set_general1_intercepts(vmcb, general1_intercepts);
> +    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
>  }
>  
>  static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Keir Fraser]

On 20/04/2012 09:15, "Jan Beulich" <JBeulich@suse.com> wrote:

>> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>> 
>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
> 
> While the patch itself looks fine, I'm having difficulty to connect the
> mentioning of TSC_MODE_ALWAYS_EMULATE to it - afaics all modes
> are affected as long as they result in d->arch.vtsc to be set.

I think the real point of this patch is they *never* want to trap and
emulate RDTSC on these newer processors.

 -- Keir

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Boris Ostrovsky]

On 04/20/12 04:45, Keir Fraser wrote:
> On 20/04/2012 09:15, "Jan Beulich"<JBeulich@suse.com>  wrote:
>
>>> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>>>
>>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
>>
>> While the patch itself looks fine, I'm having difficulty to connect the
>> mentioning of TSC_MODE_ALWAYS_EMULATE to it - afaics all modes
>> are affected as long as they result in d->arch.vtsc to be set.
>
> I think the real point of this patch is they *never* want to trap and
> emulate RDTSC on these newer processors.


Right. Mentioning TSC_MODE_ALWAYS_EMULATE explicitly wasn't probably a 
particularly good idea.

The reason I did that was because with TSC_MODE_DEFAULT guests (at least 
Linux guests) typically don't use TSC since TSC ends up being declared 
non-invariant.

-boris

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Jan Beulich]

>>> On 20.04.12 at 04:21, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> # HG changeset patch
> # User Boris Ostrovsky <boris.ostrovsky@amd.com>
> # Date 1334875170 14400
> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> 
> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> Acked-by: Wei Huang <wei.huang2@amd.com>
> Tested-by: Wei Huang <wei.huang2@amd.com>

So what's the status of the discussion around this patch? Were
your concerns all addressed, Dan? Is there any re-submisson
necessary/planned?

Jan

> diff -r 7c777cb8f705 -r 55bf11ebce87 xen/arch/x86/hvm/svm/svm.c
> --- a/xen/arch/x86/hvm/svm/svm.c	Wed Apr 18 16:49:55 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/svm.c	Thu Apr 19 18:39:30 2012 -0400
> @@ -724,12 +724,18 @@ static void svm_set_rdtsc_exiting(struct
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>      u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> +    u32 general2_intercepts = vmcb_get_general2_intercepts(vmcb);
>  
>      general1_intercepts &= ~GENERAL1_INTERCEPT_RDTSC;
> -    if ( enable )
> +    general2_intercepts &= ~GENERAL2_INTERCEPT_RDTSCP;
> +
> +    if ( enable && !cpu_has_tsc_ratio ) {
>          general1_intercepts |= GENERAL1_INTERCEPT_RDTSC;
> +        general2_intercepts |= GENERAL2_INTERCEPT_RDTSCP;
> +    }
>  
>      vmcb_set_general1_intercepts(vmcb, general1_intercepts);
> +    vmcb_set_general2_intercepts(vmcb, general2_intercepts);
>  }
>  
>  static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 25, 2012 3:27 AM
> To: Boris Ostrovsky; Dan Magenheimer
> Cc: wei.huang2@amd.com; xen-devel; keir@xen.org
> Subject: Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> 
> >>> On 20.04.12 at 04:21, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> > # HG changeset patch
> > # User Boris Ostrovsky <boris.ostrovsky@amd.com>
> > # Date 1334875170 14400
> > # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
> > # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
> > svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> >
> > When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
> > TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
> >
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> > Acked-by: Wei Huang <wei.huang2@amd.com>
> > Tested-by: Wei Huang <wei.huang2@amd.com>
> 
> So what's the status of the discussion around this patch? Were
> your concerns all addressed, Dan? Is there any re-submisson
> necessary/planned?

My concerns will be addressed when there is a fully-functional
adequately-tested full-stack implementation, rather than "we
have a new instruction that should solve (part of) this problem,
let's turn it on by default."

While I wish I could invest the time required to do (or
participate in) the testing, sadly I can't, so I understand
if my opinion is discarded.

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Jan Beulich]

>>> On 25.04.12 at 17:01, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, April 25, 2012 3:27 AM
>> To: Boris Ostrovsky; Dan Magenheimer
>> Cc: wei.huang2@amd.com; xen-devel; keir@xen.org 
>> Subject: Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is 
> supported by hardware
>> 
>> >>> On 20.04.12 at 04:21, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
>> > # HG changeset patch
>> > # User Boris Ostrovsky <boris.ostrovsky@amd.com>
>> > # Date 1334875170 14400
>> > # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
>> > # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
>> > svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>> >
>> > When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>> > TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
>> >
>> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
>> > Acked-by: Wei Huang <wei.huang2@amd.com>
>> > Tested-by: Wei Huang <wei.huang2@amd.com>
>> 
>> So what's the status of the discussion around this patch? Were
>> your concerns all addressed, Dan? Is there any re-submisson
>> necessary/planned?
> 
> My concerns will be addressed when there is a fully-functional
> adequately-tested full-stack implementation, rather than "we
> have a new instruction that should solve (part of) this problem,
> let's turn it on by default."
> 
> While I wish I could invest the time required to do (or
> participate in) the testing, sadly I can't, so I understand
> if my opinion is discarded.

As Keir had asked to get an ACK/NAK from you - is this then a NAK
or a "don't care" or yet something else (it doesn't read anywhere
close to an ACK in any case).

Jan

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Subject: RE: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> 
> >>> On 25.04.12 at 17:01, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Wednesday, April 25, 2012 3:27 AM
> >> To: Boris Ostrovsky; Dan Magenheimer
> >> Cc: wei.huang2@amd.com; xen-devel; keir@xen.org
> >> Subject: Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is
> > supported by hardware
> >>
> >> >>> On 20.04.12 at 04:21, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> >> > # HG changeset patch
> >> > # User Boris Ostrovsky <boris.ostrovsky@amd.com>
> >> > # Date 1334875170 14400
> >> > # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
> >> > # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
> >> > svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
> >> >
> >> > When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
> >> > TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
> >> >
> >> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> >> > Acked-by: Wei Huang <wei.huang2@amd.com>
> >> > Tested-by: Wei Huang <wei.huang2@amd.com>
> >>
> >> So what's the status of the discussion around this patch? Were
> >> your concerns all addressed, Dan? Is there any re-submisson
> >> necessary/planned?
> >
> > My concerns will be addressed when there is a fully-functional
> > adequately-tested full-stack implementation, rather than "we
> > have a new instruction that should solve (part of) this problem,
> > let's turn it on by default."
> >
> > While I wish I could invest the time required to do (or
> > participate in) the testing, sadly I can't, so I understand
> > if my opinion is discarded.
> 
> As Keir had asked to get an ACK/NAK from you - is this then a NAK
> or a "don't care" or yet something else (it doesn't read anywhere
> close to an ACK in any case).

Something else ;-)

I certainly don't feel comfortable ACKing it.  I'd like
to see some testing that demonstrates the patch either improves
functionality or performance without breaking other things.
But if nobody else shares my concern, I don't feel that
I have the right to block it either.

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Wei Huang]

On 04/25/2012 11:05 AM, Dan Magenheimer wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Subject: RE: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>>
>>>>> On 25.04.12 at 17:01, Dan Magenheimer<dan.magenheimer@oracle.com>  wrote:
>>>>   From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: Wednesday, April 25, 2012 3:27 AM
>>>> To: Boris Ostrovsky; Dan Magenheimer
>>>> Cc: wei.huang2@amd.com; xen-devel; keir@xen.org
>>>> Subject: Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is
>>> supported by hardware
>>>>>>> On 20.04.12 at 04:21, Boris Ostrovsky<boris.ostrovsky@amd.com>  wrote:
>>>>> # HG changeset patch
>>>>> # User Boris Ostrovsky<boris.ostrovsky@amd.com>
>>>>> # Date 1334875170 14400
>>>>> # Node ID 55bf11ebce87ceb73fb2c372dcef170ec0bb4a18
>>>>> # Parent  7c777cb8f705411b77c551f34ba88bdc09e38ab8
>>>>> svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware
>>>>>
>>>>> When running in TSC_MODE_ALWAYS_EMULATE mode on processors that support
>>>>> TSC scaling we don't need to intercept RDTSC/RDTSCP instructions.
>>>>>
>>>>> Signed-off-by: Boris Ostrovsky<boris.ostrovsky@amd.com>
>>>>> Acked-by: Wei Huang<wei.huang2@amd.com>
>>>>> Tested-by: Wei Huang<wei.huang2@amd.com>
>>>> So what's the status of the discussion around this patch? Were
>>>> your concerns all addressed, Dan? Is there any re-submisson
>>>> necessary/planned?
>>> My concerns will be addressed when there is a fully-functional
>>> adequately-tested full-stack implementation, rather than "we
>>> have a new instruction that should solve (part of) this problem,
>>> let's turn it on by default."
>>>
>>> While I wish I could invest the time required to do (or
>>> participate in) the testing, sadly I can't, so I understand
>>> if my opinion is discarded.
>> As Keir had asked to get an ACK/NAK from you - is this then a NAK
>> or a "don't care" or yet something else (it doesn't read anywhere
>> close to an ACK in any case).
> Something else ;-)
>
> I certainly don't feel comfortable ACKing it.  I'd like
> to see some testing that demonstrates the patch either improves
> functionality or performance without breaking other things.
> But if nobody else shares my concern, I don't feel that
> I have the right to block it either.

We can provide some rdtsc performance numbers. Regarding functionality, 
it is relatively hard to prove unless Dan has some more specific ideas 
of testing it. I think the hardware rdtsc scaling is inline with 
software-based emulated approach.

-Wei
>

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Keir Fraser]

On 25/04/2012 17:04, "Wei Huang" <wei.huang2@amd.com> wrote:

>> I certainly don't feel comfortable ACKing it.  I'd like
>> to see some testing that demonstrates the patch either improves
>> functionality or performance without breaking other things.
>> But if nobody else shares my concern, I don't feel that
>> I have the right to block it either.
> 
> We can provide some rdtsc performance numbers. Regarding functionality,
> it is relatively hard to prove unless Dan has some more specific ideas
> of testing it. I think the hardware rdtsc scaling is inline with
> software-based emulated approach.

I've put it in to xen-unstable. I'm not sure about putting into 4.1 for the
forthcoming release from that branch.

 -- Keir

Re: [PATCH] svm: Do not intercept RDTCS(P) when TSC scaling is supported by hardware

[Boris Ostrovsky]

On 04/25/2012 01:14 PM, Keir Fraser wrote:
> On 25/04/2012 17:04, "Wei Huang"<wei.huang2@amd.com>  wrote:
>
>>> I certainly don't feel comfortable ACKing it.  I'd like
>>> to see some testing that demonstrates the patch either improves
>>> functionality or performance without breaking other things.
>>> But if nobody else shares my concern, I don't feel that
>>> I have the right to block it either.
>>
>> We can provide some rdtsc performance numbers. Regarding functionality,
>> it is relatively hard to prove unless Dan has some more specific ideas
>> of testing it. I think the hardware rdtsc scaling is inline with
>> software-based emulated approach.
>
> I've put it in to xen-unstable. I'm not sure about putting into 4.1 for the
> forthcoming release from that branch.

As far as performance numbers are concerned, with this patch applied 
(i.e. native execution) RDTSC instruction executed in a loop takes about 
150ns per iteration. That's including a few instructions for loop control.

With full SW emulation (pre-patch) the same loop is executed in roughly 6us.

Obviously this is not a realistic scenario but it gives a feel of what 
the patch provides.

-boris