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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> It is a shame that the CR0 load breaks up the obvious connection with
> lgdt, but IIRC, that was a consequence of how the code was laid out
> previously.
> 
> Preferably with the above diff, Reviewed-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>

Thanks, Jan