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()'.
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'.
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 <[email protected]>
Signed-off-by: Gwan-gyeong Mun <[email protected]>
Co-developed-by: Sangyun Kim <[email protected]>
Signed-off-by: Sangyun Kim <[email protected]>
Signed-off-by: Jeungwoo Yoo <[email protected]>
---
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
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.
> 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 <[email protected]>
> Signed-off-by: Gwan-gyeong Mun <[email protected]>
>
> Co-developed-by: Sangyun Kim <[email protected]>
> Signed-off-by: Sangyun Kim <[email protected]>
>
> Signed-off-by: Jeungwoo Yoo <[email protected]>
> ---
> 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)
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 <[email protected]>
> Signed-off-by: Gwan-gyeong Mun <[email protected]>
>
> Co-developed-by: Sangyun Kim <[email protected]>
> Signed-off-by: Sangyun Kim <[email protected]>
>
> Signed-off-by: Jeungwoo Yoo <[email protected]>
> ---
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
2022년 9월 5일 (월) 오후 12:47, Robin Murphy <[email protected]>님이 작성:
>
> 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 <[email protected]>
> > Signed-off-by: Gwan-gyeong Mun <[email protected]>
> >
> > Co-developed-by: Sangyun Kim <[email protected]>
> > Signed-off-by: Sangyun Kim <[email protected]>
> >
> > Signed-off-by: Jeungwoo Yoo <[email protected]>
> > ---
> > 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