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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/07/2018 11:49 AM, Andrew Cooper wrote:
> 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.


This may only prove the we do need it, if the test without CLGI fails.

If the test passes I don't think we can say anything one way or the other.

I am adding Suravee and Brian, perhaps they know the answer (or can
check internally).


>
> 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.
>

KVM manipulates both GIF and RFLAGS.IF.

-boris

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