From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH RFC 28/31] xen/x86: Context switch all levelling state in context_switch() Date: Fri, 22 Jan 2016 02:52:10 -0700 Message-ID: <56A209DA02000078000C9F06@prv-mh.provo.novell.com> References: <1450301073-28191-1-git-send-email-andrew.cooper3@citrix.com> <1450301073-28191-29-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1450301073-28191-29-git-send-email-andrew.cooper3@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 16.12.15 at 22:24, wrote: > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -300,6 +300,9 @@ static void __init noinline amd_init_levelling(void) > cpumask_defaults._6c &= (~0ULL << 32); > cpumask_defaults._6c |= ecx; > } > + > + if (levelling_caps) > + ctxt_switch_levelling = amd_ctxt_switch_levelling; > } Indentation. > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -86,6 +86,13 @@ static const struct cpu_dev default_cpu = { > }; > static const struct cpu_dev *this_cpu = &default_cpu; > > +void default_ctxt_switch_levelling(const struct domain *nextd) static > +{ > + /* Nop */ > +} > +void (*ctxt_switch_levelling)(const struct domain *nextd) __read_mostly = > + default_ctxt_switch_levelling; While current and past gcc may accept (and honor) this placement of the __read_mostly annotation, I think it is wrong from a strict language syntax pov. Imo it instead ought to be void (*__read_mostly ctxt_switch_levelling)(const struct domain *nextd) = Also - indentation again. > @@ -145,6 +145,13 @@ void intel_ctxt_switch_levelling(const struct domain *nextd) > struct cpumasks *these_masks = &this_cpu(cpumasks); > const struct cpumasks *masks = &cpumask_defaults; > > + if (cpu_has_cpuid_faulting) { > + set_cpuid_faulting(nextd && is_pv_domain(nextd) && > + !is_control_domain(nextd) && > + !is_hardware_domain(nextd)); > + return; > + } Considering that you don't even probe the masking MSRs, this seems inconsistent with your "always level the entire system" choice. As said I'm opposed to that (i.e. the code here is fine), but at the very least things ought to be consistent. Jan