Re: [Xen-devel] [PATCH for-4.11] x86/suspend: Fix restoration of guest state across S3/S4

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.06.18 at 11:28, <JBeulich@xxxxxxxx> wrote:
>>>> On 21.06.18 at 10:37, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 21/06/18 16:29, Jan Beulich wrote:
>>>>>> On 20.06.18 at 12:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> The call to freeze_domains() in enter_state() guarentees that we are
>>>> running in idle context for the duration of S3/S4.
>>>>
>>>> In restore_rest_processor_state(), the stts() is problematic as it
>>>> unilaterally sets %cr0.ts even in fully_eager FPU context.  It also fails 
>>>> to
>>>> account for the non-lazy xsave state.  Luckily, these are both latent 
>>>> bugs, 
> as
>>>> the FPU state is corrected by the subsequent context switch away from the 
> idle
>>>> vcpu.
>>>>
>>>> Another aspect is that the !is_idle_vcpu(curr) paths in
>>>> restore_rest_processor_state() are actually dead code, and removing
>>>> these highlights that the segment saving logic is also unused.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> For 4.12:
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> As mentioned elsewhere (and despite having seen Jürgen's R-a-b) I'm not
>>> convinced this is an appropriate change to make for 4.11, considering how
>>> late in the process we are. If we want to consider this instead of my much
>>> smaller suggested change, then we will want to have someone actively
>>> using S3 test this thoroughly. Don't forget that whatever we do on 4.11 to
>>> address the regression will also want backporting.
>> 
>> Yes - considering how this patch ended up, I withdraw the 4.11 part of
>> it.  An earlier development version had a functional fix for lazy
>> segment restoration which is why I thought it was going to be properly
>> needed for 4.11
>> 
>> As this is straight deletion, it can wait until 4.12
> 
> Thanks. With the moved (in the EFI patch) assertion, mine can actually be
> further simplified. I'll submit this in a minute.

Actually I think no fix is needed there at all if we're always on an idle
vCPU: Nothing needs to be restored, and afaict CR0.TS is set already
anyway (due to the stts() in vcpu_save_fpu(), which was invoked when
the last non-idle vCPU was taken off of that CPU). So I won't submit
anything here for the moment, but I'll see about addressing the further
EFI issue mentioned on irc.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel