Re: [ros-dev] [ros-diffs] [sir_richard] 45052: Patch that fixes VMWare boot (and should fix QEMu/KVM boot on the testbot): [NTOS]: A trap can get us into a state where DS/ES are invalid, making any pointer dereference (on DS/ES segmented memory, not SS, the stack) crash (and probably double-fault). Therefore, we have to be careful to switch to a good DS/ES before touching the TrapFrame pointer, which we don't have in ESP like the ASM code, but in a DS/ES-segmented register. For V8086 traps we can switch to

[Timo Kreuzer]

Hi,

I suggest the following changes to the current implementation:

1.) Replace the PUSHA in the trap entry code, by a series of MOVs to the
correct stack positions. The same for the trap exit code.
KiTrapFrameFromPushaStack can be removed then. This would result in more
clear, less complex and faster code, with only  a few additional
assembly instructions in  the trap entry macro. The exit could be done
either by normal call/return or by a jmp to an exit handler.

2.) Segements should be fixed up before entering C code, as not doing so
may introduce possible compiler dependend breakages. It's also much
cleaner and there's no reason to do the same stuff later in inline
assembly instead of direcly in the asm entry point.

The resulting code might looks something like this:

    /* Allocate KTRAP_FRAME */
    sub esp, KTRAP_FRAME_LENGTH - 10 * 4

    /* Save integer registers */
    mov [esp + KTRAP_FRAME_EBP], ebp
    mov [esp + KTRAP_FRAME_EBX], ebx
    mov [esp + KTRAP_FRAME_ESI], esi
    mov [esp + KTRAP_FRAME_EDI], edi
    mov [esp + KTRAP_FRAME_EAX], eax
    mov [esp + KTRAP_FRAME_ECX], ecx
    mov [esp + KTRAP_FRAME_EDX], edx
    mov [esp + KTRAP_FRAME_EBX], ebx

    /* Save segment regs */
    mov [esp + KTRAP_FRAME_SEGDS], ds
    mov [esp + KTRAP_FRAME_SEGES], es

    /* Fixup segment regs */
    mov ax, KGDT_R3_DATA | RPL_MASK
    mov ds, ax
    mov es, ax


Timo

[Ros Arm]

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!

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). 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 EFLAGS to see if the V8086 flag is there or not, then conditionally do
the registers, then jump to the C code that will do another conditional check, and pick
the right trap entry C code... it gets quite complicated. Since there are no
"real" issue with doing it in C, other than personal choice and style, I think
we'll stick with what we have.

The patch for #1 will also start introducing some other performance enhancing features,
such as branch prediction hints for the CPU and compiler, and other little boosts. With
the pusha conversion code gone, the C code is actually more efficient than previous
assembly implementations.

Thanks for your comments and suggestions, please keep them coming. Do not be discouraged
by this particular decision regarding #2 (it's also slightly political) as a sign or
pattern that any suggestion will be ignored (you were not the first to bring up #1,
otherwise we'd have done it thanks to you).

Also, if anyone has any experience with MSC++ and could port the GCC-centric macros, that
would be much appreciated. I've given up on MSC++ after the compiler and linker have
tried, year after year, to add more and more shit to what used to be a simple portable
codebase. Buffer this, buffer that, nx this, dep that, safe seh here, hotpatch there,
manifest files to even get it to run... ARGH!

Finally, the recent fixes have solved the double fault problem, but it seems certain
flavours of VMWare use a different Video BIOS than my test machines and that V8086 C
emulation code must be hitting a bug. It could also be we're seeing the BOP and not
handling it. I'll look into this today. Note that the ASM implementation of PUSHF  (or
was it POPF) was completely broken. Someone must've made some serious typos when
*ahem* writing the code. Hopefully there are no dependencies on that bug.

-r

>   Hi,
>  
>  I suggest the following changes to the current implementation:
>  
>  1.) Replace the PUSHA in the trap entry code, by a series of MOVs to the
>  correct stack positions. The same for the trap exit code.
>  KiTrapFrameFromPushaStack can be removed then. This would result in more
>  clear, less complex and faster code, with only  a few additional
>  assembly instructions in  the trap entry macro. The exit could be done
>  either by normal call/return or by a jmp to an exit handler.
>  
>  2.) Segements should be fixed up before entering C code, as not doing so
>  may introduce possible compiler dependend breakages. It's also much
>  cleaner and there's no reason to do the same stuff later in inline
>  assembly instead of direcly in the asm entry point.
>  
>  The resulting code might looks something like this:
>  
>      /* Allocate KTRAP_FRAME */
>      sub esp, KTRAP_FRAME_LENGTH - 10 * 4
>  
>      /* Save integer registers */
>      mov [esp + KTRAP_FRAME_EBP], ebp
>      mov [esp + KTRAP_FRAME_EBX], ebx
>      mov [esp + KTRAP_FRAME_ESI], esi
>      mov [esp + KTRAP_FRAME_EDI], edi
>      mov [esp + KTRAP_FRAME_EAX], eax
>      mov [esp + KTRAP_FRAME_ECX], ecx
>      mov [esp + KTRAP_FRAME_EDX], edx
>      mov [esp + KTRAP_FRAME_EBX], ebx
>  
>      /* Save segment regs */
>      mov [esp + KTRAP_FRAME_SEGDS], ds
>      mov [esp + KTRAP_FRAME_SEGES], es
>  
>      /* Fixup segment regs */
>      mov ax, KGDT_R3_DATA | RPL_MASK
>      mov ds, ax
>      mov es, ax
>  
>  
>  Timo
>  
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[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

[Jose Catena]

>   Also, if anyone has any experience with MSC++ and
> could port the 
GCC-centric macros, that would be much appreciated.

I'm doing that. Right now I compile and run ntoskrnl in msc (vc9). Runs
mostly but I'm still fixing bugs.
I have written new small platform and compiler dependent includes and
eliminate all unnecessary conditionals everywhere else, this cleans up
things a lot.
But, I asked if my first patch was reported correctly and if I should submit
more, received no answer, and it's still not reviewed. I suppose because I
am not a respectful enough noob and didn't start kissing asses. So I'm not
going to submit any more unless this childish attitude is fixed. But I'll
put all my work periodically in my server and announce the availability here
so that anyone interested could download it and use whatever you want in
ReactOS. 

Jose Catena
DIGIWAVES S.L.

[Steven Edwards]

On Tue, Jan 12, 2010 at 12:50 PM, Jose Catena <jc1(a)diwaves.com> wrote:
>   I'm doing that. Right now I compile and run
> ntoskrnl in msc (vc9). Runs
>  mostly but I'm still fixing bugs.
>  I have written new small platform and compiler dependent includes and
>  eliminate all unnecessary conditionals everywhere else, this cleans up
>  things a lot.
>  But, I asked if my first patch was reported correctly and if I should submit
>  more, received no answer, and it's still not reviewed. I suppose because I
>  am not a respectful enough noob and didn't start kissing asses. So I'm not
>  going to submit any more unless this childish attitude is fixed. But I'll
>  put all my work periodically in my server and announce the availability here
>  so that anyone interested could download it and use whatever you want in
>  ReactOS. 
I think the inverse is true, the more aggressive of an ass you are the
more attention your patches receive. I wish I could review your
patches for you but am more in to project relations and legal issues
than coding but it sounds like you've got the right stuff. Maybe
privately talk to Fireball about review and commit access?

-- 
Steven Edwards

"There is one thing stronger than all the armies in the world, and
that is an idea whose time has come." - Victor Hugo

[Olaf Siejka]

People are kinda... busy. Those skilled enough to have a professional
opinion in your case, are even bit more busy than average. What is even
worse, some of them are temporarily away from the project atm (Stefan100 for
example). It usually helps to ask about your patch status from time to time,
or even better, catch our bugzilla maintainer, Amine, to do some bugging
around.

But sure, you can still accuse us of childish attitude, suspect of delaying
reviews deliberately, requesting ass-kissing etc. Clearly, this is the
professional attitude you wish us to adopt.

Thank you.

On Tue, Jan 12, 2010 at 12:50 PM, Jose Catena <jc1(a)diwaves.com> wrote:

>   But, I asked if my first patch was reported correctly
> and if I should 
submit
>   more, received no answer, and it's still not
> reviewed. I suppose because I
>  am not a respectful enough noob and didn't start kissing asses. So I'm not
>  going to submit any more unless this childish attitude is fixed. But I'll
>  put all my work periodically in my server and announce the availability 
here
>   so that anyone interested could download it and use
> whatever you want in
>  ReactOS.

[Jose Catena]

Amine asked for a review of my patch days ago, not me. And I only referred
to it answering a related post just to report that I did what was being
asked for but will not be submitting it as patches by now and why, but will
make it available to you anyway. 

If indeed the delay is just coz ppl is busy, no problem for me, once the
patch is reviewed and someone tells me if that's the right way to report or
how it should be done instead, I'll report more.

But if that's the case, we also should find a more efficient way soon, or
we'll need years to merge what I did in two weeks, leave alone what comes
next.

If the reason is other, again no problem for me, I save the effort to make
reports. In fact it's much better for me, coz I'm changing a lot of things
and I'd have to spend a lot of time to submit everything, in many cases
probably calling for endless discussions that I don't enjoy when these are
not productive.

I'm not actually complaining for anything! I really don't want to waste my
time nor anyone's else, I just tell what I think once to allow solutions and
proposals for the convenience of the ReactOS project. For me everything is
perfect as is now. I won't write again regarding this unless asked. 

 

Jose Catena

DIGIWAVES S.L.

 

 

From: ros-dev-bounces(a)reactos.org [mailto:[e-mail]] On
Behalf Of Olaf Siejka
Sent: Tuesday, 12 January, 2010 20:00
To: ReactOS Development List
Subject: Re: [ros-dev] [ros-diffs] [sir_richard] 45052: Patch that fixes
VMWare boot (and

 

People are kinda... busy. Those skilled enough to have a professional
opinion in your case, are even bit more busy than average. What is even
worse, some of them are temporarily away from the project atm (Stefan100 for
example). It usually helps to ask about your patch status from time to time,
or even better, catch our bugzilla maintainer, Amine, to do some bugging
around.

 

But sure, you can still accuse us of childish attitude, suspect of delaying
reviews deliberately, requesting ass-kissing etc. Clearly, this is the
professional attitude you wish us to adopt.

 

Thank you.

[Aleksey Bragin]

On Jan 12, 2010, at 8:50 PM, Jose Catena wrote:

> >   Also, if
> > anyone has any experience with MSC++ and could port the 
>  GCC-centric macros, that
> would be much appreciated.
>
>  I'm doing that. Right now I compile and run ntoskrnl in msc (vc9).  
>  Runs
>  mostly but I'm still fixing bugs.
>  I have written new small platform and compiler dependent includes and
>  eliminate all unnecessary conditionals everywhere else, this cleans up
>  things a lot.
>  But, I asked if my first patch was reported correctly and if I  
>  should submit
>  more, received no answer, and it's still not reviewed. I suppose  
>  because I
>  am not a respectful enough noob and didn't start kissing asses. So  
>  I'm not
>  going to submit any more unless this childish attitude is fixed.  
>  But I'll
>  put all my work periodically in my server and announce the  
>  availability here
>  so that anyone interested could download it and use whatever you  
>  want in
>  ReactOS.
>
>  Jose Catena
>  DIGIWAVES S.L. 
Patience, my friend. And respect. There are many different people in  
the project, with different attitude and character. There is no need  
to make everyone your enemy.
Even if it seems that patches are rotting in bugzilla, it's not fully  
true. We already learnt that hurrying to commit improperly reviewed  
patches results in spending 10x time later trying to find the issue  
in a totally unexpected place.

WBR,
Aleksey Bragin.

[Jose Catena]

>   Patience, my friend. And respect. 
I'm sorry if sounded unrespectful. I'm not very good in English, but I think
there's a lot of oversensitivity here? What I wrote could surely be written
in a much sweeter way, but I think that what I intended to say was clear
enough and I don't get how my posts are getting such reactions. 

Jose Catena
DIGIWAVES S.L.

[Javier Agustìn Fernàndez Arroyo]

well, Jose, im just reading this thread, and yes, your mail sounded a bit
rude :)

but don´t worry, Aleksey has replied, so i guess he/anyone is keeping an eye
on your patch

"I won’t write again regarding this unless asked. "
well, i dont think its a bad thing to remember devs there is a patch out
there awaiting :-)
On Tue, Jan 12, 2010 at 9:22 PM, Jose Catena <jc1(a)diwaves.com> wrote:

> >   Patience, my
> > friend. And respect. 
>  I'm sorry if sounded unrespectful. I'm not very good in English, but I
>  think
>  there's a lot of oversensitivity here? What I wrote could surely be written
>  in a much sweeter way, but I think that what I intended to say was clear
>  enough and I don't get how my posts are getting such reactions.
>
>  Jose Catena
>  DIGIWAVES S.L.
>
>
>
>   _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>

[Daniel Reimer]

I personally like it when ppl say what they think. I hate ppl who lie in 
your face pretending to be friendly and as soon as you are not there, 
swear at you.

Jose Catena schrieb:
> >   Patience, my
> > friend. And respect.
> >        
>  I'm sorry if sounded unrespectful. I'm not very good in English, but
> I think
>  there's a lot of oversensitivity here? What I wrote could surely be written
>  in a much sweeter way, but I think that what I intended to say was clear
>  enough and I don't get how my posts are getting such reactions.
>
>  Jose Catena
>  DIGIWAVES S.L.
>
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>

[Javier Agustìn Fernàndez Arroyo]

+1 for that, Daniel

On Tue, Jan 12, 2010 at 9:39 PM, Daniel Reimer <
daniel.reimer(a)stud-mail.uni-wuerzburg.de&gt; wrote:

>   I personally like it when ppl say what they think. I
> hate ppl who lie in
>  your face pretending to be friendly and as soon as you are not there,
>  swear at you.
>
>  Jose Catena schrieb:
> > >   Patience,
> > > my friend. And respect.
> > >  
> >  I'm sorry if sounded unrespectful. I'm not very good in English, but I
>  think
> >   there's a lot of oversensitivity here? What I
> > wrote could surely be 
>  written
> >   in a much sweeter way, but I think that what I
> > intended to say was clear
> >  enough and I don't get how my posts are getting such reactions.
> >
> >  Jose Catena
> >  DIGIWAVES S.L.
> >
> >
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev
> >
> >  
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>