Re: [PATCH V4 06/11] arm64: mm: Introduce VA_BITS_ACTUAL

From: Steve Capper <Steve.Capper@arm.com>
To: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: "crecklin@redhat.com" <crecklin@redhat.com>,
	"ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
	"maz@kernel.org" <maz@kernel.org>,
	"bhsharma@redhat.com" <bhsharma@redhat.com>, nd <nd@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH V4 06/11] arm64: mm: Introduce VA_BITS_ACTUAL
Date: Tue, 6 Aug 2019 11:32:04 +0000	[thread overview]
Message-ID: <20190806113202.GA11778@capper-ampere.manchester.arm.com> (raw)
In-Reply-To: <20190805172643.GM4175@arrakis.emea.arm.com>

Hi Catalin,

On Mon, Aug 05, 2019 at 06:26:43PM +0100, Catalin Marinas wrote:
> On Mon, Jul 29, 2019 at 05:21:12PM +0100, Steve Capper wrote:
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index a8a91a573bff..93341f4fe840 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -37,8 +37,6 @@
> >   * VA_START - the first kernel virtual address.
> >   */
> >  #define VA_BITS			(CONFIG_ARM64_VA_BITS)
> > -#define VA_START		(UL(0xffffffffffffffff) - \
> > -	(UL(1) << (VA_BITS - 1)) + 1)
> >  #define PAGE_OFFSET		(UL(0xffffffffffffffff) - \
> >  	(UL(1) << VA_BITS) + 1)
> >  #define KIMAGE_VADDR		(MODULES_END)
> > @@ -166,10 +164,14 @@
> >  #endif
> >  
> >  #ifndef __ASSEMBLY__
> > +extern u64			vabits_actual;
> > +#define VA_BITS_ACTUAL		({vabits_actual;})
> 
> Why not use the variable vabits_actual directly instead of defining a
> macro?
> 

I thought that it would look better to have an uppercase name for the
actual VA bits to match the existing code style for VA_BITS.

I can just rename vabits_actual => VA_BITS_ACTUAL and get rid of the
macro?

> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index ac58c69993ec..6dc7349868d9 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -321,6 +321,11 @@ __create_page_tables:
> >  	dmb	sy
> >  	dc	ivac, x6		// Invalidate potentially stale cache line
> >  
> > +	adr_l	x6, vabits_actual
> > +	str	x5, [x6]
> > +	dmb	sy
> > +	dc	ivac, x6		// Invalidate potentially stale cache line
> 
> Can we not replace vabits_user with vabits_actual and have a single
> write? Maybe not in this patch but once the series is applied, they are
> practically the same. It could be an additional patch (or define a
> vabits_user macro as vabits_actual).
> 

Thanks, I think it may be better to consolidate these in an extra patch (just
before the documentation patch). I'll add this to the series.

Cheers,
-- 
Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel