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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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.

-boris

> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel