Re: [patch V4 part 2 15/18] x86/kvm/svm: Handle hardirqs proper on guest enter/exit

[Paolo Bonzini]

On 05/05/20 15:41, Thomas Gleixner wrote:
> +	 * VMENTER enables interrupts (host state), but the kernel state is
> +	 * interrupts disabled when this is invoked. Also tell RCU about
> +	 * it. This is the same logic as for exit_to_user_mode().
> +	 *
> +	 * 1) Trace interrupts on state
> +	 * 2) Prepare lockdep with RCU on
> +	 * 3) Invoke context tracking if enabled to adjust RCU state
> +	 * 4) Tell lockdep that interrupts are enabled
> +	 *
> +	 * This has to be after x86_spec_ctrl_set_guest() because that can
> +	 * take locks (lockdep needs RCU) and calls into world and some
> +	 * more.
>  	 */
> +	trace_hardirqs_on_prepare();
> +	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
>  	guest_enter_irqoff();
> +	lockdep_hardirqs_on(CALLER_ADDR0);
>  
>  	__svm_vcpu_run(svm->vmcb_pa, (unsigned long *)&svm->vcpu.arch.regs);
>  
> @@ -3348,14 +3359,23 @@ static void svm_vcpu_run(struct kvm_vcpu
>  	loadsegment(gs, svm->host.gs);
>  #endif
>  #endif
> +
>  	/*
> -	 * Tell context tracking that this CPU is back.
> +	 * VMEXIT disables interrupts (host state, see the CLI in the ASM
> +	 * above),

Apart from the small inaccuracy in that CLI has moved to vmenter.S, the
comments and commit message don't really help my understanding of why
this is needed.

It's true that interrupts cause a vmexit, and therefore from the
processor point of view it's as if they are enabled.  However, the
interrupt remains latched until local_irq_enable() in vcpu_enter_guest,
so from the point of view of the kernel interrupts are still disabled. I
don't understand why it's necessary to inform tracing and lockdep about
a processor-internal state that doesn't percolate up to the kernel.

For VMX indeed some care is necessary, because we the interrupt is eaten
rather than latched.  Therefore, we call the interrupt handler from
handle_external_interrupt_irqoff while EFLAGS.IF is still clear.
However, if informing trace and lockdep turns out to be unnecessary
after all for SVM, it should be okay (and clearer) to place the code in
handle_external_interrupt_irqoff (also in arch/x86/kvm/vmx/vmx.c) .

Instead, if I'm wrong, the four steps above are the same in code and
comment, and same for the three steps in the comment below.  Can you
replace them with the "why" of this change?

Thanks,

Paolo

> +      but tracing and lockdep have them in state 'on'. Same as
> +	 * enter_from_user_mode().
> +	 *
> +	 * 1) Tell lockdep that interrupts are disabled
> +	 * 2) Invoke context tracking if enabled to reactivate RCU
> +	 * 3) Trace interrupts off state
>  	 *
>  	 * This needs to be done before the below as native_read_msr()
>  	 * contains a tracepoint and x86_spec_ctrl_restore_host() calls
>  	 * into world and some more.
>  	 */
> +	lockdep_hardirqs_off(CALLER_ADDR0);
>  	guest_exit_irqoff();
> +	trace_hardirqs_off_prepare();
>