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;
From:   "Li, Xin3" <xin3.li@intel.com>
To:     Peter Zijlstra <peterz@infradead.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "Tian, Kevin" <kevin.tian@intel.com>
Subject: RE: [RESEND PATCH 5/6] KVM: x86/VMX: add kvm_vmx_reinject_nmi_irq()
 for NMI/IRQ reinjection
Thread-Topic: [RESEND PATCH 5/6] KVM: x86/VMX: add kvm_vmx_reinject_nmi_irq()
 for NMI/IRQ reinjection
Thread-Index: AQHY9M9YytlyKV7ob0GK/oJbeBhPY6433PwAgACmfxA=
Date:   Thu, 10 Nov 2022 20:53:30 +0000
Message-ID: <BN6PR1101MB2161976800EB14B74A24D9F3A8019@BN6PR1101MB2161.namprd11.prod.outlook.com>
References: <20221110061545.1531-1-xin3.li@intel.com>
 <20221110061545.1531-6-xin3.li@intel.com>
 <Y2y+YgBUYuUHbPtd@hirez.programming.kicks-ass.net>
In-Reply-To: <Y2y+YgBUYuUHbPtd@hirez.programming.kicks-ass.net>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.6.500.17
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?nhp6Enmg6dmIJqQMDYUx6CvAjopgGVzxc3Jj4pW1C4usRzMhMsPwzhArzk9n?=
 =?us-ascii?Q?MAjRZH1a4RC3TWPGBwiYpVLm0dALeQD60PEtAqThcXmuIq6AnXqm0qcLOiW3?=
 =?us-ascii?Q?NndjbbMfzzPnHjSS8/ZKp3fB5ou/GN8CKCNrwStZ47+U/5ArFKseAyRMsQFb?=
 =?us-ascii?Q?bJVICURFX6ab/mmwZhusFWJvSdaiUnW7ul2EiQH1Fgr/74zEoz4RJNSKMU8M?=
 =?us-ascii?Q?DcumvjM0TiuJ47L8bMk1dgNIFYjcvwZftVNFp51uBL5UqgGVAOeDOT7/GQbP?=
 =?us-ascii?Q?ol3HVls2epOL2Sgyq2r5rONbaKE9aZq/o985K/89cEj0cVp7RqSqir3rrQ8p?=
 =?us-ascii?Q?xR8wNI2e96GYjc9+318YAwvyzFfnAu+ZzfuPDKr7Sso2ta6qMNX5TrGqc+Cb?=
 =?us-ascii?Q?ymt1xE4VCcq98bLV5y2mcaM8fHvj2eoYecLfFCpKGMhv7Nh1myWxCFn/krQc?=
 =?us-ascii?Q?WOWlUfMAeNntAMATonzNcTlY81VTp6swBYEisgRElkv2mhAQwrdMpZVeKEbg?=
 =?us-ascii?Q?H7bQMWuST2tKXYbGob9lGXaXc4MT48jwdznw2FkHd4f4QDDYuvIzwLO/np4E?=
 =?us-ascii?Q?Oi8Hk/yJ+7M9V2KNfJ1fWBfn5CsCJF8EXwNwRMDLsCtwGfYnjbPMQYMqXBc2?=
 =?us-ascii?Q?jgGUJV+DpXzLJImNIsIXM+kbPL0DTC+dzHQIjw/8ixQ4++RVvt/t/w3BLTKF?=
 =?us-ascii?Q?mosr4yGFtHSibWBizRqqQqNGZDjhFLaxDXmwTR4qRMKzKlp1QbR9jJadri0N?=
 =?us-ascii?Q?6M2hvvBS5CBcuQ09SwE4VUntzduysxrBSQR/tSU6arV/ujp9FYgPpKBGjMuW?=
 =?us-ascii?Q?sTlPCvRqxYwzC55IsKNvFqq+lpd/2/IgXrw7J85brk3T01TaqcBolrkcPMCU?=
 =?us-ascii?Q?XvKJK2/sfEN1+7lnynvMT1fgbvzLPofn46JPynP2jGrBFppkMx4ANKt89eQa?=
 =?us-ascii?Q?aBhrn7KQVl8KJ5OraIqQPZxnKOM4MAg34Xgmn61RCZIYl0HdkHyadRHWXErA?=
 =?us-ascii?Q?kOAjynVl7yzxCOHwrsH0mQcndWAv+J1+GDFEtgmNhyhhjm2UTpZvG1GWTg+6?=
 =?us-ascii?Q?mqY9k+lKQ0PJXbVmc6BMm/AbUAQ6PIfRQcvE+/04njwyldhe/coxB+MrK8o2?=
 =?us-ascii?Q?b3cKLh5imWTQLTcTXpNTsxxbLb8eKGFobuqjPwhgfVlgpciCfU372ErIUvPa?=
 =?us-ascii?Q?IoM7YYSNY17VWUGwQVzOiMOFvL4kMWpkXTEcDf7XCxdyjYlOeElzCV7JIES/?=
 =?us-ascii?Q?c9AqM54SIOv+mw1+CZ8+JOVGW3qPnua/2E223ETE65xka1EPXq1iiTK2DjzM?=
 =?us-ascii?Q?5PfmG92RLQDOUbwJcyzKloBJCyKNuXocs6kaOSotfK8SO3hcVnL8S9W2VLi+?=
 =?us-ascii?Q?O1qZu9iSeTz5BgnEkEl3ruz2trVKyqDouxFbaTpeQsZ0CVO/Q3MLhAHPbVJo?=
 =?us-ascii?Q?+jnvejq/90XVzycCUd4dDOe/KdXxlE/VOOVx8N3S9KCG6Hju1NyPT+Wls+B/?=
 =?us-ascii?Q?Z6Gbytxaq5FUXpIJ/9bfuCWypkSJh6mtdYG4pNy5IyGwM3r1mqyA2Cxfwda1?=
 =?us-ascii?Q?2pVvyNuy9BpSVC2s8DM=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN6PR1101MB2161.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e13389eb-d1e8-47ad-79c1-08dac35d9f8d
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Nov 2022 20:53:30.2257
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: AbjhiAA95iZzjBjKT48Xkp82RmUgjK/ZVrSmuFzNxORk+oPvCQkCxJVzdelHzJ5iWwCRqBr63AyeR7bNjFXefw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6162
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > To eliminate dispatching NMI/IRQ through the IDT, add
> > kvm_vmx_reinject_nmi_irq(), which calls external_interrupt() for IRQ
> > reinjection.
> >
> > Lastly replace calling a NMI/IRQ handler in an IDT descriptor with
> > calling kvm_vmx_reinject_nmi_irq().
> >
> > Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> > Signed-off-by: Xin Li <xin3.li@intel.com>
>=20
> Idem.
>=20
>=20
> > +#if IS_ENABLED(CONFIG_KVM_INTEL)
> > +/*
> > + * KVM VMX reinjects NMI/IRQ on its current stack, it's a sync
> > + * call thus the values in the pt_regs structure are not used in
> > + * executing NMI/IRQ handlers, except cs.RPL and flags.IF, which
> > + * are both always 0 in the VMX NMI/IRQ reinjection context. Thus
> > + * we simply allocate a zeroed pt_regs structure on current stack
> > + * to call external_interrupt().
> > + */
> > +void kvm_vmx_reinject_nmi_irq(u32 vector)
>=20
> noinstr ?

Seems it's not needed to me.

>=20
> > +{
> > +	struct pt_regs irq_regs;
> > +
> > +	memset(&irq_regs, 0, sizeof(irq_regs));
> > +
> > +	if (vector =3D=3D NMI_VECTOR)
> > +		return exc_nmi(&irq_regs);
> > +
> > +	external_interrupt(&irq_regs, vector); }
> > +EXPORT_SYMBOL_GPL(kvm_vmx_reinject_nmi_irq);
> > +#endif
>=20
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index
> > 63247c57c72c..b457e4888468 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -46,6 +46,7 @@
> >  #include <asm/mshyperv.h>
> >  #include <asm/mwait.h>
> >  #include <asm/spec-ctrl.h>
> > +#include <asm/traps.h>
> >  #include <asm/virtext.h>
> >  #include <asm/vmx.h>
> >
> > @@ -6758,15 +6759,11 @@ static void vmx_apicv_post_state_restore(struct
> kvm_vcpu *vcpu)
> >  	memset(vmx->pi_desc.pir, 0, sizeof(vmx->pi_desc.pir));  }
> >
> > -void vmx_do_interrupt_nmi_irqoff(unsigned long entry);
> > -
> > -static void handle_interrupt_nmi_irqoff(struct kvm_vcpu *vcpu,
> > -					unsigned long entry)
> > +static void handle_interrupt_nmi_irqoff(struct kvm_vcpu *vcpu, u32
> > +vector)
> >  {
> > -	bool is_nmi =3D entry =3D=3D (unsigned long)asm_exc_nmi_noist;
> > -
> > -	kvm_before_interrupt(vcpu, is_nmi ? KVM_HANDLING_NMI :
> KVM_HANDLING_IRQ);
> > -	vmx_do_interrupt_nmi_irqoff(entry);
> > +	kvm_before_interrupt(vcpu, vector =3D=3D NMI_VECTOR ?
> > +				   KVM_HANDLING_NMI :
> KVM_HANDLING_IRQ);
> > +	kvm_vmx_reinject_nmi_irq(vector);
> >  	kvm_after_interrupt(vcpu);
> >  }
> >
> > @@ -6792,7 +6789,6 @@ static void handle_nm_fault_irqoff(struct
> > kvm_vcpu *vcpu)
> >
> >  static void handle_exception_nmi_irqoff(struct vcpu_vmx *vmx)  {
> > -	const unsigned long nmi_entry =3D (unsigned long)asm_exc_nmi_noist;
> >  	u32 intr_info =3D vmx_get_intr_info(&vmx->vcpu);
> >
> >  	/* if exit due to PF check for async PF */ @@ -6806,20 +6802,19 @@
> > static void handle_exception_nmi_irqoff(struct vcpu_vmx *vmx)
> >  		kvm_machine_check();
> >  	/* We need to handle NMIs before interrupts are enabled */
> >  	else if (is_nmi(intr_info))
> > -		handle_interrupt_nmi_irqoff(&vmx->vcpu, nmi_entry);
> > +		handle_interrupt_nmi_irqoff(&vmx->vcpu, NMI_VECTOR);
> >  }
> >
> >  static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu)
> > {
> >  	u32 intr_info =3D vmx_get_intr_info(vcpu);
> >  	unsigned int vector =3D intr_info & INTR_INFO_VECTOR_MASK;
> > -	gate_desc *desc =3D (gate_desc *)host_idt_base + vector;
> >
> >  	if (KVM_BUG(!is_external_intr(intr_info), vcpu->kvm,
> >  	    "KVM: unexpected VM-Exit interrupt info: 0x%x", intr_info))
> >  		return;
> >
> > -	handle_interrupt_nmi_irqoff(vcpu, gate_offset(desc));
> > +	handle_interrupt_nmi_irqoff(vcpu, vector);
> >  	vcpu->arch.at_instruction_boundary =3D true;  }
>=20
> How does any of this work? You're calling into entry/noinstr code from a
> random context.

Can you please elaborate your concern a bit more?

We are here in handle_external_interrupt_irqoff () because an external
interrupt happened when a guest was running and the CPU vm-exits to host
to dispatch to the IRQ handler with IRQ disabled.