Xen devel - Oct 2009 - [PATCH] pvcpuid: mask TSC invariant bit for various circumstances

pvcpuid: mask TSC invariant bit for PV domains if migration
is not disabled and TSC is not emulated

(Need similar patch for HVM domain)

Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>

diff -r d7d7f978d704 xen/arch/x86/domain.c
--- a/xen/arch/x86/domain.c	Tue Oct 20 14:36:01 2009 +0100
+++ b/xen/arch/x86/domain.c	Mon Oct 26 16:13:43 2009 -0600
@@ -51,6 +51,7 @@
 #include <asm/nmi.h>
 #include <xen/numa.h>
 #include <xen/iommu.h>
+#include <public/arch-x86/cpuid.h>
 #ifdef CONFIG_COMPAT
 #include <compat/vcpu.h>
 #endif
@@ -2044,6 +2045,13 @@ void domain_cpuid(
             *ebx = cpuid->ebx;
             *ecx = cpuid->ecx;
             *edx = cpuid->edx;
+            if ( input == XEN_CPUID_APM_FUNCTION )
+            {
+                /* mask TSC invariant bit if migration is not disabled
+                   and TSC is not emulated */
+                if ( !d->disable_migrate && !d->arch.vtsc )
+                    *edx &= ~XEN_CPUID_APM_EDX_TSC_INVARIANT;
+            }
             return;
         }
     }
diff -r d7d7f978d704 xen/include/public/arch-x86/cpuid.h
--- a/xen/include/public/arch-x86/cpuid.h	Tue Oct 20 14:36:01 2009 +0100
+++ b/xen/include/public/arch-x86/cpuid.h	Mon Oct 26 16:13:43 2009 -0600
@@ -65,4 +65,8 @@
 #define _XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD 0
 #define XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD  (1u<<0)
 
+/* Does the host support TSC Invariance (in Advanced Power Management)? */
+#define XEN_CPUID_APM_FUNCTION          0x80000007
+#define XEN_CPUID_APM_EDX_TSC_INVARIANT (1u<<8)
+
 #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */


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

On 26/10/2009 23:12, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> pvcpuid: mask TSC invariant bit for PV domains if migration
> is not disabled and TSC is not emulated
> 
> (Need similar patch for HVM domain)
> 
> Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
This only affects PV domUs. I suggest patching pv_cpuid() to catch dom0, and
then really all domUs should be handled by xc_cpuid_apply_policy(), which
pushes CPUID info down to the hypervisor, as called from xend''s
image.py:setCpuid(). The advantage of this is that the hypervisor does not
need to be informed of the ''disable_migrate'' flag at all -- it
can be
assumed to be true for dom0 -- and it can be handled entirely in the tools.
Hence I can get rid of the new domctl, which is nice.

 -- Keir



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

> > pvcpuid: mask TSC invariant bit for PV domains if migration
> > is not disabled and TSC is not emulated
> > 
> > (Need similar patch for HVM domain)
> > 
> > Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
> 
> This only affects PV domUs. I suggest patching pv_cpuid() to 
> catch dom0, and
> then really all domUs should be handled by 
> xc_cpuid_apply_policy(), which
> pushes CPUID info down to the hypervisor, as called from xend''s
> image.py:setCpuid(). The advantage of this is that the 
> hypervisor does not
> need to be informed of the ''disable_migrate'' flag at all
-- it can be
> assumed to be true for dom0 -- and it can be handled entirely 
> in the tools.
> Hence I can get rid of the new domctl, which is nice.
I''m not done using the disable_migrate flag yet though.
This was just low-hanging fruit.

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

On 27/10/2009 01:09, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
>> This only affects PV domUs. I suggest patching pv_cpuid() to
>> catch dom0, and
>> then really all domUs should be handled by
>> xc_cpuid_apply_policy(), which
>> pushes CPUID info down to the hypervisor, as called from
xend''s
>> image.py:setCpuid(). The advantage of this is that the
>> hypervisor does not
>> need to be informed of the ''disable_migrate'' flag at
all -- it can be
>> assumed to be true for dom0 -- and it can be handled entirely
>> in the tools.
>> Hence I can get rid of the new domctl, which is nice.
> 
> I''m not done using the disable_migrate flag yet though.
> This was just low-hanging fruit.
Okay. Well I nack this new patch anyway.

 -- Keir



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

> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
> On 27/10/2009 01:09, "Dan Magenheimer" 
> <dan.magenheimer@oracle.com> wrote:
> 
> >> This only affects PV domUs. I suggest patching pv_cpuid() to
> >> catch dom0, and
> >> then really all domUs should be handled by
> >> xc_cpuid_apply_policy(), which
> >> pushes CPUID info down to the hypervisor, as called from
xend''s
> >> image.py:setCpuid(). The advantage of this is that the
> >> hypervisor does not
> >> need to be informed of the ''disable_migrate''
flag at all
> -- it can be
> >> assumed to be true for dom0 -- and it can be handled entirely
> >> in the tools.
> >> Hence I can get rid of the new domctl, which is nice.
> > 
> > I''m not done using the disable_migrate flag yet though.
> > This was just low-hanging fruit.
> 
> Okay. Well I nack this new patch anyway.
Frankly, I''m  not very thrilled with the hack-y pvcpuid
requirement anyway.  It seemed a nice architected interface
if cpuid would work, but would be too easy for a well-intentioned
app developer to use cpuid instead of pvcpuid and make
incorrect assumptions that would result in app breakage.

So let me reiterate my objective and perhaps we can come
up with something better:

An app needs to definitively determine programmatically
once when it is starting up if it can always safely use
rdtsc or must fall back to always use an OS API.

It can safely use rdtsc (and rdtsc will be faster than
the OS API) if any of the following are true:

1) tsc_native==0
2) nomigrate==1 AND physical-TSC-Invariant==1 AND
a)   ( tsc_native==1 OR
b)      ( tsc_native==0 BUT Xen-forces-tsc_native-to-1 ) )
3) tsc_native==1 AND nomigrate==0 AND 
a)     ( ( physical-TSC-Invariant==1 AND incarnation==0 ) OR
b)       ( incarnation>0 BUT Xen-forces-tsc_native-to-0 ) )

It can safely use rdtscp by post-applying pvclock scaling if:

4) pvrdtscp==1

It must use the OS API if:

5) tsc_native==1 AND
     ( physical-TSC-Invariant==0 OR
        ( incarnation>0 AND XEN-never-overrides-tsc_native ) )

Choice 3b follows the Intel proposed HVM approach of using
native TSC if no migration has occurred and switching to
emulated TSC after any migration.

Choice 2b is where I was headed with the nomigrate option:
Since TSC is "safe" when physical-TSC-invariant and nomigrate
is set, there''s no need to emulate rdtsc.

I''m beginning to wonder if tsc_native should be a tri-state
with the default being "adaptive":  Xen guarantees rdtsc
is always monotonically-increasing regardless of the physical
machine and regardless of migration but if at all possible
uses native rdtsc for best performance.

In fact, by making it a quad-state, the pvrdtscp option could
be incorporated as well.

tsc_native==0: "adaptive", always correct but performance
                when possible
tsc_native==1: best performance, correctness be d*mned
tsc_native==2: always emulate for correctness, reduced but
                consistent performance
tsc_native==3: best performance, all apps must use rdtscp+pvclock
                to guarantee correctness

I can describe these more completely if this seems like
a promising direction.

Dan

P.S. In case it''s not obvious, "incarnation==0" means
"domain-has-never-migrated" and "incarnation>0" means
"domain-has-migrated-at-least-once".  Also save/restore
is considered a migrate.

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

On 27/10/2009 17:03, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> Frankly, I''m  not very thrilled with the hack-y pvcpuid
> requirement anyway.  It seemed a nice architected interface
> if cpuid would work, but would be too easy for a well-intentioned
> app developer to use cpuid instead of pvcpuid and make
> incorrect assumptions that would result in app breakage.
What''s hacky about it? It''ll probably get used in any
''solution'' in some
way, e.g., to probe for wacky new userland hypercall interfaces. It''s
our
standard interface to probe for features and information direct from the
hypervisor.

 -- Keir



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

> > Frankly, I''m  not very thrilled with the hack-y pvcpuid
> > requirement anyway.  It seemed a nice architected interface
> > if cpuid would work, but would be too easy for a well-intentioned
> > app developer to use cpuid instead of pvcpuid and make
> > incorrect assumptions that would result in app breakage.
> 
> What''s hacky about it? It''ll probably get used in any 
> ''solution'' in some
> way, e.g., to probe for wacky new userland hypercall 
> interfaces. It''s our
> standard interface to probe for features and information 
> direct from the
> hypervisor.
No offense intended.  The hacky part is my attempt for
the same cpuid bit (Invariant TSC) to have slightly
different interpretations depending on whether it is
physical (cpuid) or virtualized (pvcpuid); trying to
overload the definition of a single cpuid bit this way seemed
on closer thought fraught with potential confusion, especially
since it isn''t a complete solution (for testing rdtsc
"safeness") anyway.

On the other hand, short of exposing a larger set
of "userland hypercalls" (whatever their underlying
implementation), I''m not seeing any better
way to provide this information to an app.

So are you suggesting that the best mechanism for
"userland hypercalls" is special reserved cpuid leaves?

Also, any comments on the meat of my last reply?

Thanks,
Dan

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

On 27/10/2009 20:40, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> The hacky part is my attempt for
> the same cpuid bit (Invariant TSC) to have slightly
> different interpretations depending on whether it is
> physical (cpuid) or virtualized (pvcpuid)
How are you overloading it? According to your last patch you would only make
that bit visible to the guest when the TSC is invariant, and will remain
invariant (due to migration being disabled). That''s within native
semantics
of that bit, right?
> So are you suggesting that the best mechanism for
> "userland hypercalls" is special reserved cpuid leaves?
If you want to provide ''feature flags'' and other such info to
the guest, it
seems a pretty reasonable approach, since it already exists and that''s
what
it''s intended for. There may be other things you want to do for which
it is
not an appropriate solution, but everything so far seems handleable via it.
> Also, any comments on the meat of my last reply?
Looked pretty complicated to me. Can''t say as I fully understand it.

 -- Keir



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

> On 27/10/2009 20:40, "Dan Magenheimer" 
> <dan.magenheimer@oracle.com> wrote:
> 
> > The hacky part is my attempt for
> > the same cpuid bit (Invariant TSC) to have slightly
> > different interpretations depending on whether it is
> > physical (cpuid) or virtualized (pvcpuid)
> 
> How are you overloading it? According to your last patch you 
> would only make
> that bit visible to the guest when the TSC is invariant, and 
> will remain
> invariant (due to migration being disabled). That''s within 
> native semantics of that bit, right?
Yes, the semantics are intended to be the same.  As long
as migration is disabled, it should work as intended.
I guess I was thinking into the next step of getting it
to work properly across certain "safe" migrations and
worrying that the pvcpuid-Invariant-TSC could change
dynamically whereas a physical cpuid-Invariant-TSC bit
would never change.

So after nack''ing you seem to be trying to convince me
that the patch is fine.  What was your concern?
> > So are you suggesting that the best mechanism for
> > "userland hypercalls" is special reserved cpuid leaves?
> 
> If you want to provide ''feature flags'' and other such
info to
> the guest, it
> seems a pretty reasonable approach, since it already exists 
> and that''s what
> it''s intended for. There may be other things you want to do 
> for which it is
> not an appropriate solution, but everything so far seems 
> handleable via it.
Would you consider it a good solution for returning slightly
larger pieces of information, more than one bit but small
enough to fit in eax+ebx+ecx+edx?
> > Also, any comments on the meat of my last reply?
> 
> Looked pretty complicated to me. Can''t say as I fully understand
it.
Yes, I wish it could be simpler.  It comes down to four
possible choices.  The first two are "always emulate
rdtsc" and "never emulate rdtsc" and these are already
implemented but nobody is very happy with either so
I''m looking for in-between solutions.  So I''m proposing
two more:

A) Xen is responsible for correctness but will provide
   native rdtsc performance whenever feasible; and
B) Xen is NOT responsible for correctness, rdtsc will
   NEVER be emulated, but Xen will provide sufficient
   information (via rdtscp and pvclock info) to ensure 
   an app can always synthesize a correct timestamp,
   even across migration

(A) would be a compromise I would propose for the default
for all VMs and (B) would be for intelligent apps that need
timestamps at a super-high-frequency and are willing
to change code and control their VM environment to
maximize performance.

Does that part make sense and sound reasonable?
Everything else is mechanism -- plus communication of
relevant data from Xen to an app -- to support these choices.

Dan

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

On 27/10/2009 22:03, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> So after nack''ing you seem to be trying to convince me
> that the patch is fine.  What was your concern?
Your patch only affected PV domUs; not dom0 nor HVM domUs. Further it was
probably implemented in the wrong place -- this policy ought to be
expressible in xc_cpuid_x86.c. This has the advantage it can be overridden
in domain config files, just like any other CPUID bit. Only dom0 policy has
to be hardcoded in the hypervisor itself.
> Would you consider it a good solution for returning slightly
> larger pieces of information, more than one bit but small
> enough to fit in eax+ebx+ecx+edx?
Very likely, yes.
> A) Xen is responsible for correctness but will provide
>    native rdtsc performance whenever feasible; and
If you can do this correctly, why would anyone use the always-emulate mode?
> B) Xen is NOT responsible for correctness, rdtsc will
>    NEVER be emulated, but Xen will provide sufficient
>    information (via rdtscp and pvclock info) to ensure
>    an app can always synthesize a correct timestamp,
>    even across migration
This seems like an extension which could be applicable to any TSC mode,
rather than a new mode in its own right.

 -- Keir




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

> On 27/10/2009 22:03, "Dan Magenheimer" 
> <dan.magenheimer@oracle.com> wrote:
> 
> > So after nack''ing you seem to be trying to convince me
> > that the patch is fine.  What was your concern?
> 
> Your patch only affected PV domUs; not dom0 nor HVM domUs. 
> Further it was
> probably implemented in the wrong place -- this policy ought to be
> expressible in xc_cpuid_x86.c. This has the advantage it can 
> be overridden
> in domain config files, just like any other CPUID bit. Only 
> dom0 policy has
> to be hardcoded in the hypervisor itself.
Oops, I''m sorry, I had a half-formed reply to this and
never finished/sent it:

The patch is in domain_cpuid, which gets called from
hvm_cpuid so shouldn''t it affect HVMs also?  I hadn''t thought
about dom0, but since dom0 never migrates, the result
should be the same (i.e. never a reason to override
the physical TSC invariant bit).

As for doing things in xc_cpuid_x86.c, I guess I don''t
see this at all as a policy decision, only as mechanism
driven by other policy decisions (nomigrate and tsc_native).
Teaching all toolsets the subtleties of how to guarantee
correctness for a legal processor instruction doesn''t
seem like a proper split between policy and mechanism
to me.

But, that said, I''m still not convinced my patch is the
right answer.  If possible (e.g. using the CPUID masking
feature Jan referred to), Xen should always hide the TSC
Invariant bit from all guests (OS''s and apps) and only
expose it via pvcpuid or some other mechanism.
Otherwise an OS or app may make false assumptions
based on an architecturally-specified cpuid bit.
 
> > Would you consider it a good solution for returning slightly
> > larger pieces of information, more than one bit but small
> > enough to fit in eax+ebx+ecx+edx?
> 
> Very likely, yes.
OK, I''ll think about using pvcpuid instead of special MSRs.
> > A) Xen is responsible for correctness but will provide
> >    native rdtsc performance whenever feasible; and
> 
> If you can do this correctly, why would anyone use the 
> always-emulate mode?
I agree always-emulate becomes a degenerate case that
probably would rarely get used, but it might get used,
for example, to debug timing issues or test worst-case
(or post-migration) performance for high-frequency-rdtsc
guests/apps.
> > B) Xen is NOT responsible for correctness, rdtsc will
> >    NEVER be emulated, but Xen will provide sufficient
> >    information (via rdtscp and pvclock info) to ensure
> >    an app can always synthesize a correct timestamp,
> >    even across migration
> 
> This seems like an extension which could be applicable to any 
> TSC mode,
> rather than a new mode in its own right.
Conceptually, you are right.  But rather than add yet
another vm.cfg parameter and domctl, I''m thinking this
simpler modal approach to reduce complexity.

In fact, never-emulate is a degenerate case of pvrdtsc,
but for similar reasons I think a never-emulate option
should be kept around.

So the direction I''m heading is, instead of the three vm.cfg
parameters -- tsc_native, nomigrate, pvrdtsc -- there would
be a single quad-state parameter, tsc_mode:

0 = (default) Xen guarantees rdtsc correctness but uses
    emulation only when necessary
1 = rdtsc is always native
2 = rdtsc is always emulated
3 = (a) IF processor supports rdtscp instruction:
       rdtsc/p is native, Xen provides TSC_AUX and pvclock info
    (b) IF processor doesn''t support rdtscp instruction:
       rdtsc/p is emulated (with degenerate TSC_AUX/pvclock info)

Thanks,
Dan


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

>>> Dan Magenheimer <dan.magenheimer@oracle.com> 28.10.09 17:27
>>>
>But, that said, I''m still not convinced my patch is the
>right answer.  If possible (e.g. using the CPUID masking
>feature Jan referred to), Xen should always hide the TSC
Actually I just realized that this feature would probably not help you
at all: The bit you''re after is in leaf 0x80000007, but the masking
applies
only to leaf 1 (plus 0x80000001 for AMD).
>Invariant bit from all guests (OS''s and apps) and only
>expose it via pvcpuid or some other mechanism.
>Otherwise an OS or app may make false assumptions
>based on an architecturally-specified cpuid bit.
 
Jan


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

Hmmm... this could be a real problem for some apps!

Is this only a problem for PV domains?  I.e, is cpuid
always trapped by Xen for HVM domains?
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@novell.com]
> Sent: Wednesday, October 28, 2009 10:39 AM
> To: Keir Fraser; Xen-Devel (E-mail); Dan Magenheimer
> Subject: RE: [Xen-devel] [PATCH] pvcpuid: mask TSC invariant bit for
> various circumstances 
> 
> >>> Dan Magenheimer <dan.magenheimer@oracle.com> 28.10.09
17:27 >>>
> >But, that said, I''m still not convinced my patch is the
> >right answer.  If possible (e.g. using the CPUID masking
> >feature Jan referred to), Xen should always hide the TSC
> 
> Actually I just realized that this feature would probably not help you
> at all: The bit you''re after is in leaf 0x80000007, but the 
> masking applies
> only to leaf 1 (plus 0x80000001 for AMD).
> 
> >Invariant bit from all guests (OS''s and apps) and only
> >expose it via pvcpuid or some other mechanism.
> >Otherwise an OS or app may make false assumptions
> >based on an architecturally-specified cpuid bit.
>  
> Jan
> 
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Yes, CPUID is always trapped for HVM guests.

 -- Keir

On 28/10/2009 16:50, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> Hmmm... this could be a real problem for some apps!
> 
> Is this only a problem for PV domains?  I.e, is cpuid
> always trapped by Xen for HVM domains?
> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@novell.com]
>> Sent: Wednesday, October 28, 2009 10:39 AM
>> To: Keir Fraser; Xen-Devel (E-mail); Dan Magenheimer
>> Subject: RE: [Xen-devel] [PATCH] pvcpuid: mask TSC invariant bit for
>> various circumstances
>> 
>>>>> Dan Magenheimer <dan.magenheimer@oracle.com> 28.10.09
17:27 >>>
>>> But, that said, I''m still not convinced my patch is the
>>> right answer.  If possible (e.g. using the CPUID masking
>>> feature Jan referred to), Xen should always hide the TSC
>> 
>> Actually I just realized that this feature would probably not help you
>> at all: The bit you''re after is in leaf 0x80000007, but the
>> masking applies
>> only to leaf 1 (plus 0x80000001 for AMD).
>> 
>>> Invariant bit from all guests (OS''s and apps) and only
>>> expose it via pvcpuid or some other mechanism.
>>> Otherwise an OS or app may make false assumptions
>>> based on an architecturally-specified cpuid bit.
>>  
>> Jan
>> 
>> 


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

On 10/28/09 09:27, Dan Magenheimer wrote:
> 3 = (a) IF processor supports rdtscp instruction:
>        rdtsc/p is native, Xen provides TSC_AUX and pvclock info
>     (b) IF processor doesn''t support rdtscp instruction:
>        rdtsc/p is emulated (with degenerate TSC_AUX/pvclock info)
>   
How will this interact with operating systems which also want to use
TSC_AUX when rdtscp is available?

    J

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

> From: Jeremy Fitzhardinge [mailto:jeremy@goop.org]
> Sent: Thursday, October 29, 2009 4:07 PM
> To: Dan Magenheimer
> Cc: Keir Fraser; Xen-Devel (E-mail)
> Subject: Re: [Xen-devel] [PATCH] pvcpuid: mask TSC invariant bit for
> various circumstances
> 
> On 10/28/09 09:27, Dan Magenheimer wrote:
> > 3 = (a) IF processor supports rdtscp instruction:
> >        rdtsc/p is native, Xen provides TSC_AUX and pvclock info
> >     (b) IF processor doesn''t support rdtscp instruction:
> >        rdtsc/p is emulated (with degenerate TSC_AUX/pvclock info)
> >   
> 
> How will this interact with operating systems which also want to use
> TSC_AUX when rdtscp is available?
Thanks for the review!

Xen does not currently expose rdtscp through pvcpuid, even when
the cpuid rdtscp bit is present.  If that should change, this
option would have to "disable" it for pvcpuid (to a PV OS)
for that guest.

Dan

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