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;
Message-ID: <966b1f45-ba5d-febd-e365-29308a9a59b4@intel.com>
Date:   Thu, 28 Apr 2022 17:26:52 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v3 04/21] x86/virt/tdx: Add skeleton for detecting and
 initializing TDX on demand
Content-Language: en-US
To:     Kai Huang <kai.huang@intel.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     seanjc@google.com, pbonzini@redhat.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
References: <cover.1649219184.git.kai.huang@intel.com>
 <32dcf4c7acc95244a391458d79cd6907125c5c29.1649219184.git.kai.huang@intel.com>
 <ac482f2b-d2d1-0643-faa4-1b36340268c5@intel.com>
 <22e3adf42b8ea2cae3aabc26f762acb983133fea.camel@intel.com>
 <c833aff2-b459-a1d7-431f-bce5c5f29182@intel.com>
 <37efe2074eba47c51bf5c1a2369a05ddf9082885.camel@intel.com>
 <3731a852-71b8-b081-2426-3b0a650e174c@intel.com>
 <edcae7ab1e6a074255a6624e8e0536bd77f84eed.camel@intel.com>
 <0aa81fd0-a491-847d-9fc6-4b853f2cf7b4@intel.com>
 <af6fccf2f6f8d83593f0eedd003c7cd07f89274d.camel@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <af6fccf2f6f8d83593f0eedd003c7cd07f89274d.camel@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/28/22 17:11, Kai Huang wrote:
> This is true.  So I think w/o taking the lock is also fine, as the TDX module
> initialization is a state machine.  If any cpu goes offline during logical-cpu
> level initialization and TDH.SYS.LP.INIT isn't done on that cpu, then later the
> TDH.SYS.CONFIG will fail.  Similarly, if any cpu going offline causes
> TDH.SYS.KEY.CONFIG is not done for any package, then TDH.SYS.TDMR.INIT will
> fail.

Right.  The worst-case scenario is someone is mucking around with CPU
hotplug during TDX initialization is that TDX initialization will fail.

We *can* fix some of this at least and provide coherent error messages
with a pattern like this:

	cpus_read_lock();
	// check that all MADT-enumerated CPUs are online
	tdx_init();
	cpus_read_unlock();

That, of course, *does* prevent CPUs from going offline during
tdx_init().  It also provides a nice place for an error message:

	pr_warn("You offlined a CPU then want to use TDX?  Sod off.\n");

> A problem (I realized it exists in current implementation too) is shutting down
> the TDX module, which requires calling TDH.SYS.LP.SHUTDOWN on all BIOS-enabled
> cpus.  Kernel can do this SEAMCALL at most for all present cpus.  However when
> any cpu is offline, this SEAMCALL won't be called on it, and it seems we need to
> add new CPU hotplug callback to call this SEAMCALL when the cpu is online again.

Hold on a sec.  If you call TDH.SYS.LP.SHUTDOWN on any CPU, then TDX
stops working everywhere, right?  But, if someone offlines one CPU, we
don't want TDX to stop working everywhere.