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

[Arun Chandran]

Hi Geoff, Mark,

On Fri, Aug 15, 2014 at 11:51 PM, Mark Rutland <mark.rutland at arm.com> wrote:
> Hi Geoff,
>
> On Fri, Aug 15, 2014 at 06:20:21PM +0100, Geoff Levand wrote:
>> Hi Arun,
>>
>> On Tue, 2014-08-12 at 18:12 +0530, Arun Chandran wrote:
>> > soft_restart() will fail on arm64 systems that does not
>> > quarantee the flushing of cache to PoC with flush_cache_all().
>> >
>> > soft_restart(addr)
>> > {
>> >     push_to_stack(addr);
>> >
>> >     Do mm setup for restart;
>> >     Flush&turnoff D-cache;
>> >
>> >     pop_from_stack(addr); --> fails here as addr is not at PoC
>> >     cpu_reset(addr) --> Jumps to invalid address
>> > }
>>
>> 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?
>
> If so, what's wrong with something like:
>
> #define ADDR_INVALID ((unsigned long)-1)
> static DEFINE_PER_CPU(unsigned long, return_addr) = ADDR_INVALID;
>
> int spin_table_cpu_disable(unsigned int cpu)
> {
>         if (per_cpu(return_addr, cpu) != ADDR_INVALID)
>                 return 0;
>
>         return -EOPNOTSUPP;
> }
>
> 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);
> }
>
> [...]
>
>> > diff --git a/arch/arm64/include/asm/proc-fns.h b/arch/arm64/include/asm/proc-fns.h
>> > index 0c657bb..e18d5d0 100644
>> > --- a/arch/arm64/include/asm/proc-fns.h
>> > +++ b/arch/arm64/include/asm/proc-fns.h
>> > @@ -32,6 +32,7 @@ extern void cpu_cache_off(void);
>> >  extern void cpu_do_idle(void);
>> >  extern void cpu_do_switch_mm(unsigned long pgd_phys, struct mm_struct *mm);
>> >  extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
>> > +extern void cpu_soft_restart(unsigned long addr) __attribute__((noreturn));
>>
>> Function prototypes are never definitions, so remove this 'extern'
>> keyword.  checkpatch should have warned about this.  If it did not,
>> report it to the checkpatch maintainers.
>
> Good point.
>
> Arun, could you fix up the latest version [1] of your patch to not use
> extern for the function declaration?

Yes I will fix and name it as V1 and send.
>
> If you'd be willing to spin a preparatory patch removing the other
> externs on function declarations in asm/proc-fns.h, that would be
> appreciated. Remember to add a Reported-by for Geoff.
>
Yes I will be happy to do that :)

> Also, please remember to use a version number in the patch subject (e.g.
> "[PATCHv2] arm64: convert part of soft_restart() to assembly"), as that
> will make it easier to find the latest version in future.
>
OK. Done.

> [...]
>
>> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> > index 7736779..a7c3fce 100644
>> > --- a/arch/arm64/mm/proc.S
>> > +++ b/arch/arm64/mm/proc.S
>> > @@ -76,6 +76,40 @@ ENTRY(cpu_reset)
>> >     ret     x0
>> >  ENDPROC(cpu_reset)
>> >
>> > +   .align 3
>> > +1: .quad   memstart_addr
>> > +
>> > +ENTRY(cpu_soft_restart)
>> > +   adr     x1, cpu_reset
>> > +   adr     x2, 1b
>> > +
>> > +   /* virt_to_phys(cpu_reset) */
>> > +   ldr     x3, [x2]
>> > +   ldr     x3, [x3]
>> > +   mov     x4, #1
>> > +   lsl     x4, x4, #(VA_BITS - 1)
>> > +   add     x1, x1, x4
>> > +   add     x1, x1, x3
>> > +
>> > +   /* Save it; We can't use stack as it is going to run with caches OFF */
>> > +   mov     x19, x0
>> > +   mov     x20, x1
>> > +
>> > +   bl      setup_mm_for_reboot
>> > +
>> > +   bl      flush_cache_all
>> > +   /* Turn D-cache off */
>> > +   bl      cpu_cache_off
>> > +   /* Push out any further dirty data, and ensure cache is empty */
>> > +   bl      flush_cache_all
>>
>> It would be nice to have some blank lines above the comments.  Same
>> below.
>
> Makes sense to me. For the latest version [1], that should only mean a
> line after the call to cpu_cache_off in cpu_soft_restart.
>

Ok.
--Arun