| Message ID | 20170111144118.17062-2-cov@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Some minor comments below, nothing fundamental (as long as you say the new sequence doesn't have the speculative TLB load problem I mentioned on a previous version). On Wed, Jan 11, 2017 at 09:41:15AM -0500, Christopher Covington wrote: > diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt > index 405da11..7151aed 100644 > --- a/Documentation/arm64/silicon-errata.txt > +++ b/Documentation/arm64/silicon-errata.txt > @@ -42,24 +42,25 @@ file acts as a registry of software workarounds in the Linux Kernel and > will be updated when new workarounds are committed and backported to > stable kernels. > > -| Implementor | Component | Erratum ID | Kconfig | > -+----------------+-----------------+-----------------+-------------------------+ > -| ARM | Cortex-A53 | #826319 | ARM64_ERRATUM_826319 | > -| ARM | Cortex-A53 | #827319 | ARM64_ERRATUM_827319 | > -| ARM | Cortex-A53 | #824069 | ARM64_ERRATUM_824069 | > -| ARM | Cortex-A53 | #819472 | ARM64_ERRATUM_819472 | > -| ARM | Cortex-A53 | #845719 | ARM64_ERRATUM_845719 | > -| ARM | Cortex-A53 | #843419 | ARM64_ERRATUM_843419 | > -| ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | > -| ARM | Cortex-A57 | #852523 | N/A | > -| ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | > -| ARM | Cortex-A72 | #853709 | N/A | > -| ARM | MMU-500 | #841119,#826419 | N/A | > -| | | | | > -| Cavium | ThunderX ITS | #22375, #24313 | CAVIUM_ERRATUM_22375 | > -| Cavium | ThunderX ITS | #23144 | CAVIUM_ERRATUM_23144 | > -| Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | > -| Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | > -| Cavium | ThunderX SMMUv2 | #27704 | N/A | > -| | | | | > -| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | > +| Implementor | Component | Erratum ID | Kconfig | > ++---------------+-----------------+-----------------+--------------------------+ > +| ARM | Cortex-A53 | #826319 | ARM64_ERRATUM_826319 | > +| ARM | Cortex-A53 | #827319 | ARM64_ERRATUM_827319 | > +| ARM | Cortex-A53 | #824069 | ARM64_ERRATUM_824069 | > +| ARM | Cortex-A53 | #819472 | ARM64_ERRATUM_819472 | > +| ARM | Cortex-A53 | #845719 | ARM64_ERRATUM_845719 | > +| ARM | Cortex-A53 | #843419 | ARM64_ERRATUM_843419 | > +| ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | > +| ARM | Cortex-A57 | #852523 | N/A | > +| ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | > +| ARM | Cortex-A72 | #853709 | N/A | > +| ARM | MMU-500 | #841119,#826419 | N/A | > +| | | | | > +| Cavium | ThunderX ITS | #22375, #24313 | CAVIUM_ERRATUM_22375 | > +| Cavium | ThunderX ITS | #23144 | CAVIUM_ERRATUM_23144 | > +| Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | > +| Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | > +| Cavium | ThunderX SMMUv2 | #27704 | N/A | > +| | | | | > +| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | > +| Qualcomm | Falkor v1 | E1003 | QCOM_FALKOR_ERRATUM_1003 | Please don't change the "Implementor" column width, there is no point and it makes the patch harder to read (i.e. this hunk should only have one line). > diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c > index 4c63cb1..5a0a82a 100644 > --- a/arch/arm64/mm/context.c > +++ b/arch/arm64/mm/context.c > @@ -87,6 +87,11 @@ static void flush_context(unsigned int cpu) > /* Update the list of reserved ASIDs and the ASID bitmap. */ > bitmap_clear(asid_map, 0, NUM_USER_ASIDS); > > + /* Reserve ASID for Falkor erratum 1003 */ > + if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) && > + cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003)) > + __set_bit(FALKOR_RESERVED_ASID, asid_map); > + > /* > * Ensure the generation bump is observed before we xchg the > * active_asids. > @@ -244,6 +249,11 @@ static int asids_init(void) > panic("Failed to allocate bitmap for %lu ASIDs\n", > NUM_USER_ASIDS); > > + /* Reserve ASID for Falkor erratum 1003 */ > + if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) && > + cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003)) > + __set_bit(FALKOR_RESERVED_ASID, asid_map); > + > pr_info("ASID allocator initialised with %lu entries\n", NUM_USER_ASIDS); > return 0; > } You could as well write a small static function in this file and call it twice. > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > index 32682be..9ee46df 100644 > --- a/arch/arm64/mm/proc.S > +++ b/arch/arm64/mm/proc.S > @@ -23,6 +23,7 @@ > #include <asm/assembler.h> > #include <asm/asm-offsets.h> > #include <asm/hwcap.h> > +#include <asm/mmu_context.h> > #include <asm/pgtable.h> > #include <asm/pgtable-hwdef.h> > #include <asm/cpufeature.h> > @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) > ENTRY(cpu_do_switch_mm) > mmid x1, x1 // get mm->context.id > bfi x0, x1, #48, #16 // set the ASID > +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 > +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 > + mrs x2, ttbr0_el1 > + mov x3, #FALKOR_RESERVED_ASID > + bfi x2, x3, #48, #16 // reserved ASID + old BADDR > + msr ttbr0_el1, x2 > + isb > + bfi x2, x0, #0, #48 // reserved ASID + new BADDR > + msr ttbr0_el1, x2 > + isb > +alternative_else_nop_endif > +#endif > msr ttbr0_el1, x0 // set TTBR0 > isb > post_ttbr0_update_workaround Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.
On 11/01/17 18:06, Catalin Marinas wrote: > Some minor comments below, nothing fundamental (as long as you say the > new sequence doesn't have the speculative TLB load problem I mentioned > on a previous version). > > On Wed, Jan 11, 2017 at 09:41:15AM -0500, Christopher Covington wrote: >> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt >> index 405da11..7151aed 100644 >> --- a/Documentation/arm64/silicon-errata.txt >> +++ b/Documentation/arm64/silicon-errata.txt >> @@ -42,24 +42,25 @@ file acts as a registry of software workarounds in the Linux Kernel and >> will be updated when new workarounds are committed and backported to >> stable kernels. >> >> -| Implementor | Component | Erratum ID | Kconfig | >> -+----------------+-----------------+-----------------+-------------------------+ >> -| ARM | Cortex-A53 | #826319 | ARM64_ERRATUM_826319 | >> -| ARM | Cortex-A53 | #827319 | ARM64_ERRATUM_827319 | >> -| ARM | Cortex-A53 | #824069 | ARM64_ERRATUM_824069 | >> -| ARM | Cortex-A53 | #819472 | ARM64_ERRATUM_819472 | >> -| ARM | Cortex-A53 | #845719 | ARM64_ERRATUM_845719 | >> -| ARM | Cortex-A53 | #843419 | ARM64_ERRATUM_843419 | >> -| ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | >> -| ARM | Cortex-A57 | #852523 | N/A | >> -| ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | >> -| ARM | Cortex-A72 | #853709 | N/A | >> -| ARM | MMU-500 | #841119,#826419 | N/A | >> -| | | | | >> -| Cavium | ThunderX ITS | #22375, #24313 | CAVIUM_ERRATUM_22375 | >> -| Cavium | ThunderX ITS | #23144 | CAVIUM_ERRATUM_23144 | >> -| Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | >> -| Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | >> -| Cavium | ThunderX SMMUv2 | #27704 | N/A | >> -| | | | | >> -| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | >> +| Implementor | Component | Erratum ID | Kconfig | >> ++---------------+-----------------+-----------------+--------------------------+ >> +| ARM | Cortex-A53 | #826319 | ARM64_ERRATUM_826319 | >> +| ARM | Cortex-A53 | #827319 | ARM64_ERRATUM_827319 | >> +| ARM | Cortex-A53 | #824069 | ARM64_ERRATUM_824069 | >> +| ARM | Cortex-A53 | #819472 | ARM64_ERRATUM_819472 | >> +| ARM | Cortex-A53 | #845719 | ARM64_ERRATUM_845719 | >> +| ARM | Cortex-A53 | #843419 | ARM64_ERRATUM_843419 | >> +| ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | >> +| ARM | Cortex-A57 | #852523 | N/A | >> +| ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | >> +| ARM | Cortex-A72 | #853709 | N/A | >> +| ARM | MMU-500 | #841119,#826419 | N/A | >> +| | | | | >> +| Cavium | ThunderX ITS | #22375, #24313 | CAVIUM_ERRATUM_22375 | >> +| Cavium | ThunderX ITS | #23144 | CAVIUM_ERRATUM_23144 | >> +| Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | >> +| Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | >> +| Cavium | ThunderX SMMUv2 | #27704 | N/A | >> +| | | | | >> +| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | >> +| Qualcomm | Falkor v1 | E1003 | QCOM_FALKOR_ERRATUM_1003 | > > Please don't change the "Implementor" column width, there is no point > and it makes the patch harder to read (i.e. this hunk should only have > one line). > >> diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c >> index 4c63cb1..5a0a82a 100644 >> --- a/arch/arm64/mm/context.c >> +++ b/arch/arm64/mm/context.c >> @@ -87,6 +87,11 @@ static void flush_context(unsigned int cpu) >> /* Update the list of reserved ASIDs and the ASID bitmap. */ >> bitmap_clear(asid_map, 0, NUM_USER_ASIDS); >> >> + /* Reserve ASID for Falkor erratum 1003 */ >> + if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) && >> + cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003)) >> + __set_bit(FALKOR_RESERVED_ASID, asid_map); >> + >> /* >> * Ensure the generation bump is observed before we xchg the >> * active_asids. >> @@ -244,6 +249,11 @@ static int asids_init(void) >> panic("Failed to allocate bitmap for %lu ASIDs\n", >> NUM_USER_ASIDS); >> >> + /* Reserve ASID for Falkor erratum 1003 */ >> + if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) && >> + cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003)) >> + __set_bit(FALKOR_RESERVED_ASID, asid_map); >> + >> pr_info("ASID allocator initialised with %lu entries\n", NUM_USER_ASIDS); >> return 0; >> } > > You could as well write a small static function in this file and call it > twice. > >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S >> index 32682be..9ee46df 100644 >> --- a/arch/arm64/mm/proc.S >> +++ b/arch/arm64/mm/proc.S >> @@ -23,6 +23,7 @@ >> #include <asm/assembler.h> >> #include <asm/asm-offsets.h> >> #include <asm/hwcap.h> >> +#include <asm/mmu_context.h> >> #include <asm/pgtable.h> >> #include <asm/pgtable-hwdef.h> >> #include <asm/cpufeature.h> >> @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) >> ENTRY(cpu_do_switch_mm) >> mmid x1, x1 // get mm->context.id >> bfi x0, x1, #48, #16 // set the ASID >> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 >> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 >> + mrs x2, ttbr0_el1 >> + mov x3, #FALKOR_RESERVED_ASID >> + bfi x2, x3, #48, #16 // reserved ASID + old BADDR >> + msr ttbr0_el1, x2 >> + isb >> + bfi x2, x0, #0, #48 // reserved ASID + new BADDR >> + msr ttbr0_el1, x2 >> + isb >> +alternative_else_nop_endif >> +#endif >> msr ttbr0_el1, x0 // set TTBR0 >> isb >> post_ttbr0_update_workaround > > Please move the above hunk to a pre_ttbr0_update_workaround macro for > consistency with post_ttbr0_update_workaround. In which case (and also for consistency), should we add that pre_ttbr0 macro to entry.S, just before __uaccess_ttbr0_enable? It may not be needed in the SW pan case, but it is probably worth entertaining the idea that there may be something to do there... Thanks, M.
On Wed, Jan 11, 2017 at 06:06:27PM +0000, Catalin Marinas wrote: > On Wed, Jan 11, 2017 at 09:41:15AM -0500, Christopher Covington wrote: > > -| Implementor | Component | Erratum ID | Kconfig | > > +| Implementor | Component | Erratum ID | Kconfig | > > +| Qualcomm | Falkor v1 | E1003 | QCOM_FALKOR_ERRATUM_1003 | > > Please don't change the "Implementor" column width, there is no point > and it makes the patch harder to read (i.e. this hunk should only have > one line). It'll need to affect all lines since the kconfig column needs to expand by at least one character to fit QCOM_FALKOR_ERRATUM_1003. I beleive the intent here was to keep the table fitting into a width of 80 characters. IMO we should allow the table to expand past 80 chars (everyone reading this file should be able to resize tehir terminal), and only expand the kconfig column. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/11/2017 12:33 PM, Mark Rutland wrote: > It'll need to affect all lines since the kconfig column needs to expand > by at least one character to fit QCOM_FALKOR_ERRATUM_1003. Or we can make the macro shorter.
On Wed, Jan 11, 2017 at 12:35:55PM -0600, Timur Tabi wrote: > On 01/11/2017 12:33 PM, Mark Rutland wrote: > >It'll need to affect all lines since the kconfig column needs to expand > >by at least one character to fit QCOM_FALKOR_ERRATUM_1003. > > Or we can make the macro shorter. The name, as it is, is perfectly descriptive. Let's not sacrifice legibility over a non-issue. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/11/2017 12:37 PM, Mark Rutland wrote: > The name, as it is, is perfectly descriptive. > > Let's not sacrifice legibility over a non-issue. I don't want to kick a dead horse or anything, but changing it to QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems without sacrificing anything.
On Wed, Jan 11, 2017 at 06:22:08PM +0000, Marc Zyngier wrote: > On 11/01/17 18:06, Catalin Marinas wrote: > > On Wed, Jan 11, 2017 at 09:41:15AM -0500, Christopher Covington wrote: > >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > >> index 32682be..9ee46df 100644 > >> --- a/arch/arm64/mm/proc.S > >> +++ b/arch/arm64/mm/proc.S > >> @@ -23,6 +23,7 @@ > >> #include <asm/assembler.h> > >> #include <asm/asm-offsets.h> > >> #include <asm/hwcap.h> > >> +#include <asm/mmu_context.h> > >> #include <asm/pgtable.h> > >> #include <asm/pgtable-hwdef.h> > >> #include <asm/cpufeature.h> > >> @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) > >> ENTRY(cpu_do_switch_mm) > >> mmid x1, x1 // get mm->context.id > >> bfi x0, x1, #48, #16 // set the ASID > >> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 > >> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 > >> + mrs x2, ttbr0_el1 > >> + mov x3, #FALKOR_RESERVED_ASID > >> + bfi x2, x3, #48, #16 // reserved ASID + old BADDR > >> + msr ttbr0_el1, x2 > >> + isb > >> + bfi x2, x0, #0, #48 // reserved ASID + new BADDR > >> + msr ttbr0_el1, x2 > >> + isb > >> +alternative_else_nop_endif > >> +#endif > >> msr ttbr0_el1, x0 // set TTBR0 > >> isb > >> post_ttbr0_update_workaround > > > > Please move the above hunk to a pre_ttbr0_update_workaround macro for > > consistency with post_ttbr0_update_workaround. > > In which case (and also for consistency), should we add that pre_ttbr0 > macro to entry.S, just before __uaccess_ttbr0_enable? It may not be > needed in the SW pan case, but it is probably worth entertaining the > idea that there may be something to do there... Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0(). Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 12:40:42PM -0600, Timur Tabi wrote: > On 01/11/2017 12:37 PM, Mark Rutland wrote: > >The name, as it is, is perfectly descriptive. > > > >Let's not sacrifice legibility over a non-issue. > > I don't want to kick a dead horse or anything, but changing it to > QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems > without sacrificing anything. The CPU is called "Falkor", not "FLKR", and we're not coming up with an ACPI table name... The ARM Ltd. erratum numbers are global to all parts, so we don't include the part name. Is the 1003 erratum number specific to Falkor? If it's global, you could use QCOM_ERRATUM_1003 instead. Otherwise, QCOM_FALKOR_ERRATUM_1003 is preferable. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[finally, some proper bikeshedding] On 11/01/17 18:40, Timur Tabi wrote: > On 01/11/2017 12:37 PM, Mark Rutland wrote: >> The name, as it is, is perfectly descriptive. >> >> Let's not sacrifice legibility over a non-issue. > > I don't want to kick a dead horse or anything, but changing it to > QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems without > sacrificing anything. Other than not being able to grep for the core name in the source tree, how do you suggest we pronounce FLKR? Because so far, it rolls off the tongue in an interesting way... Thanks, M.
On Wed, Jan 11, 2017 at 06:37:39PM +0000, Mark Rutland wrote: > On Wed, Jan 11, 2017 at 12:35:55PM -0600, Timur Tabi wrote: > > On 01/11/2017 12:33 PM, Mark Rutland wrote: > > >It'll need to affect all lines since the kconfig column needs to expand > > >by at least one character to fit QCOM_FALKOR_ERRATUM_1003. > > > > Or we can make the macro shorter. > > The name, as it is, is perfectly descriptive. > > Let's not sacrifice legibility over a non-issue. I agree, I didn't realise that the we expand the last column already. It's a non-issue indeed.
On Wed, Jan 11, 2017 at 06:40:52PM +0000, Mark Rutland wrote: > On Wed, Jan 11, 2017 at 06:22:08PM +0000, Marc Zyngier wrote: > > On 11/01/17 18:06, Catalin Marinas wrote: > > > On Wed, Jan 11, 2017 at 09:41:15AM -0500, Christopher Covington wrote: > > >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > > >> index 32682be..9ee46df 100644 > > >> --- a/arch/arm64/mm/proc.S > > >> +++ b/arch/arm64/mm/proc.S > > >> @@ -23,6 +23,7 @@ > > >> #include <asm/assembler.h> > > >> #include <asm/asm-offsets.h> > > >> #include <asm/hwcap.h> > > >> +#include <asm/mmu_context.h> > > >> #include <asm/pgtable.h> > > >> #include <asm/pgtable-hwdef.h> > > >> #include <asm/cpufeature.h> > > >> @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) > > >> ENTRY(cpu_do_switch_mm) > > >> mmid x1, x1 // get mm->context.id > > >> bfi x0, x1, #48, #16 // set the ASID > > >> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 > > >> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 > > >> + mrs x2, ttbr0_el1 > > >> + mov x3, #FALKOR_RESERVED_ASID > > >> + bfi x2, x3, #48, #16 // reserved ASID + old BADDR > > >> + msr ttbr0_el1, x2 > > >> + isb > > >> + bfi x2, x0, #0, #48 // reserved ASID + new BADDR > > >> + msr ttbr0_el1, x2 > > >> + isb > > >> +alternative_else_nop_endif > > >> +#endif > > >> msr ttbr0_el1, x0 // set TTBR0 > > >> isb > > >> post_ttbr0_update_workaround > > > > > > Please move the above hunk to a pre_ttbr0_update_workaround macro for > > > consistency with post_ttbr0_update_workaround. > > > > In which case (and also for consistency), should we add that pre_ttbr0 > > macro to entry.S, just before __uaccess_ttbr0_enable? It may not be > > needed in the SW pan case, but it is probably worth entertaining the > > idea that there may be something to do there... > > Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0(). This may be fine if my assumptions about this erratum are correct. In the cpu_set_reserved_ttbr0() case we set TTBR0_EL1 to a table without any entries, so no new entries could be tagged with the old ASID.
On Wed, Jan 11, 2017 at 06:22:08PM +0000, Marc Zyngier wrote: > On 11/01/17 18:06, Catalin Marinas wrote: > > On Wed, Jan 11, 2017 at 09:41:15AM -0500, Christopher Covington wrote: > >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > >> index 32682be..9ee46df 100644 > >> --- a/arch/arm64/mm/proc.S > >> +++ b/arch/arm64/mm/proc.S > >> @@ -23,6 +23,7 @@ > >> #include <asm/assembler.h> > >> #include <asm/asm-offsets.h> > >> #include <asm/hwcap.h> > >> +#include <asm/mmu_context.h> > >> #include <asm/pgtable.h> > >> #include <asm/pgtable-hwdef.h> > >> #include <asm/cpufeature.h> > >> @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) > >> ENTRY(cpu_do_switch_mm) > >> mmid x1, x1 // get mm->context.id > >> bfi x0, x1, #48, #16 // set the ASID > >> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 > >> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 > >> + mrs x2, ttbr0_el1 > >> + mov x3, #FALKOR_RESERVED_ASID > >> + bfi x2, x3, #48, #16 // reserved ASID + old BADDR > >> + msr ttbr0_el1, x2 > >> + isb > >> + bfi x2, x0, #0, #48 // reserved ASID + new BADDR > >> + msr ttbr0_el1, x2 > >> + isb > >> +alternative_else_nop_endif > >> +#endif > >> msr ttbr0_el1, x0 // set TTBR0 > >> isb > >> post_ttbr0_update_workaround > > > > Please move the above hunk to a pre_ttbr0_update_workaround macro for > > consistency with post_ttbr0_update_workaround. > > In which case (and also for consistency), should we add that pre_ttbr0 > macro to entry.S, just before __uaccess_ttbr0_enable? It may not be > needed in the SW pan case, but it is probably worth entertaining the > idea that there may be something to do there... It may actually be needed in entry.S as well. With SW PAN, we move the context switching from cpu_do_switch_mm to the kernel_exit macro when returning to user. In this case we are switching from the reserved ASID 0 and reserved TTBR0_EL1 (pointing to a zeroed page) to the user's TTBR0_EL1 and ASID. If the ASID switch isn't taken into account, we may end up with new TLB entries being tagged with the reserved ASID. Apart from a potential loss of protection with TTBR0 PAN, is there anything else that could go wrong? Maybe a TLB conflict if we mix TLBs from multiple address spaces tagged with the same reserved ASID. If the above is an issue, we would need to patch __uaccess_ttbr0_enable() as well, though I'm more inclined to make this erratum not selectable when TTBR0 PAN is enabled.
On Thu, Jan 12, 2017 at 03:55:58PM +0000, Catalin Marinas wrote: > On Wed, Jan 11, 2017 at 06:22:08PM +0000, Marc Zyngier wrote: > > On 11/01/17 18:06, Catalin Marinas wrote: > > > On Wed, Jan 11, 2017 at 09:41:15AM -0500, Christopher Covington wrote: > > >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > > >> index 32682be..9ee46df 100644 > > >> --- a/arch/arm64/mm/proc.S > > >> +++ b/arch/arm64/mm/proc.S > > >> @@ -23,6 +23,7 @@ > > >> #include <asm/assembler.h> > > >> #include <asm/asm-offsets.h> > > >> #include <asm/hwcap.h> > > >> +#include <asm/mmu_context.h> > > >> #include <asm/pgtable.h> > > >> #include <asm/pgtable-hwdef.h> > > >> #include <asm/cpufeature.h> > > >> @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) > > >> ENTRY(cpu_do_switch_mm) > > >> mmid x1, x1 // get mm->context.id > > >> bfi x0, x1, #48, #16 // set the ASID > > >> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 > > >> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 > > >> + mrs x2, ttbr0_el1 > > >> + mov x3, #FALKOR_RESERVED_ASID > > >> + bfi x2, x3, #48, #16 // reserved ASID + old BADDR > > >> + msr ttbr0_el1, x2 > > >> + isb > > >> + bfi x2, x0, #0, #48 // reserved ASID + new BADDR > > >> + msr ttbr0_el1, x2 > > >> + isb > > >> +alternative_else_nop_endif > > >> +#endif > > >> msr ttbr0_el1, x0 // set TTBR0 > > >> isb > > >> post_ttbr0_update_workaround > > > > > > Please move the above hunk to a pre_ttbr0_update_workaround macro for > > > consistency with post_ttbr0_update_workaround. > > > > In which case (and also for consistency), should we add that pre_ttbr0 > > macro to entry.S, just before __uaccess_ttbr0_enable? It may not be > > needed in the SW pan case, but it is probably worth entertaining the > > idea that there may be something to do there... > > It may actually be needed in entry.S as well. With SW PAN, we move the > context switching from cpu_do_switch_mm to the kernel_exit macro when > returning to user. In this case we are switching from the reserved ASID > 0 and reserved TTBR0_EL1 (pointing to a zeroed page) to the user's > TTBR0_EL1 and ASID. If the ASID switch isn't taken into account, we may > end up with new TLB entries being tagged with the reserved ASID. Apart > from a potential loss of protection with TTBR0 PAN, is there anything > else that could go wrong? Maybe a TLB conflict if we mix TLBs from > multiple address spaces tagged with the same reserved ASID. > > If the above is an issue, we would need to patch > __uaccess_ttbr0_enable() as well, though I'm more inclined to make this > erratum not selectable when TTBR0 PAN is enabled. I don't think that's a reasonable approach. By all means change the default, but we need to support kernel images with both of these kconfig options enabled. Will -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 12, 2017 at 03:45:48PM +0000, Catalin Marinas wrote: > On Wed, Jan 11, 2017 at 06:40:52PM +0000, Mark Rutland wrote: > > Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0(). > > This may be fine if my assumptions about this erratum are correct. In > the cpu_set_reserved_ttbr0() case we set TTBR0_EL1 to a table without > any entries, so no new entries could be tagged with the old ASID. For some reason, I was under the impression that the issue was old table entries being allocated to the new ASID. Looking over the series again, it's not clear to me precisely which cases can occur. It would be good to see that clarified. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On 01/11/2017 01:45 PM, Mark Rutland wrote: > On Wed, Jan 11, 2017 at 12:40:42PM -0600, Timur Tabi wrote: >> On 01/11/2017 12:37 PM, Mark Rutland wrote: >>> The name, as it is, is perfectly descriptive. >>> >>> Let's not sacrifice legibility over a non-issue. >> >> I don't want to kick a dead horse or anything, but changing it to >> QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems >> without sacrificing anything. > > The CPU is called "Falkor", not "FLKR", and we're not coming up with an > ACPI table name... > > The ARM Ltd. erratum numbers are global to all parts, so we don't > include the part name. Is the 1003 erratum number specific to Falkor? > > If it's global, you could use QCOM_ERRATUM_1003 instead. E1003 is specific to Falkor, and hopefully just its first major revision. Qualcomm Technology's first/previous generation ARMv8 custom microarchitecture used errata numbers below 1000. I am not aware of global coordination in the numbering, unfortunately. > Otherwise, QCOM_FALKOR_ERRATUM_1003 is preferable. Thanks, Cov
On 01/12/2017 11:12 AM, Mark Rutland wrote: > On Thu, Jan 12, 2017 at 03:45:48PM +0000, Catalin Marinas wrote: >> On Wed, Jan 11, 2017 at 06:40:52PM +0000, Mark Rutland wrote: > >>> Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0(). >> >> This may be fine if my assumptions about this erratum are correct. In >> the cpu_set_reserved_ttbr0() case we set TTBR0_EL1 to a table without >> any entries, so no new entries could be tagged with the old ASID. > > For some reason, I was under the impression that the issue was old table > entries being allocated to the new ASID. Looking over the series again, > it's not clear to me precisely which cases can occur. > > It would be good to see that clarified. I'll add more general background the commit message in the next revision and I've also asked directly about this. The answer is that page table entries using the new translation table base address will be allocated into the TLB using the old ASID. If employing the workaround, it's possible for page table entries using the new translation table base to be allocated into the TLB using the reserved ASID. Cov
Hi Catalin, On 01/11/2017 01:06 PM, Catalin Marinas wrote: > Some minor comments below, nothing fundamental (as long as you say the > new sequence doesn't have the speculative TLB load problem I mentioned > on a previous version). This workaround is documented as providing functional correctness for both explicit and speculative memory accesses. >> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S >> index 32682be..9ee46df 100644 >> --- a/arch/arm64/mm/proc.S >> +++ b/arch/arm64/mm/proc.S >> @@ -23,6 +23,7 @@ >> #include <asm/assembler.h> >> #include <asm/asm-offsets.h> >> #include <asm/hwcap.h> >> +#include <asm/mmu_context.h> >> #include <asm/pgtable.h> >> #include <asm/pgtable-hwdef.h> >> #include <asm/cpufeature.h> >> @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) >> ENTRY(cpu_do_switch_mm) >> mmid x1, x1 // get mm->context.id >> bfi x0, x1, #48, #16 // set the ASID >> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 >> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 >> + mrs x2, ttbr0_el1 >> + mov x3, #FALKOR_RESERVED_ASID >> + bfi x2, x3, #48, #16 // reserved ASID + old BADDR >> + msr ttbr0_el1, x2 >> + isb >> + bfi x2, x0, #0, #48 // reserved ASID + new BADDR >> + msr ttbr0_el1, x2 >> + isb >> +alternative_else_nop_endif >> +#endif >> msr ttbr0_el1, x0 // set TTBR0 >> isb >> post_ttbr0_update_workaround > > Please move the above hunk to a pre_ttbr0_update_workaround macro for > consistency with post_ttbr0_update_workaround. How should I deal with inputs to the macro? A) Use no input parameters and hard code x0 usage B) Use a macro input parameter for the new TTBR value (x0) C) Something else Thanks, Cov
diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt index 405da11..7151aed 100644 --- a/Documentation/arm64/silicon-errata.txt +++ b/Documentation/arm64/silicon-errata.txt @@ -42,24 +42,25 @@ file acts as a registry of software workarounds in the Linux Kernel and will be updated when new workarounds are committed and backported to stable kernels. -| Implementor | Component | Erratum ID | Kconfig | -+----------------+-----------------+-----------------+-------------------------+ -| ARM | Cortex-A53 | #826319 | ARM64_ERRATUM_826319 | -| ARM | Cortex-A53 | #827319 | ARM64_ERRATUM_827319 | -| ARM | Cortex-A53 | #824069 | ARM64_ERRATUM_824069 | -| ARM | Cortex-A53 | #819472 | ARM64_ERRATUM_819472 | -| ARM | Cortex-A53 | #845719 | ARM64_ERRATUM_845719 | -| ARM | Cortex-A53 | #843419 | ARM64_ERRATUM_843419 | -| ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | -| ARM | Cortex-A57 | #852523 | N/A | -| ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | -| ARM | Cortex-A72 | #853709 | N/A | -| ARM | MMU-500 | #841119,#826419 | N/A | -| | | | | -| Cavium | ThunderX ITS | #22375, #24313 | CAVIUM_ERRATUM_22375 | -| Cavium | ThunderX ITS | #23144 | CAVIUM_ERRATUM_23144 | -| Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | -| Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | -| Cavium | ThunderX SMMUv2 | #27704 | N/A | -| | | | | -| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | +| Implementor | Component | Erratum ID | Kconfig | ++---------------+-----------------+-----------------+--------------------------+ +| ARM | Cortex-A53 | #826319 | ARM64_ERRATUM_826319 | +| ARM | Cortex-A53 | #827319 | ARM64_ERRATUM_827319 | +| ARM | Cortex-A53 | #824069 | ARM64_ERRATUM_824069 | +| ARM | Cortex-A53 | #819472 | ARM64_ERRATUM_819472 | +| ARM | Cortex-A53 | #845719 | ARM64_ERRATUM_845719 | +| ARM | Cortex-A53 | #843419 | ARM64_ERRATUM_843419 | +| ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | +| ARM | Cortex-A57 | #852523 | N/A | +| ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | +| ARM | Cortex-A72 | #853709 | N/A | +| ARM | MMU-500 | #841119,#826419 | N/A | +| | | | | +| Cavium | ThunderX ITS | #22375, #24313 | CAVIUM_ERRATUM_22375 | +| Cavium | ThunderX ITS | #23144 | CAVIUM_ERRATUM_23144 | +| Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | +| Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | +| Cavium | ThunderX SMMUv2 | #27704 | N/A | +| | | | | +| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | +| Qualcomm | Falkor v1 | E1003 | QCOM_FALKOR_ERRATUM_1003 | diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1117421..2a80ac9 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -479,6 +479,17 @@ config CAVIUM_ERRATUM_27456 If unsure, say Y. +config QCOM_FALKOR_ERRATUM_1003 + bool "Falkor E1003: Incorrect translation due to ASID change" + default y + help + An incorrect translation TLBI entry may be created while changing the + ASID and translation table address together for TTBR0_EL1. The + workaround for this issue is to use a reserved ASID in + cpu_do_switch_mm() before switching to the target ASID. + + If unsure, say Y. + endmenu diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h index 4174f09..5aaf7ee 100644 --- a/arch/arm64/include/asm/cpucaps.h +++ b/arch/arm64/include/asm/cpucaps.h @@ -35,7 +35,8 @@ #define ARM64_HYP_OFFSET_LOW 14 #define ARM64_MISMATCHED_CACHE_LINE_SIZE 15 #define ARM64_HAS_NO_FPSIMD 16 +#define ARM64_WORKAROUND_QCOM_FALKOR_E1003 17 -#define ARM64_NCAPS 17 +#define ARM64_NCAPS 18 #endif /* __ASM_CPUCAPS_H */ diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h index 0363fe8..9632b05 100644 --- a/arch/arm64/include/asm/mmu_context.h +++ b/arch/arm64/include/asm/mmu_context.h @@ -19,6 +19,10 @@ #ifndef __ASM_MMU_CONTEXT_H #define __ASM_MMU_CONTEXT_H +#define FALKOR_RESERVED_ASID 1 + +#ifndef __ASSEMBLY__ + #include <linux/compiler.h> #include <linux/sched.h> @@ -220,4 +224,6 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next, void verify_cpu_asid_bits(void); -#endif +#endif /* !__ASSEMBLY__ */ + +#endif /* !__ASM_MMU_CONTEXT_H */ diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c index b75e917..787b542 100644 --- a/arch/arm64/kernel/cpu_errata.c +++ b/arch/arm64/kernel/cpu_errata.c @@ -130,6 +130,13 @@ const struct arm64_cpu_capabilities arm64_errata[] = { .def_scope = SCOPE_LOCAL_CPU, .enable = cpu_enable_trap_ctr_access, }, +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 + { + .desc = "Qualcomm Falkor erratum 1003", + .capability = ARM64_WORKAROUND_QCOM_FALKOR_E1003, + MIDR_RANGE(MIDR_QCOM_FALKOR_V1, 0x00, 0x00), + }, +#endif { } }; diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c index 4c63cb1..5a0a82a 100644 --- a/arch/arm64/mm/context.c +++ b/arch/arm64/mm/context.c @@ -87,6 +87,11 @@ static void flush_context(unsigned int cpu) /* Update the list of reserved ASIDs and the ASID bitmap. */ bitmap_clear(asid_map, 0, NUM_USER_ASIDS); + /* Reserve ASID for Falkor erratum 1003 */ + if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) && + cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003)) + __set_bit(FALKOR_RESERVED_ASID, asid_map); + /* * Ensure the generation bump is observed before we xchg the * active_asids. @@ -244,6 +249,11 @@ static int asids_init(void) panic("Failed to allocate bitmap for %lu ASIDs\n", NUM_USER_ASIDS); + /* Reserve ASID for Falkor erratum 1003 */ + if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) && + cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003)) + __set_bit(FALKOR_RESERVED_ASID, asid_map); + pr_info("ASID allocator initialised with %lu entries\n", NUM_USER_ASIDS); return 0; } diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 32682be..9ee46df 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -23,6 +23,7 @@ #include <asm/assembler.h> #include <asm/asm-offsets.h> #include <asm/hwcap.h> +#include <asm/mmu_context.h> #include <asm/pgtable.h> #include <asm/pgtable-hwdef.h> #include <asm/cpufeature.h> @@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume) ENTRY(cpu_do_switch_mm) mmid x1, x1 // get mm->context.id bfi x0, x1, #48, #16 // set the ASID +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003 +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003 + mrs x2, ttbr0_el1 + mov x3, #FALKOR_RESERVED_ASID + bfi x2, x3, #48, #16 // reserved ASID + old BADDR + msr ttbr0_el1, x2 + isb + bfi x2, x0, #0, #48 // reserved ASID + new BADDR + msr ttbr0_el1, x2 + isb +alternative_else_nop_endif +#endif msr ttbr0_el1, x0 // set TTBR0 isb post_ttbr0_update_workaround