Re: [Xen-devel] Resume from suspend to RAM broken when using early microcode updates

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.18 at 17:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 11/04/18 15:49, Konrad Rzeszutek Wilk wrote:
>> On Wed, Apr 11, 2018 at 01:12:51PM +0100, Andrew Cooper wrote:
>>> On 11/04/18 13:01, Simon Gaiser wrote:
>>>> Andrew Cooper:
>>>>> On 11/04/18 12:48, Simon Gaiser wrote:
>>>>>> 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.
>>>>> 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)
>>> 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.
>>>
>>>>> 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.
>> 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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel