Re: [Xen-devel] [PATCH] SVM: limit GIF=0 region

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/06/2018 08:32, Jan Beulich wrote:
> Use EFLAGS.IF for all ordinary purposes; there's in particular no need
> to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
> has the additional advantage that svm_stgi_label now indeed marks the
> only place where GIF is being set.
>
> A note regarding the main STI placement: Orignally I had it at the place
> the main STGI was sitting at so far. However, my Fam15 box reliably
> locks up hard with this, unless I have the NMI watchdog enabled. I can
> only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
> some other condition (the lockup occurs only after exiting the boot
> loader in the guest). As there's nothing wrong with interrupts being on
> right after VMRUN, I've decided to put the STI right after the CLGI
> (matching what KVM does, i.e. having a fair chance of working
> everywhere).

I'd like some input from AMD on this observation, because it is
completely bizarre.

>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -43,7 +43,7 @@ ENTRY(svm_asm_do_resume)
>          lea  irq_stat+IRQSTAT_softirq_pending(%rip),%rdx
>          xor  %ecx,%ecx
>          shl  $IRQSTAT_shift,%eax
> -        CLGI
> +        cli
>          cmp  %ecx,(%rdx,%rax,1)
>          jne  .Lsvm_process_softirqs
>  
> @@ -57,7 +57,7 @@ UNLIKELY_START(ne, nsvm_hap)
>           * Someone shot down our nested p2m table; go round again
>           * and nsvm_vcpu_switch() will fix it for us.
>           */
> -        STGI
> +        sti
>          jmp  .Lsvm_do_resume
>  __UNLIKELY_END(nsvm_hap)
>  
> @@ -87,7 +87,11 @@ __UNLIKELY_END(nsvm_hap)
>          pop  %rsi
>          pop  %rdi
>  
> +        CLGI

As an observation, I'm going to need to insert a call to C here, to fix
LBR handling on pre-LBR-virt capable hardware.  Yet another of the many
many broken things with debug handling.

> +        sti
>          VMRUN
> +        STGI
> +GLOBAL(svm_stgi_label)
>  
>          SAVE_ALL
>  
> @@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: 
> acd */
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>  
> -        STGI
> -GLOBAL(svm_stgi_label)

Unfortunately, the stgi label must remain here. 
SPEC_CTRL_ENTRY_FROM_HVM depends on NMIs being blocked to avoid
corrupting guest state.

Otherwise, LGTM.

~Andrew

>          mov  %rsp,%rdi
>          call svm_vmexit_handler
>          jmp  .Lsvm_do_resume
>  
>  .Lsvm_process_softirqs:
> -        STGI
> +        sti
>          call do_softirq
>          jmp  .Lsvm_do_resume
>
>
>
>


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