On Thu, Jul 18, 2013 at 8:08 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> 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.
Here for me, I prefer a separate asm blob of entry_vmx instead of one
"hidden" in a C function, which may make it much harder to trace for
someone not familiar with vmx in KVM.
>
>>> 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.
>
>>> 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".
>
>>> 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.
>
>>> 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
> C code
So this is a style problem on the basis of right code generation
indeed. When I write codes of this version, it occurs that the
compiler optimized some of my codes and something goes wrong. If we
use C style, we need setjmp/longjmp, otherwise we need big asm blob to
forbid compiler optimization.
I prefer to Paolo indeed as to my writing the two versions. Writing
asm in C is sometimes uncomfortable (e.g. %rax and %%rax, and
considering the C codes before and after the asm blob). Actually, we
can hide setjmp in vmx_on and longjmp in the asm codes after executing
exit_handler, thus we just need to call vmx_on to enter VM and return
a designed value (e.g. -1) in exit_handler to exit VM. Then any
following codes don't need to care about setjmp/longjmp.
Arthur
>
> Paolo
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
