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:     "Christopherson,, Sean" <seanjc@google.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        "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>,
        "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/oJbeBhPY6433PwAgACmfxCAAO8aAIAAL0qAgAAENoCAAAgUAIAAGnuAgAA1qHCAACERAIADuBAAgABQYICAAXpoEIAAGnIAgAK/reCAANL/AIAAhVlg
Date:   Fri, 18 Nov 2022 00:05:53 +0000
Message-ID: <BN6PR1101MB21611347D37CF40403B974EDA8099@BN6PR1101MB2161.namprd11.prod.outlook.com>
References: <Y24908NWCdzUNqI0@hirez.programming.kicks-ass.net>
 <6fd26a70-3774-6ae7-73ea-4653aee106f0@redhat.com>
 <Y25a0Z2tOMWYZs4j@hirez.programming.kicks-ass.net>
 <BN6PR1101MB216141A21353AB84CEA541DFA8009@BN6PR1101MB2161.namprd11.prod.outlook.com>
 <Y26jkHfK9INwU7Yy@hirez.programming.kicks-ass.net>
 <BN6PR1101MB2161E8217F50D18C56E5864EA8059@BN6PR1101MB2161.namprd11.prod.outlook.com>
 <Y3IFo9NrAcYalBzM@hirez.programming.kicks-ass.net>
 <BN6PR1101MB2161299749E12D484DE9302BA8049@BN6PR1101MB2161.namprd11.prod.outlook.com>
 <Y3NZQBJugRt07udw@hirez.programming.kicks-ass.net>
 <DM5PR1101MB2172D7D7BC49255DB3752802A8069@DM5PR1101MB2172.namprd11.prod.outlook.com>
 <Y3ZYiKbJacmejY3K@google.com>
In-Reply-To: <Y3ZYiKbJacmejY3K@google.com>
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?aB3THmhxR+k9//7OJ/fg3wFgDv6deCId7u3GJ59TcS8kue5Bd+gM1GY1b578?=
 =?us-ascii?Q?utIpto11FhAArTEdbyUT2B28fG/zIucdzLpJAKhdoeA+yyE019PyHO9hdufO?=
 =?us-ascii?Q?Va80osTR8cJo3+rp5F0i7F9uAtbIuGr33pZsDGbLBVNVayXgZEUJLaz55WjD?=
 =?us-ascii?Q?Xl/l3Du1MlRZv+s22HmkWLJN0vHvK/6BJeEH0jIQawYcX/octoFg+PcOUY8P?=
 =?us-ascii?Q?9DzBCIAQ/rcw1OilwHnP6/nmGgRUPT7wc8cw6289plO/FcaCwy0xf5ZGHlfD?=
 =?us-ascii?Q?T9iJ7Ceqb1ivA+hDBtb4zT3Ym7rUvlA2Gx/5mcxheJPlP7qaedQ59DZQhU9E?=
 =?us-ascii?Q?L5zUSFriT+/clgJLMd25Hx/l7EW0H+RnAAF6NTe56OAriqs1BEOFKJtoxFKT?=
 =?us-ascii?Q?Q6MD+VJG/cx0fidZGsKCBWfQeFnPGYxx7oRJResdh27oe2K2ch7j47Ax41Xm?=
 =?us-ascii?Q?8NxjFPOgGUspgsp1EZ+hfuI4jg4RPissQ0XMuXDW//x3e/nOdYlb6IHMpduf?=
 =?us-ascii?Q?g8f6fmkhUnUTq01dCABzA98ZwTczfOjvvjKJmrHkRUFeJph9weJMqbZ0Yu4r?=
 =?us-ascii?Q?kFYS7r88Roh9ccOL+qSInrZwpFrtxYHcwmZJSDEYz5Q7bnf9hWo1SSjf0CvY?=
 =?us-ascii?Q?GZYYFv5Nkc2ufud/W2nVaRsCobAQ7h/NsBKJB+8DG9FpdQdsXcuUKGwCbxoo?=
 =?us-ascii?Q?JWZaMu9N1EoqKXvFmpQvN9uV3ltzxrWRxBCDvU1W0OnJs7DwsEdMgd+zGrfF?=
 =?us-ascii?Q?6UN2IN55JXeidC6BP88cekKDoCFrqWdjkiEwTMx7SNA2X12tpZXBLawYCVZ3?=
 =?us-ascii?Q?cKMwBKsPa9wihXLKTr45E10pkSW+O1Z7eM3gg6bBGWdozQLCr7+zn6evz9o2?=
 =?us-ascii?Q?QHfg/+hWhYqaqXHcypth/MeEa+2hk5SLLm45dNLvU/jpRv0jh9/Devs6D+I6?=
 =?us-ascii?Q?EC2oxIV5YgvB7S00DmKViV/ssQTFbqSvchMslitPPIqvrIOcOniXHeJJo1fh?=
 =?us-ascii?Q?HzrNvUkuM63SBYhixzsPUBnikcs6+I2uF7IX53EGO0x9NrEGdeMvaW97yE60?=
 =?us-ascii?Q?7ssLWumsuVOMIEv6ypt1SwdnshwgJQQ8RYI2PwFVPQdsV6ZH/3INhf0kTnxf?=
 =?us-ascii?Q?dkm98BnWZT8q+nsiWwJt6jEi1MQm3SVZgUquUVGY46XqSTHcErqXrJR6mmXA?=
 =?us-ascii?Q?CM7QL85ufKn7ZYfsjJP0fpHJrNHsUIquDT1NtoKRMVnX4AJDCY5L4VKK7eJc?=
 =?us-ascii?Q?KKdx9rr6mk8+9t21Nmh4bcN1nH21+nIfi2VpdzAvLWA5nyo0+sUkvRAe68Ii?=
 =?us-ascii?Q?eTMSBdrB1ai9y+1jL1XwRumxxzY+NK+RgtYDB7mMcLLDGWaIJG6L1G3nrok2?=
 =?us-ascii?Q?P7+wg8C0aH6v7P7yogaG1bHgo1LbgvGUglq3CjS4xWEqbfL2d27K/waGfd7K?=
 =?us-ascii?Q?m/rV7ifNFZnmJFMZ/OPcZ3whkm3TjeuK5xDkTr12QGgQOrOTfKD8AmvMEDvH?=
 =?us-ascii?Q?ZQde8tDCV9v5uZzpWCovafLP3hngAgS05FLJCwvsiyHZnU/JlWNQ80dg97CR?=
 =?us-ascii?Q?R50czSiB7JGWB5nDEeo=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: 2374ee2e-c3b0-469a-fd68-08dac8f8a8d4
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Nov 2022 00:05:53.5853
 (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: AW3Odj5t1Gbjf5kG0960FKxqWjAWjcJ24qYcdRmcFujbhBxct8NWQTvbYJwQMvRgxQqZLstmkVXs7GemeweZOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5164
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

> > > > > > > But what about NMIs, afaict this is all horribly broken for N=
MIs.
> > > > > > > So the whole VMX thing latches the NMI (which stops NMI
> > > > > > > recursion), right?
> > > > > > >
> > > > > > > But then you drop out of noinstr code, which means any
> > > > > > > random exception can happen (kprobes #BP, hw_breakpoint #DB,
> > > > > > > or even #PF due to random nonsense like *SAN). This
> > > > > > > exception will do IRET and clear the NMI latch, all before
> > > > > > > you get to run any of the NMI code.
> > > > > >
> > > > > > What you said here implies that we have this problem in the exi=
sting
> code.
> > > > > > Because a fake iret stack is created to call the NMI handler
> > > > > > in the IDT NMI descriptor, which lastly executes the IRET instr=
uction.
> > > > >
> > > > > I can't follow; of course the IDT handler terminates with IRET, i=
t has to
> no?
> > > > >
> > > > > And yes, the current code appears to suffer the same defect.
>=20
> That defect isn't going to be fixed simply by changing how KVM forwards N=
MIs
> though.  IIUC, _everything_ between VM-Exit and the invocation of the NMI
> handler needs to be noinstr.  On VM-Exit due to NMI, NMIs are blocked.  I=
f a
> #BP/#DB/#PF occurs before KVM gets to kvm_x86_handle_exit_irqoff(), the
> subsequent IRET will unblock NMIs before the original NMI is serviced, i.=
e. a
> second NMI could come in at anytime regardless of how KVM forwards the NM=
I
> to the kernel.
>=20
> Is there any way to solve this without tagging everything noinstr?  There=
 is a
> metric shit ton of code between VM-Exit and the handling of NMIs, and muc=
h
> of that code is common helpers.  It might be possible to hoist NMI handle=
r
> much earlier, though we'd need to do a super thorough audit to ensure all
> necessary host state is restored.

As NMI is the only vector with this potential issue, it sounds a good idea
to only promote its handling.


> > > > With FRED, ERETS/ERETU replace IRET, and use bit 28 of the popped
> > > > CS field to control whether to unblock NMI. If bit 28 of the field
> > > > (above the selector) is 1, ERETS/ERETU unblocks NMIs.
>=20
> Side topic, there's a bug in the ISE docs.  Section "9.4.2 NMI Blocking" =
states
> that bit 16 holds the "unblock NMI" magic, which I'm guessing is a holdov=
er
> from an earlier revision of FRED.

Good catch, the latest 3.0 draft spec changed it to bit 28, but section 9.4=
.2
didn't get a proper update.

>=20
>   As specified in Section 6.1.3 and Section 6.2.3, ERETS and ERETU each
> unblocks NMIs
>   if bit 16 of the popped CS field is 1. The following items detail how t=
his
> behavior may be
>   changed in VMX non-root operation, depending on the settings of certain=
 VM-
> execution
>   controls:
>=20
> > > Yes, I know that. It is one of the many improvements FRED brings.
> > > Ideally the IBT WAIT-FOR-ENDBR state also gets squirreled away in
> > > the hardware exception frame, but that's still up in the air I
> > > believe :/
> > >
> > > Anyway.. given there is interrupt priority and NMI is pretty much on
> > > top of everything else the reinject crap *should* run NMI first.
> > > That way NMI runs with the latch disabled and whatever other pending
> interrupts will run later.
> > >
> > > But that all is still broken because afaict the current code also
> > > leaves noinstr -- and once you leave noinstr (or use a static_key,
> > > static_call or anything else that
> > > *can* change at runtime) you can't guarantee nothing.
> >
> > For NMI, HPA asked me to use "int $2", as it switches to the NMI IST
> > stack to execute the NMI handler, essentially like how HW deals with a
> > NMI in host. and I tested it with NMI watchdog, it looks working fine.
>=20
> Heh, well yeah, because that's how KVM used to handle NMIs back before I
> reworked NMI handling to use the direct call method.  Ironically, that or=
iginal
> change was done in part to try and make it _easier_ to deal with FRED (ba=
ck
> before FRED was publicly disclosed).
>=20
> If KVM reverts to INTn, the fix to route KVM=3D>NMI through the non-IST e=
ntry
> can be reverted too.
>=20
>   a217a6593cec ("KVM/VMX: Invoke NMI non-IST entry instead of IST entry")
>   1a5488ef0dcf ("KVM: VMX: Invoke NMI handler via indirect call instead o=
f
> INTn")

Sure, I'm just thinking to put asm("int $2") in a new function exc_raise_nm=
i()
defined in arch/x86/kernel/traps.c. Thus we will need to change it only
whenever we have any better facility, and no KVM VMX change required.