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

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

[Zhuo Song]

* Since there would not be 32-bit hypervisor, we do not need
  hypervisor_is_64bit() again.

* Remove xen_64bit from xc_cpuid_pv_policy().

* Because is_64bit only depends on is_pae, only use is_pae for both
  vendor specific functions.

* Move conditions for LM/NX masking into architectural logic

Signed-off-by: Zhuo Song <songzhuo.sz@alibaba-inc.com>
---
 tools/libxc/xc_cpuid_x86.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 6b81641..710fd61 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -34,13 +34,6 @@
 #define DEF_MAX_INTELEXT  0x80000008u
 #define DEF_MAX_AMDEXT    0x8000001cu
 
-static int hypervisor_is_64bit(xc_interface *xch)
-{
-    xen_capabilities_info_t xen_caps = "";
-    return ((xc_version(xch, XENVER_capabilities, &xen_caps) == 0) &&
-            (strstr(xen_caps, "x86_64") != NULL));
-}
-
 static void cpuid(const unsigned int *input, unsigned int *regs)
 {
     unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
@@ -95,13 +88,11 @@ static void amd_xc_cpuid_policy(
         break;
 
     case 0x80000001: {
-        int is_64bit = hypervisor_is_64bit(xch) && is_pae;
-
         if ( !is_pae )
             clear_bit(X86_FEATURE_PAE, regs[3]);
 
         /* Filter all other features according to a whitelist. */
-        regs[2] &= ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) |
+        regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) |
                     bitmaskof(X86_FEATURE_CMP_LEGACY) |
                     (is_nestedhvm ? bitmaskof(X86_FEATURE_SVM) : 0) |
                     bitmaskof(X86_FEATURE_CR8_LEGACY) |
@@ -116,8 +107,8 @@ static void amd_xc_cpuid_policy(
                     bitmaskof(X86_FEATURE_TBM) |
                     bitmaskof(X86_FEATURE_DBEXT));
         regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */
-                    (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
-                    (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
+                    bitmaskof(X86_FEATURE_NX) |
+                    bitmaskof(X86_FEATURE_LM) |
                     bitmaskof(X86_FEATURE_SYSCALL) |
                     bitmaskof(X86_FEATURE_MP) |
                     bitmaskof(X86_FEATURE_MMXEXT) |
@@ -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));
         break;
     }
 
@@ -392,6 +381,8 @@ static void xc_cpuid_hvm_policy(
 
     case 0x80000001:
         if ( !is_pae ) {
+            clear_bit(X86_FEATURE_LAHF_LM, regs[2]);
+            clear_bit(X86_FEATURE_LM, regs[3]);
             clear_bit(X86_FEATURE_NX, regs[3]);
             clear_bit(X86_FEATURE_PSE36, regs[3]);
         }
@@ -442,7 +433,7 @@ static void xc_cpuid_pv_policy(
 {
     DECLARE_DOMCTL;
     unsigned int guest_width;
-    int guest_64bit, xen_64bit = hypervisor_is_64bit(xch);
+    int guest_64bit;
     char brand[13];
     uint64_t xfeature_mask;
 
@@ -474,7 +465,7 @@ static void xc_cpuid_pv_policy(
     switch ( input[0] )
     {
     case 0x00000001:
-        if ( !xen_64bit || strstr(brand, "AMD") )
+        if ( strstr(brand, "AMD") )
             clear_bit(X86_FEATURE_SEP, regs[3]);
         clear_bit(X86_FEATURE_DS, regs[3]);
         clear_bit(X86_FEATURE_ACC, regs[3]);
-- 
1.8.5.2 (Apple Git-48)

[PATCH v2 2/2] xc_cpuid_x86.c: Remove limit for RDTSCP

[Zhuo Song]

Since RDTSCP could be used for both 64-bit and 32-bit architectures,
we do not need the tying to 64-bit in intel_xc_cpuid_policy().

Signed-off-by: Zhuo Song <songzhuo.sz@alibaba-inc.com>
---
 tools/libxc/xc_cpuid_x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 710fd61..e7b50b1 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -193,7 +193,7 @@ static void intel_xc_cpuid_policy(
         regs[3] &= (bitmaskof(X86_FEATURE_NX) |
                     bitmaskof(X86_FEATURE_LM) |
                     (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
-                    (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0));
+                    (bitmaskof(X86_FEATURE_RDTSCP));
         break;
     }
 
-- 
1.8.5.2 (Apple Git-48)

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

[Jan Beulich]

>>> On 09.09.14 at 06:31, <alfred.z.song@gmail.com> 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).

Jan

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

[Ian Campbell]

On Tue, 2014-09-09 at 11:45 +0100, Jan Beulich wrote:
> >>> On 09.09.14 at 06:31, <alfred.z.song@gmail.com> 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.

Ian.

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

[Andrew Cooper]

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@gmail.com> 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.

~Andrew

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

[Ian Campbell]

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@gmail.com> 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.

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

[Andrew Cooper]

On 09/09/14 13:43, Ian Campbell wrote:
> 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@gmail.com> 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?

I made the same mistake.  That is setting FEATURE_LM in an AND mask,
where the underlying bit may or may not be set from the common logic. 
It is certainly not obvious.

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

It is not the current processor mode.  All of this logic is run during
domain create, before hvmloader starts.

>
> 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.
>

It kind of was.  It was a hybrid of "Xen is 64bit" (i.e. will permit the
guest to move into long mode if it tries), and "we have advertised pae
to the guest" which is a prerequisite to enter long mode.

All this code is nasty.

~Andrew

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

[z]

[-- Attachment #1.1: Type: text/plain, Size: 3763 bytes --]

On Tue, Sep 9, 2014 at 8:49 PM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 09/09/14 13:43, Ian Campbell wrote:
> > 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@gmail.com> 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?
>
> I made the same mistake.  That is setting FEATURE_LM in an AND mask,
> where the underlying bit may or may not be set from the common logic.
> It is certainly not obvious.
>
> >
> > Does real h/w change the cpuid features reported depending on the
> > current processor mode?
>
> It is not the current processor mode.  All of this logic is run during
> domain create, before hvmloader starts.
>
> >
> > 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.
> >
>
> It kind of was.  It was a hybrid of "Xen is 64bit" (i.e. will permit the
> guest to move into long mode if it tries), and "we have advertised pae
> to the guest" which is a prerequisite to enter long mode.
>

So I think is_pae is unique condition for a vm trying to enter long mode,
since Xen is always 64-bit now ( also removed hypervisor_is_64bit ).
Although is_pae seems not that obvious.

Zhuo


>
> All this code is nasty.
>
> ~Andrew
>
>

[-- Attachment #1.2: Type: text/html, Size: 5483 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 09.09.14 at 14:29, <andrew.cooper3@citrix.com> wrote:
> 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.

Or drop the conditional, as suggested before. After all I don't think
Intel CPUs advertise the bit in CPUID only when in long mode. Plus
even if they did, what we do (did) here still wouldn't match their
behavior (since is_64bit really is a hypervisor property, not a guest
one). I.e. at the tools level we should leave the flag as is. And if
indeed CPUID output changes when entering long mode (didn't
check the spec), this could only be mimicked in hypervisor code
anyway.

Jan

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

[z]

[-- Attachment #1.1: Type: text/plain, Size: 1985 bytes --]

On Tue, Sep 9, 2014 at 9:23 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 09.09.14 at 14:29, <andrew.cooper3@citrix.com> wrote:
> > 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.
>
> Or drop the conditional, as suggested before. After all I don't think
> Intel CPUs advertise the bit in CPUID only when in long mode. Plus
> even if they did, what we do (did) here still wouldn't match their
> behavior (since is_64bit really is a hypervisor property, not a guest
> one). I.e. at the tools level we should leave the flag as is. And if
> indeed CPUID output changes when entering long mode (didn't
> check the spec), this could only be mimicked in hypervisor code
> anyway.
>
> Jan
>
>
Hi, Jan

Generally, I agree with you.
Another implicit reason is Xen is always 64-bit now, which also means that
the Intel CPU (if not AMD) on which the hypervisor is running should be
64-bit.
The SYSCALL could be reported to guest as long as the CPU supports it and
it would be emulated once guest run its own CPUID, and is_pae could even be
dropped to some extent. But from performance point of view, there might be
more work in the handler of vmexit (e.g. adding more bits to be emulated).

And I agree with you is_pae does not mean the guest must be long mode,
instead, it is long mode _or_ pae mode.  In pae mode, SYSCALL should not be
exported to guest either. So the conditional is_pae for SYSCALL is not that
accurate (as you said it is partial effective). Maybe we need a better way
here. But I can not figure it out yet.

Zhuo

[-- Attachment #1.2: Type: text/html, Size: 2819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 09.09.14 at 16:43, <alfred.z.song@gmail.com> wrote:
> And I agree with you is_pae does not mean the guest must be long mode,
> instead, it is long mode _or_ pae mode.

Not even that - is_pae only allows the guest to enter PAE mode,
it doesn't require it to.

Jan

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

[z]

[-- Attachment #1.1: Type: text/plain, Size: 719 bytes --]

On Tue, Sep 9, 2014 at 10:49 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 09.09.14 at 16:43, <alfred.z.song@gmail.com> wrote:
> > And I agree with you is_pae does not mean the guest must be long mode,
> > instead, it is long mode _or_ pae mode.
>
> Not even that - is_pae only allows the guest to enter PAE mode,
> it doesn't require it to.
>
> Jan
>
>
Sorry. Yep. If so, is_pae is not proper here for SYSCALL which only
requires long mode for Intel. There might be two ways (one of them):

1. make a new flag (not is_64bit, since it is "a property of hypervisor")
to indicate the guest is long mode (not pae) and keep the work at tools
level
2. simply leave the work to vmexit handler if guest run CPUID

Zhuo

[-- Attachment #1.2: Type: text/html, Size: 1326 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[z]

[-- Attachment #1.1: Type: text/plain, Size: 1003 bytes --]

On Tue, Sep 9, 2014 at 11:13 PM, z <alfred.z.song@gmail.com> wrote:

>
>
> On Tue, Sep 9, 2014 at 10:49 PM, Jan Beulich <JBeulich@suse.com> wrote:
>
>> >>> On 09.09.14 at 16:43, <alfred.z.song@gmail.com> wrote:
>> > And I agree with you is_pae does not mean the guest must be long mode,
>> > instead, it is long mode _or_ pae mode.
>>
>> Not even that - is_pae only allows the guest to enter PAE mode,
>> it doesn't require it to.
>>
>> Jan
>>
>>
> Sorry. Yep. If so, is_pae is not proper here for SYSCALL which only
> requires long mode for Intel. There might be two ways (one of them):
>
> 1. make a new flag (not is_64bit, since it is "a property of hypervisor")
> to indicate the guest is long mode (not pae) and keep the work at tools
> level
> 2. simply leave the work to vmexit handler if guest run CPUID
>
> Zhuo
>
>
>

Hi, Jan

It seems that vmx_cpuid_intercept() has covered SYSCALL masking when
vmexit. So we do not need is_pae here again and it could be dropped from my
point of view.

Zhuo

[-- Attachment #1.2: Type: text/html, Size: 2100 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 10.09.14 at 04:06, <alfred.z.song@gmail.com> wrote:
> It seems that vmx_cpuid_intercept() has covered SYSCALL masking when
> vmexit. So we do not need is_pae here again and it could be dropped from my
> point of view.

Good - looking forward to see an updated patch then.

Jan

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

[z]

[-- Attachment #1.1: Type: text/plain, Size: 2081 bytes --]

On Tue, Sep 9, 2014 at 8:21 PM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:

> On Tue, 2014-09-09 at 11:45 +0100, Jan Beulich wrote:
> > >>> On 09.09.14 at 06:31, <alfred.z.song@gmail.com> 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.
>
> Ian.
>

Hi, Ian & Jan

It seems that SYSCALL is valid only in 64-bit mode in Intel's
implementation, but for AMD, it is available in any mode (64/32).
So it should be vendor-dependent. That's why I keep Is_pae here for Intel.

Zhuo

[-- Attachment #1.2: Type: text/html, Size: 3084 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 09.09.14 at 14:21, <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-09-09 at 11:45 +0100, Jan Beulich wrote:
>> >>> On 09.09.14 at 06:31, <alfred.z.song@gmail.com> 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.

It's the result of ditching is_64bit: Intel supports syscall only in
64-bit mode. I.e. I suppose this is an (only partially successful)
attempt to not misguide OSes that can't ever run in 64-bit mode.

Jan