Xen devel - Dec 2009 - [PATCH 02/02] VMX: Add HVM RDTSCP support

Add RDTSCP instruction support for HVM VMX guest.
 - RDTSCP is introduced in Nehalem processor on Intel platform. Like RDTSC,
RDTSCP will return the TSC value, besides, it will return the low 32bit of
TSC_AUX MSR. Currently Linux kernel will write (node_id << 12 |
process_id)
into that MSR, so that when guest execs RDTSCP, it will also get processor
information.
 - This instruction is supported for HVM only when the hardware has this
capability (indicated by cpuid).

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 11.12.09
12:53 >>>
>Add RDTSCP instruction support for HVM VMX guest.
> - RDTSCP is introduced in Nehalem processor on Intel platform. Like RDTSC,
>RDTSCP will return the TSC value, besides, it will return the low 32bit of
>TSC_AUX MSR. Currently Linux kernel will write (node_id << 12 |
process_id)
>into that MSR, so that when guest execs RDTSCP, it will also get processor
>information.
> - This instruction is supported for HVM only when the hardware has this
>capability (indicated by cpuid).
This
>--- a/xen/arch/x86/hvm/vmx/vmx.c	Tue Dec 08 14:14:27 2009 +0000
>+++ b/xen/arch/x86/hvm/vmx/vmx.c	Fri Dec 11 19:10:53 2009 +0800
>@@ -138,7 +138,7 @@ static DEFINE_PER_CPU(struct vmx_msr_sta
> 
> static u32 msr_index[VMX_MSR_COUNT] > {
>-    MSR_LSTAR, MSR_STAR, MSR_SYSCALL_MASK
>+    MSR_LSTAR, MSR_STAR, MSR_SYSCALL_MASK, MSR_TSC_AUX
> };
> 
> static void vmx_save_host_msrs(void)
>@@ -146,8 +146,11 @@ static void vmx_save_host_msrs(void)
>     struct vmx_msr_state *host_msr_state = &this_cpu(host_msr_state);
>     int i;
> 
>-    for ( i = 0; i < VMX_MSR_COUNT; i++ )
>+    for ( i = 0; i < VMX_MSR_COUNT - 1; i++ )
>         rdmsrl(msr_index[i], host_msr_state->msrs[i]);
>+
>+    if ( cpu_has_rdtscp )
>+        rdmsrl(MSR_TSC_AUX,
host_msr_state->msrs[VMX_INDEX_MSR_TSC_AUX]);
> }
> 
> #define WRITE_MSR(address)                                              \
seems to be calling for future problems - if anyone adds an element to
msr_index[] before MSR_TSC_AUX, the logic will silently break. A
BUILD_BUG_ON() and a comment would be the minimum thing to add
in my opinion.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan, 
	Thanks for reminder. I ever noticed this problem, but I forgot to modify the
code in that version.
Attached is the updated one.

Best Regards,
-- Dongxiao

Jan Beulich wrote:
>>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 11.12.09
12:53 >>>
>> Add RDTSCP instruction support for HVM VMX guest.
>> - RDTSCP is introduced in Nehalem processor on Intel platform. Like
>> RDTSC, RDTSCP will return the TSC value, besides, it will return the
>> low 32bit of TSC_AUX MSR. Currently Linux kernel will write (node_id
>> << 12 | process_id) into that MSR, so that when guest execs
RDTSCP,
>> it will also get processor information. - This instruction is
>> supported for HVM only when the hardware has this capability
>> (indicated by cpuid). 
> 
> This
> 
>> --- a/xen/arch/x86/hvm/vmx/vmx.c	Tue Dec 08 14:14:27 2009 +0000
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c	Fri Dec 11 19:10:53 2009 +0800
>> @@ -138,7 +138,7 @@ static DEFINE_PER_CPU(struct vmx_msr_sta
>> 
>> static u32 msr_index[VMX_MSR_COUNT] >> {
>> -    MSR_LSTAR, MSR_STAR, MSR_SYSCALL_MASK
>> +    MSR_LSTAR, MSR_STAR, MSR_SYSCALL_MASK, MSR_TSC_AUX };
>> 
>> static void vmx_save_host_msrs(void)
>> @@ -146,8 +146,11 @@ static void vmx_save_host_msrs(void)
>>     struct vmx_msr_state *host_msr_state >>
&this_cpu(host_msr_state);     int i;
>> 
>> -    for ( i = 0; i < VMX_MSR_COUNT; i++ )
>> +    for ( i = 0; i < VMX_MSR_COUNT - 1; i++ )
>>         rdmsrl(msr_index[i], host_msr_state->msrs[i]); +
>> +    if ( cpu_has_rdtscp )
>> +        rdmsrl(MSR_TSC_AUX,
>> host_msr_state->msrs[VMX_INDEX_MSR_TSC_AUX]); } 
>> 
>> #define WRITE_MSR(address)                                          
>> \ 
> 
> seems to be calling for future problems - if anyone adds an element to
> msr_index[] before MSR_TSC_AUX, the logic will silently break. A
> BUILD_BUG_ON() and a comment would be the minimum thing to add
> in my opinion.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 11.12.09
14:10 >>>
>	Thanks for reminder. I ever noticed this problem, but I forgot to modify
the code in that version.
>Attached is the updated one.
Sorry, but I don''t think this is any better. Sill no build-time
checking, and
still the potential for getting completely out of sync what is already very
fragile (the VMX_INDEX_MSR_* enumeration and the msr_index array):
Just to repeat, what if somebody adds another enumerator (naturally)
at the end (i.e. after VMX_INDEX_MSR_TSC_AUX), and the
corresponding MSR index at the end of msr_index[]?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan, 
	Here is the updated one, I add the BUILD_BUG_ON() and some comments. 
Also CC Dan. Thanks!

Best Regards, 
-- Dongxiao

Jan Beulich wrote:
>>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 11.12.09
14:10 >>>
>> 	Thanks for reminder. I ever noticed this problem, but I forgot to
>> modify the code in that version. Attached is the updated one.
> 
> Sorry, but I don''t think this is any better. Sill no build-time
> checking, and still the potential for getting completely out of sync
> what is already very fragile (the VMX_INDEX_MSR_* enumeration and the
> msr_index array): 
> Just to repeat, what if somebody adds another enumerator (naturally)
> at the end (i.e. after VMX_INDEX_MSR_TSC_AUX), and the
> corresponding MSR index at the end of msr_index[]?
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 11.12.09
17:22 >>>
>	Here is the updated one, I add the BUILD_BUG_ON() and some comments. 
I still don''t think that''s what we want: It''s not a
bug if MSR_INDEX_SIZE
is not 3. A BUILD_BUG_ON() should check things that ought to be
guaranteed, but shouldn''t require adjustment each time some table
gets expanded/shrunk. What you want to minimally check is
ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX &&
VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 1.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Jan, 
	There are two kinds of conditions for adding new MSRs, let''s call it
CONDITION 1
and CONDITION 2.
1. Developer adding new MSR both in msr_index[] and the enum.
2. Developer adding new MSR *ONLY* in enum. (Like TSC_AUX MSR)

My original thought is that, since both msr_index and enum is seldom modified,
so I
add that BUILD_BUG_ON(MSR_INDEX_SIZE != 3) to reminder the developer that
currently
we only save three MSRs for host state. However, as you said, he need to adjust
the
BUILD_BUG_ON Condition each time for "CONDITION 1". But if he only
adds MSR into
enum (CONDITION 2, also like the TSC_AUX MSR), then he is not needed to modify
the
BUILD_BUG_ON Condition.

I understand what you mean now. This is to reminder developer that, if he want
to add
new MSR both into the enum and msr_index[], it should be placed in the end of
msr_index[],
but BEFORE VMX_INDEX_MSR_TSC_AUX in enum. Then he is not needed to modify he
following condition check.
"ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX && 
VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 1".
But it he ONLY wants to add the MSR in enum (CONDITION 2, also like the TSC_AUX
MSR),
the modifcation to BUILD_BUG_ON condition is needed:
"ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX && 
VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 2".

Both approaches have the effect to reminder that, MSRs in enum is not the same
as MSRs
in msr_index[]. Anyway I attached the patch according to your suggestion.

Jan Beulich wrote:
>>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 11.12.09
17:22 >>>
>> 	Here is the updated one, I add the BUILD_BUG_ON() and some comments.
> 
> I still don''t think that''s what we want: It''s
not a bug if
> MSR_INDEX_SIZE is not 3. A BUILD_BUG_ON() should check things that
> ought to be guaranteed, but shouldn''t require adjustment each time
> some table 
> gets expanded/shrunk. What you want to minimally check is
> ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX &&
> VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 1.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

One other concern with this patch... is it possible
for an HVM to migrate from a machine that has rdtscp
support to a machine that does not?  If so, the
migration could result in a nasty crash unless you
also have code that emulates rdtscp if it is not
present.  Some of the code exists for this already
but I think you need to intercept illegal instruction
traps, filter out the traps caused by rdtscp,
and inject the rest.  (This is done already for
PV domains.)
> -----Original Message-----
> From: Xu, Dongxiao [mailto:dongxiao.xu@intel.com]
> Sent: Friday, December 11, 2009 4:33 PM
> To: Jan Beulich
> Cc: Dan Magenheimer; xen-devel@lists.xensource.com; Keir Fraser
> Subject: RE: [Xen-devel][PATCH 02/02] VMX: Add HVM RDTSCP support
> 
> 
> Hi, Jan, 
> 	There are two kinds of conditions for adding new MSRs, 
> let''s call it CONDITION 1
> and CONDITION 2.
> 1. Developer adding new MSR both in msr_index[] and the enum.
> 2. Developer adding new MSR *ONLY* in enum. (Like TSC_AUX MSR)
> 
> My original thought is that, since both msr_index and enum is 
> seldom modified, so I 
> add that BUILD_BUG_ON(MSR_INDEX_SIZE != 3) to reminder the 
> developer that currently
> we only save three MSRs for host state. However, as you said, 
> he need to adjust the 
> BUILD_BUG_ON Condition each time for "CONDITION 1". But if he 
> only adds MSR into
> enum (CONDITION 2, also like the TSC_AUX MSR), then he is not 
> needed to modify the
> BUILD_BUG_ON Condition.
> 
> I understand what you mean now. This is to reminder developer 
> that, if he want to add
> new MSR both into the enum and msr_index[], it should be 
> placed in the end of msr_index[],
> but BEFORE VMX_INDEX_MSR_TSC_AUX in enum. Then he is not 
> needed to modify he
> following condition check.
> "ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX && 
> VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 1".
> But it he ONLY wants to add the MSR in enum (CONDITION 2, 
> also like the TSC_AUX MSR),
> the modifcation to BUILD_BUG_ON condition is needed:
> "ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX && 
> VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 2".
> 
> Both approaches have the effect to reminder that, MSRs in 
> enum is not the same as MSRs
> in msr_index[]. Anyway I attached the patch according to your 
> suggestion.
> 
> Jan Beulich wrote:
> >>>> "Xu, Dongxiao" <dongxiao.xu@intel.com>
11.12.09 17:22 >>>
> >> 	Here is the updated one, I add the BUILD_BUG_ON() and 
> some comments.
> > 
> > I still don''t think that''s what we want:
It''s not a bug if
> > MSR_INDEX_SIZE is not 3. A BUILD_BUG_ON() should check things that
> > ought to be guaranteed, but shouldn''t require adjustment each
time
> > some table 
> > gets expanded/shrunk. What you want to minimally check is
> > ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX &&
> > VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 1.
> > 
> > Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Whether a system has rdtscp support is indicated by
the cpuid. Management tool or system admin should
use CPUIDdetermine whether the migration is allowed. 
I think besides RDTSCP, we already have such cases.

Thanks!
Dongxiao

Dan Magenheimer wrote:
> One other concern with this patch... is it possible
> for an HVM to migrate from a machine that has rdtscp
> support to a machine that does not?  If so, the
> migration could result in a nasty crash unless you
> also have code that emulates rdtscp if it is not
> present.  Some of the code exists for this already
> but I think you need to intercept illegal instruction
> traps, filter out the traps caused by rdtscp,
> and inject the rest.  (This is done already for
> PV domains.)
> 
>> -----Original Message-----
>> From: Xu, Dongxiao [mailto:dongxiao.xu@intel.com]
>> Sent: Friday, December 11, 2009 4:33 PM
>> To: Jan Beulich
>> Cc: Dan Magenheimer; xen-devel@lists.xensource.com; Keir Fraser
>> Subject: RE: [Xen-devel][PATCH 02/02] VMX: Add HVM RDTSCP support
>> 
>> 
>> Hi, Jan,
>> 	There are two kinds of conditions for adding new MSRs,
>> let''s call it CONDITION 1
>> and CONDITION 2.
>> 1. Developer adding new MSR both in msr_index[] and the enum.
>> 2. Developer adding new MSR *ONLY* in enum. (Like TSC_AUX MSR)
>> 
>> My original thought is that, since both msr_index and enum is seldom
>> modified, so I add that BUILD_BUG_ON(MSR_INDEX_SIZE != 3) to
>> reminder the developer that currently we only save three MSRs for
>> host state. However, as you said, 
>> he need to adjust the
>> BUILD_BUG_ON Condition each time for "CONDITION 1". But if he
only
>> adds MSR into enum (CONDITION 2, also like the TSC_AUX MSR), then he
>> is not needed to modify the BUILD_BUG_ON Condition.
>> 
>> I understand what you mean now. This is to reminder developer that,
>> if he want to add new MSR both into the enum and msr_index[], it
>> should be 
>> placed in the end of msr_index[],
>> but BEFORE VMX_INDEX_MSR_TSC_AUX in enum. Then he is not
>> needed to modify he
>> following condition check.
>> "ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX &&
>> VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 1".
>> But it he ONLY wants to add the MSR in enum (CONDITION 2,
>> also like the TSC_AUX MSR),
>> the modifcation to BUILD_BUG_ON condition is needed:
>> "ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX &&
>> VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 2".
>> 
>> Both approaches have the effect to reminder that, MSRs in
>> enum is not the same as MSRs
>> in msr_index[]. Anyway I attached the patch according to your
>> suggestion. 
>> 
>> Jan Beulich wrote:
>>>>>> "Xu, Dongxiao" <dongxiao.xu@intel.com>
11.12.09 17:22 >>>
>>>> 	Here is the updated one, I add the BUILD_BUG_ON() and some
>>>> comments. 
>>> 
>>> I still don''t think that''s what we want:
It''s not a bug if
>>> MSR_INDEX_SIZE is not 3. A BUILD_BUG_ON() should check things that
>>> ought to be guaranteed, but shouldn''t require adjustment
each time
>>> some table gets expanded/shrunk. What you want to minimally check
is
>>> ARRAY_SIZE(msr_index) == VMX_INDEX_MSR_TSC_AUX &&
>>> VMX_INDEX_MSR_TSC_AUX == VMX_MSR_COUNT - 1.
>>> 
>>> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 12.12.09
00:32 >>>
>Both approaches have the effect to reminder that, MSRs in enum is not the
same as MSRs
>in msr_index[]. Anyway I attached the patch according to your suggestion.
Thanks!

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel