Re: [PATCH v5 02/22] cc_platform: Add new attribute to prevent ACPI CPU hotplug

From: Kai Huang <kai.huang@intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc: linux-acpi@vger.kernel.org, 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, thomas.lendacky@amd.com,
	Tianyu.Lan@microsoft.com, rdunlap@infradead.org, Jason@zx2c4.com,
	juri.lelli@redhat.com, mark.rutland@arm.com, frederic@kernel.org,
	yuehaibing@huawei.com, dongli.zhang@oracle.com
Subject: Re: [PATCH v5 02/22] cc_platform: Add new attribute to prevent ACPI CPU hotplug
Date: Wed, 20 Jul 2022 11:54:47 +1200	[thread overview]
Message-ID: <b14ef9354392474b0988dde063bdb186a48424a8.camel@intel.com> (raw)
In-Reply-To: <baaae4b3-7f7d-b193-3546-70170b8b460d@intel.com>

On Tue, 2022-07-19 at 10:46 -0700, Dave Hansen wrote:
> On 7/13/22 04:09, Kai Huang wrote:
> ...
> > "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."
> 
> That's a start.  It also probably needs to say that the security
> perimeter includes all logical CPUs, though.

To me it is kinda implied.  But I have sent email to TDX spec owner to see
whether we can say it more explicitly.

> 
> >  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;
> > +       }
> 
> Is this the right place?  There are other sanity checks in
> acpi_processor_hotadd_init() and it seems like a better spot.

It has below additional check:

        if (invalid_phys_cpuid(pr->phys_id))
                return -ENODEV;

        status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
        if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
                return -ENODEV;

I don't know exactly when will the first "invalid_phys_cpuid()" case happen, but
the CPU is enumerated as "present" only after the second check.  I.e. if BIOS is
buggy and somehow sends a ACPI CPU hot-add event to kernel w/o having the CPU
being actually hot-added, the kernel just returns -ENODEV here.

So to me, adding to acpi_map_cpu() is more reasonable, because by reaching here,
it is sure that a real CPU is being hot-added.

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