* [PATCH] x86/emul: Adjustments to exception error code handling
@ 2018-02-05 10:59 Andrew Cooper
2018-02-05 13:32 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2018-02-05 10:59 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
The mkec() end result in c/s bac133d43 wasn't really what I intended, and
unfortunately hides programmer errors if passing an error code with an
exception which doesn't take one.
Rework mkec() completely to simply insert X86_EVENT_NO_EC if no error code
parameter was provided. It still is the programmers responsibility to get
error codes correct, but an error will now trip an ASSERT() later during event
injection. All current generate_exception_if() users appear to be correct.
There is a minor improvement:
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-301 (-301)
function old new delta
protmode_load_seg 1460 1442 -18
x86_emulate 103975 103692 -283
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index f22f821..0b472b0 100644
--- 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; \
} \
--
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: Adjustments to exception error code handling
2018-02-05 10:59 [PATCH] x86/emul: Adjustments to exception error code handling Andrew Cooper
@ 2018-02-05 13:32 ` Jan Beulich
2018-02-05 16:00 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2018-02-05 13:32 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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. 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Adjustments to exception error code handling
2018-02-05 13:32 ` Jan Beulich
@ 2018-02-05 16:00 ` Andrew Cooper
2018-02-05 16:26 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2018-02-05 16:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/emul: Adjustments to exception error code handling
2018-02-05 16:00 ` Andrew Cooper
@ 2018-02-05 16:26 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2018-02-05 16:26 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 05.02.18 at 17:00, <andrew.cooper3@citrix.com> wrote:
> 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()
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-05 16:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-05 10:59 [PATCH] x86/emul: Adjustments to exception error code handling Andrew Cooper
2018-02-05 13:32 ` Jan Beulich
2018-02-05 16:00 ` Andrew Cooper
2018-02-05 16:26 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).