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

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH] x86/emul: Split exception handling out of invoke_stub()
Date: Thu, 25 Jan 2018 04:41:38 -0700	[thread overview]
Message-ID: <5A69D08202000078001A2546@prv-mh.provo.novell.com> (raw)
In-Reply-To: <85b8214f-9932-2bb2-9f6c-510288433d02@citrix.com>

>>> 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