Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Sep 2022 19:21:24 +0900
From:   Hyeonggon Yoo <42.hyeyoo@gmail.com>
To:     Jeungwoo Yoo <casionwoo@gmail.com>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>,
        Sangyun Kim <sangyun.kim@snu.ac.kr>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64: Remove d-cache clean operation at
 preserve_boot_args().
Message-ID: <YxXNpJlWdDvZqJKQ@hyeyoo>
References: <20220904193020.65556-1-casionwoo@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220904193020.65556-1-casionwoo@gmail.com>
Precedence: bulk

note that I am not an expert on arm64 so I may be wrong,
and I'll be happy to be proven wrong ;)

On Sun, Sep 04, 2022 at 09:30:19PM +0200, Jeungwoo Yoo wrote:
> Kernel expects only the clean operation as a booting requirement in
> arm64 architecture [1], therefore, the kernel has to invalidate any
> cache entries after accessing a memory in the booting time (before
> enabling D-cache and MMU) not to overwrite the memory with the stale
> cache entry.

Yes.

> Same applied in preserve_boot_args(), kernel saves boot arguments into
> 'boot_args' and invalidates the corresponding cache entry. However,
> according to the 'dcache_inval_poc()' implementation, the cache entry
> will be not only invalidated but also cleaned.

Yeah, that's when @start or @end passed to dcache_inval_poc() is not aligned to
cache line size. and @end may not be aligned if cache line size is not 32.
(@start is always aligned)


> That means if there is a
> stale cache entry corresponding to the address of the 'boot_args', the
> saved boot arguments in 'boot_args' will be overwritten by the stale
> cache entry.

To clarify, "If existing cache entry became stale after writing to memory..."

> Therefore, it uses 'dv ivac' instruction directly instead
> of calling 'dcache_inval_poc()'.
> 
> The address of the 'boot_args' is aligned to the cache line size and the
> size of 'boot_args' is 32 byte (8 byte * 4), therefore, a single
> invalidate operation is enough to invalidate the cache line belonging to
> the 'boot_args'.

Is the cache line size always >= 32 bytes? 
If I'm not mistaken, the minimum size is 16 bytes.

> Sometimes clean operation is required not to lose any contents in the
> cache entry but not the target of the invalidation. However, in this
> case, there is no valid cache entries at a very early booting stage and
> preserve_boot_args() is not called by any other (non-primary) CPUs.
> Therefore, this invalidation operation will not introduce any problems.

I think this patch makes sense. Losing data in invalidation
operation is not a concern for booting process. And it may lead to writing
stale data to memory if:

	- a boot loader only cleans address range corresponding to
	  kernel image (and not invalidate) according to booting.rst

Thanks!

> [1] in Documentation/arm64/booting.rst:
> The address range corresponding to the loaded kernel image must be
> cleaned to the PoC.
> 
> Co-developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> 
> Co-developed-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> 
> Signed-off-by: Jeungwoo Yoo <casionwoo@gmail.com>
> ---

there should be no newlines between tags :)

>  arch/arm64/kernel/head.S | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index cefe6a73ee54..916227666b07 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -121,9 +121,7 @@ SYM_CODE_START_LOCAL(preserve_boot_args)
>  
>  	dmb	sy				// needed before dc ivac with
>  						// MMU off
> -
> -	add	x1, x0, #0x20			// 4 x 8 bytes
> -	b	dcache_inval_poc		// tail call
> +	dc	ivac, x0			// Invalidate potentially stale cache line
>  SYM_CODE_END(preserve_boot_args)
>  
>  SYM_FUNC_START_LOCAL(clear_page_tables)
> -- 
> 2.34.3
> 

-- 
Thanks,
Hyeonggon