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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/05/18 16:25, Jan Beulich wrote:
>>>> On 07.05.18 at 16:19, <andrew.cooper3@citrix.com> wrote:
>> On 07/05/18 15:11, Jan Beulich wrote:
>>>>>> On 04.05.18 at 17:11, <JBeulich@suse.com> 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.

We want to retain the one STGI on the svm_stgi_label, but I think all
other CLGI/STGI should be downgraded to CLI/STI.

>
>> As for running with interrupts disabled, that is already the case on the
>> VMX side, and I don't see why the SVM side needs to be different.
> Sure, as does SVM - CLGI is a superset of CLI, after all. My observation
> was just that this state of interrupts being disabled can't be observed by
> users of the normal infrastructure (inspecting EFLAGS.IF).

Ah ok.

~Andrew

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