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....@runbox.com> > Signed-off-by: Jan Beulich <jbeul...@suse.com> > > --- 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. We've literally just loaded the GDT, at which point reloading all segments *is* the expected thing to do. 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 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.coop...@citrix.com>
