Re: [Xen-devel] [PATCH] x86/emul: Adjustments to exception error code handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.18 at 17:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 05/02/18 13:32, Jan Beulich wrote:
>>>>> On 05.02.18 at 11:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -877,14 +877,12 @@ do {                                                  
>>>   \
>>>      if ( rc ) goto done;                                \
>>>  } while (0)
>>>  
>>> -static inline int mkec(uint8_t e, int32_t ec, ...)
>>> -{
>>> -    return (e < 32 && ((1u << e) & EXC_HAS_EC)) ? ec : X86_EVENT_NO_EC;
>>> -}
>>> +/* CPP magic.  Chooses ec if not empty, otherwise X86_EVENT_NO_EC. */
>>> +#define mkec(ignore, x, ...) x
>>>  
>>>  #define generate_exception_if(p, e, ec...)                                \
>>>  ({  if ( (p) ) {                                                          \
>>> -        x86_emul_hw_exception(e, mkec(e, ##ec, 0), ctxt);                 \
>>> +        x86_emul_hw_exception(e, mkec(X, ##ec, X86_EVENT_NO_EC), ctxt);   \
>>>          rc = X86EMUL_EXCEPTION;                                           \
>>>          goto done;                                                        \
>>>      }                                                                     \
>> This orphans EXC_HAS_EC, which makes me wonder what assertion
>> you're talking about in the description.
> 
> {pv,hvm}_inject_event()

Which means that ...

>> The way things are before
>> your change means that at least an exception with error code will
>> be delivered properly (the error code will be zero then) if it wasn't
>> specified in the invocation (which, as you may recall, I actually
>> consider useful, but you did object to making this an "officially"
>> allowed mechanism).
> 
> It also meant that programming errors go completely unnoticed, which is
> worse.
> 
>> With your change in place, an assertion will
>> supposedly trigger (wherever that is), killing the host or (in a
>> release build) leading to some other behavior that's likely fatal to
>> a guest. Would the guest perhaps get to see an error code of all
>> ones?
> 
> In a release builds, it depends how vicious the vmentry checks are.

... covers only half of it - there are no such checks at all for PV.

>>  If, otoh, we could know at build time that something is wrong,
>> I would be quite a bit more in agreement with doing such a change,
>> most importantly because those exception raising paths are rarely
>> hit, and are mostly (if not entirely) untested by the test harness.
> 
> I was originally aiming for a build time check, but the check_fpu_exn()
> and protmode_load_seg() paths at least have non-constant exceptions.
> 
> We could force a constant exception by BUILD_BUG_ON(e >= 32), and
> opencode the result of check_fpu_exn() (which is the only case which
> can't be converted to a constant exception) to use
> x86_emul_hw_exception() directly with suitable auditing.

I'd prefer to avoid such open coding. Would the combination of
__builtin_constant_p() and a reference to a link-time undefined
symbol not do the job?

Jan


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