Re: [Xen-devel] [PATCH v2 1/2] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-09-09 at 13:29 +0100, Andrew Cooper wrote:
> On 09/09/14 13:21, Ian Campbell wrote:
> > On Tue, 2014-09-09 at 11:45 +0100, Jan Beulich wrote:
> >>>>> On 09.09.14 at 06:31, <alfred.z.song@xxxxxxxxx> wrote:
> >>> @@ -195,16 +186,14 @@ static void intel_xc_cpuid_policy(
> >>>          break;
> >>>  
> >>>      case 0x80000001: {
> >>> -        int is_64bit = hypervisor_is_64bit(xch) && is_pae;
> >>> -
> >>>          /* Only a few features are advertised in Intel's 0x80000001. */
> >>> -        regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) |
> >>> -                               bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
> >>> -                               bitmaskof(X86_FEATURE_ABM);
> >>> -        regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
> >>> -                    (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
> >>> -                    (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
> >>> -                    (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0));
> >>> +        regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) |
> >>> +                    bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
> >>> +                    bitmaskof(X86_FEATURE_ABM);
> >>> +        regs[3] &= (bitmaskof(X86_FEATURE_NX) |
> >>> +                    bitmaskof(X86_FEATURE_LM) |
> >>> +                    (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
> >>> +                    (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0));
> >> As said before, tying these two features to is_pae seems a
> >> little strange, but if the tools maintainers can live with that, I
> >> guess I can too (short of having a better suggestion other
> >> than to drop the conditionals altogether).
> > Patch #2 here seems to remove it from the RDTSCP, surely that should be
> > folded in.
> >
> > I also don't understand the link between PAE and the presence of
> > SYSCALL.
> 
> On Intel, syscall is strictly only available in long mode, being an AMD
> instruction mandated in the 64bit spec.
> 
> is_64bit is disappearing as Xen is unconditionally 64bit these days, but
> preventing the guest using PAE will preclude it being able to enter long
> mode.
> 
> I would agree that it is not necessarily obvious, and based on this
> consideration, I think it would be better to keep the variable
> "is_64bit" as it is more informative than "is_pae" in the contexts used.

But right above we are advertising X86_FEATURE_LM unconditionally, so
what is to stop the guest switching to long mode and therefore using
syscall?

Does real h/w change the cpuid features reported depending on the
current processor mode? 

One other bit of confusion I'm having is whether is_pae refers to the
guest or the host. Previously is_64bit seemed to be a hybrid of both...
Ian.


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