Two miscellenous xen-detect patches.

Two miscellenous xen-detect patches.

[John Haxby]

I needed xen-detect this morning so as usual I pulled it from git and it
didn't work and it threw a compilation warning.   Thinking the problem
was the strict-aliasing warning (patch 1) I fixed that and ... it still
didn't work.  Well, that's not entirely true, it was just
over-enthusiastically claiming everything except dom0 was hvm.  Patch 2
reverses the order of the pv and hvm checks which works on everything I've
tried it with.

jch

[PATCH 1/2] xen-detect: fix strict-aliasing compilation warning.

[John Haxby]

With gcc 4.8.3, compiling xen-detect gives a compilation warning if
you're optimising:

$ cc -Wall -Os xen-detect.c
xen-detect.c: In function ‘check_for_xen’:
xen-detect.c:65:9: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
         *(uint32_t *)(signature + 0) = regs[1];
         ^

Signed-off-by: John Haxby <john.haxby@oracle.com>
---
 tools/misc/xen-detect.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/misc/xen-detect.c b/tools/misc/xen-detect.c
index 787b5da..19c66d1 100644
--- a/tools/misc/xen-detect.c
+++ b/tools/misc/xen-detect.c
@@ -54,28 +54,27 @@ static void cpuid(uint32_t idx, uint32_t *regs, int pv_context)
 
 static int check_for_xen(int pv_context)
 {
-    uint32_t regs[4];
-    char signature[13];
+    union
+    {
+        uint32_t regs[4];
+        char signature[17];
+    } u;
     uint32_t base;
 
     for ( base = 0x40000000; base < 0x40010000; base += 0x100 )
     {
-        cpuid(base, regs, pv_context);
-
-        *(uint32_t *)(signature + 0) = regs[1];
-        *(uint32_t *)(signature + 4) = regs[2];
-        *(uint32_t *)(signature + 8) = regs[3];
-        signature[12] = '\0';
+        cpuid(base, u.regs, pv_context);
+        u.signature[16] = '\0';
 
-        if ( !strcmp("XenVMMXenVMM", signature) && (regs[0] >= (base + 2)) )
+        if ( !strcmp("XenVMMXenVMM", u.signature+4) && (u.regs[0] >= (base + 2)) )
             goto found;
     }
 
     return 0;
 
  found:
-    cpuid(base + 1, regs, pv_context);
-    return regs[0];
+    cpuid(base + 1, u.regs, pv_context);
+    return u.regs[0];
 }
 
 static jmp_buf sigill_jmp;
-- 
1.9.3


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

[PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM

[John Haxby]

At some stage, the cpuid instruction used to detect a xen hvm domain
also started working in a pv domain so pv domains were being identified
as hvm (dom0 excepted).  Change the order so that pv is tested for
first.

Signed-off-by: John Haxby <john.haxby@oracle.com>
---
 tools/misc/xen-detect.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/misc/xen-detect.c b/tools/misc/xen-detect.c
index 19c66d1..c8fccf9 100644
--- a/tools/misc/xen-detect.c
+++ b/tools/misc/xen-detect.c
@@ -132,15 +132,10 @@ int main(int argc, char **argv)
         }
     }
 
-    /* Check for execution in HVM context. */
-    detected = XEN_HVM;
-    if ( (version = check_for_xen(0)) != 0 )
-        goto out;
-
     /*
      * Set up a signal handler to test the paravirtualised CPUID instruction.
      * If executed outside Xen PV context, the extended opcode will fault, we
-     * will longjmp via the signal handler, and print "Not running on Xen".
+     * will longjmp via the signal handler, then check for HVM.
      */
     detected = XEN_PV;
     if ( !setjmp(sigill_jmp)
@@ -148,6 +143,11 @@ int main(int argc, char **argv)
          && ((version = check_for_xen(1)) != 0) )
         goto out;
 
+    /* Check for execution in HVM context. */
+    detected = XEN_HVM;
+    if ( (version = check_for_xen(0)) != 0 )
+        goto out;
+
     detected = XEN_NONE;
 
  out:
-- 
1.9.3

Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning.

[Andrew Cooper]

On 01/12/14 14:37, John Haxby wrote:
> With gcc 4.8.3, compiling xen-detect gives a compilation warning if
> you're optimising:
>
> $ cc -Wall -Os xen-detect.c
> xen-detect.c: In function ‘check_for_xen’:
> xen-detect.c:65:9: warning: dereferencing type-punned pointer will break
> strict-aliasing rules [-Wstrict-aliasing]
>          *(uint32_t *)(signature + 0) = regs[1];
>          ^
>
> Signed-off-by: John Haxby <john.haxby@oracle.com>

Why are you compiling without the CFLAGS from the Xen build system?

We explicitly disable strict alias optimisations, because optimisations
based upon the aliasing rules in C is mad.  Even when you eliminate all
the warnings, there are still subtle bugs because the compiler is free
to assume a lot more than a programmer would typically deem reasonable.

~Andrew

> ---
>  tools/misc/xen-detect.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/tools/misc/xen-detect.c b/tools/misc/xen-detect.c
> index 787b5da..19c66d1 100644
> --- a/tools/misc/xen-detect.c
> +++ b/tools/misc/xen-detect.c
> @@ -54,28 +54,27 @@ static void cpuid(uint32_t idx, uint32_t *regs, int pv_context)
>  
>  static int check_for_xen(int pv_context)
>  {
> -    uint32_t regs[4];
> -    char signature[13];
> +    union
> +    {
> +        uint32_t regs[4];
> +        char signature[17];
> +    } u;
>      uint32_t base;
>  
>      for ( base = 0x40000000; base < 0x40010000; base += 0x100 )
>      {
> -        cpuid(base, regs, pv_context);
> -
> -        *(uint32_t *)(signature + 0) = regs[1];
> -        *(uint32_t *)(signature + 4) = regs[2];
> -        *(uint32_t *)(signature + 8) = regs[3];
> -        signature[12] = '\0';
> +        cpuid(base, u.regs, pv_context);
> +        u.signature[16] = '\0';
>  
> -        if ( !strcmp("XenVMMXenVMM", signature) && (regs[0] >= (base + 2)) )
> +        if ( !strcmp("XenVMMXenVMM", u.signature+4) && (u.regs[0] >= (base + 2)) )
>              goto found;
>      }
>  
>      return 0;
>  
>   found:
> -    cpuid(base + 1, regs, pv_context);
> -    return regs[0];
> +    cpuid(base + 1, u.regs, pv_context);
> +    return u.regs[0];
>  }
>  
>  static jmp_buf sigill_jmp;



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

Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning.

[John Haxby]

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


> On 1 Dec 2014, at 17:15, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 01/12/14 14:37, John Haxby wrote:
>> With gcc 4.8.3, compiling xen-detect gives a compilation warning if
>> you're optimising:
>> 
>> $ cc -Wall -Os xen-detect.c
>> xen-detect.c: In function ‘check_for_xen’:
>> xen-detect.c:65:9: warning: dereferencing type-punned pointer will break
>> strict-aliasing rules [-Wstrict-aliasing]
>>         *(uint32_t *)(signature + 0) = regs[1];
>>         ^
>> 
>> Signed-off-by: John Haxby <john.haxby@oracle.com <mailto:john.haxby@oracle.com>>
> 
> Why are you compiling without the CFLAGS from the Xen build system?
> 
> We explicitly disable strict alias optimisations, because optimisations
> based upon the aliasing rules in C is mad.  Even when you eliminate all
> the warnings, there are still subtle bugs because the compiler is free
> to assume a lot more than a programmer would typically deem reasonable.


I wasn’t building the whole system, I just wanted xen-detect so I pulled it out and compiled it; I usually use "-Wall -Os” because the combination finds problems I might otherwise overlook.   The patch also removes three lines of code :) but you can take it or leave it as you choose.   The other patch — reversing the code of pv and hvm checking — was the real problem.

jch

[-- Attachment #1.2: Type: text/html, Size: 7060 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 1/2] xen-detect: fix strict-aliasing compilation warning.

[John Haxby]

On 01/12/14 17:15, Andrew Cooper wrote:
> On 01/12/14 14:37, John Haxby wrote:
>> With gcc 4.8.3, compiling xen-detect gives a compilation warning if
>> you're optimising:
>>
>> $ cc -Wall -Os xen-detect.c
>> xen-detect.c: In function ‘check_for_xen’:
>> xen-detect.c:65:9: warning: dereferencing type-punned pointer will break
>> strict-aliasing rules [-Wstrict-aliasing]
>>          *(uint32_t *)(signature + 0) = regs[1];
>>          ^
>>
>> Signed-off-by: John Haxby <john.haxby@oracle.com>
> 
> Why are you compiling without the CFLAGS from the Xen build system?
> 
> We explicitly disable strict alias optimisations, because optimisations
> based upon the aliasing rules in C is mad.  Even when you eliminate all
> the warnings, there are still subtle bugs because the compiler is free
> to assume a lot more than a programmer would typically deem reasonable.

Do you want me to repost the second patch (the actual bug fix one) so
that it doesn't assume the line number changes and whatnot for this one?

jch


> 
> ~Andrew
> 
>> ---
>>  tools/misc/xen-detect.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/misc/xen-detect.c b/tools/misc/xen-detect.c
>> index 787b5da..19c66d1 100644
>> --- a/tools/misc/xen-detect.c
>> +++ b/tools/misc/xen-detect.c
>> @@ -54,28 +54,27 @@ static void cpuid(uint32_t idx, uint32_t *regs, int pv_context)
>>  
>>  static int check_for_xen(int pv_context)
>>  {
>> -    uint32_t regs[4];
>> -    char signature[13];
>> +    union
>> +    {
>> +        uint32_t regs[4];
>> +        char signature[17];
>> +    } u;
>>      uint32_t base;
>>  
>>      for ( base = 0x40000000; base < 0x40010000; base += 0x100 )
>>      {
>> -        cpuid(base, regs, pv_context);
>> -
>> -        *(uint32_t *)(signature + 0) = regs[1];
>> -        *(uint32_t *)(signature + 4) = regs[2];
>> -        *(uint32_t *)(signature + 8) = regs[3];
>> -        signature[12] = '\0';
>> +        cpuid(base, u.regs, pv_context);
>> +        u.signature[16] = '\0';
>>  
>> -        if ( !strcmp("XenVMMXenVMM", signature) && (regs[0] >= (base + 2)) )
>> +        if ( !strcmp("XenVMMXenVMM", u.signature+4) && (u.regs[0] >= (base + 2)) )
>>              goto found;
>>      }
>>  
>>      return 0;
>>  
>>   found:
>> -    cpuid(base + 1, regs, pv_context);
>> -    return regs[0];
>> +    cpuid(base + 1, u.regs, pv_context);
>> +    return u.regs[0];
>>  }
>>  
>>  static jmp_buf sigill_jmp;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 


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

Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning.

[Andrew Cooper]

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

On 01/12/14 18:45, John Haxby wrote:
>
>> On 1 Dec 2014, at 17:15, Andrew Cooper <andrew.cooper3@citrix.com
>> <mailto:andrew.cooper3@citrix.com>> wrote:
>>
>> On 01/12/14 14:37, John Haxby wrote:
>>> With gcc 4.8.3, compiling xen-detect gives a compilation warning if
>>> you're optimising:
>>>
>>> $ cc -Wall -Os xen-detect.c
>>> xen-detect.c: In function ‘check_for_xen’:
>>> xen-detect.c:65:9: warning: dereferencing type-punned pointer will break
>>> strict-aliasing rules [-Wstrict-aliasing]
>>>         *(uint32_t *)(signature + 0) = regs[1];
>>>         ^
>>>
>>> Signed-off-by: John Haxby <john.haxby@oracle.com
>>> <mailto:john.haxby@oracle.com>>
>>
>> Why are you compiling without the CFLAGS from the Xen build system?
>>
>> We explicitly disable strict alias optimisations, because optimisations
>> based upon the aliasing rules in C is mad.  Even when you eliminate all
>> the warnings, there are still subtle bugs because the compiler is free
>> to assume a lot more than a programmer would typically deem reasonable.
>
>
> I wasn’t building the whole system, I just wanted xen-detect so I
> pulled it out and compiled it; I usually use "-Wall -Os” because the
> combination finds problems I might otherwise overlook.   The patch
> also removes three lines of code :) but you can take it or leave it as
> you choose.   The other patch — reversing the code of pv and hvm
> checking — was the real problem.
>
> jch

I feel it would be neater to fix this by using the
XEN_CPUID_SIGNATURE_E{B,C,D}X constants from the API.  This fixes the
strict aliasing, and does away with the string handling completely.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 9560 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 1/2] xen-detect: fix strict-aliasing compilation warning.

[Andrew Cooper]

On 03/12/14 15:39, John Haxby wrote:
> On 01/12/14 17:15, Andrew Cooper wrote:
>> On 01/12/14 14:37, John Haxby wrote:
>>> With gcc 4.8.3, compiling xen-detect gives a compilation warning if
>>> you're optimising:
>>>
>>> $ cc -Wall -Os xen-detect.c
>>> xen-detect.c: In function ‘check_for_xen’:
>>> xen-detect.c:65:9: warning: dereferencing type-punned pointer will break
>>> strict-aliasing rules [-Wstrict-aliasing]
>>>          *(uint32_t *)(signature + 0) = regs[1];
>>>          ^
>>>
>>> Signed-off-by: John Haxby <john.haxby@oracle.com>
>> Why are you compiling without the CFLAGS from the Xen build system?
>>
>> We explicitly disable strict alias optimisations, because optimisations
>> based upon the aliasing rules in C is mad.  Even when you eliminate all
>> the warnings, there are still subtle bugs because the compiler is free
>> to assume a lot more than a programmer would typically deem reasonable.
> Do you want me to repost the second patch (the actual bug fix one) so
> that it doesn't assume the line number changes and whatnot for this one?
>
> jch

With a pragmatic hat on, making more stuff "-Wall" safe is probably
better, although production code should use the surrounding infrastructure.

With all of these patches, you must CC the toolstack maintainers.  If
you believe it should make the cut for 4.5, you must also CC Konrad and
argue for a release ack.

~Andrew


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

Re: [PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM

[Andrew Cooper]

On 01/12/14 14:37, John Haxby wrote:
> At some stage, the cpuid instruction used to detect a xen hvm domain
> also started working in a pv domain so pv domains were being identified
> as hvm (dom0 excepted).  Change the order so that pv is tested for
> first.
>
> Signed-off-by: John Haxby <john.haxby@oracle.com>

This will have happened as a side effect of Intels CPUID-faulting
ability present in IvyBridge servers and later, which permits Xen the
ability to intercept regular cpuid instructions.

On the other hand, the forced emulation prefix is now valid in HVM
guests in debug Xens with the "hvm_fep" command line option.  In that
case, the HVM domain would be erroneously identified as PV.

Perhaps it is worth having an explicit guest type available in the cpuid
leaves themselves, so guest userspace need not guess at all?

~Andrew