Re: [PATCH v2 0/8] x86emul: a few small steps towards disintegration

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2023 11:54, Roger Pau Monné wrote:
> On Thu, Mar 30, 2023 at 09:53:23AM +0200, Jan Beulich wrote:
>> On 29.03.2023 16:17, Roger Pau Monné wrote:
>>> Do we expect to be able to split other opcode handling into separate
>>> files?
>>
>> As per above, "expect" is perhaps too optimistic. I'd say "hope", in
>> particular for SIMD code (which I guess is now the main part of the
>> ISA as well as the sources, at least number-of-insns-wise). One
>> possible (likely intermediate) approach might be to move all SIMD code
>> from the huge switch() statement to its own file/function, invoked
>> from that huge switch()'s default: case. It may then still be a big
>> switch() statement in (e.g.) simd.c, but we'd then at least have two
>> of them, each about half the size of what we have right now. Plus it
>> may allow some (most?) of the X86EMUL_NO_SIMD #ifdef-ary to be dropped,
>> as the new file - like fpu.c - could then itself be built only
>> conditionally.
> 
> I don't like the handling of SIMD from a default case in the global
> switch much, as we then could end up chaining all kind of random
> handling in the default case.  It's IMO clearer if we can detect and
> forward insn to the SIMD code when we know it's a SIMD instruction.

Avoiding of which would require ...

>>> I wonder how tied together are the remaining cases, and whether we
>>> will be able to split more of them into separate units?
>>
>> That's the big open question indeed. As per above - with some effort
>> I expect all SIMD code could collectively be moved out; further
>> splitting would likely end up more involved.
>>
>>> Overall I don't think the disintegration makes the code harder, as the split
>>> cases seems to be fully isolated, I do however wonder whether further splits
>>> might not be so isolated?
>>
>> And again - indeed. This series, while already a lot of code churn, is
>> only collecting some of the very low hanging fruit. But at least I
>> hope that the pieces now separated out won't need a lot of touching
>> later on, except of course to add support for new insns.
> 
> OK, I'm a bit concerned that we end up growing duplicated switch
> cases, in the sense that we will have the following:
> 
> switch ( insn )
> {
> case 0x100:
> case 0x1f0:
> case 0x200:
>       x86emul_foo();
> ...
> }
> 
> x86emul_foo()
> {
>     switch (insn )
>     {
>     case 0x100:
>         /* Handle. */
>       break;
> 
>     case 0x1f0:
>         /* Handle. */
>       break;
> 
>     case 0x200:
>         ...
>     }
> }
> 
> So that we would end up having to add the opcodes twice, once in the
> generic switch, and then again at place the insn are actually handled.

... doing exactly this, which looks like we a agree we'd like to avoid.
I'm also not really concerned of "chaining all kind of random handling
in the default case" - where things are terminated with an error code
doesn't really matter. As first step here I'm literally thinking of
splitting the huge switch() into two. The only thing that I can see may
get in the way is the resulting SIMD function requiring like 20
parameters.

> So far the introduced splits seems fine in that they deal with a
> contiguous range of opcodes.
> 
> For patches 1-7:
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks!

> Patch 8 I'm unsure, I guess it should be up to the user to decide
> whether to use -Os or some other optimization?
> 
> I expect introspection users likely care way more about the speed
> rather than the size of the generated x86 emulator code?

Perhaps. But do we want to introduce another Kconfig control just
for that? And wouldn't there likely be other performance concerns,
if performance really mattered in the introspection case?

Jan