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;
Date:   Fri, 30 Jun 2023 14:24:56 -0700
In-Reply-To: <20230630190514.GH3436214@ls.amr.corp.intel.com>
Mime-Version: 1.0
References: <cover.1687784645.git.kai.huang@intel.com> <104d324cd68b12e14722ee5d85a660cccccd8892.1687784645.git.kai.huang@intel.com>
 <20230628131717.GE2438817@hirez.programming.kicks-ass.net>
 <0c9639db604a0670eeae5343d456e43d06b35d39.camel@intel.com>
 <20230630092615.GD2533791@hirez.programming.kicks-ass.net>
 <2659d6eef84f008635ba300f4712501ac88cef2c.camel@intel.com>
 <20230630183020.GA4253@hirez.programming.kicks-ass.net> <20230630190514.GH3436214@ls.amr.corp.intel.com>
Message-ID: <ZJ9IKALhz1Q6ogu1@google.com>
Subject: Re: [PATCH v12 07/22] x86/virt/tdx: Add skeleton to enable TDX on demand
From:   Sean Christopherson <seanjc@google.com>
To:     Isaku Yamahata <isaku.yamahata@gmail.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Kai Huang <kai.huang@intel.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        Ashok Raj <ashok.raj@intel.com>,
        Tony Luck <tony.luck@intel.com>,
        "david@redhat.com" <david@redhat.com>,
        "bagasdotme@gmail.com" <bagasdotme@gmail.com>,
        Dave Hansen <dave.hansen@intel.com>,
        "ak@linux.intel.com" <ak@linux.intel.com>,
        Rafael J Wysocki <rafael.j.wysocki@intel.com>,
        "kirill.shutemov@linux.intel.com" <kirill.shutemov@linux.intel.com>,
        Reinette Chatre <reinette.chatre@intel.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        Isaku Yamahata <isaku.yamahata@intel.com>,
        "nik.borisov@suse.com" <nik.borisov@suse.com>,
        "hpa@zytor.com" <hpa@zytor.com>, Sagi Shahar <sagis@google.com>,
        "imammedo@redhat.com" <imammedo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>, Chao Gao <chao.gao@intel.com>,
        Len Brown <len.brown@intel.com>,
        "sathyanarayanan.kuppuswamy@linux.intel.com" 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Ying Huang <ying.huang@intel.com>,
        Dan J Williams <dan.j.williams@intel.com>,
        "x86@kernel.org" <x86@kernel.org>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Fri, Jun 30, 2023, Isaku Yamahata wrote:
> On Fri, Jun 30, 2023 at 08:30:20PM +0200,
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Fri, Jun 30, 2023 at 09:55:32AM +0000, Huang, Kai wrote:
> > > On Fri, 2023-06-30 at 11:26 +0200, Peter Zijlstra wrote:
> > > > On Thu, Jun 29, 2023 at 12:10:00AM +0000, Huang, Kai wrote:
> > > > > On Wed, 2023-06-28 at 15:17 +0200, Peter Zijlstra wrote:
> > > > > > On Tue, Jun 27, 2023 at 02:12:37AM +1200, Kai Huang wrote:
> > > > > > > +EXPORT_SYMBOL_GPL(tdx_cpu_enable);
> > > > > > 
> > > > > > I can't find a single caller of this.. why is this exported?
> > > > > 
> > > > > It's for KVM TDX patch to use, which isn't in this series.
> > > > > 
> > > > > I'll remove the export.  KVM TDX series can export it.
> > > > 
> > > > Fair enough; where will the KVM TDX series call this? Earlier there was
> > > > talk about doing it at kvm module load time -- but I objected (and still
> > > > do object) to that.
> > > > 
> > > > What's the current plan?
> > > > 
> > > 
> > > The direction is still doing it during module load (not my series anyway).  But
> > > this can be a separate discussion with KVM maintainers involved.
> > 
> > They all on Cc afaict.
> > 
> > > I understand you have concern that you don't want to have the memory & cpu time
> > > wasted on enabling TDX by default.  For that we can have a kernel command line
> > > to disable TDX once for all (we can even make it default).
> > 
> > That's insane, I don't want to totally disable it. I want it done at
> > guard creation. Do the whole TDX setup the moment you actually create a
> > TDX guast.
> > 
> > Totally killing TDX is stupid, 

I dunno about that, *totally* killing TDX would make my life a lot simpler ;-)

> > just about as stupid as doing it on module load (which equates to always
> > doing it).
> > 
> > > Also, KVM will have a module parameter 'enable_tdx'.  I am hoping this could
> > > reduce your concern too.
> > 
> > I don't get this obsession with doing at module load time :/

Waiting until userspace attempts to create the first TDX guest adds complexity
and limits what KVM can do to harden itself.  Currently, all feature support in
KVM is effectively frozen at module load.  E.g. most of the setup code is
contained in __init functions, many module-scoped variables are effectively 
RO after init (though they can't be marked as such until we smush kvm-intel.ko
and kvm-amd.ko into kvm.ko, which is tentatively the long-term plan).  All of
those patterns would get tossed aside if KVM waits until userspace attempts to
create the first guest.

The userspace experience would also be poor, as KVM can't know whether or TDX is
actually supported until the TDX module is fully loaded and configured.  KVM waits
until VM creation to enable VMX, but that's pure enabling and more or less
guaranteed to succeed, e.g. will succeed barring hardware failures, software bugs,
or *severe* memory pressure.

There are also latency and noisy neighbor concerns, e.g. we *really* don't want
to end up in a situation where creating a TDX guest for a customer can observe
arbitrary latency *and* potentially be disruptive to VMs already running on the
host.

Userspace can workaround the second and third issues by spawning a dummy TDX guest
as early as possible, but that adds complexity to userspace, especially if there's
any desire for it to be race free, e.g. with respect to reporting system capabilities
to the control plan.

On the flip side, limited hardware availability (unless Intel has changed its
tune) and the amount of enabling that's required in BIOS and whatnot makes it
highly unlikely that random Linux users are going to unknowingly boot with TDX
enabled.

That said, if this is a sticking point, let's just make enable_tdx off by default,
i.e. force userspace to opt-in.  Deployments that *know* they may want to schedule
TDX VMs on the host can simply force the module param.  And for everyone else,
since KVM is typically configured as a module by distros, KVM can be unloaded and
reload if the user realizes they want TDX well after the system is up and running.

> The KVM maintainers prefer the initialization on kvm_intel.ko loading time. [1]

You can say "Sean", I'm not the bogeyman :-)

> I can change enable_tdx parameter for kvm_intel.ko instead of boolean.
> Something like
> 
> enable_tdx
>         ondemand: on-demand initialization when creating the first TDX guest
>         onload:   initialize TDX module when loading kvm_intel.ko

No, that's the most complex path and makes no one happy.

>         disable:  disable TDX support