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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Besides being certain that such an attempt, if it made it into a
submitted patch in the first place, would be caught by review, I
don't see you addressing my main counter argument. Preventing a
declared function to be called can be had by other means, e.g.
__attribute__((__warning__())).

>>> @@ -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'm not following, I'm afraid: The above together with

    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX(9, /* Default emul stub */
                                         5 + IOEMUL_QUIRK_STUB_BYTES));

(where the literal numbers live next to what they describe)
did very well provide some level of guarding.

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

Oh, my fault - I read the STUB_BUF_SHIFT expression as min() when
it really is max(). So yes, we can rely on there being 64 bytes.
(Everything further down therefore becomes moot afaict.)

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

Not sure what per-CPU data structures you talk about. I wasn't
thinking of space savings in particular, but rather about getting
our setting in sync with actual hardware. Its value is, afaics,
used in a far more relevant way by xmalloc() and friends.

Jan

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