Re: [PATCH 1/3] x86/S3: Use percpu_traps_init() rather than opencoding SYSCALL/SYSENTER restoration

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2020 20:24, Andrew Cooper wrote:
> On 21/04/2020 08:24, Jan Beulich wrote:
>> On 20.04.2020 16:59, Andrew Cooper wrote:
>>> @@ -46,24 +36,13 @@ void restore_rest_processor_state(void)
>>>      /* Restore full CR4 (inc MCE) now that the IDT is in place. */
>>>      write_cr4(mmu_cr4_features);
>>>  
>>> -    /* Recover syscall MSRs */
>>> -    wrmsrl(MSR_LSTAR, saved_lstar);
>>> -    wrmsrl(MSR_CSTAR, saved_cstar);
>>> -    wrmsrl(MSR_STAR, XEN_MSR_STAR);
>>> -    wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK);
>>> +    /* (re)initialise SYSCALL/SYSENTER state, amongst other things. */
>>> +    percpu_traps_init();
>> Without tweaks to subarch_percpu_traps_init() this assumes that,
>> now and going forward, map_domain_page() will work reliably at
>> this early point of resume. I'm not opposed, i.e.
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I think this reasonable to expect, and robust going forward.
> 
> We are properly in d[IDLE]v0 context when it comes to pagetables, and
> there is nothing interesting between this point and coming fully back
> online.
> 
> That said, I could easily move the call to later in the resume path for
> even more certainty.
> 
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 3ad7dfc9a3..d5a468eddd 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -297,6 +297,8 @@ static int enter_state(u32 state)
>      ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
>      spec_ctrl_exit_idle(ci);
>  
> +    /* (re)initialise SYSCALL/SYSENTER state, amongst other things. */
> +    percpu_traps_init();
> +
>   done:
>      spin_debug_enable();
>      local_irq_restore(flags);
> 
> In fact - I prefer this, because it works towards one low priority goal
> of deleting {save,restore}_rest_processor_state() which I've still got a
> pending series for.
> 
> Would your ack still stand if I tweak the patch in this way?

Yes.

Jan