Re: [Xen-devel] [PATCH v3 2/2] SVM: introduce a VM entry helper

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/05/18 16:46, Boris Ostrovsky wrote:
> On 05/07/2018 11:29 AM, Andrew Cooper wrote:
>> On 07/05/18 16:25, Jan Beulich wrote:
>>>>>> On 07.05.18 at 16:19, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 07/05/18 15:11, Jan Beulich wrote:
>>>>>>>> On 04.05.18 at 17:11, <JBeulich@xxxxxxxx> wrote:
>>>>>> --- a/xen/arch/x86/hvm/svm/entry.S
>>>>>> +++ b/xen/arch/x86/hvm/svm/entry.S
>>>>>> @@ -61,23 +61,8 @@ UNLIKELY_START(ne, nsvm_hap)
>>>>>>          jmp  .Lsvm_do_resume
>>>>>>  __UNLIKELY_END(nsvm_hap)
>>>>>>  
>>>>>> -        call svm_asid_handle_vmrun
>>>>>> -
>>>>>> -        cmpb $0,tb_init_done(%rip)
>>>>>> -UNLIKELY_START(nz, svm_trace)
>>>>>> -        call svm_trace_vmentry
>>>>>> -UNLIKELY_END(svm_trace)
>>>>>> -
>>>>>> -        mov  VCPU_svm_vmcb(%rbx),%rcx
>>>>>> -        mov  UREGS_rax(%rsp),%rax
>>>>>> -        mov  %rax,VMCB_rax(%rcx)
>>>>>> -        mov  UREGS_rip(%rsp),%rax
>>>>>> -        mov  %rax,VMCB_rip(%rcx)
>>>>>> -        mov  UREGS_rsp(%rsp),%rax
>>>>>> -        mov  %rax,VMCB_rsp(%rcx)
>>>>>> -        mov  UREGS_eflags(%rsp),%rax
>>>>>> -        or   $X86_EFLAGS_MBS,%rax
>>>>>> -        mov  %rax,VMCB_rflags(%rcx)
>>>>>> +        mov  %rsp, %rdi
>>>>>> +        call svm_vmenter_helper
>>>>> While I had committed this earlier today, there's one concern I've just 
>>>>> come
>>>>> to think of: Now that we're calling into C land with CLGI in effect (for 
>>>> more
>>>>> than just the trivial svm_trace_vmentry()) we are at risk of confusing
>>>>> parties using local_irq_is_enabled(), first and foremost
>>>>> common/spinlock.c:check_lock(). While it's some extra overhead, I wonder
>>>>> whether the call wouldn't better be framed by a CLI/STI pair.
>>>> I can't see why the SVM vmentry path uses CLGI/STGI in the first place.
>>>>
>>>> The VMX path uses plain cli/sti and our NMI/MCE handlers can cope. 
>>>> Furthermore, processing NMIs/MCEs at this point will be more efficient
>>>> that taking a vmentry then immediately exiting again.
>>> Perhaps you're right, i.e. we could replace all current CLGI/STGI by
>>> CLI/STI, adding a single STGI right after VMRUN.
>
> The APM say "It is assumed that VMM software cleared GIF some time before
> executing the VMRUN instruction, to ensure an atomic state switch."
>
> Not sure if this is meant as suggestion or requirement.

Hmm - that can probably be tested with this proposed patch and a very
high frequency NMI perf counter.

Basically every other hypervisor does CLGI; VMSAVE (host state); VMLOAD
(guest state); VMRUN, and Xen's lack of doing this is why we have to
play with the IDT IST settings, as well as why we can't cope cleanly
with stack overflows.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel