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;
Feedback-ID: ie3994620:Fastmail
Date:   Mon, 27 Mar 2023 14:09:32 +0300
From:   "Kirill A. Shutemov" <kirill@shutemov.name>
To:     Baoquan He <bhe@redhat.com>
Cc:     Dave Hansen <dave.hansen@intel.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, x86@kernel.org,
        Eric Biederman <ebiederm@xmission.com>,
        kexec@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86: Disable kexec for TDX guests
Message-ID: <20230327110932.z3wjdgfuay5dd2or@box.shutemov.name>
References: <20230325160128.21857-1-kirill.shutemov@linux.intel.com>
 <ebead33b-0594-73df-56ae-f40473ac0ffc@intel.com>
 <20230325192524.wetlbycbcsxc4plk@box>
 <b16bcda0-f190-7849-cbbb-412d328c8806@intel.com>
 <ZCDy+nsjPsi/Lllh@MiWiFi-R3L-srv>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZCDy+nsjPsi/Lllh@MiWiFi-R3L-srv>
Precedence: bulk

On Mon, Mar 27, 2023 at 09:35:54AM +0800, Baoquan He wrote:
> On 03/26/23 at 10:01am, Dave Hansen wrote:
> > On 3/25/23 12:25, Kirill A. Shutemov wrote:
> > > On Sat, Mar 25, 2023 at 09:25:36AM -0700, Dave Hansen wrote:
> > >> On 3/25/23 09:01, Kirill A. Shutemov wrote:
> > >>> The last item is tricky. TDX guests use ACPI MADT MPWK to bring up
> > >>> secondary CPUs. The mechanism doesn't allow to put a CPU back offline if
> > >>> it has woken up.
> > >> ...
> > >>> +int arch_kexec_load(void)
> > >>> +{
> > >>> +	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
> > >>> +		pr_warn_once("Disable kexec: not yet supported in TDX guest\n");
> > >>> +		return -EOPNOTSUPP;
> > >>> +	}
> > >>> +
> > >>> +	return 0;
> > >>> +}
> > >>
> > >> So, let's put all this together:
> > >>
> > >> 1. TDX implementations use MADT for wakeup exclusively right now (but
> > >>    are not necessarily _required_ to do so forever)
> > >> 2. MADT doesn't support CPU offlining
> > >> 3. kexec() requires offlining
> > >>
> > >> Thus, current TDX implementations can't support TDX guests.  This
> > >> *doesn't* say that TDX will always use the MADT for wakeups.
> > >>
> > >> Yet, the check you have here is for TDX and *not* for the MADT.
> > > 
> > > As I described in the commit message there are more than MADT that is
> > > required to get kexec in TDX guest.
> > 
> > I kinda think we should do both.
> > 
> > Let's make sure that all systems that depend on MADT wakeups can't
> > kexec() until the ACPI folks work out what to do there.
> > 
> > Separately, let's either fix or *mark* the kexec()-incompatible pieces
> > that *ARE* specific to TDX.
> > 
> > >> That seems wrong.
> > >>
> > >> Let's say SEV or arm64 comes along and uses the MADT for their guests.
> > >> They'll add another arch_kexec_load(), with a check for *their* feature.
> > >>
> > >> This all seems like you should be disabling kexec() the moment the MADT
> > >> CPU wakeup is used instead of making it based on TDX.
> > > 
> > > I guess we can go this path if you are fine with taking CR4.MCE and shared
> > > memory reverting patches (they require some rework, but I can get them
> > > into shape quickly). After that we can forbid kexec on machines with MADT
> > > if nr_cpus > 1.
> > 
> > This goes back to what I asked before: is anyone actually going to *use*
> > a single-processor system that wants to kexec()?  If not, let's not
> > waste the time to introduce code that is just going to bitrot.  Just
> > mark it broken and move on with life.
> 
> Now we have two API for kexec: kexec_load and kexec_file_load. They can
> be used to do kexec reboot, or crash dumping. For crash dumping, we
> usually only use one cpu to do the vmcore dumping. At least on our
> Fedora/centos-stream/RHEL, we do like this with kernel parameter
> 'nr_cpus=1' added by default. Unless people explicitly remove the
> 'nr_cpus=1' restriction or set nr_cpus= to other number to persue
> multithread dumping in kdump kernel.

Hm. I'm not sure how to determine if the target kernel wants to use >1
CPU. Scanning cmdline looks fragile. And who said the target kernel is
Linux.

I guess we can park all CPUs, but CPU0 and target kernel will just fail to
bring them up which is non-fatal issue (at least for Linux).

I admit that all looks hackish.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov