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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2021 20:56, Andrew Cooper wrote:
> 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.

Indeed I, too, have been wondering whether "raise" wouldn't be better
(but see also below). "generate_exception" predates my participation
in development, iirc.

>>  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 )

Looks good to me at a first glance, except that I think the left
side of the != in the BUILD_BUG_ON() needs converting to bool
(either explicitly, or by shifting right X86_EXC_HAS_EC instead
of shifting left 1u).

> It's obviously playing behind your back, unlike generate_exception(),

Playing behind our backs? I don't think I see what you mean. 

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

Are you telling me that the compiler doesn't do enough constant
folding there? And I'm also not seeing what doesn't get handled
correctly there.

> 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.

Not sure here. I wouldn't object the change you suggest, but I've
never had a problem with generate_exception_if(). I guess it's
less clear with RAISE_IF(), but that may be an entirely personal
perception. It was for that reason though that I suggested (now
adapting to the alternative naming) RAISE_UD_IF() as a possible
shorthand. As a good example where I think all the extra if()
would make the code harder to read, please see e.g. the pending
"x86emul: support tile multiplication insns".

>  Frankly, areas of
> x86_emulate() would benefit greatly from extra spacing per our normal
> coding style.

I wonder what you refer to by "normal" here: If I guess correctly
what you mean, I don't think ./CODING_STYLE mandates anything.
Not even blank lines between non-fall-through case label blocks.
In recent work I've been trying to separate logical chunks, but I
guess I may not have gone far enough for your taste ...

Really what I'm still hoping to have a good idea for is how to
split up the gigantic switch() statement in there, without having
to introduce dozens of functions with very long parameter lists,
and without sacrificing the centralized SIMD handling immediately
after that switch(). Ideally I'd want to even split up the
monolithic source file, as - especially with older gcc - its
compilation has been dominating build time particularly when
passing make a sufficiently high -j.

Jan