From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Nikos Nikoleris <nikos.nikoleris@arm.com>
Cc: pbonzini@redhat.com, thuth@redhat.com, andrew.jones@linux.dev,
kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [kvm-unit-tests RFC PATCH 19/19] arm/arm64: Rework the cache maintenance in asm_mmu_disable
Date: Tue, 9 Aug 2022 15:22:57 +0100 [thread overview]
Message-ID: <YvJtwWcKkcxLUVif@monolith.localdoman> (raw)
In-Reply-To: <3fba260d-bfca-14ea-7bdd-3e55f3d1e276@arm.com>
On Tue, Aug 09, 2022 at 02:53:34PM +0100, Nikos Nikoleris wrote:
> Hi Alex,
>
> On 09/08/2022 10:15, Alexandru Elisei wrote:
> > asm_mmu_disable is overly ambitious and provably incorrect:
> >
> > 1. It tries to clean and invalidate the data caches for the *entire*
> > memory, which is highly unnecessary, as it's very unlikely that a test
> > will write to the entire memory, and even more unlikely that a test will
> > modify the text section of the test image.
> >
>
> While it appears that we don't modify the text section, there is some
> loading happening before we start executing a test. Are you sure that the
> loader doesn't leave the memory dirty?
Yes, it's in the boot protocol for Linux [1]. I also mentioned this in the
commit message for the previous patch.
[1] https://elixir.bootlin.com/linux/v5.19/source/Documentation/arm64/booting.rst#L180
>
> > 2. There is no corresponding dcache invalidate command for the entire
> > memory in asm_mmu_enable, leaving it up to the test that disabled the
> > MMU to do the cache maintenance in an asymmetrical fashion: only for
> > re-enabling the MMU, but not for disabling it.
> >
> > 3. It's missing the DMB SY memory barrier to ensure that the dcache
> > maintenance is performed after the last store executed in program order
> > before calling asm_mmu_disable.
> >
>
> I am not sure why this is needed. In general, iiuc, a store to location x
> followed by a DC CVAC to x in program order don't need an barrier (see Arm
> ARM ARM DDI 0487G.b "Data cache maintenance instructions" at K11.5.1 and
Just a note, the latest public version is H.a.
K11.5.1 looks to me like it deals with ordering of the cache maintenance
operations with regards to memory accesses that are *after* the CMO in
program order, this patch is about memory accesses that are *before* the
CMO in program order.
> "Ordering and completion of data and instruction cache instructions" at
> D4-2656). It doesn't hurt to have it but I think it's unnecessary.
D4-2656 is about PAC, I assume you meant D4-2636 judging from the section
name (please correct me if I'm wrong):
"All data cache instructions, other than DC ZVA, that specify an address:
[..]
Can execute in any order relative to loads or stores that access any
address with the Device memory attribute, or with Normal memory with Inner
Non-cacheable attribute unless a DMB or DSB is executed between the
instructions."
Since the maintenance is performed with the MMU off, I think the DMB SY is
required as per the architecture.
I prefer to keep the maintenance after the MMU is disabled, to allow for
any kind of translation table setups that a test might conjure up (a test
in theory can create and install its own translation tables).
Thanks,
Alex
>
> Thanks,
>
> Nikos
>
> > Fix all of the issues in one go, by doing the cache maintenance only for
> > the stack, as that is out of the control of the C code, and add the missing
> > memory barrier.
> >
> > The code used to test that mmu_disable works correctly is similar to the
> > code used to test commit 410b3bf09e76 ("arm/arm64: Perform dcache clean
> > + invalidate after turning MMU off"), with extra cache maintenance
> > added:
> >
> > +#include <alloc_page.h>
> > +#include <asm/cacheflush.h>
> > +#include <asm/mmu.h>
> > int main(int argc, char **argv)
> > {
> > + int *x = alloc_page();
> > + bool pass = true;
> > + int i;
> > +
> > + for (i = 0; i < 1000000; i++) {
> > + *x = 0x42;
> > + dcache_clean_addr_poc((unsigned long)x);
> > + mmu_disable();
> > + if (*x != 0x42) {
> > + pass = false;
> > + break;
> > + }
> > + *x = 0x50;
> > + /* Needed for the invalidation only. */
> > + dcache_clean_inval_addr_poc((unsigned long)x);
> > + mmu_enable(current_thread_info()->pgtable);
> > + if (*x != 0x50) {
> > + pass = false;
> > + break;
> > + }
> > + }
> > + report(pass, "MMU disable cache maintenance");
> >
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> > arm/cstart.S | 11 ++++++-----
> > arm/cstart64.S | 11 +++++------
> > 2 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/arm/cstart.S b/arm/cstart.S
> > index fc7c558802f1..b27de44f30a6 100644
> > --- a/arm/cstart.S
> > +++ b/arm/cstart.S
> > @@ -242,11 +242,12 @@ asm_mmu_disable:
> > mcr p15, 0, r0, c1, c0, 0
> > isb
> > - ldr r0, =__phys_offset
> > - ldr r0, [r0]
> > - ldr r1, =__phys_end
> > - ldr r1, [r1]
> > - dcache_by_line_op dccimvac, sy, r0, r1, r2, r3
> > + dmb sy
> > + mov r0, sp
> > + lsr r0, #THREAD_SHIFT
> > + lsl r0, #THREAD_SHIFT
> > + add r1, r0, #THREAD_SIZE
> > + dcache_by_line_op dccmvac, sy, r0, r1, r3, r4
> > mov pc, lr
> > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > index 1ce6b9e14d23..af4970775298 100644
> > --- a/arm/cstart64.S
> > +++ b/arm/cstart64.S
> > @@ -283,12 +283,11 @@ asm_mmu_disable:
> > msr sctlr_el1, x0
> > isb
> > - /* Clean + invalidate the entire memory */
> > - adrp x0, __phys_offset
> > - ldr x0, [x0, :lo12:__phys_offset]
> > - adrp x1, __phys_end
> > - ldr x1, [x1, :lo12:__phys_end]
> > - dcache_by_line_op civac, sy, x0, x1, x2, x3
> > + dmb sy
> > + mov x9, sp
> > + and x9, x9, #THREAD_MASK
> > + add x10, x9, #THREAD_SIZE
> > + dcache_by_line_op cvac, sy, x9, x10, x11, x12
> > ret
next prev parent reply other threads:[~2022-08-09 14:22 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-09 9:15 [kvm-unit-tests RFC PATCH 00/19] arm/arm64: Rework cache maintenance at boot Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 01/19] Makefile: Define __ASSEMBLY__ for assembly files Alexandru Elisei
2022-08-09 12:36 ` Nikos Nikoleris
2022-09-20 8:11 ` Andrew Jones
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 02/19] lib/alloc_phys: Initialize align_min Alexandru Elisei
2022-09-20 8:20 ` Andrew Jones
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 03/19] lib/alloc_phys: Use phys_alloc_aligned_safe and rename it to memalign_early Alexandru Elisei
2022-09-20 8:27 ` Andrew Jones
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 04/19] powerpc: Use the page allocator Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 05/19] lib/alloc_phys: Remove locking Alexandru Elisei
2022-09-20 8:45 ` Andrew Jones
2022-09-20 13:20 ` Alexandru Elisei
2022-09-20 14:59 ` Andrew Jones
2022-09-26 15:04 ` Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 06/19] lib/alloc_phys: Remove allocation accounting Alexandru Elisei
2022-09-20 8:40 ` Andrew Jones
2022-09-20 13:19 ` Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 07/19] arm/arm64: Mark the phys_end parameter as unused in setup_mmu() Alexandru Elisei
2022-09-20 8:58 ` Andrew Jones
2022-09-26 11:01 ` Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 08/19] arm/arm64: Use pgd_alloc() to allocate mmu_idmap Alexandru Elisei
2022-09-20 9:05 ` Andrew Jones
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 09/19] arm/arm64: Zero secondary CPUs' stack Alexandru Elisei
2022-08-09 12:56 ` Nikos Nikoleris
2022-08-10 9:42 ` Alexandru Elisei
2022-08-10 10:00 ` Nikos Nikoleris
2022-09-20 9:24 ` Andrew Jones
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 10/19] arm/arm64: Enable the MMU early Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 11/19] arm/arm64: Map the UART when creating the translation tables Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 12/19] arm/arm64: assembler.h: Replace size with end address for dcache_by_line_op Alexandru Elisei
2022-08-09 13:01 ` Nikos Nikoleris
2022-09-20 9:37 ` Andrew Jones
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 13/19] arm: page.h: Add missing libcflat.h include Alexandru Elisei
2022-09-20 9:39 ` Andrew Jones
2022-09-26 11:02 ` Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 14/19] arm/arm64: Add C functions for doing cache maintenance Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 15/19] lib/alloc_phys: Add callback to perform " Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 16/19] arm/arm64: Allocate secondaries' stack using the page allocator Alexandru Elisei
2022-09-20 9:58 ` Andrew Jones
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 17/19] arm/arm64: Configure secondaries' stack before enabling the MMU Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 18/19] arm/arm64: Perform dcache maintenance at boot Alexandru Elisei
2022-08-09 9:15 ` [kvm-unit-tests RFC PATCH 19/19] arm/arm64: Rework the cache maintenance in asm_mmu_disable Alexandru Elisei
2022-08-09 13:53 ` Nikos Nikoleris
2022-08-09 14:22 ` Alexandru Elisei [this message]
2022-08-09 15:53 ` Nikos Nikoleris
2022-08-09 16:53 ` Alexandru Elisei
2022-08-09 19:48 ` Nikos Nikoleris
2022-08-10 8:52 ` Alexandru Elisei
2022-08-09 9:49 ` [kvm-unit-tests RFC PATCH 00/19] arm/arm64: Rework cache maintenance at boot Alexandru Elisei
2023-11-05 10:16 ` Alexandru Elisei
2023-11-06 9:37 ` Shaoqin Huang
2023-11-07 9:01 ` Alexandru Elisei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YvJtwWcKkcxLUVif@monolith.localdoman \
--to=alexandru.elisei@arm.com \
--cc=andrew.jones@linux.dev \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=nikos.nikoleris@arm.com \
--cc=pbonzini@redhat.com \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).