Re: [PATCH v3 10/10] xen/arm: Enable errata for secondary CPU on hotplug after the boot

From: Dario Faggioli <dfaggioli@suse.com>
To: Julien Grall <julien.grall@arm.com>,
	Mirela Simonovic <mirela.simonovic@aggios.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Davorin Mista <dm@aggios.com>,
	Xen Devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH v3 10/10] xen/arm: Enable errata for secondary CPU on hotplug after the boot
Date: Thu, 10 May 2018 18:12:18 +0200	[thread overview]
Message-ID: <c95c5aad1f25bf1b9a8b6376b4cb94936d52b573.camel@suse.com> (raw)
In-Reply-To: <300e3466-8214-764a-6c33-ce8aaea31ed9@arm.com>

[-- Attachment #1.1: Type: text/plain, Size: 2282 bytes --]

On Thu, 2018-05-10 at 16:13 +0100, Julien Grall wrote:
> On 10/05/18 16:00, Mirela Simonovic wrote:
> > > If you add your callback to CPU_UP_PREPARE, instead than to
> > > CPU_STARTING, SCHED_OP(init_pdata) wouldn't be called, without
> > > having
> > > to fiddle with priorities.
> 
> This function will enable capabilities on a given CPU, most of them
> are 
> touching system registers. So it is necessary to add the callback to 
> CPU_STARTING.
> 
Right, I guess that answers my question.

> I always like to understand what I maintain :). Why do you need to 
> change the priority if you just return an error in the notifier?
> 
> At the moment, notify_cpu_starting has a BUG_ON() in it. But ideally
> I 
> would like to either replace that BUG_ON by cpu_stop or just
> returning 
> an error to give a chance of the architecture deciding what to do
> (this 
> does not have to be done today).
> 
The problem is that, currently, once we've reached CPU_STARTING, we
assume that the CPU bringup has gone ok, and things can't fail.

Therefore, the only place when we undo what CPU_STARTING does is during
CPU_DEAD, i.e., during hotunplug/suspend/teardown.

I understand the point of having to run stuff on the CPU that is coming
up, as well as your more general point.

However, I don't know whether allowing CPU_STARTING to fail is the best
way to achieve what you want to achieve, nor whether the BUG_ON in
notify_cpu_starting() is the only issue you'll face trying to do that.

I'd say that, if CPU_STARTING can fail, we need an appropriate rollback
step, i.e., the flow must become something like (but I'd appreciate the
opinion of x86 and core hypervisor maintainers about this):

CPU_UP_PREPARE --> CPU_STARTING xx> CPU_DIDNT_START --> CPU_UP_CANCEL

At which point, e.g., from the scheduler point of view, we can try to
put a call to SCHED_OP(deinit_pdata) in CPU_DIDNT_START, and that would
avoid the problem Mirela is facing, without having to play with
priorities.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel