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, 24 Jun 2022 10:39:21 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     Kai Huang <kai.huang@intel.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        seanjc@google.com, pbonzini@redhat.com, dave.hansen@intel.com,
        len.brown@intel.com, tony.luck@intel.com,
        rafael.j.wysocki@intel.com, reinette.chatre@intel.com,
        dan.j.williams@intel.com, peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        isaku.yamahata@intel.com
Subject: Re: [PATCH v5 06/22] x86/virt/tdx: Add skeleton to initialize TDX on
 demand
Message-ID: <20220624023916.GC15566@gao-cwp>
References: <cover.1655894131.git.kai.huang@intel.com>
 <c751d1ce046ccc139a8bb34e04d70b1d6bc34a8d.1655894131.git.kai.huang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c751d1ce046ccc139a8bb34e04d70b1d6bc34a8d.1655894131.git.kai.huang@intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Wed, Jun 22, 2022 at 11:16:29PM +1200, Kai Huang wrote:
>Before the TDX module can be used to create and run TD guests, it must
>be loaded into the isolated region pointed by the SEAMRR and properly
>initialized.  The TDX module is expected to be loaded by BIOS before
>booting to the kernel, and the kernel is expected to detect and
>initialize it.
>
>The TDX module can be initialized only once in its lifetime.  Instead
>of always initializing it at boot time, this implementation chooses an
>on-demand approach to initialize TDX until there is a real need (e.g
>when requested by KVM).  This avoids consuming the memory that must be
>allocated by kernel and given to the TDX module as metadata (~1/256th of
>the TDX-usable memory), and also saves the time of initializing the TDX
>module (and the metadata) when TDX is not used at all.  Initializing the
>TDX module at runtime on-demand also is more flexible to support TDX
>module runtime updating in the future (after updating the TDX module, it
>needs to be initialized again).
>
>Add a placeholder tdx_init() to detect and initialize the TDX module on
>demand, with a state machine protected by mutex to support concurrent
>calls from multiple callers.
>
>The TDX module will be initialized in multi-steps defined by the TDX
>architecture:
>
>  1) Global initialization;
>  2) Logical-CPU scope initialization;
>  3) Enumerate the TDX module capabilities and platform configuration;
>  4) Configure the TDX module about usable memory ranges and global
>     KeyID information;
>  5) Package-scope configuration for the global KeyID;
>  6) Initialize usable memory ranges based on 4).
>
>The TDX module can also be shut down at any time during its lifetime.
>In case of any error during the initialization process, shut down the
>module.  It's pointless to leave the module in any intermediate state
>during the initialization.
>
>Signed-off-by: Kai Huang <kai.huang@intel.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>

One nit below:

>+static int __tdx_init(void)
>+{
>+	int ret;
>+
>+	/*
>+	 * Initializing the TDX module requires running some code on
>+	 * all MADT-enabled CPUs.  If not all MADT-enabled CPUs are
>+	 * online, it's not possible to initialize the TDX module.
>+	 *
>+	 * For simplicity temporarily disable CPU hotplug to prevent
>+	 * any CPU from going offline during the initialization.
>+	 */
>+	cpus_read_lock();
>+
>+	/*
>+	 * Check whether all MADT-enabled CPUs are online and return
>+	 * early with an explicit message so the user can be aware.
>+	 *
>+	 * Note ACPI CPU hotplug is prevented when TDX is enabled, so
>+	 * num_processors always reflects all present MADT-enabled
>+	 * CPUs during boot when disabled_cpus is 0.
>+	 */
>+	if (disabled_cpus || num_online_cpus() != num_processors) {
>+		pr_err("Unable to initialize the TDX module when there's offline CPU(s).\n");
>+		ret = -EINVAL;
>+		goto out;
>+	}
>+
>+	ret = init_tdx_module();
>+	if (ret == -ENODEV) {
>+		pr_info("TDX module is not loaded.\n");

tdx_module_status should be set to TDX_MODULE_NONE here.

>+		goto out;
>+	}