From mboxrd@z Thu Jan  1 00:00:00 1970
From: mark.rutland@arm.com (Mark Rutland)
Date: Fri, 15 Aug 2014 19:21:57 +0100
Subject: [PATCH] Arm64: convert soft_restart() to assembly code
In-Reply-To: <1408123221.22761.38.camel@smoke>
References: <1407847365-10873-1-git-send-email-achandran@mvista.com>
 <1408123221.22761.38.camel@smoke>
Message-ID: <20140815182157.GD21908@leverpostej>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

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?

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.

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.

[...]

> > 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.

Cheers,
Mark.

[1] lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279390.html