Received-SPF: pass (google.com: domain of linux-kernel+bounces-158912-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 25 Apr 2024 09:35:16 -0700
In-Reply-To: <61ec08765f0cd79f2d5ea1e2acf285ea9470b239.camel@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <22b19d11-056c-402b-ac19-a389000d6339@intel.com>
 <ZiKoqMk-wZKdiar9@google.com> <deb9ccacc4da04703086d7412b669806133be047.camel@intel.com>
 <ZiaWMpNm30DD1A-0@google.com> <3771fee103b2d279c415e950be10757726a7bd3b.camel@intel.com>
 <Zib76LqLfWg3QkwB@google.com> <6e83e89f145aee496c6421fc5a7248aae2d6f933.camel@intel.com>
 <d0563f077a7f86f90e72183cf3406337423f41fe.camel@intel.com>
 <ZifQiCBPVeld-p8Y@google.com> <61ec08765f0cd79f2d5ea1e2acf285ea9470b239.camel@intel.com>
Message-ID: <ZiqGRErxDJ1FE8iA@google.com>
Subject: Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module
From: Sean Christopherson <seanjc@google.com>
To: Kai Huang <kai.huang@intel.com>
Cc: Tina Zhang <tina.zhang@intel.com>, Hang Yuan <hang.yuan@intel.com>, 
	Bo2 Chen <chen.bo@intel.com>, "sagis@google.com" <sagis@google.com>, 
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Erdem Aktas <erdemaktas@google.com>, 
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "pbonzini@redhat.com" <pbonzini@redhat.com>, 
	Isaku Yamahata <isaku.yamahata@intel.com>, 
	"isaku.yamahata@linux.intel.com" <isaku.yamahata@linux.intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 23, 2024, Kai Huang wrote:
> On Tue, 2024-04-23 at 08:15 -0700, Sean Christopherson wrote:
> > Presumably that approach relies on something blocking onlining CPUs whe=
n TDX is
> > active.  And if that's not the case, the proposed patches are buggy.
>=20
> The current patch ([PATCH 023/130] KVM: TDX: Initialize the TDX module
> when loading the KVM intel kernel module) indeed is buggy, but I don't
> quite follow why we need to block onlining CPU  when TDX is active?

I was saying that based on my reading of the code, either (a) the code is b=
uggy
or (b) something blocks onlining CPUs when TDX is active.  Sounds like the =
answer
is (a).

> There's no hard things that prevent us to do so.  KVM just need to do
> VMXON + tdx_cpu_enable() inside kvm_online_cpu().
>=20
> >=20
> > > Btw, the ideal (or probably the final) plan is to handle tdx_cpu_enab=
le()
> > > in TDX's own CPU hotplug callback in the core-kernel and hide it from=
 all
> > > other in-kernel TDX users. =C2=A0
> > >=20
> > > Specifically:
> > >=20
> > > 1) that callback, e.g., tdx_online_cpu() will be placed _before_ any =
in-
> > > kernel TDX users like KVM's callback.
> > > 2) In tdx_online_cpu(), we do VMXON + tdx_cpu_enable() + VMXOFF, and
> > > return error in case of any error to prevent that cpu from going onli=
ne.
> > >=20
> > > That makes sure that, if TDX is supported by the platform, we basical=
ly
> > > guarantees all online CPUs are ready to issue SEAMCALL (of course, th=
e in-
> > > kernel TDX user still needs to do VMXON for it, but that's TDX user's
> > > responsibility).
> > >=20
> > > But that obviously needs to move VMXON to the core-kernel.
> >=20
> > It doesn't strictly have to be core kernel per se, just in code that si=
ts below
> > KVM, e.g. in a seperate module called VAC[*] ;-)
> >=20
> > [*] https://lore.kernel.org/all/ZW6FRBnOwYV-UCkY@google.com
>=20
> Could you elaborate why vac.ko is necessary?
>=20
> Being a module natually we will need to handle module init and exit.  But
> TDX cannot be disabled and re-enabled after initialization, so in general
> the vac.ko doesn't quite fit for TDX.
>=20
> And I am not sure what's the fundamental difference between managing TDX
> module in a module vs in the core-kernel from KVM's perspective.

VAC isn't strictly necessary.  What I was saying is that it's not strictly
necessary for the core kernel to handle VMXON either.  I.e. it could be don=
e in
something like VAC, or it could be done in the core kernel.

The important thing is that they're handled by _one_ entity.  What we have =
today
is probably the worst setup; VMXON is handled by KVM, but TDX.SYS.LP.INIT i=
s
handled by core kernel (sort of).