[PATCH] Arm64: convert soft_restart() to assembly code

[Christoffer Dall]

On Wed, Aug 20, 2014 at 12:48 PM, Mark Rutland <mark.rutland at arm.com> wrote:
> On Mon, Aug 18, 2014 at 06:33:36PM +0100, Christoffer Dall wrote:
>> On Mon, Aug 18, 2014 at 6:02 PM, Mark Rutland <mark.rutland at arm.com> wrote:
>> > Hi Geoff,
>> >
>> > On Fri, Aug 15, 2014 at 07:53:19PM +0100, Geoff Levand wrote:
>> >> Hi Mark,
>> >>
>> >> On Fri, 2014-08-15 at 19:21 +0100, Mark Rutland wrote:
>> >> > On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> >> > > For the cpu-ops shutdown I'm working on I need a call to move the
>> >> > > secondary processors to an identity mapped spin loop after the identity
>> >> > > map is enabled.  I want to do this in C code, so it needs to happen
>> >> > > after the identity map is enabled, and before the dcache is disabled.
>> >> > >
>> >> > > I think to do this we can keep the existing soft_restart(addr) routine
>> >> > > with something like this:
>> >> > >
>> >> > > void soft_restart(unsigned long addr)
>> >> > > {
>> >> > >   setup_mm_for_reboot();
>> >> > >
>> >> > > #if defined(CONFIG_SMP)
>> >> > >   smp_secondary_shutdown();
>> >> > > #endif
>> >> > >
>> >> > >   cpu_soft_restart(addr);
>> >> > >
>> >> > >   /* Should never get here */
>> >> > >   BUG();
>> >> > > }
>> >> > >
>> >> >
>> >> > I don't follow why you need a hook in the middle of soft_restart. That
>> >> > sounds like a layering violation to me.
>> >> >
>> >> > I assume this is for implementing the spin-table cpu-return-addr idea?
>> >>
>> >> Yes.
>> >>
>> >> > If so, what's wrong with something like:
>> >>
>> >> > void spin_table_cpu_die(unsigned int cpu)
>> >> > {
>> >> >     unsigned long release_addr = per_cpu(return_addr, cpu);
>> >> >
>> >> >     /*
>> >> >      * We should have a local_disable(DBG|ASYNC|FIQ|IRQ) function or
>> >> >      * something similar as these are all context synchronising and
>> >> >      * therefore expensive.
>> >> >      */
>> >> >     local_dbg_disable();
>> >> >     local_async_disable();
>> >> >     local_fiq_disable();
>> >> >     arch_local_irq_disable();
>> >> >
>> >> >     soft_restart(release_addr);
>> >> > }
>> >>
>> >> OK, this is a much simpler way than what I was thinking, which
>> >> was to have the secondaries spin in the kernel until the main
>> >> cpu shutdown.  I'll switch over to this, thanks.
>> >
>> > I just realised that this is still missing the jump to EL2 that I
>> > mentioned a while back.
>> >
>> > I think what we need to do is:
>> >
>> > * Have KVM (if present) tears itself down prior to cpu_die, restoring
>> >   the __hyp_stub_vectors in VBAR_EL2 and disabling the MMU, and caches.
>> >
>> > * Add a mechanism to __hyp_stub_vectors to allow a hypercall to
>> >   call a function at EL2. We should be able to replace the current
>> >   hyp_stub el1_sync handler with that, and rework KVM to call a function
>> >   at EL2 to setup VBAR_EL2 appropriately at init time.
>> >
>> Why do you need to change the current mechanism?  Is this due to the
>> CPU being in a different state when restarted with the MMU enabled in
>> EL2 or something like that?
>
> Something like that, yes.
>
> For hotplug with spin-table we need to return CPUs to the spin-table in
> the mode they entered to prevent mismatched modes when we throw those
> CPUs back into the kernel. For kexec we need to move the final CPU up to
> the mode it started in before we branch to the new kernel. If we don't
> do that then we either get mismatched modes or lose the use of EL2.
>
> Whatever mechanism we use for this needs to be independent of KVM.
> Ideally this would be in the hyp_stub vectors and we'd have KVM tear
> itself down at EL2 and restore the hyp_stub before we offline a CPU.
>
> I'd rather not have a custom set of EL2 vectors that the spin-table code
> has to install via the curent mechanism, so IMO reworking the hyp stub
> to implement a simple function call hypercall would be preferable.  KVM
> can use that to set up its vectors and the spin-table and kexec code
> could use to leave the kernel at EL2.
>
So you'd still always assume the hyp-stub mechanism has the MMU turned
off at EL2, but just make it easier for callers to deal with,
essentially. As far as I can tell, there shouldn't be any problems
converting the hyp-stub API to specify a function to call in EL2
rather than the current method of replacing the vectors.

Letting KVM tear itself down and re-establish the hyp-stub API as it
was at boot time seems completely reasonable to me.

-Christoffer