Date:   Mon, 13 Mar 2023 10:18:50 -0700
In-Reply-To: <ce2330db94b05605a0649a3da0595211c5bd71dd.camel@intel.com>
Mime-Version: 1.0
References: <20230310214232.806108-1-seanjc@google.com> <20230310214232.806108-6-seanjc@google.com>
 <ce2330db94b05605a0649a3da0595211c5bd71dd.camel@intel.com>
Message-ID: <ZA9avcHRPoIqZP/n@google.com>
Subject: Re: [PATCH v2 05/18] x86/reboot: KVM: Disable SVM during reboot via
 virt/KVM reboot callback
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:
> > Use the virt callback to disable SVM (and set GIF=3D1) during an emerge=
ncy
> > instead of blindly attempting to disable SVM.=EF=BF=BD Like the VMX cas=
e, if KVM
> > (or an out-of-tree hypervisor) isn't loaded/active, SVM can't be in use=
.
> >=20
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
>=20
> [...]
>=20
> > -#if IS_ENABLED(CONFIG_KVM_INTEL)
> > +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> > =EF=BF=BD/* RCU-protected callback to disable virtualization prior to r=
eboot. */
> > =EF=BF=BDstatic cpu_emergency_virt_cb __rcu *cpu_emergency_virt_callbac=
k;
> > =EF=BF=BD
> > @@ -821,7 +821,7 @@ int crashing_cpu =3D -1;
> > =EF=BF=BD */
> > =EF=BF=BDvoid cpu_emergency_disable_virtualization(void)
> > =EF=BF=BD{
> > -#if IS_ENABLED(CONFIG_KVM_INTEL)
> > +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> > =EF=BF=BD	cpu_emergency_virt_cb *callback;
> > =EF=BF=BD
> > =EF=BF=BD	rcu_read_lock();
> > @@ -830,8 +830,6 @@ void cpu_emergency_disable_virtualization(void)
> > =EF=BF=BD		callback();
> > =EF=BF=BD	rcu_read_unlock();
> > =EF=BF=BD#endif
> > -	/* KVM_AMD doesn't yet utilize the common callback. */
> > -	cpu_emergency_svm_disable();
> > =EF=BF=BD}
>=20
> Shouldn't the callback be always present since you want to consider 'out-=
of-
> tree' hypervisor case?

No?  The kernel doesn't provide any guarantees for out-of-tree code.  I don=
't have
a super strong preference, though I do like the effective documentation the=
 checks
provide.  Buy more importantly, my understanding is that the x86 maintainer=
s want
to limit the exposure for these types of interfaces, e.g. `git grep IS_ENAB=
LED\(CONFIG_KVM`
for a variety of hooks that are enabled iff KVM is enabled in the kernel co=
nfig.