Re: [Xen-devel] [PATCH v3 09/24] x86/emul: Provide a wrapper to x86_emulate() to ASSERT() certain behaviour

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.16 at 14:50, <andrew.cooper3@xxxxxxxxxx> 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


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