| Message ID | 1480513841-7565-10-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > In debug builds, confirm that some properties of x86_emulate()'s behaviour > actually hold. The first property, fixed in a previous change, is that retire > flags are only ever set in the X86EMUL_OKAY case. > > While adjusting the userspace test harness to cope with ASSERT() in > x86_emulate.h, fix a build problem introduced in c/s 122dd9575c7 "x86emul: > in_longmode() should not ignore ->read_msr() errors" by providing an > implementation of likely()/unlikely(). Oh, I'm sorry for that one. When moving that patch ahead of about 50 other ones touching the emulator, I didn't notice I should have pulled that addition out from another patch. > --- a/tools/tests/x86_emulator/x86_emulate.c > +++ b/tools/tests/x86_emulator/x86_emulate.c > @@ -50,4 +50,7 @@ typedef bool bool_t; > #define __init > #define __maybe_unused __attribute__((__unused__)) > > +#define likely(x) __builtin_expect(!!(x),1) > +#define unlikely(x) __builtin_expect(!!(x),0) Please use true/false here and add blanks after the commas. > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -2404,6 +2404,11 @@ x86_decode( > #undef insn_fetch_bytes > #undef insn_fetch_type > > +/* Undo DEBUG wrapper. */ > +#ifdef x86_emulate > +#undef x86_emulate > +#endif I don't see the need for the #ifdef here. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -23,6 +23,10 @@ > #ifndef __X86_EMULATE_H__ > #define __X86_EMULATE_H__ > > +#ifndef ASSERT > +#define ASSERT assert > +#endif This doesn't seem to belong here (as there's nothing making sure assert is defined), and duplicates an existing #define in the test harness'es x86_emulate.c. I could agree to deleting that other one and wrapping the one here with #ifndef __XEN__. > @@ -554,6 +558,27 @@ x86_emulate( > const struct x86_emulate_ops *ops); > > /* > + * In debug builds, wrap x86_emulate() with some assertions about its expected > + * behaviour. > + */ > +#ifndef NDEBUG Mind swapping the order of comment and #ifndef, to make it more reasonable to possibly add further things into this guarded block? Jan
On 01/12/16 10:40, Jan Beulich wrote: > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -2404,6 +2404,11 @@ x86_decode( >> #undef insn_fetch_bytes >> #undef insn_fetch_type >> >> +/* Undo DEBUG wrapper. */ >> +#ifdef x86_emulate >> +#undef x86_emulate >> +#endif > I don't see the need for the #ifdef here. It will break the non-debug build if removed, as x86_emulate wouldn't be a define. > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -23,6 +23,10 @@ >> #ifndef __X86_EMULATE_H__ >> #define __X86_EMULATE_H__ >> >> +#ifndef ASSERT >> +#define ASSERT assert >> +#endif > This doesn't seem to belong here (as there's nothing making sure > assert is defined), and duplicates an existing #define in the test > harness'es x86_emulate.c. I could agree to deleting that other one > and wrapping the one here with #ifndef __XEN__. Ok. > >> @@ -554,6 +558,27 @@ x86_emulate( >> const struct x86_emulate_ops *ops); >> >> /* >> + * In debug builds, wrap x86_emulate() with some assertions about its expected >> + * behaviour. >> + */ >> +#ifndef NDEBUG > Mind swapping the order of comment and #ifndef, to make it more > reasonable to possibly add further things into this guarded block? Ok. ~Andrew
>>> On 01.12.16 at 11:58, <andrew.cooper3@citrix.com> wrote: > On 01/12/16 10:40, Jan Beulich wrote: >> >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >>> @@ -2404,6 +2404,11 @@ x86_decode( >>> #undef insn_fetch_bytes >>> #undef insn_fetch_type >>> >>> +/* Undo DEBUG wrapper. */ >>> +#ifdef x86_emulate >>> +#undef x86_emulate >>> +#endif >> I don't see the need for the #ifdef here. > > It will break the non-debug build if removed, as x86_emulate wouldn't be > a define. #undef for something that isn't a #define is well defined to do nothing. Jan
diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c index f255fef..b54fd11 100644 --- a/tools/tests/x86_emulator/test_x86_emulator.c +++ b/tools/tests/x86_emulator/test_x86_emulator.c @@ -1,3 +1,4 @@ +#include <assert.h> #include <errno.h> #include <limits.h> #include <stdbool.h> diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c index c46b7fc..3272867 100644 --- a/tools/tests/x86_emulator/x86_emulate.c +++ b/tools/tests/x86_emulator/x86_emulate.c @@ -50,4 +50,7 @@ typedef bool bool_t; #define __init #define __maybe_unused __attribute__((__unused__)) +#define likely(x) __builtin_expect(!!(x),1) +#define unlikely(x) __builtin_expect(!!(x),0) + #include "x86_emulate/x86_emulate.c" diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index bacdee6..e4643a3 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2404,6 +2404,11 @@ x86_decode( #undef insn_fetch_bytes #undef insn_fetch_type +/* Undo DEBUG wrapper. */ +#ifdef x86_emulate +#undef x86_emulate +#endif + int x86_emulate( struct x86_emulate_ctxt *ctxt, diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index ef39601..f84ced2 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -23,6 +23,10 @@ #ifndef __X86_EMULATE_H__ #define __X86_EMULATE_H__ +#ifndef ASSERT +#define ASSERT assert +#endif + #define MAX_INST_LEN 15 struct x86_emulate_ctxt; @@ -554,6 +558,27 @@ x86_emulate( const struct x86_emulate_ops *ops); /* + * In debug builds, wrap x86_emulate() with some assertions about its expected + * behaviour. + */ +#ifndef NDEBUG +static inline int x86_emulate_wrapper( + struct x86_emulate_ctxt *ctxt, + const struct x86_emulate_ops *ops) +{ + int rc = x86_emulate(ctxt, ops); + + /* Retire flags should only be set for successful instruction emulation. */ + if ( rc != X86EMUL_OKAY ) + ASSERT(ctxt->retire.raw == 0); + + return rc; +} + +#define x86_emulate x86_emulate_wrapper +#endif + +/* * Given the 'reg' portion of a ModRM byte, and a register block, return a * pointer into the block that addresses the relevant register. * @highbyte_regs specifies whether to decode AH,CH,DH,BH.
In debug builds, confirm that some properties of x86_emulate()'s behaviour actually hold. The first property, fixed in a previous change, is that retire flags are only ever set in the X86EMUL_OKAY case. While adjusting the userspace test harness to cope with ASSERT() in x86_emulate.h, fix a build problem introduced in c/s 122dd9575c7 "x86emul: in_longmode() should not ignore ->read_msr() errors" by providing an implementation of likely()/unlikely(). Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> v3: * New --- tools/tests/x86_emulator/test_x86_emulator.c | 1 + tools/tests/x86_emulator/x86_emulate.c | 3 +++ xen/arch/x86/x86_emulate/x86_emulate.c | 5 +++++ xen/arch/x86/x86_emulate/x86_emulate.h | 25 +++++++++++++++++++++++++ 4 files changed, 34 insertions(+)