More PCID fixes
Subsystems (3)
Andy Lutomirski
maintainercover More PCID fixes
This gets rid of the problematic VM_BUG_ON and fixes hibernation.
Andy Lutomirski (2):
x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
arch/x86/mm/tlb.c | 22 +++++++++++++++++++++- arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-)
Signature · 1 line
2.13.5
Andy Lutomirski
maintainerPATCH 1/2 x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()
If we hit the VM_BUG_ON(), we're detecting a genuinely bad situation, but we're very unlikely to get a useful call trace.
Make it a warning instead.
--- arch/x86/mm/tlb.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
arch/x86/mm/tlb.c +21 -1
@@ -121,8 +121,28 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,* hypothetical buggy code that directly switches to swapper_pg_dir * without going through leave_mm() / switch_mm_irqs_off() or that * does something like write_cr3(read_cr3_pa()). +* +* Only do this check if CONFIG_DEBUG_VM=y because __read_cr3() +* isn't free. */ -VM_BUG_ON (__read_cr3 () != (__sme_pa (real_prev->pgd) | prev_asid)); +#ifdef CONFIG_DEBUG_VM +if (WARN_ON_ONCE (__read_cr3 () != + (__sme_pa (real_prev->pgd) | prev_asid))) { +/* +* If we were to BUG here, we'd be very likely to kill +* the system so hard that we don't see the call trace. +* Try to recover instead by ignoring the error and doing +* a global flush to minimize the change of corruption. +* +* (This is far from being a fully correct recovery. +* Architecturally, the CPU could prefetch something +* back into an incorrect ASID slot and leave it there +* to cause trouble down the road. It's better than +* nothing, though.) +*/ +__flush_tlb_all (); + } +#endif if (real_prev == next) { VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=
Signature · 1 line
2.13.5
Andy Lutomirski
maintainerRe: [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()
Quoted 2 lines · Andy Lutomirski
If we hit the
VM_BUG_ON(), we're detecting a genuinely bad situation, but we're very unlikely to get a useful call trace.
Sigh, typo below. Ingo, if you apply this version, can you fix it?
Quoted 1 line
+ * a global flush to minimize the change of corruption.
chance of corruption.
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()
In reply to Andy Lutomirskix86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()
Commit-ID: a376e7f99be7c1e15b2d986e49b2bec834904381 Gitweb: http://git.kernel.org/tip/a376e7f99be7c1e15b2d986e49b2bec834904381 Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 7 Sep 2017 22:06:57 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 13 Sep 2017 09:50:52 +0200
x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()
If we hit the VM_BUG_ON(), we're detecting a genuinely bad situation, but we're very unlikely to get a useful call trace.
Make it a warning instead.
--- arch/x86/mm/tlb.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
arch/x86/mm/tlb.c +21 -1
@@ -121,8 +121,28 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,* hypothetical buggy code that directly switches to swapper_pg_dir * without going through leave_mm() / switch_mm_irqs_off() or that * does something like write_cr3(read_cr3_pa()). +* +* Only do this check if CONFIG_DEBUG_VM=y because __read_cr3() +* isn't free. */ -VM_BUG_ON (__read_cr3 () != (__sme_pa (real_prev->pgd) | prev_asid)); +#ifdef CONFIG_DEBUG_VM +if (WARN_ON_ONCE (__read_cr3 () != + (__sme_pa (real_prev->pgd) | prev_asid))) { +/* +* If we were to BUG here, we'd be very likely to kill +* the system so hard that we don't see the call trace. +* Try to recover instead by ignoring the error and doing +* a global flush to minimize the chance of corruption. +* +* (This is far from being a fully correct recovery. +* Architecturally, the CPU could prefetch something +* back into an incorrect ASID slot and leave it there +* to cause trouble down the road. It's better than +* nothing, though.) +*/ +__flush_tlb_all (); + } +#endif if (real_prev == next) { VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=
Andy Lutomirski
maintainerPATCH 2/2 x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
In reply to Andy LutomirskiJiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!
FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.
--- arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
arch/x86/power/hibernate_64.c +20 -1
@@ -295,7 +295,26 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)return -EOVERFLOW ; rdr->jump_address = (unsigned long )restore_registers; rdr->jump_address_phys =__pa_symbol (restore_registers); - rdr->cr3 = restore_cr3; + +/* +* The restore code fixes up CR3 and CR4 in the following sequence: +* +* [in hibernation asm] +* 1. CR3 <= temporary page tables +* 2. CR4 <= mmu_cr4_features (from the kernel that restores us) +* 3. CR3 <= rdr->cr3 +* 4. CR4 <= mmu_cr4_features (from us, i.e. the image kernel) +* [in restore_processor_state()] +* 5. CR4 <= saved CR4 +* 6. CR3 <= saved CR3 +* +* Our mmu_cr4_features has CR4.PCIDE=0, and toggling +* CR4.PCIDE while CR3's PCID bits are nonzero is illegal, so +* rdr->cr3 needs to point to valid page tables but must not +* have any of the PCID bits set. +*/ + rdr->cr3 = restore_cr3 & ~CR3_PCID_MASK ; + rdr->magic =RESTORE_MAGIC ;hibernation_e820_save (rdr->e820_digest);
Signature · 1 line
2.13.5
Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
Quoted 11 lines · Andy Lutomirski
Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!
Quoted 2 lines
FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.
I've just performed 15 hibernation cycles with current Linus' tree (5969d1bb3082) with these two patches applied on top of it, and I haven't encountered any issue (and the warning in switch_mm_irqs_off() didn't trigger either).
Quoted 3 lines
Reported-by Jiri Kosina <[email protected]>Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")Signed-off-by Andy Lutomirski <[email protected]>
Thanks!
Signature · 2 lines
Jiri Kosina SUSE Labs
Andy Lutomirski
maintainerRe: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
Quoted 21 lines · Jiri Kosina
Quoted 11 lines · Andy Lutomirski
Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!
Quoted 2 lines
FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.
I've just performed 15 hibernation cycles with current Linus' tree (
5969d1bb3082) with these two patches applied on top of it, and I haven't encountered any issue (and the warning inswitch_mm_irqs_off()didn't trigger either).
Quoted 6 lines
Quoted 3 lines
Reported-by Jiri Kosina <[email protected]>Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")Signed-off-by Andy Lutomirski <[email protected]>Tested-by Jiri Kosina <[email protected]>
Ingo, please do *not* apply this patch yet. The code is fine, but the comment is about to become wrong. I just found a nasty initialization order issue, and I need to rework a bunch of the way we deal with PCIDE.
Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
Quoted 26 lines · Andy Lutomirski
Quoted 25 lines · Jiri Kosina
Quoted 11 lines · Andy Lutomirski
Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!
Quoted 2 lines
FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.
I've just performed 15 hibernation cycles with current Linus' tree (
5969d1bb3082) with these two patches applied on top of it, and I haven't encountered any issue (and the warning inswitch_mm_irqs_off()didn't trigger either).Quoted 3 lines
Reported-by Jiri Kosina <[email protected]>Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")Signed-off-by Andy Lutomirski <[email protected]>
Quoted 7 lines
Quoted 2 lines
Tested-by Jiri Kosina <[email protected]>Ingo, please do *not* apply this patch yet. The code is fine, but the comment is about to become wrong. I just found a nasty initialization order issue, and I need to rework a bunch of the way we deal with PCIDE.
Ok, I'll delay everything PCID delayed - once you've gathered it all together please send a full series against Linus's latest collecting all the fixes/cleanups.
If you find unexpected complications then there will be a point in time where it might be better to just disable PCID for this release and re-try in v4.15. As the number and complexity of fixes increases so does the risk that we'll introduce some last-minute regression.
Thanks,
Ingo
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
In reply to Andy Lutomirskix86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
Commit-ID: f34902c5c6c08024371202a680ce69f2d488776d Gitweb: http://git.kernel.org/tip/f34902c5c6c08024371202a680ce69f2d488776d Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 7 Sep 2017 22:06:58 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 13 Sep 2017 09:52:37 +0200
x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3
Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!
FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.
--- arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
arch/x86/power/hibernate_64.c +20 -1
@@ -295,7 +295,26 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)return -EOVERFLOW ; rdr->jump_address = (unsigned long )restore_registers; rdr->jump_address_phys =__pa_symbol (restore_registers); - rdr->cr3 = restore_cr3; + +/* +* The restore code fixes up CR3 and CR4 in the following sequence: +* +* [in hibernation asm] +* 1. CR3 <= temporary page tables +* 2. CR4 <= mmu_cr4_features (from the kernel that restores us) +* 3. CR3 <= rdr->cr3 +* 4. CR4 <= mmu_cr4_features (from us, i.e. the image kernel) +* [in restore_processor_state()] +* 5. CR4 <= saved CR4 +* 6. CR3 <= saved CR3 +* +* Our mmu_cr4_features has CR4.PCIDE=0, and toggling +* CR4.PCIDE while CR3's PCID bits are nonzero is illegal, so +* rdr->cr3 needs to point to valid page tables but must not +* have any of the PCID bits set. +*/ + rdr->cr3 = restore_cr3 & ~CR3_PCID_MASK ; + rdr->magic =RESTORE_MAGIC ;hibernation_e820_save (rdr->e820_digest);