Re: [PATCH] x86/S3: put data segment registers into known state upon resume

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/07/2020 00:29, M. Vefa Bicakci wrote:
> On 7/23/20 7:00 PM, Andrew Cooper wrote:
>> On 23/07/2020 16:19, Jan Beulich wrote:
>>> On 23.07.2020 16:40, Andrew Cooper wrote:
>>>> On 20/07/2020 16:20, Jan Beulich wrote:
>>>>> wakeup_32 sets %ds and %es to BOOT_DS, while leaving %fs at what
>>>>> wakeup_start did set it to, and %gs at whatever BIOS did load into
>>>>> it.
>>>>> All of this may end up confusing the first load_segments() to run on
>>>>> the BSP after resume, in particular allowing a non-nul selector value
>>>>> to be left in %fs.
>>>>>
>>>>> Alongside %ss, also put all other data segment registers into the
>>>>> same
>>>>> state that the boot and CPU bringup paths put them in.
>>>>>
>>>>> Reported-by: M. Vefa Bicakci <m.v.b@xxxxxxxxxx>
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>
>>>>> --- a/xen/arch/x86/acpi/wakeup_prot.S
>>>>> +++ b/xen/arch/x86/acpi/wakeup_prot.S
>>>>> @@ -52,6 +52,16 @@ ENTRY(s3_resume)
>>>>>           mov     %eax, %ss
>>>>>           mov     saved_rsp(%rip), %rsp
>>>>>   +        /*
>>>>> +         * Also put other segment registers into known state,
>>>>> like would
>>>>> +         * be done on the boot path. This is in particular
>>>>> necessary for
>>>>> +         * the first load_segments() to work as intended.
>>>>> +         */
>>>> I don't think the comment is helpful, not least because it refers to a
>>>> broken behaviour in load_segemnts() which is soon going to change
>>>> anyway.
>>> Well, I can drop it. I merely thought I'd be nice and comment my
>>> code once in a while (and the comment could be dropped / adjusted
>>> when load_segments() changes)...
>>>
>>>> We've literally just loaded the GDT, at which point reloading all
>>>> segments *is* the expected thing to do.
>>> In a way, unless some/all are assumed to already hold a nul selector.
>>>
>>>> I'd recommend that the diff be simply:
>>>>
>>>> diff --git a/xen/arch/x86/acpi/wakeup_prot.S
>>>> b/xen/arch/x86/acpi/wakeup_prot.S
>>>> index dcc7e2327d..a2c41c4f3f 100644
>>>> --- a/xen/arch/x86/acpi/wakeup_prot.S
>>>> +++ b/xen/arch/x86/acpi/wakeup_prot.S
>>>> @@ -49,6 +49,10 @@ ENTRY(s3_resume)
>>>>           mov     %rax, %cr0
>>>>             mov     $__HYPERVISOR_DS64, %eax
>>>> +        mov     %eax, %ds
>>>> +        mov     %eax, %es
>>>> +        mov     %eax, %fs
>>>> +        mov     %eax, %gs
>>>>           mov     %eax, %ss
>>>>           mov     saved_rsp(%rip), %rsp
>>> So I had specifically elected to not put the addition there, to make
>>> sure the stack would get established first. But seeing both Roger
>>> and you ask me to do otherwise - well, so be it then.
>>
>> There is no IDT.  Any fault is will be triple, irrespective of the exact
>> code layout.
>>
>> This sequence actually matches what we have in __high_start().
>>
>> I don't think it is wise to write code which presumes that
>> __HYPERVISOR_DS64 is 0 (it happens to be, but could easily be 0xe010 as
>> well), or that the trampoline has fixed behaviours for the segments.
>
> Hello Jan and Andrew,
>
> Is there anything I can do to help with the delivery/merging of this
> patch?

It was committed last Friday.

https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=55f8c389d4348cc517946fdcb10794112458e81e

I presume Jan will backport it to stable trees when he's not OoO.

~Andrew