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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 26 Apr 2024 13:41:27 +0100
From: Marc Zyngier <maz@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: <linuxarm@...wei.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra
	<peterz@...radead.org>,
	<linux-pm@...r.kernel.org>,
	<loongarch@...ts.linux.dev>,
	<linux-acpi@...r.kernel.org>,
	<linux-arch@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<kvmarm@...ts.linux.dev>,
	<x86@...nel.org>,
	Russell King <linux@...linux.org.uk>,
	"Rafael J . Wysocki"
	<rafael@...nel.org>,
	Miguel Luis <miguel.luis@...cle.com>,
	"James Morse"
	<james.morse@....com>,
	Salil Mehta <salil.mehta@...wei.com>,
	Jean-Philippe
 Brucker <jean-philippe@...aro.org>,
	Catalin Marinas
	<catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Ingo Molnar
	<mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen
	<dave.hansen@...ux.intel.com>,
	<justin.he@....com>,
	<jianyong.wu@....com>
Subject: Re: [PATCH v7 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

On Thu, 25 Apr 2024 17:55:27 +0100,
Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> 
> On Thu, 25 Apr 2024 16:00:17 +0100
> Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> 
> > On Thu, 25 Apr 2024 13:31:50 +0100
> > Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> > 
> > > On Wed, 24 Apr 2024 16:33:22 +0100
> > > Marc Zyngier <maz@...nel.org> wrote:

[...]

> > >   
> > > > I'd expect the handling side to look like this (will not compile, but
> > > > you'll get the idea):    
> > > Hi Marc,
> > > 
> > > In general this looks good - but...
> > > 
> > > I haven't gotten to the bottom of why yet (and it might be a side
> > > effect of how I hacked the test by lying in minimal fashion and
> > > just frigging the MADT read functions) but the hotplug flow is only getting
> > > as far as calling __cpu_up() before it seems to enter an infinite loop.
> > > That is it never gets far enough to fail this test.
> > > 
> > > Getting stuck in a psci cpu_on call.  I'm guessing something that
> > > we didn't get to in the earlier gicv3 calls before bailing out is blocking that?
> > > Looks like it gets to
> > > SMCCC smc
> > > and is never seen again.
> > > 
> > > Any ideas on where to look?  The one advantage so far of the higher level
> > > approach is we never tried the hotplug callbacks at all so avoided hitting
> > > that call.  One (little bit horrible) solution that might avoid this would 
> > > be to add another cpuhp state very early on and fail at that stage.
> > > I'm not keen on doing that without a better explanation than I have so far!  
> > 
> > Whilst it still doesn't work I suspect I'm loosing ability to print to the console
> > between that point and somewhat later and real problem is
> > elsewhere.

Sorry, travelling at the moment, so only spotted this now.

> 
> Hi again,
> 
> Found it I think.  cpuhp calls between cpu:bringup and ap:online 
> arm made from notify_cpu_starting() are clearly marked as nofail with a comment.
> STARTING must not fail!
> 
> https://elixir.bootlin.com/linux/latest/source/kernel/cpu.c#L1642

Ah, now that rings a bell! ;-)

> 
> Whilst I have no immediate idea why that comment is there it is pretty strong
> argument against trying to have the CPUHP_AP_IRQ_GIC_STARTING callback fail
> and expecting it to carry on working :( 
> There would have been a nice print message, but given I don't appear to have
> a working console after that stage I never see it.
> 
> So the best I have yet come up with for this is the option of a new callback registered
> in gic_smp_init()
> 
> cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
> 			  "irqchip/arm/gicv3:checkrdist",
> 			  gic_broken_rdist, NULL);
> 
> with callback being simply 
> 
> static int gic_broken_rdist(unsigned int cpu)
> {
> 	if (cpumask_test_cpu(cpu, &broken_rdists))
> 		return -EINVAL;
> 
> 	return 0;
> }
> 
> That gets called cpuhp_up_callbacks() and is allows to fail and roll back the steps.
> 
> Not particularly satisfying but keeps the logic confined to the gicv3 driver.
> 
> What do you think?

Good enough for me. Cc me on the resulting patch when you repost it so
that I can eyeball it, but this is IMO the right direction.

Thanks,

	M.

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