On 27/04/2021 08:09, Jan Beulich wrote:
> On 26.04.2021 14:45, Andrew Cooper wrote:
>> ... rather than having separate definitions locally. EXC_HAS_EC in
>> particular
>> is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC.
>>
>> Also switch a raw 14 for X86_EXC_PF in x86_emul_pagefault().
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Wei Liu <w...@xen.org>
>> ---
>> xen/arch/x86/x86_emulate/x86_emulate.c | 739
>> ++++++++++++++++-----------------
>> xen/arch/x86/x86_emulate/x86_emulate.h | 4 +-
>> 2 files changed, 361 insertions(+), 382 deletions(-)
> This is a lot of code churn (almost all some slight growth) for this
> kind of a change.
Interestingly, no lines needed re-wrapping as a consequence.
[Answering out of order]
> The other option,
> not reducing churn but reducing rather than increasing code volume (and
> hence imo helping readability), would be to have shorthands for at
> least some frequently raised exceptions like #UD and #GP - e.g.
> generate_ud_if(). Thougths?
#UD and #GP[0] are the overwhelming majority of cases. If you want to
reduce code volume further, I've always found the "generate" on the
front rather gratuitous. "raise" is a more common expression to use
with exceptions.
> I'm not opposed, but I'd like to explore alternatives
> first. I know you often dislike token concatenation in macros, which
> would be my first suggestion to get churn down here.
Outside of a few specific cases, yes, but as you can see in XTF,
exception handling is one area where IMO clarity can be improved with
certain limited macro magic.
In the emulator, I could get behind a single #define RAISE() along the
lines of:
#define RAISE(e, ...)
do {
BUILD_BUG_ON(((X86_EXC_HAS_EC & (1u << X86_EXC_##e)) !=
__count_args(__VA_ARGS__));
x86_emul_hw_exception(X86_EXC_##e, __count_args(__VA_ARGS__) ?
__VA_ARGS__ : X86_EVENT_NO_EC, ctxt);
rc = X86EMUL_EXCEPTION;
goto done;
} while ( 0 )
It's obviously playing behind your back, unlike generate_exception(),
and does build-time checking that error codes are handled correctly
(unlike mkec() which has a substantial quantity of code bloat to not
actually handle it correctly at runtime).
I dislike _if() suffixed macros, again for obfuscation reasons.
if ( foo )
RAISE(UD);
is far more normal C than
RAISE_IF(UD, foo);
or
RAISE_IF(foo, UD);
neither if which is a natural reading order. The reduction in line
count does not equate to improved code clarity. Frankly, areas of
x86_emulate() would benefit greatly from extra spacing per our normal
coding style.
~Andrew
