Re: [PATCHv2 5/6] arm64: Use __pa_symbol for _end

[Catalin Marinas]

On Tue, Nov 15, 2016 at 04:09:07PM -0800, Laura Abbott wrote:
> On 11/15/2016 10:35 AM, Catalin Marinas wrote:
> > I'm fine with __pa_symbol use entirely from under arch/arm64. But if you
> > want to use __pa_symbol, I tried to change most (all?) places where
> > necessary, together with making virt_to_phys() only deal with the kernel
> > linear mapping. Not sure it looks cleaner, especially the
> > __va(__pa_symbol()) cases (we could replace the latter with another
> > macro and proper comment):
> 
> I agree everything should be converted over, I was considering doing
> that in a separate patch but this covers everything nicely. Are you
> okay with me folding this in? (Few comments below)

Yes. I would also like Ard to review it since he introduced the current
__virt_to_phys() macro.

> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index eac3dbb7e313..e02f45e5ee1b 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -169,15 +169,22 @@ extern u64			kimage_voffset;
> >   */
> >  #define __virt_to_phys_nodebug(x) ({					\
> >  	phys_addr_t __x = (phys_addr_t)(x);				\
> > -	__x & BIT(VA_BITS - 1) ? (__x & ~PAGE_OFFSET) + PHYS_OFFSET :	\
> > -				 (__x - kimage_voffset); })
> > +	VM_BUG_ON(!(__x & BIT(VA_BITS - 1)));				\
> > +	((__x & ~PAGE_OFFSET) + PHYS_OFFSET);				\
> > +})
> 
> I do think this is easier to understand vs the ternary operator.
> I'll add a comment detailing the use of __pa vs __pa_symbol somewhere
> as well.

Of course, a comment is welcome (I just did a quick hack to check that
it works).

> > --- a/arch/arm64/include/asm/mmu_context.h
> > +++ b/arch/arm64/include/asm/mmu_context.h
> > @@ -44,7 +44,7 @@ static inline void contextidr_thread_switch(struct task_struct *next)
> >   */
> >  static inline void cpu_set_reserved_ttbr0(void)
> >  {
> > -	unsigned long ttbr = virt_to_phys(empty_zero_page);
> > +	unsigned long ttbr = __pa_symbol(empty_zero_page);
> >  
> >  	write_sysreg(ttbr, ttbr0_el1);
> >  	isb();
> > @@ -113,7 +113,7 @@ static inline void cpu_install_idmap(void)
> >  	local_flush_tlb_all();
> >  	cpu_set_idmap_tcr_t0sz();
> >  
> > -	cpu_switch_mm(idmap_pg_dir, &init_mm);
> > +	cpu_switch_mm(__va(__pa_symbol(idmap_pg_dir)), &init_mm);
> 
> Yes, the __va(__pa_symbol(..)) idiom needs to be macroized and commented...

Indeed. At the same time we should also replace the LMADDR macro in
hibernate.c with whatever you come up with.

> > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
> > index d55a7b09959b..81c03c74e5fe 100644
> > --- a/arch/arm64/kernel/hibernate.c
> > +++ b/arch/arm64/kernel/hibernate.c
> > @@ -51,7 +51,7 @@
> >  extern int in_suspend;
> >  
> >  /* Find a symbols alias in the linear map */
> > -#define LMADDR(x)	phys_to_virt(virt_to_phys(x))
> > +#define LMADDR(x)	__va(__pa_symbol(x))
> 
> ...Perhaps just borrowing this macro?

Yes but I don't particularly like the name, especially since it goes
into a .h file. Maybe __lm_sym_addr() or something else if you have a
better idea.

> > diff --git a/arch/arm64/mm/physaddr.c b/arch/arm64/mm/physaddr.c
> > index 874c78201a2b..98dae943e496 100644
> > --- a/arch/arm64/mm/physaddr.c
> > +++ b/arch/arm64/mm/physaddr.c
> > @@ -14,8 +14,8 @@ unsigned long __virt_to_phys(unsigned long x)
> >  		 */
> >  		return (__x & ~PAGE_OFFSET) + PHYS_OFFSET;
> >  	} else {
> > -		VIRTUAL_BUG_ON(x < kimage_vaddr || x >= (unsigned long)_end);
> > -		return (__x - kimage_voffset);
> > +		WARN_ON(1);
> 
> Was the deletion of the BUG_ON here intentional? VIRTUAL_BUG_ON
> is the check enabled by CONFIG_DEBUG_VIRTUAL vs just CONFIG_DEBUG_VM.
> I intentionally kept CONFIG_DEBUG_VIRTUAL separate since the checks
> are expensive.

I wanted to always get a warning but fall back to __phys_addr_symbol()
so that I can track down other uses of __virt_to_phys() on kernel
symbols without killing the kernel. A better option would have been
VIRTUAL_WARN_ON (or *_ONCE) but we don't have it. VM_WARN_ON, as you
said, is independent of CONFIG_DEBUG_VIRTUAL.

We could as well kill the system with VIRTUAL_BUG_ON in this case but I
thought we should be more gentle until all the __virt_to_phys use-cases
are sorted out.

-- 
Catalin