Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

From: Marc Zyngier <maz@kernel.org>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>, <linux-pm@vger.kernel.org>,
	<loongarch@lists.linux.dev>, <linux-acpi@vger.kernel.org>,
	<linux-arch@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <kvmarm@lists.linux.dev>,
	<x86@kernel.org>, Russell King <linux@armlinux.org.uk>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Miguel Luis <miguel.luis@oracle.com>,
	"James Morse" <james.morse@arm.com>,
	Salil Mehta <salil.mehta@huawei.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Hanjun Guo <guohanjun@huawei.com>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>, <linuxarm@huawei.com>,
	<justin.he@arm.com>, <jianyong.wu@arm.com>,
	"Lorenzo\ Pieralisi" <lpieralisi@kernel.org>,
	Sudeep Holla <sudeep.holla@arm.com>
Subject: Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs
Date: Sun, 28 Apr 2024 12:28:03 +0100	[thread overview]
Message-ID: <87frv5u3p8.wl-maz@kernel.org> (raw)
In-Reply-To: <20240426192858.000033d9@huawei.com>

On Fri, 26 Apr 2024 19:28:58 +0100,
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> 
> I'll not send a formal v9 until early next week, so here is the current state
> if you have time to take another look before then.

Don't bother resending this on my account -- you only sent it on
Friday and there hasn't been much response to it yet. There is still a
problem (see below), but looks otherwise OK.

[...]

> @@ -2363,11 +2381,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
>  				(struct acpi_madt_generic_interrupt *)header;
>  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> +	int cpu = get_cpu_for_acpi_id(gicc->uid);

I already commented that get_cpu_for_acpi_id() can...

>  	void __iomem *redist_base;
>  
> -	if (!acpi_gicc_is_usable(gicc))
> +	/* Neither enabled or online capable means it doesn't exist, skip it */
> +	if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
>  		return 0;
>  
> +	/*
> +	 * Capable but disabled CPUs can be brought online later. What about
> +	 * the redistributor? ACPI doesn't want to say!
> +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> +	 * Otherwise, prevent such CPUs from being brought online.
> +	 */
> +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> +		pr_warn("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> +		cpumask_set_cpu(cpu, &broken_rdists);

... return -EINVAL, and then be passed to cpumask_set_cpu(), with
interesting effects. It shouldn't happen, but I trust anything that
comes from firmware tables as much as I trust a campaigning
politician's promises. This should really result in the RD being
considered unusable, but without affecting any CPU (there is no valid
CPU the first place).

Another question is what get_cpu_for acpi_id() returns for a disabled
CPU. A valid CPU number? Or -EINVAL?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.