| Message ID | 1555663857-4173-1-git-send-email-fionali-oc@zhaoxin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 19/04/2019 09:50, FionaLi-oc wrote: > When executing SYSEXIT or SYSENTRY in Zhaoxin CPU, CPU needs to > save or restore a set of MSRs. > > Signed-off-by: FionaLi-oc <fionali-oc@zhaoxin.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> I've pulled this into x86-next, and it will get committed when CI has been repaired and is working again.
>>> On 19.04.19 at 10:50, <fionali-oc@zhaoxin.com> wrote: > When executing SYSEXIT or SYSENTRY in Zhaoxin CPU, CPU needs to SYSENTER > --- a/xen/arch/x86/x86_64/traps.c > +++ b/xen/arch/x86/x86_64/traps.c > @@ -334,7 +334,8 @@ void subarch_percpu_traps_init(void) > (unsigned long)lstar_enter); > stub_va += offset; > > - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) ) > + if ( boot_cpu_data.x86_vendor & > + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | X86_VENDOR_SHANGHAI) ) > { > /* SYSENTER entry. */ > wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom); How is this hunk related to the title of the change? I think the title wants to be adjusted. Furthermore for all of the changes done, wouldn't we better switch to use cpu_has_sep? init_amd() as well as default_init() already clear this flag. Andrew, thoughts? Jan
On 25/04/2019 14:21, Jan Beulich wrote: >>>> On 19.04.19 at 10:50, <fionali-oc@zhaoxin.com> wrote: >> When executing SYSEXIT or SYSENTRY in Zhaoxin CPU, CPU needs to > SYSENTER > >> --- a/xen/arch/x86/x86_64/traps.c >> +++ b/xen/arch/x86/x86_64/traps.c >> @@ -334,7 +334,8 @@ void subarch_percpu_traps_init(void) >> (unsigned long)lstar_enter); >> stub_va += offset; >> >> - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) ) >> + if ( boot_cpu_data.x86_vendor & >> + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | X86_VENDOR_SHANGHAI) ) >> { >> /* SYSENTER entry. */ >> wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom); > How is this hunk related to the title of the change? I think the > title wants to be adjusted. > > Furthermore for all of the changes done, wouldn't we better > switch to use cpu_has_sep? init_amd() as well as default_init() > already clear this flag. Andrew, thoughts? I wondered exactly that after queuing this patch, but didn't get around to experimenting. We have to be a little careful with the ordering of operations. cpu_has_sep is visible in CPUID but needs clobbering in AMD/Hygon before cpu_has_sep is safe to use (although for the MSRs, it should be safe). ~Andrew
>>> On 25.04.19 at 15:25, <andrew.cooper3@citrix.com> wrote: > On 25/04/2019 14:21, Jan Beulich wrote: >>>>> On 19.04.19 at 10:50, <fionali-oc@zhaoxin.com> wrote: >>> --- a/xen/arch/x86/x86_64/traps.c >>> +++ b/xen/arch/x86/x86_64/traps.c >>> @@ -334,7 +334,8 @@ void subarch_percpu_traps_init(void) >>> (unsigned long)lstar_enter); >>> stub_va += offset; >>> >>> - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) > ) >>> + if ( boot_cpu_data.x86_vendor & >>> + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | X86_VENDOR_SHANGHAI) ) >>> { >>> /* SYSENTER entry. */ >>> wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom); >> How is this hunk related to the title of the change? I think the >> title wants to be adjusted. >> >> Furthermore for all of the changes done, wouldn't we better >> switch to use cpu_has_sep? init_amd() as well as default_init() >> already clear this flag. Andrew, thoughts? > > I wondered exactly that after queuing this patch, but didn't get around > to experimenting. > > We have to be a little careful with the ordering of operations. > cpu_has_sep is visible in CPUID but needs clobbering in AMD/Hygon before > cpu_has_sep is safe to use (although for the MSRs, it should be safe). trap_init() (and hence percpu_traps_init()) runs after, in particular, init_speculation_mitigations(), which means all feature collection and massaging must have happened. So unless you explicitly say otherwise, I'd like to ask FionaLi to submit a v2 with that change (and with the other cosmetic adjustments carried out). Jan
On 25/04/2019 14:37, Jan Beulich wrote: >>>> On 25.04.19 at 15:25, <andrew.cooper3@citrix.com> wrote: >> On 25/04/2019 14:21, Jan Beulich wrote: >>>>>> On 19.04.19 at 10:50, <fionali-oc@zhaoxin.com> wrote: >>>> --- a/xen/arch/x86/x86_64/traps.c >>>> +++ b/xen/arch/x86/x86_64/traps.c >>>> @@ -334,7 +334,8 @@ void subarch_percpu_traps_init(void) >>>> (unsigned long)lstar_enter); >>>> stub_va += offset; >>>> >>>> - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) >> ) >>>> + if ( boot_cpu_data.x86_vendor & >>>> + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | X86_VENDOR_SHANGHAI) ) >>>> { >>>> /* SYSENTER entry. */ >>>> wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom); >>> How is this hunk related to the title of the change? I think the >>> title wants to be adjusted. >>> >>> Furthermore for all of the changes done, wouldn't we better >>> switch to use cpu_has_sep? init_amd() as well as default_init() >>> already clear this flag. Andrew, thoughts? >> I wondered exactly that after queuing this patch, but didn't get around >> to experimenting. >> >> We have to be a little careful with the ordering of operations. >> cpu_has_sep is visible in CPUID but needs clobbering in AMD/Hygon before >> cpu_has_sep is safe to use (although for the MSRs, it should be safe). > trap_init() (and hence percpu_traps_init()) runs after, in > particular, init_speculation_mitigations(), which means all > feature collection and massaging must have happened. So > unless you explicitly say otherwise, I'd like to ask FionaLi > to submit a v2 with that change (and with the other cosmetic > adjustments carried out). I'll drop this patch from x86-next. Overall, I think a cpu_has_sep based solution would be preferable. ~Andrew
Andrew， Do I need to submit a v3 with cpu_has_sep based solution? Or do you deal with it? > -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: Thursday, April 25, 2019 9:42 PM > To: Jan Beulich <JBeulich@suse.com>; FionaLi-oc <FionaLi-oc@zhaoxin.com> > Cc: Roger Pau Monne <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; > xen-devel <xen-devel@lists.xenproject.org>; Cobe Chen(BJ-RD) > <CobeChen@zhaoxin.com> > Subject: Re: [PATCH 2/2 v2] x86/acpi: Improve suspend and resume process for > Zhaoxin CPU > > On 25/04/2019 14:37, Jan Beulich wrote: > >>>> On 25.04.19 at 15:25, <andrew.cooper3@citrix.com> wrote: > >> On 25/04/2019 14:21, Jan Beulich wrote: > >>>>>> On 19.04.19 at 10:50, <fionali-oc@zhaoxin.com> wrote: > >>>> --- a/xen/arch/x86/x86_64/traps.c > >>>> +++ b/xen/arch/x86/x86_64/traps.c > >>>> @@ -334,7 +334,8 @@ void subarch_percpu_traps_init(void) > >>>> (unsigned long)lstar_enter); > >>>> stub_va += offset; > >>>> > >>>> - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | > X86_VENDOR_CENTAUR) > >> ) > >>>> + if ( boot_cpu_data.x86_vendor & > >>>> + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | > >>>> + X86_VENDOR_SHANGHAI) ) > >>>> { > >>>> /* SYSENTER entry. */ > >>>> wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom); > >>> How is this hunk related to the title of the change? I think the > >>> title wants to be adjusted. > >>> > >>> Furthermore for all of the changes done, wouldn't we better switch > >>> to use cpu_has_sep? init_amd() as well as default_init() already > >>> clear this flag. Andrew, thoughts? > >> I wondered exactly that after queuing this patch, but didn't get > >> around to experimenting. > >> > >> We have to be a little careful with the ordering of operations. > >> cpu_has_sep is visible in CPUID but needs clobbering in AMD/Hygon > >> before cpu_has_sep is safe to use (although for the MSRs, it should be safe). > > trap_init() (and hence percpu_traps_init()) runs after, in particular, > > init_speculation_mitigations(), which means all feature collection and > > massaging must have happened. So unless you explicitly say otherwise, > > I'd like to ask FionaLi to submit a v2 with that change (and with the > > other cosmetic adjustments carried out). > > I'll drop this patch from x86-next. Overall, I think a cpu_has_sep based > solution would be preferable. > > ~Andrew
diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c index 9e69bf2..b0ef1c2 100644 --- a/xen/arch/x86/acpi/suspend.c +++ b/xen/arch/x86/acpi/suspend.c @@ -27,7 +27,8 @@ void save_rest_processor_state(void) rdmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base); rdmsrl(MSR_CSTAR, saved_cstar); rdmsrl(MSR_LSTAR, saved_lstar); - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) ) + if ( boot_cpu_data.x86_vendor & + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | X86_VENDOR_SHANGHAI) ) { rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp); rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip); @@ -51,7 +52,8 @@ void restore_rest_processor_state(void) wrgsbase(saved_gs_base); wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base); - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) ) + if ( boot_cpu_data.x86_vendor & + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | X86_VENDOR_SHANGHAI) ) { /* Recover sysenter MSRs */ wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp); diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c index 44af765..6506d6a 100644 --- a/xen/arch/x86/x86_64/traps.c +++ b/xen/arch/x86/x86_64/traps.c @@ -334,7 +334,8 @@ void subarch_percpu_traps_init(void) (unsigned long)lstar_enter); stub_va += offset; - if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) ) + if ( boot_cpu_data.x86_vendor & + (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR | X86_VENDOR_SHANGHAI) ) { /* SYSENTER entry. */ wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom);
When executing SYSEXIT or SYSENTRY in Zhaoxin CPU, CPU needs to save or restore a set of MSRs. Signed-off-by: FionaLi-oc <fionali-oc@zhaoxin.com> --- xen/arch/x86/acpi/suspend.c | 6 ++++-- xen/arch/x86/x86_64/traps.c | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-)