* Two miscellenous xen-detect patches. @ 2014-12-01 14:37 John Haxby 2014-12-01 14:37 ` [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning John Haxby 2014-12-01 14:37 ` [PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM John Haxby 0 siblings, 2 replies; 9+ messages in thread From: John Haxby @ 2014-12-01 14:37 UTC (permalink / raw) To: xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning. 2014-12-01 14:37 Two miscellenous xen-detect patches John Haxby @ 2014-12-01 14:37 ` John Haxby 2014-12-01 17:15 ` Andrew Cooper 2014-12-01 14:37 ` [PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM John Haxby 1 sibling, 1 reply; 9+ messages in thread From: John Haxby @ 2014-12-01 14:37 UTC (permalink / raw) To: xen-devel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning. 2014-12-01 14:37 ` [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning John Haxby @ 2014-12-01 17:15 ` Andrew Cooper 2014-12-01 18:45 ` John Haxby 2014-12-03 15:39 ` John Haxby 0 siblings, 2 replies; 9+ messages in thread From: Andrew Cooper @ 2014-12-01 17:15 UTC (permalink / raw) To: John Haxby, xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning. 2014-12-01 17:15 ` Andrew Cooper @ 2014-12-01 18:45 ` John Haxby 2014-12-03 15:44 ` Andrew Cooper 2014-12-03 15:39 ` John Haxby 1 sibling, 1 reply; 9+ messages in thread From: John Haxby @ 2014-12-01 18:45 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning. 2014-12-01 18:45 ` John Haxby @ 2014-12-03 15:44 ` Andrew Cooper 0 siblings, 0 replies; 9+ messages in thread From: Andrew Cooper @ 2014-12-03 15:44 UTC (permalink / raw) To: John Haxby; +Cc: xen-devel [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning. 2014-12-01 17:15 ` Andrew Cooper 2014-12-01 18:45 ` John Haxby @ 2014-12-03 15:39 ` John Haxby 2014-12-03 15:51 ` Andrew Cooper 1 sibling, 1 reply; 9+ messages in thread From: John Haxby @ 2014-12-03 15:39 UTC (permalink / raw) To: Andrew Cooper, xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning. 2014-12-03 15:39 ` John Haxby @ 2014-12-03 15:51 ` Andrew Cooper 0 siblings, 0 replies; 9+ messages in thread From: Andrew Cooper @ 2014-12-03 15:51 UTC (permalink / raw) To: John Haxby, xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM 2014-12-01 14:37 Two miscellenous xen-detect patches John Haxby 2014-12-01 14:37 ` [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning John Haxby @ 2014-12-01 14:37 ` John Haxby 2014-12-03 16:00 ` Andrew Cooper 1 sibling, 1 reply; 9+ messages in thread From: John Haxby @ 2014-12-01 14:37 UTC (permalink / raw) To: xen-devel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM 2014-12-01 14:37 ` [PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM John Haxby @ 2014-12-03 16:00 ` Andrew Cooper 0 siblings, 0 replies; 9+ messages in thread From: Andrew Cooper @ 2014-12-03 16:00 UTC (permalink / raw) To: John Haxby, xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-12-03 16:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-01 14:37 Two miscellenous xen-detect patches John Haxby 2014-12-01 14:37 ` [PATCH 1/2] xen-detect: fix strict-aliasing compilation warning John Haxby 2014-12-01 17:15 ` Andrew Cooper 2014-12-01 18:45 ` John Haxby 2014-12-03 15:44 ` Andrew Cooper 2014-12-03 15:39 ` John Haxby 2014-12-03 15:51 ` Andrew Cooper 2014-12-01 14:37 ` [PATCH 2/2] xen-detect: check for XEN_PV before XEN_HVM John Haxby 2014-12-03 16:00 ` Andrew Cooper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.