[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Thu, 23 Jul 2020 17:00:32 +0100
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "M. Vefa Bicakci" <m.v.b@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Thu, 23 Jul 2020 16:00:45 +0000
  • Ironport-sdr: xwhDdYuWlOcdmIa3e8tGYl1S09NqyRiXNeYFP7dk3c7IBOvqtPBOz2WkIrQliKapKQcTUAZw1H H1j/WMMYfRYgnvluBV8qtj/WU4gRIICYk7IBoPjT5FKfI/314DRZCeR94r7QbLI+BtS7hitl4I MT1q0+PPtpjQ33Z33B5jWdcziIOEuVD57spAaYX1+hQPylFrBxBhiO9hRkj/hRI9wt0JmpWOeh U/qcK3sAFcvWzkfIXAHfk8byOAOJHcZbm4LPErwPldUule1DuSJr/GvWZBxTZcUAvYxLUiCgM1 SdY=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.