Date:   Mon, 13 Mar 2023 11:40:50 -0700
In-Reply-To: <4ab086ad30cf5e1ddea863e1a2bcfa21ca72d37e.camel@intel.com>
Mime-Version: 1.0
References: <20230310214232.806108-1-seanjc@google.com> <20230310214232.806108-8-seanjc@google.com>
 <4ab086ad30cf5e1ddea863e1a2bcfa21ca72d37e.camel@intel.com>
Message-ID: <ZA9t04+rtW22AMQq@google.com>
Subject: Re: [PATCH v2 07/18] x86/reboot: Disable virtualization during reboot
 iff callback is registered
From:   Sean Christopherson <seanjc@google.com>
To:     Kai Huang <kai.huang@intel.com>
Cc:     "tglx@linutronix.de" <tglx@linutronix.de>,
        "x86@kernel.org" <x86@kernel.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Chao Gao <chao.gao@intel.com>,
        "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 13, 2023, Huang, Kai wrote:
> On Fri, 2023-03-10 at 13:42 -0800, Sean Christopherson wrote:
> > Attempt to disable virtualization during an emergency reboot if and onl=
y
> > if there is a registered virt callback, i.e. iff a hypervisor (KVM) is
> > active.  If there's no active hypervisor, then the CPU can't be operati=
ng
> > with VMX or SVM enabled (barring an egregious bug).
>=20
> IIUC, this patch is the final one that you want to achieve how the "disab=
le
> virtualization" callback should work in the non-KVM core kernel (the rest
> patches are related to moving VMXOFF code to KVM as the core-kernel now j=
ust
> calls the callback, etc). =EF=BF=BD
>=20
> There are middle step patches (2-7) to eventually help to get to this poi=
nt.=20
> But to be honest, personally, I am not sure whether those patches are nec=
essary,
> i.e. to me they actually cost more time to review since I have to think w=
hether
> such intermediate status is reasonable or not.  I am wondering whether we=
 can
> just merge those patches together as single one, so it's easy to see what=
 is the
> final goal to achieve?

I agree that the fine granularity makes it difficult to see the final form,=
 but
from a bisection perspective I really, really want each change to be isolat=
ed as
much as possible.  This code is extremely difficult, if not practically imp=
ossible,
to exhaustively test due to multiple points of entry from "this should neve=
r happen!"
types of flows.  If any of these changes breaks someones deployment, I want=
 to
make it as easy as possible for that someone to determine exactly what brok=
e.