Re: [PATCH] x86/ioemul: Rewrite stub generation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/04/2020 16:28, Jan Beulich wrote:
> On 27.04.2020 14:20, Andrew Cooper wrote:
>> The logic is completely undocumented and almost impossible to follow.  It
>> actually uses return oriented programming.  Rewrite it to conform to more
>> normal call mechanics, and leave a big comment explaining thing.  As well as
>> the code being easier to follow, it will execute faster as it isn't fighting
>> the branch predictor.
>>
>> Move the ioemul_handle_quirk() function pointer from traps.c to
>> ioport_emulate.c.  There is no reason for it to be in neither of the two
>> translation units which use it.  Alter the behaviour to return the number of
>> bytes written into the stub.
>>
>> Access the addresses of the host/guest helpers with extern const char arrays.
>> Nothing good will come of C thinking they are regular functions.
> I agree with the C aspect, but imo the assembly routines should,
> with the changes you make, be marked as being ordinary functions.

Despite the changes, they are still very much not ordinary functions,
and cannot be used by C.

I have no objection to marking them as STT_FUNCTION (as this doesn't
mean C function), but...

> A reasonable linker would then warn about the C file wanting an
> STT_OBJECT while the assembly file provides an STT_FUNC. I'd
> therefore prefer, along with marking the functions as such, to
> have them also declared as functions in C.

... there is literally nothing safe which C can do with them other than
manipulate their address.

Writing it like this is specifically prevents something from compiling
which will explode at runtime, is someone is naive enough to try using
the function from C.

>> --- a/xen/arch/x86/ioport_emulate.c
>> +++ b/xen/arch/x86/ioport_emulate.c
>> @@ -8,7 +8,10 @@
>>  #include <xen/sched.h>
>>  #include <xen/dmi.h>
>>  
>> -static bool ioemul_handle_proliant_quirk(
>> +unsigned int (*ioemul_handle_quirk)(
>> +    u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
> Would you mind adding __read_mostly on this occasion?
>
>> @@ -19,18 +22,16 @@ static bool ioemul_handle_proliant_quirk(
>>          0xa8, 0x80, /*    test $0x80, %al */
>>          0x75, 0xfb, /*    jnz 1b          */
>>          0x9d,       /*    popf            */
>> -        0xc3,       /*    ret             */
>>      };
>>      uint16_t port = regs->dx;
>>      uint8_t value = regs->al;
>>  
>>      if ( (opcode != 0xee) || (port != 0xcd4) || !(value & 0x80) )
>> -        return false;
>> +        return 0;
>>  
>>      memcpy(io_emul_stub, stub, sizeof(stub));
>> -    BUILD_BUG_ON(IOEMUL_QUIRK_STUB_BYTES < sizeof(stub));
> So you treat a build failure for a runtime crash.

I presume you mean s/treat/trade/ here, and the answer is no, not really.

There is nothing which actually forced a connection between the build
time checks and runtime behaviour, so it was only a facade of safety,
not real safety.

>  I can see the
> advantages of the new approach, but the original got away with
> less stub space.

Stub space doesn't matter, so long as it fits.  In particular, ...

> If our L1_CACHE_SHIFT wasn't hard coded to 7
> just to cover some unusual CPUs, your new approach would, if I
> got the counting and calculations right, not work (with a value
> resulting in a 64-byte cache line size).

... the SYSCALL stubs use 64 bytes so Xen cannot function with a shift
less than 7.

> Introducing a Kconfig
> option for this should imo not come with a need to re-work the
> logic here again. Therefore I'd like us to think about a way
> to make the space needed not exceed 32 bytes.

And why would we ever want an option like that?  (I know how you're
going to answer this, so I'm going to pre-emptively point out that there
are hundreds of kilobytes of easier-to-shrink per-cpu data structures
than this one).

Honestly, this suggestion is a total waste of time and effort.  It is an
enormous amount of complexity to micro-optimise a problem which doesn't
exist in the first place.

The stubs are already 128 bytes per CPU and cannot be made smaller. 
Attempting to turn this particular stub into <32 has no benefit (the
stubs don't actually get smaller), and major costs.

~Andrew