Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <3547101508b3f168cce202827bd73a051224a542.camel@intel.com>
Subject: Re: [PATCH v3 04/21] x86/virt/tdx: Add skeleton for detecting and
 initializing TDX on demand
From:   Kai Huang <kai.huang@intel.com>
To:     Dave Hansen <dave.hansen@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
Date:   Fri, 29 Apr 2022 12:59:52 +1200
In-Reply-To: <966b1f45-ba5d-febd-e365-29308a9a59b4@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>
         <966b1f45-ba5d-febd-e365-29308a9a59b4@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 2022-04-28 at 17:26 -0700, Dave Hansen wrote:
> 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");

Yes this is better.

The problem is how to check MADT-enumerated CPUs are online?

I checked the code, and it seems we can use 'num_processors + disabled_cpus' as
MADT-enumerated CPUs?  In fact, there should be no 'disabled_cpus' for TDX, so I
think:

	if (disabled_cpus || num_processors != num_online_cpus()) {
		pr_err("Initializing the TDX module requires all MADT-
enumerated CPUs being onine.");
		return -EINVAL;
	}

But I may have misunderstanding.

> 
> > 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?  
> 

Yes.

But tot shut down the TDX module, it's better to call  LP.SHUTDOWN on all 
logical cpus as suggested by spec.

> But, if someone offlines one CPU, we
> don't want TDX to stop working everywhere.

Right.   I am talking about when initializing fails due to any reason (i.e. -
ENOMEM), currently we shutdown the TDX module.  When shutting down the TDX
module, we want to call LP.SHUTDOWN on all logical cpus.  If there's any CPU
being offline when we do the shutdown, then LP.SHUTDOWN won't be called for that
cpu. 

But as you suggested above, if we have an early check whether all MADT-
enumerated CPUs are online and if not we return w/o shutting down the TDX
module, then if we shutdown the module the LP.SHUTDOWN will be called on all
cpus.