* [RFC PATCH] arm: Fix cache inconsistency when using fixmap
@ 2017-02-22 15:51 Jon Medhurst (Tixy)
2017-02-25 1:06 ` Stefan Agner
0 siblings, 1 reply; 9+ messages in thread
From: Jon Medhurst (Tixy) @ 2017-02-22 15:51 UTC (permalink / raw)
To: linux-arm-kernel
Cacheable memory mappings need to be marked as shareable otherwise the
CPU accessing fixmap memory may end up with local cachelines for that
memory, resulting in different CPUs in the system seeing different
values for the same memory location.
This issue was discovered when investigating failures in the ARM kprobe
tests. kprobes uses patch_text() to modify the kernel image, which
when CONFIG_DEBUG_RODATA is enabled makes use of fixmap to map the page
to be modified. As the shareable attribute wasn't being set in the PTE
entry for that page, a write to it wasn't being broadcast to other
caches in the system, which in the the case being investigated was
CPU's in the other cluster of a big.LITTLE system.
Fixes: a5f4c561b3b1 ("ARM: 8415/1: early fixmap support for earlycon")
Cc: stable at vger.kernel.org # v4.3+
Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
The fixmap changes in the original commit a5f4c561b3b1 look a bit iffy
to me, shouldn't it have have made use of PAGE_KERNEL or pgprot_kernel
or something like that to get PTE attributes suitable for the system
being run, rather than rolling it's own set of values?
That's why I'm marking this as an RFC, perhaps the correct fix is to
rework the code to use the defines from arch/arm/include/asm/pgtable.h
arch/arm/include/asm/fixmap.h | 2 +-
arch/arm/probes/kprobes/test-core.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h
index 5c17d2dec777..26d4a4677cd7 100644
--- a/arch/arm/include/asm/fixmap.h
+++ b/arch/arm/include/asm/fixmap.h
@@ -41,7 +41,7 @@ static const enum fixed_addresses __end_of_fixed_addresses =
#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY)
-#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)
+#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_SHARED | L_PTE_MT_WRITEBACK)
#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY)
/* Used by set_fixmap_(io|nocache), both meant for mapping a device */
diff --git a/arch/arm/probes/kprobes/test-core.h b/arch/arm/probes/kprobes/test-core.h
index 94285203e9f7..cde2b4e9358a 100644
--- a/arch/arm/probes/kprobes/test-core.h
+++ b/arch/arm/probes/kprobes/test-core.h
@@ -8,7 +8,7 @@
* published by the Free Software Foundation.
*/
-#define VERBOSE 0 /* Set to '1' for more logging of test cases */
+#define VERBOSE 1 /* Set to '1' for more logging of test cases */
#ifdef CONFIG_THUMB2_KERNEL
#define NORMAL_ISA "16"
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-22 15:51 [RFC PATCH] arm: Fix cache inconsistency when using fixmap Jon Medhurst (Tixy) @ 2017-02-25 1:06 ` Stefan Agner 2017-02-26 14:35 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Stefan Agner @ 2017-02-25 1:06 UTC (permalink / raw) To: linux-arm-kernel [also added Ard] On 2017-02-22 07:51, Jon Medhurst (Tixy) wrote: > Cacheable memory mappings need to be marked as shareable otherwise the > CPU accessing fixmap memory may end up with local cachelines for that > memory, resulting in different CPUs in the system seeing different > values for the same memory location. > > This issue was discovered when investigating failures in the ARM kprobe > tests. kprobes uses patch_text() to modify the kernel image, which > when CONFIG_DEBUG_RODATA is enabled makes use of fixmap to map the page > to be modified. As the shareable attribute wasn't being set in the PTE > entry for that page, a write to it wasn't being broadcast to other > caches in the system, which in the the case being investigated was > CPU's in the other cluster of a big.LITTLE system. > > Fixes: a5f4c561b3b1 ("ARM: 8415/1: early fixmap support for earlycon") > Cc: stable at vger.kernel.org # v4.3+ > > Signed-off-by: Jon Medhurst <tixy@linaro.org> > --- > > The fixmap changes in the original commit a5f4c561b3b1 look a bit iffy > to me, shouldn't it have have made use of PAGE_KERNEL or pgprot_kernel > or something like that to get PTE attributes suitable for the system > being run, rather than rolling it's own set of values? The PAGE_KERNEL points to pgprot_kernel, which in turn is dynamically setup in build_mem_type_table. This initialization is taking place late in the boot process, too late for early fixmap... Early fixmap gets shut down before going SMP, so non shared mappings are not an issue there. But FIXMAP_PAGE_NORMAL is also used for regular fixmap, which is essentially the problem. Also, regular fixmap does not make sure of the cache policy due to that, but just uses the hardcoded writeback policy... With that in mind, populating kern_pgprot with a reasonable default which works for early fixmap might be a reasonable approach. Later kern_pgprot gets setup with the appropriate shared attribute and cache policy... diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h index 5c17d2dec777..c663e562716c 100644 --- a/arch/arm/include/asm/fixmap.h +++ b/arch/arm/include/asm/fixmap.h @@ -39,12 +39,11 @@ static const enum fixed_addresses __end_of_fixed_addresses = __end_of_fixmap_region > __end_of_early_ioremap_region ? __end_of_fixmap_region : __end_of_early_ioremap_region; -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY) - -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) +#define FIXMAP_PAGE_NORMAL (pgprot_kernel | L_PTE_XN) #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ +#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY) #define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | L_PTE_SHARED) #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 4001dd15818d..9cb7c559263f 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -65,7 +65,8 @@ pmdval_t user_pmd_table = _PAGE_USER_TABLE; static unsigned int cachepolicy __initdata = CPOLICY_WRITEBACK; static unsigned int ecc_mask __initdata = 0; pgprot_t pgprot_user; -pgprot_t pgprot_kernel; +pgprot_t pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | + L_PTE_DIRTY | L_PTE_MT_WRITEBACK); pgprot_t pgprot_hyp_device; pgprot_t pgprot_s2; pgprot_t pgprot_s2_device; > That's why I'm marking this as an RFC, perhaps the correct fix is to > rework the code to use the defines from arch/arm/include/asm/pgtable.h > > arch/arm/include/asm/fixmap.h | 2 +- > arch/arm/probes/kprobes/test-core.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > index 5c17d2dec777..26d4a4677cd7 100644 > --- a/arch/arm/include/asm/fixmap.h > +++ b/arch/arm/include/asm/fixmap.h > @@ -41,7 +41,7 @@ static const enum fixed_addresses __end_of_fixed_addresses = > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | > L_PTE_DIRTY) > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > +#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_SHARED | > L_PTE_MT_WRITEBACK) > #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > diff --git a/arch/arm/probes/kprobes/test-core.h > b/arch/arm/probes/kprobes/test-core.h > index 94285203e9f7..cde2b4e9358a 100644 > --- a/arch/arm/probes/kprobes/test-core.h > +++ b/arch/arm/probes/kprobes/test-core.h > @@ -8,7 +8,7 @@ > * published by the Free Software Foundation. > */ > > -#define VERBOSE 0 /* Set to '1' for more logging of test cases */ > +#define VERBOSE 1 /* Set to '1' for more logging of test cases */ That seems to be unrelated. -- Stefan > > #ifdef CONFIG_THUMB2_KERNEL > #define NORMAL_ISA "16" ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-25 1:06 ` Stefan Agner @ 2017-02-26 14:35 ` Ard Biesheuvel 2017-02-26 14:44 ` Ard Biesheuvel 2017-02-27 14:35 ` Jon Medhurst (Tixy) 0 siblings, 2 replies; 9+ messages in thread From: Ard Biesheuvel @ 2017-02-26 14:35 UTC (permalink / raw) To: linux-arm-kernel On 25 February 2017 at 01:06, Stefan Agner <stefan@agner.ch> wrote: > [also added Ard] > Thanks > On 2017-02-22 07:51, Jon Medhurst (Tixy) wrote: >> Cacheable memory mappings need to be marked as shareable otherwise the >> CPU accessing fixmap memory may end up with local cachelines for that >> memory, resulting in different CPUs in the system seeing different >> values for the same memory location. >> >> This issue was discovered when investigating failures in the ARM kprobe >> tests. kprobes uses patch_text() to modify the kernel image, which >> when CONFIG_DEBUG_RODATA is enabled makes use of fixmap to map the page >> to be modified. As the shareable attribute wasn't being set in the PTE >> entry for that page, a write to it wasn't being broadcast to other >> caches in the system, which in the the case being investigated was >> CPU's in the other cluster of a big.LITTLE system. >> >> Fixes: a5f4c561b3b1 ("ARM: 8415/1: early fixmap support for earlycon") >> Cc: stable at vger.kernel.org # v4.3+ >> >> Signed-off-by: Jon Medhurst <tixy@linaro.org> >> --- >> >> The fixmap changes in the original commit a5f4c561b3b1 look a bit iffy >> to me, shouldn't it have have made use of PAGE_KERNEL or pgprot_kernel >> or something like that to get PTE attributes suitable for the system >> being run, rather than rolling it's own set of values? > > The PAGE_KERNEL points to pgprot_kernel, which in turn is dynamically > setup in build_mem_type_table. This initialization is taking place late > in the boot process, too late for early fixmap... > > Early fixmap gets shut down before going SMP, so non shared mappings are > not an issue there. But FIXMAP_PAGE_NORMAL is also used for regular > fixmap, which is essentially the problem. Also, regular fixmap does not > make sure of the cache policy due to that, but just uses the hardcoded > writeback policy... > > With that in mind, populating kern_pgprot with a reasonable default > which works for early fixmap might be a reasonable approach. Later > kern_pgprot gets setup with the appropriate shared attribute and cache > policy... > > diff --git a/arch/arm/include/asm/fixmap.h > b/arch/arm/include/asm/fixmap.h > index 5c17d2dec777..c663e562716c 100644 > --- a/arch/arm/include/asm/fixmap.h > +++ b/arch/arm/include/asm/fixmap.h > @@ -39,12 +39,11 @@ static const enum fixed_addresses > __end_of_fixed_addresses = > __end_of_fixmap_region > __end_of_early_ioremap_region ? > __end_of_fixmap_region : __end_of_early_ioremap_region; > > -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN > | L_PTE_DIRTY) > - > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | > L_PTE_MT_WRITEBACK) > +#define FIXMAP_PAGE_NORMAL (pgprot_kernel | L_PTE_XN) > #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > +#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN > | L_PTE_DIRTY) > #define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | > L_PTE_MT_DEV_SHARED | L_PTE_SHARED) > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO > > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c > index 4001dd15818d..9cb7c559263f 100644 > --- a/arch/arm/mm/mmu.c > +++ b/arch/arm/mm/mmu.c > @@ -65,7 +65,8 @@ pmdval_t user_pmd_table = _PAGE_USER_TABLE; > static unsigned int cachepolicy __initdata = CPOLICY_WRITEBACK; > static unsigned int ecc_mask __initdata = 0; > pgprot_t pgprot_user; > -pgprot_t pgprot_kernel; > +pgprot_t pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | > + L_PTE_DIRTY | L_PTE_MT_WRITEBACK); > pgprot_t pgprot_hyp_device; > pgprot_t pgprot_s2; > pgprot_t pgprot_s2_device; > This looks correct to me, and avoids using shareable attributes inadvertently. However, we could make this a bit more private to the fixmap code by using something like diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h index 5c17d2dec777..6c375aea8f22 100644 --- a/arch/arm/include/asm/fixmap.h +++ b/arch/arm/include/asm/fixmap.h @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses = #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY) -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?: FIXMAP_PAGE_COMMON) | \ + L_PTE_MT_WRITEBACK) +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL) +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY) /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | L_PTE_SHARED) +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | \ + L_PTE_SHARED) #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO #define __early_set_fixmap __set_fixmap (and drop the change to mm/mmu.c), which boils down to the same for fixmap but does not affect other users of pgprot_kernel. Also, it appears these definitions are broken under STRICT_MM_TYPECHECKS so this is a good opportunity to get that fixed as well. Thank, Ard. > > >> That's why I'm marking this as an RFC, perhaps the correct fix is to >> rework the code to use the defines from arch/arm/include/asm/pgtable.h >> >> arch/arm/include/asm/fixmap.h | 2 +- >> arch/arm/probes/kprobes/test-core.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >> index 5c17d2dec777..26d4a4677cd7 100644 >> --- a/arch/arm/include/asm/fixmap.h >> +++ b/arch/arm/include/asm/fixmap.h >> @@ -41,7 +41,7 @@ static const enum fixed_addresses __end_of_fixed_addresses = >> >> #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | >> L_PTE_DIRTY) >> >> -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> +#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_SHARED | >> L_PTE_MT_WRITEBACK) >> #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> >> /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> diff --git a/arch/arm/probes/kprobes/test-core.h >> b/arch/arm/probes/kprobes/test-core.h >> index 94285203e9f7..cde2b4e9358a 100644 >> --- a/arch/arm/probes/kprobes/test-core.h >> +++ b/arch/arm/probes/kprobes/test-core.h >> @@ -8,7 +8,7 @@ >> * published by the Free Software Foundation. >> */ >> >> -#define VERBOSE 0 /* Set to '1' for more logging of test cases */ >> +#define VERBOSE 1 /* Set to '1' for more logging of test cases */ > > That seems to be unrelated. > > -- > Stefan > >> >> #ifdef CONFIG_THUMB2_KERNEL >> #define NORMAL_ISA "16" ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-26 14:35 ` Ard Biesheuvel @ 2017-02-26 14:44 ` Ard Biesheuvel 2017-02-27 14:35 ` Jon Medhurst (Tixy) 1 sibling, 0 replies; 9+ messages in thread From: Ard Biesheuvel @ 2017-02-26 14:44 UTC (permalink / raw) To: linux-arm-kernel On 26 February 2017 at 14:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 25 February 2017 at 01:06, Stefan Agner <stefan@agner.ch> wrote: [...] >> Early fixmap gets shut down before going SMP, so non shared mappings are >> not an issue there. But FIXMAP_PAGE_NORMAL is also used for regular >> fixmap, which is essentially the problem. Also, regular fixmap does not >> make sure of the cache policy due to that, but just uses the hardcoded >> writeback policy... >> >> With that in mind, populating kern_pgprot with a reasonable default >> which works for early fixmap might be a reasonable approach. Later >> kern_pgprot gets setup with the appropriate shared attribute and cache >> policy... >> >> diff --git a/arch/arm/include/asm/fixmap.h >> b/arch/arm/include/asm/fixmap.h >> index 5c17d2dec777..c663e562716c 100644 >> --- a/arch/arm/include/asm/fixmap.h >> +++ b/arch/arm/include/asm/fixmap.h >> @@ -39,12 +39,11 @@ static const enum fixed_addresses >> __end_of_fixed_addresses = >> __end_of_fixmap_region > __end_of_early_ioremap_region ? >> __end_of_fixmap_region : __end_of_early_ioremap_region; >> >> -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN >> | L_PTE_DIRTY) >> - >> -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | >> L_PTE_MT_WRITEBACK) >> +#define FIXMAP_PAGE_NORMAL (pgprot_kernel | L_PTE_XN) >> #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> >> /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> +#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN >> | L_PTE_DIRTY) >> #define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | >> L_PTE_MT_DEV_SHARED | L_PTE_SHARED) >> #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO >> >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c >> index 4001dd15818d..9cb7c559263f 100644 >> --- a/arch/arm/mm/mmu.c >> +++ b/arch/arm/mm/mmu.c >> @@ -65,7 +65,8 @@ pmdval_t user_pmd_table = _PAGE_USER_TABLE; >> static unsigned int cachepolicy __initdata = CPOLICY_WRITEBACK; >> static unsigned int ecc_mask __initdata = 0; >> pgprot_t pgprot_user; >> -pgprot_t pgprot_kernel; >> +pgprot_t pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | >> + L_PTE_DIRTY | L_PTE_MT_WRITEBACK); >> pgprot_t pgprot_hyp_device; >> pgprot_t pgprot_s2; >> pgprot_t pgprot_s2_device; >> > > This looks correct to me, and avoids using shareable attributes inadvertently. > > However, we could make this a bit more private to the fixmap code by > using something like > > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > index 5c17d2dec777..6c375aea8f22 100644 > --- a/arch/arm/include/asm/fixmap.h > +++ b/arch/arm/include/asm/fixmap.h > @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses = > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | > L_PTE_XN | L_PTE_DIRTY) > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?: > FIXMAP_PAGE_COMMON) | \ > + L_PTE_MT_WRITEBACK) ... only this L_PTE_MT_WRITEBACK belongs next to FIXMAP_PAGE_COMMON, since pgprot_kernel will already have a memory type index set, which may conflict with MT_WRITEBACK > +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL) > +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY) > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | > L_PTE_MT_DEV_SHARED | L_PTE_SHARED) > +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON | > L_PTE_MT_DEV_SHARED | \ > + L_PTE_SHARED) > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO > > #define __early_set_fixmap __set_fixmap > > (and drop the change to mm/mmu.c), which boils down to the same for > fixmap but does not affect other users of pgprot_kernel. Also, it > appears these definitions are broken under STRICT_MM_TYPECHECKS so > this is a good opportunity to get that fixed as well. > > Thank, > Ard. > > >> >> >>> That's why I'm marking this as an RFC, perhaps the correct fix is to >>> rework the code to use the defines from arch/arm/include/asm/pgtable.h >>> >>> arch/arm/include/asm/fixmap.h | 2 +- >>> arch/arm/probes/kprobes/test-core.h | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >>> index 5c17d2dec777..26d4a4677cd7 100644 >>> --- a/arch/arm/include/asm/fixmap.h >>> +++ b/arch/arm/include/asm/fixmap.h >>> @@ -41,7 +41,7 @@ static const enum fixed_addresses __end_of_fixed_addresses = >>> >>> #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | >>> L_PTE_DIRTY) >>> >>> -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >>> +#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_SHARED | >>> L_PTE_MT_WRITEBACK) >>> #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >>> >>> /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >>> diff --git a/arch/arm/probes/kprobes/test-core.h >>> b/arch/arm/probes/kprobes/test-core.h >>> index 94285203e9f7..cde2b4e9358a 100644 >>> --- a/arch/arm/probes/kprobes/test-core.h >>> +++ b/arch/arm/probes/kprobes/test-core.h >>> @@ -8,7 +8,7 @@ >>> * published by the Free Software Foundation. >>> */ >>> >>> -#define VERBOSE 0 /* Set to '1' for more logging of test cases */ >>> +#define VERBOSE 1 /* Set to '1' for more logging of test cases */ >> >> That seems to be unrelated. >> >> -- >> Stefan >> >>> >>> #ifdef CONFIG_THUMB2_KERNEL >>> #define NORMAL_ISA "16" ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-26 14:35 ` Ard Biesheuvel 2017-02-26 14:44 ` Ard Biesheuvel @ 2017-02-27 14:35 ` Jon Medhurst (Tixy) 2017-02-27 18:40 ` Stefan Agner 1 sibling, 1 reply; 9+ messages in thread From: Jon Medhurst (Tixy) @ 2017-02-27 14:35 UTC (permalink / raw) To: linux-arm-kernel On Sun, 2017-02-26 at 14:35 +0000, Ard Biesheuvel wrote: > However, we could make this a bit more private to the fixmap code by > using something like > > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > index 5c17d2dec777..6c375aea8f22 100644 > --- a/arch/arm/include/asm/fixmap.h > +++ b/arch/arm/include/asm/fixmap.h > @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses = > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | > L_PTE_XN | L_PTE_DIRTY) > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?: The code above misses out setting L_PTE_XN when using pgprot_kernel > FIXMAP_PAGE_COMMON) | \ > + L_PTE_MT_WRITEBACK) > +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL) > +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY) > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | > L_PTE_MT_DEV_SHARED | L_PTE_SHARED) > +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON | > L_PTE_MT_DEV_SHARED | \ > + L_PTE_SHARED) > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO > > #define __early_set_fixmap __set_fixmap > > (and drop the change to mm/mmu.c), which boils down to the same for > fixmap but does not affect other users of pgprot_kernel. Also, it > appears these definitions are broken under STRICT_MM_TYPECHECKS so > this is a good opportunity to get that fixed as well. I like this method better because as you say it keeps things private to fixmap, and it doesn't risk affecting other things. As for getting STRICT_MM_TYPECHECKS working that looks good too, but should be a separate cleanup patch, especially as a fix for the cache problem possibly should go to stable kernels. I also think FIXMAP_PAGE_COMMON should be renamed (prefixed with '__') as it's an implementation detail an not a memory type used with fixmap. So I was thinking, one patch as a bugfix: ---------------------------------------------------------------------- diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h index 5c17d2dec777..4e6784dc5668 100644 --- a/arch/arm/include/asm/fixmap.h +++ b/arch/arm/include/asm/fixmap.h @@ -41,7 +41,8 @@ static const enum fixed_addresses __end_of_fixed_addresses = #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY) -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) +#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ + FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ ---------------------------------------------------------------------- Second patch as a cleanup. Note this is different to Ard's version as it uses PAGE_KERNEL rather than open coding the same code from pgtable.h to add XN permisions (Also, the default generic definition of FIXMAP_PAGE_NORMAL is PAGE_KERNEL, and the only reason we want to change it is to select an alternate value if that is not yet setup) ---------------------------------------------------------------------- diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h index 4e6784dc5668..ec689c6aa644 100644 --- a/arch/arm/include/asm/fixmap.h +++ b/arch/arm/include/asm/fixmap.h @@ -39,14 +39,15 @@ static const enum fixed_addresses __end_of_fixed_addresses = __end_of_fixmap_region > __end_of_early_ioremap_region ? __end_of_fixmap_region : __end_of_early_ioremap_region; -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY) +#define __FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY) -#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ - FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) +#define FIXMAP_PAGE_NORMAL (pgprot_val(pgprot_kernel) ? PAGE_KERNEL : \ + __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)) +#define FIXMAP_PAGE_RO _MOD_PROT(FIXMAP_PAGE_NORMAL, L_PTE_RDONLY) /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | L_PTE_SHARED) +#define FIXMAP_PAGE_IO __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | \ + L_PTE_SHARED) #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO #define __early_set_fixmap __set_fixmap ---------------------------------------------------------------------- ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-27 14:35 ` Jon Medhurst (Tixy) @ 2017-02-27 18:40 ` Stefan Agner 2017-02-27 18:43 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Stefan Agner @ 2017-02-27 18:40 UTC (permalink / raw) To: linux-arm-kernel On 2017-02-27 06:35, Jon Medhurst (Tixy) wrote: > On Sun, 2017-02-26 at 14:35 +0000, Ard Biesheuvel wrote: > >> However, we could make this a bit more private to the fixmap code by >> using something like >> >> >> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >> index 5c17d2dec777..6c375aea8f22 100644 >> --- a/arch/arm/include/asm/fixmap.h >> +++ b/arch/arm/include/asm/fixmap.h >> @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses = >> >> #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | >> L_PTE_XN | L_PTE_DIRTY) >> >> -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?: > > The code above misses out setting L_PTE_XN when using pgprot_kernel > >> FIXMAP_PAGE_COMMON) | \ >> + L_PTE_MT_WRITEBACK) >> +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL) >> +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY) >> >> /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | >> L_PTE_MT_DEV_SHARED | L_PTE_SHARED) >> +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON | >> L_PTE_MT_DEV_SHARED | \ >> + L_PTE_SHARED) >> #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO >> >> #define __early_set_fixmap __set_fixmap >> >> (and drop the change to mm/mmu.c), which boils down to the same for >> fixmap but does not affect other users of pgprot_kernel. Also, it >> appears these definitions are broken under STRICT_MM_TYPECHECKS so >> this is a good opportunity to get that fixed as well. > > I like this method better because as you say it keeps things private to > fixmap, and it doesn't risk affecting other things. Yes, I agree this is nicer than my proposal. > > As for getting STRICT_MM_TYPECHECKS working that looks good too, but > should be a separate cleanup patch, especially as a fix for the cache > problem possibly should go to stable kernels. > > I also think FIXMAP_PAGE_COMMON should be renamed (prefixed with '__') > as it's an implementation detail an not a memory type used with fixmap. > > So I was thinking, one patch as a bugfix: > > ---------------------------------------------------------------------- > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > index 5c17d2dec777..4e6784dc5668 100644 > --- a/arch/arm/include/asm/fixmap.h > +++ b/arch/arm/include/asm/fixmap.h > @@ -41,7 +41,8 @@ static const enum fixed_addresses __end_of_fixed_addresses = > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | > L_PTE_DIRTY) > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > +#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ > + FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > ---------------------------------------------------------------------- > > Second patch as a cleanup. Note this is different to Ard's version as > it uses PAGE_KERNEL rather than open coding the same code from > pgtable.h to add XN permisions (Also, the default generic definition of > FIXMAP_PAGE_NORMAL is PAGE_KERNEL, and the only reason we want to change > it is to select an alternate value if that is not yet setup) > > ---------------------------------------------------------------------- > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > index 4e6784dc5668..ec689c6aa644 100644 > --- a/arch/arm/include/asm/fixmap.h > +++ b/arch/arm/include/asm/fixmap.h > @@ -39,14 +39,15 @@ static const enum fixed_addresses __end_of_fixed_addresses = > __end_of_fixmap_region > __end_of_early_ioremap_region ? > __end_of_fixmap_region : __end_of_early_ioremap_region; > > -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | > L_PTE_DIRTY) > +#define __FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN > | L_PTE_DIRTY) > > -#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ > - FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > +#define FIXMAP_PAGE_NORMAL (pgprot_val(pgprot_kernel) ? PAGE_KERNEL : \ > + __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)) > +#define FIXMAP_PAGE_RO _MOD_PROT(FIXMAP_PAGE_NORMAL, L_PTE_RDONLY) > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | > L_PTE_SHARED) > +#define FIXMAP_PAGE_IO __pgprot(__FIXMAP_PAGE_COMMON | > L_PTE_MT_DEV_SHARED | \ > + L_PTE_SHARED) > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO > > #define __early_set_fixmap __set_fixmap > ---------------------------------------------------------------------- Fix and cleanup looks good to me, Reviewed-by: Stefan Agner <stefan@agner.ch> -- Stefan ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-27 18:40 ` Stefan Agner @ 2017-02-27 18:43 ` Ard Biesheuvel 2017-02-27 18:56 ` Jon Medhurst (Tixy) 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2017-02-27 18:43 UTC (permalink / raw) To: linux-arm-kernel On 27 February 2017 at 18:40, Stefan Agner <stefan@agner.ch> wrote: > On 2017-02-27 06:35, Jon Medhurst (Tixy) wrote: >> On Sun, 2017-02-26 at 14:35 +0000, Ard Biesheuvel wrote: >> >>> However, we could make this a bit more private to the fixmap code by >>> using something like >>> >>> >>> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >>> index 5c17d2dec777..6c375aea8f22 100644 >>> --- a/arch/arm/include/asm/fixmap.h >>> +++ b/arch/arm/include/asm/fixmap.h >>> @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses = >>> >>> #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | >>> L_PTE_XN | L_PTE_DIRTY) >>> >>> -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >>> -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >>> +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?: >> >> The code above misses out setting L_PTE_XN when using pgprot_kernel >> >>> FIXMAP_PAGE_COMMON) | \ >>> + L_PTE_MT_WRITEBACK) >>> +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL) >>> +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY) >>> >>> /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >>> -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | >>> L_PTE_MT_DEV_SHARED | L_PTE_SHARED) >>> +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON | >>> L_PTE_MT_DEV_SHARED | \ >>> + L_PTE_SHARED) >>> #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO >>> >>> #define __early_set_fixmap __set_fixmap >>> >>> (and drop the change to mm/mmu.c), which boils down to the same for >>> fixmap but does not affect other users of pgprot_kernel. Also, it >>> appears these definitions are broken under STRICT_MM_TYPECHECKS so >>> this is a good opportunity to get that fixed as well. >> >> I like this method better because as you say it keeps things private to >> fixmap, and it doesn't risk affecting other things. > > Yes, I agree this is nicer than my proposal. > >> >> As for getting STRICT_MM_TYPECHECKS working that looks good too, but >> should be a separate cleanup patch, especially as a fix for the cache >> problem possibly should go to stable kernels. >> >> I also think FIXMAP_PAGE_COMMON should be renamed (prefixed with '__') >> as it's an implementation detail an not a memory type used with fixmap. >> >> So I was thinking, one patch as a bugfix: >> >> ---------------------------------------------------------------------- >> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >> index 5c17d2dec777..4e6784dc5668 100644 >> --- a/arch/arm/include/asm/fixmap.h >> +++ b/arch/arm/include/asm/fixmap.h >> @@ -41,7 +41,8 @@ static const enum fixed_addresses __end_of_fixed_addresses = >> >> #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | >> L_PTE_DIRTY) >> >> -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> +#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ >> + FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> >> /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> ---------------------------------------------------------------------- >> >> Second patch as a cleanup. Note this is different to Ard's version as >> it uses PAGE_KERNEL rather than open coding the same code from >> pgtable.h to add XN permisions (Also, the default generic definition of >> FIXMAP_PAGE_NORMAL is PAGE_KERNEL, and the only reason we want to change >> it is to select an alternate value if that is not yet setup) >> >> ---------------------------------------------------------------------- >> diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >> index 4e6784dc5668..ec689c6aa644 100644 >> --- a/arch/arm/include/asm/fixmap.h >> +++ b/arch/arm/include/asm/fixmap.h >> @@ -39,14 +39,15 @@ static const enum fixed_addresses __end_of_fixed_addresses = >> __end_of_fixmap_region > __end_of_early_ioremap_region ? >> __end_of_fixmap_region : __end_of_early_ioremap_region; >> >> -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | >> L_PTE_DIRTY) >> +#define __FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN >> | L_PTE_DIRTY) >> >> -#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ >> - FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> +#define FIXMAP_PAGE_NORMAL (pgprot_val(pgprot_kernel) ? PAGE_KERNEL : \ >> + __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)) >> +#define FIXMAP_PAGE_RO _MOD_PROT(FIXMAP_PAGE_NORMAL, L_PTE_RDONLY) >> >> /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | >> L_PTE_SHARED) >> +#define FIXMAP_PAGE_IO __pgprot(__FIXMAP_PAGE_COMMON | >> L_PTE_MT_DEV_SHARED | \ >> + L_PTE_SHARED) >> #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO >> >> #define __early_set_fixmap __set_fixmap >> ---------------------------------------------------------------------- > > Fix and cleanup looks good to me, > > Reviewed-by: Stefan Agner <stefan@agner.ch> > Likewise, Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-27 18:43 ` Ard Biesheuvel @ 2017-02-27 18:56 ` Jon Medhurst (Tixy) 2017-02-27 19:47 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Jon Medhurst (Tixy) @ 2017-02-27 18:56 UTC (permalink / raw) To: linux-arm-kernel On Mon, 2017-02-27 at 18:43 +0000, Ard Biesheuvel wrote: > On 27 February 2017 at 18:40, Stefan Agner <stefan@agner.ch> wrote: > > On 2017-02-27 06:35, Jon Medhurst (Tixy) wrote: > > > On Sun, 2017-02-26 at 14:35 +0000, Ard Biesheuvel wrote: > > > > > > > However, we could make this a bit more private to the fixmap code by > > > > using something like > > > > > > > > > > > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > > > > index 5c17d2dec777..6c375aea8f22 100644 > > > > --- a/arch/arm/include/asm/fixmap.h > > > > +++ b/arch/arm/include/asm/fixmap.h > > > > @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses = > > > > > > > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | > > > > L_PTE_XN | L_PTE_DIRTY) > > > > > > > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > > > > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > > > > +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?: > > > > > > The code above misses out setting L_PTE_XN when using pgprot_kernel > > > > > > > FIXMAP_PAGE_COMMON) | \ > > > > + L_PTE_MT_WRITEBACK) > > > > +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL) > > > > +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY) > > > > > > > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > > > > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | > > > > L_PTE_MT_DEV_SHARED | L_PTE_SHARED) > > > > +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON | > > > > L_PTE_MT_DEV_SHARED | \ > > > > + L_PTE_SHARED) > > > > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO > > > > > > > > #define __early_set_fixmap __set_fixmap > > > > > > > > (and drop the change to mm/mmu.c), which boils down to the same for > > > > fixmap but does not affect other users of pgprot_kernel. Also, it > > > > appears these definitions are broken under STRICT_MM_TYPECHECKS so > > > > this is a good opportunity to get that fixed as well. > > > > > > I like this method better because as you say it keeps things private to > > > fixmap, and it doesn't risk affecting other things. > > > > Yes, I agree this is nicer than my proposal. > > > > > > > > As for getting STRICT_MM_TYPECHECKS working that looks good too, but > > > should be a separate cleanup patch, especially as a fix for the cache > > > problem possibly should go to stable kernels. > > > > > > I also think FIXMAP_PAGE_COMMON should be renamed (prefixed with '__') > > > as it's an implementation detail an not a memory type used with fixmap. > > > > > > So I was thinking, one patch as a bugfix: > > > > > > ---------------------------------------------------------------------- > > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > > > index 5c17d2dec777..4e6784dc5668 100644 > > > --- a/arch/arm/include/asm/fixmap.h > > > +++ b/arch/arm/include/asm/fixmap.h > > > @@ -41,7 +41,8 @@ static const enum fixed_addresses __end_of_fixed_addresses = > > > > > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | > > > L_PTE_DIRTY) > > > > > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > > > +#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ > > > + FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > > > #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > > > > > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > > > ---------------------------------------------------------------------- > > > > > > Second patch as a cleanup. Note this is different to Ard's version as > > > it uses PAGE_KERNEL rather than open coding the same code from > > > pgtable.h to add XN permisions (Also, the default generic definition of > > > FIXMAP_PAGE_NORMAL is PAGE_KERNEL, and the only reason we want to change > > > it is to select an alternate value if that is not yet setup) > > > > > > ---------------------------------------------------------------------- > > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h > > > index 4e6784dc5668..ec689c6aa644 100644 > > > --- a/arch/arm/include/asm/fixmap.h > > > +++ b/arch/arm/include/asm/fixmap.h > > > @@ -39,14 +39,15 @@ static const enum fixed_addresses __end_of_fixed_addresses = > > > __end_of_fixmap_region > __end_of_early_ioremap_region ? > > > __end_of_fixmap_region : __end_of_early_ioremap_region; > > > > > > -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | > > > L_PTE_DIRTY) > > > +#define __FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN > > > > L_PTE_DIRTY) > > > > > > -#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ > > > - FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) > > > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) > > > +#define FIXMAP_PAGE_NORMAL (pgprot_val(pgprot_kernel) ? PAGE_KERNEL : \ > > > + __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)) > > > +#define FIXMAP_PAGE_RO _MOD_PROT(FIXMAP_PAGE_NORMAL, L_PTE_RDONLY) > > > > > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ > > > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | > > > L_PTE_SHARED) > > > +#define FIXMAP_PAGE_IO __pgprot(__FIXMAP_PAGE_COMMON | > > > L_PTE_MT_DEV_SHARED | \ > > > + L_PTE_SHARED) > > > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO > > > > > > #define __early_set_fixmap __set_fixmap > > > ---------------------------------------------------------------------- > > > > Fix and cleanup looks good to me, > > > > Reviewed-by: Stefan Agner <stefan@agner.ch> > > > > Likewise, > > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Thanks both. I'll do some more testing then post proper patches with commit messages, and add something like Based on changes Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH] arm: Fix cache inconsistency when using fixmap 2017-02-27 18:56 ` Jon Medhurst (Tixy) @ 2017-02-27 19:47 ` Ard Biesheuvel 0 siblings, 0 replies; 9+ messages in thread From: Ard Biesheuvel @ 2017-02-27 19:47 UTC (permalink / raw) To: linux-arm-kernel On 27 February 2017 at 18:56, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > On Mon, 2017-02-27 at 18:43 +0000, Ard Biesheuvel wrote: >> On 27 February 2017 at 18:40, Stefan Agner <stefan@agner.ch> wrote: >> > On 2017-02-27 06:35, Jon Medhurst (Tixy) wrote: >> > > On Sun, 2017-02-26 at 14:35 +0000, Ard Biesheuvel wrote: >> > > >> > > > However, we could make this a bit more private to the fixmap code by >> > > > using something like >> > > > >> > > > >> > > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >> > > > index 5c17d2dec777..6c375aea8f22 100644 >> > > > --- a/arch/arm/include/asm/fixmap.h >> > > > +++ b/arch/arm/include/asm/fixmap.h >> > > > @@ -41,11 +41,14 @@ static const enum fixed_addresses __end_of_fixed_addresses = >> > > > >> > > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | >> > > > L_PTE_XN | L_PTE_DIRTY) >> > > > >> > > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> > > > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> > > > +#define __FIXMAP_MT_NORMAL ((pgprot_val(pgprot_kernel) ?: >> > > >> > > The code above misses out setting L_PTE_XN when using pgprot_kernel >> > > >> > > > FIXMAP_PAGE_COMMON) | \ >> > > > + L_PTE_MT_WRITEBACK) >> > > > +#define FIXMAP_PAGE_NORMAL __pgprot(__FIXMAP_MT_NORMAL) >> > > > +#define FIXMAP_PAGE_RO __pgprot(__FIXMAP_MT_NORMAL | L_PTE_RDONLY) >> > > > >> > > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> > > > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | >> > > > L_PTE_MT_DEV_SHARED | L_PTE_SHARED) >> > > > +#define FIXMAP_PAGE_IO __pgprot(FIXMAP_PAGE_COMMON | >> > > > L_PTE_MT_DEV_SHARED | \ >> > > > + L_PTE_SHARED) >> > > > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO >> > > > >> > > > #define __early_set_fixmap __set_fixmap >> > > > >> > > > (and drop the change to mm/mmu.c), which boils down to the same for >> > > > fixmap but does not affect other users of pgprot_kernel. Also, it >> > > > appears these definitions are broken under STRICT_MM_TYPECHECKS so >> > > > this is a good opportunity to get that fixed as well. >> > > >> > > I like this method better because as you say it keeps things private to >> > > fixmap, and it doesn't risk affecting other things. >> > >> > Yes, I agree this is nicer than my proposal. >> > >> > > >> > > As for getting STRICT_MM_TYPECHECKS working that looks good too, but >> > > should be a separate cleanup patch, especially as a fix for the cache >> > > problem possibly should go to stable kernels. >> > > >> > > I also think FIXMAP_PAGE_COMMON should be renamed (prefixed with '__') >> > > as it's an implementation detail an not a memory type used with fixmap. >> > > >> > > So I was thinking, one patch as a bugfix: >> > > >> > > ---------------------------------------------------------------------- >> > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >> > > index 5c17d2dec777..4e6784dc5668 100644 >> > > --- a/arch/arm/include/asm/fixmap.h >> > > +++ b/arch/arm/include/asm/fixmap.h >> > > @@ -41,7 +41,8 @@ static const enum fixed_addresses __end_of_fixed_addresses = >> > > >> > > #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | >> > > L_PTE_DIRTY) >> > > >> > > -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> > > +#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ >> > > + FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> > > #define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> > > >> > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> > > ---------------------------------------------------------------------- >> > > >> > > Second patch as a cleanup. Note this is different to Ard's version as >> > > it uses PAGE_KERNEL rather than open coding the same code from >> > > pgtable.h to add XN permisions (Also, the default generic definition of >> > > FIXMAP_PAGE_NORMAL is PAGE_KERNEL, and the only reason we want to change >> > > it is to select an alternate value if that is not yet setup) >> > > >> > > ---------------------------------------------------------------------- >> > > diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h >> > > index 4e6784dc5668..ec689c6aa644 100644 >> > > --- a/arch/arm/include/asm/fixmap.h >> > > +++ b/arch/arm/include/asm/fixmap.h >> > > @@ -39,14 +39,15 @@ static const enum fixed_addresses __end_of_fixed_addresses = >> > > __end_of_fixmap_region > __end_of_early_ioremap_region ? >> > > __end_of_fixmap_region : __end_of_early_ioremap_region; >> > > >> > > -#define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | >> > > L_PTE_DIRTY) >> > > +#define __FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN >> > > > L_PTE_DIRTY) >> > > >> > > -#define FIXMAP_PAGE_NORMAL (pgprot_kernel ? pgprot_kernel | L_PTE_XN : \ >> > > - FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) >> > > -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) >> > > +#define FIXMAP_PAGE_NORMAL (pgprot_val(pgprot_kernel) ? PAGE_KERNEL : \ >> > > + __pgprot(__FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK)) >> > > +#define FIXMAP_PAGE_RO _MOD_PROT(FIXMAP_PAGE_NORMAL, L_PTE_RDONLY) >> > > >> > > /* Used by set_fixmap_(io|nocache), both meant for mapping a device */ >> > > -#define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | >> > > L_PTE_SHARED) >> > > +#define FIXMAP_PAGE_IO __pgprot(__FIXMAP_PAGE_COMMON | >> > > L_PTE_MT_DEV_SHARED | \ >> > > + L_PTE_SHARED) >> > > #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO >> > > >> > > #define __early_set_fixmap __set_fixmap >> > > ---------------------------------------------------------------------- >> > >> > Fix and cleanup looks good to me, >> > >> > Reviewed-by: Stefan Agner <stefan@agner.ch> >> > >> >> Likewise, >> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Thanks both. I'll do some more testing then post proper patches with > commit messages, and add something like > > Based on changes > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > If you insist, but between Stefan, you and me, I am hardly the person to have given the most substantial input into this discussion ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-02-27 19:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-22 15:51 [RFC PATCH] arm: Fix cache inconsistency when using fixmap Jon Medhurst (Tixy) 2017-02-25 1:06 ` Stefan Agner 2017-02-26 14:35 ` Ard Biesheuvel 2017-02-26 14:44 ` Ard Biesheuvel 2017-02-27 14:35 ` Jon Medhurst (Tixy) 2017-02-27 18:40 ` Stefan Agner 2017-02-27 18:43 ` Ard Biesheuvel 2017-02-27 18:56 ` Jon Medhurst (Tixy) 2017-02-27 19:47 ` Ard Biesheuvel
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 NNTP newsgroup(s).