Re: [PATCH RFC 02/31] tools/libxc: Use public/featureset.h for cpuid policy generation

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2016-01-05 at 15:42 +0000, Andrew Cooper wrote:
> On 05/01/16 15:02, Ian Campbell wrote:
> > On Tue, 2016-01-05 at 14:23 +0000, Andrew Cooper wrote:
> > > On 05/01/16 14:18, Ian Campbell wrote:
> > > > On Tue, 2016-01-05 at 14:17 +0000, Andrew Cooper wrote:
> > > > > On 05/01/16 14:13, Ian Campbell wrote:
> > > > > > On Tue, 2015-12-22 at 09:29 -0700, Jan Beulich wrote:
> > > > > > > > > > On 16.12.15 at 22:24, <andrew.cooper3@citrix.com>
> > > > > > > > > > wrote:
> > > > > > > > --- a/xen/include/public/arch-x86/featureset.h
> > > > > > > > +++ b/xen/include/public/arch-x86/featureset.h
> > > > > > > > @@ -163,6 +163,7 @@
> > > > > > > >  
> > > > > > > >  /* Intel-defined CPU features, CPUID level
> > > > > > > > 0x00000007:0.ebx,
> > > > > > > > word
> > > > > > > > 5 */
> > > > > > > >  #define X86_FEATURE_FSGSBASE      ( 5*32+ 0) /*
> > > > > > > > {RD,WR}{FS,GS}BASE
> > > > > > > > instructions */
> > > > > > > > +#define X86_FEATURE_TSC_ADJUST    ( 5*32+ 1) /* TSC_ADJUST
> > > > > > > > MSR
> > > > > > > > available */
> > > > > > > This would probably better go into patch 1.
> > > > > > Tools would then see this defined twice with only patch 1
> > > > > > applied,
> > > > > > and
> > > > > > since the value is actually different I think the compiler will
> > > > > > complain.
> > > > > > 
> > > > > > -#define X86_FEATURE_TSC_ADJUST   1 /* Tsc thread offset */
> > > > > > +#define X86_FEATURE_TSC_ADJUST    ( 5*32+ 1) /* TSC_ADJUST MSR
> > > > > > available */
> > > > > > 
> > > > > > The comment change seems to be a semantic one? Or was it wrong
> > > > > > beofre?
> > > > > Changing patch 1 won't affect the compilation of libxc.  Observe
> > > > > in
> > > > > the
> > > > > penultimate hunk that I also change the #include
> > > > Ah yes.
> > > > 
> > > > What about my comment on the comment changing?
> > > Ah yes - the changes are just semantic.  Also observe that the same
> > > hunk
> > > also modifies the libxc macros to modulo 32.
> > I'm not talking about the value at all, since I realised that the
> > masking
> > makes the values the same. I'm talking about the /* comment */ next to
> > it
> > which has also changed.
> > 
> > > In the end, both the hypervisor and libxc are dealing with
> > > hardware-specified bits in registers.  This patch is no resulting
> > > change
> > > to behaviour.
> > But do "Tsc thread offset" and "TSC_ADJUST MSR available" mean the same
> > thing? Or was "Tsc thread offset" previously actively wrong or
> > misleading
> > in some way?
> 
> The latter is the accurate meaning of the bit in cpuid.  The former is
> misleading IMO, and covers one use for it in a Linux world.

Thanks, I just wanted to be sure it was an intentional change and not e.g.
a cut-and-paste mistake.

Ian.

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