[ros-dev] [ros-diffs] [sir_richard] 45052: Patch that fixes VMWare boot (and

[Timo Kreuzer]

Ros Arm wrote:
> Hi Timo,
>
> Regarding #1, we actually have this working in our tree. It was required for the new SYSENTRY C handler. So thank you for the advice and for pointing that out. Unfortunately I will not credit you for the idea since the code was already written, I hope this is understandable. We had to do this not for performance, but for compatibility with the Eoi helper function which is exported. The performance difference between pushad and doing the instructions manually (and later fixing up) was actually not that big, since pushad has certain pipeline advantages. However, when dealing with interrupts, it starts becoming more of a problem (although that code is not in mainline yet). Also your sample code has some bugs: the last integer register save is duplicated, and it would be better to use a symbolic name instead of 10 * 4. That value is also wrong, since it depends on whether or not the trap generates an error code or not. x86 is not easy, have to be careful!
>   
I know this, I wrote the amd64 trap handlers ;)
I was expecting the error code to be already pushed in this case. I also
agree a symbolic name is better, but using a name with a wrong /
misleading meaning seems worse. KTRAP_FRAME_LENGTH -
KTRAP_FRAME_PREVIOUS_MODE is 0x8C - 0x48 = 0x44, which is identical to
KTRAP_FRAME_EAX. The latter would make much more sense. What you want is
the size of the KTRAP_FRAME minus the size of the fields that are
already pushed on the stack. But these fields are at the bottom (higher
addresses) of the structure. Therefore it makes no sense to substract
something counted from the top of the structure ;)

> Regarding #2, I unfortunately do not see the need to add more assembly when it is not needed. There are no subtle compiler breakages involved, since it is impossible for the compiler to add a DS/ES access for no reason at all (can you come up with an example?), this is part of the contract the compiler has to make (if compilers did this, it would break a whole class of embedded and real-mode software). 
The assumption here is that the compiler would never generate assembly
code that uses the ds or es segment selectors unless you dereference a
pointer. But I don't kow of any rule that guarantees that. (Do you?) In
fact in a flat memory model ds and ss are supposed to point to the same
flat 32 bit address space, so the compiler might decide to use ds/es
(for example to make use of string operations on local variables). This
might usually not happen, but if it's not forbidden, we must expect it
to happen.

I actually have an example:

int
foo(int x)
{
    switch (x)
    {
        case 0: return 4;
        case 1: return 19;
        case 2: return 2;
        case 3: return 11;
        case 4: return 121;
    }
    return 0;
}

gcc will create this:

    push ebp
    mov ebp, esp
    mov eax, [ebp+8]
    cmp eax, 4
    ja default
    jmp ds:table[eax*4]

This might also not be used, but it just serves an an example of what
the compiler might decide to do.


> Because the stack is safe, I think it's worth avoiding the extra lines of assembly. The C inlines will produce exactly the same code anyway. Also, it's not as easy as you make it seem, since for a FAST-V86 trap, we don't want to save the registers, just change them. So we need to insert a check for ...
>   
What's the reason for not saving them?


Timo