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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/02/18 13:32, Jan Beulich wrote:
>>>> On 05.02.18 at 11:59, <andrew.cooper3@citrix.com> 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()

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

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

~Andrew

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