Re: [PATCH] x86/emul: Use existing X86_EXC_* constants

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> ---
>>  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