* [PATCH] x86/emul: Split exception handling out of invoke_stub()
@ 2018-01-24 18:16 Andrew Cooper
2018-01-25 10:49 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2018-01-24 18:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
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__, and print the
instruction stream hexdump at WARNING as well.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Split exception handling out of invoke_stub()
2018-01-24 18:16 [PATCH] x86/emul: Split exception handling out of invoke_stub() Andrew Cooper
@ 2018-01-25 10:49 ` Jan Beulich
2018-01-25 11:09 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2018-01-25 10:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 24.01.18 at 19:16, <andrew.cooper3@citrix.com> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Split exception handling out of invoke_stub()
2018-01-25 10:49 ` Jan Beulich
@ 2018-01-25 11:09 ` Andrew Cooper
2018-01-25 11:41 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2018-01-25 11:09 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 25/01/18 10:49, Jan Beulich wrote:
>>>> On 24.01.18 at 19:16, <andrew.cooper3@citrix.com> 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.
The point of printing the instruction stream at the WARNING is that it
uniquely identifies the invoke_stub() call, just like the __LINE__
information does, and this rearrangement makes __LINE__ awkward to use.
We'd need another __XEN__-guarded local variable on the stack.
The tradeoff for livepatching is how likely we are to have a
livepatchable security issue which modifies something in x86_emulate.c
which results in perturbance of __LINE__, vs the utility of using
__LINE__ in the first place.
All uses of __LINE__ here are part of x86_emulate(), but we have had
issues in the past which are fixed exclusively in the x86_decode() path.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Split exception handling out of invoke_stub()
2018-01-25 11:09 ` Andrew Cooper
@ 2018-01-25 11:41 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2018-01-25 11:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 25.01.18 at 12:09, <andrew.cooper3@citrix.com> wrote:
> On 25/01/18 10:49, Jan Beulich wrote:
>>>>> On 24.01.18 at 19:16, <andrew.cooper3@citrix.com> 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.
>
> The point of printing the instruction stream at the WARNING is that it
> uniquely identifies the invoke_stub() call, just like the __LINE__
> information does,
I don't think I see why that would be. There are certainly
instructions which we encode in more than one place (first
and foremost {,v}pmovmskb and vmovmskp{s,d}. This set is
liable to grow once we get to support AVX512.
> and this rearrangement makes __LINE__ awkward to use.
> We'd need another __XEN__-guarded local variable on the stack.
Why? Just add a line number field to stub_exn_info.
> The tradeoff for livepatching is how likely we are to have a
> livepatchable security issue which modifies something in x86_emulate.c
> which results in perturbance of __LINE__, vs the utility of using
> __LINE__ in the first place.
>
> All uses of __LINE__ here are part of x86_emulate(), but we have had
> issues in the past which are fixed exclusively in the x86_decode() path.
I'm afraid I can't really conclude what you're trying to tell me here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-01-25 11:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-24 18:16 [PATCH] x86/emul: Split exception handling out of invoke_stub() Andrew Cooper
2018-01-25 10:49 ` Jan Beulich
2018-01-25 11:09 ` Andrew Cooper
2018-01-25 11:41 ` Jan Beulich
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.