Message-ID: <173b20166a77012669fdc2c600556fca0623d0b1.camel@intel.com>
Date:   Wed, 13 Jul 2022 23:09:27 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     linux-acpi@...r.kernel.org, seanjc@...gle.com, pbonzini@...hat.com,
        len.brown@...el.com, tony.luck@...el.com,
        rafael.j.wysocki@...el.com, reinette.chatre@...el.com,
        dan.j.williams@...el.com, peterz@...radead.org, ak@...ux.intel.com,
        kirill.shutemov@...ux.intel.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com,
        isaku.yamahata@...el.com, thomas.lendacky@....com,
        Tianyu.Lan@...rosoft.com, rdunlap@...radead.org, Jason@...c4.com,
        juri.lelli@...hat.com, mark.rutland@....com, frederic@...nel.org,
        yuehaibing@...wei.com, dongli.zhang@...cle.com
Subject: Re: [PATCH v5 02/22] cc_platform: Add new attribute to prevent ACPI
 CPU hotplug

On Mon, 2022-06-27 at 17:05 +1200, Kai Huang wrote:
> On Fri, 2022-06-24 at 11:57 -0700, Dave Hansen wrote:
> > On 6/22/22 04:15, Kai Huang wrote:
> > > Platforms with confidential computing technology may not support ACPI
> > > CPU hotplug when such technology is enabled by the BIOS.  Examples
> > > include Intel platforms which support Intel Trust Domain Extensions
> > > (TDX).
> > > 
> > > If the kernel ever receives ACPI CPU hotplug event, it is likely a BIOS
> > > bug.  For ACPI CPU hot-add, the kernel should speak out this is a BIOS
> > > bug and reject the new CPU.  For hot-removal, for simplicity just assume
> > > the kernel cannot continue to work normally, and BUG().
> > 
> > So, the kernel is now declaring ACPI CPU hotplug and TDX to be
> > incompatible and even BUG()'ing if we see them together.  Has anyone
> > told the firmware guys about this?  Is this in a spec somewhere?  When
> > the kernel goes boom, are the firmware folks going to cry "Kernel bug!!"?
> > 
> > This doesn't seem like something the kernel should be doing unilaterally.
> 
> TDX doesn't support ACPI CPU hotplug (both hot-add and hot-removal) is an
> architectural behaviour.  The public specs doesn't explicitly say  it, but it is
> implied:
> 
> 1) During platform boot MCHECK verifies all logical CPUs on all packages that
> they are TDX compatible, and it keeps some information, such as total CPU
> packages and total logical cpus at some location of SEAMRR so it can later be
> used by P-SEAMLDR and TDX module.  Please see "3.4 SEAMLDR_SEAMINFO" in the P-
> SEAMLDR spec:
> 
> https://cdrdv2.intel.com/v1/dl/getContent/733584
> 
> 2) Also some SEAMCALLs must be called on all logical CPUs or CPU packages that
> the platform has (such as such as TDH.SYS.INIT.LP and TDH.SYS.KEY.CONFIG),
> otherwise the further step of TDX module initialization will fail.
> 
> Unfortunately there's no public spec mentioning what's the behaviour of ACPI CPU
> hotplug on TDX enabled platform.  For instance, whether BIOS will ever get the
> ACPI CPU hot-plug event, or if BIOS gets the event, will it suppress it.  What I
> got from Intel internally is a non-buggy BIOS should never report such event to
> the kernel, so if kernel receives such event, it should be fair enough to treat
> it as BIOS bug.
> 
> But theoretically, the BIOS isn't in TDX's TCB, and can be from 3rd party..
> 
> Also, I was told "CPU hot-plug is a system feature, not a CPU feature or Intel
> architecture feature", so Intel doesn't have an architectural specification for
> CPU hot-plug. 
> 
> At the meantime, I am pushing Intel internally to add some statements regarding
> to the TDX and CPU hotplug interaction to the BIOS write guide and make it
> public.  I guess this is the best thing we can do.
> 
> Regarding to the code change, I agree the BUG() isn't good.  I used it because:
> 1) this basically on a theoretical problem and shouldn't happen in practice; 2)
> because there's no architectural specification regarding to the behaviour of TDX
> when CPU hot-removal, so I just used BUG() in assumption that TDX isn't safe to
> use anymore.

Hi Dave,

Trying to close how to handle ACPI CPU hotplug for TDX.  Could you give some
suggestion?

After discussion with TDX guys, they have agreed they will add below to either
the TDX module spec or TDX whitepaper:

"TDX doesn’t support adding or removing CPUs from TDX security perimeter. The
BIOS should prevent CPUs from being hot-added or hot-removed after platform
boots."

This means if TDX is enabled in BIOS, a non-buggy BIOS should never deliver ACPI
CPU hotplug event to kernel, otherwise it is a BIOS bug.  And this is only
related to whether TDX is enabled in BIOS, no matter whether the TDX module has
been loaded/initialized or not.

So I think the proper way to handle is: we still have code to detect whether TDX
is enabled by BIOS (patch 01 in this series), and when ACPI CPU hotplug happens
on TDX enabled platform, we print out error message saying it is a BIOS bug.

Specifically, for CPU hot-add, we can print error message and reject the new
CPU.  For CPU hot-removal, when the kernel receives this event, the CPU hot-
removal has already handled by BIOS so the kernel cannot reject it.  So I think
we can either BUG(), or say "TDX is broken and please reboot the machine".

I guess BUG() would catch a lot of eyeball, so how about choose the latter, like
below?

--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -799,6 +799,7 @@ static void __init acpi_set_irq_model_ioapic(void)
  */
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 #include <acpi/processor.h>
+#include <asm/tdx.h>
 
 static int acpi_map_cpu2node(acpi_handle handle, int cpu, int physid)
 {
@@ -819,6 +820,12 @@ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid,
u32 acpi_id,
 {
        int cpu;
 
+       if (platform_tdx_enabled()) {
+               pr_err("BIOS bug: CPU (physid %u) hot-added on TDX enabled
platform. Reject it.\n",
+                               physid);
+               return -EINVAL;
+       }
+
        cpu = acpi_register_lapic(physid, acpi_id, ACPI_MADT_ENABLED);
        if (cpu < 0) {
                pr_info("Unable to map lapic to logical cpu number\n");
@@ -835,6 +842,10 @@ EXPORT_SYMBOL(acpi_map_cpu);
 
 int acpi_unmap_cpu(int cpu)
 {
+       if (platform_tdx_enabled())
+               pr_err("BIOS bug: CPU %d hot-removed on TDX enabled platform.
TDX is broken. Please reboot the machine.\n",
+                               cpu);
+
 #ifdef CONFIG_ACPI_NUMA
        set_apicid_to_node(per_cpu(x86_cpu_to_apicid, cpu), NUMA_NO_NODE);
 #endif


-- 
Thanks,
-Kai

Powered by blists - more mailing lists