[PATCH] arm64: Remove d-cache clean operation at preserve_boot_args().

[Jeungwoo Yoo]

2022년 9월 5일 (월) 오후 12:47, Robin Murphy <robin.murphy at arm.com>님이 작성:
>
> On 2022-09-04 20:30, 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.
> >
> > 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. 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. Therefore, it uses 'dv ivac' instruction directly instead
> > of calling 'dcache_inval_poc()'.
>
> You've already said in the first paragraph that we expect these
> locations to be clean. Clean lines are not written back, so your
> reasoning here is spurious. If boot_args has somehow become dirtied such
> that the clean operation *would* write back to memory, that can only
> mean one of two things: either the kernel image was not cleaned per the
> boot protocol, in which case there's every chance that other things will
> also go wrong elsewhere and there's not much we can do, or the prior
> stores hit in the cache (either unexpectedly or because the MMU was left
> on), in which case we almost certainly *would* want writeback here anyway.

Yes, you are right. I misunderstood that the "clean operation" will
propagate the content from the d-cache to the memory always.
However, as the cache line is already cleaned by the bootloader and
not modified in the d-cache, the "clean operation" in
"dcache_inval_poc()" will be ignored.
Thank you for your correction.

>
> > 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'.
>
> The architecture allows the CWG to be as small as 2 words, so this is
> clearly untrue.
>
> Thanks,
> Robin.
>
> > 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.
> >
> > [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 at intel.com>
> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> >
> > Co-developed-by: Sangyun Kim <sangyun.kim at snu.ac.kr>
> > Signed-off-by: Sangyun Kim <sangyun.kim at snu.ac.kr>
> >
> > Signed-off-by: Jeungwoo Yoo <casionwoo at gmail.com>
> > ---
> >   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)



-- 
Best Regards,
Jeungwoo Yoo