Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Mar 2023 10:57:31 -0700
In-Reply-To: <20230307023946.14516-34-xin3.li@intel.com>
Mime-Version: 1.0
References: <20230307023946.14516-1-xin3.li@intel.com> <20230307023946.14516-34-xin3.li@intel.com>
Message-ID: <ZBtBizw+IuUKwztN@google.com>
Subject: Re: [PATCH v5 33/34] KVM: x86/vmx: call external_interrupt() to
 handle IRQ in IRQ caused VM exits
From:   Sean Christopherson <seanjc@google.com>
To:     Xin Li <xin3.li@intel.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, hpa@zytor.com, peterz@infradead.org,
        andrew.cooper3@citrix.com, pbonzini@redhat.com,
        ravi.v.shankar@intel.com
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Mon, Mar 06, 2023, Xin Li wrote:
> @@ -6923,7 +6924,26 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu)
>  		return;
>  
>  	kvm_before_interrupt(vcpu, KVM_HANDLING_IRQ);
> -	vmx_do_interrupt_irqoff(gate_offset(desc));
> +	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> +		struct vcpu_vmx *vmx = to_vmx(vcpu);
> +		struct pt_regs regs = {};
> +
> +		/*
> +		 * Create an event return stack frame with the
> +		 * host context immediately after a VM exit.

Why snapshot the context immediately after VM-Exit?  It diverges from what is
done in the non-FRED path, and it seems quite misleading and maybe even dangerous.
The RSP and RIP values are long since gone, e.g. if something explodes, the stack
trace will be outright wrong.

> +		 *
> +		 * All other fields of the pt_regs structure are
> +		 * cleared to 0.
> +		 */
> +		regs.ss		= __KERNEL_DS;
> +		regs.sp		= vmx->loaded_vmcs->host_state.rsp;
> +		regs.flags	= X86_EFLAGS_FIXED;
> +		regs.cs		= __KERNEL_CS;
> +		regs.ip		= (unsigned long)vmx_vmexit;
> +
> +		external_interrupt(&regs, vector);

I assume FRED still uses the stack, so why not do something similar to
vmx_do_interrupt_irqoff() and build @regs after an explicit CALL?  Might even
be possible to share some/all of VMX_DO_EVENT_IRQOFF.

> +	} else

Curly braces needed since the first half has 'em.

> +		vmx_do_interrupt_irqoff(gate_offset(desc));
>  	kvm_after_interrupt(vcpu);
>  
>  	vcpu->arch.at_instruction_boundary = true;
> -- 
> 2.34.1
>