Resume from suspend to RAM broken when using early microcode updates
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to enter_state() (from power.c) it doesn't survive the local_irq_restore(flags) call (at least a printk() after the call doesn't output anything on the serial console).
I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
If I simply move the microcode_resume_cpu(0) directly before the local_irq_restore(flags) everything seems to work fine. But I'm not sure if this has unintended consequences.
I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
Simon
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 12 lines · Simon Gaiser
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
Quoted 8 lines
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
S3 support is a very unloved area of the hypervisor.
Yes - we definitely need to get microcode reloaded before interrupts are enabled. That said, I would have expected a backtrace complaining about a GP fault if we had hit the use of IBRS/IBPB before the microcode was reloaded.
~Andrew
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Andrew Cooper:
Quoted 22 lines · Simon Gaiser
Quoted 12 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
Quoted 8 lines
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
Quoted 4 lines
S3 support is a very unloved area of the hypervisor.
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving microcode_resume_cpu(0) directly before the local_irq_restore(flags) call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)
Quoted 3 lines
That said, I would have expected a backtrace complaining about a GP fault if we had hit the use of IBRS/IBPB before the microcode was reloaded.
Yeah, not sure what's happening here. I don't get any output from after local_irq_restore(flags). If you have some ideas for more debug output I can easily test it.
Simon
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 26 lines
Quoted 1 line
Quoted 1 line · On 11.04.18 at 14:01, <[email protected]> wrote:
Andrew Cooper:
Quoted 24 lines · Simon Gaiser
Quoted 12 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
Quoted 8 lines
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
S3 support is a very unloved area of the hypervisor.
Quoted 7 lines
Quoted 2 lines
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)
Yes, there would be a problem: microcode_resume_cpu() spin_lock()-s almost first thing, and this would break our (simplistic) lock checking. Putting it also ahead of spin_debug_enable() should work otoh.
Once at it, cpufreq_add_cpu() should be moved ahead of the enable_cpu label as well, as cpufreq_del_cpu() wasn't called yet at the point of the only goto to that label.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 35 lines
Quoted 27 lines
Quoted 2 lines · On 11.04.18 at 14:11, <[email protected]> wrote:
Quoted 1 line · On 11.04.18 at 14:01, <[email protected]> wrote:
Andrew Cooper:
Quoted 24 lines · Simon Gaiser
Quoted 12 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
Quoted 8 lines
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
S3 support is a very unloved area of the hypervisor.
Quoted 7 lines
Quoted 2 lines
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)
Quoted 8 lines
Yes, there would be a problem:
microcode_resume_cpu()spin_lock()-s almost first thing, and this would break our (simplistic) lock checking. Putting it also ahead ofspin_debug_enable()should work otoh.Once at it,
cpufreq_add_cpu()should be moved ahead of theenable_cpulabel as well, ascpufreq_del_cpu()wasn't called yet at the point of the only goto to that label.
And I think console_end_sync() want to be moved earlier then as well.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Jan Beulich:
Quoted 40 lines
Quoted 40 lines
Quoted 27 lines
Quoted 2 lines · On 11.04.18 at 14:11, <[email protected]> wrote:
Quoted 1 line · On 11.04.18 at 14:01, <[email protected]> wrote:
Andrew Cooper:
Quoted 24 lines · Simon Gaiser
Quoted 12 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
Quoted 8 lines
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
S3 support is a very unloved area of the hypervisor.
Quoted 7 lines
Quoted 2 lines
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)Yes, there would be a problem:
microcode_resume_cpu()spin_lock()-s almost first thing, and this would break our (simplistic) lock checking. Putting it also ahead ofspin_debug_enable()should work otoh.
Quoted 6 lines
Quoted 3 lines
Once at it,
cpufreq_add_cpu()should be moved ahead of theenable_cpulabel as well, ascpufreq_del_cpu()wasn't called yet at the point of the only goto to that label.And I think
console_end_sync()want to be moved earlier then as well.
Where exactly? console_end_sync() seems to match the position of console_start_sync().
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 46 lines
Quoted 1 line
Quoted 1 line · On 11.04.18 at 14:46, <[email protected]> wrote:
Jan Beulich:
Quoted 44 lines
Quoted 43 lines
Quoted 27 lines
Quoted 2 lines · On 11.04.18 at 14:11, <[email protected]> wrote:
Quoted 1 line · On 11.04.18 at 14:01, <[email protected]> wrote:
Andrew Cooper:
Quoted 24 lines · Simon Gaiser
Quoted 12 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
Quoted 8 lines
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
S3 support is a very unloved area of the hypervisor.
Quoted 7 lines
Quoted 2 lines
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)Yes, there would be a problem:
microcode_resume_cpu()spin_lock()-s almost first thing, and this would break our (simplistic) lock checking. Putting it also ahead ofspin_debug_enable()should work otoh.Once at it,
cpufreq_add_cpu()should be moved ahead of theenable_cpulabel as well, ascpufreq_del_cpu()wasn't called yet at the point of the only goto to that label.
Quoted 5 lines
Quoted 2 lines
And I think
console_end_sync()want to be moved earlier then as well.Where exactly?
console_end_sync()seems to match the position ofconsole_start_sync().
The question isn't symmetry with the start_sync, but the fact that at the right log level (and on big systems) microcode updates can be quite verbose. We don't want all this output to go out in sync mode, I think, albeit then again doing the output in normal mode may mean some of it gets discarded (but personally I think that's acceptable).
As to where exactly, the easiest seems to be to hand you a patch. Please give this a try. Of course none of this addresses a possible NMI or #MC occurring before the microcode loading.
Jan
x86: correct ordering of operations during S3 resume
Microcode loading needs to happen before re-enabling interrupts, in case only updated microcode allows the use of e.g. the SPEC_{CTRL,CMD} MSRs. Otoh it doesn't need to happen at all when we didn't suspend in the first place. It needs to happen before spin_debug_enable() though, as it acquires a lock and hence would otherwise make common/spinlock.c:check_lock() unhappy. As micrcode loading can be pretty verbose, also make sure it only runs after console_end_sync().
cpufreq_add_cpu() doesn't need calling on the only "goto enable_cpu" path, which sits ahead of cpufreq_del_cpu().
xen/arch/x86/acpi/power.c +6 -3
@@ -203,6 +203,7 @@ static int enter_state(u32 state)printk (XENLOG_ERR "Some devices failed to power down." ); system_state = SYS_STATE_resume;device_power_up (error); +console_end_sync (); error = -EIO ; goto done; }
@@ -243,17 +244,19 @@ static int enter_state(u32 state)if ( (state ==ACPI_STATE_S3 ) && error )tboot_s3_error (error); +console_end_sync (); + +microcode_resume_cpu (0 ); + done:spin_debug_enable ();local_irq_restore (flags); -console_end_sync ();acpi_sleep_post (state);if (hvm_cpu_up () )BUG (); +cpufreq_add_cpu (0 ); enable_cpu: -cpufreq_add_cpu (0 ); -microcode_resume_cpu (0 );rcu_barrier ();mtrr_aps_sync_begin ();enable_nonboot_cpus ();
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 1/2 x86/microcode: Indicate "not found" in rc of microcode_resume_cpu()
Make it possible to distinguish between a failure to load a microcode update and a failure to find any matching microcode update by returning -ENOENT (instead of -EIO) in the later case.
--- xen/arch/x86/microcode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
xen/arch/x86/microcode.c +1 -1
@@ -248,7 +248,7 @@ int microcode_resume_cpu(unsigned int cpu)__microcode_fini_cpu (cpu); uci->cpu_sig = nsig; - err = -EIO ; + err = -ENOENT ;for_each_online_cpu ( cpu2 ) { uci = &per_cpu (ucode_cpu_info, cpu2);
Signature · 7 lines
2.16.2 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 2/2 x86: correct ordering of operations during S3 resume
Microcode loading needs to happen before re-enabling interrupts, in case only updated microcode allows the use of e.g. the SPEC_{CTRL,CMD} MSRs. Otoh it doesn't need to happen at all when we didn't suspend in the first place. It needs to happen before spin_debug_enable() though, as it acquires a lock and hence would otherwise make common/spinlock.c:check_lock() unhappy. As microcode loading can be pretty verbose, also make sure it only runs after console_end_sync().
cpufreq_add_cpu() doesn't need calling on the only "goto enable_cpu" path, which sits ahead of cpufreq_del_cpu().
--- xen/arch/x86/acpi/power.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
xen/arch/x86/acpi/power.c +8 -3
@@ -203,6 +203,7 @@ static int enter_state(u32 state)printk (XENLOG_ERR "Some devices failed to power down." ); system_state = SYS_STATE_resume;device_power_up (error); +console_end_sync (); error = -EIO ; goto done; }
@@ -243,17 +244,21 @@ static int enter_state(u32 state)if ( (state ==ACPI_STATE_S3 ) && error )tboot_s3_error (error); +console_end_sync (); + + error =microcode_resume_cpu (0 ); +if (error && error != -ENOENT ) +panic ("Could not restore microcode on boot cpu (%d)" , error); + done:spin_debug_enable ();local_irq_restore (flags); -console_end_sync ();acpi_sleep_post (state);if (hvm_cpu_up () )BUG (); +cpufreq_add_cpu (0 ); enable_cpu: -cpufreq_add_cpu (0 ); -microcode_resume_cpu (0 );rcu_barrier ();mtrr_aps_sync_begin ();enable_nonboot_cpus ();
Signature · 7 lines
2.16.2 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH v2 2/2 x86: correct ordering of operations during S3 resume
From: Jan Beulich <[email protected]>
Microcode loading needs to happen before re-enabling interrupts, in case only updated microcode allows the use of e.g. the SPEC_{CTRL,CMD} MSRs. Otoh it doesn't need to happen at all when we didn't suspend in the first place. It needs to happen before spin_debug_enable() though, as it acquires a lock and hence would otherwise make common/spinlock.c:check_lock() unhappy. As micrcode loading can be pretty verbose, also make sure it only runs after console_end_sync().
cpufreq_add_cpu() doesn't need calling on the only "goto enable_cpu" path, which sits ahead of cpufreq_del_cpu().
---
Sorry didn't want to rewrite the author. No other change in v2.
xen/arch/x86/acpi/power.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
xen/arch/x86/acpi/power.c +8 -3
@@ -203,6 +203,7 @@ static int enter_state(u32 state)printk (XENLOG_ERR "Some devices failed to power down." ); system_state = SYS_STATE_resume;device_power_up (error); +console_end_sync (); error = -EIO ; goto done; }
@@ -243,17 +244,21 @@ static int enter_state(u32 state)if ( (state ==ACPI_STATE_S3 ) && error )tboot_s3_error (error); +console_end_sync (); + + error =microcode_resume_cpu (0 ); +if (error && error != -ENOENT ) +panic ("Could not restore microcode on boot cpu (%d)" , error); + done:spin_debug_enable ();local_irq_restore (flags); -console_end_sync ();acpi_sleep_post (state);if (hvm_cpu_up () )BUG (); +cpufreq_add_cpu (0 ); enable_cpu: -cpufreq_add_cpu (0 ); -microcode_resume_cpu (0 );rcu_barrier ();mtrr_aps_sync_begin ();enable_nonboot_cpus ();
Signature · 7 lines
2.16.2 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v2 2/2] x86: correct ordering of operations during S3 resume
xen/arch/x86/acpi/power.c static int enter_state(u32 state) · line 250
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 11.04.18 at 22:21, <[email protected]> wrote:
xen/arch/x86/acpi/power.c +4
-0@@ -243,17 +244,21 @@ static int enter_state(u32 state)if ( (state ==ACPI_STATE_S3 ) && error )tboot_s3_error (error); +console_end_sync (); + + error =microcode_resume_cpu (0 ); +if (error && error != -ENOENT )
Missing blanks.
xen/arch/x86/acpi/power.c static int enter_state(u32 state) · line 251
Quoted 1 line
xen/arch/x86/acpi/power.c +1
-0+panic ("Could not restore microcode on boot cpu (%d)" , error);
Andrew, you've suggested the panic() here, but I'm not convinced this is strictly necessary. For most ucode updates we're fine without, and could accept them being re-applied post-resume. That'll make a lot of false positive panics. Furthermore, in case there are really mixed steppings, receiving -ENOENT here still means we're going to die soon after. I.e. (rare) false negatives are possible as well.
Instead what I think we want is a feature comparison after resume:
Any feature we (or any alive guests) have in active use needs to be available. That is, host_cpuid_policy must not have changed (the two {hvm,pv}_max_cpuid_policy are only derived, and hence won't need separate checking afaict; without that checking host_cpuid_policy might be too strict, but then again compiling a list of features we actually use would be prone to go stale very quickly once use of new features is introduced, unless we tied this into cpu_has_* checks, implying that any such check means the feature is used if available, yet that would in turn have issues in particular with the uses in the insn emulator).
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH 1/2] x86/microcode: Indicate "not found" in rc of microcode_resume_cpu()
In reply to Simon GaiserQuoted 15 lines
Quoted 1 line
Quoted 1 line · On 11.04.18 at 22:14, <[email protected]> wrote:
Make it possible to distinguish between a failure to load a microcode update and a failure to find any matching microcode update by returning
-ENOENT(instead of-EIO) in the later case.Signed-off-by Simon Gaiser <[email protected]>--- xen/arch/x86/microcode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)xen/arch/x86/microcode.c +0
-0
xen/arch/x86/microcode.c int microcode_resume_cpu(unsigned int cpu) · line 254
Quoted 8 lines
xen/arch/x86/microcode.c +1
-1@@ -248,7 +248,7 @@ int microcode_resume_cpu(unsigned int cpu)__microcode_fini_cpu (cpu); uci->cpu_sig = nsig; - err = -EIO ; + err = -ENOENT ;for_each_online_cpu ( cpu2 ) { uci = &per_cpu (ucode_cpu_info, cpu2);
I think this should be part of the other patch, but I'll reply there in a minute.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 22 lines · Simon Gaiser
Andrew Cooper:
Quoted 21 lines · Simon Gaiser
Quoted 20 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
Quoted 8 lines
Quoted 4 lines
S3 support is a very unloved area of the hypervisor.
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)
Judging by what is going on, it wants to be between tboot_s3_error() and the done label.
We only need to restore microcode if we successfully went into S3. The done and enable_cpu labels are only used by paths which don't need to restore microcode.
OTOH, you should check the return value and panic if restoration failed. As you've seen, the system won't survive trying to blindly continue resuming.
Quoted 7 lines
Quoted 4 lines
That said, I would have expected a backtrace complaining about a GP fault if we had hit the use of IBRS/IBPB before the microcode was reloaded.
Yeah, not sure what's happening here. I don't get any output from after
local_irq_restore(flags). If you have some ideas for more debug output I can easily test it.
In hindsight, I am. We take a #GP fault because of a bad MSR, and at the head of the exception handler try to use the same bad MSR. It will repeatedly fault until hitting a guard page (or other read-only page), at which point we take a double fault, and suffer a #GP yet again. Taking a #DF will reset the stack to a moderately sane value, and the system will livelock taking faults.
This is an unfortunate consequence of having $MAGIC in the exception handlers.
~Andrew
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 58 lines · Andrew Cooper
Quoted 22 lines · Simon Gaiser
Andrew Cooper:
Quoted 21 lines · Simon Gaiser
Quoted 20 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
Quoted 8 lines
Quoted 4 lines
S3 support is a very unloved area of the hypervisor.
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)Judging by what is going on, it wants to be between
tboot_s3_error()and the done label.We only need to restore microcode if we successfully went into S3. The done and
enable_cpulabels are only used by paths which don't need to restore microcode.OTOH, you should check the return value and panic if restoration failed. As you've seen, the system won't survive trying to blindly continue resuming.
Quoted 7 lines
Quoted 4 lines
That said, I would have expected a backtrace complaining about a GP fault if we had hit the use of IBRS/IBPB before the microcode was reloaded.
Yeah, not sure what's happening here. I don't get any output from after
local_irq_restore(flags). If you have some ideas for more debug output I can easily test it.In hindsight, I am. We take a #GP fault because of a bad MSR, and at the head of the exception handler try to use the same bad MSR. It will repeatedly fault until hitting a guard page (or other read-only page), at which point we take a double fault, and suffer a #GP yet again. Taking a #DF will reset the stack to a moderately sane value, and the system will livelock taking faults.
Quoted 2 lines
This is an unfortunate consequence of having $MAGIC in the exception handlers.
We can just disable IBRS before going to sleep?
Quoted 7 lines
~Andrew
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 56 lines · Konrad Rzeszutek Wilk
Quoted 55 lines · Andrew Cooper
Quoted 22 lines · Simon Gaiser
Andrew Cooper:
Quoted 21 lines · Simon Gaiser
Quoted 20 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
Quoted 8 lines
Quoted 4 lines
S3 support is a very unloved area of the hypervisor.
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)Judging by what is going on, it wants to be between
tboot_s3_error()and the done label.We only need to restore microcode if we successfully went into S3. The done and
enable_cpulabels are only used by paths which don't need to restore microcode.OTOH, you should check the return value and panic if restoration failed. As you've seen, the system won't survive trying to blindly continue resuming.
Quoted 6 lines
Quoted 3 lines
That said, I would have expected a backtrace complaining about a GP fault if we had hit the use of IBRS/IBPB before the microcode was reloaded.
Yeah, not sure what's happening here. I don't get any output from after
local_irq_restore(flags). If you have some ideas for more debug output I can easily test it.In hindsight, I am. We take a #GP fault because of a bad MSR, and at the head of the exception handler try to use the same bad MSR. It will repeatedly fault until hitting a guard page (or other read-only page), at which point we take a double fault, and suffer a #GP yet again. Taking a #DF will reset the stack to a moderately sane value, and the system will livelock taking faults.
Quoted 3 lines
Quoted 2 lines
This is an unfortunate consequence of having $MAGIC in the exception handlers.
We can just disable IBRS before going to sleep?
The problem is the use of MSR_SPEC_CTRL/MSR_PRED_CMD before we've reloaded the microcode which causes those MSRs to magic themselves into existence.
~Andrew
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 58 lines
Quoted 1 line
Quoted 1 line · On 11.04.18 at 17:05, <[email protected]> wrote:
Quoted 56 lines · Konrad Rzeszutek Wilk
Quoted 55 lines · Andrew Cooper
Quoted 22 lines · Simon Gaiser
Andrew Cooper:
Quoted 21 lines · Simon Gaiser
Quoted 20 lines
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
Quoted 8 lines
Quoted 4 lines
S3 support is a very unloved area of the hypervisor.
Yes - we definitely need to get microcode reloaded before interrupts are enabled.
Do you see any problems with simply moving
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)call? (I'm not familiar with the code at all and (early) resume handling sounds like something which is easy to break in non obvious ways)Judging by what is going on, it wants to be between
tboot_s3_error()and the done label.We only need to restore microcode if we successfully went into S3. The done and
enable_cpulabels are only used by paths which don't need to restore microcode.OTOH, you should check the return value and panic if restoration failed. As you've seen, the system won't survive trying to blindly continue resuming.
Quoted 6 lines
Quoted 3 lines
That said, I would have expected a backtrace complaining about a GP fault if we had hit the use of IBRS/IBPB before the microcode was reloaded.
Yeah, not sure what's happening here. I don't get any output from after
local_irq_restore(flags). If you have some ideas for more debug output I can easily test it.In hindsight, I am. We take a #GP fault because of a bad MSR, and at the head of the exception handler try to use the same bad MSR. It will repeatedly fault until hitting a guard page (or other read-only page), at which point we take a double fault, and suffer a #GP yet again. Taking a #DF will reset the stack to a moderately sane value, and the system will livelock taking faults.
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
This is an unfortunate consequence of having $MAGIC in the exception handlers.
We can just disable IBRS before going to sleep?
The problem is the use of MSR_SPEC_CTRL/MSR_PRED_CMD before we've reloaded the microcode which causes those MSRs to magic themselves into existence.
Well, Konrad certainly has a point: Just like we could make SPEC_CTRL_ENTRY_FROM_INTR_IST skip the WRMSR by clearing BTI_IST_WRMSR, we could add a conditional branch to DO_SPEC_CTRL_ENTRY's maybexen case. The former would also allow to deal with an early (after resume) NMI or #MC.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 23 lines
Quoted 1 line
Quoted 1 line · On 11.04.18 at 13:51, <[email protected]> wrote:
Quoted 12 lines · Simon Gaiser
Hi,
when I use early microcode loading with the microcode update with the BTI mitigations, resuming from suspend to RAM is broken.
Based on added logging to
enter_state()(frompower.c) it doesn't survive thelocal_irq_restore(flags)call (at least aprintk()after the call doesn't output anything on the serial console).I guess that some irq handler tries to use IBRS/IBPB. But the microcode is only loaded later.
Quoted 8 lines
If I simply move the
microcode_resume_cpu(0)directly before thelocal_irq_restore(flags)everything seems to work fine. But I'm not sure if this has unintended consequences.I tested the above with Xen 4.8.3 from Qubes which includes the BTI and microcode patches from staging-4.8. AFAICS there are no commits which changes the affected code or other commits which sound relevant so this probably affected also all the newer branches.
Quoted 6 lines
S3 support is a very unloved area of the hypervisor.
Yes - we definitely need to get microcode reloaded before interrupts are enabled. That said, I would have expected a backtrace complaining about a GP fault if we had hit the use of IBRS/IBPB before the microcode was reloaded.
Wouldn't the #GP handling re-raise a #GP for the same reason, until hitting the end of the stack, making it a #DF, the handler of which would yet again have the same issue? This would end with an infinite loop between #GP and #DF handlers (never triple faulting), and no output.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel