* [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).