Re: [Xen-devel] [PATCH] x86/emul: Split exception handling out of invoke_stub()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.18 at 19:16, <andrew.cooper3@xxxxxxxxxx> wrote:
> For a release build, bloat-o-meter reports:
> 
>   add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-5111 (-5111)
>   function                                     old     new   delta
>   x86_emulate                               126458  121347   -5111
> 
> or in other words, a 4% redunction in code size from this change alone.
> 
> While shuffling things around, drop the use of __LINE__,

While the rest of the change is fine, I continue to object to the
removal of __LINE__ here - afaict it is awkward to reconstruct the
line number when being presented just the address. At the very
least you'd have to run a tool like addr2line, which assumes you
have the correct binary to hand (which is not very likely based on
my experience). However much I can agree that line numbers get
in the way of live patching, there are cases where problem
analysis is quite a bit harder without them. And this is an example
thereof.

Jan

> and print the instruction stream hexdump at WARNING as well.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> 
> I stubled onto this while looking at the domain_crash() side of things.  It
> appears that your AVX series makes the problem more pronounced, due to more
> codepaths using invoke_stub().
> ---
>  xen/arch/x86/x86_emulate/x86_emulate.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c 
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index ff0a003..6dccf4e 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -866,7 +866,7 @@ static inline int mkec(uint8_t e, int32_t ec, ...)
>  
>  #ifdef __XEN__
>  # define invoke_stub(pre, post, constraints...) do {                    \
> -    union stub_exception_token res_ = { .raw = ~0 };                    \
> +    stub_exn_info = (union stub_exception_token) { .raw = ~0 };         \
>      asm volatile ( pre "\n\tINDIRECT_CALL %[stub]\n\t" post "\n"        \
>                     ".Lret%=:\n\t"                                       \
>                     ".pushsection .fixup,\"ax\"\n"                       \
> @@ -875,21 +875,11 @@ static inline int mkec(uint8_t e, int32_t ec, ...)
>                     "jmp .Lret%=\n\t"                                    \
>                     ".popsection\n\t"                                    \
>                     _ASM_EXTABLE(.Lret%=, .Lfix%=)                       \
> -                   : [exn] "+g" (res_), constraints,                    \
> +                   : [exn] "+g" (stub_exn_info), constraints,           \
>                       [stub] "r" (stub.func),                            \
>                       "m" (*(uint8_t(*)[MAX_INST_LEN + 1])stub.ptr) );   \
> -    if ( unlikely(~res_.raw) )                                          \
> -    {                                                                   \
> -        gprintk(XENLOG_WARNING,                                         \
> -                "exception %u (ec=%04x) in emulation stub (line %u)\n", \
> -                res_.fields.trapnr, res_.fields.ec, __LINE__);          \
> -        gprintk(XENLOG_INFO, "stub: %"__stringify(MAX_INST_LEN)"ph\n",  \
> -                stub.func);                                             \
> -        generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD);    \
> -        domain_crash(current->domain);                                  \
> -        rc = X86EMUL_UNHANDLEABLE;                                      \
> -        goto done;                                                      \
> -    }                                                                   \
> +    if ( unlikely(~stub_exn_info.raw) )                                 \
> +        goto emulation_stub_failure;                                    \
>  } while (0)
>  #else
>  # define invoke_stub(pre, post, constraints...)                         \
> @@ -3000,6 +2990,9 @@ x86_emulate(
>      struct fpu_insn_ctxt fic = { .type = X86EMUL_FPU_none, .exn_raised = -1 
> };
>      struct x86_emulate_stub stub = {};
>      DECLARE_ALIGNED(mmval_t, mmval);
> +#ifdef __XEN__
> +    union stub_exception_token stub_exn_info;
> +#endif
>  
>      ASSERT(ops->read);
>  
> @@ -8012,6 +8005,18 @@ x86_emulate(
>      put_stub(stub);
>      return rc;
>  #undef state
> +
> +#ifdef __XEN__
> + emulation_stub_failure:
> +    gprintk(XENLOG_WARNING, "exception %u (ec=%04x) in emulation stub\n",
> +            stub_exn_info.fields.trapnr, stub_exn_info.fields.ec);
> +    gprintk(XENLOG_WARNING, "  stub: %"__stringify(MAX_INST_LEN)"ph\n",
> +            stub.func);
> +    generate_exception_if(stub_exn_info.fields.trapnr == EXC_UD, EXC_UD);
> +    domain_crash(current->domain);
> +    rc = X86EMUL_UNHANDLEABLE;
> +    goto done;
> +#endif
>  }
>  
>  #undef op_bytes
> -- 
> 2.1.4



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel