On Thu, Jul 18, 2013 at 02:08:51PM +0200, Paolo Bonzini wrote:
> Il 18/07/2013 13:06, Gleb Natapov ha scritto:
> > On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote:
> >>>> and for a testsuite I'd prefer the latter---which means I'd still favor
> >>>> setjmp/longjmp.
> >>>>
> >>>> Now, here is the long explanation.
> >>>>
> >>>> I must admit that the code looks nice. There are some nits I'd like to
> >>>> see done differently (such as putting vmx_return at the beginning of the
> >>>> while (1), and the vmresume asm at the end), but it is indeed nice.
> >>>
> >>> Why do you prefer setjmp/longjmp then?
> >>
> >> Because it is still deceiving, and I dislike the deceit more than I like
> >> the linear code flow.
> >>
> > What is deceiving about it? Of course for someone who has no idea how
> > vmx works the code will not be obvious, but why should we care. For
> > someone who knows what is deceiving about returning into the same
> > function guest was launched from by using VMX mechanism
>
> The way the code is written is deceiving. If I see
>
> asm("vmlaunch; seta %0")
> while (ret)
>
> I expect HOST_RIP to point at the seta or somewhere near, not at a
> completely different label somewhere else.
>
Why would you expect that assuming you know what vmlaunch is? So it is OK
when HOST_RIP point to somewhere outside a function, but "deceiving" if
it point 3 lines down in the same function? I have a hard time following
this logic.
> >> instead of longjmp()?
>
> Look again at the setjmp/longjmp version. longjmp is not used to handle
> vmexit. It is used to jump back from the vmexit handler to main, which
> is exactly what setjmp/longjmp is meant for.
>
That's because simple return will not work in that version, this is
artifact of how vmexit was done.
> >> It is still somewhat magic: the "while (ret)" is only there to please
> >> the compiler
> >
> > No, it it there to catch vmlaunch/vmresume errors.
>
> You could change it to "while (0)" and it would still work. That's what
> I mean by "only there to please the compiler".
But while (1) will not, so the code is executed, it is not just for
compiler there, but it is executed only if vmlaunch/vmresume fails.
>
> >> the compiler, and you rely on the compiler not changing %rsp between the
> >> vmlaunch and the vmx_return label. Minor nit, you cannot anymore print
> > HOST_RSP should be loaded on each guest entry.
>
> Right, but my point is: without a big asm blob, you don't know the right
> value to load. It depends on the generated code. And the big asm blob
> limits a lot the "code looks nice" value of this approach.
>
I said it number of times already, this is not about "code looking nice",
"code looks like KVM" or use less assembler as possible", this is about
linear data flow. It is not fun chasing code execution path. Yes, you
can argue that vmlaunch/vmresume inherently non linear, but there is a
difference between skipping one instruction and remain in the same
function and on the same stack, or jump completely to a different
context.
Speaking about KVM. Guest enter/exit assembly blob is around ~50 lines
if assembly code and more then half of that is saving restoring context.
And the code plays some tricks there for optimization that we do not
need to do here, so I expect the code to be even smaller, not much more
then 10 lines of assembly excluding state save/restore.
> >> different error messages for vmlaunch vs. vmresume failure.
> > Just because the same variable is used to store error values :)
> > Make vmlaunch use err1 and vmresume err2 and do "while (!err1 & !err2)"
>
> Yeah. :)
>
> >> In the end the choice is between "all in asm" and "small asm trampoline"
> >> (which also happens to use setjmp/longjmp, but perhaps Arthur can
> >> propagate return codes without using setjmp/longjmp, too).
> >>
> >>> Rewriting the whole guest entry exit path in asm like you suggest bellow
> >>> will eliminate the magic too.
> >>
> >>> I much prefer one big asm statement than many small asm statement
> >>> scattered among two or three C lines.
> >>
> >> It's not many asm statements, it's just a dozen lines:
> >>
> > I am not talking about overall file, but the how vmx_run() is written:
> > asm()
> > C code
> > asm()
> > C code
> > ...
> >
> > I much prefer:
> > C code
> >
> > big asm() blob
> >
> > C code.
>
> Me too. But the trampoline version is neither, it is
>
> static inline void vmlaunch() { asm("vmlaunch") }
> static inline void vmresume() { asm("vmresume") }
> small asm() blob
I is small only because context save restore is hidden behind
macro and 4 asm instructions (vmlaunch/seta/vmresume/seta) a hidden
somewhere else. The actually differences in asm instruction between both
version will not be bigger then a couple of lines (if we will not take
setjmp/longjmp implementation into account :)), but I do not even see
why we discuss this argument since minimizing asm instructions here is
not an point. We should not use more then needed to achieve the goal of
course, but design should not be around number of assembly lines.
> C code
>
> Paolo
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
