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

[Jan Beulich]

>>> On 05.02.18 at 17:00,  wrote:
> On 05/02/18 13:32, Jan Beulich wrote:
> On 05.02.18 at 11:59,  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Andrew Cooper]

On 05/02/18 13:32, Jan Beulich wrote:
 On 05.02.18 at 11:59,  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

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

[Jan Beulich]

>>> On 05.02.18 at 11:59,  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. 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). 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? 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.

Jan


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